by Jishnu Prakash

[permalink] [raw]

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

Hi Jonathan,

On 10/27/2021 11:35 PM, Jonathan Cameron wrote:
> On Tue, 26 Oct 2021 21:34:35 +0530
> Jishnu Prakash <[email protected]> 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 <[email protected]>
> Drive be review as I was looking at the other bits of the series.
> Personally I'd prefer to see this as 2 patches, 1st one does
> the refactoring and 2nd adds the gen2 specific parts.
>
> Jonathan
>
>
>>
>> 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;
> Indenting in here is inconsistent.
> Personally I'd just stop bothering with the lining everything up.
Will fix this in the next post.
>
>> + int (*disable_channel)(struct adc_tm5_channel *channel);
>> + int (*configure)(struct adc_tm5_channel *channel, int low,
>> + int high);
> Why the line break? It's under 80 chars without it.
Will fix this in the next post.
>
>> + irqreturn_t (*isr)(int irq, void *data);
>> + char *irq_name;
>> + int gen;
>> +};
>>
>> /**
>> * struct adc_tm5_channel - ADC Thermal Monitoring channel data.
>> @@ -100,6 +167,12 @@ struct adc_tm5_chip;
>> * @prescale: channel scaling performed on the input signal.
>> * @hw_settle_time: the time between AMUX being configured and the
>> * start of conversion.
>> + * @decimation: sampling rate supported for the channel.
>> + * @avg_samples: ability to provide single result from the ADC
>> + * that is an average of multiple measurements.
>> + * @high_thr_en: channel upper voltage threshold enable state.
>> + * @low_thr_en: channel lower voltage threshold enable state.
>> + * @meas_en: recurring measurement enable state
>> * @iio: IIO channel instance used by this channel.
>> * @chip: ADC TM chip instance.
>> * @tzd: thermal zone device used by this channel.
>> @@ -110,6 +183,11 @@ struct adc_tm5_channel {
>> enum adc_tm5_cal_method cal_method;
>> unsigned int prescale;
>> 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.
> Not very informative. What is it protecting access to?
> Ah. I see this one has already been commented on by Dmitry
Will expand this desciption 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 int32_t adc_tm5_gen2_conv_req(struct adc_tm5_chip *chip)
>> +{
>> + int ret = 0;
> No need to initialise as set in all paths.
Will fix this in the next post.
>
>> + u8 data = 0;
>> + unsigned int count;
>> +
>> + data = ADC_TM_GEN2_EN;
>> + ret = adc_tm5_write(chip, ADC_TM_GEN2_EN_CTL1, &data, 1);
>> + if (ret < 0) {
>> + pr_err("adc-tm enable failed with %d\n", ret);
> General mix of pr_err and dev_err. Should all be dev_err unless
> they aren't associated with a particular device for some reason.
I'll change them all to dev_err in the next post.
>
>> + return ret;
>> + }
>> +
>> + data = ADC_TM_GEN2_CFG_HS_FLAG;
>> + ret = adc_tm5_write(chip, ADC_TM_GEN2_CFG_HS_SET, &data, 1);
>> + if (ret < 0) {
>> + pr_err("adc-tm handshake failed with %d\n", ret);
>> + return ret;
>> + }
>> +
>> + data = ADC_TM_GEN2_CONV_REQ_EN;
>> + ret = adc_tm5_write(chip, ADC_TM_GEN2_CONV_REQ, &data, 1);
>> + if (ret < 0) {
>> + pr_err("adc-tm request conversion failed with %d\n", ret);
>> + return ret;
>> + }
>> +
>> + /*
>> + * SW sets a handshake bit and waits for PBS to clear it
>> + * before the next conversion request can be queued.
>> + */
>> +
>> + for (count = 0; count < ADC_TM_GEN2_POLL_RETRY_COUNT; count++) {
>> + ret = adc_tm5_read(chip, ADC_TM_GEN2_CFG_HS_SET, &data, sizeof(data));
>> + if (ret < 0) {
>> + pr_err("adc-tm read failed with %d\n", ret);
>> + return ret;
>> + }
>> +
>> + if (!(data & ADC_TM_GEN2_CFG_HS_FLAG))
>> + return ret;
>> + usleep_range(ADC_TM_GEN2_POLL_DELAY_MIN_US,
>> + ADC_TM_GEN2_POLL_DELAY_MAX_US);
>> + }
>> +
>> + pr_err("adc-tm conversion request handshake timed out\n");
>> +
>> + return -ETIMEDOUT;
>> +}
>> +
>> +static int adc_tm5_gen2_disable_channel(struct adc_tm5_channel *channel)
>> +{
>> + struct adc_tm5_chip *chip = channel->chip;
>> + int ret;
>> + u8 val;
>> +
>> + mutex_lock(&chip->adc_mutex_lock);
>> +
>> + channel->meas_en = false;
>> + channel->high_thr_en = false;
>> + channel->low_thr_en = false;
>> +
>> + ret = adc_tm5_read(chip, ADC_TM_GEN2_CH_CTL, &val, sizeof(val));
>> + if (ret < 0) {
>> + pr_err("adc-tm block read failed with %d\n", ret);
> Why pr_err rather then dev_err?
Will fix this in the next post.
>
>> + goto disable_fail;
>> + }
>> +
>> + val &= ~ADC_TM_GEN2_TM_CH_SEL;
>> + val |= FIELD_PREP(ADC_TM_GEN2_TM_CH_SEL, channel->channel);
>> +
>> + ret = adc_tm5_write(chip, ADC_TM_GEN2_CH_CTL, &val, 1);
>> + if (ret < 0) {
>> + dev_err(chip->dev, "adc-tm channel disable failed with %d\n", ret);
>> + goto disable_fail;
>> + }
>> +
>> + val = 0;
>> + ret = adc_tm5_write(chip, ADC_TM_GEN2_MEAS_IRQ_EN, &val, 1);
>> + if (ret < 0) {
>> + dev_err(chip->dev, "adc-tm interrupt disable failed with %d\n", ret);
>> + goto disable_fail;
>> + }
>> +
>> +
>> + ret = adc_tm5_gen2_conv_req(channel->chip);
>> + if (ret < 0)
>> + dev_err(chip->dev, "adc-tm channel configure failed with %d\n", ret);
>> +
>> +disable_fail:
>> + mutex_unlock(&chip->adc_mutex_lock);
>> + return ret;
>> +}
>> +
> ...
>
>>
>> +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 },
>> + .disable_channel = adc_tm5_disable_channel,
>> + .configure = adc_tm5_configure,
> This might all look nicer if split into two patches.
> 1st of which does the various structure moves and addes the callbacks for the
> TM5. 2nd of which introduces the gen2 version of everything.
>
> Patch 1 would be a noop which is usually easy to tell, and patch 2 would be
> clean addition of new code.

Yes, I'll split this patch as you recommended in the next post.

>
>> + .isr = adc_tm5_isr,
>> + .irq_name = "pm-adc-tm5",
>> + .gen = ADC_TM5,
>> +};
>> +

Thanks,

Jishnu