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[] = {
