Re: [PATCH v4 1/2] thermal: add support for the MCU controlled FAN on Khadas boards

From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Neil Armstrong <narmstrong@baylibre.com>, lee.jones@linaro.org
Cc: khilman@baylibre.com, linux-amlogic@lists.infradead.org,
	linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, rui.zhang@intel.com,
	amit.kucheria@verdurent.com,
	Amit Kucheria <amit.kucheria@linaro.org>
Subject: Re: [PATCH v4 1/2] thermal: add support for the MCU controlled FAN on Khadas boards
Date: Tue, 23 Jun 2020 10:47:51 +0200	[thread overview]
Message-ID: <5fdd616f-812d-55fa-0c2f-301bd3c5edeb@linaro.org> (raw)
In-Reply-To: <5f7154f6-c527-6665-4f7b-affd3cd62b70@baylibre.com>

Hi Neil,

On 23/06/2020 10:25, Neil Armstrong wrote:
> Hi Daniel,
> 
> On 22/06/2020 21:46, Daniel Lezcano wrote:
>> On 18/06/2020 15:38, Neil Armstrong wrote:
>>> The new Khadas VIM2 and VIM3 boards controls the cooling fan via the
>>> on-board microcontroller.
>>>
>>> This implements the FAN control as thermal devices and as cell of the Khadas
>>> MCU MFD driver.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org>
>>> ---
>>> Hi Lee,
>>>
>>> Could you apply this patch via the MFD tree since it depends on
>>> the linux/mfd/khadas-mcu.h header ?
>>>
>>> This patch is unchanged from the v3 serie.
>>>

[ ... ]

>> Nitpicking : move the save section to suspend.
> 
> OK, but moving this makes khadas_mcu_fan_disable() useless.

It is fine. Seeing the shutdown calling the disable function which in
turn saves the state looks strange but it is not critical.

If you want to keep it as it is, I'm fine with that too.

>>> +	return 0;
>>> +}
>>> +
>>> +static void khadas_mcu_fan_shutdown(struct platform_device *pdev)
>>> +{
>>> +	khadas_mcu_fan_disable(&pdev->dev);
>>> +}
>>> +
>>> +#ifdef CONFIG_PM_SLEEP
>>> +static int khadas_mcu_fan_suspend(struct device *dev)
>>> +{
>>> +	return khadas_mcu_fan_disable(dev);
>>> +}
>>> +
>>> +static int khadas_mcu_fan_resume(struct device *dev)
>>> +{
>>> +	struct khadas_mcu_fan_ctx *ctx = dev_get_drvdata(dev);
>>> +
>>> +	return khadas_mcu_fan_set_level(ctx, ctx->level);
>>
>> Out of curiosity, did you check the fan is not continuously spinning
>> after a resume when the suspend happened during a mitigation phase?
> 
> No, but I took the logic from the hmwmon pwm-fan driver.
> 
> Not sure this is critical here.
No, logically you should the fan spinning at resume time even if the
board is not hot, until the post resume notification is called which
will update the thermal zone and set the cooling device state to zero again.

Thanks
  -- Daniel

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog