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

[Andy Shevchenko]

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_8_NAME                  "ad4130-8"
> >
> > What the meaning of -8 ? Is it number of channels? Or is it part of
> > the official model (part number)? Can we see, btw, Datasheet: tag with
> > a corresponding link in the commit message?
>
> That's just the name specified in the datasheet. I honestly don't have
> much of an idea about why it is like that. Also, I already put the
> datasheet in the .yaml documentation.

That's cool!

> Do you really also want it
> in each commit message too?

No, just in this one.

...

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

...

> >> +#define AD4130_FREQ_FACTOR             1000000000ull
> >> +#define AD4130_DB3_FACTOR              1000
> >
> > Ditto.
>
> AD4130_DB3_FACTOR is unit-less. In the datasheet, the relation between
> sampling frequency and 3db frequency is represented as a 0.xyz value,
> hence why the db3_div values and 1000 factor.

But does the above mean MILLI or KILO? Similar for the FREQ factor.

...

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

...

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

...

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

...

> >> +       if (setup_info->enabled_channels)
> >> +               return -EINVAL;
> >
> > -EBUSY?
> >
>
> Eh, I don't think so. It would be pretty impossible for the code to hit
> this if statement, taking into account the ad4130_find_slot() logic.
> I could as well not have it at all.

If it's a dead code, we do not want 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?).

...

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

...

> >> +       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)));

...

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

...

> >> +static int ad4130_parse_fw_children(struct iio_dev *indio_dev)
> >> +{
> >> +       struct ad4130_state *st = iio_priv(indio_dev);
> >> +       struct device *dev = &st->spi->dev;
> >> +       struct fwnode_handle *child;
> >> +       int ret;
> >> +
> >> +       indio_dev->channels = st->chans;
> >> +
> >> +       device_for_each_child_node(dev, child) {
> >> +               ret = ad4130_parse_fw_channel(indio_dev, child);
> >> +               if (ret)
> >> +                       break;
> >> +       }
> >
> >> +       fwnode_handle_put(child);
> >
> > There is no need to put fwnode if child is NULL. Moreover, the above
> > pattern might be percepted wrongly, i.e. one may think that
> > fwnode_handle_put() is a must after a loop.
> >
>
> fwnode_handle_put already checks if the child is NULL. Why do the same
> check twice?

Exactly my point. Why do you check it twice?

...

> > Can you explain why regmap locking is needed?

> Am I supposed to set .disable_locking = true since SPI has its own
> locking?

You tell me. I have no idea of what the locking schema is being used
in your code.

-- 
With Best Regards,
Andy Shevchenko