Re: [PATCH v4 5/7] drivers: thermal: tsens: add interrupt support for 9860 driver

[Amit Kucheria <amit.kucheria@linaro.org>]

On Wed, Jul 22, 2020 at 12:52 AM <ansuelsmth@gmail.com> wrote:
>
>
>
> > -----Messaggio originale-----
> > Da: Amit Kucheria <amit.kucheria@linaro.org>
> > Inviato: lunedì 20 luglio 2020 11:41
> > A: Ansuel Smith <ansuelsmth@gmail.com>
> > Cc: Rob Herring <robh+dt@kernel.org>; Andy Gross <agross@kernel.org>;
> > Bjorn Andersson <bjorn.andersson@linaro.org>; Zhang Rui
> > <rui.zhang@intel.com>; Daniel Lezcano <daniel.lezcano@linaro.org>;
> > Michael Turquette <mturquette@baylibre.com>; Stephen Boyd
> > <sboyd@kernel.org>; Linux PM list <linux-pm@vger.kernel.org>; linux-arm-
> > msm <linux-arm-msm@vger.kernel.org>; DTML
> > <devicetree@vger.kernel.org>; Linux Kernel Mailing List <linux-
> > kernel@vger.kernel.org>; linux-clk <linux-clk@vger.kernel.org>
> > Oggetto: Re: [PATCH v4 5/7] drivers: thermal: tsens: add interrupt support
> > for 9860 driver
> >
> > Hi Ansuel,
> >
> > Thanks for this patch.
> >
> > On Thu, Jul 16, 2020 at 7:58 AM Ansuel Smith <ansuelsmth@gmail.com>
> > wrote:
> > >
> > > Add interrupt support for 9860 tsens driver used to set thermal trip
> > > point for the system.
> >
> > typo: 8960
> >
> > You've used the names 8960 and ipq8064 interchangeably throughout the
> > series. AFAICT, msm8960, ipq8064 and apq8064 use the same IP version
> > of tsens. Please use 8960 in all patches, descriptions and dt-binding.
> > to reflect the filename for the driver.
> > Then add ipq8064 and apq8064 in a comment in the driver like here to
> > show that the driver also supports these other SoCs:
> > https://elixir.bootlin.com/linux/v5.8-
> > rc4/source/drivers/thermal/qcom/tsens-v0_1.c#L328
> >
> > You can also add a new compatible string for ipq8064 as a separate
> > patch at the end of the series.
> >
> > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > > ---
> > >  drivers/thermal/qcom/tsens-8960.c | 197
> > +++++++++++++++++++++++++++---
> > >  drivers/thermal/qcom/tsens.h      |   3 +
> > >  2 files changed, 186 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/thermal/qcom/tsens-8960.c
> > b/drivers/thermal/qcom/tsens-8960.c
> > > index 45788eb3c666..20d0bfb10f1f 100644
> > > --- a/drivers/thermal/qcom/tsens-8960.c
> > > +++ b/drivers/thermal/qcom/tsens-8960.c
> > > @@ -8,6 +8,7 @@
> > >  #include <linux/bitops.h>
> > >  #include <linux/regmap.h>
> > >  #include <linux/mfd/syscon.h>
> > > +#include <linux/interrupt.h>
> > >  #include <linux/thermal.h>
> > >  #include "tsens.h"
> > >
> > > @@ -27,7 +28,6 @@
> > >  /* CNTL_ADDR bitmasks */
> > >  #define EN                     BIT(0)
> > >  #define SW_RST                 BIT(1)
> > > -#define SENSOR0_EN             BIT(3)
> > >  #define SLP_CLK_ENA            BIT(26)
> > >  #define SLP_CLK_ENA_8660       BIT(24)
> > >  #define MEASURE_PERIOD         1
> > > @@ -41,14 +41,26 @@
> > >
> > >  #define THRESHOLD_ADDR         0x3624
> > >  /* THRESHOLD_ADDR bitmasks */
> > > +#define THRESHOLD_MAX_CODE             0x20000
> > > +#define THRESHOLD_MIN_CODE             0
> > >  #define THRESHOLD_MAX_LIMIT_SHIFT      24
> > >  #define THRESHOLD_MIN_LIMIT_SHIFT      16
> > >  #define THRESHOLD_UPPER_LIMIT_SHIFT    8
> > >  #define THRESHOLD_LOWER_LIMIT_SHIFT    0
> > > +#define THRESHOLD_MAX_LIMIT_MASK       (THRESHOLD_MAX_CODE
> > << \
> > > +                                               THRESHOLD_MAX_LIMIT_SHIFT)
> > > +#define THRESHOLD_MIN_LIMIT_MASK       (THRESHOLD_MAX_CODE <<
> > \
> > > +                                               THRESHOLD_MIN_LIMIT_SHIFT)
> > > +#define THRESHOLD_UPPER_LIMIT_MASK     (THRESHOLD_MAX_CODE
> > << \
> > > +                                               THRESHOLD_UPPER_LIMIT_SHIFT)
> > > +#define THRESHOLD_LOWER_LIMIT_MASK     (THRESHOLD_MAX_CODE
> > << \
> > > +                                               THRESHOLD_LOWER_LIMIT_SHIFT)
> > >
> > >  /* Initial temperature threshold values */
> > > -#define LOWER_LIMIT_TH         0x50
> > > -#define UPPER_LIMIT_TH         0xdf
> > > +#define LOWER_LIMIT_TH_8960    0x50
> > > +#define UPPER_LIMIT_TH_8960    0xdf
> > > +#define LOWER_LIMIT_TH_8064    0x9d /* 95C */
> > > +#define UPPER_LIMIT_TH_8064    0xa6 /* 105C */
> > >  #define MIN_LIMIT_TH           0x0
> > >  #define MAX_LIMIT_TH           0xff
> > >
> > > @@ -57,6 +69,170 @@
> > >  #define TRDY_MASK              BIT(7)
> > >  #define TIMEOUT_US             100
> > >
> > > +#define TSENS_EN               BIT(0)
> > > +#define TSENS_SW_RST           BIT(1)
> > > +#define TSENS_ADC_CLK_SEL      BIT(2)
> > > +#define SENSOR0_EN             BIT(3)
> > > +#define SENSOR1_EN             BIT(4)
> > > +#define SENSOR2_EN             BIT(5)
> > > +#define SENSOR3_EN             BIT(6)
> > > +#define SENSOR4_EN             BIT(7)
> > > +#define SENSORS_EN             (SENSOR0_EN | SENSOR1_EN | \
> > > +                               SENSOR2_EN | SENSOR3_EN | SENSOR4_EN)
> > > +#define TSENS_8064_SENSOR5_EN                          BIT(8)
> > > +#define TSENS_8064_SENSOR6_EN                          BIT(9)
> > > +#define TSENS_8064_SENSOR7_EN                          BIT(10)
> > > +#define TSENS_8064_SENSOR8_EN                          BIT(11)
> > > +#define TSENS_8064_SENSOR9_EN                          BIT(12)
> > > +#define TSENS_8064_SENSOR10_EN                         BIT(13)
> > > +#define TSENS_8064_SENSORS_EN                          (SENSORS_EN | \
> > > +                                               TSENS_8064_SENSOR5_EN | \
> > > +                                               TSENS_8064_SENSOR6_EN | \
> > > +                                               TSENS_8064_SENSOR7_EN | \
> > > +                                               TSENS_8064_SENSOR8_EN | \
> > > +                                               TSENS_8064_SENSOR9_EN | \
> > > +                                               TSENS_8064_SENSOR10_EN)
> > > +
> > > +u32 tsens_8960_slope[] = {
> > > +                       1176, 1176, 1154, 1176,
> > > +                       1111, 1132, 1132, 1199,
> > > +                       1132, 1199, 1132
> > > +                       };
> > > +
> > > +/* Temperature on y axis and ADC-code on x-axis */
> > > +static inline int code_to_mdegC(u32 adc_code, const struct
> > tsens_sensor *s)
> > > +{
> > > +       int slope, offset;
> > > +
> > > +       slope = thermal_zone_get_slope(s->tzd);
> > > +       offset = CAL_MDEGC - slope * s->offset;
> > > +
> > > +       return adc_code * slope + offset;
> > > +}
> > > +
> > > +static void notify_uspace_tsens_fn(struct work_struct *work)
> > > +{
> > > +       struct tsens_sensor *s = container_of(work, struct tsens_sensor,
> > > +                                                               notify_work);
> > > +
> > > +       sysfs_notify(&s->tzd->device.kobj, NULL, "type");
> > > +}
> > > +
> > > +static void tsens_scheduler_fn(struct work_struct *work)
> > > +{
> > > +       struct tsens_priv *priv =
> > > +               container_of(work, struct tsens_priv, tsens_work);
> > > +       unsigned int threshold, threshold_low, code, reg, sensor;
> > > +       unsigned long mask;
> > > +       bool upper_th_x, lower_th_x;
> > > +       int ret;
> > > +
> > > +       ret = regmap_read(priv->tm_map, STATUS_CNTL_ADDR_8064,
> > &reg);
> > > +       if (ret)
> > > +               return;
> > > +       reg = reg | LOWER_STATUS_CLR | UPPER_STATUS_CLR;
> > > +       ret = regmap_write(priv->tm_map, STATUS_CNTL_ADDR_8064, reg);
> > > +       if (ret)
> > > +               return;
> > > +
> > > +       mask = ~(LOWER_STATUS_CLR | UPPER_STATUS_CLR);
> > > +       ret = regmap_read(priv->tm_map, THRESHOLD_ADDR, &threshold);
> > > +       if (ret)
> > > +               return;
> > > +       threshold_low = (threshold & THRESHOLD_LOWER_LIMIT_MASK) >>
> > > +                       THRESHOLD_LOWER_LIMIT_SHIFT;
> > > +       threshold = (threshold & THRESHOLD_UPPER_LIMIT_MASK) >>
> > > +                   THRESHOLD_UPPER_LIMIT_SHIFT;
> > > +
> > > +       ret = regmap_read(priv->tm_map, STATUS_CNTL_ADDR_8064,
> > &reg);
> > > +       if (ret)
> > > +               return;
> > > +
> > > +       ret = regmap_read(priv->tm_map, CNTL_ADDR, &sensor);
> > > +       if (ret)
> > > +               return;
> > > +       sensor &= (uint32_t)TSENS_8064_SENSORS_EN;
> > > +       sensor >>= SENSOR0_SHIFT;
> > > +
> > > +       /* Constraint: There is only 1 interrupt control register for all
> > > +        * 11 temperature sensor. So monitoring more than 1 sensor based
> > > +        * on interrupts will yield inconsistent result. To overcome this
> > > +        * issue we will monitor only sensor 0 which is the master sensor.
> > > +        */
> > > +
> > > +       /* Skip if the sensor is disabled */
> > > +       if (sensor & 1) {
> > > +               ret = regmap_read(priv->tm_map, priv->sensor[0].status,
> > &code);
> > > +               if (ret)
> > > +                       return;
> > > +               upper_th_x = code >= threshold;
> > > +               lower_th_x = code <= threshold_low;
> > > +               if (upper_th_x)
> > > +                       mask |= UPPER_STATUS_CLR;
> > > +               if (lower_th_x)
> > > +                       mask |= LOWER_STATUS_CLR;
> > > +               if (upper_th_x || lower_th_x) {
> > > +                       /* Notify user space */
> > > +                       schedule_work(&priv->sensor[0].notify_work);
> > > +                       pr_debug("Trigger (%d degrees) for sensor %d\n",
> > > +                                code_to_mdegC(code, &priv->sensor[0]), 0);
> > > +               }
> > > +       }
> > > +       regmap_write(priv->tm_map, STATUS_CNTL_ADDR_8064, reg &
> > mask);
> > > +}
> > > +
> > > +static irqreturn_t tsens_isr(int irq, void *data)
> > > +{
> > > +       struct tsens_priv *priv = data;
> > > +
> > > +       schedule_work(&priv->tsens_work);
> > > +       return IRQ_HANDLED;
> >
> >
> > Have you considered trying to reuse the regmap and interrupt handling
> > infrastructure in tsens.c that I used to convert over everything after
> > IP version 0.1?
> >
> > I started converting over 8960 but never managed to finish testing
> > this[1]. I'd be happy for you to take this over and get it working so
> > the 8960 doesn't end up being a completely separate driver from the
> > other platforms.
> >
> > [1]
> > https://git.linaro.org/people/amit.kucheria/kernel.git/log/?h=wrk3/tsens-
> > 8960-breakage
> >
>
> Thanks a lot for the link. I started doing some test and I think the only general
> code we will be able to use will be the init_common. The get temp and
> the function to convert code to decg are very different from the one used in
> 8960.  Do you think keep a custom get temp function is good or not?

OK. Let's start common init code and custom get_temp and adc-to-degc functions.

Regards,
Amit