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

[Cosmin Tanislav]

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.

> > > > +       int                             samp_freq_avail_len;
> > > > +       int                             samp_freq_avail[3][2];
>
> > > > +       int                             db3_freq_avail_len;
> > > > +       int                             db3_freq_avail[3][2];
> > >
> > > These 3:s can be defined?
> > I could define IIO_AVAIL_RANGE_LEN and IIO_AVAIL_SINGLE_LEN and then
> > define another IIO_AVAIL_LEN that is the max between the two.
> > But that's just over-complicating it, really.
>
> I was talking only about 3:s (out array). IIRC I saw 3 hard coded in
> the driver, but not sure if its meaning is the same. Might be still
> good to define
Actually I just checked, and it's not even needed. The framework
always expects 3 elements for IIO_AVAIL_RANGE. I'll keep those two
3s as they are.

> > > > +       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.

> > > > +       regmap_update_bits(st->regmap, AD4130_REG_IO_CONTROL, mask,
> > > > +                          value ? mask : 0);
> > >
> > > One line?
> > >
> > > No error check?
> >
> > I actually can't think of a scenario where this would fail. It doesn't
> > if the chip is not even connected.
>
> Why to check errors in many other cases then? Be consistent one way or
> the other.

Yeah, right. I didn't add any error checking because the callback can't
handle errors, so all I can do is print a message to dmesg.

> > > > + 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.

> > > > +               *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.

> > > > +       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.

> > > > +       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.