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.
