Re: [PATCH v3 10/10] iio: adc: ti-ads1015: Switch to read_avail

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Sun, 20 Mar 2022 19:14:28 +0100
Marek Vasut <marex@denx.de> wrote:

> Replace sysfs attributes with read_avail() callback. This also permits
> removal of ads1115_info, since the scale attribute tables are now part
> of chip data.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Daniel Baluta <daniel.baluta@nxp.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> V3: New patch
> ---
>  drivers/iio/adc/ti-ads1015.c | 102 +++++++++++++++++++----------------
>  1 file changed, 57 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 66431d1445d9b..b1257f65d7431 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -81,6 +81,9 @@ struct ads1015_chip_data {
>  	int				num_channels;
>  	const struct iio_info		*info;
>  	const unsigned int		*data_rate;
Should probably change this to signed.

> +	const unsigned int		data_rate_len;
> +	const unsigned int		*scale;

Why unsigned int given we use it as an array of signed ints?

> +	const unsigned int		scale_len;
>  	bool				has_comparator;
>  };
>  
> @@ -108,10 +111,18 @@ static const unsigned int ads1115_data_rate[] = {
>   * Translation from PGA bits to full-scale positive and negative input voltage
>   * range in mV
>   */
> -static int ads1015_fullscale_range[] = {
> +static const int ads1015_fullscale_range[] = {
Technically unrelated but good fix and not hurting patch readability significantly
so perhaps not worth a separate patch.

>  	6144, 4096, 2048, 1024, 512, 256, 256, 256
>  };
>  
> +static const int ads1015_scale[] = {	/* 12bit ADC */
> +	256, 11, 512, 11, 1024, 11, 2048, 11, 4096, 11, 6144, 11
I wonder if it's worth either using a 2D array and casting
a dimension away, or perhaps just formatting these pair wise
so we can see what is going on more obviously?

I don't feel strongly about this so up to you.

> +};
> +
> +static const int ads1115_scale[] = {	/* 16bit ADC */
> +	256, 15, 512, 15, 1024, 15, 2048, 15, 4096, 15, 6144, 15
> +};