| Message ID | 1326899836-14470-1-git-send-email-ldewangan@nvidia.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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?
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. -- 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
> 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 -- 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
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. -- 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
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. -- 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
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. -- 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
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 -- 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
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. -- 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
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, }, };
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(-)