[U-Boot] [PATCH 1/5] power: regulator: Add ctrl_reg and volt_reg fields for pmic

[Przemyslaw Marczak]

Hello Keerthy,

On 09/14/2016 10:24 AM, Keerthy wrote:
> Hi Marczak,
>
> On Wednesday 14 September 2016 01:33 PM, Przemyslaw Marczak wrote:
>> Hello Keerthy,
>>
>> On 09/14/2016 06:28 AM, Keerthy wrote:
>>> The ctrl reg contains bit fields to enable and disable regulators,
>>> and volt_reg has the bit fields to configure the voltage values.
>>> The registers are frequently accessed hence make them part
>>> of dm_regulator_uclass_platdata structure.
>>>
>>> Signed-off-by: Keerthy <j-keerthy at ti.com>
>>> ---
>>>   include/power/regulator.h | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/include/power/regulator.h b/include/power/regulator.h
>>> index 9bcd728..57b14a3 100644
>>> --- a/include/power/regulator.h
>>> +++ b/include/power/regulator.h
>>> @@ -171,6 +171,8 @@ struct dm_regulator_uclass_platdata {
>>>       bool boot_on;
>>>       const char *name;
>>>       int flags;
>>> +    u8 ctrl_reg;
>>> +    u8 volt_reg;
>>>   };
>>>     /* Regulator device operations */
>>
>> This structure above is used for some common "high-level" data, which
>> can be used by regulator uclass driver.
>>
>> Even if most of PMICs has some ctrl/volt/etc regs, the regulator uclass
>> driver doesn't know, how to use it, so from this point of view it is
>> useless.
>>
>> But, you can keep device/driver data in a proper fields. Please look at
>> those files:
>>
>> drivers/power/regulator/fixed.c:119
>> drivers/power/regulator/pfuze100.c:567
>>
>> To store some device internal data, you can use:
>> .platdata_auto_alloc_size -> with access by dev_get_platdata()
>> .priv_auto_alloc_size -> with access by dev_get_priv()
>
> Thanks for a quick review. I did look at some of those options before 
> introducing volt and ctrl here.
>
> Many PMICs will have ctrl/volt registers we might end up having lot of 
> private strutures with the same ctrl/volt fields. I agree uclass 
> driver will not know how to use it.
>
> If i have to draw parallels from the kernel world regulator_desc is a 
> common structure which hosts vsel_reg/enable_reg fields.
>
> Isn't it better to have a common structure instead of having a some 
> platform specific structure that might have the same fields?
>
> Let me know your thoughts on this.
>
>>
>> Best regards,
>
>
>

You are right and I agree with you that make things common is a good 
approach.

At the begin of introducing this framework, I just wanted to provide a 
simple user interface for regulators, so I didn't tried to put all 
common things into a single structure, because not always it could be 
useful.

The present structure layout seems to be a good representation of 
regulator's node in the device-tree.

In the other hand, each driver can provide a static arrays with proper 
data like reg/mask/etc,
the driver:
drivers/power/regulator/s5m8767.c- is a good example.

I'm not going to break your solution, but let's wait for Simon's opinion.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com