Re: [PATCH v2 5/7] thermal: rcar_gen3_thermal: enable hardware interrupts for trip points

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 4765 bytes --]

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@ragnatech.se>

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]