On Tue, May 20, 2025 at 10:25:03PM +0200, Lothar Rubusch wrote:
> > On Sun, May 18, 2025 at 11:13:18AM +0000, Lothar Rubusch wrote:
...
> > > +static int adxl313_set_act_inact_en(struct adxl313_data *data,
> > > + enum adxl313_activity_type type,
> > > + bool cmd_en)
> > > +{
> > > + unsigned int axis_ctrl = 0;
> > > + unsigned int threshold;
> > > + bool en;
> > > + int ret;
> > > +
> > > + if (type == ADXL313_ACTIVITY)
> > > + axis_ctrl = ADXL313_ACT_XYZ_EN;
> > > +
> > > + ret = regmap_update_bits(data->regmap,
> > > + ADXL313_REG_ACT_INACT_CTL,
> > > + axis_ctrl,
> > > + cmd_en ? 0xff : 0x00);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = regmap_read(data->regmap, adxl313_act_thresh_reg[type], &threshold);
> > > + if (ret)
> > > + return ret;
> >
> > > + en = false;
> >
> > Instead...
> >
> > > + if (type == ADXL313_ACTIVITY)
> > > + en = cmd_en && threshold;
> >
> > else
> > en = false;
> >
> > > + return regmap_update_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> > > + adxl313_act_int_reg[type],
> > > + en ? adxl313_act_int_reg[type] : 0);
> > > +}
>
> The above is a good example for the following. From time to time, I face
> the situation in a function where I'd like to end up with something like
>
> if (foo = A) {
> var = thenDoA();
> } else {
> var = thenDoB();
> }
> doSomething(var);
>
> In a first patch I'll introduce only the following and remark in the
> commit message, that this will be extended. Since smatch/sparse tool
> will complain, I'll need to fiddle around with initializations
> (becoming obsolete in the end), e.g. I'll end up with something like
> this in a first patch A:
>
> var = nonsense;
> if (foo = A) {
> var = thenDoA();
> }
> doSomething(var);
>
> This is the case for switch(type) case IIO_...MAG: as only type (for
> now). This is the case for this is_act_inact_enabled(),
> set_act_inact(), etc.
>
> I assume it's better to simplify each commit individually and don't
> leave the "churn" around which might make sense in combination with a
> follow patch? Is this a general approach I should follow?
I believe so.
> Or, can it be legitimate to just split an if/else and add if-clause in
> a patch A and the else clause in the other patch B, since both are
> probably actually not complex. Such that patch A for itself looks a
> bit odd, but will make sense together with patch B?
Yes, but just make sure the each of the patches (after being applied) give the
plausible result.
...
> > > +static int adxl313_read_event_config(struct iio_dev *indio_dev,
> > > + const struct iio_chan_spec *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir)
> > > +{
> > > + struct adxl313_data *data = iio_priv(indio_dev);
> >
> > > + bool int_en;
> >
> > Why? You return the int here... I would expect rather to see unsigned int...
> >
> > > + int ret;
> > > +
> > > + switch (type) {
> > > + case IIO_EV_TYPE_MAG:
> > > + switch (dir) {
> > > + case IIO_EV_DIR_RISING:
> > > + ret = adxl313_is_act_inact_en(data,
> > > + ADXL313_ACTIVITY,
> > > + &int_en);
> > > + if (ret)
> > > + return ret;
> > > + return int_en;
> >
> > ...or even simply
> >
> > return adx1313...(...);
> >
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
>
> This one here is interesting, to my understanding I followed here e.g.
> the approach of the ADXL380 which is supposed to be a quite recent
> driver [the _read/write_event_config() there.]
> Now, your remark made me think: I'm unsure, can I actually I implement
> the following approach here?
> - return >0 : true
=1, but yes. We have plenty of functions like this in the kernel.
> - return =0 : false
> - return <0 : error
>
> It seems to work (unsure about the error cases, though),
> but much cleaner and simpler! I'll send that in v2,
> pls let me know if I missunderstood you.
--
With Best Regards,
Andy Shevchenko
