RE: [RESEND PATCH V5 7/8] thermal: da9062/61: Thermal junction temperature monitoring driver

[Steve Twiss]

On 19 February 2017 01:40, Eduardo Valentin wrote:

Hi Eduardo,

My apologies in taking so long to reply.
There were *no* problems with implementing your requests. See below.
I will have sent these changes as PATCH V6.

https://lkml.org/lkml/2017/3/27/253

Regards,
Steve

> To: Steve Twiss
> Subject: Re: [RESEND PATCH V5 7/8] thermal: da9062/61: Thermal junction
> temperature monitoring driver

[...]

> I see no reason why this driver cannot have the COMPILE_TEST flag.
> Tested myself here so:
> 
> +	depends on MFD_DA9062 || COMPILE_TEST

Added.

> please cleanup the minor issues checkpatch complains:
> /scripts/checkpatch.pl --strict <your patch>

I have fixed all of those for latest checkpatch.pl script, this time using "--strict".

[...]

> > +static void da9062_thermal_poll_on(struct work_struct *work)
> > +{
> > +	struct da9062_thermal *thermal = container_of(work,
> > +						struct da9062_thermal,
> > +						work.work);
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	/* clear E_TEMP */
> > +	ret = regmap_write(thermal->hw->regmap,
> > +				DA9062AA_EVENT_B,
> > +				DA9062AA_E_TEMP_MASK);
> > +	if (ret < 0) {
> > +		dev_err(thermal->dev,
> > +			"Cannot clear the TJUNC temperature status\n");
> > +		goto err_enable_irq;
> > +	}
> > +
> > +	/* Now read E_TEMP again: it is acting like a status bit.
> > +	 * If over-temperature, then this status will be true.
> > +	 * If not over-temperature, this status will be false.
> > +	 */
> > +	ret = regmap_read(thermal->hw->regmap,
> > +			  DA9062AA_EVENT_B,
> > +			  &val);
> > +	if (ret < 0) {
> > +		dev_err(thermal->dev,
> > +			"Cannot check the TJUNC temperature status\n");
> > +		goto err_enable_irq;
> > +	} else {
> > +		if (val & DA9062AA_E_TEMP_MASK) {
> > +			mutex_lock(&thermal->lock);
> > +			thermal->temperature = DA9062_MILLI_CELSIUS(125);
> > +			mutex_unlock(&thermal->lock);
> > +			thermal_zone_device_update(thermal->zone,
> > +				THERMAL_EVENT_UNSPECIFIED);
> > +
> > +			schedule_delayed_work(&thermal->work,
> > +				msecs_to_jiffies(thermal->zone->passive_delay));
> > +			return;
> > +		} else {
> > +			mutex_lock(&thermal->lock);
> > +			thermal->temperature = DA9062_MILLI_CELSIUS(0);
> > +			mutex_unlock(&thermal->lock);
> > +			thermal_zone_device_update(thermal->zone,
> > +				THERMAL_EVENT_UNSPECIFIED);
> > +		}
> > +	}
> 
> The above code is a little confusing, can it be maybe, better read like
> this?
> 
> diff --git a/drivers/thermal/da9062-thermal.c b/drivers/thermal/da9062-
> thermal.c
> index 52a095d..6ac8574 100644
> --- a/drivers/thermal/da9062-thermal.c
> +++ b/drivers/thermal/da9062-thermal.c
> @@ -95,26 +95,26 @@ static void da9062_thermal_poll_on(struct work_struct
> *work)
>  		dev_err(thermal->dev,
>  			"Cannot check the TJUNC temperature status\n");
>  		goto err_enable_irq;
> -	} else {
> -		if (val & DA9062AA_E_TEMP_MASK) {
> -			mutex_lock(&thermal->lock);
> -			thermal->temperature = DA9062_MILLI_CELSIUS(125);
> -			mutex_unlock(&thermal->lock);
> -			thermal_zone_device_update(thermal->zone,
> -				THERMAL_EVENT_UNSPECIFIED);
> -
> -			schedule_delayed_work(&thermal->work,
> +	}
> +
> +	if (val & DA9062AA_E_TEMP_MASK) {
> +		mutex_lock(&thermal->lock);
> +		thermal->temperature = DA9062_MILLI_CELSIUS(125);
> +		mutex_unlock(&thermal->lock);
> +		thermal_zone_device_update(thermal->zone,
> +				THERMAL_EVENT_UNSPECIFIED);
> +
> +		schedule_delayed_work(&thermal->work,
>  				msecs_to_jiffies(thermal->zone->passive_delay));
> -			return;
> -		} else {
> -			mutex_lock(&thermal->lock);
> -			thermal->temperature = DA9062_MILLI_CELSIUS(0);
> -			mutex_unlock(&thermal->lock);
> -			thermal_zone_device_update(thermal->zone,
> -			THERMAL_EVENT_UNSPECIFIED);
> -		}
> +		return;
>  	}
> 
> +	mutex_lock(&thermal->lock);
> +	thermal->temperature = DA9062_MILLI_CELSIUS(0);
> +	mutex_unlock(&thermal->lock);
> +	thermal_zone_device_update(thermal->zone,
> +			THERMAL_EVENT_UNSPECIFIED);
> +
>  err_enable_irq:
>  	enable_irq(thermal->irq);
>  }

That makes more sense getting rid of those else clauses.
Applied that, thanks.

Regards,
Steve