Skip to contentSkip to conversation

Drive-by changes to axp20x drivers

PATCH Applied Uwe Kleine-König 4 participants 8 messages Sep 21 – Oct 4

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

cover Drive-by changes to axp20x drivers

Hello,

while trying to help a Debian user debugging why USB doesn't work in the installer on a bananapi, I discovered these patch opportunities. They are only build tested.

The first patch touches a different subsystem than the other three, but given that maintainership overlaps and the first two patches are identical in spirit, I still created a single series only. The two subsets are orthogonal, so can be picked up individually by their respective subsystem maintainers (and please do so assuming you're happy with the patches).

Best regards
Uwe

Uwe Kleine-König (4):
pinctrl: axp209: Drop check for disabled devices
power: supply: axp20x*: Drop check for disabled devices
power: supply: axp20x_usb: Introduce a helper variable for &pdev->dev
power: supply: axp20x_usb: Improve probe error reporting

 drivers/pinctrl/pinctrl-axp209.c        |  3 --
 drivers/power/supply/axp20x_ac_power.c  |  3 --
 drivers/power/supply/axp20x_battery.c   |  3 --
 drivers/power/supply/axp20x_usb_power.c | 50 ++++++++++++-------------
 4 files changed, 25 insertions(+), 34 deletions(-)

base-commit: 3f2425f5b5bbbdd991ca9cdfd5502e68d8895998

Signature · 1 line
2.47.3
#1 lore raw

PATCH 1/4 pinctrl: axp209: Drop check for disabled devices

Since commit 6b5c350648b8 ("mfd: mfd-core: Honour Device Tree's request to disable a child-device") the mfd core checks for disabled devices, so there is no use in doing that again in the mfd child driver.

Drop the check.

---
 drivers/pinctrl/pinctrl-axp209.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/pinctrl/pinctrl-axp209.c +0 -3
index 2bd8487484a8..5539ce642b39 100644
@@ -442,9 +442,6 @@ static int axp20x_pctl_probe(struct platform_device *pdev)
 	struct pinctrl_desc *pctrl_desc;
 	int ret;
 
-	if (!of_device_is_available(pdev->dev.of_node))
-		return -ENODEV;
-
 	if (!axp20x) {
 		dev_err(&pdev->dev, "Parent drvdata not set\n");
 		return -EINVAL;
Signature · 1 line
2.47.3
maintainer
#2 lore raw

Re: [PATCH 1/4] pinctrl: axp209: Drop check for disabled devices

Quoted 7 lines · Uwe Kleine-König

Since commit 6b5c350648b8 ("mfd: mfd-core: Honour Device Tree's request to disable a child-device") the mfd core checks for disabled devices, so there is no use in doing that again in the mfd child driver.

Drop the check.

This patch 1/4 applied to the pin control tree.

Yours,
Linus Walleij

#3 lore raw

PATCH 2/4 power: supply: axp20x*: Drop check for disabled devices

In reply to Uwe Kleine-König

Since commit 6b5c350648b8 ("mfd: mfd-core: Honour Device Tree's request to disable a child-device") the mfd core checks for disabled devices, so there is no use in doing that again in the mfd child driver.

Drop the check in the two power drivers.

---
 drivers/power/supply/axp20x_ac_power.c  | 3 ---
 drivers/power/supply/axp20x_battery.c   | 3 ---
 drivers/power/supply/axp20x_usb_power.c | 3 ---
 3 files changed, 9 deletions(-)
drivers/power/supply/axp20x_ac_power.c +0 -3
index d9892b351621..46889567bc18 100644
@@ -326,9 +326,6 @@ static int axp20x_ac_power_probe(struct platform_device *pdev)
 	const struct axp_data *axp_data;
 	int i, irq, ret;
 
-	if (!of_device_is_available(pdev->dev.of_node))
-		return -ENODEV;
-
 	if (!axp20x) {
 		dev_err(&pdev->dev, "Parent drvdata not set\n");
 		return -EINVAL;
drivers/power/supply/axp20x_battery.c +0 -3
index ee8701a6e907..c119014b319a 100644
@@ -1097,9 +1097,6 @@ static int axp20x_power_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int ret;
 
-	if (!of_device_is_available(pdev->dev.of_node))
-		return -ENODEV;
-
 	axp20x_batt = devm_kzalloc(&pdev->dev, sizeof(*axp20x_batt),
 				   GFP_KERNEL);
 	if (!axp20x_batt)
drivers/power/supply/axp20x_usb_power.c +0 -3
index e951b2b64b25..463f9fe35cec 100644
@@ -924,9 +924,6 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 	const struct axp_data *axp_data;
 	int i, irq, ret;
 
-	if (!of_device_is_available(pdev->dev.of_node))
-		return -ENODEV;
-
 	if (!axp20x) {
 		dev_err(&pdev->dev, "Parent drvdata not set\n");
 		return -EINVAL;
Signature · 1 line
2.47.3
#4 lore raw

PATCH 3/4 power: supply: axp20x_usb: Introduce a helper variable for &pdev->dev

In reply to Uwe Kleine-König

With the next commit adding several new usages of &pdev->dev, a helper variable is well justified. To not clutter that commit, split out the introduction of the helper out in this separate change to simplify review.

---
 drivers/power/supply/axp20x_usb_power.c | 34 ++++++++++++-------------
 1 file changed, 17 insertions(+), 17 deletions(-)
drivers/power/supply/axp20x_usb_power.c +17 -17
index 463f9fe35cec..7ee3ec49d695 100644
@@ -918,70 +918,70 @@ static void axp20x_usb_power_parse_dt(struct device *dev,
 
 static int axp20x_usb_power_probe(struct platform_device *pdev)
 {
-	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct axp20x_dev *axp20x = dev_get_drvdata(dev->parent);
 	struct power_supply_config psy_cfg = {};
 	struct axp20x_usb_power *power;
 	const struct axp_data *axp_data;
 	int i, irq, ret;
 
 	if (!axp20x) {
-		dev_err(&pdev->dev, "Parent drvdata not set\n");
+		dev_err(dev, "Parent drvdata not set\n");
 		return -EINVAL;
 	}
 
-	axp_data = of_device_get_match_data(&pdev->dev);
+	axp_data = of_device_get_match_data(dev);
 
-	power = devm_kzalloc(&pdev->dev,
-			     struct_size(power, irqs, axp_data->num_irq_names),
+	power = devm_kzalloc(dev, struct_size(power, irqs, axp_data->num_irq_names),
 			     GFP_KERNEL);
 	if (!power)
 		return -ENOMEM;
 
 	platform_set_drvdata(pdev, power);
 
-	power->dev = &pdev->dev;
+	power->dev = dev;
 	power->axp_data = axp_data;
 	power->regmap = axp20x->regmap;
 	power->num_irqs = axp_data->num_irq_names;
 
-	power->curr_lim_fld = devm_regmap_field_alloc(&pdev->dev, power->regmap,
+	power->curr_lim_fld = devm_regmap_field_alloc(dev, power->regmap,
 						      axp_data->curr_lim_fld);
 	if (IS_ERR(power->curr_lim_fld))
 		return PTR_ERR(power->curr_lim_fld);
 
-	axp20x_usb_power_parse_dt(&pdev->dev, power);
+	axp20x_usb_power_parse_dt(dev, power);
 
-	ret = axp20x_regmap_field_alloc_optional(&pdev->dev, power->regmap,
+	ret = axp20x_regmap_field_alloc_optional(dev, power->regmap,
 						 axp_data->vbus_valid_bit,
 						 &power->vbus_valid_bit);
 	if (ret)
 		return ret;
 
-	ret = axp20x_regmap_field_alloc_optional(&pdev->dev, power->regmap,
+	ret = axp20x_regmap_field_alloc_optional(dev, power->regmap,
 						 axp_data->vbus_mon_bit,
 						 &power->vbus_mon_bit);
 	if (ret)
 		return ret;
 
-	ret = axp20x_regmap_field_alloc_optional(&pdev->dev, power->regmap,
+	ret = axp20x_regmap_field_alloc_optional(dev, power->regmap,
 						 axp_data->usb_bc_en_bit,
 						 &power->usb_bc_en_bit);
 	if (ret)
 		return ret;
 
-	ret = axp20x_regmap_field_alloc_optional(&pdev->dev, power->regmap,
+	ret = axp20x_regmap_field_alloc_optional(dev, power->regmap,
 						 axp_data->usb_bc_det_fld,
 						 &power->usb_bc_det_fld);
 	if (ret)
 		return ret;
 
-	ret = axp20x_regmap_field_alloc_optional(&pdev->dev, power->regmap,
+	ret = axp20x_regmap_field_alloc_optional(dev, power->regmap,
 						 axp_data->vbus_disable_bit,
 						 &power->vbus_disable_bit);
 	if (ret)
 		return ret;
 
-	ret = devm_delayed_work_autocancel(&pdev->dev, &power->vbus_detect,
+	ret = devm_delayed_work_autocancel(dev, &power->vbus_detect,
 					   axp_data->axp20x_read_vbus);
 	if (ret)
 		return ret;
@@ -1008,10 +1008,10 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 			return ret;
 	}
 
-	psy_cfg.fwnode = dev_fwnode(&pdev->dev);
+	psy_cfg.fwnode = dev_fwnode(dev);
 	psy_cfg.drv_data = power;
 
-	power->supply = devm_power_supply_register(&pdev->dev,
+	power->supply = devm_power_supply_register(dev,
 						   axp_data->power_desc,
 						   &psy_cfg);
 	if (IS_ERR(power->supply))
@@ -1024,7 +1024,7 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 			return irq;
 
 		power->irqs[i] = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
-		ret = devm_request_any_context_irq(&pdev->dev, power->irqs[i],
+		ret = devm_request_any_context_irq(dev, power->irqs[i],
 						   axp20x_usb_power_irq, 0,
 						   DRVNAME, power);
 		if (ret < 0)
Signature · 1 line
2.47.3
#5 lore raw

PATCH 4/4 power: supply: axp20x_usb: Improve probe error reporting

In reply to Uwe Kleine-König

Don't be silent about probe fail issues. Just having axp20x-usb-power-supply in /sys/kernel/debug/devices_deferred without a reason is making it harder than necessary to debug the actual issue. So be a bit more cooperative and add an error message for the typical failure points.

Exit paths where the error is -ENOMEM and register write failures are skipped, these should be quite obvious even without a dedicated error message.

---
 drivers/power/supply/axp20x_usb_power.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
drivers/power/supply/axp20x_usb_power.c +9 -6
index 7ee3ec49d695..7625366104bc 100644
@@ -925,10 +925,8 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 	const struct axp_data *axp_data;
 	int i, irq, ret;
 
-	if (!axp20x) {
-		dev_err(dev, "Parent drvdata not set\n");
-		return -EINVAL;
-	}
+	if (!axp20x)
+		return dev_err_probe(dev, -EINVAL, "Parent drvdata not set\n");
 
 	axp_data = of_device_get_match_data(dev);
 
@@ -998,7 +996,9 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 			ret = axp_data->axp20x_cfg_adc_reg(power);
 
 		if (ret)
-			return ret;
+			return dev_err_probe(dev, ret,
+					     "Failed to initialize for %s usage\n",
+					     IS_ENABLED(CONFIG_AXP20X_ADC) ? "iio" : "adc");
 	}
 
 	if (power->usb_bc_en_bit) {
@@ -1015,12 +1015,14 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 						   axp_data->power_desc,
 						   &psy_cfg);
 	if (IS_ERR(power->supply))
-		return PTR_ERR(power->supply);
+		return dev_err_probe(dev, PTR_ERR(power->supply),
+				     "Failed to register power supply\n");
 
 	/* Request irqs after registering, as irqs may trigger immediately */
 	for (i = 0; i < axp_data->num_irq_names; i++) {
 		irq = platform_get_irq_byname(pdev, axp_data->irq_names[i]);
 		if (irq < 0)
+			/* platform_get_irq_byname() already produced an error message */
 			return irq;
 
 		power->irqs[i] = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
@@ -1028,6 +1030,7 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 						   axp20x_usb_power_irq, 0,
 						   DRVNAME, power);
 		if (ret < 0)
+			/* devm_request_any_context_irq() already produced an error message */
 			return ret;
 	}
 
Signature · 1 line
2.47.3
maintainer Reviewed-by on the v1 series
#6 lore raw
In reply to Uwe Kleine-König
Quoted 14 lines · Uwe Kleine-König

Hello,

while trying to help a Debian user debugging why USB doesn't work in the installer on a bananapi, I discovered these patch opportunities. They are only build tested.

The first patch touches a different subsystem than the other three, but given that maintainership overlaps and the first two patches are identical in spirit, I still created a single series only. The two subsets are orthogonal, so can be picked up individually by their respective subsystem maintainers (and please do so assuming you're happy with the patches).

Quoted 8 lines

Best regards
Uwe

Uwe Kleine-König (4):
pinctrl: axp209: Drop check for disabled devices
power: supply: axp20x*: Drop check for disabled devices
power: supply: axp20x_usb: Introduce a helper variable for &pdev->dev
power: supply: axp20x_usb: Improve probe error reporting

Whole series is

#7 lore raw

Re: (subset) [PATCH 0/4] Drive-by changes to axp20x drivers

In reply to Uwe Kleine-König
Quoted 4 lines · Uwe Kleine-König

while trying to help a Debian user debugging why USB doesn't work in the installer on a bananapi, I discovered these patch opportunities. They are only build tested.

Quoted 8 lines

The first patch touches a different subsystem than the other three, but given that maintainership overlaps and the first two patches are identical in spirit, I still created a single series only. The two subsets are orthogonal, so can be picked up individually by their respective subsystem maintainers (and please do so assuming you're happy with the patches).

[...]

Applied, thanks!

[2/4] power: supply: axp20x*: Drop check for disabled devices

      commit: c9ed44ab230f4359cdb3a72263c65daf31bd8528
[3/4] power: supply: axp20x_usb: Introduce a helper variable for &pdev->dev
      commit: da54acbc2ff2470c49259a91a5f48c4b0cce09d7
[4/4] power: supply: axp20x_usb: Improve probe error reporting
      commit: f945606a8cb34c97d644ed487282f96641b05185

Best regards,

Signature · 1 line
Sebastian Reichel <[email protected]>