From: Jishnu Prakash <jprakash@codeaurora.org>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
Andy Gross <agross@kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Rob Herring <robh+dt@kernel.org>, Zhang Rui <rui.zhang@intel.com>,
Daniel Lezcano <daniel.lezcano@linaro.org>,
Amit Kucheria <amitk@kernel.org>,
Jonathan Cameron <jic23@kernel.org>,
Hartmut Knaack <knaack.h@gmx.de>,
Lars-Peter Clausen <lars@metafoo.de>,
Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Cc: linux-arm-msm@vger.kernel.org, linux-pm@vger.kernel.org,
devicetree@vger.kernel.org, linux-iio@vger.kernel.org,
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
Jonathan Cameron <Jonathan.Cameron@huawei.com>
Subject: Re: [PATCH v5 7/9] thermal: qcom: add support for adc-tm5 PMIC thermal monitor
Date: Fri, 25 Sep 2020 17:12:05 +0530 [thread overview]
Message-ID: <fe3fc299-5f1d-5318-49f6-b8d42d0615da@codeaurora.org> (raw)
In-Reply-To: <20200914154809.192174-8-dmitry.baryshkov@linaro.org>
Hi Dmitry,
Recently I was testing the ADC_TM driver added with these changes on
SC7180 and I could see that this patch needs a few changes, which I'll
mention below.
On 9/14/2020 9:18 PM, Dmitry Baryshkov wrote:
> Add support for Thermal Monitoring part of PMIC5. This part is closely
> coupled with ADC, using it's channels directly. ADC-TM support
> generating interrupts on ADC value crossing low or high voltage bounds,
> which is used to support thermal trip points.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/iio/adc/qcom-vadc-common.c | 62 +++
> drivers/iio/adc/qcom-vadc-common.h | 3 +
> drivers/thermal/qcom/Kconfig | 11 +
> drivers/thermal/qcom/Makefile | 1 +
> drivers/thermal/qcom/qcom-spmi-adc-tm5.c | 583 +++++++++++++++++++++++
> 5 files changed, 660 insertions(+)
> create mode 100644 drivers/thermal/qcom/qcom-spmi-adc-tm5.c
>
> diff --git a/drivers/iio/adc/qcom-vadc-common.c b/drivers/iio/adc/qcom-vadc-common.c
> index 40d77b3af1bb..e58e393b8713 100644
> --- a/drivers/iio/adc/qcom-vadc-common.c
> +++ b/drivers/iio/adc/qcom-vadc-common.c
> @@ -377,6 +377,42 @@ static int qcom_vadc_map_voltage_temp(const struct vadc_map_pt *pts,
> return 0;
> }
>
> +static irqreturn_t adc_tm5_isr(int irq, void *data)
> +{
> + struct adc_tm5_chip *chip = data;
> + u8 status_low, status_high, ctl;
> + int ret = 0, i = 0;
> +
> + ret = adc_tm5_read(chip, ADC_TM5_STATUS_LOW, &status_low, 1);
> + if (ret) {
> + dev_err(chip->dev, "read status low failed with %d\n", ret);
> + return IRQ_HANDLED;
> + }
> +
> + ret = adc_tm5_read(chip, ADC_TM5_STATUS_HIGH, &status_high, 1);
> + if (ret) {
> + dev_err(chip->dev, "read status high failed with %d\n", ret);
> + return IRQ_HANDLED;
> + }
> +
> + for (i = 0; i < chip->nchannels; i++) {
> + bool upper_set = false, lower_set = false;
> + unsigned int ch = chip->channels[i].channel;
> +
> + if (!chip->channels[i].tzd) {
> + dev_err_once(chip->dev, "thermal device not found\n");
> + continue;
> + }
> +
> + ret = adc_tm5_read(chip, ADC_TM5_M_EN(ch), &ctl, 1);
> +
> + if (ret) {
> + dev_err(chip->dev, "ctl read failed with %d\n", ret);
> + continue;
> + }
> +
> + lower_set = (status_low & BIT(ch)) &&
> + (ctl & ADC_TM5_M_MEAS_EN) &&
> + (ctl & ADC_TM5_M_LOW_THR_INT_EN);
> +
> + upper_set = (status_high & BIT(ch)) &&
> + (ctl & ADC_TM5_M_MEAS_EN) &&
> + (ctl & ADC_TM5_M_HIGH_THR_INT_EN);
> +
> + if (upper_set || lower_set)
> + thermal_zone_device_update(chip->channels[i].tzd,
> + THERMAL_EVENT_UNSPECIFIED);
When using thermal_zone_device_update here, it internally calls
tz->ops->get_temp, which maps to adc_tm5_get_temp defined just below.
This in turn calls iio_read_channel_processed, which internally calls a
mutex and this results in a mutex being called from atomic context.
To avoid this, the interrupt should be requested as a threaded IRQ.
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int adc_tm5_get_temp(void *data, int *temp)
> +{
> + struct adc_tm5_channel *channel = data;
> + int ret, milli_celsius;
> +
> + if (!channel || !channel->iio)
> + return -EINVAL;
> +
> + ret = iio_read_channel_processed(channel->iio, &milli_celsius);
> + if (ret < 0)
> + return ret;
> +
> + *temp = milli_celsius;
> +
> + return 0;
> +}
> +
> +static int adc_tm5_disable_channel(struct adc_tm5_channel *channel)
> +{
> + struct adc_tm5_chip *chip = channel->chip;
> + unsigned int reg = ADC_TM5_M_EN(channel->channel);
> +
> + return adc_tm5_reg_update(chip, reg,
> + ADC_TM5_M_MEAS_EN | ADC_TM5_M_HIGH_THR_INT_EN | ADC_TM5_M_LOW_THR_INT_EN,
> + 0);
> +}
> +
> +static int adc_tm5_configure(struct adc_tm5_channel *channel, int low_temp, int high_temp)
> +{
> + struct adc_tm5_chip *chip = channel->chip;
> + u8 buf[8];
> + u16 reg = ADC_TM5_M_ADC_CH_SEL_CTL(channel->channel);
> + int ret = 0;
> +
> + ret = adc_tm5_read(chip, reg, buf, sizeof(buf));
> + if (ret) {
> + dev_err(chip->dev, "block read failed with %d\n", ret);
> + return ret;
> + }
> +
> + /* Update ADC channel select */
> + buf[0] = channel->adc_channel;
> +
> + /* Warm temperature corresponds to low voltage threshold */
> + if (high_temp != INT_MAX) {
> + u16 adc_code = qcom_adc_tm5_temp_volt_scale(channel->prescale,
> + chip->data->full_scale_code_volt, high_temp);
> +
> + buf[1] = adc_code & 0xff;
> + buf[2] = adc_code >> 8;
> + buf[7] |= ADC_TM5_M_LOW_THR_INT_EN;
> + }
> +
> + /* Cool temperature corresponds to high voltage threshold */
> + if (low_temp != -INT_MAX) {
> + u16 adc_code = qcom_adc_tm5_temp_volt_scale(channel->prescale,
> + chip->data->full_scale_code_volt, low_temp);
> +
> + buf[3] = adc_code & 0xff;
> + buf[4] = adc_code >> 8;
> + buf[7] |= ADC_TM5_M_HIGH_THR_INT_EN;
> + }
> +
> + /* Update timer select */
> + buf[5] = ADC5_TIMER_SEL_2;
> +
> + /* Set calibration select, hw_settle delay */
> + buf[6] &= ~ADC_TM5_M_CTL_HW_SETTLE_DELAY_MASK;
> + buf[6] |= FIELD_PREP(ADC_TM5_M_CTL_HW_SETTLE_DELAY_MASK, channel->hw_settle_time);
> + buf[6] &= ~ADC_TM5_M_CTL_CAL_SEL_MASK;
> + buf[6] |= FIELD_PREP(ADC_TM5_M_CTL_CAL_SEL_MASK, channel->cal_method);
> +
> + buf[7] |= ADC_TM5_M_MEAS_EN;
> +
> + ret = adc_tm5_write(chip, reg, buf, sizeof(buf));
> + if (ret)
> + dev_err(chip->dev, "buf write failed\n");
> +
> + return ret;
> +}
> +
> +static int adc_tm5_set_trips(void *data, int low_temp, int high_temp)
> +{
> + struct adc_tm5_channel *channel = data;
> + struct adc_tm5_chip *chip;
> + int ret;
> +
> + if (!channel)
> + return -EINVAL;
> +
> + chip = channel->chip;
> + dev_dbg(chip->dev, "%d:low_temp(mdegC):%d, high_temp(mdegC):%d\n",
> + channel->channel, low_temp, high_temp);
> +
> + if (high_temp == INT_MAX && low_temp <= -INT_MAX)
> + ret = adc_tm5_disable_channel(channel);
> + else
> + ret = adc_tm5_configure(channel, low_temp, high_temp);
In addition to the configurations done in adc_tm5_configure, you also
need to write to the registers at 0x3546 (write 0x80 to enable the
ADC_TM peripheral overall) and 0x3547 (this is the conversion request
strobe, you need to write to bit 7 here too, to initiate the recurring
measurements).
> +
> + return ret;
> +}
> +
> +static int adc_tm5_probe(struct platform_device *pdev)
> +{
> + struct device_node *node = pdev->dev.of_node;
> + struct device *dev = &pdev->dev;
> + struct adc_tm5_chip *adc_tm;
> + struct regmap *regmap;
> + int ret, irq;
> + u32 reg;
> +
> + regmap = dev_get_regmap(dev->parent, NULL);
> + if (!regmap)
> + return -ENODEV;
> +
> + ret = of_property_read_u32(node, "reg", ®);
> + if (ret)
> + return ret;
> +
> + adc_tm = devm_kzalloc(&pdev->dev, sizeof(*adc_tm), GFP_KERNEL);
> + if (!adc_tm)
> + return -ENOMEM;
> +
> + adc_tm->regmap = regmap;
> + adc_tm->dev = dev;
> + adc_tm->base = reg;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(dev, "get_irq failed: %d\n", irq);
> + return irq;
> + }
> +
> + ret = adc_tm5_get_dt_data(adc_tm, node);
> + if (ret) {
> + dev_err(dev, "get dt data failed: %d\n", ret);
> + return ret;
> + }
> +
> + ret = adc_tm5_init(adc_tm);
> + if (ret) {
> + dev_err(dev, "adc-tm init failed\n");
> + return ret;
> + }
> +
> + ret = adc_tm5_register_tzd(adc_tm);
> + if (ret) {
> + dev_err(dev, "tzd register failed\n");
> + return ret;
> + }
> +
> + return devm_request_irq(dev, irq, adc_tm5_isr, 0, "pm-adc-tm5", adc_tm);
The interrupt should be requested with devm_request_threaded_irq, with
the existing interrupt handler being given as the threaded function, for
the reason mentioned above earlier.
> +}
> +
>
next prev parent reply other threads:[~2020-09-25 11:42 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-14 15:48 [PATCH v5 0/9] qcom: pm8150: add support for thermal monitoring Dmitry Baryshkov
2020-09-14 15:48 ` [PATCH v5 1/9] dt-bindings: thermal: qcom: add adc-thermal monitor bindings Dmitry Baryshkov
2020-09-22 23:40 ` Rob Herring
2020-09-23 9:07 ` Dmitry Baryshkov
2020-09-23 14:00 ` Rob Herring
2020-09-23 15:22 ` Jonathan Cameron
2020-09-14 15:48 ` [PATCH v5 2/9] fixp-arith: add a linear interpolation function Dmitry Baryshkov
2020-09-14 15:48 ` [PATCH v5 3/9] iio: adc: qcom-vadc: move several adc5 functions to common file Dmitry Baryshkov
2020-09-14 15:48 ` [PATCH v5 4/9] iio: adc: qcom-vadc-common: use fixp_linear_interpolate Dmitry Baryshkov
2020-09-14 15:48 ` [PATCH v5 5/9] iio: adc: qcom-spmi-adc5: use of_device_get_match_data Dmitry Baryshkov
2020-09-14 15:48 ` [PATCH v5 6/9] iio: provide of_iio_channel_get_by_name() and devm_ version it Dmitry Baryshkov
2020-09-14 15:48 ` [PATCH v5 7/9] thermal: qcom: add support for adc-tm5 PMIC thermal monitor Dmitry Baryshkov
2020-09-25 11:42 ` Jishnu Prakash [this message]
2020-09-14 15:48 ` [PATCH v5 8/9] arm64: dts: qcom: pm8150x: add definitions for adc-tm5 part Dmitry Baryshkov
2020-09-14 15:48 ` [PATCH v5 9/9] arm64: dts: sm8250-mtp: add thermal zones using pmic's adc-tm5 Dmitry Baryshkov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fe3fc299-5f1d-5318-49f6-b8d42d0615da@codeaurora.org \
--to=jprakash@codeaurora.org \
--cc=Jonathan.Cameron@huawei.com \
--cc=agross@kernel.org \
--cc=amitk@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=daniel.lezcano@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.baryshkov@linaro.org \
--cc=jic23@kernel.org \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=manivannan.sadhasivam@linaro.org \
--cc=pmeerw@pmeerw.net \
--cc=robh+dt@kernel.org \
--cc=rui.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).