Re: [PATCH v2 4/9] iio: adc: ad7606: Move software functions into common file

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 10 Dec 2024 10:46:44 +0000
Guillaume Stols <gstols@baylibre.com> wrote:

> Since the register are always the same, whatever bus is used, moving the
> software functions into the main file avoids the code to be duplicated
> in both SPI and parallel version of the driver.
> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
Hi Guillaume,

Patch content is fine, but I'd ideally like you to take the opportunity to
tidy up some of the really inconsistent indentation in the code you are moving.

> int ad7616_write_scale_sw(struct iio_dev *indio_dev, int ch, int val)
>  {
>  	struct ad7606_state *st = iio_priv(indio_dev);
> +	unsigned int ch_addr, mode, ch_index;
>  
> -	st->sw_mode_en = st->bops->sw_mode_config &&
> -			 device_property_present(st->dev, "adi,sw-mode");
> -	if (!st->sw_mode_en)
> -		return 0;
> +	/*
> +	 * Ad7616 has 16 channels divided in group A and group B.
> +	 * The range of channels from A are stored in registers with address 4
> +	 * while channels from B are stored in register with address 6.
> +	 * The last bit from channels determines if it is from group A or B
> +	 * because the order of channels in iio is 0A, 0B, 1A, 1B...
> +	 */
> +	ch_index = ch >> 1;
> +
> +	ch_addr = AD7616_RANGE_CH_ADDR(ch_index);
> +
> +	if ((ch & 0x1) == 0) /* channel A */
> +		ch_addr += AD7616_RANGE_CH_A_ADDR_OFF;
> +	else	/* channel B */
> +		ch_addr += AD7616_RANGE_CH_B_ADDR_OFF;
> +
> +	/* 0b01 for 2.5v, 0b10 for 5v and 0b11 for 10v */
> +	mode = AD7616_RANGE_CH_MODE(ch_index, ((val + 1) & 0b11));
>  
> -	indio_dev->info = &ad7606_info_sw_mode;
> +	return ad7606_write_mask(st, ch_addr, AD7616_RANGE_CH_MSK(ch_index),
> +				     mode);

Another odd indent to fix.

> +}
> +
> +static int ad7616_write_os_sw(struct iio_dev *indio_dev, int val)
> +{
> +	struct ad7606_state *st = iio_priv(indio_dev);
> +
> +	return ad7606_write_mask(st, AD7616_CONFIGURATION_REGISTER,
> +				     AD7616_OS_MASK, val << 2);

Trivial: Odd indentation choice.

> +}
> +
> +static int ad7606_write_scale_sw(struct iio_dev *indio_dev, int ch, int val)
> +{
> +	struct ad7606_state *st = iio_priv(indio_dev);
> +
> +	return ad7606_write_mask(st,
> +				 AD7606_RANGE_CH_ADDR(ch),

In general maybe aim for more consistent indentation choice. I'd pull this time up
on the line above.

> +				 AD7606_RANGE_CH_MSK(ch),
> +				 AD7606_RANGE_CH_MODE(ch, val));
> +}
> +
> +static int ad7606_write_os_sw(struct iio_dev *indio_dev, int val)
> +{
> +	struct ad7606_state *st = iio_priv(indio_dev);
> +
> +	return st->bops->reg_write(st, AD7606_OS_MODE, val);
> +}
> +
> +static int ad7616_sw_mode_setup(struct iio_dev *indio_dev)
> +{
> +	struct ad7606_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	/*
> +	 * Scale can be configured individually for each channel
> +	 * in software mode.
> +	 */
> +
> +	st->write_scale = ad7616_write_scale_sw;
> +	st->write_os = &ad7616_write_os_sw;
> +
> +	ret = st->bops->sw_mode_config(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	/* Activate Burst mode and SEQEN MODE */
> +	return ad7606_write_mask(st,
> +			      AD7616_CONFIGURATION_REGISTER,

Whilst moving the code, feel free to tidy up the indent for inconsistent
cases.  Align this lot just after the opening bracket etc.

> +			      AD7616_BURST_MODE | AD7616_SEQEN_MODE,
> +			      AD7616_BURST_MODE | AD7616_SEQEN_MODE);
> +}

Thanks,

Jonathan