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

[Keerthy]

On Wednesday 14 September 2016 05:26 PM, Przemyslaw Marczak wrote:
> 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.

okay.

>
> Best regards,