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

[Nuno Sá]

Hi David,

Couple of minor things...

On Thu, 2023-10-12 at 15:45 -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 velocity must be read
> from the config register.
> 
> The cases of hardwired position or velocity modes is not supported as
> there would be no way to configure the device.
> 
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> ---
>  drivers/iio/resolver/ad2s1210.c | 193 +++++++++++++++++++++++++++-----
>  1 file changed, 162 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/iio/resolver/ad2s1210.c b/drivers/iio/resolver/ad2s1210.c
> index 1bd1b950e7cc..e6d3f31d529f 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];
> @@ -437,6 +443,40 @@ static void ad2s1210_push_events(struct iio_dev
> *indio_dev,
>  	st->prev_fault_flags = flags;
>  }
>  
> +/**
> + * Reads position or velocity from the config registers.
> + *
> + * This is used when the mode gpios are not available.
> + *
> + * Must be called with the lock held.
> + *
> + * @param st The device state.
> + * @param val Pointer to hold the value read.
> + * @param msb_reg The register address of the MSB register.
> + * @param lsb_reg The register address of the LSB register.
> + * @return 0 on success, negative error code otherwise.
> + */
> +static int ad2s1210_read_val_from_config(struct ad2s1210_state *st, __be16
> *val,
> +					 u8 msb_reg, u8 lsb_reg)
> +{
> +	unsigned int reg_val;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, msb_reg, &reg_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	((u8 *)val)[0] = reg_val;
> +
> +	ret = regmap_read(st->regmap, lsb_reg, &reg_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	((u8 *)val)[1] = reg_val;

These casts are not that nice... Is sparse even ok with this without __force?
I didn't looked at the datasheet so I have no idea but is regmap_bulk_read() an
option? It would simplify things.

> +
> +	return 0;
> +}

...

> 
>  	ad2s1210_push_events(indio_dev, st->sample.fault, pf->timestamp);
> @@ -1299,9 +1397,33 @@ 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, "position") == 0)
> +			st->fixed_mode = MOD_POS;
> +		else if (strcmp(str_val, "velocity") == 0)
> +			st->fixed_mode = MOD_VEL;
> +		else if (strcmp(str_val, "config") == 0)
> +			st->fixed_mode = MOD_CONFIG;
> +		else
> +			return dev_err_probe(dev, -EINVAL,
> +				"invalid adi,fixed-mode property value:
> %s\n",
> +				str_val);
> +
> +		if (st->fixed_mode != MOD_CONFIG)
> +			return dev_err_probe(dev, -EINVAL,
> +				"only adi,fixed-mode=\"config\" is
> supported\n");

Why not?

if (strcmp(str_val, "config"))
	return dev_err_probe();

st->fixed_mode = MOD_CONFIG;

Am I missing something obvious?

- Nuno Sá