On Thu, 09 Jun 2022 00:13:47 +0100
Aidan MacDonald <aidanmacdonald.0x0@xxxxxxxxx> wrote:
> Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> writes:
>
> > 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.
> >
>
> I agree with your point in general, but here it seems impossible to get
> away from variant IDs because the channel numbering depends on it and we
> use the channel number to decide what number of bits to use.
Ah. I'd missed that detail. Perhaps add a comment to remind us that's
the case in future.
> The code
> I'm replacing is just disguising the variant IDs by giving every variant
> its own set of functions.
>
> To me it seemed clearer to describe the channel properties and then use
> one read_raw function for all variants, but when I did that it turned
> out not to make any difference in size for x86. Probably because tables
> encode a lot of redundant information compared to switches. It also
> relied on a hack to associate extra info with an iio_chan_spec so it
> wasn't much of an improvement, in the end.
>
> So it's a question of using a variant ID explicitly or having separate
> functions for each device. Combining the functions with an explicit ID
> saves 752 bytes on x86, once the axp192 is added, and I don't think it
> is any harder to understand than the separate functions. And it's still
> possible to use a separate function when needed.
>
> Nonetheless, if you'd prefer to stick with separate functions I'm fine
> with that.
This may well be a case of doing what you have here for now, but revisit
in future if we end up with more cases of this turning up in the function.
It may well become too complex and need the separate functions again.
Jonathan
>
> Regards,
> Aidan
>
> >> ---
> >> 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[] = {
>
