[RFC PATCH 10/11] omap4: thermal: add basic CPU thermal zone

[Valentin, Eduardo]

Hello Santosh,

On Mon, May 28, 2012 at 12:48 PM, Felipe Balbi <balbi at ti.com> wrote:
> Hi,
>
> On Mon, May 28, 2012 at 03:03:26PM +0530, Shilimkar, Santosh wrote:
>> On Fri, May 25, 2012 at 1:56 PM, Eduardo Valentin
>> <eduardo.valentin at ti.com> wrote:
>> > This patch exposes OMAP4 thermal sensor as a thermal zone
>> > named "cpu". Only thermal creation is done here.
>> >
>> > TODO:
>> >
>> >  - Add cooling bindings
>> >  - Add extrapolation rules
>> >
>> > Signed-off-by: Eduardo Valentin <eduardo.valentin at ti.com>
>> > ---
>> >  drivers/thermal/Kconfig         |   12 ++++++
>> >  drivers/thermal/Makefile        |    1 +
>> >  drivers/thermal/omap-bandgap.c  |    1 +
>> >  drivers/thermal/omap-bandgap.h  |   12 ++++++
>> >  drivers/thermal/omap4-thermal.c |   72 +++++++++++++++++++++++++++++++++++++++
>> >  5 files changed, 98 insertions(+), 0 deletions(-)
>> >  create mode 100644 drivers/thermal/omap4-thermal.c
>> >
>> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> > index ffdd240..2e82797 100644
>> > --- a/drivers/thermal/Kconfig
>> > +++ b/drivers/thermal/Kconfig
>> > @@ -39,3 +39,15 @@ config OMAP_BANDGAP
>> >          This includes alert interrupts generation and also the TSHUT
>> >          support.
>> >
>> > +config OMAP4_THERMAL
>> > +       bool "Texas Instruments OMAP4 thermal support"
>> > +       depends on OMAP_BANDGAP
>> > +       depends on ARCH_OMAP4
>> > +       help
>> > +         If you say yes here you get thermal support for the Texas Instruments
>> > +         OMAP4 SoC family. The current chip supported are:
>> > +          - OMAP4460
>> > +
>> It's more of IP feature than OMAP specific, so something like
>>
>> config  HAVE_BANDGAP_THERMAL_SUPPORT
>>
>> and then let processor's which support enable it. That OMAP varients
>> in AMXX etc if needed can make use of it.
>
> Those are just an OMAP in disguise.
>
>> If you agree, then rest of the driver also can be cleaned to avoid
>> omap_* in file names and variables.
>>
>> Apart from this minor comment, rest of the patch looks fine to me.
>
> Then it will appear as a fully generic bandgap driver, which is not the
> case. This is really a TI thing, right ?

I have to agree with Felipe here. I do see your point to have a name
which is applicable to AMxx, but having such generic naming is not
helping either :-( as this bandgap driver is not supposed to be
generic.

I though of "ti_bandgap.*", but still seams to be misleading..

I guess same applies to usb_phy and scm.

Again, if you have a name that fits better I won't hesitate to use it :-)

>
> --
> balbi



-- 

Eduardo Valentin