Re: [PATCH V3 3/4] thermal: qcom: Add support for multiple generations of devices

[Jishnu Prakash <quic_jprakash@xxxxxxxxxxx>]

Hi Dmitry,


On 11/29/2021 7:55 AM, Dmitry Baryshkov wrote:
> On 23/11/2021 08:57, Jishnu Prakash wrote:
> > Refactor code to support multiple generations of ADC_TM devices
> > by defining gen number, irq name and disable, configure and isr
> > APIs in the individual data structs.
> >
> > Signed-off-by: Jishnu Prakash <quic_jprakash@xxxxxxxxxxx>
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
>
> Minor question below.
>
> > ---
> >   drivers/thermal/qcom/qcom-spmi-adc-tm5.c | 76 
> > ++++++++++++++++++++------------
> >   1 file changed, 48 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/thermal/qcom/qcom-spmi-adc-tm5.c 
> > b/drivers/thermal/qcom/qcom-spmi-adc-tm5.c
> > index 824671c..fc8cd45 100644
> > --- a/drivers/thermal/qcom/qcom-spmi-adc-tm5.c
> > +++ b/drivers/thermal/qcom/qcom-spmi-adc-tm5.c
> > @@ -78,11 +78,10 @@ enum adc5_timer_select {
> >       ADC5_TIMER_SEL_NONE,
> >   };
> >   -struct adc_tm5_data {
> > -    const u32    full_scale_code_volt;
> > -    unsigned int    *decimation;
> > -    unsigned int    *hw_settle;
> > -    bool        is_hc;
> > +enum adc5_gen {
> > +    ADC_TM5,
> > +    ADC_TM_HC,
> > +    ADC_TM5_MAX
> >   };
> >     enum adc_tm5_cal_method {
> > @@ -92,6 +91,18 @@ enum adc_tm5_cal_method {
> >   };
> >     struct adc_tm5_chip;
> > +struct adc_tm5_channel;
> > +
> > +struct adc_tm5_data {
> > +    const u32 full_scale_code_volt;
> > +    unsigned int *decimation;
> > +    unsigned int *hw_settle;
> > +    int (*disable_channel)(struct adc_tm5_channel *channel);
> > +    int (*configure)(struct adc_tm5_channel *channel, int low, int 
> > high);
> > +    irqreturn_t (*isr)(int irq, void *data);
> > +    char *irq_name;
> > +    int gen;
> > +};
>
> Any reason for moving the adc_tm5_data definition? It is still prefix 
> with the adc_tmp5_channel declaration.


There's no strong reason, I just thought it would look better to keep 
all the major struct definitions together after the enum definitions.


> >     /**
> >    * struct adc_tm5_channel - ADC Thermal Monitoring channel data.
> > @@ -139,22 +150,6 @@ struct adc_tm5_chip {
> >       u16            base;
> >   };
> >   -static const struct adc_tm5_data adc_tm5_data_pmic = {
> > -    .full_scale_code_volt = 0x70e4,
> > -    .decimation = (unsigned int []) { 250, 420, 840 },
> > -    .hw_settle = (unsigned int []) { 15, 100, 200, 300, 400, 500, 
> > 600, 700,
> > -                     1000, 2000, 4000, 8000, 16000, 32000,
> > -                     64000, 128000 },
> > -};
> > -
> > -static const struct adc_tm5_data adc_tm_hc_data_pmic = {
> > -    .full_scale_code_volt = 0x70e4,
> > -    .decimation = (unsigned int []) { 256, 512, 1024 },
> > -    .hw_settle = (unsigned int []) { 0, 100, 200, 300, 400, 500, 
> > 600, 700,
> > -                     1000, 2000, 4000, 6000, 8000, 10000 },
> > -    .is_hc = true,
> > -};
> > -

Thanks,

Jishnu