linux-kernel - Re: [PATCH v2 7/7] iio: adc: ad4170: Add support for weigh scale and RTD sensors

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VerW=GnsomWLqUyK6AWU+dVDPxhAVmafuwy4cDpbyPVUA@mail.gmail.com>
Date: Wed, 30 Apr 2025 01:26:41 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, jic23@...nel.org, lars@...afoo.de, 
	Michael.Hennerich@...log.com, dlechner@...libre.com, nuno.sa@...log.com, 
	andy@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	marcelo.schmitt1@...il.com
Subject: Re: [PATCH v2 7/7] iio: adc: ad4170: Add support for weigh scale and
 RTD sensors

On Mon, Apr 28, 2025 at 3:30 PM Marcelo Schmitt
<marcelo.schmitt@...log.com> wrote:
>
> The AD4170 design has features to aid interfacing with weigh scale and RTD
> sensors that are expected to be setup with external circuitry for proper
> sensor operation. A key characteristic of those sensors is that the circuit
> they are in must be excited with a pair of signals. The external circuit
> can be excited either by voltage supply or by AD4170 excitation signals.
> The sensor can then be read through a different pair of lines that are
> connected to AD4170 ADC.
>
> Configure AD4170 to handle external circuit sensors.

...

> +static const unsigned int ad4170_iout_current_ua_tbl[] = {
> +       0, 10, 50, 100, 250, 500, 1000, 1500

Leave trailing comma.

> +};

...

> +       if (st->pins_fn[ain_n] & AD4170_PIN_VBIAS) {
> +               *ain_voltage = (st->vrefs_uv[AD4170_AVDD_SUP]
> +                               - st->vrefs_uv[AD4170_AVSS_SUP]) / 2;

Don't wrap like this, keep the logical split, and looking at this I
would just put it either on a single line, or use a temporary variable
for the expression in parentheses.

> +               return 0;
> +       }

...

> +static int ad4170_validate_excitation_pins(struct ad4170_state *st,
> +                                          u32 *exc_pins, int num_exc_pins)
> +{
> +       struct device *dev = &st->spi->dev;
> +       int ret, i;

Should 'i' be signed?

> +       for (i = 0; i < num_exc_pins; i++) {
> +               unsigned int pin = exc_pins[i];
> +
> +               ret = ad4170_find_table_index(ad4170_iout_pin_tbl, pin);
> +               if (ret < 0)
> +                       return dev_err_probe(dev, ret,
> +                                            "Invalid excitation pin: %u\n",
> +                                            pin);
> +
> +               if (pin <= AD4170_MAX_ANALOG_PINS) {
> +                       if (st->pins_fn[pin] != AD4170_PIN_UNASIGNED)
> +                               return dev_err_probe(dev, -EINVAL,
> +                                                    "Pin %u already used with fn %u\n",
> +                                                    pin, st->pins_fn[pin]);
> +
> +                       st->pins_fn[pin] |= AD4170_PIN_CURRENT_OUT;
> +               } else {
> +                       unsigned int gpio = pin - AD4170_CURRENT_SRC_I_OUT_PIN_GPIO0;
> +
> +                       if (st->gpio_fn[gpio] != AD4170_GPIO_UNASIGNED)
> +                               return dev_err_probe(dev, -EINVAL,
> +                                                    "GPIO %u already used with fn %u\n",
> +                                                    gpio, st->gpio_fn[gpio]);
> +
> +                       st->gpio_fn[gpio] |= AD4170_GPIO_AC_EXCITATION;
> +               }
> +       }
> +       return 0;
> +}

...

Also consider inverting the conditional in ad4170_setup_bridge() to
drop indentation level in a lot of LoCs.

...

> +       switch (s_type) {
> +       case AD4170_ADC_SENSOR:
> +               ret = ad4170_parse_adc_channel_type(dev, child, chan);
> +               if (ret < 0)

Why ' < 0'?

> +                       return ret;
> +
> +               break;
> +       case AD4170_WEIGH_SCALE_SENSOR:
> +               fallthrough;
> +       case AD4170_THERMOCOUPLE_SENSOR:
> +               fallthrough;
> +       case AD4170_RTD_SENSOR:
> +               ret = ad4170_parse_external_sensor(st, child, setup, chan,
> +                                                  s_type);
> +               if (ret < 0)

Ditto.

> +                       return ret;
>
> +               break;
> +       default:
> +               return -EINVAL;
> +       }

-- 
With Best Regards,
Andy Shevchenko