Re: [PATCH V2 3/3] thermal: qcom: add support for PMIC5 Gen2 ADCTM

[Jishnu Prakash]

Hi Dmitry,

On 10/27/2021 8:04 AM, Dmitry Baryshkov wrote:
> On 26/10/2021 19:04, Jishnu Prakash wrote:
> > Add support for PMIC5 Gen2 ADC_TM, used on PMIC7 chips. It is a
> > close counterpart of PMIC7 ADC and has the same functionality as
> > PMIC5 ADC_TM, for threshold monitoring and interrupt generation.
> > It is present on PMK8350 alone, like PMIC7 ADC and can be used
> > to monitor up to 8 ADC channels, from any of the PMIC7 PMICs
> > having ADC on a target, through PBS(Programmable Boot Sequence).
> >
> > Signed-off-by: Jishnu Prakash <quic_jprakash@xxxxxxxxxxx>
> > ---
> >   drivers/thermal/qcom/qcom-spmi-adc-tm5.c | 418 
> > +++++++++++++++++++++++++++++--
> >   1 file changed, 399 insertions(+), 19 deletions(-)
> >
> >       unsigned int        hw_settle_time;
> > +    unsigned int        decimation;    /* For Gen2 ADC_TM */
> > +    unsigned int        avg_samples;    /* For Gen2 ADC_TM */
> > +    bool            high_thr_en;        /* For Gen2 ADC_TM */
> > +    bool            low_thr_en;        /* For Gen2 ADC_TM */
> > +    bool            meas_en;        /* For Gen2 ADC_TM */
> >       struct iio_channel    *iio;
> >       struct adc_tm5_chip    *chip;
> >       struct thermal_zone_device *tzd;
> > @@ -123,9 +201,12 @@ struct adc_tm5_channel {
> >    * @channels: array of ADC TM channel data.
> >    * @nchannels: amount of channels defined/allocated
> >    * @decimation: sampling rate supported for the channel.
> > + *      Applies to all channels, used only on Gen1 ADC_TM.
> >    * @avg_samples: ability to provide single result from the ADC
> > - *    that is an average of multiple measurements.
> > + *      that is an average of multiple measurements. Applies to all
> > + *      channels, used only on Gen1 ADC_TM.
> >    * @base: base address of TM registers.
> > + * @adc_mutex_lock: ADC_TM mutex lock.
>
> Please specify that it is used only for gen2 and that it keeps written 
> and cached channel setup in sync (feel free to correct this 
> description according to your understanding, I might be wrong here).


I'll add to the description in the next post.


> >    */
> >   struct adc_tm5_chip {
> >       struct regmap        *regmap;
> > @@ -136,14 +217,15 @@ struct adc_tm5_chip {
> >       unsigned int        decimation;
> >       unsigned int        avg_samples;
> >       u16            base;
> > +    struct mutex        adc_mutex_lock;
> >   };
> >   -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 },
> > +enum adc_tm_gen2_time_select {
> > +    MEAS_INT_50MS = 0,
> > +    MEAS_INT_100MS,
> > +    MEAS_INT_1S,
> > +    MEAS_INT_SET,
> > +    MEAS_INT_NONE,
> >   };
>
> Move this enum to the top, closer to the rest of definitions.


Will move it in the next post


> >     static int adc_tm5_read(struct adc_tm5_chip *adc_tm, u16 offset, 
> > u8 *data, int len)
> > @@ -210,6 +292,61 @@ static irqreturn_t adc_tm5_isr(int irq, void *data)
> >       return IRQ_HANDLED;
> >   }
> >   +static irqreturn_t adc_tm5_gen2_isr(int irq, void *data)
> > +{
> >           tzd = devm_thermal_zone_of_sensor_register(adc_tm->dev,
> >                                  adc_tm->channels[i].channel,
> >                                  &adc_tm->channels[i],
> > -                               &adc_tm5_ops);
> > +                               &adc_tm5_thermal_ops);
> >           if (IS_ERR(tzd)) {
> >               if (PTR_ERR(tzd) == -ENODEV) {
> >                   dev_warn(adc_tm->dev, "thermal sensor on channel %d 
> > is not used\n",
> > @@ -395,6 +710,11 @@ static int adc_tm5_init(struct adc_tm5_chip *chip)
> >           }
> >       }
> >   +    if (chip-->data->gen == ADC_TM5_GEN2) {
> > +        mutex_init(&chip->adc_mutex_lock);
> > +        return ret;
> > +    }
> > +
>
> Just init the mutex always, there is no need to be so picky in the 
> init code.
Will fix it in the next post.
> >       buf[0] = chip->decimation;
> >       buf[1] = chip->avg_samples | ADC_TM5_FAST_AVG_EN;
> >       buf[2] = ADC_TM5_TIMER1;
> > @@ -415,7 +735,7 @@ static int adc_tm5_get_dt_channel_data(struct 
> > adc_tm5_chip *adc_tm,
> >                          struct device_node *node)
> >       { }
> >   };
> >   MODULE_DEVICE_TABLE(of, adc_tm5_match_table);
Thanks,
Jishnu