Re: [PATCH v1 3/3] iio: adc: ad4130: add AD4130 driver

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Apr 14, 2022 at 5:53 PM Cosmin Tanislav <demonsingur@xxxxxxxxx> wrote:
> On 4/14/22 16:45, Andy Shevchenko wrote:
> > On Thu, Apr 14, 2022 at 2:06 PM Cosmin Tanislav <demonsingur@xxxxxxxxx> wrote:
> >> On 4/13/22 18:41, Andy Shevchenko wrote:
> >>> On Wed, Apr 13, 2022 at 1:41 PM Cosmin Tanislav <demonsingur@xxxxxxxxx> wrote:

...

> >>>> +#define AD4130_RESET_CLK_COUNT         64
> >>>> +#define AD4130_RESET_BUF_SIZE          (AD4130_RESET_CLK_COUNT / 8)
> >>>
> >>> To be more precise shouldn't the above need to have DIV_ROUND_UP() ?
> >>
> >> Does it look like 64 / 8 needs any rounding?
> >
> > Currently no, but if someone puts 63 there or 65, what would be the outcome?
> > OTOH, you may add a static assert to guarantee that CLK_COUNT is multiple of 8.
> >
>
> No one will. 64 is defined in the datasheet and will never change. I'm
> not gonna do anything about it. Actually, I can do something about it.
> Remove AD4130_RESET_CLK_COUNT and only define AD4130_RESET_BUF_SIZE as
> 8.

It would be better, indeed. And to whom it may concern you may add a
comment explaining how 8 is derived.

...

> >>>> +       if (reg >= ARRAY_SIZE(ad4130_reg_size))
> >>>> +               return -EINVAL;
> >>>
> >>> When this condition is true?
> >>
> >> When the user tries reading a register from direct_reg_access
> >> that hasn't had its size defined.
> >
> > But how is it possible? Is the reg parameter taken directly from the user?
> >
>
> Users can write whatever they want to direct_reg_access. Unless I add
> max_register to the regmap_config, the register that the user selects
> will just be passed to our reg_read and reg_write callbacks.
>
> Then it will be checked against the register size table.

Thanks, I got it.

...

> >>>> + out:
> >>>
> >>> out_unlock: ?
> >>> Ditto for similar cases.
> >>
> >> There's a single label in the function, and there's a mutex being
> >> taken, and, logically, the mutex must be released on the exit path.
> >> It's clear what the label is for to me.
> >
> > Wasn't clear to me until I went to the end of each of them (who
> > guarantees that's the case for all of them?).
>
> Let's hope other people looking at that code will be able to figure out
> what that label does then.

OK. Let the maintainer decide.

...

> >>>> +               *val = st->bipolar ? -(1 << (chan->scan_type.realbits - 1)) : 0;
> >>>
> >>> Hmm... It seems like specific way to have a sign_extended, or actually
> >>> reduced) mask.
> >>> Can you rewrite it with the (potential)UB-free approach?
> >>>
> >>> (Note, that if realbits == 32, this will have a lot of fun in
> >>> accordance with C standard.)
> >>
> >> Can you elaborate on this? The purpose of this statement is to shift the
> >> results so that, when bipolar configuration is enabled, the raw value is
> >> offset with 1 << (realbits - 1) towards negative.
> >>
> >> For the 24bit chips, 0x800000 becomes 0x000000.
> >>
> >> Maybe you misread it as left shift on a negative number? The number
> >> is turned negative only after the shift...
> >
> > 1 << 31 is UB in accordance with the C standard.
> >
> > And the magic above seems to me the opposite to what sign_extend()
> > does. Maybe even providing a general function for sign_comact() or so
> > (you name it) would be also nice to have.
>
> I'm not trying to comact (I guess you meant compact) the sign of any
> value. Please try to understand what is written in there. It's not
> magic. If the chip is 24bit, and it's set up as bipolar, the raw value
> must be offset by -0x800000, to account for 0x800000 being the
> zero-scale value. For 16 bits, it's 0x8000.

Yes, you shift zero offset to some value. I see that in several
drivers, so I think it would be nice to have a macro for that
somewhere in math.h. But it can be done later on.

...

> >>>> +       ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
> >>>> +                                AD4130_WATERMARK_MASK,
> >>>> +                                FIELD_PREP(AD4130_WATERMARK_MASK,
> >>>> +                                           ad4130_watermark_reg_val(eff)));
> >>>
> >>> Temporary variable for mask?
> >>
> >> You mean for value?
> >
> >        mask = AD4130_WATERMARK_MASK;
> >
> >        ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
> >                                 mask, FIELD_PREP(mask,
> > ad4130_watermark_reg_val(eff)));
>
> Please bother reading the macro definition next-time. The mask argument
> to FIELD_PREP must be a compile-time constant.

Yes, it needs to be u32_encode_bits(), but in any case it's up to you,
it's not anyhow a critical change.

...

> >>>> +       if (ret <= 0)
> >>>
> >>> = 0 ?! Can you elaborate, please, this case taking into account below?
> >>>
> >>
> >> I guess I just did it because voltage = 0 doesn't make sense and would
> >> make scale be 0.0.
> >
> > Again, what's the meaning of having it in the conjunction with
> > dev_err_probe() call?
> >
> >>>> +               return dev_err_probe(dev, ret, "Cannot use reference %u\n",
> >>>> +                                    ref_sel);
> >
> > It's confusing. I believe you need two different messages if you want
> > to handle the 0 case.
>
> Why would I? The chip can't possibly use regulators with a voltage of 0,
> right? Or dummy regulators, since these return negative. I think it's
> fine as it is.

Confusing part is what dev_err_probe() prints here when ret == 0.


--
With Best Regards,
Andy Shevchenko