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

[Marcelo Schmitt]

...
> > +static int ad4170_setup_rtd(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)
> > +{
> > +	int current_src, ret, i;
> > +
> > +	for (i = 0; i < num_exc_pins; i++) {
> > +		unsigned int pin = exc_pins[i];
> > +
> > +		current_src |= FIELD_PREP(AD4170_CURRENT_SRC_I_OUT_PIN_MSK,
> > pin);
> > +		current_src |= FIELD_PREP(AD4170_CURRENT_SRC_I_OUT_VAL_MSK,
> > exc_cur);
> > +
> > +		ret = regmap_write(st->regmap16, AD4170_CURRENT_SRC_REG(i),
> > +				   current_src);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	if (ac_excited)
> > +		setup->misc |= FIELD_PREP(AD4170_MISC_CHOP_IEXC_MSK,
> > +					  num_exc_pins == 2 ? 0x2 : 0x3);
> > +
> > +	return 0;
> > +}
> 
> In the above I do not see any explicit GPIO configuration which makes me wonder
> if having the RTD is mutual exclusive with having GPIOs?

For RTD sensors, it's recommended to use a different excitation mechanism which
is configured in CURRENT_SOURCE and MISCELLANEOUS registers. The current source
for RTD sensor excitation could come from a GPIO, but only implemented support
for outputting the current from an analog pin. Will handle the case of using
GPIOs in v2.

> 
> > +
> > +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)
> > +{
> > +	int current_src, ret, i;
> > +
> > +	if (!ac_excited)
> > +		return 0;
> 
> Same as above, if !ac_excited, can't we use the GPIOs? because
> ad4170_validate_excitation_pins() just unconditionally sets
> AD4170_GPIO_AC_EXCITATION. Or maybe this DT property is only adding
> complexity... See below

I see, maybe AD4170_GPIO_EXCITATION would better describe the function. 

...
> > +
> > +			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;
> 
> Not sure if you gain much with having the funcs OR'ed like this... If I'm not
> missing nothing it's only about logging in ad4170_validate_excitation_pins()?
> It's up to you but I would consider using bitmaps (unsigned long) for this and
> just test the bits.
> 
Ah, yes, a bitmap is what was trying to do but misimplemented it.
There are 4 possible functions for AD4170 GPIOs.
(1) Power-down switches (currently not supported);
(2) External sensor excitation;
(3) GPIO;
(4) CHANNEL_TO_GPIO (not eager to support that).

If a GPIO is used for external circuit excitation, we don't want to export it as
a GPIO. If the GPIO were set a power-down switch we would also not expose it.

...
> > +
> > +	ac_excited = fwnode_property_read_bool(child, "adi,ac-excited");
> > +
> > +	num_exc_pins = fwnode_property_count_u32(child, "adi,excitation-
> > pins");
> > +	if (num_exc_pins != 2 && num_exc_pins != 4)
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "Invalid number of excitation pins\n");
> 
> Can't we assume that a valid num_exc_pins property means ac_excited = true?
> Because that looks to be the logic in ad4170_validate_excitation_pins().

Unfortunately, no. The user may want and set AD4170 to DC excite the external
circuit. They may want to use the same set of excitation pins, but not enable
channel chop or output current chop (leading to DC excitation). Well, we may
choose to not support the DC case in the driver I guess, but since this is
already fairly complicated, why not going an step further to support it fully?

> 
> > +
> > +	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");
> > +
...
> > @@ -2081,6 +2412,10 @@ static int ad4170_parse_firmware(struct iio_dev
> > *indio_dev)
> >  
> >  	/* Only create a GPIO chip if flagged for it */
> >  	if (device_property_read_bool(&st->spi->dev, "gpio-controller")) {
> > +		for (i = 0; i < AD4170_NUM_GPIO_PINS; i++)
> > +			if (st->gpio_fn[i] != AD4170_GPIO_UNASIGNED)
> > +				return 0;
> 
> I think you could improve this... You're taking an all or nothing approach.
> IIUC, we can have cases where only two GPIOs are in use which means we could use
> the other 2? There the gpiochio init_valid_mask() call that you could
> potentially use.

Isn't gpiochio init_valid_mask() only to distinguish between GPIOs that can
(or cannot) be used as interrupts? Not sure AD4170 GPIOs can be used for
interrupts at all (think they can't) so didn't implement init_valid_mask().

Thanks,
Marcelo