| Message ID | 1473827299-29944-2-git-send-email-j-keerthy@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
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@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() Best regards,
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@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,
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@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,
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@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,
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 */
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@ti.com> --- include/power/regulator.h | 2 ++ 1 file changed, 2 insertions(+)