Date:   Wed, 8 Jun 2022 23:11:51 +0300
From:   Cosmin Tanislav <demonsingur@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Cosmin Tanislav <cosmin.tanislav@...log.com>
Subject: Re: [PATCH v4 2/2] iio: adc: ad4130: add AD4130 driver



On 6/8/22 18:59, Andy Shevchenko wrote:
> On Wed, Jun 8, 2022 at 12:19 PM Cosmin Tanislav <demonsingur@...il.com> 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.

Powered by blists - more mailing lists