Re: [PATCH v7] hwmon: Add driver for EXYNOS4 TMU

Re: [PATCH v7] hwmon: Add driver for EXYNOS4 TMU

[Paul Bolle]

(This is an attempt to do a bit of review after the fact. See, this
appears to be to the patch that ended up as commit
9d97e5c81e15afaef65d00f077f863c94f750839 in the mainline tree. Since
that tree is at v3.2-rc2 now this might be in time for v3.2. If my
comments have merit, that is.)

On Wed, 2011-09-07 at 18:49 +0900, Donggeun Kim wrote:
> This patch allows to read temperature
> from TMU(Thermal Management Unit) of SAMSUNG EXYNOS4 series of SoC.
[...]
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 0b62c3c..c6fb761 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -303,6 +303,16 @@ config SENSORS_DS1621
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called ds1621.
>  
> +config SENSORS_EXYNOS4_TMU
> +	tristate "Temperature sensor on Samsung EXYNOS4"
> +	depends on EXYNOS4_DEV_TMU

It doesn't look like that Kconfig symbol is part of the tree just yet.
That means people will not be able to build this driver from the
mainline tree. Why is this dependency needed? In a (rather quick) scan
of the code of this driver I couldn't spot anything not yet available in
the tree.

> +	help
> +	  If you say yes here you get support for TMU (Thermal Managment
> +	  Unit) on SAMSUNG EXYNOS4 series of SoC.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called exynos4-tmu.
> +
>  config SENSORS_I5K_AMB
>  	tristate "FB-DIMM AMB temperature sensor on Intel 5000 series chipsets"
>  	depends on PCI && EXPERIMENTAL
[...]
> diff --git a/drivers/hwmon/exynos4_tmu.c b/drivers/hwmon/exynos4_tmu.c
> new file mode 100644
> index 0000000..0170c90
> --- /dev/null
> +++ b/drivers/hwmon/exynos4_tmu.c
[...]
> +#ifdef CONFIG_PM
> +static int exynos4_tmu_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	exynos4_tmu_control(pdev, false);
> +
> +	return 0;
> +}
> +
> +static int exynos4_tmu_resume(struct platform_device *pdev)
> +{
> +	exynos4_tmu_initialize(pdev);
> +	exynos4_tmu_control(pdev, true);
> +
> +	return 0;
> +}
> +#else
> +#define exynos4_tmu_suspend NULL
> +#define exynos4_tmu_resume NULL
> +#endif
> +
> +static struct platform_driver exynos4_tmu_driver = {
> +	.driver = {
> +		.name   = "exynos4-tmu",
> +		.owner  = THIS_MODULE,
> +	},
> +	.probe = exynos4_tmu_probe,
> +	.remove	= __devexit_p(exynos4_tmu_remove),
> +	.suspend = exynos4_tmu_suspend,
> +	.resume = exynos4_tmu_resume,
> +};

A common idiom seems to be (I'm speaking from memory here) to
wrap .suspend and .resume inside an "#ifdef CONFIG_PM" / "#endif" pair.
That would allow to drop both "#define exynos4_tmu_suspend NULL" and
"#define exynos4_tmu_resume NULL" above. Would that work here too?


Paul Bolle

Re: [PATCH v7] hwmon: Add driver for EXYNOS4 TMU

[Guenter Roeck]

On Tue, 2011-11-15 at 15:34 -0500, Paul Bolle wrote:
> (This is an attempt to do a bit of review after the fact. See, this
> appears to be to the patch that ended up as commit
> 9d97e5c81e15afaef65d00f077f863c94f750839 in the mainline tree. Since
> that tree is at v3.2-rc2 now this might be in time for v3.2. If my
> comments have merit, that is.)
> 
> On Wed, 2011-09-07 at 18:49 +0900, Donggeun Kim wrote:
> > This patch allows to read temperature
> > from TMU(Thermal Management Unit) of SAMSUNG EXYNOS4 series of SoC.
> [...]
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 0b62c3c..c6fb761 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -303,6 +303,16 @@ config SENSORS_DS1621
> >  	  This driver can also be built as a module.  If so, the module
> >  	  will be called ds1621.
> >  
> > +config SENSORS_EXYNOS4_TMU
> > +	tristate "Temperature sensor on Samsung EXYNOS4"
> > +	depends on EXYNOS4_DEV_TMU
> 
> It doesn't look like that Kconfig symbol is part of the tree just yet.
> That means people will not be able to build this driver from the
> mainline tree. Why is this dependency needed? In a (rather quick) scan
> of the code of this driver I couldn't spot anything not yet available in
> the tree.
> 
I have to defer to the driver author for that. Maybe the dependency was
renamed at some point, or removed altogether.

> > +	help
> > +	  If you say yes here you get support for TMU (Thermal Managment
> > +	  Unit) on SAMSUNG EXYNOS4 series of SoC.
> > +
> > +	  This driver can also be built as a module. If so, the module
> > +	  will be called exynos4-tmu.
> > +
> >  config SENSORS_I5K_AMB
> >  	tristate "FB-DIMM AMB temperature sensor on Intel 5000 series chipsets"
> >  	depends on PCI && EXPERIMENTAL
> [...]
> > diff --git a/drivers/hwmon/exynos4_tmu.c b/drivers/hwmon/exynos4_tmu.c
> > new file mode 100644
> > index 0000000..0170c90
> > --- /dev/null
> > +++ b/drivers/hwmon/exynos4_tmu.c
> [...]
> > +#ifdef CONFIG_PM
> > +static int exynos4_tmu_suspend(struct platform_device *pdev, pm_message_t state)
> > +{
> > +	exynos4_tmu_control(pdev, false);
> > +
> > +	return 0;
> > +}
> > +
> > +static int exynos4_tmu_resume(struct platform_device *pdev)
> > +{
> > +	exynos4_tmu_initialize(pdev);
> > +	exynos4_tmu_control(pdev, true);
> > +
> > +	return 0;
> > +}
> > +#else
> > +#define exynos4_tmu_suspend NULL
> > +#define exynos4_tmu_resume NULL
> > +#endif
> > +
> > +static struct platform_driver exynos4_tmu_driver = {
> > +	.driver = {
> > +		.name   = "exynos4-tmu",
> > +		.owner  = THIS_MODULE,
> > +	},
> > +	.probe = exynos4_tmu_probe,
> > +	.remove	= __devexit_p(exynos4_tmu_remove),
> > +	.suspend = exynos4_tmu_suspend,
> > +	.resume = exynos4_tmu_resume,
> > +};
> 
> A common idiom seems to be (I'm speaking from memory here) to
> wrap .suspend and .resume inside an "#ifdef CONFIG_PM" / "#endif" pair.
> That would allow to drop both "#define exynos4_tmu_suspend NULL" and
> "#define exynos4_tmu_resume NULL" above. Would that work here too?
> 
Seems to be a matter of opinion. I personally don't care one way or the
other, but I was told some time ago that the above method would be
preferred over using a second #ifdef.

Guenter

Re: [PATCH v7] hwmon: Add driver for EXYNOS4 TMU

[Donggeun Kim]

On 2011년 11월 16일 06:24, Guenter Roeck wrote:
> On Tue, 2011-11-15 at 15:34 -0500, Paul Bolle wrote:
>> (This is an attempt to do a bit of review after the fact. See, this
>> appears to be to the patch that ended up as commit
>> 9d97e5c81e15afaef65d00f077f863c94f750839 in the mainline tree. Since
>> that tree is at v3.2-rc2 now this might be in time for v3.2. If my
>> comments have merit, that is.)
>>
>> On Wed, 2011-09-07 at 18:49 +0900, Donggeun Kim wrote:
>>> This patch allows to read temperature
>>> from TMU(Thermal Management Unit) of SAMSUNG EXYNOS4 series of SoC.
>> [...]
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index 0b62c3c..c6fb761 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -303,6 +303,16 @@ config SENSORS_DS1621
>>>  	  This driver can also be built as a module.  If so, the module
>>>  	  will be called ds1621.
>>>  
>>> +config SENSORS_EXYNOS4_TMU
>>> +	tristate "Temperature sensor on Samsung EXYNOS4"
>>> +	depends on EXYNOS4_DEV_TMU
>>
>> It doesn't look like that Kconfig symbol is part of the tree just yet.
>> That means people will not be able to build this driver from the
>> mainline tree. Why is this dependency needed? In a (rather quick) scan
>> of the code of this driver I couldn't spot anything not yet available in
>> the tree.
>>
> I have to defer to the driver author for that. Maybe the dependency was
> renamed at some point, or removed altogether.
> 
The dependency will be renamed to 'ARCH_EXYNOS4'.

Thanks,
Donggeun

Re: [PATCH v7] hwmon: Add driver for EXYNOS4 TMU

[Guenter Roeck]

On Thu, Nov 17, 2011 at 12:20:31AM -0500, Donggeun Kim wrote:
[ ... ]
> >>>  
> >>> +config SENSORS_EXYNOS4_TMU
> >>> +	tristate "Temperature sensor on Samsung EXYNOS4"
> >>> +	depends on EXYNOS4_DEV_TMU
> >>
> >> It doesn't look like that Kconfig symbol is part of the tree just yet.
> >> That means people will not be able to build this driver from the
> >> mainline tree. Why is this dependency needed? In a (rather quick) scan
> >> of the code of this driver I couldn't spot anything not yet available in
> >> the tree.
> >>
> > I have to defer to the driver author for that. Maybe the dependency was
> > renamed at some point, or removed altogether.
> > 
> The dependency will be renamed to 'ARCH_EXYNOS4'.
> 
Please submit a patch.

Thanks,
Guenter