Re: [PATCH v2 4/6] iio: amplifiers: ad8366: add device tree support

[David Lechner <dlechner@baylibre.com>]

On 1/26/26 7:51 AM, Rodrigo Alencar via B4 Relay wrote:
> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Add device-tree support by dropping the enum ID in favor of extended
> chip info table, containing:
> - gain_step, indicating with sign the start of the code range;
> - num_channels, to indicate the number IIO channels;
> - pack_code() function to describe how SPI buffer is populated;
> 
> With this, switch cases on the device type were dropped:
> - probe() function adjusted accordingly;
> - Simplified read_raw() and write_raw() callbacks;
> - mutex_lock()/mutex_unlock() replaced for guard(mutex)() to allow
>   moving to early returns;
> 

...

> +static size_t simple_pack_code(struct ad8366_state *st)

This name is a bit generic. I would call it e.g. hmc792_pack_code()
so that it clearly belongs to this driver.

Or drop this function and make the logic elsewhere:

	if (info->pack_code)
		return info->pack_code(st);

	st->data[0] = st->ch[0];
	return 1;

Then the info struct definitions would be less verbose.

> +{
> +	st->data[0] = st->ch[0];
> +	return 1;
> +}
> +

...

>  static int ad8366_probe(struct spi_device *spi)
>  {
>  	struct device *dev = &spi->dev;
> @@ -264,35 +221,22 @@ static int ad8366_probe(struct spi_device *spi)
>  		return ret;
>  
>  	st->spi = spi;
> -	st->type = spi_get_device_id(spi)->driver_data;
> +	st->info = spi_get_device_match_data(spi);
> +	if (!st->info)
> +		return dev_err_probe(dev, -EINVAL, "Invalid device info\n");
>  
> -	switch (st->type) {
> -	case ID_AD8366:
> -		indio_dev->channels = ad8366_channels;
> -		indio_dev->num_channels = ARRAY_SIZE(ad8366_channels);
> -		break;
> -	case ID_ADA4961:
> -	case ID_ADL5240:
> -	case ID_HMC792:
> -	case ID_HMC1119:
> -		st->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> -		if (IS_ERR(st->reset_gpio))
> -			return dev_err_probe(dev, PTR_ERR(st->reset_gpio),
> -					     "Failed to get reset GPIO\n");

As a precursor cleanup, st->reset_gpio can be removed and turned into a
local variable. It isn't used anywhere else.

Also, this could use a comment to explain that previously the driver
specifically had the reset gpio for some chips that don't actually
have a reset pin. It could have been wired up to the power/enable pin
instead, so some users might be relying on this to turn the chip on
rather than reset it.

> +	st->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(st->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->reset_gpio),
> +				     "Failed to get reset GPIO\n");
>  
> -		indio_dev->channels = ada4961_channels;
> -		indio_dev->num_channels = ARRAY_SIZE(ada4961_channels);
> -		break;
> -	default:
> -		return dev_err_probe(dev, -EINVAL, "Invalid device ID\n");
> -	}
> -
> -	st->info = &ad8366_infos[st->type];
>  	indio_dev->name = spi_get_device_id(spi)->name;
>  	indio_dev->info = &ad8366_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ad8366_channels;
> +	indio_dev->num_channels = st->info->num_channels;
>  
> -	ret = ad8366_write(indio_dev, 0, 0);
> +	ret = ad8366_write_code(st);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "failed to write initial gain\n");
>