Re: [PATCH v6] Thermal: introduce INT3406 thermal driver

[Aaron Lu <aaron.lu@intel.com>]

On 12/23/2014 09:26 AM, Aaron Lu wrote:
> On 12/22/2014 05:53 PM, Lee Jones wrote:
>> On Mon, 22 Dec 2014, Zhang Rui wrote:
>>
>>> On Thu, 2014-12-11 at 16:38 +0800, Aaron Lu wrote:
>>>> INT3406 ACPI device object resembles an ACPI video output device, but its
>>>> _BCM is said to be deprecated and should not be used. So we will make
>>>> use of the raw interface to do the actual cooling. Due to this, the
>>>> backlight core has some modifications. Also, to re-use some of the ACPI
>>>> video module's code, one function has been exported.
>>>>
>>>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>>>
>>> Jingoo and Lee,
>>>
>>> are you okay with the changes in drivers/video/backlight/backlight.c and
>>> include/linux/backlight.h?
>>
>> NB: Jingoo still needs to review the crux of the patch.
>>
>>>> ---
>>>> v6: Fix an issue that wrongly set error path return value as reported
>>>> by Olof Johansson.
>>>>
>>>>  drivers/acpi/video.c                              |  77 ++++----
>>>>  drivers/thermal/Kconfig                           |  26 +--
>>>>  drivers/thermal/int340x_thermal/Kconfig           |  41 ++++
>>>>  drivers/thermal/int340x_thermal/Makefile          |   1 +
>>>>  drivers/thermal/int340x_thermal/int3406_thermal.c | 229 ++++++++++++++++++++++
>>>>  drivers/video/backlight/backlight.c               |  44 +++--
>>>>  include/acpi/video.h                              |  20 ++
>>>>  include/linux/backlight.h                         |   2 +
>>>>  8 files changed, 366 insertions(+), 74 deletions(-)
>>>>  create mode 100644 drivers/thermal/int340x_thermal/Kconfig
>>>>  create mode 100644 drivers/thermal/int340x_thermal/int3406_thermal.c
>>
>> I gather by the message at the top that you're looking for an Ack so
>> this can be routed through another subsystem.  Not going to happen.
>>
>> So you're on v6 already and a) no one has mentioned that introducing a
>> new driver AND making core framework changes (in a different subsystem
>> to boot) in one patch is bad and b) this is the first time you've
>> Cc'ed the maintainers of the aforementioned subsystem?
>>
>> Moving forward you should split this patch into component parts and
>> resend -- only this time ensure you Cc all maintainers in the first
>> instance, rather than as as afterthought.
> 
> OK, thanks for the suggestion.
> 
>>
>>>> -config INT340X_THERMAL
>>>> -	tristate "ACPI INT340X thermal drivers"
>>>> -	depends on X86 && ACPI
>>>> -	select THERMAL_GOV_USER_SPACE
>>>> -	select ACPI_THERMAL_REL
>>>> -	select ACPI_FAN
>>
>> This patch also relies on backlight as well, no?
> 
> Yes it does, will add it in next revision.

My fault. INT340X_THERMAL does not rely on backlight, INT3406 does.
Since I have marked INT3406 depends on ACPI_VIDEO, which depends on
BACKLIGHT, so we are good here.

Thanks,
Aaron