> -----Messaggio originale----- > Da: Amit Kucheria <amit.kucheria@xxxxxxxxxx> > Inviato: lunedì 20 luglio 2020 11:41 > A: Ansuel Smith <ansuelsmth@xxxxxxxxx> > Cc: Rob Herring <robh+dt@xxxxxxxxxx>; Andy Gross <agross@xxxxxxxxxx>; > Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>; Zhang Rui > <rui.zhang@xxxxxxxxx>; Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>; > Michael Turquette <mturquette@xxxxxxxxxxxx>; Stephen Boyd > <sboyd@xxxxxxxxxx>; Linux PM list <linux-pm@xxxxxxxxxxxxxxx>; linux-arm- > msm <linux-arm-msm@xxxxxxxxxxxxxxx>; DTML > <devicetree@xxxxxxxxxxxxxxx>; Linux Kernel Mailing List <linux- > kernel@xxxxxxxxxxxxxxx>; linux-clk <linux-clk@xxxxxxxxxxxxxxx> > 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@xxxxxxxxx> > 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@xxxxxxxxx> > > --- > > 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, > ®); > > + 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, > ®); > > + 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? > > +} > > + > > +static void hw_init(struct tsens_priv *priv) > > +{ > > + int ret; > > + unsigned int reg_cntl = 0, reg_cfg = 0, reg_thr = 0; > > + unsigned int reg_status_cntl = 0; > > + > > + regmap_read(priv->tm_map, CNTL_ADDR, ®_cntl); > > + regmap_write(priv->tm_map, CNTL_ADDR, reg_cntl | > TSENS_SW_RST); > > + > > + reg_cntl |= SLP_CLK_ENA | (MEASURE_PERIOD << 18) | > > + (((1 << priv->num_sensors) - 1) << SENSOR0_SHIFT); > > + regmap_write(priv->tm_map, CNTL_ADDR, reg_cntl); > > + regmap_read(priv->tm_map, STATUS_CNTL_ADDR_8064, > ®_status_cntl); > > + reg_status_cntl |= LOWER_STATUS_CLR | UPPER_STATUS_CLR | > > + MIN_STATUS_MASK | MAX_STATUS_MASK; > > + regmap_write(priv->tm_map, STATUS_CNTL_ADDR_8064, > reg_status_cntl); > > + reg_cntl |= TSENS_EN; > > + regmap_write(priv->tm_map, CNTL_ADDR, reg_cntl); > > + > > + regmap_read(priv->tm_map, CONFIG_ADDR, ®_cfg); > > + if (priv->num_sensors > 1) > > + reg_cfg = (reg_cfg & ~CONFIG_MASK) | CONFIG; > > + else > > + reg_cfg = (reg_cfg & ~CONFIG_MASK) | > > + (CONFIG << CONFIG_SHIFT_8660); > > + regmap_write(priv->tm_map, CONFIG_ADDR, reg_cfg); > > + > > + reg_thr |= (LOWER_LIMIT_TH_8064 << > THRESHOLD_LOWER_LIMIT_SHIFT) | > > + (UPPER_LIMIT_TH_8064 << THRESHOLD_UPPER_LIMIT_SHIFT) > | > > + (MIN_LIMIT_TH << THRESHOLD_MIN_LIMIT_SHIFT) | > > + (MAX_LIMIT_TH << THRESHOLD_MAX_LIMIT_SHIFT); > > + > > + regmap_write(priv->tm_map, THRESHOLD_ADDR, reg_thr); > > + > > + ret = devm_request_irq(priv->dev, priv->tsens_irq, tsens_isr, > > + IRQF_TRIGGER_RISING, "tsens_interrupt", priv); > > + if (ret < 0) { > > + dev_err(priv->dev, "request_irq FAIL: %d", ret); > > + return; > > + } > > + > > + INIT_WORK(&priv->tsens_work, tsens_scheduler_fn); > > +} > > + > > static int suspend_8960(struct tsens_priv *priv) > > { > > int ret; > > @@ -191,6 +367,8 @@ static int init_8960(struct tsens_priv *priv) > > if (i >= 5) > > priv->sensor[i].status = S0_STATUS_ADDR + 40; > > priv->sensor[i].status += i * 4; > > + priv->sensor[i].slope = tsens_8960_slope[i]; > > + INIT_WORK(&priv->sensor[i].notify_work, > notify_uspace_tsens_fn); > > } > > > > reg_cntl = SW_RST; > > @@ -241,18 +419,9 @@ static int calibrate_8960(struct tsens_priv > *priv) > > > > kfree(data); > > > > - return 0; > > -} > > - > > -/* 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; > > + hw_init(priv); > > > > - slope = thermal_zone_get_slope(s->tzd); > > - offset = CAL_MDEGC - slope * s->offset; > > - > > - return adc_code * slope + offset; > > This code move hunk belongs in a separate patch. > > > + return 0; > > } > > > > static int get_temp_8960(const struct tsens_sensor *s, int *temp) > > diff --git a/drivers/thermal/qcom/tsens.h > b/drivers/thermal/qcom/tsens.h > > index 59d01162c66a..e66048fabcc7 100644 > > --- a/drivers/thermal/qcom/tsens.h > > +++ b/drivers/thermal/qcom/tsens.h > > @@ -48,6 +48,7 @@ enum tsens_irq_type { > > struct tsens_sensor { > > struct tsens_priv *priv; > > struct thermal_zone_device *tzd; > > + struct work_struct notify_work; > > int offset; > > unsigned int hw_id; > > int slope; > > @@ -559,6 +560,7 @@ struct tsens_priv { > > struct regmap *tm_map; > > struct regmap *srot_map; > > u32 tm_offset; > > + u32 tsens_irq; > > > > /* lock for upper/lower threshold interrupts */ > > spinlock_t ul_lock; > > @@ -568,6 +570,7 @@ struct tsens_priv { > > struct tsens_features *feat; > > const struct reg_field *fields; > > const struct tsens_ops *ops; > > + struct work_struct tsens_work; > > > > struct dentry *debug_root; > > struct dentry *debug; > > -- > > 2.27.0 > >
