Re: [PATCH v2 15/15] drivers: thermal: tsens: Add interrupt support

Quoting Amit Kucheria (2019-08-27 05:14:11)

Document arguments and return value? Maybe summary can be 'convert
tsens temperature to hardware register value'?

+ */
+static int tsens_mC_to_hw(struct tsens_sensor *s, int temp)
+{
+       struct tsens_priv *priv = s->priv;
+
+       if (priv->feat->adc) {
+               /* milliC to C to adc code */
+               return degc_to_code(temp / 1000, s);
+       }
+
+       /* milliC to deciC */
+       return temp / 100;
+}
+
+static inline unsigned int tsens_ver(struct tsens_priv *priv)

Can this return the enum instead of unsigned int?

+{
+       return priv->feat->ver_major;
+}
+
+/**
+ * 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, u32 hw_id,
+                                  enum tsens_irq_type irq_type, bool enable)
+{
+       u32 index;
+
+       if (enable) {
+               switch (irq_type) {
+               case UPPER:
+                       index = UP_INT_CLEAR_0 + hw_id;
+                       break;
+               case LOWER:
+                       index = LOW_INT_CLEAR_0 + hw_id;
+                       break;
+               }
+               regmap_field_write(priv->rf[index], 0);
+       } else {
+               switch (irq_type) {
+               case UPPER:
+                       index = UP_INT_CLEAR_0 + hw_id;
+                       break;
+               case LOWER:
+                       index = LOW_INT_CLEAR_0 + hw_id;
+                       break;
+               }
+               regmap_field_write(priv->rf[index], 1);
+       }

De-dup the switch statement and have

	regmap_field_write(priv->rf[index], enable ? 1 : 0);

+}
+
+/**
+ * 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, u32 hw_id,
+                                  enum tsens_irq_type irq_type, bool enable)
+{
+       u32 index_mask, index_clear;
+
+       if (enable) {
+               switch (irq_type) {
+               case UPPER:
+                       index_mask = UP_INT_MASK_0 + hw_id;
+                       break;
+               case LOWER:
+                       index_mask = LOW_INT_MASK_0 + hw_id;
+                       break;
+               }
+               regmap_field_write(priv->rf[index_mask], 0);
+       } else {
+               /* To disable the interrupt flag for a sensor:

Nitpick: Wrong comment style.

+                *  1. Mask further interrupts for this sensor
+                *  2. Write 1 followed by 0 to clear the interrupt
+                */
+               switch (irq_type) {
+               case UPPER:
+                       index_mask  = UP_INT_MASK_0 + hw_id;
+                       index_clear = UP_INT_CLEAR_0 + hw_id;
+                       break;
+               case LOWER:
+                       index_mask  = LOW_INT_MASK_0 + hw_id;
+                       index_clear = LOW_INT_CLEAR_0 + hw_id;
+                       break;
+               }

Please extract index_mask and index_clear assignments to one switch
statement and then change the sequence to an if/else

	if (enable) {
               regmap_field_write(priv->rf[index_mask], 1);
               regmap_field_write(priv->rf[index_clear], 1);
               regmap_field_write(priv->rf[index_clear], 0);
       } else {
               regmap_field_write(priv->rf[index_mask], 0);
       }

+}
+
+/**
+ * tsens_set_interrupt - Disable an interrupt (enable = false)
+ *                       Re-enable an interrupt (enable = true)
+ */
+static void tsens_set_interrupt(struct tsens_priv *priv, u32 hw_id,
+                               enum tsens_irq_type irq_type, bool enable)
+{
+       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, hw_id, irq_type, enable);
+       else
+               tsens_set_interrupt_v1(priv, hw_id, irq_type, enable);
+}
+
+/**
+ * tsens_threshold_violated - Check if a sensor temperature violated a preset threshold
+ *

Document arguments?

+ * Return: 0 if threshold was not violated, 1 if it was violated and negative
+ * errno in case of errors
+ */
+static int tsens_threshold_violated(struct tsens_priv *priv, u32 hw_id,
+                                   struct tsens_irq_data *d)
+{
+       int ret;
+
+       ret = regmap_field_read(priv->rf[UPPER_STATUS_0 + hw_id], &d->up_viol);
+       if (ret)
+               return ret;
+       ret = regmap_field_read(priv->rf[LOWER_STATUS_0 + hw_id], &d->low_viol);
+       if (ret)
+               return ret;
+       if (d->up_viol || d->low_viol)
+               return 1;
+
+       return 0;
+}
+
+static int tsens_read_irq_state(struct tsens_priv *priv, u32 hw_id,
+                               struct tsens_sensor *s, struct tsens_irq_data *d)
+{
+       int 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(s, UP_THRESH_0 + hw_id);
+       d->low_thresh = tsens_hw_to_mC(s, LOW_THRESH_0 + hw_id);
+
+       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);
+
+       /* v1, v0.1 don't have a irq mask register */
+       return 0;
+}
+
+irqreturn_t tsens_irq_thread(int irq, void *data)
+{
+       struct tsens_priv *priv = data;
+       struct tsens_irq_data d;
+       bool enable = true, disable = false;
+       unsigned long flags;
+       int temp, ret, i;
+
+       /*
+        * Check if any sensor raised an IRQ - for each sensor connected to the
+        * TSENS block if it set the threshold violation bit.
+        */
+       for (i = 0; i < priv->num_sensors; i++) {
+               bool trigger = 0;

How about trigger = false? It's a bool.

+               struct tsens_sensor *s = &priv->sensor[i];
+               u32 hw_id = s->hw_id;
+
+               if (IS_ERR(priv->sensor[i].tzd))
+                       continue;
+               if (!tsens_threshold_violated(priv, hw_id, &d))
+                       continue;
+               ret = get_temp_tsens_valid(s, &temp);
+               if (ret) {
+                       dev_err(priv->dev, "[%u] %s: error reading sensor\n", hw_id, __func__);

I hope there isn't an interrupt storm where we're trying to print out
messages from the irq handler.

+                       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, 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, 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, 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, hw_id, LOWER, enable);
+                       } else {
+                               trigger = 1;
+                               /* Keep irq masked */
+                       }
+               }
+
+               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;

Should we return IRQ_NONE in the case that the above for loop didn't
find anything in those if/else-ifs?

+}
+
+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, hw_id, LOWER, enable);
+       tsens_set_interrupt(priv, hw_id, UPPER, enable);

Just pass true? Why is there an enable local variable?

+
+       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;
+}
+

[...]

Leave this where it was, i.e. before the for loop? Or is that a bug and
it doesn't actually work unless it's after the for loop? In which case,
this should go to the previous patch.

Do we really need this? Maybe just bail out in silence because this
should never happen?

+               return -ENODEV;
+       }
+
+       irq = platform_get_irq_byname(pdev, "uplow");
+       if (irq < 0) {
+               dev_err(&pdev->dev, "%s: missing irq in dt: uplow\n", __func__);

You can drop the error print. I upstreamed a change to print the error
generically in the core.

+               return irq;

Did we need to put_device() here?

+       }
+
+       ret = devm_request_threaded_irq(&pdev->dev, irq,
+                                       NULL, tsens_irq_thread,
+                                       IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+                                       dev_name(&pdev->dev), priv);
+       if (ret) {
+               dev_err(&pdev->dev, "%s: failed to get irq\n", __func__);
+               goto err_put_device;
+       }
+       enable_irq_wake(irq);
        return 0;
+
+err_put_device:
+       put_device(&pdev->dev);
+       return ret;