[PATCH V1 2/2] regulator: tps65910: Initialize n_voltages for rails.

[PATCH V1 2/2] regulator: tps65910: Initialize n_voltages for rails.

[Laxman Dewangan]

Initializing the number of voltages supported by different
rails of pmic device tps65911.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/regulator/tps65910-regulator.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/tps65910-regulator.c b/drivers/regulator/tps65910-regulator.c
index 6742418..3ac5f91 100644
--- a/drivers/regulator/tps65910-regulator.c
+++ b/drivers/regulator/tps65910-regulator.c
@@ -188,56 +188,67 @@ static struct tps_info tps65911_regs[] = {
 		.name = "VDD1",
 		.min_uV = 600000,
 		.max_uV = 4500000,
+		.table_len = 73,
 	},
 	{
 		.name = "VDD2",
 		.min_uV = 600000,
 		.max_uV = 4500000,
+		.table_len = 73,
 	},
 	{
 		.name = "VDDCTRL",
 		.min_uV = 600000,
 		.max_uV = 1400000,
+		.table_len = 65,
 	},
 	{
 		.name = "LDO1",
 		.min_uV = 1000000,
 		.max_uV = 3300000,
+		.table_len = 47,
 	},
 	{
 		.name = "LDO2",
 		.min_uV = 1000000,
 		.max_uV = 3300000,
+		.table_len = 47,
 	},
 	{
 		.name = "LDO3",
 		.min_uV = 1000000,
 		.max_uV = 3300000,
+		.table_len = 24,
 	},
 	{
 		.name = "LDO4",
 		.min_uV = 1000000,
 		.max_uV = 3300000,
+		.table_len = 47,
 	},
 	{
 		.name = "LDO5",
 		.min_uV = 1000000,
 		.max_uV = 3300000,
+		.table_len = 24,
 	},
 	{
 		.name = "LDO6",
 		.min_uV = 1000000,
 		.max_uV = 3300000,
+		.table_len = 24,
 	},
 	{
 		.name = "LDO7",
 		.min_uV = 1000000,
 		.max_uV = 3300000,
+		.table_len = 24,
 	},
 	{
 		.name = "LDO8",
 		.min_uV = 1000000,
 		.max_uV = 3300000,
+		.table_len = 24,
 	},
 };
 
-- 
1.7.1.1

RE: [PATCH V1 2/2] regulator: tps65910: Initialize n_voltages for rails.

[Stephen Warren]

Laxman Dewangan wrote at Wednesday, January 18, 2012 8:17 AM:
> Initializing the number of voltages supported by different
> rails of pmic device tps65911.
...
> diff --git a/drivers/regulator/tps65910-regulator.c b/drivers/regulator/tps65910-regulator.c
...
> @@ -188,56 +188,67 @@ static struct tps_info tps65911_regs[] = {
>  		.name = "VDD1",
>  		.min_uV = 600000,
>  		.max_uV = 4500000,
> +		.table_len = 73,
>  	},

Forgive me if these comments are bogus since I'm not familiar with
regulators much, but:

a) Don't you want to use ARRAY_SIZE() instead of hard-coding table_len's
value?

b) Don't you need to set .table to actually point at the table too? If not,
isn't tps65910_list_voltage() going to dereference a NULL pointer, since
it will dereference .table if .table_len is set?

-- 
nvpublic

Re: [PATCH V1 2/2] regulator: tps65910: Initialize n_voltages for rails.

[Mark Brown]

On Wed, Jan 18, 2012 at 09:54:27AM -0800, Stephen Warren wrote:
> Laxman Dewangan wrote at Wednesday, January 18, 2012 8:17 AM:

> > +		.table_len = 73,
> >  	},

> Forgive me if these comments are bogus since I'm not familiar with
> regulators much, but:

> a) Don't you want to use ARRAY_SIZE() instead of hard-coding table_len's
> value?

> b) Don't you need to set .table to actually point at the table too? If not,
> isn't tps65910_list_voltage() going to dereference a NULL pointer, since
> it will dereference .table if .table_len is set?

It's not actually a table at all any more as the driver calculates the
values, table_len is misnamed and should be renamed as well as set.

RE: [PATCH V1 2/2] regulator: tps65910: Initialize n_voltages for rails.

[Laxman Dewangan]

> On Wednesday, January 18, 2012 11:26 PM, Mark Brown wrote:
> 
> > > +		.table_len = 73,
> > >  	},
> 
> > Forgive me if these comments are bogus since I'm not familiar with
> > regulators much, but:
> 
> > a) Don't you want to use ARRAY_SIZE() instead of hard-coding table_len's
> > value?
> 
> > b) Don't you need to set .table to actually point at the table too? If not,
> > isn't tps65910_list_voltage() going to dereference a NULL pointer, since
> > it will dereference .table if .table_len is set?
> 
> It's not actually a table at all any more as the driver calculates the
> values, table_len is misnamed and should be renamed as well as set.


Do you want to rename the variable in this patch or in separate patch?
In place of table_len, I will say n_voltages as this is most common name
in regulator world.


> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V1 2/2] regulator: tps65910: Initialize n_voltages for rails.

[Mark Brown]

On Thu, Jan 19, 2012 at 08:54:19AM +0530, Laxman Dewangan wrote:

> Do you want to rename the variable in this patch or in separate patch?
> In place of table_len, I will say n_voltages as this is most common name
> in regulator world.

Probably two is clearer, though if it's not set already for any of the
regulators just one should be fine.

RE: [PATCH V1 2/2] regulator: tps65910: Initialize n_voltages for rails.

[Laxman Dewangan]

On Thursday 19 January 2012 04:10 PM, Mark Brown wrote:
> > Do you want to rename the variable in this patch or in separate patch?
> > In place of table_len, I will say n_voltages as this is most common name
> > in regulator world.
> 
> Probably two is clearer, though if it's not set already for any of the
> regulators just one should be fine.

The table_len is already used in some of entry i.e. for tps65910. I filled
entry for another device tps65911. So I would like to go for separate patch
for renaming the variables "table" to "voltage_table" and "table_len" to
"n_voltages".
Please let me know your opinion.

Re: [PATCH V1 2/2] regulator: tps65910: Initialize n_voltages for rails.

[Mark Brown]

On Thu, Jan 19, 2012 at 04:25:17PM +0530, Laxman Dewangan wrote:
> On Thursday 19 January 2012 04:10 PM, Mark Brown wrote:

> The table_len is already used in some of entry i.e. for tps65910. I filled
> entry for another device tps65911. So I would like to go for separate patch
> for renaming the variables "table" to "voltage_table" and "table_len" to
> "n_voltages".
> Please let me know your opinion.

That's fine.  If in doubt and it doesn't introduce build breakage
splitting up into more patches is rarely a problem.

RE: [PATCH V1 2/2] regulator: tps65910: Initialize n_voltages for rails.

[Laxman Dewangan]

On Thursday 19 January 2012 04:31 PM, Mark Brown wrote:

> > The table_len is already used in some of entry i.e. for tps65910. I filled
> > entry for another device tps65911. So I would like to go for separate patch
> > for renaming the variables "table" to "voltage_table" and "table_len" to
> > "n_voltages".
> > Please let me know your opinion.
> 
> That's fine.  If in doubt and it doesn't introduce build breakage
> splitting up into more patches is rarely a problem.


Ok, So to avoid the mess-ups, I will fetch the code from your tree 
http://git.kernel.org/?p=linux/kernel/git/broonie/regulator.git;a=shortlog;h=refs/heads/for-next-next
once you apply this change and then push the variable name change in new patch.

Laxman

Re: [PATCH V1 2/2] regulator: tps65910: Initialize n_voltages for rails.

[Mark Brown]

On Wed, Jan 18, 2012 at 08:47:16PM +0530, Laxman Dewangan wrote:
> Initializing the number of voltages supported by different
> rails of pmic device tps65911.

Applied, thanks.