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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 28 Mar 2022 02:31:32 +0200
Marek Vasut <marex@denx.de> wrote:

> On 3/27/22 17:18, Jonathan Cameron wrote:
> > On Tue, 22 Mar 2022 23:02:10 +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 <andy@kernel.org>
> >> Cc: Daniel Baluta <daniel.baluta@nxp.com>
> >> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> > 
> > Sorry, I didn't catch your question on v3 about why I was advocating
> > signed.
> > 
> > You are passing pointers to those arrays as signed in the
> > read_avail.
> > 
> > Obviously you can 'get away with it' because the values are small
> > positive numbers and hence in 2's complement the data representation
> > will be the same.  Not pretty though so my inclination would
> > be to keep them signed everywhere.
> > 
> > If you are fine with that change I can change it whilst applying if
> > nothing else comes up in review.  
> 
> I'm fine with it, although I did switch them all to unsigned int in this 
> V4 (unless I'm missing something still).

You switched to unsigned int, but...

+static int ads1015_read_avail(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals, int *type, int *length,
+			      long mask)
+{
+	struct ads1015_data *data = iio_priv(indio_dev);
+
+	if (chan->type != IIO_VOLTAGE)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*type = IIO_VAL_FRACTIONAL_LOG2;
+		*vals =  data->chip->scale;

This then uses them as signed integers.  Which as mentioned is
technically not a bug, but that's only because the numbers
are small..  So better to go signed throughout.


+		*length = data->chip->scale_len;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*type = IIO_VAL_INT;
+		*vals = data->chip->data_rate;
+		*length = data->chip->data_rate_len;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+