| Message ID | 20170219014007.GA20845@localhost.localdomain (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Eduardo Valentin |
| Headers | show |
On 19 February 2017 01:40, Eduardo Valentin wrote: > Subject: Re: [RESEND PATCH V5 7/8] thermal: da9062/61: Thermal junction > temperature monitoring driver > > Steve, > > Apologize for the very late answer on your driver. I still have few > minor requests, please check then: > Hi Eduardo, No problem. I'll check your comments out, rebase to the latest linux-mainline tag and then send out a new patch set. Regards, Steve
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
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); }