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

Andy Shevchenko
2025-04-29T22:27:19Z

On Mon, Apr 28, 2025 at 3:30 PM Marcelo Schmitt
<marcelo.schmitt@analog.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