usb: fotg210-hcd: use sysfs_emit() to instead of scnprintf()

PATCH In review Andy Shevchenko 3 participants 15 messages Jan 20, 2023 – Feb 16, 2023

Subsystems (2)

USB Faraday FOTG210 USB2 dual-role controller

lore

Share & tools

lore

Andy Shevchenko

author

2023-01-20 15:44 UTC #0 lore raw

PATCH 1/5 usb: fotg210-hcd: use sysfs_emit() to instead of scnprintf()

Follow the advice of the Documentation/filesystems/sysfs.rst and show() should only use sysfs_emit() or sysfs_emit_at() when formatting the value to be returned to user space.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/usb/fotg210/fotg210-hcd.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

drivers/usb/fotg210/fotg210-hcd.c +1 -4

index 7bd1e8f3080d..46752a75c428 100644
@@ -4686,14 +4686,11 @@ static ssize_t uframe_periodic_max_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct fotg210_hcd *fotg210;
-	int n;
 
 	fotg210 = hcd_to_fotg210(bus_to_hcd(dev_get_drvdata(dev)));
-	n = scnprintf(buf, PAGE_SIZE, "%d\n", fotg210->uframe_periodic_max);
-	return n;
+	return sysfs_emit(buf, "%d\n", fotg210->uframe_periodic_max);
 }
 
-
 static ssize_t uframe_periodic_max_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {

Signature · 1 line

2.39.0

Andy Shevchenko

author

2023-01-20 15:44 UTC #1 lore raw

PATCH 2/5 usb: fotg210-hcd: Don't shadow error codes in store()

kstrtox() along with regmap API can return different error codes based on circumstances.

Don't shadow them when returning to the caller.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/usb/fotg210/fotg210-hcd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/usb/fotg210/fotg210-hcd.c +4 -2

index 46752a75c428..5a934f5343a7 100644
@@ -4702,8 +4702,10 @@ static ssize_t uframe_periodic_max_store(struct device *dev,
 	ssize_t ret;
 
 	fotg210 = hcd_to_fotg210(bus_to_hcd(dev_get_drvdata(dev)));
-	if (kstrtouint(buf, 0, &uframe_periodic_max) < 0)
-		return -EINVAL;
+
+	ret = kstrtouint(buf, 0, &uframe_periodic_max);
+	if (ret)
+		return ret;
 
 	if (uframe_periodic_max < 100 || uframe_periodic_max >= 125) {
 		fotg210_info(fotg210, "rejecting invalid request for uframe_periodic_max=%u\n",

Signature · 1 line

2.39.0

Linus Walleij

Reviewed-by on v1 2/5

2023-01-26 20:00 UTC #2 lore raw

Re: [PATCH v1 2/5] usb: fotg210-hcd: Don't shadow error codes in store()

Quoted 6 lines · Andy Shevchenko

kstrtox() along with regmap API can return different error codes based on circumstances.
Don't shadow them when returning to the caller.
Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Andy Shevchenko

author

2023-01-20 15:44 UTC #3 lore raw

PATCH 3/5 usb: fotg210-udc: remove redundant error logging

In reply to Andy Shevchenko

A call to platform_get_irq() already prints an error on failure within its own implementation. So printing another error based on its return value in the caller is redundant and should be removed. The clean up also makes if condition block braces unnecessary. Remove that as well.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/usb/fotg210/fotg210-udc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/usb/fotg210/fotg210-udc.c +2 -4

index 4334504fccc8..18d254125186 100644
@@ -1169,10 +1169,8 @@ int fotg210_udc_probe(struct platform_device *pdev, struct fotg210 *fotg)
 	int i;
 
 	irq = platform_get_irq(pdev, 0);
-	if (irq < 0) {
-		pr_err("could not get irq\n");
-		return -ENODEV;
-	}
+	if (irq < 0)
+		return irq;
 
 	/* initialize udc */
 	fotg210 = kzalloc(sizeof(struct fotg210_udc), GFP_KERNEL);

Signature · 1 line

2.39.0

Linus Walleij

Reviewed-by on v1 3/5

2023-01-26 20:01 UTC #4 lore raw

Re: [PATCH v1 3/5] usb: fotg210-udc: remove redundant error logging

Quoted 6 lines · Andy Shevchenko

A call to platform_get_irq() already prints an error on failure within its own implementation. So printing another error based on its return value in the caller is redundant and should be removed. The clean up also makes if condition block braces unnecessary. Remove that as well.
Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Andy Shevchenko

author

2023-01-20 15:44 UTC #5 lore raw

PATCH 4/5 usb: fotg210: Switch to use dev_err_probe()

In reply to Andy Shevchenko

Switch to use dev_err_probe() to simplify the error paths and unify message template.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/usb/fotg210/fotg210-core.c | 6 ++----
 drivers/usb/fotg210/fotg210-hcd.c  | 8 +++-----
 drivers/usb/fotg210/fotg210-udc.c  | 2 +-
 3 files changed, 6 insertions(+), 10 deletions(-)

drivers/usb/fotg210/fotg210-core.c +2 -4

index c06f8eb3acbd..ce00d9407ce5 100644
@@ -50,10 +50,8 @@ static int fotg210_gemini_init(struct fotg210 *fotg, struct resource *res,
 	int ret;
 
 	map = syscon_regmap_lookup_by_phandle(np, "syscon");
-	if (IS_ERR(map)) {
-		dev_err(dev, "no syscon\n");
-		return PTR_ERR(map);
-	}
+	if (IS_ERR(map))
+		return dev_err_probe(dev, PTR_ERR(map), "no syscon\n");
 	fotg->map = map;
 	wakeup = of_property_read_bool(np, "wakeup-source");

drivers/usb/fotg210/fotg210-hcd.c +3 -5

index 5a934f5343a7..613d29f04bcb 100644
@@ -5575,8 +5575,7 @@ int fotg210_hcd_probe(struct platform_device *pdev, struct fotg210 *fotg)
 	hcd = usb_create_hcd(&fotg210_fotg210_hc_driver, dev,
 			dev_name(dev));
 	if (!hcd) {
-		dev_err(dev, "failed to create hcd\n");
-		retval = -ENOMEM;
+		retval = dev_err_probe(dev, -ENOMEM, "failed to create hcd\n");
 		goto fail_create_hcd;
 	}

@@ -5600,7 +5599,7 @@ int fotg210_hcd_probe(struct platform_device *pdev, struct fotg210 *fotg)
 
 	retval = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (retval) {
-		dev_err(dev, "failed to add hcd with err %d\n", retval);
+		dev_err_probe(dev, retval, "failed to add hcd\n");
 		goto failed_put_hcd;
 	}
 	device_wakeup_enable(hcd->self.controller);

@@ -5611,8 +5610,7 @@ int fotg210_hcd_probe(struct platform_device *pdev, struct fotg210 *fotg)
 failed_put_hcd:
 	usb_put_hcd(hcd);
 fail_create_hcd:
-	dev_err(dev, "init %s fail, %d\n", dev_name(dev), retval);
-	return retval;
+	return dev_err_probe(dev, retval, "init %s fail\n", dev_name(dev));
 }
 
 /*

drivers/usb/fotg210/fotg210-udc.c +1 -1

index 18d254125186..5b515f5cb2d7 100644
@@ -1258,7 +1258,7 @@ int fotg210_udc_probe(struct platform_device *pdev, struct fotg210 *fotg)
 	ret = request_irq(irq, fotg210_irq, IRQF_SHARED,
 			  udc_name, fotg210);
 	if (ret < 0) {
-		dev_err(dev, "request_irq error (%d)\n", ret);
+		dev_err_probe(dev, ret, "request_irq error\n");
 		goto err_req;
 	}

Signature · 1 line

2.39.0

Linus Walleij

Reviewed-by on v1 4/5

2023-01-26 20:01 UTC #6 lore raw

Re: [PATCH v1 4/5] usb: fotg210: Switch to use dev_err_probe()

Quoted 4 lines · Andy Shevchenko

Switch to use dev_err_probe() to simplify the error paths and unify message template.
Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Christophe JAILLET

2023-02-16 08:07 UTC #7 lore raw

Re: [PATCH v1 4/5] usb: fotg210: Switch to use dev_err_probe()

In reply to Andy Shevchenko

Le 20/01/2023 à 16:44, Andy Shevchenko a écrit :

Quoted 51 lines

Switch to use dev_err_probe() to simplify the error paths and unify message template.

Signed-off-by Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/[email protected]>

---
  drivers/usb/fotg210/fotg210-core.c | 6 ++----
  drivers/usb/fotg210/fotg210-hcd.c  | 8 +++-----
  drivers/usb/fotg210/fotg210-udc.c  | 2 +-
  3 files changed, 6 insertions(+), 10 deletions(-)

drivers/usb/fotg210/fotg210-core.c +2 -4

index c06f8eb3acbd..ce00d9407ce5 100644
@@ -50,10 +50,8 @@ static int fotg210_gemini_init(struct fotg210 *fotg, struct resource *res,
  	int ret;
 
  	map = syscon_regmap_lookup_by_phandle(np, "syscon");
-	if (IS_ERR(map)) {
-		dev_err(dev, "no syscon\n");
-		return PTR_ERR(map);
-	}
+	if (IS_ERR(map))
+		return dev_err_probe(dev, PTR_ERR(map), "no syscon\n");
  	fotg->map = map;
  	wakeup = of_property_read_bool(np, "wakeup-source");

drivers/usb/fotg210/fotg210-hcd.c +2 -3

index 5a934f5343a7..613d29f04bcb 100644
@@ -5575,8 +5575,7 @@ int fotg210_hcd_probe(struct platform_device *pdev, struct fotg210 *fotg)
  	hcd = usb_create_hcd(&fotg210_fotg210_hc_driver, dev,
  			dev_name(dev));
  	if (!hcd) {
-		dev_err(dev, "failed to create hcd\n");
-		retval = -ENOMEM;
+		retval = dev_err_probe(dev, -ENOMEM, "failed to create hcd\n");
  		goto fail_create_hcd;
  	}

@@ -5600,7 +5599,7 @@ int fotg210_hcd_probe(struct platform_device *pdev, struct fotg210 *fotg)
 
  	retval = usb_add_hcd(hcd, irq, IRQF_SHARED);
  	if (retval) {
-		dev_err(dev, "failed to add hcd with err %d\n", retval);
+		dev_err_probe(dev, retval, "failed to add hcd\n");
  		goto failed_put_hcd;
  	}
  	device_wakeup_enable(hcd->self.controller);

drivers/usb/fotg210/fotg210-hcd.c int fotg210_hcd_probe(struct platform_device *pdev, struct fotg210 *fotg) · line 5613

Quoted 6 lines

drivers/usb/fotg210/fotg210-hcd.c +1 -2

@@ -5611,8 +5610,7 @@ int fotg210_hcd_probe(struct platform_device *pdev, struct fotg210 *fotg)
  failed_put_hcd:
  	usb_put_hcd(hcd);
  fail_create_hcd:
-	dev_err(dev, "init %s fail, %d\n", dev_name(dev), retval);
-	return retval;
+	return dev_err_probe(dev, retval, "init %s fail\n", dev_name(dev));

Hi,
the patch is already applied, but is dev_name(dev) needed here?

Quoted 16 lines

drivers/usb/fotg210/fotg210-hcd.c +0 -0

  }
 
  /*

drivers/usb/fotg210/fotg210-udc.c +1 -1

index 18d254125186..5b515f5cb2d7 100644
@@ -1258,7 +1258,7 @@ int fotg210_udc_probe(struct platform_device *pdev, struct fotg210 *fotg)
  	ret = request_irq(irq, fotg210_irq, IRQF_SHARED,
  			  udc_name, fotg210);
  	if (ret < 0) {
-		dev_err(dev, "request_irq error (%d)\n", ret);
+		dev_err_probe(dev, ret, "request_irq error\n");
  		goto err_req;
  	}

Linus Walleij

2023-02-16 08:30 UTC #8 lore raw

Re: [PATCH v1 4/5] usb: fotg210: Switch to use dev_err_probe()

Quoted 6 lines · Christophe JAILLET

drivers/usb/fotg210/fotg210-hcd.c int fotg210_hcd_probe(struct platform_device *pdev, struct fotg210 *fotg) · line 5613
Quoted 3 lines
drivers/usb/fotg210/fotg210-hcd.c +1 -2
-     dev_err(dev, "init %s fail, %d\n", dev_name(dev), retval);
-     return retval;
+     return dev_err_probe(dev, retval, "init %s fail\n", dev_name(dev));
Hi,
the patch is already applied, but is dev_name(dev) needed here?

Not really but it's not a big deal either, it's just a string. Feel free to send an incremental patch dropping it :)

Yours,
Linus Walleij

Andy Shevchenko

author

2023-01-20 15:44 UTC #9 lore raw

PATCH 5/5 usb: fotg210: use devm_platform_get_and_ioremap_resource()

In reply to Andy Shevchenko

Convert platform_get_resource(), devm_ioremap_resource() to a single call to devm_platform_get_and_ioremap_resource(), as this is exactly what this function does.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/usb/fotg210/fotg210-core.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

drivers/usb/fotg210/fotg210-core.c +1 -5

index ce00d9407ce5..202d80adca2c 100644
@@ -135,11 +135,7 @@ static int fotg210_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	fotg->dev = dev;
 
-	fotg->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!fotg->res)
-		return -ENODEV;
-
-	fotg->base = devm_ioremap_resource(dev, fotg->res);
+	fotg->base = devm_platform_get_and_ioremap_resource(pdev, 0, &fotg->res);
 	if (!fotg->base)
 		return -ENOMEM;

Signature · 1 line

2.39.0

Linus Walleij

Reviewed-by on v1 5/5

2023-01-26 20:02 UTC #10 lore raw

Re: [PATCH v1 5/5] usb: fotg210: use devm_platform_get_and_ioremap_resource()

Quoted 5 lines · Andy Shevchenko

Convert platform_get_resource(), devm_ioremap_resource() to a single call to devm_platform_get_and_ioremap_resource(), as this is exactly what this function does.
Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Andy Shevchenko

author

2023-01-27 08:55 UTC #11 lore raw

Re: [PATCH v1 5/5] usb: fotg210: use devm_platform_get_and_ioremap_resource()

Quoted 2 lines · Linus Walleij

On Fri, Jan 20, 2023 at 4:44 PM Andy Shevchenko
<[email protected]> wrote:

...

Quoted 1 line

Reviewed-by Linus Walleij <[email protected]>

Thanks for all your reviews!

But I think you are a bit late with them as Greg applied the series like a couple of days ago.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Linus Walleij

2023-01-27 09:10 UTC #12 lore raw

Re: [PATCH v1 5/5] usb: fotg210: use devm_platform_get_and_ioremap_resource()

Quoted 4 lines · Andy Shevchenko

Quoted 2 lines · Linus Walleij
On Fri, Jan 20, 2023 at 4:44 PM Andy Shevchenko
<[email protected]> wrote:

Quoted 8 lines

...
Quoted 1 line
Reviewed-by Linus Walleij <[email protected]>
Thanks for all your reviews!
But I think you are a bit late with them as Greg applied the series like a couple of days ago.

Yeah he's so quick and I'm so slow...
But these days we record the mail thread link to lore in the commit so I can still share the blame ;)

Yours,
Linus Walleij

Andy Shevchenko

author

2023-01-27 09:16 UTC #13 lore raw

Re: [PATCH v1 5/5] usb: fotg210: use devm_platform_get_and_ioremap_resource()

Quoted 6 lines · Linus Walleij

Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Linus Walleij
On Fri, Jan 20, 2023 at 4:44 PM Andy Shevchenko
<[email protected]> wrote:

...

Quoted 10 lines

Quoted 6 lines
Quoted 1 line
Reviewed-by Linus Walleij <[email protected]>
Thanks for all your reviews!
But I think you are a bit late with them as Greg applied the series like a couple of days ago.
Yeah he's so quick and I'm so slow...
But these days we record the mail thread link to lore in the commit so I can still share the blame ;)

Right, what I meant is not diminish your efforts, but to say that they won't make Git history.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Linus Walleij

Reviewed-by on v1 1/5

2023-01-26 20:00 UTC #14 lore raw

In reply to Andy Shevchenko

Quoted 5 lines · Andy Shevchenko

Follow the advice of the Documentation/filesystems/sysfs.rst and show() should only use sysfs_emit() or sysfs_emit_at() when formatting the value to be returned to user space.
Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij