Re: [PATCH v2 11/17] iio: adc: axp20x_adc: Consolidate ADC raw read functions

[Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>]

On Tue,  7 Jun 2022 16:53:18 +0100
Aidan MacDonald <aidanmacdonald.0x0@xxxxxxxxx> wrote:

> Add an axp20x_id variant field to the axp_data struct and use it
> to consolidate the adc_raw functions, reducing code duplication.
> Variant IDs are chosen to match the OF compatible strings.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@xxxxxxxxx>

Hi Aidan,

I'm not a big fan of using variant IDs, rather than a description
of what is actually different between devices.  Long term, variant
IDs tend to scale (as we add more supported devices) much worse
than a flag describing the actual difference.

Here I would have a field in struct axp_data called something like
discharge_curr_res and set it to 12 or 13 as appropriate.

> ---
>  drivers/iio/adc/axp20x_adc.c | 83 +++++++++++++++---------------------
>  1 file changed, 34 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
> index 9d5b1de24908..0260433782d8 100644
> --- a/drivers/iio/adc/axp20x_adc.c
> +++ b/drivers/iio/adc/axp20x_adc.c
> @@ -71,6 +71,18 @@ struct axp20x_adc_iio {
>  	const struct axp_data	*data;
>  };
>  
> +struct axp_data {
> +	const struct iio_info		*iio_info;
> +	int				num_channels;
> +	struct iio_chan_spec const	*channels;
> +	unsigned long			adc_en1_mask;
> +	unsigned long			adc_en2_mask;
> +	int				(*adc_rate)(struct axp20x_adc_iio *info,
> +						    int rate);
> +	struct iio_map			*maps;
> +	enum axp20x_variants		axp20x_id;
> +};
> +
>  enum axp20x_adc_channel_v {
>  	AXP20X_ACIN_V = 0,
>  	AXP20X_VBUS_V,
> @@ -237,15 +249,24 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>  	struct axp20x_adc_iio *info = iio_priv(indio_dev);
>  	int ret, size;
>  
> -	/*
> -	 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
> -	 * stored on 12 bits, not 13 bits. Only discharging current is on 13
> -	 * bits.
> -	 */
> -	if (chan->type == IIO_CURRENT && chan->channel == AXP20X_BATT_DISCHRG_I)
> -		size = 13;
> -	else
> +	switch (info->data->axp20x_id) {
> +	case AXP202_ID:
> +	case AXP209_ID:
> +		/*
> +		 * N.B.:  Unlike the Chinese datasheets tell, the charging current is
> +		 * stored on 12 bits, not 13 bits. Only discharging current is on 13
> +		 * bits.
> +		 */
> +		if (chan->type == IIO_CURRENT && chan->channel == AXP20X_BATT_DISCHRG_I)

This line is getting a bit long, break it after the &&

> +			size = 13;
> +		else
> +			size = 12;
> +		break;
> +
> +	default:
>  		size = 12;
> +		break;
> +	}
>  
>  	ret = axp20x_read_variable_width(info->regmap, chan->address, size);
>  	if (ret < 0)
> @@ -255,34 +276,6 @@ static int axp20x_adc_raw(struct iio_dev *indio_dev,
>  	return IIO_VAL_INT;
>  }
>  
> -static int axp22x_adc_raw(struct iio_dev *indio_dev,
> -			  struct iio_chan_spec const *chan, int *val)
> -{
> -	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> -	int ret;
> -
> -	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
> -	if (ret < 0)
> -		return ret;
> -
> -	*val = ret;
> -	return IIO_VAL_INT;
> -}
> -
> -static int axp813_adc_raw(struct iio_dev *indio_dev,
> -			  struct iio_chan_spec const *chan, int *val)
> -{
> -	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> -	int ret;
> -
> -	ret = axp20x_read_variable_width(info->regmap, chan->address, 12);
> -	if (ret < 0)
> -		return ret;
> -
> -	*val = ret;
> -	return IIO_VAL_INT;
> -}
> -
>  static int axp20x_adc_scale_voltage(int channel, int *val, int *val2)
>  {
>  	switch (channel) {
> @@ -522,7 +515,7 @@ static int axp22x_read_raw(struct iio_dev *indio_dev,
>  		return axp22x_adc_scale(chan, val, val2);
>  
>  	case IIO_CHAN_INFO_RAW:
> -		return axp22x_adc_raw(indio_dev, chan, val);
> +		return axp20x_adc_raw(indio_dev, chan, val);
>  
>  	default:
>  		return -EINVAL;
> @@ -542,7 +535,7 @@ static int axp813_read_raw(struct iio_dev *indio_dev,
>  		return axp813_adc_scale(chan, val, val2);
>  
>  	case IIO_CHAN_INFO_RAW:
> -		return axp813_adc_raw(indio_dev, chan, val);
> +		return axp20x_adc_raw(indio_dev, chan, val);
>  
>  	default:
>  		return -EINVAL;
> @@ -620,17 +613,6 @@ static int axp813_adc_rate(struct axp20x_adc_iio *info, int rate)
>  				 AXP813_ADC_RATE_HZ(rate));
>  }
>  
> -struct axp_data {
> -	const struct iio_info		*iio_info;
> -	int				num_channels;
> -	struct iio_chan_spec const	*channels;
> -	unsigned long			adc_en1_mask;
> -	int				(*adc_rate)(struct axp20x_adc_iio *info,
> -						    int rate);
> -	bool				adc_en2;
> -	struct iio_map			*maps;
> -};
> -
>  static const struct axp_data axp20x_data = {
>  	.iio_info = &axp20x_adc_iio_info,
>  	.num_channels = ARRAY_SIZE(axp20x_adc_channels),
> @@ -639,6 +621,7 @@ static const struct axp_data axp20x_data = {
>  	.adc_rate = axp20x_adc_rate,
>  	.adc_en2 = true,
>  	.maps = axp20x_maps,
> +	.axp20x_id = AXP209_ID,
>  };
>  
>  static const struct axp_data axp22x_data = {
> @@ -649,6 +632,7 @@ static const struct axp_data axp22x_data = {
>  	.adc_rate = axp22x_adc_rate,
>  	.adc_en2 = false,
>  	.maps = axp22x_maps,
> +	.axp20x_id = AXP221_ID,
>  };
>  
>  static const struct axp_data axp813_data = {
> @@ -659,6 +643,7 @@ static const struct axp_data axp813_data = {
>  	.adc_rate = axp813_adc_rate,
>  	.adc_en2 = false,
>  	.maps = axp22x_maps,
> +	.axp20x_id = AXP813_ID,
>  };
>  
>  static const struct of_device_id axp20x_adc_of_match[] = {