| Message ID | 1414668053-31370-7-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Krzysztof, On 10/30/2014 12:20 PM, Krzysztof Kozlowski wrote: > static int max77802_pmic_probe(struct platform_device *pdev) > { > struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent); > - struct max77686_platform_data *pdata = dev_get_platdata(iodev->dev); > struct max77802_regulator_prv *max77802; > - int i, ret = 0, val; > + int i, val; > struct regulator_config config = { }; > > - /* This is allocated by the MFD driver */ > - if (!pdata) { > - dev_err(&pdev->dev, "no platform data found for regulator\n"); > - return -ENODEV; > - } > - > max77802 = devm_kzalloc(&pdev->dev, > sizeof(struct max77802_regulator_prv), > GFP_KERNEL); > if (!max77802) > return -ENOMEM; > > - if (iodev->dev->of_node) { > - ret = max77802_pmic_dt_parse_pdata(pdev, pdata); > - if (ret) > - return ret; > + if (!pdev->dev.of_node) { > + /* > + * Backward DTS compatiblity where regulator driver had not > + * a compatible property for itself. > + */ > + if (!iodev->dev->of_node) { > + dev_err(&pdev->dev, "No OF node for driver and its parent\n"); > + return -EINVAL; > + } > + pdev->dev.of_node = of_get_child_by_name(iodev->dev->of_node, > + "regulators"); > + max77802->missing_of_node = true; > } I may be missing something but I don't understand why a compatible string for the regulators sub-node is needed. Isn't enough to just fill the .regulators_node field in the struct regulator_desc? e.g: .regulators_node = of_match_ptr("regulators") for max77802 .regulators_node = of_match_ptr("voltage-regulators") for max77686 AFAIU this should be enough for the core to extract the init_data and will make your change much more simpler and you can drop patches 1-3 and 13-14. Or maybe I misread the regulator_of_get_init_data() function? Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On czw, 2014-10-30 at 12:50 +0100, Javier Martinez Canillas wrote: > Hello Krzysztof, > > On 10/30/2014 12:20 PM, Krzysztof Kozlowski wrote: > > static int max77802_pmic_probe(struct platform_device *pdev) > > { > > struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent); > > - struct max77686_platform_data *pdata = dev_get_platdata(iodev->dev); > > struct max77802_regulator_prv *max77802; > > - int i, ret = 0, val; > > + int i, val; > > struct regulator_config config = { }; > > > > - /* This is allocated by the MFD driver */ > > - if (!pdata) { > > - dev_err(&pdev->dev, "no platform data found for regulator\n"); > > - return -ENODEV; > > - } > > - > > max77802 = devm_kzalloc(&pdev->dev, > > sizeof(struct max77802_regulator_prv), > > GFP_KERNEL); > > if (!max77802) > > return -ENOMEM; > > > > - if (iodev->dev->of_node) { > > - ret = max77802_pmic_dt_parse_pdata(pdev, pdata); > > - if (ret) > > - return ret; > > + if (!pdev->dev.of_node) { > > + /* > > + * Backward DTS compatiblity where regulator driver had not > > + * a compatible property for itself. > > + */ > > + if (!iodev->dev->of_node) { > > + dev_err(&pdev->dev, "No OF node for driver and its parent\n"); > > + return -EINVAL; > > + } > > + pdev->dev.of_node = of_get_child_by_name(iodev->dev->of_node, > > + "regulators"); > > + max77802->missing_of_node = true; > > } > > I may be missing something but I don't understand why a compatible string > for the regulators sub-node is needed. Isn't enough to just fill the > .regulators_node field in the struct regulator_desc? e.g: > > .regulators_node = of_match_ptr("regulators") for max77802 > .regulators_node = of_match_ptr("voltage-regulators") for max77686 > > AFAIU this should be enough for the core to extract the init_data and will > make your change much more simpler and you can drop patches 1-3 and 13-14. > > Or maybe I misread the regulator_of_get_init_data() function? The regulator_of_get_init_data() searches from dev->of_node or its child node. But dev->of_node is NULL. That's why of_compatible is needed. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Krzysztof, On 10/30/2014 01:10 PM, Krzysztof Kozlowski wrote: >> >> I may be missing something but I don't understand why a compatible string >> for the regulators sub-node is needed. Isn't enough to just fill the >> .regulators_node field in the struct regulator_desc? e.g: >> >> .regulators_node = of_match_ptr("regulators") for max77802 >> .regulators_node = of_match_ptr("voltage-regulators") for max77686 >> >> AFAIU this should be enough for the core to extract the init_data and will >> make your change much more simpler and you can drop patches 1-3 and 13-14. >> >> Or maybe I misread the regulator_of_get_init_data() function? > > The regulator_of_get_init_data() searches from dev->of_node or its child > node. > > But dev->of_node is NULL. > > That's why of_compatible is needed. Yes but regulator_register() does dev = config->dev and config->dev is set to config.dev = iodev->dev in the driver probe function which is the pdev->dev.parent (the PMIC struct device) that has an associated of_node. So, regulator_of_get_init_data() will call of_get_child_by_name() passing the PMIC of_node and the sub-node name that contains the regulators. That is, whatever was set in desc->regulators_node and that should be enough. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On czw, 2014-10-30 at 13:21 +0100, Javier Martinez Canillas wrote: > Hello Krzysztof, > > On 10/30/2014 01:10 PM, Krzysztof Kozlowski wrote: > >> > >> I may be missing something but I don't understand why a compatible string > >> for the regulators sub-node is needed. Isn't enough to just fill the > >> .regulators_node field in the struct regulator_desc? e.g: > >> > >> .regulators_node = of_match_ptr("regulators") for max77802 > >> .regulators_node = of_match_ptr("voltage-regulators") for max77686 > >> > >> AFAIU this should be enough for the core to extract the init_data and will > >> make your change much more simpler and you can drop patches 1-3 and 13-14. > >> > >> Or maybe I misread the regulator_of_get_init_data() function? > > > > The regulator_of_get_init_data() searches from dev->of_node or its child > > node. > > > > But dev->of_node is NULL. > > > > That's why of_compatible is needed. > > Yes but regulator_register() does dev = config->dev and config->dev is set > to config.dev = iodev->dev in the driver probe function which is the > pdev->dev.parent (the PMIC struct device) that has an associated of_node. > > So, regulator_of_get_init_data() will call of_get_child_by_name() passing > the PMIC of_node and the sub-node name that contains the regulators. That > is, whatever was set in desc->regulators_node and that should be enough. I missed that one in max77802 (in max77686: config.dev = &pdev->dev). Now I wonder if it is proper to attach regulators to driver's parent device. Consider regulator_register, around line 3640: rdev->dev.parent = dev; The parent of regulators will be equal to parent of regulator driver - main MFD driver. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Krzysztof, On 10/30/2014 01:30 PM, Krzysztof Kozlowski wrote: tor_of_get_init_data() function? >> > >> > The regulator_of_get_init_data() searches from dev->of_node or its child >> > node. >> > >> > But dev->of_node is NULL. >> > >> > That's why of_compatible is needed. >> >> Yes but regulator_register() does dev = config->dev and config->dev is set >> to config.dev = iodev->dev in the driver probe function which is the >> pdev->dev.parent (the PMIC struct device) that has an associated of_node. >> >> So, regulator_of_get_init_data() will call of_get_child_by_name() passing >> the PMIC of_node and the sub-node name that contains the regulators. That >> is, whatever was set in desc->regulators_node and that should be enough. > > I missed that one in max77802 (in max77686: config.dev = &pdev->dev). > Now I wonder if it is proper to attach regulators to driver's parent > device. Consider regulator_register, around line 3640: > rdev->dev.parent = dev; > The parent of regulators will be equal to parent of regulator driver - > main MFD driver. > An early version of the max77802 regulator driver also set config.dev to &pdev->dev but then Mark asked me if I was sure that it should not be the MFD device instead. I then read in regulator_register() that the rdev->dev.parent was set to the device passed in config.dev as you said so I was convinced that it should be the parent device instead. Please take a look at [0] for that discussion. Best regards, Javier [0]: https://lkml.org/lkml/2014/6/17/174 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On czw, 2014-10-30 at 13:42 +0100, Javier Martinez Canillas wrote: > Hello Krzysztof, > > On 10/30/2014 01:30 PM, Krzysztof Kozlowski wrote: > tor_of_get_init_data() function? > >> > > >> > The regulator_of_get_init_data() searches from dev->of_node or its child > >> > node. > >> > > >> > But dev->of_node is NULL. > >> > > >> > That's why of_compatible is needed. > >> > >> Yes but regulator_register() does dev = config->dev and config->dev is set > >> to config.dev = iodev->dev in the driver probe function which is the > >> pdev->dev.parent (the PMIC struct device) that has an associated of_node. > >> > >> So, regulator_of_get_init_data() will call of_get_child_by_name() passing > >> the PMIC of_node and the sub-node name that contains the regulators. That > >> is, whatever was set in desc->regulators_node and that should be enough. > > > > I missed that one in max77802 (in max77686: config.dev = &pdev->dev). > > Now I wonder if it is proper to attach regulators to driver's parent > > device. Consider regulator_register, around line 3640: > > rdev->dev.parent = dev; > > The parent of regulators will be equal to parent of regulator driver - > > main MFD driver. > > > > An early version of the max77802 regulator driver also set config.dev to > &pdev->dev but then Mark asked me if I was sure that it should not be the > MFD device instead. > > I then read in regulator_register() that the rdev->dev.parent was set > to the device passed in config.dev as you said so I was convinced that it > should be the parent device instead. > > Please take a look at [0] for that discussion. To me a intuitive structure would be: MFD device | - clock device | - clock1 - clock2 - regulator device | - LDO1 - LDO2 etc. This also maps to structure in DTS. dev_err* messages and any allocations should be done on behalf of regulator device, not parent. Various drivers do this differently... The wm8* drivers set it mostly to parent (MFD)... I do not insists, especially because using parent's device would make this driver simpler. Mark, maybe you could shed light on it? Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Krzysztof, On 10/30/2014 01:53 PM, Krzysztof Kozlowski wrote: > > To me a intuitive structure would be: > MFD device > | > - clock device > | > - clock1 > - clock2 > - regulator device > | > - LDO1 > - LDO2 > etc. > > This also maps to structure in DTS. dev_err* messages and any > allocations should be done on behalf of regulator device, not parent. > > Various drivers do this differently... The wm8* drivers set it mostly to > parent (MFD)... > > I do not insists, especially because using parent's device would make > this driver simpler. > Another reason to set it to the parent is to lookup the regulator input supply node. The regulator_register() function does: if (supply) { struct regulator_dev *r; r = regulator_dev_lookup(dev, supply, &ret); where dev = config->dev so that will determine on which device node your input supplies have to be defined. For example on the Peach Pit DTS has this: max77802: max77802-pmic@9 { ... inb1-supply = <&tps65090_dcdc2>; ... inb10-supply = <&tps65090_dcdc1>; inl1-supply = <&buck5_reg>; ... inl10-supply = <&buck7_reg>; ... regulators { ... }; }; which is how most (all?) DTS define the input supplies. If you instead do config.dev = &pdev->dev, then the input supplies have to be in the "regulators" node which is not the standard AFAICT. This is not a problem for max77686 because I see that DTS don't define the input supplies but I guess that is because the power scheme is not completely modeled. > Mark, maybe you could shed light on it? > > > Best regards, > Krzysztof > > Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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/max77802.c b/drivers/regulator/max77802.c index 60daca2028e9..2cb938610230 100644 --- a/drivers/regulator/max77802.c +++ b/drivers/regulator/max77802.c @@ -70,7 +70,9 @@ static unsigned int ramp_table_77802_4bit[] = { }; struct max77802_regulator_prv { + /* Array indexed by regulator id */ unsigned int opmode[MAX77802_REG_MAX]; + bool missing_of_node; }; static inline int max77802_map_mode(int mode) @@ -362,6 +364,7 @@ static struct regulator_ops max77802_buck_dvs_ops = { /* LDOs 3-7, 9-14, 18-26, 28, 29, 32-34 */ #define regulator_77802_desc_p_ldo(num, supply, log) { \ .name = "LDO"#num, \ + .of_match = of_match_ptr("LDO"#num), \ .id = MAX77802_LDO##num, \ .supply_name = "inl"#supply, \ .ops = &max77802_ldo_ops_logic##log, \ @@ -380,6 +383,7 @@ static struct regulator_ops max77802_buck_dvs_ops = { /* LDOs 1, 2, 8, 15, 17, 27, 30, 35 */ #define regulator_77802_desc_n_ldo(num, supply, log) { \ .name = "LDO"#num, \ + .of_match = of_match_ptr("LDO"#num), \ .id = MAX77802_LDO##num, \ .supply_name = "inl"#supply, \ .ops = &max77802_ldo_ops_logic##log, \ @@ -398,6 +402,7 @@ static struct regulator_ops max77802_buck_dvs_ops = { /* BUCKs 1, 6 */ #define regulator_77802_desc_16_buck(num) { \ .name = "BUCK"#num, \ + .of_match = of_match_ptr("BUCK"#num), \ .id = MAX77802_BUCK##num, \ .supply_name = "inb"#num, \ .ops = &max77802_buck_16_dvs_ops, \ @@ -416,6 +421,7 @@ static struct regulator_ops max77802_buck_dvs_ops = { /* BUCKS 2-4 */ #define regulator_77802_desc_234_buck(num) { \ .name = "BUCK"#num, \ + .of_match = of_match_ptr("BUCK"#num), \ .id = MAX77802_BUCK##num, \ .supply_name = "inb"#num, \ .ops = &max77802_buck_234_ops, \ @@ -435,6 +441,7 @@ static struct regulator_ops max77802_buck_dvs_ops = { /* BUCK 5 */ #define regulator_77802_desc_buck5(num) { \ .name = "BUCK"#num, \ + .of_match = of_match_ptr("BUCK"#num), \ .id = MAX77802_BUCK##num, \ .supply_name = "inb"#num, \ .ops = &max77802_buck_dvs_ops, \ @@ -453,6 +460,7 @@ static struct regulator_ops max77802_buck_dvs_ops = { /* BUCKs 7-10 */ #define regulator_77802_desc_buck7_10(num) { \ .name = "BUCK"#num, \ + .of_match = of_match_ptr("BUCK"#num), \ .id = MAX77802_BUCK##num, \ .supply_name = "inb"#num, \ .ops = &max77802_buck_dvs_ops, \ @@ -513,83 +521,31 @@ static struct regulator_desc regulators[] = { regulator_77802_desc_n_ldo(35, 2, 1), }; -#ifdef CONFIG_OF -static int max77802_pmic_dt_parse_pdata(struct platform_device *pdev, - struct max77686_platform_data *pdata) -{ - struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent); - struct device_node *pmic_np, *regulators_np; - struct max77686_regulator_data *rdata; - struct of_regulator_match rmatch; - unsigned int i; - - pmic_np = iodev->dev->of_node; - regulators_np = of_get_child_by_name(pmic_np, "regulators"); - if (!regulators_np) { - dev_err(&pdev->dev, "could not find regulators sub-node\n"); - return -EINVAL; - } - - pdata->num_regulators = ARRAY_SIZE(regulators); - rdata = devm_kzalloc(&pdev->dev, sizeof(*rdata) * - pdata->num_regulators, GFP_KERNEL); - if (!rdata) { - of_node_put(regulators_np); - return -ENOMEM; - } - - for (i = 0; i < pdata->num_regulators; i++) { - rmatch.name = regulators[i].name; - rmatch.init_data = NULL; - rmatch.of_node = NULL; - if (of_regulator_match(&pdev->dev, regulators_np, &rmatch, - 1) != 1) { - dev_warn(&pdev->dev, "No matching regulator for '%s'\n", - rmatch.name); - continue; - } - rdata[i].initdata = rmatch.init_data; - rdata[i].of_node = rmatch.of_node; - rdata[i].id = regulators[i].id; - } - - pdata->regulators = rdata; - of_node_put(regulators_np); - - return 0; -} -#else -static int max77802_pmic_dt_parse_pdata(struct platform_device *pdev, - struct max77686_platform_data *pdata) -{ - return 0; -} -#endif /* CONFIG_OF */ - static int max77802_pmic_probe(struct platform_device *pdev) { struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent); - struct max77686_platform_data *pdata = dev_get_platdata(iodev->dev); struct max77802_regulator_prv *max77802; - int i, ret = 0, val; + int i, val; struct regulator_config config = { }; - /* This is allocated by the MFD driver */ - if (!pdata) { - dev_err(&pdev->dev, "no platform data found for regulator\n"); - return -ENODEV; - } - max77802 = devm_kzalloc(&pdev->dev, sizeof(struct max77802_regulator_prv), GFP_KERNEL); if (!max77802) return -ENOMEM; - if (iodev->dev->of_node) { - ret = max77802_pmic_dt_parse_pdata(pdev, pdata); - if (ret) - return ret; + if (!pdev->dev.of_node) { + /* + * Backward DTS compatiblity where regulator driver had not + * a compatible property for itself. + */ + if (!iodev->dev->of_node) { + dev_err(&pdev->dev, "No OF node for driver and its parent\n"); + return -EINVAL; + } + pdev->dev.of_node = of_get_child_by_name(iodev->dev->of_node, + "regulators"); + max77802->missing_of_node = true; } config.dev = iodev->dev; @@ -599,11 +555,9 @@ static int max77802_pmic_probe(struct platform_device *pdev) for (i = 0; i < MAX77802_REG_MAX; i++) { struct regulator_dev *rdev; - int id = pdata->regulators[i].id; + int id = regulators[i].id; int shift = max77802_get_opmode_shift(id); - - config.init_data = pdata->regulators[i].initdata; - config.of_node = pdata->regulators[i].of_node; + int ret; ret = regmap_read(iodev->regmap, regulators[i].enable_reg, &val); if (ret < 0) { @@ -627,15 +581,26 @@ static int max77802_pmic_probe(struct platform_device *pdev) rdev = devm_regulator_register(&pdev->dev, ®ulators[i], &config); if (IS_ERR(rdev)) { + ret = PTR_ERR(rdev); dev_err(&pdev->dev, - "regulator init failed for %d\n", i); - return PTR_ERR(rdev); + "regulator init failed for %d: %d\n", i, ret); + return ret; } } return 0; } +static int max77802_pmic_remove(struct platform_device *pdev) +{ + struct max77802_regulator_prv *max77802 = platform_get_drvdata(pdev); + + if (max77802->missing_of_node) + of_node_put(pdev->dev.of_node); + + return 0; +} + static const struct platform_device_id max77802_pmic_id[] = { {"max77802-pmic", 0}, { }, @@ -648,6 +613,7 @@ static struct platform_driver max77802_pmic_driver = { .owner = THIS_MODULE, }, .probe = max77802_pmic_probe, + .remove = max77802_pmic_remove, .id_table = max77802_pmic_id, };
The driver is used only on Exynos based boards with DTS support. Convert the driver to DTS-only version. Parse all regulators at once, not one-by-one. Remove dependency on data provided by max77686 MFD driver. Use new DT style parsing method for regulators init data. The regulator driver now should have its own of_compatible property. If it is not present then it will use "regulators" sub-node of parent's node. Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> --- drivers/regulator/max77802.c | 108 +++++++++++++++---------------------------- 1 file changed, 37 insertions(+), 71 deletions(-)