[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <37ac71be-78d6-a266-045b-18164d715e57@gmail.com>
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