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

2016-09-15 Thread 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 
---
  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,

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2016-09-15 Thread 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 
---
  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 Institute Poland
Samsung Electronics
p.marc...@samsung.com

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2016-09-14 Thread Przemyslaw Marczak 

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 
---
  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,
--
Przemyslaw Marczak
Samsung R Institute Poland
Samsung Electronics
p.marc...@samsung.com
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2016-09-14 Thread Keerthy 

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 
---
  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,

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2016-09-13 Thread Keerthy 
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 
---
 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 */
-- 
1.9.1

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot