Re: [PATCH 2/4] OMAP: OPP: twl/tps: Introduce TWL/TPS-specific code

[Nishanth Menon <nm@ti.com>]

Gopinath, Thara had written, on 09/16/2010 08:51 AM, the following:
> 
>>> -----Original Message-----
>>> From: Menon, Nishanth
>>> Sent: Thursday, September 16, 2010 5:45 PM
>>> To: Gopinath, Thara
>>> Cc: Kevin Hilman; linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org
>>> Subject: Re: [PATCH 2/4] OMAP: OPP: twl/tps: Introduce TWL/TPS-specific code
>>>
>>> Gopinath, Thara had written, on 09/16/2010 05:40 AM, the following:
>>>>>> -----Original Message-----
>>>>>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of
>>> Kevin
>>>>>> Hilman
>>>>>> Sent: Thursday, September 16, 2010 3:27 AM
>>>>>> To: linux-omap@vger.kernel.org
>>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>>> Subject: [PATCH 2/4] OMAP: OPP: twl/tps: Introduce TWL/TPS-specific code
>>>>>>
>>>>>> From: Paul Walmsley <paul@pwsan.com>
>>>>>>
>>>>>> The OPP layer code should be independent of the PMIC,
>>>>>> introduce the TWL/TPS-specific code out to its own file.
>>>> Hello Kevin,
>>>>
>>>> I have been using this code for a while now. I really do not think wee need a separate
>>>> file for implementing the vsel to voltage in (uV) and vice versa formulas. Today only voltage
>>> This split introduces a PMIC level abstraction already. Do you have a
>>> suggestion which file it should go to? It is definitely not part of
>>> opp.c, not part of other existing twl files as well. the job of this
>>> file was to introduce conversion routines which can be used by any layer
>>> (voltage layer if need be - it used to be srf and smartreflex before)..
>>> in fact one of your voltage layer patches introduces capability for 6030
>>> as well
>>> http://marc.info/?l=linux-omap&m=128213020927919&w=2
> 
> Yes one of my patches has introduces this coz I had no other way
> to add OMAP4 support. But I still do not understand why cant these
> APIs be implemented in twl-core.c or twl4030-power.c? 
Why there? Twl power does regulator operations not conversion 
operations. core is not the place either as it is function independent.

> 
>>>> layer is interested in these conversions. Voltage layer has a structure that can be populated with
>>>> the information required from the PMIC. We only need to add two more function pointers to this
>>> structure.
>>>> This info can then be passed from the actual PMIC driver file. This
>>> will make it much
>>>> more simpler for OMAP4 where we have different formulas between different revisions of PMIC. Also
>>>> in the omap voltage code we will no longer have to hard code omap_twl_vsel_to_uv and
>>> omap_twl_uv_to_vsel.
>>> I think the problem is with the voltage layer (which has not been posted
>>> upstream) which is using hard coded function pointer. What the patchset
>>> should have done is to introduce function pointers registration from
>>> twl_tps.c to voltage layer and voltage layer should ideally been using
>>> function pointers by itself.
>>>
>>>> So please consider dropping this patch from this series.
>>> I think I disagree - rationale for having this separated as a pmic
>>> specific file is still sound, only the implementation of the future
>>> framework should have changed (it should be using function pointers
>>> instead of hardcoded function names). in fact I can add additional
>>> suggestion for the voltage layer: the pmic selection should be done from
>>> a board file - This will allow voltage layer to handle numerous PMICs
>>> and combination of PMICs controlling various domains as well.. the only
>>> neat way to handle it is ofcourse using function pointers.
> 
> Exactly if voltage layer has to use a func ptr, why not let them be populated
> by the PMIC driver files directly?
Again, we are going into the details of how voltage layer will be 
implemented.. What suggestion do you do when on board x vdd1 is driven 
by PMIC1, vdd2 by PMIC2? it will have to be somesort of different 
registration mechanism. but nothing prevents the framework from 
registering the pointers defined in twl_tps.c



> 
>>>
>>> PS: Suggestion
>>> - please fix your mailer to round off for 70/80 char.
> 
> Will try.
> 
>>> - might be good to point folks to rfc patchset for voltage layer to give
>>> context.
> 
> https://patchwork.kernel.org/patch/119429/

thanks..

-- 
Regards,
Nishanth Menon