| 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
| ||
|
Message-ID: <20250525115723.7b05b8ad@jic23-huawei>
Date: Sun, 25 May 2025 11:57:23 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-gpio@...r.kernel.org>, <linux-kernel@...r.kernel.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>, <linus.walleij@...aro.org>,
<brgl@...ev.pl>, <marcelo.schmitt1@...il.com>
Subject: Re: [PATCH v3 10/10] iio: adc: ad4170: Add support for weigh scale
and RTD sensors
On Tue, 13 May 2025 09:36:47 -0300
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.
>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
A few comments inline. In general this is in a good shape subject
to us figuring out the right dt-binding.
Jonathan
> +
> +static int ad4170_setup_bridge(struct ad4170_state *st,
> + struct fwnode_handle *child,
> + struct ad4170_setup *setup, u32 *exc_pins,
> + int num_exc_pins, int exc_cur, bool ac_excited)
> +{
> + unsigned long gpio_mask;
> + int ret;
> +
> + /*
> + * If a specific current is provided through
> + * adi,excitation-current-microamp, set excitation pins provided through
> + * adi,excitation-pins to excite the bridge circuit.
> + */
> + if (exc_cur > 0)
> + return ad4170_setup_current_src(st, child, setup, exc_pins,
> + num_exc_pins, exc_cur,
> + ac_excited);
> +
> + /*
> + * Else, use predefined ACX1, ACX1 negated, ACX2, ACX2 negated signals
> + * to AC excite the bridge. Those signals are output on GPIO2, GPIO0,
> + * GPIO3, and GPIO1, respectively. If only two pins are specified for AC
> + * excitation, use ACX1 and ACX2 (GPIO2 and GPIO3).
> + *
> + * Also, to avoid any short-circuit condition when more than one channel
> + * is enabled, set GPIO2 and GPIO0 high, and set GPIO1 and GPIO3 low to
> + * DC excite the bridge whenever a channel without AC excitation is
> + * selected. That is needed because GPIO pins are controlled by the next
> + * highest priority GPIO function when a channel doesn't enable AC
> + * excitation. See datasheet Figure 113 Weigh Scale (AC Excitation) for
> + * an example circuit diagram.
> + */
> + if (num_exc_pins == 2) {
> + setup->misc |= FIELD_PREP(AD4170_MISC_CHOP_ADC_MSK, 0x3);
> +
> + gpio_mask = AD4170_GPIO_MODE_GPIO3_MSK | AD4170_GPIO_MODE_GPIO2_MSK;
> + ret = regmap_update_bits(st->regmap, AD4170_GPIO_MODE_REG, gpio_mask,
> + AD4170_GPIO_MODE_GPIO_OUTPUT << (2 * 3) |
> + AD4170_GPIO_MODE_GPIO_OUTPUT << (2 * 2));
Can we find a way to document relevance of 2 and 3 here? Ideally
appropriate defines or similar. Feels like a FIELD_PREP is appropriate.
> + if (ret)
> + return ret;
> +
> + ret = regmap_set_bits(st->regmap,
> + AD4170_GPIO_OUTPUT_REG,
> + BIT(3) | BIT(2));
These are also cryptic. Probably some FIELD_PREP() here as well.
> + if (ret)
> + return ret;
> +
> + st->gpio_fn[3] |= AD4170_GPIO_OUTPUT;
> + st->gpio_fn[2] |= AD4170_GPIO_OUTPUT;
> + } else {
> + setup->misc |= FIELD_PREP(AD4170_MISC_CHOP_ADC_MSK, 0x2);
> +
> + gpio_mask = AD4170_GPIO_MODE_GPIO3_MSK | AD4170_GPIO_MODE_GPIO2_MSK |
> + AD4170_GPIO_MODE_GPIO1_MSK | AD4170_GPIO_MODE_GPIO0_MSK;
> + ret = regmap_update_bits(st->regmap, AD4170_GPIO_MODE_REG, gpio_mask,
> + AD4170_GPIO_MODE_GPIO_OUTPUT << (2 * 3) |
> + AD4170_GPIO_MODE_GPIO_OUTPUT << (2 * 2) |
> + AD4170_GPIO_MODE_GPIO_OUTPUT << (2 * 1) |
> + AD4170_GPIO_MODE_GPIO_OUTPUT << (2 * 0));
As above. FIELD_PREP()
> + if (ret)
> + return ret;
> +
> + ret = regmap_set_bits(st->regmap,
> + AD4170_GPIO_OUTPUT_REG,
> + BIT(3) | BIT(2) | BIT(1) | BIT(0));
> + if (ret)
> + return ret;
> +
> + st->gpio_fn[3] |= AD4170_GPIO_OUTPUT;
> + st->gpio_fn[2] |= AD4170_GPIO_OUTPUT;
> + st->gpio_fn[1] |= AD4170_GPIO_OUTPUT;
> + st->gpio_fn[0] |= AD4170_GPIO_OUTPUT;
> + }
> +
> + return 0;
> +}
> +
> +static int ad4170_parse_external_sensor(struct ad4170_state *st,
> + struct fwnode_handle *child,
> + struct ad4170_setup *setup,
> + struct iio_chan_spec *chan, u8 s_type)
> +{
> + unsigned int num_exc_pins, exc_cur, reg_val;
> + struct device *dev = &st->spi->dev;
> + u32 pins[2], exc_pins[4];
> + bool ac_excited, vbias;
> + unsigned int i;
> + int ret;
> +
> + ret = fwnode_property_read_u32_array(child, "diff-channels", pins,
> + ARRAY_SIZE(pins));
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to read sensor diff-channels\n");
> +
> + chan->differential = true;
> + chan->channel = pins[0];
> + chan->channel2 = pins[1];
> +
> + ac_excited = fwnode_property_read_bool(child, "adi,excitation-ac");
> +
> + num_exc_pins = fwnode_property_count_u32(child, "adi,excitation-pins");
> + if (num_exc_pins != 1 && num_exc_pins != 2 && num_exc_pins != 4)
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid number of excitation pins\n");
> +
> + ret = fwnode_property_read_u32_array(child, "adi,excitation-pins",
> + exc_pins, num_exc_pins);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to read adi,excitation-pins\n");
> +
> + ret = ad4170_validate_excitation_pins(st, exc_pins, num_exc_pins);
> + if (ret)
> + return ret;
> +
> + exc_cur = 0;
> + ret = fwnode_property_read_u32(child, "adi,excitation-current-microamp",
> + &exc_cur);
> + if (ret && s_type == AD4170_RTD_SENSOR)
> + return dev_err_probe(dev, ret,
> + "Failed to read adi,excitation-current-microamp\n");
> +
> + for (i = 0; i < ARRAY_SIZE(ad4170_iout_current_ua_tbl); i++)
> + if (ad4170_iout_current_ua_tbl[i] == exc_cur)
> + break;
> +
> + if (i == ARRAY_SIZE(ad4170_iout_current_ua_tbl))
> + return dev_err_probe(dev, ret,
> + "Invalid excitation current: %uuA\n",
> + exc_cur);
> +
> + /* Get the excitation current configuration value */
> + exc_cur = ret;
> +
> + if (s_type == AD4170_THERMOCOUPLE_SENSOR) {
> + vbias = fwnode_property_read_bool(child, "adi,vbias");
> + if (vbias) {
> + st->pins_fn[chan->channel2] |= AD4170_PIN_VBIAS;
> + reg_val = BIT(chan->channel2);
> + return regmap_write(st->regmap, AD4170_V_BIAS_REG,
> + reg_val);
> + }
> + }
> + if (s_type == AD4170_WEIGH_SCALE_SENSOR) {
> + ret = ad4170_setup_bridge(st, child, setup, exc_pins,
> + num_exc_pins, exc_cur, ac_excited);
No {} needed as both single statements.
> + } else {
> + ret = ad4170_setup_rtd(st, child, setup, exc_pins, num_exc_pins,
> + exc_cur, ac_excited);
> + }
> + return ret;
> +}
> +
> static int ad4170_parse_reference(struct ad4170_state *st,
> struct fwnode_handle *child,
> struct ad4170_setup *setup)
> @@ -1849,6 +2231,7 @@ static int ad4170_parse_channel_node(struct iio_dev *indio_dev,
> struct ad4170_state *st = iio_priv(indio_dev);
> struct device *dev = &st->spi->dev;
> struct ad4170_chan_info *chan_info;
> + u8 s_type = AD4170_ADC_SENSOR;
> struct ad4170_setup *setup;
> struct iio_chan_spec *chan;
> unsigned int ch_reg;
> @@ -1880,10 +2263,32 @@ static int ad4170_parse_channel_node(struct iio_dev *indio_dev,
> if (ret)
> return ret;
>
> - ret = ad4170_parse_adc_channel_type(dev, child, chan);
> - if (ret < 0)
> - return ret;
> + ret = fwnode_property_read_u8(child, "adi,sensor-type", &s_type);
use same pattern for handling defaults as I suggested in earlier patches.
s_type = ADI4170_ADC_SESNOR;
fwnode_property_read_u8(child, "adi,sensor-type", &s_type);
if (s_type > ...
> + if (!ret) {
> + if (s_type > AD4170_THERMOCOUPLE_SENSOR)
> + return dev_err_probe(dev, ret,
> + "Invalid adi,sensor-type: %u\n",
> + s_type);
> + }
> + switch (s_type) {
> + case AD4170_ADC_SENSOR:
> + ret = ad4170_parse_adc_channel_type(dev, child, chan);
> + if (ret)
> + return ret;
>
> + break;
> + case AD4170_WEIGH_SCALE_SENSOR:
> + case AD4170_THERMOCOUPLE_SENSOR:
> + case AD4170_RTD_SENSOR:
> + ret = ad4170_parse_external_sensor(st, child, setup, chan,
> + s_type);
> + if (ret)
> + return ret;
> +
> + break;
> + default:
> + return -EINVAL;
> + }
> bipolar = fwnode_property_read_bool(child, "bipolar");
> setup->afe |= FIELD_PREP(AD4170_AFE_BIPOLAR_MSK, bipolar);
> if (bipolar)
> @@ -2087,6 +2492,12 @@ static int ad4170_parse_firmware(struct iio_dev *indio_dev)
> for (i = 0; i < AD4170_NUM_ANALOG_PINS; i++)
> st->pins_fn[i] = AD4170_PIN_UNASIGNED;M
UNASSIGNED
(obviously I missed this earlier)
>
> + for (i = 0; i < AD4170_NUM_GPIO_PINS; i++)
> + st->gpio_fn[i] = AD4170_GPIO_UNASIGNED;
Same here.
> +
> + for (i = 0; i < AD4170_NUM_CURRENT_SRC; i++)
> + st->cur_src_pins[i] = AD4170_CURRENT_SRC_DISABLED;
> +
> /* On power on, device defaults to using SDO pin for data ready signal */
> st->int_pin_sel = AD4170_INT_PIN_SDO;
> ret = device_property_match_property_string(dev, "interrupt-names",
Powered by blists - more mailing lists