On Thu, 22 Aug 2024 14:39:55 -0500
David Lechner [off-list ref] wrote:

On 8/22/24 7:45 AM, Esteban Blanc wrote:

quoted

This adds a new driver for the Analog Devices INC. AD4030-24 ADC.

The driver implements basic support for the AD4030-24 1 channel
differential ADC with hardware gain and offset control.

Signed-off-by: Esteban Blanc <eblanc@baylibre.com>

A couple of comments on comments inline mainly to point out
one 'lazy' alternative that is very common for the IIO_VAL_INT
write case.

quoted

+static int ad4030_single_conversion(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan, int *val)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = ad4030_set_mode(indio_dev, BIT(chan->channel));
+	if (ret)
+		return ret;
+
+	ret = ad4030_exit_config_mode(st);
+	if (ret)
+		goto out_exit_config_mode_error;  

Looks like we could just return ret here.

quoted

+
+	ret = ad4030_conversion(st, chan);
+	if (ret)
+		goto out_error;
+
+	if (chan->channel % 2)
+		*val = st->rx_data.buffered[chan->channel / 2].common;
+	else
+		*val = st->rx_data.buffered[chan->channel / 2].val;
+
+out_error:
+	ad4030_enter_config_mode(st);
+
+out_exit_config_mode_error:
+
+	if (ret)
+		return ret;
+
+	return IIO_VAL_INT;  

This can be moved before out_error:, then we can just have
return ret here and leave out the if.

I'd assume not quite because we need to go back into config mode
even on error.

I'd be tempted to have separate error block and just duplicate
that one call.

quoted

+}

quoted

+static int ad4030_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int val,
+			    int val2, long info)
+{
+	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+		switch (info) {
+		case IIO_CHAN_INFO_CALIBSCALE:
+			return ad4030_set_chan_gain(indio_dev, chan->channel,
+						    val, val2);
+
+		case IIO_CHAN_INFO_CALIBBIAS:
+			return ad4030_set_chan_offset(indio_dev, chan->channel,
+						      val);  

Need to add .write_raw_get_fmt to struct iio_info below to set
IIO_CHAN_INFO_CALIBBIAS to IIO_VAL_INT. Othwerwise, the defualt
IIO_VAL_INT_PLUS_MICRO is used and val2 would have considered
for handling negative values.

Lazy approach is 
	if (val2 != 0)
		return -EINVAL;
We do this a fair bit in drivers to avoid a very minimal write_raw_fmt
callback.

But 'right way' is to tell the core that it's an int.

quoted

+
+		default:
+			return -EINVAL;
+		}
+	}
+
+	unreachable();
+}

quoted

+	indio_dev->name = st->chip->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &ad4030_iio_info;
+	indio_dev->channels = st->chip->channels;
+	indio_dev->available_scan_masks = st->chip->available_masks;
+	indio_dev->masklength = st->chip->available_masks_len;  

indio_dev->masklengh is marked as [INTERN] so should not be set by drivers.

It will now give a compile error if you try this on linux-next or
the iio.git/togreg tree.