Re: [PATCH v2] iio: resolver: ad2s1210: add support for adi,fixed-mode

[Jonathan Cameron]

On Tue, 17 Oct 2023 10:26:43 +0200
Nuno Sá <noname.nuno@xxxxxxxxx> wrote:

> On Mon, 2023-10-16 at 08:54 -0500, David Lechner wrote:
> > It is possible to use the AD2S1210 with hardwired mode pins (A0 and A1).
> > According to the devicetree bindings, in this case the adi,fixed-mode
> > property will specify which of the 3 possible modes the mode pins are
> > hardwired for and the gpio-modes property is not allowed.
> > 
> > This adds support for the case where the mode pins are hardwired for
> > config mode. In this configuration, the position and value must be read
> > from the config register.
> > 
> > The case of hardwired position or velocity mode is not supported as
> > there would be no way to configure the device.
> > 
> > Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> > ---
> >   
> 
> LGTM
> 
> Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> 
Applied to the togreg branch of iio.git and pushed out as testing for 0-day
to see what we missed.

This probably won't make 6.7. Still no problem with getting it queued up for
6.8.

Thanks,

Jonathan

> > v2 changes:
> > * Use regmap_bulk_read() instead of new local function.
> > * Simplify adi,fixed-mode property error checking.
> > 
> >  drivers/iio/resolver/ad2s1210.c | 150 +++++++++++++++++++++++++-------
> >  1 file changed, 119 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/iio/resolver/ad2s1210.c b/drivers/iio/resolver/ad2s1210.c
> > index 1bd1b950e7cc..7f688bfe2172 100644
> > --- a/drivers/iio/resolver/ad2s1210.c
> > +++ b/drivers/iio/resolver/ad2s1210.c
> > @@ -141,7 +141,7 @@ struct ad2s1210_state {
> >  	struct spi_device *sdev;
> >  	/** GPIO pin connected to SAMPLE line. */
> >  	struct gpio_desc *sample_gpio;
> > -	/** GPIO pins connected to A0 and A1 lines. */
> > +	/** GPIO pins connected to A0 and A1 lines (optional). */
> >  	struct gpio_descs *mode_gpios;
> >  	/** Used to access config registers. */
> >  	struct regmap *regmap;
> > @@ -149,6 +149,8 @@ struct ad2s1210_state {
> >  	unsigned long clkin_hz;
> >  	/** Available raw hysteresis values based on resolution. */
> >  	int hysteresis_available[2];
> > +	/* adi,fixed-mode property - only valid when mode_gpios == NULL. */
> > +	enum ad2s1210_mode fixed_mode;
> >  	/** The selected resolution */
> >  	enum ad2s1210_resolution resolution;
> >  	/** Copy of fault register from the previous read. */
> > @@ -175,6 +177,9 @@ static int ad2s1210_set_mode(struct ad2s1210_state *st,
> > enum ad2s1210_mode mode)
> >  	struct gpio_descs *gpios = st->mode_gpios;
> >  	DECLARE_BITMAP(bitmap, 2);
> >  
> > +	if (!gpios)
> > +		return mode == st->fixed_mode ? 0 : -EOPNOTSUPP;
> > +
> >  	bitmap[0] = mode;
> >  
> >  	return gpiod_set_array_value(gpios->ndescs, gpios->desc, gpios->info,
> > @@ -276,7 +281,8 @@ static int ad2s1210_regmap_reg_read(void *context,
> > unsigned int reg,
> >  	 * parity error. The fault register is read-only and the D7 bit means
> >  	 * something else there.
> >  	 */
> > -	if (reg != AD2S1210_REG_FAULT && st->rx[1] & AD2S1210_ADDRESS_DATA)
> > +	if ((reg > AD2S1210_REG_VELOCITY_LSB && reg != AD2S1210_REG_FAULT)
> > +	     && st->rx[1] & AD2S1210_ADDRESS_DATA)
> >  		return -EBADMSG;
> >  
> >  	*val = st->rx[1];
> > @@ -450,21 +456,53 @@ static int ad2s1210_single_conversion(struct iio_dev
> > *indio_dev,
> >  	ad2s1210_toggle_sample_line(st);
> >  	timestamp = iio_get_time_ns(indio_dev);
> >  
> > -	switch (chan->type) {
> > -	case IIO_ANGL:
> > -		ret = ad2s1210_set_mode(st, MOD_POS);
> > -		break;
> > -	case IIO_ANGL_VEL:
> > -		ret = ad2s1210_set_mode(st, MOD_VEL);
> > -		break;
> > -	default:
> > -		return -EINVAL;
> > +	if (st->fixed_mode == MOD_CONFIG) {
> > +		unsigned int reg_val;
> > +
> > +		switch (chan->type) {
> > +		case IIO_ANGL:
> > +			ret = regmap_bulk_read(st->regmap,
> > +					       AD2S1210_REG_POSITION_MSB,
> > +					       &st->sample.raw, 2);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			break;
> > +		case IIO_ANGL_VEL:
> > +			ret = regmap_bulk_read(st->regmap,
> > +					       AD2S1210_REG_VELOCITY_MSB,
> > +					       &st->sample.raw, 2);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			break;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +
> > +		ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, &reg_val);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		st->sample.fault = reg_val;
> > +	} else {
> > +		switch (chan->type) {
> > +		case IIO_ANGL:
> > +			ret = ad2s1210_set_mode(st, MOD_POS);
> > +			break;
> > +		case IIO_ANGL_VEL:
> > +			ret = ad2s1210_set_mode(st, MOD_VEL);
> > +			break;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = spi_read(st->sdev, &st->sample, 3);
> > +		if (ret < 0)
> > +			return ret;
> >  	}
> > -	if (ret < 0)
> > -		return ret;
> > -	ret = spi_read(st->sdev, &st->sample, 3);
> > -	if (ret < 0)
> > -		return ret;
> >  
> >  	switch (chan->type) {
> >  	case IIO_ANGL:
> > @@ -1252,27 +1290,53 @@ static irqreturn_t ad2s1210_trigger_handler(int irq,
> > void *p)
> >  	ad2s1210_toggle_sample_line(st);
> >  
> >  	if (test_bit(0, indio_dev->active_scan_mask)) {
> > -		ret = ad2s1210_set_mode(st, MOD_POS);
> > -		if (ret < 0)
> > -			goto error_ret;
> > -
> > -		ret = spi_read(st->sdev, &st->sample, 3);
> > -		if (ret < 0)
> > -			goto error_ret;
> > +		if (st->fixed_mode == MOD_CONFIG) {
> > +			ret = regmap_bulk_read(st->regmap,
> > +					       AD2S1210_REG_POSITION_MSB,
> > +					       &st->sample.raw, 2);
> > +			if (ret < 0)
> > +				goto error_ret;
> > +		} else {
> > +			ret = ad2s1210_set_mode(st, MOD_POS);
> > +			if (ret < 0)
> > +				goto error_ret;
> > +
> > +			ret = spi_read(st->sdev, &st->sample, 3);
> > +			if (ret < 0)
> > +				goto error_ret;
> > +		}
> >  
> >  		memcpy(&st->scan.chan[chan++], &st->sample.raw, 2);
> >  	}
> >  
> >  	if (test_bit(1, indio_dev->active_scan_mask)) {
> > -		ret = ad2s1210_set_mode(st, MOD_VEL);
> > -		if (ret < 0)
> > -			goto error_ret;
> > +		if (st->fixed_mode == MOD_CONFIG) {
> > +			ret = regmap_bulk_read(st->regmap,
> > +					       AD2S1210_REG_VELOCITY_MSB,
> > +					       &st->sample.raw, 2);
> > +			if (ret < 0)
> > +				goto error_ret;
> > +		} else {
> > +			ret = ad2s1210_set_mode(st, MOD_VEL);
> > +			if (ret < 0)
> > +				goto error_ret;
> > +
> > +			ret = spi_read(st->sdev, &st->sample, 3);
> > +			if (ret < 0)
> > +				goto error_ret;
> > +		}
> >  
> > -		ret = spi_read(st->sdev, &st->sample, 3);
> > +		memcpy(&st->scan.chan[chan++], &st->sample.raw, 2);
> > +	}
> > +
> > +	if (st->fixed_mode == MOD_CONFIG) {
> > +		unsigned int reg_val;
> > +
> > +		ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, &reg_val);
> >  		if (ret < 0)
> > -			goto error_ret;
> > +			return ret;
> >  
> > -		memcpy(&st->scan.chan[chan++], &st->sample.raw, 2);
> > +		st->sample.fault = reg_val;
> >  	}
> >  
> >  	ad2s1210_push_events(indio_dev, st->sample.fault, pf->timestamp);
> > @@ -1299,9 +1363,24 @@ static const struct iio_info ad2s1210_info = {
> >  static int ad2s1210_setup_properties(struct ad2s1210_state *st)
> >  {
> >  	struct device *dev = &st->sdev->dev;
> > +	const char *str_val;
> >  	u32 val;
> >  	int ret;
> >  
> > +	ret = device_property_read_string(dev, "adi,fixed-mode", &str_val);
> > +	if (ret == -EINVAL)
> > +		st->fixed_mode = -1;
> > +	else if (ret < 0)
> > +		return dev_err_probe(dev, ret,
> > +			"failed to read adi,fixed-mode property\n");
> > +	else {
> > +		if (strcmp(str_val, "config"))
> > +			return dev_err_probe(dev, -EINVAL,
> > +				"only adi,fixed-mode=\"config\" is
> > supported\n");
> > +
> > +		st->fixed_mode = MOD_CONFIG;
> > +	}
> > +
> >  	ret = device_property_read_u32(dev, "assigned-resolution-bits",
> > &val);
> >  	if (ret < 0)
> >  		return dev_err_probe(dev, ret,
> > @@ -1357,12 +1436,21 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state
> > *st)
> >  				     "failed to request sample GPIO\n");
> >  
> >  	/* both pins high means that we start in config mode */
> > -	st->mode_gpios = devm_gpiod_get_array(dev, "mode", GPIOD_OUT_HIGH);
> > +	st->mode_gpios = devm_gpiod_get_array_optional(dev, "mode",
> > +						       GPIOD_OUT_HIGH);
> >  	if (IS_ERR(st->mode_gpios))
> >  		return dev_err_probe(dev, PTR_ERR(st->mode_gpios),
> >  				     "failed to request mode GPIOs\n");
> >  
> > -	if (st->mode_gpios->ndescs != 2)
> > +	if (!st->mode_gpios && st->fixed_mode == -1)
> > +		return dev_err_probe(dev, -EINVAL,
> > +			"must specify either adi,fixed-mode or mode-
> > gpios\n");
> > +
> > +	if (st->mode_gpios && st->fixed_mode != -1)
> > +		return dev_err_probe(dev, -EINVAL,
> > +			"must specify only one of adi,fixed-mode or mode-
> > gpios\n");
> > +
> > +	if (st->mode_gpios && st->mode_gpios->ndescs != 2)
> >  		return dev_err_probe(dev, -EINVAL,
> >  				     "requires exactly 2 mode-gpios\n");
> >    
>