Re: [PATCH v4 2/2] iio: adc: ad4130: add AD4130 driver

[Cosmin Tanislav]

On 6/8/22 18:59, Andy Shevchenko wrote:
> On Wed, Jun 8, 2022 at 12:19 PM Cosmin Tanislav <demonsingur@xxxxxxxxx> wrote:
> > AD4130-8 is an ultra-low power, high precision, measurement solution for
> > low bandwidth battery operated applications.
> >
> > The fully integrated AFE (Analog Front-End) includes a multiplexer for up
> > to 16 single-ended or 8 differential inputs, PGA (Programmable Gain
> > Amplifier), 24-bit Sigma-Delta ADC, on-chip reference and oscillator,
> > selectable filter options, smart sequencer, sensor biasing and excitation
> > options, diagnostics, and a FIFO buffer.
>
> I believe we may gain a few LoCs by slightly bending the rule of 80.
> Also see below.

I'll only go over the 80 columns limit if Jonathan agrees to it.

> > +       *size = ad4130_reg_size[reg];
> > +       if (!*size)
> > +               return -EINVAL;
>
> Is this check necessary?

Yes. I haven't described all registers in the table, and the registers
can be accessed by the user via the debugfs_reg_access() method.

> > +static void ad4130_gpio_set(struct gpio_chip *gc, unsigned int offset,
> > +                           int value)
> > +{
> > +       struct ad4130_state *st = gpiochip_get_data(gc);
> > +       unsigned int real_offset = st->gpio_offsets[offset];
>
> Can't you use valid_mask instead of this additional array? In such a
> case the real offset can be got by the number of the set bit, no?

I'm not sure what you mean by this? If valid_mask will prevent all
GPIOs equivalent to the bits not set in the mask from being exposed,
then yes, it could be useful. I wish I knew about it earlier since
it's already the second driver in which I use this approach.

> > +       for (i = 0; i < AD4130_MAX_SETUPS; i++) {
> > +               struct ad4130_slot_info *slot_info = &st->slots_info[i];
> > +
> > +               /* Immediately accept a matching setup info. */
>
> > +               if (!memcmp(target_setup_info, &slot_info->setup,
> > +                           sizeof(*target_setup_info))) {
>
> Instead, you may use crc32 and save it, the matching will be much faster.
>
> The example, where it's done for the same purposes (to compare later)
> https://elixir.bootlin.com/linux/latest/source/drivers/acpi/scan.c#L659

I think it's fine as it is. Most people won't use than many channels
anyway.

> > +                       *slot = i;
> > +                       return 0;
> > +               }
> > +
> > +               /* Ignore all setups which are used by enabled channels. */
> > +               if (slot_info->enabled_channels)
> > +                       continue;
> > +
> > +               /* Find the least used slot. */
>
> Have you considered to use
> https://elixir.bootlin.com/linux/latest/source/include/linux/list_lru.h
> ?

No. And I don't think I intend to.
> > +       const struct ad4130_filter_config *filter_config =
> > +               &ad4130_filter_configs[filter_mode];
>
> One line? Or even a helper, since you are using this more than once.

I don't think creating a helper would be helpful here. I can save like,
one character. Or you meant a helper that also declares the
filter_config variable? That would make the code even harder to read.

> > +       switch (ref_sel) {
> > +       case AD4130_REF_REFIN1:
> > +               ret = regulator_get_voltage(st->regulators[2].consumer);
> > +               break;
> > +       case AD4130_REF_REFIN2:
> > +               ret = regulator_get_voltage(st->regulators[3].consumer);
> > +               break;
> > +       case AD4130_REF_AVDD_AVSS:
> > +               ret = regulator_get_voltage(st->regulators[0].consumer);
> > +               break;
> > +       case AD4130_REF_REFOUT_AVSS:
> > +               ret = st->int_ref_uv;
> > +               break;
> > +       default:
> > +               ret = -EINVAL;
> > +               break;
> > +       }
>
> > +       if (ret < 0)
> > +               return dev_err_probe(dev, ret, "Cannot use reference %u\n",
> > +                                    ref_sel);
>
> Can it be moved to the caller where it would cleaner to use, I think?
> As a good side effect the all above will be shortened to just return directly.

I'm pretty sure I remember Jonathan suggested moving it inside the
function.

> > +       ret = ad4130_get_ref_voltage(st, setup_info->ref_sel);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       return 0;
>
> In all cases what does the positive return value mean?
> If there is no meaning, drop all these ' < 0' parts and esp. here use simply
>
> return ad4130_get_ref_voltage(...);

ad4130_get_ref_voltage() returns the voltage of the specified reference
via its return value.

The voltage would be positive, while an error code would be negative.
Same for ad4130_find_table_index() where < 0 is also used.

> > +               for (j = 0; j < AD4130_MAX_PGA; j++) {
> > +                       unsigned int pow = resolution + j - st->bipolar;
>
> > +                       unsigned int nv = div_u64((((u64)ret * NANO) >>
> > +                                                  pow), MILLI);
>
> It will be much better if you make it on one line. Moreover, it seems
> it's ivariamt to the loop, why it's inside the loop?

pow depends on j, nv depends on pow.