| Message ID | 20210130172747.2022977-4-timon.baetz@protonmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | charger-supply for max8997_charger | expand |
On Sat, Jan 30, 2021 at 05:30:14PM +0000, Timon Baetz wrote: > Get regulator from parent device's node and extcon by name. > > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com> > --- > drivers/power/supply/max8997_charger.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c > index 321bd6b8ee41..625d8cc4312a 100644 > --- a/drivers/power/supply/max8997_charger.c > +++ b/drivers/power/supply/max8997_charger.c > @@ -168,6 +168,7 @@ static int max8997_battery_probe(struct platform_device *pdev) > int ret = 0; > struct charger_data *charger; > struct max8997_dev *iodev = dev_get_drvdata(pdev->dev.parent); > + struct device_node *np = pdev->dev.of_node; > struct i2c_client *i2c = iodev->i2c; > struct max8997_platform_data *pdata = iodev->pdata; > struct power_supply_config psy_cfg = {}; > @@ -237,20 +238,23 @@ static int max8997_battery_probe(struct platform_device *pdev) > return PTR_ERR(charger->battery); > } > > + // grab regulator from parent device's node > + pdev->dev.of_node = iodev->dev->of_node; > charger->reg = devm_regulator_get_optional(&pdev->dev, "charger"); > + pdev->dev.of_node = np; I think the device does not have its own node anymore. Or did I miss something? > if (IS_ERR(charger->reg)) { > if (PTR_ERR(charger->reg) == -EPROBE_DEFER) > return -EPROBE_DEFER; > dev_info(&pdev->dev, "couldn't get charger regulator\n"); > } > - charger->edev = extcon_get_edev_by_phandle(&pdev->dev, 0); > - if (IS_ERR(charger->edev)) { > - if (PTR_ERR(charger->edev) == -EPROBE_DEFER) > + charger->edev = extcon_get_extcon_dev("max8997-muic"); > + if (IS_ERR_OR_NULL(charger->edev)) { > + if (!charger->edev) Isn't NULL returned when there is simply no extcon? It's different than deferred probe. Returning here EPROBE_DEFER might lead to infinite probe tries (on every new device probe) instead of just failing it. Best regards, Krzysztof > return -EPROBE_DEFER; > dev_info(charger->dev, "couldn't get extcon device\n"); > } > > - if (!IS_ERR(charger->reg) && !IS_ERR(charger->edev)) { > + if (!IS_ERR(charger->reg) && !IS_ERR_OR_NULL(charger->edev)) { > INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker); > ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger); > if (ret) { > -- > 2.25.1 > >
On Sun, 31 Jan 2021 18:28:40 +0100, Krzysztof Kozlowski wrote: > On Sat, Jan 30, 2021 at 05:30:14PM +0000, Timon Baetz wrote: > > Get regulator from parent device's node and extcon by name. > > > > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com> > > --- > > drivers/power/supply/max8997_charger.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c > > index 321bd6b8ee41..625d8cc4312a 100644 > > --- a/drivers/power/supply/max8997_charger.c > > +++ b/drivers/power/supply/max8997_charger.c > > @@ -168,6 +168,7 @@ static int max8997_battery_probe(struct platform_device *pdev) > > int ret = 0; > > struct charger_data *charger; > > struct max8997_dev *iodev = dev_get_drvdata(pdev->dev.parent); > > + struct device_node *np = pdev->dev.of_node; > > struct i2c_client *i2c = iodev->i2c; > > struct max8997_platform_data *pdata = iodev->pdata; > > struct power_supply_config psy_cfg = {}; > > @@ -237,20 +238,23 @@ static int max8997_battery_probe(struct platform_device *pdev) > > return PTR_ERR(charger->battery); > > } > > > > + // grab regulator from parent device's node > > + pdev->dev.of_node = iodev->dev->of_node; > > charger->reg = devm_regulator_get_optional(&pdev->dev, "charger"); > > + pdev->dev.of_node = np; > > I think the device does not have its own node anymore. Or did I miss > something? The idea is to reset of_node to whatever it was before (NULL) and basically leave the device unchanged. Probe might run again because of deferral. > > if (IS_ERR(charger->reg)) { > > if (PTR_ERR(charger->reg) == -EPROBE_DEFER) > > return -EPROBE_DEFER; > > dev_info(&pdev->dev, "couldn't get charger regulator\n"); > > } > > - charger->edev = extcon_get_edev_by_phandle(&pdev->dev, 0); > > - if (IS_ERR(charger->edev)) { > > - if (PTR_ERR(charger->edev) == -EPROBE_DEFER) > > + charger->edev = extcon_get_extcon_dev("max8997-muic"); > > + if (IS_ERR_OR_NULL(charger->edev)) { > > + if (!charger->edev) > > Isn't NULL returned when there is simply no extcon? It's different than > deferred probe. Returning here EPROBE_DEFER might lead to infinite probe > tries (on every new device probe) instead of just failing it. extcon_get_extcon_dev() just loops through all registered extcon devices and compared names. It will return NULL when "max8997-muic" isn't registered yet. extcon_get_extcon_dev() never returns EPROBE_DEFER so checking for NULL seems to be the only way. Other drivers using that function also do NULL check and return EPROBE_DEFER. Thanks for reviewing, Timon
On Mon, Feb 01, 2021 at 09:26:42AM +0000, Timon Baetz wrote: > On Sun, 31 Jan 2021 18:28:40 +0100, Krzysztof Kozlowski wrote: > > On Sat, Jan 30, 2021 at 05:30:14PM +0000, Timon Baetz wrote: > > > Get regulator from parent device's node and extcon by name. > > > > > > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com> > > > --- > > > drivers/power/supply/max8997_charger.c | 12 ++++++++---- > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c > > > index 321bd6b8ee41..625d8cc4312a 100644 > > > --- a/drivers/power/supply/max8997_charger.c > > > +++ b/drivers/power/supply/max8997_charger.c > > > @@ -168,6 +168,7 @@ static int max8997_battery_probe(struct platform_device *pdev) > > > int ret = 0; > > > struct charger_data *charger; > > > struct max8997_dev *iodev = dev_get_drvdata(pdev->dev.parent); > > > + struct device_node *np = pdev->dev.of_node; > > > struct i2c_client *i2c = iodev->i2c; > > > struct max8997_platform_data *pdata = iodev->pdata; > > > struct power_supply_config psy_cfg = {}; > > > @@ -237,20 +238,23 @@ static int max8997_battery_probe(struct platform_device *pdev) > > > return PTR_ERR(charger->battery); > > > } > > > > > > + // grab regulator from parent device's node > > > + pdev->dev.of_node = iodev->dev->of_node; > > > charger->reg = devm_regulator_get_optional(&pdev->dev, "charger"); > > > + pdev->dev.of_node = np; > > > > I think the device does not have its own node anymore. Or did I miss > > something? > > The idea is to reset of_node to whatever it was before (NULL) and basically > leave the device unchanged. Probe might run again because of deferral. Good point. > > > > if (IS_ERR(charger->reg)) { > > > if (PTR_ERR(charger->reg) == -EPROBE_DEFER) > > > return -EPROBE_DEFER; > > > dev_info(&pdev->dev, "couldn't get charger regulator\n"); > > > } > > > - charger->edev = extcon_get_edev_by_phandle(&pdev->dev, 0); > > > - if (IS_ERR(charger->edev)) { > > > - if (PTR_ERR(charger->edev) == -EPROBE_DEFER) > > > + charger->edev = extcon_get_extcon_dev("max8997-muic"); > > > + if (IS_ERR_OR_NULL(charger->edev)) { > > > + if (!charger->edev) > > > > Isn't NULL returned when there is simply no extcon? It's different than > > deferred probe. Returning here EPROBE_DEFER might lead to infinite probe > > tries (on every new device probe) instead of just failing it. > > extcon_get_extcon_dev() just loops through all registered extcon devices > and compared names. It will return NULL when "max8997-muic" isn't > registered yet. extcon_get_extcon_dev() never returns EPROBE_DEFER so > checking for NULL seems to be the only way. Other drivers using that > function also do NULL check and return EPROBE_DEFER. Indeed, thanks for clarification. Looks good: Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof
On Mon, 1 Feb 2021 19:03:35 +0100, Krzysztof Kozlowski wrote: > On Mon, Feb 01, 2021 at 09:26:42AM +0000, Timon Baetz wrote: > > On Sun, 31 Jan 2021 18:28:40 +0100, Krzysztof Kozlowski wrote: > > > On Sat, Jan 30, 2021 at 05:30:14PM +0000, Timon Baetz wrote: > > > > Get regulator from parent device's node and extcon by name. > > > > > > > > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com> > > > > --- > > > > drivers/power/supply/max8997_charger.c | 12 ++++++++---- > > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c > > > > index 321bd6b8ee41..625d8cc4312a 100644 > > > > --- a/drivers/power/supply/max8997_charger.c > > > > +++ b/drivers/power/supply/max8997_charger.c > > > > @@ -168,6 +168,7 @@ static int max8997_battery_probe(struct platform_device *pdev) > > > > int ret = 0; > > > > struct charger_data *charger; > > > > struct max8997_dev *iodev = dev_get_drvdata(pdev->dev.parent); > > > > + struct device_node *np = pdev->dev.of_node; > > > > struct i2c_client *i2c = iodev->i2c; > > > > struct max8997_platform_data *pdata = iodev->pdata; > > > > struct power_supply_config psy_cfg = {}; > > > > @@ -237,20 +238,23 @@ static int max8997_battery_probe(struct platform_device *pdev) > > > > return PTR_ERR(charger->battery); > > > > } > > > > > > > > + // grab regulator from parent device's node > > > > + pdev->dev.of_node = iodev->dev->of_node; > > > > charger->reg = devm_regulator_get_optional(&pdev->dev, "charger"); > > > > + pdev->dev.of_node = np; > > > > > > I think the device does not have its own node anymore. Or did I miss > > > something? > > > > The idea is to reset of_node to whatever it was before (NULL) and basically > > leave the device unchanged. Probe might run again because of deferral. > > Good point. > > > > > > > if (IS_ERR(charger->reg)) { > > > > if (PTR_ERR(charger->reg) == -EPROBE_DEFER) > > > > return -EPROBE_DEFER; > > > > dev_info(&pdev->dev, "couldn't get charger regulator\n"); > > > > } > > > > - charger->edev = extcon_get_edev_by_phandle(&pdev->dev, 0); > > > > - if (IS_ERR(charger->edev)) { > > > > - if (PTR_ERR(charger->edev) == -EPROBE_DEFER) > > > > + charger->edev = extcon_get_extcon_dev("max8997-muic"); > > > > + if (IS_ERR_OR_NULL(charger->edev)) { > > > > + if (!charger->edev) > > > > > > Isn't NULL returned when there is simply no extcon? It's different than > > > deferred probe. Returning here EPROBE_DEFER might lead to infinite probe > > > tries (on every new device probe) instead of just failing it. > > > > extcon_get_extcon_dev() just loops through all registered extcon devices > > and compared names. It will return NULL when "max8997-muic" isn't > > registered yet. extcon_get_extcon_dev() never returns EPROBE_DEFER so > > checking for NULL seems to be the only way. Other drivers using that > > function also do NULL check and return EPROBE_DEFER. > > Indeed, thanks for clarification. Looks good: > > Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Is something blocking this from being accepted? Timon
On Sat, 30 Jan 2021 17:30:14 +0000, Timon Baetz wrote: > Get regulator from parent device's node and extcon by name. > > Signed-off-by: Timon Baetz <timon.baetz@protonmail.com> > --- > drivers/power/supply/max8997_charger.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c > index 321bd6b8ee41..625d8cc4312a 100644 > --- a/drivers/power/supply/max8997_charger.c > +++ b/drivers/power/supply/max8997_charger.c > @@ -168,6 +168,7 @@ static int max8997_battery_probe(struct platform_device *pdev) > int ret = 0; > struct charger_data *charger; > struct max8997_dev *iodev = dev_get_drvdata(pdev->dev.parent); > + struct device_node *np = pdev->dev.of_node; > struct i2c_client *i2c = iodev->i2c; > struct max8997_platform_data *pdata = iodev->pdata; > struct power_supply_config psy_cfg = {}; > @@ -237,20 +238,23 @@ static int max8997_battery_probe(struct platform_device *pdev) > return PTR_ERR(charger->battery); > } > > + // grab regulator from parent device's node > + pdev->dev.of_node = iodev->dev->of_node; > charger->reg = devm_regulator_get_optional(&pdev->dev, "charger"); > + pdev->dev.of_node = np; > if (IS_ERR(charger->reg)) { > if (PTR_ERR(charger->reg) == -EPROBE_DEFER) > return -EPROBE_DEFER; > dev_info(&pdev->dev, "couldn't get charger regulator\n"); > } > - charger->edev = extcon_get_edev_by_phandle(&pdev->dev, 0); > - if (IS_ERR(charger->edev)) { > - if (PTR_ERR(charger->edev) == -EPROBE_DEFER) > + charger->edev = extcon_get_extcon_dev("max8997-muic"); > + if (IS_ERR_OR_NULL(charger->edev)) { > + if (!charger->edev) > return -EPROBE_DEFER; > dev_info(charger->dev, "couldn't get extcon device\n"); > } > > - if (!IS_ERR(charger->reg) && !IS_ERR(charger->edev)) { > + if (!IS_ERR(charger->reg) && !IS_ERR_OR_NULL(charger->edev)) { > INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker); > ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger); > if (ret) { Hello, I am not sure if I am missing something here. The other 2 patches of the series already made it into linux-next but this is still pending. Do I have to resubmit? The patch still applies without conflicts. Any help would be appreciated. Thanks, Timon
diff --git a/drivers/power/supply/max8997_charger.c b/drivers/power/supply/max8997_charger.c index 321bd6b8ee41..625d8cc4312a 100644 --- a/drivers/power/supply/max8997_charger.c +++ b/drivers/power/supply/max8997_charger.c @@ -168,6 +168,7 @@ static int max8997_battery_probe(struct platform_device *pdev) int ret = 0; struct charger_data *charger; struct max8997_dev *iodev = dev_get_drvdata(pdev->dev.parent); + struct device_node *np = pdev->dev.of_node; struct i2c_client *i2c = iodev->i2c; struct max8997_platform_data *pdata = iodev->pdata; struct power_supply_config psy_cfg = {}; @@ -237,20 +238,23 @@ static int max8997_battery_probe(struct platform_device *pdev) return PTR_ERR(charger->battery); } + // grab regulator from parent device's node + pdev->dev.of_node = iodev->dev->of_node; charger->reg = devm_regulator_get_optional(&pdev->dev, "charger"); + pdev->dev.of_node = np; if (IS_ERR(charger->reg)) { if (PTR_ERR(charger->reg) == -EPROBE_DEFER) return -EPROBE_DEFER; dev_info(&pdev->dev, "couldn't get charger regulator\n"); } - charger->edev = extcon_get_edev_by_phandle(&pdev->dev, 0); - if (IS_ERR(charger->edev)) { - if (PTR_ERR(charger->edev) == -EPROBE_DEFER) + charger->edev = extcon_get_extcon_dev("max8997-muic"); + if (IS_ERR_OR_NULL(charger->edev)) { + if (!charger->edev) return -EPROBE_DEFER; dev_info(charger->dev, "couldn't get extcon device\n"); } - if (!IS_ERR(charger->reg) && !IS_ERR(charger->edev)) { + if (!IS_ERR(charger->reg) && !IS_ERR_OR_NULL(charger->edev)) { INIT_WORK(&charger->extcon_work, max8997_battery_extcon_evt_worker); ret = devm_add_action(&pdev->dev, max8997_battery_extcon_evt_stop_work, charger); if (ret) {
Get regulator from parent device's node and extcon by name. Signed-off-by: Timon Baetz <timon.baetz@protonmail.com> --- drivers/power/supply/max8997_charger.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)