Quoting Amit Kucheria (2019-07-25 15:18:50) > diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c > index 13a875b99094..f94ef79c37bc 100644 > --- a/drivers/thermal/qcom/tsens-common.c > +++ b/drivers/thermal/qcom/tsens-common.c > @@ -13,6 +13,22 @@ > #include <linux/regmap.h> > #include "tsens.h" > > +/* IRQ state, mask and clear */ > +struct tsens_irq_data { > + u32 up_viol; Is viol a violation? Maybe some kernel doc would be useful here. > + int up_thresh; > + u32 up_irq_mask; > + u32 up_irq_clear; > + u32 low_viol; > + int low_thresh; > + u32 low_irq_mask; > + u32 low_irq_clear; > + u32 crit_viol; > + u32 crit_thresh; > + u32 crit_irq_mask; > + u32 crit_irq_clear; > +}; > + > char *qfprom_read(struct device *dev, const char *cname) > { > struct nvmem_cell *cell; > @@ -65,6 +81,18 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *p1, > } > } > > +static inline u32 degc_to_code(int degc, const struct tsens_sensor *sensor) > +{ > + u32 code = (degc * sensor->slope + sensor->offset) / SLOPE_FACTOR; This can't overflow 32-bits with the multiply and add? > + > + if (code > THRESHOLD_MAX_ADC_CODE) > + code = THRESHOLD_MAX_ADC_CODE; > + else if (code < THRESHOLD_MIN_ADC_CODE) > + code = THRESHOLD_MIN_ADC_CODE; Looks like return clamp(code, THRESHOLD_MIN_ADC_CODE, THRESHOLD_MAX_ADC_CODE); > + pr_debug("%s: raw_code: 0x%x, degc:%d\n", __func__, code, degc); > + return code; > +} > + > static inline int code_to_degc(u32 adc_code, const struct tsens_sensor *s) > { > int degc, num, den; > @@ -106,6 +134,363 @@ static int tsens_hw_to_mC(char *str, struct tsens_sensor *s, int field, int temp > } > } > > +/** > + * tsens_mC_to_hw - Return correct value to be written to threshold > + * registers, whether in ADC code or deciCelsius depending on IP version > + */ > +static int tsens_mC_to_hw(struct tsens_sensor *s, int temp) > +{ > + struct tsens_priv *priv = s->priv; > + > + if (priv->feat->adc) { > + /* milli to C to adc code */ > + return degc_to_code(temp / 1000, s); > + } else { > + /* milli to deci C */ > + return (temp / 100); > + } Drop the else and just return without parenthesis. > +} > + > +static inline unsigned int tsens_ver(struct tsens_priv *priv) > +{ > + return priv->feat->ver_major; > +} > + > +static inline u32 irq_mask(u32 hw_id) Is this used? > +{ > + return 1 << hw_id; > +} > + > +/** > + * tsens_set_interrupt_v1 - Disable an interrupt (enable = false) > + * Re-enable an interrupt (enable = true) > + */ > +static void tsens_set_interrupt_v1(struct tsens_priv *priv, const struct tsens_irq_data d, > + u32 hw_id, enum tsens_irq_type irq_type, bool enable) > +{ > + if (enable) { > + switch (irq_type) { > + case UPPER: > + regmap_field_write(priv->rf[UP_INT_CLEAR_0 + hw_id], 0); > + break; > + case LOWER: > + regmap_field_write(priv->rf[LOW_INT_CLEAR_0 + hw_id], 0); > + break; > + default: > + dev_err(priv->dev, "%s: Invalid irq_type\n", __func__); > + break; > + } > + } else { > + switch (irq_type) { > + case UPPER: > + regmap_field_write(priv->rf[UP_INT_CLEAR_0 + hw_id], 1); > + break; > + case LOWER: > + regmap_field_write(priv->rf[LOW_INT_CLEAR_0 + hw_id], 1); > + break; > + default: > + dev_err(priv->dev, "%s: Invalid irq_type\n", __func__); > + break; > + } > + } > +} > + > +/** > + * tsens_set_interrupt_v2 - Disable an interrupt (enable = false) > + * Re-enable an interrupt (enable = true) > + */ > +static void tsens_set_interrupt_v2(struct tsens_priv *priv, const struct tsens_irq_data d, > + u32 hw_id, enum tsens_irq_type irq_type, bool enable) > +{ > + if (enable) { > + switch (irq_type) { > + case UPPER: > + regmap_field_write(priv->rf[UP_INT_MASK_0 + hw_id], 0); Maybe just have a variable like mask_reg that is equal to UP_INT_MASK, etc? And then one regmap_field_write() call outside the switch that does the write to 0? > + break; > + case LOWER: > + regmap_field_write(priv->rf[LOW_INT_MASK_0 + hw_id], 0); > + break; > + case CRITICAL: > + regmap_field_write(priv->rf[CRIT_INT_MASK_0 + hw_id], 0); > + break; > + default: > + dev_err(priv->dev, "%s: Invalid irq_type\n", __func__); > + break; > + } > + } else { > + /* To disable the interrupt flag for a sensor: > + * 1. Mask further interrupts for this sensor > + * 2. Write 1 followed by 0 to clear the interrupt > + */ > + switch (irq_type) { > + case UPPER: > + regmap_field_write(priv->rf[UP_INT_MASK_0 + hw_id], 1); > + regmap_field_write(priv->rf[UP_INT_CLEAR_0 + hw_id], 1); > + regmap_field_write(priv->rf[UP_INT_CLEAR_0 + hw_id], 0); Same comment here. Use a local variable for mask and clear and then write the register with three regmap_field_write() calls? > + break; > + case LOWER: > + regmap_field_write(priv->rf[LOW_INT_MASK_0 + hw_id], 1); > + regmap_field_write(priv->rf[LOW_INT_CLEAR_0 + hw_id], 1); > + regmap_field_write(priv->rf[LOW_INT_CLEAR_0 + hw_id], 0); > + break; > + case CRITICAL: > + regmap_field_write(priv->rf[CRIT_INT_MASK_0 + hw_id], 1); > + regmap_field_write(priv->rf[CRIT_INT_CLEAR_0 + hw_id], 1); > + regmap_field_write(priv->rf[CRIT_INT_CLEAR_0 + hw_id], 0); > + break; > + default: I'm not sure we actually need this. Modern compilers check for not catching an enum value in a switch case so this shouldn't even really matter. > + dev_err(priv->dev, "%s: Invalid irq_type\n", __func__); > + break; > + } > + } > +} > + > +/** > + * tsens_set_interrupt - Disable an interrupt (enable = false) > + * Re-enable an interrupt (enable = true) > + */ > +static void tsens_set_interrupt(struct tsens_priv *priv, const struct tsens_irq_data d, Why not pass a pointer to tsens_irq_data? > + u32 hw_id, enum tsens_irq_type irq_type, bool enable) > +{ > + /* FIXME: remove tsens_irq_data */ > + dev_dbg(priv->dev, "[%u] %s: %s -> %s\n", hw_id, __func__, > + irq_type ? ((irq_type == 1) ? "UP" : "CRITICAL") : "LOW", > + enable ? "en" : "dis"); > + if (tsens_ver(priv) > VER_1_X) > + tsens_set_interrupt_v2(priv, d, hw_id, irq_type, enable); > + else > + tsens_set_interrupt_v1(priv, d, hw_id, irq_type, enable); > +} > + > +static int tsens_read_irq_state(struct tsens_priv *priv, u32 hw_id, > + struct tsens_sensor *s, struct tsens_irq_data *d) > +{ > + int ret, up_temp, low_temp; > + > + if (hw_id > priv->num_sensors) { > + dev_err(priv->dev, "%s Invalid hw_id\n", __func__); > + return -EINVAL; > + } > + > + ret = regmap_field_read(priv->rf[UPPER_STATUS_0 + hw_id], &d->up_viol); > + if (ret) > + return ret; > + ret = regmap_field_read(priv->rf[UP_THRESH_0 + hw_id], &up_temp); > + if (ret) > + return ret; > + ret = regmap_field_read(priv->rf[LOWER_STATUS_0 + hw_id], &d->low_viol); > + if (ret) > + return ret; > + ret = regmap_field_read(priv->rf[LOW_THRESH_0 + hw_id], &low_temp); > + if (ret) > + return ret; > + ret = regmap_field_read(priv->rf[UP_INT_CLEAR_0 + hw_id], &d->up_irq_clear); > + if (ret) > + return ret; > + ret = regmap_field_read(priv->rf[LOW_INT_CLEAR_0 + hw_id], &d->low_irq_clear); > + if (ret) > + return ret; > + if (tsens_ver(priv) > VER_1_X) { > + ret = regmap_field_read(priv->rf[UP_INT_MASK_0 + hw_id], &d->up_irq_mask); > + if (ret) > + return ret; > + ret = regmap_field_read(priv->rf[LOW_INT_MASK_0 + hw_id], &d->low_irq_mask); > + if (ret) > + return ret; > + } else { > + /* No mask register on older TSENS */ > + d->up_irq_mask = 0; > + d->low_irq_mask = 0; > + } > + > + d->up_thresh = tsens_hw_to_mC("upthresh", s, UP_THRESH_0, up_temp); > + d->low_thresh = tsens_hw_to_mC("lowthresh", s, LOW_THRESH_0, low_temp); > + > + dev_dbg(priv->dev, "[%u] %s%s: status(%u|%u) | clr(%u|%u) | mask(%u|%u)\n", > + hw_id, __func__, (d->up_viol || d->low_viol) ? "(V)" : "", > + d->low_viol, d->up_viol, d->low_irq_clear, d->up_irq_clear, > + d->low_irq_mask, d->up_irq_mask); > + dev_dbg(priv->dev, "[%u] %s%s: thresh: (%d:%d)\n", hw_id, __func__, > + (d->up_viol || d->low_viol) ? "(violation)" : "", > + d->low_thresh, d->up_thresh); > + > + return 0; > +} > + > +static inline u32 masked_irq(u32 hw_id, u32 mask, enum tsens_ver ver) > +{ > + if (ver > VER_1_X) { > + return mask & (1 << hw_id); > + } else { > + /* v1, v0.1 don't have a irq mask register */ > + return 0; > + } Same return comment. > +} > + > +static unsigned long tsens_filter_active_sensors(struct tsens_priv *priv) > +{ > + int i, ret, up, low; > + unsigned long mask = 0; > + > + for (i = 0; i < priv->num_sensors; i++) { > + struct tsens_sensor *s = &priv->sensor[i]; > + u32 hw_id = s->hw_id; > + > + if (IS_ERR(priv->sensor[i].tzd)) > + continue; > + ret = regmap_field_read(priv->rf[UPPER_STATUS_0 + hw_id], &up); > + if (ret) > + return ret; Probably don't want to return ret here given that this returns a mask. Maybe push this into the callsite loop and then break out or return an error from there instead. Making a mask via a loop and then using that again the second time to test the bit is more complicated. > + ret = regmap_field_read(priv->rf[LOWER_STATUS_0 + hw_id], &low); > + if (ret) > + return ret; > + if (up || low) > + set_bit(hw_id, &mask); > + } > + dev_dbg(priv->dev, "%s: hw_id mask: 0x%lx\n", __func__, mask); > + > + return mask; > +} > + > +irqreturn_t tsens_irq_thread(int irq, void *data) > +{ > + struct tsens_priv *priv = data; > + int temp, ret, i; > + unsigned long flags; > + bool enable = true, disable = false; > + unsigned long mask = tsens_filter_active_sensors(priv); > + > + if (!mask) { > + dev_err(priv->dev, "%s: Spurious interrupt?\n", __func__); Do we need the spurious irq print? Doesn't genirq already tell us about spurious interrupts and shuts them down? > + return IRQ_NONE; > + } > + > + /* Check if any sensor raised an IRQ - for each sensor connected to the /* * Please make multiline comments * like this */ > + * TSENS block if it set the threshold violation bit. > + */ > + for (i = 0; i < priv->num_sensors; i++) { > + struct tsens_sensor *s = &priv->sensor[i]; > + struct tsens_irq_data d; > + u32 hw_id = s->hw_id; > + bool trigger = 0; > + > + if (!test_bit(hw_id, &mask)) > + continue; > + if (IS_ERR(priv->sensor[i].tzd)) > + continue; > + ret = get_temp_tsens_valid(s, &temp); > + if (ret) { > + dev_err(priv->dev, "[%u] %s: error reading sensor\n", hw_id, __func__); > + continue; > + } > + > + spin_lock_irqsave(&priv->ul_lock, flags); > + > + tsens_read_irq_state(priv, hw_id, s, &d); > + > + if (d.up_viol && > + !masked_irq(hw_id, d.up_irq_mask, tsens_ver(priv))) { > + tsens_set_interrupt(priv, d, hw_id, UPPER, disable); > + if (d.up_thresh > temp) { > + dev_dbg(priv->dev, "[%u] %s: re-arm upper\n", > + priv->sensor[i].hw_id, __func__); > + /* unmask the interrupt for this sensor */ > + tsens_set_interrupt(priv, d, hw_id, UPPER, enable); > + } else { > + trigger = 1; > + /* Keep irq masked */ > + } > + } else if (d.low_viol && > + !masked_irq(hw_id, d.low_irq_mask, tsens_ver(priv))) { > + tsens_set_interrupt(priv, d, hw_id, LOWER, disable); > + if (d.low_thresh < temp) { > + dev_dbg(priv->dev, "[%u] %s: re-arm low\n", > + priv->sensor[i].hw_id, __func__); > + /* unmask the interrupt for this sensor */ > + tsens_set_interrupt(priv, d, hw_id, LOWER, enable); > + } else { > + trigger = 1; > + /* Keep irq masked */ > + } > + } > + > + /* TODO: (amit) REALLY??? */ Remove it, and the mb? > + mb(); > + > + spin_unlock_irqrestore(&priv->ul_lock, flags); > + > + if (trigger) { > + dev_dbg(priv->dev, "[%u] %s: TZ update trigger (%d mC)\n", > + hw_id, __func__, temp); > + thermal_zone_device_update(priv->sensor[i].tzd, > + THERMAL_EVENT_UNSPECIFIED); > + } else { > + dev_dbg(priv->dev, "[%u] %s: no violation: %d\n", > + hw_id, __func__, temp); > + } > + } > + > + return IRQ_HANDLED; > +} > + > +int tsens_set_trips(void *_sensor, int low, int high) > +{ > + struct tsens_sensor *s = _sensor; > + struct tsens_priv *priv = s->priv; > + struct device *dev = priv->dev; > + struct tsens_irq_data d; > + unsigned long flags; > + int high_val, low_val, cl_high, cl_low; > + bool enable = true; > + u32 hw_id = s->hw_id; > + > + dev_dbg(dev, "[%u] %s: proposed thresholds: (%d:%d)\n", > + hw_id, __func__, low, high); > + > + cl_high = clamp_val(high, -40000, 120000); > + cl_low = clamp_val(low, -40000, 120000); > + > + high_val = tsens_mC_to_hw(s, cl_high); > + low_val = tsens_mC_to_hw(s, cl_low); > + > + spin_lock_irqsave(&priv->ul_lock, flags); > + > + tsens_read_irq_state(priv, hw_id, s, &d); > + > + /* Write the new thresholds and clear the status */ > + regmap_field_write(priv->rf[LOW_THRESH_0 + hw_id], low_val); > + regmap_field_write(priv->rf[UP_THRESH_0 + hw_id], high_val); > + tsens_set_interrupt(priv, d, hw_id, LOWER, enable); > + tsens_set_interrupt(priv, d, hw_id, UPPER, enable); > + > + /* TODO: (amit) REALLY??? */ > + mb(); Again! > + > + spin_unlock_irqrestore(&priv->ul_lock, flags); > + > + dev_dbg(dev, "[%u] %s: (%d:%d)->(%d:%d)\n", > + s->hw_id, __func__, d.low_thresh, d.up_thresh, cl_low, cl_high); > + > + return 0; > +} > + > +int tsens_enable_irq(struct tsens_priv *priv) > +{ > + int ret; > + int val = (tsens_ver(priv) > VER_1_X) ? 7 : 1; Drop useless parenthesis. > + > + ret = regmap_field_write(priv->rf[INT_EN], val); > + if (ret < 0) > + dev_err(priv->dev, "%s: failed to enable interrupts\n", __func__); > + > + return ret; > +} > + > +void tsens_disable_irq(struct tsens_priv *priv) > +{ > + regmap_field_write(priv->rf[INT_EN], 0); > +} > + > int get_temp_tsens_valid(struct tsens_sensor *s, int *temp) > { > struct tsens_priv *priv = s->priv; > @@ -334,6 +719,88 @@ int __init init_common(struct tsens_priv *priv) > goto err_put_device; > } > } > + for (i = 0, j = UPPER_STATUS_0; i < priv->feat->max_sensors; i++, j++) { > + priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map, > + priv->fields[j]); > + if (IS_ERR(priv->rf[j])) { > + ret = PTR_ERR(priv->rf[j]); > + goto err_put_device; > + } > + } > + for (i = 0, j = LOWER_STATUS_0; i < priv->feat->max_sensors; i++, j++) { > + priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map, > + priv->fields[j]); > + if (IS_ERR(priv->rf[j])) { > + ret = PTR_ERR(priv->rf[j]); > + goto err_put_device; > + } > + } > + for (i = 0, j = CRITICAL_STATUS_0; i < priv->feat->max_sensors; i++, j++) { > + priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map, > + priv->fields[j]); > + if (IS_ERR(priv->rf[j])) { > + ret = PTR_ERR(priv->rf[j]); > + goto err_put_device; > + } > + } Can you make a function for this? Takes priv, and a 'j' and then does the allocation and returns failure if something went bad? Then it's a simple devm_tsens_regmap_field_alloc(dev, priv, CRITICAL_STATUS_0); pile of calls. Maybe it could even be iterated through for j too in another loop, not sure how the registers are setup though. > + for (i = 0, j = UP_THRESH_0; i < priv->feat->max_sensors; i++, j++) { > + priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map, > + priv->fields[j]); > + if (IS_ERR(priv->rf[j])) { > + ret = PTR_ERR(priv->rf[j]); > + goto err_put_device; > + } > + } > + for (i = 0, j = LOW_THRESH_0; i < priv->feat->max_sensors; i++, j++) { > + priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map, > + priv->fields[j]); > + if (IS_ERR(priv->rf[j])) { > + ret = PTR_ERR(priv->rf[j]); > + goto err_put_device; > + } > + } > + for (i = 0, j = UP_INT_CLEAR_0; i < priv->feat->max_sensors; i++, j++) { > + priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map, > + priv->fields[j]); > + if (IS_ERR(priv->rf[j])) { > + ret = PTR_ERR(priv->rf[j]); > + goto err_put_device; > + } > + } > + for (i = 0, j = LOW_INT_CLEAR_0; i < priv->feat->max_sensors; i++, j++) { > + priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map, > + priv->fields[j]); > + if (IS_ERR(priv->rf[j])) { > + ret = PTR_ERR(priv->rf[j]); > + goto err_put_device; > + } > + } > + for (i = 0, j = UP_INT_MASK_0; i < priv->feat->max_sensors; i++, j++) { > + priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map, > + priv->fields[j]); > + if (IS_ERR(priv->rf[j])) { > + ret = PTR_ERR(priv->rf[j]); > + goto err_put_device; > + } > + } > + for (i = 0, j = LOW_INT_MASK_0; i < priv->feat->max_sensors; i++, j++) { > + priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map, > + priv->fields[j]); > + if (IS_ERR(priv->rf[j])) { > + ret = PTR_ERR(priv->rf[j]); > + goto err_put_device; > + } > + } > + > + priv->rf[INT_EN] = devm_regmap_field_alloc(dev, priv->tm_map, > + priv->fields[INT_EN]); > + if (IS_ERR(priv->rf[INT_EN])) { > + ret = PTR_ERR(priv->rf[INT_EN]); > + goto err_put_device; > + } > + > + tsens_enable_irq(priv); > + spin_lock_init(&priv->ul_lock); Maybe you should initialize the spinlock before requesting the irq. > > tsens_debug_init(op); > > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c > index 772aa76b50e1..bc83e40ac0cf 100644 > --- a/drivers/thermal/qcom/tsens.c > +++ b/drivers/thermal/qcom/tsens.c > @@ -7,6 +7,7 @@ > #include <linux/err.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_platform.h> > #include <linux/platform_device.h> > #include <linux/pm.h> > #include <linux/slab.h> > @@ -78,12 +79,15 @@ MODULE_DEVICE_TABLE(of, tsens_table); > static const struct thermal_zone_of_device_ops tsens_of_ops = { > .get_temp = tsens_get_temp, > .get_trend = tsens_get_trend, > + .set_trips = tsens_set_trips, > }; > > static int tsens_register(struct tsens_priv *priv) > { > - int i; > + int i, ret, irq; > struct thermal_zone_device *tzd; > + struct resource *res; > + struct platform_device *op = of_find_device_by_node(priv->dev->of_node); What if this fails? > > for (i = 0; i < priv->num_sensors; i++) { > priv->sensor[i].priv = priv; > @@ -96,7 +100,25 @@ static int tsens_register(struct tsens_priv *priv) > if (priv->ops->enable) > priv->ops->enable(priv, i); > } > + > + res = platform_get_resource(op, IORESOURCE_IRQ, 0); > + if (res) { > + irq = res->start; > + ret = devm_request_threaded_irq(&op->dev, irq, > + NULL, tsens_irq_thread, > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > + res->name, priv); > + if (ret) { > + dev_err(&op->dev, "%s: failed to get irq\n", __func__); > + goto err_put_device; > + } > + enable_irq_wake(irq); > + } > return 0; > + > +err_put_device: > + put_device(&op->dev); > + return ret; > } > > static int tsens_probe(struct platform_device *pdev) > diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h > index e1d6af71b2b9..aab47337b797 100644 > --- a/drivers/thermal/qcom/tsens.h > +++ b/drivers/thermal/qcom/tsens.h > @@ -13,8 +13,10 @@ > #define CAL_DEGC_PT2 120 > #define SLOPE_FACTOR 1000 > #define SLOPE_DEFAULT 3200 > +#define THRESHOLD_MAX_ADC_CODE 0x3ff > +#define THRESHOLD_MIN_ADC_CODE 0x0 > > - > +#include <linux/interrupt.h> Include this in the C driver? > #include <linux/thermal.h> > #include <linux/regmap.h> > > @@ -26,6 +28,12 @@ enum tsens_ver { > VER_2_X, > }; > > +enum tsens_irq_type { > + LOWER, > + UPPER,