Hi Niklas,
On Fri, Mar 17, 2017 at 04:52:58PM +0100, Niklas Söderlund wrote:
> Enable hardware trip points by implementing the set_trips callback. The
> thermal core will take care of setting the initial trip point window and
> to update it once the driver reports a TSC has moved outside it.
>
> The interrupt structure for this device is a bit odd. There is not a
> dedicated IRQ for each TSC, instead the interrupts are shared between
> all TSCs. IRQn is fired if the temp monitored in IRQTEMPn is reached in
> any of the TSCs, example IRQ3 is fired if temperature in IRQTEMP3 is
> reached in either TSC0, TSC1 or TSC2.
>
> For this reason the usage of interrupts in this driver is an all-on or
> all-off design. When an interrupt happens all TSCs are checked and all
> thermal zones are updated. This could be refined to be more fine grained
> but the thermal core takes care of only updating the thermal zones that
> have left their trip point window.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
Looks mostly good, thanks!
...
> static struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops = {
> .get_temp = rcar_gen3_thermal_get_temp,
> + .set_trips = rcar_gen3_thermal_set_trips,
> };
>
> +static void rcar_thermal_irq_set(struct rcar_gen3_thermal_priv *priv, int on)
'bool' instead of 'int'?
> +{
> + unsigned int i;
> + u32 val;
> +
> + val = on ? IRQ_TEMPD1 | IRQ_TEMP2 : 0;
Very minor, merge the 2 lines?
u32 val = on ? IRQ_TEMPD1 | IRQ_TEMP2 : 0;
> +
> + for (i = 0; i < priv->num_tscs; i++)
> + rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQMSK, val);
> +}
> +
> +static irqreturn_t rcar_gen3_thermal_irq(int irq, void *data)
> +{
> + struct rcar_gen3_thermal_priv *priv = data;
> + unsigned long flags;
> + u32 status;
> + int i, ret = IRQ_HANDLED;
> +
> + spin_lock_irqsave(&priv->lock, flags);
Why _irqsave? You are in interrupt context already.
> + for (i = 0; i < priv->num_tscs; i++) {
> + status = rcar_gen3_thermal_read(priv->tscs[i], REG_GEN3_IRQSTR);
> + rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQSTR, 0);
> + if (status)
> + ret = IRQ_WAKE_THREAD;
> + }
> +
> + if (ret == IRQ_WAKE_THREAD)
> + rcar_thermal_irq_set(priv, 0);
'false' instead of '0'? I think this is more readable.
> + spin_unlock_irqrestore(&priv->lock, flags);
Add a blank line before unlock?
> +
> + return ret;
> +}
> +
> +static irqreturn_t rcar_gen3_thermal_irq_thread(int irq, void *data)
> +{
> + struct rcar_gen3_thermal_priv *priv = data;
> + unsigned long flags;
> + int i;
> +
> + for (i = 0; i < priv->num_tscs; i++)
> + thermal_zone_device_update(priv->tscs[i]->zone,
> + THERMAL_EVENT_UNSPECIFIED);
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + rcar_thermal_irq_set(priv, 1);
'true' instead of '1'?
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> + return IRQ_HANDLED;
> +}
> +
...
> @@ -252,7 +351,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> struct resource *res;
> struct thermal_zone_device *zone;
> - int ret, i;
> + int ret, irq, i;
> + char *irqname;
> const struct rcar_gen3_thermal_data *match_data =
> of_device_get_match_data(dev);
>
> @@ -269,8 +369,27 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> if (!priv)
> return -ENOMEM;
>
> + spin_lock_init(&priv->lock);
> +
> platform_set_drvdata(pdev, priv);
>
> + for (i = 0; i < 2; i++) {
Maybe a comment saying that the driver works with two interrupts
currently, so the '2' gets explained?
> + irq = platform_get_irq(pdev, i);
> + if (irq < 0)
> + return irq;
> +
> + irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
> + dev_name(dev), i);
> + if (!irqname)
> + return -ENOMEM;
> +
> + ret = devm_request_threaded_irq(dev, irq, rcar_gen3_thermal_irq,
> + rcar_gen3_thermal_irq_thread,
> + IRQF_SHARED, irqname, priv);
> + if (ret)
> + return ret;
> + }
> +
> pm_runtime_enable(dev);
> pm_runtime_get_sync(dev);
>
> @@ -307,6 +426,12 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> }
> tsc->zone = zone;
> priv->num_tscs++;
> +
> + ret = of_thermal_get_ntrips(tsc->zone);
> + if (ret < 0)
> + goto error_unregister;
> +
> + dev_info(dev, "TSC%d: Loaded %d trip points\n", i, ret);
> }
>
> if (!priv->num_tscs) {
> @@ -314,6 +439,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> goto error_unregister;
> }
>
> + rcar_thermal_irq_set(priv, 1)
'true' instead of '1' again?
> +
Kind regards,
Wolfram
Attachment:
signature.asc
Description: PGP signature
