Re: [PATCH 2/6] iio: adc: ad4030: add driver for ad4030-24

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 22 Aug 2024 14:39:55 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 8/22/24 7:45 AM, Esteban Blanc wrote:
> > 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.

> > +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.
> 
> > +
> > +	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.
> 
> > +}

> > +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.

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

> 
> > +	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.