Re: [PATCH 08/14] iio: adc: rzg2l_adc: Prepare for the addition of RZ/G3S support

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 4 Dec 2024 10:40:58 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Jonathan,
> 
> On Tue, Dec 3, 2024 at 9:09 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Tue,  3 Dec 2024 13:13:08 +0200
> > Claudiu <claudiu.beznea@tuxon.dev> wrote:  
> > > The ADC IP available on the RZ/G3S differs slightly from the one found on
> > > the RZ/G2L. The identified differences are as follows:
> > > - different number of channels (one being used for temperature conversion);
> > >   consequently, various registers differ
> > > - different default sampling periods
> > > - the RZ/G3S variant lacks the ADVIC register.
> > >
> > > To accommodate these differences, the rzg2l_adc driver has been updated by
> > > introducing the struct rzg2l_adc_hw_params, which encapsulates the
> > > hardware-specific differences between the IP variants. A pointer to an
> > > object of type struct rzg2l_adc_hw_params is embedded in
> > > struct rzg2l_adc_data.
> > >
> > > Additionally, the completion member of struct rzg2l_adc_data was relocated
> > > to avoid potential padding, if any.
> > >
> > > The code has been adjusted to utilize hardware-specific parameters stored
> > > in the new structure instead of relying on plain macros.
> > >
> > > The check of chan->channel in rzg2l_adc_read_raw() function, against the
> > > driver specific mask was removed as the subsystem should have already
> > > been done this before reaching the rzg2l_adc_read_raw() function.
> > >
> > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > > ---
> > >  drivers/iio/adc/rzg2l_adc.c | 92 ++++++++++++++++++++++++++-----------
> > >  1 file changed, 64 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> > > index fda8b42ded81..aff41152ebf8 100644
> > > --- a/drivers/iio/adc/rzg2l_adc.c
> > > +++ b/drivers/iio/adc/rzg2l_adc.c
> > > @@ -32,20 +32,15 @@
> > >  #define RZG2L_ADM1_MS                        BIT(2)
> > >  #define RZG2L_ADM1_BS                        BIT(4)
> > >  #define RZG2L_ADM1_EGA_MASK          GENMASK(13, 12)
> > > -#define RZG2L_ADM2_CHSEL_MASK                GENMASK(7, 0)
> > >  #define RZG2L_ADM3_ADIL_MASK         GENMASK(31, 24)
> > >  #define RZG2L_ADM3_ADCMP_MASK                GENMASK(23, 16)
> > > -#define RZG2L_ADM3_ADCMP_E           FIELD_PREP(RZG2L_ADM3_ADCMP_MASK, 0xe)
> > > -#define RZG2L_ADM3_ADSMP_MASK                GENMASK(15, 0)
> > >
> > >  #define RZG2L_ADINT                  0x20
> > > -#define RZG2L_ADINT_INTEN_MASK               GENMASK(7, 0)
> > >  #define RZG2L_ADINT_CSEEN            BIT(16)
> > >  #define RZG2L_ADINT_INTS             BIT(31)
> > >
> > >  #define RZG2L_ADSTS                  0x24
> > >  #define RZG2L_ADSTS_CSEST            BIT(16)
> > > -#define RZG2L_ADSTS_INTST_MASK               GENMASK(7, 0)
> > >
> > >  #define RZG2L_ADIVC                  0x28
> > >  #define RZG2L_ADIVC_DIVADC_MASK              GENMASK(8, 0)
> > > @@ -56,12 +51,26 @@
> > >  #define RZG2L_ADCR(n)                        (0x30 + ((n) * 0x4))
> > >  #define RZG2L_ADCR_AD_MASK           GENMASK(11, 0)
> > >
> > > -#define RZG2L_ADSMP_DEFAULT_SAMPLING 0x578
> > > -
> > > -#define RZG2L_ADC_MAX_CHANNELS               8
> > > -#define RZG2L_ADC_CHN_MASK           0x7
> > >  #define RZG2L_ADC_TIMEOUT            usecs_to_jiffies(1 * 4)
> > >
> > > +/**
> > > + * struct rzg2l_adc_hw_params - ADC hardware specific parameters
> > > + * @default_adsmp: default ADC sampling period (see ADM3 register)
> > > + * @adsmp_mask: ADC sampling period mask (see ADM3 register)
> > > + * @adint_inten_mask: conversion end interrupt mask (see ADINT register)
> > > + * @default_adcmp: default ADC cmp (see ADM3 register)
> > > + * @num_channels: number of supported channels
> > > + * @adivc: specifies if ADVIC register is available
> > > + */
> > > +struct rzg2l_adc_hw_params {
> > > +     u16 default_adsmp;
> > > +     u16 adsmp_mask;
> > > +     u16 adint_inten_mask;
> > > +     u8 default_adcmp;
> > > +     u8 num_channels;
> > > +     bool adivc;
> > > +};
> > > +
> > >  struct rzg2l_adc_data {
> > >       const struct iio_chan_spec *channels;
> > >       u8 num_channels;
> > > @@ -71,10 +80,11 @@ struct rzg2l_adc {
> > >       void __iomem *base;
> > >       struct reset_control *presetn;
> > >       struct reset_control *adrstn;
> > > -     struct completion completion;
> > >       const struct rzg2l_adc_data *data;
> > > +     const struct rzg2l_adc_hw_params *hw_params;
> > > +     u16 *last_val;
> > > +     struct completion completion;
> > >       struct mutex lock;
> > > -     u16 last_val[RZG2L_ADC_MAX_CHANNELS];  
> >
> > Just make this big enough for the max device.  Chances are it will make little or
> > no difference to this allocation and nice to avoid the dynamic part.
> >
> > Feel free to add a runtime check to make sure this is big enough to avoid any
> > future problems with forgetting to update it.  
> 
> Flexible array member and the new __counted_by() attribute?
Messy as it's embedded in iio_dev via a rather round about route.
It happens to be at the end of that structure but that's an implementation detail.

So in this particular case I'd go with no for flexible array and __counted_by.
It is very unlikely the difference in size will actually result in a bigger allocation
as it's a substantial allocation however big this is.

Jonathan

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>