| Message ID | 4F734460.4060605@samsung.com |
|---|---|
| State | New |
| Headers | show |
On 28 March 2012 22:33, Karol Lewandowski <k.lewandowsk@samsung.com> wrote: > On 24.03.2012 10:49, Thomas Abraham wrote: > > Hi Thomas! > >> Add device tree based discovery support for max8997. > ... >> +Regulators: The regulators of max8997 that have to be instantiated should be >> +included in a sub-node named 'regulators'. Regulator nodes included in this >> +sub-node should be of the format as below. Note: The 'n' in LDOn and BUCKn >> +represents the LDO or BUCK number as per the datasheet of max8997. >> + >> + For LDO's: >> + LDOn { >> + standard regulator bindings here >> + }; >> + >> + For BUCK's: >> + BUCKn { >> + standard regulator bindings here >> + }; >> + > > > Small note - driver supports[1] configuring following regulators by > using respective DT node names: > > - EN32KHz_AP > - EN32KHz_CP > - ENVICHG > - ESAFEOUT1 > - ESAFEOUT2 > - CHARGER > - CHARGER_CV > - CHARGER_TOPOFF > > I wonder if these should be mentioned in documentation too. > > [1] These are used in e.g. mach-nuri.c Yes, I missed the above regulators in the documentation. I have included them now and will resubmit this patch. > >> diff --git a/drivers/regulator/max8997.c b/drivers/regulator/max8997.c> index 9657929..dce8aaf 100644 > >> --- a/drivers/regulator/max8997.c >> +++ b/drivers/regulator/max8997.c > .. >> +static int max8997_pmic_dt_parse_pdata(struct max8997_dev *iodev, >> + struct max8997_platform_data *pdata) >> +{ >> + struct device_node *pmic_np, *regulators_np, *reg_np; >> + struct max8997_regulator_data *rdata; >> + unsigned int i, dvs_voltage_nr = 1, ret; >> + >> + pmic_np = iodev->dev->of_node; >> + if (!pmic_np) { >> + dev_err(iodev->dev, "could not find pmic sub-node\n"); >> + return -ENODEV; >> + } >> + >> + regulators_np = of_find_node_by_name(pmic_np, "regulators"); >> + if (!regulators_np) { >> + dev_err(iodev->dev, "could not find regulators sub-node\n"); >> + return -EINVAL; >> + } >> + >> + /* count the number of regulators to be supported in pmic */ >> + pdata->num_regulators = 0; >> + for_each_child_of_node(regulators_np, reg_np) >> + pdata->num_regulators++; >> + >> + rdata = devm_kzalloc(iodev->dev, sizeof(*rdata) * >> + pdata->num_regulators, GFP_KERNEL); >> + if (!rdata) { >> + dev_err(iodev->dev, "could not allocate memory for " >> + "regulator data\n"); >> + return -ENOMEM; >> + } > >> + pdata->regulators = rdata; > >> + for_each_child_of_node(regulators_np, reg_np) { >> + for (i = 0; i < ARRAY_SIZE(regulators); i++) >> + if (!of_node_cmp(reg_np->name, regulators[i].name)) >> + break; >> + rdata->id = i; > > > rdata->id will be equal to ARRAY_SIZE(regulators) when one adds DT node > name (below "regulators") which is different from what can be found in > regulators[] table. > > On my test machine this results in hard lockup - possibly because > something tries to access regulators[ARRAY_SIZE(regulators)] > later on. > > Following patch fixes this on my machine (using DTS with misspelled LDO1 for LDx1): > > diff --git a/drivers/regulator/max8997.c b/drivers/regulator/max8997.c > index dce8aaf..c20fd72 100644 > --- a/drivers/regulator/max8997.c > +++ b/drivers/regulator/max8997.c > @@ -1011,6 +1011,13 @@ static int max8997_pmic_dt_parse_pdata(struct max8997_dev *iodev, > for (i = 0; i < ARRAY_SIZE(regulators); i++) > if (!of_node_cmp(reg_np->name, regulators[i].name)) > break; > + > + if (i == ARRAY_SIZE(regulators)) { > + dev_warn(iodev->dev, "don't know how to configure regulator %s\n", > + reg_np->name); > + continue; > + } > + > rdata->id = i; > rdata->initdata = of_get_regulator_init_data( > iodev->dev, reg_np); > Thanks for this fix. I have merged this change into this patch. Regards, Thomas. > > Regards, > -- > Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform
On Wed, Apr 18, 2012 at 12:05:59AM +0530, Thomas Abraham wrote: > On 28 March 2012 22:33, Karol Lewandowski <k.lewandowsk@samsung.com> wrote: > >> + For BUCK's: No 's here, BTW. > > - EN32KHz_AP > > - EN32KHz_CP > > - ENVICHG > > - ESAFEOUT1 > > - ESAFEOUT2 > > - CHARGER > > - CHARGER_CV > > - CHARGER_TOPOFF > > I wonder if these should be mentioned in documentation too. > Yes, I missed the above regulators in the documentation. I have > included them now and will resubmit this patch. Please omit the clocks; these are obviously a bodge due to the inability to support clocks off-SoC so we shouldn't be enshrining them in the device tree bindings.
Hi Mark, On 18 April 2012 00:08, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Wed, Apr 18, 2012 at 12:05:59AM +0530, Thomas Abraham wrote: >> On 28 March 2012 22:33, Karol Lewandowski <k.lewandowsk@samsung.com> wrote: > >> >> + For BUCK's: > > No 's here, BTW. Ok. > >> > - EN32KHz_AP >> > - EN32KHz_CP >> > - ENVICHG >> > - ESAFEOUT1 >> > - ESAFEOUT2 >> > - CHARGER >> > - CHARGER_CV >> > - CHARGER_TOPOFF > >> > I wonder if these should be mentioned in documentation too. > >> Yes, I missed the above regulators in the documentation. I have >> included them now and will resubmit this patch. > > Please omit the clocks; these are obviously a bodge due to the inability > to support clocks off-SoC so we shouldn't be enshrining them in the > device tree bindings. Thanks for the suggestion. I have removed EN32KHz_AP and EN32KHz_CP from the list. The rest are either voltage (fixed) or current regulators. Regards, Thomas.
diff --git a/drivers/regulator/max8997.c b/drivers/regulator/max8997.c index dce8aaf..c20fd72 100644 --- a/drivers/regulator/max8997.c +++ b/drivers/regulator/max8997.c @@ -1011,6 +1011,13 @@ static int max8997_pmic_dt_parse_pdata(struct max8997_dev *iodev, for (i = 0; i < ARRAY_SIZE(regulators); i++) if (!of_node_cmp(reg_np->name, regulators[i].name)) break; + + if (i == ARRAY_SIZE(regulators)) { + dev_warn(iodev->dev, "don't know how to configure regulator %s\n", + reg_np->name); + continue; + } + rdata->id = i; rdata->initdata = of_get_regulator_init_data( iodev->dev, reg_np);