Re: [PATCH 1/2] backlight: bd6107: Make use of the helper function dev_err_probe()

On Fri, Sep 17, 2021 at 07:05:28PM +0800, Cai Huoqing wrote:

Hi
Thanks for your feedback.
On 17 9月 21 10:17:29, Daniel Thompson wrote:

quoted

On Fri, Sep 17, 2021 at 11:13:06AM +0800, Cai Huoqing wrote:

quoted

When possible use dev_err_probe help to properly deal with the
PROBE_DEFER error, the benefit is that DEFER issue will be logged
in the devices_deferred debugfs file.
Using dev_err_probe() can reduce code size, and the error value
gets printed.

Signed-off-by: Cai Huoqing <redacted>

Change seems OK but does this really need to be done one file at a time?
I'd prefer to see all of backlight handled in one go (given the scope of
this change if applied across the kernel it needs automatic tooling
anyway).

Only two related patches for backlight.

Can you explain how you reach this conclusion?

I only checked two drivers (the first two when you list the drivers
an alphabetic order) but both contain code that appears to match the
pattern you are targeting.

I try to keep one patch for a subdriver, sometimes different
subdriver owner need to mark reviwed independently.

For mechanical patches of this nature I don't think there is any need
for acks from individual backlight driver authors. Reviews are 100%
welcome but we would not hold a single patch back until all authors
have reviewed it.


Daniel.

quoted

quoted

---
 drivers/video/backlight/bd6107.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/video/backlight/bd6107.c b/drivers/video/backlight/bd6107.c
index 515184fbe33a..e21b793302a2 100644
--- a/drivers/video/backlight/bd6107.c
+++ b/drivers/video/backlight/bd6107.c

@@ -120,7 +120,6 @@ static int bd6107_probe(struct i2c_client *client,
 	struct backlight_device *backlight;
 	struct backlight_properties props;
 	struct bd6107 *bd;
-	int ret;
 
 	if (pdata == NULL) {
 		dev_err(&client->dev, "No platform data\n");

@@ -148,11 +147,9 @@ static int bd6107_probe(struct i2c_client *client,
 	 * the reset.
 	 */
 	bd->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
-	if (IS_ERR(bd->reset)) {
-		dev_err(&client->dev, "unable to request reset GPIO\n");
-		ret = PTR_ERR(bd->reset);
-		return ret;
-	}
+	if (IS_ERR(bd->reset))
+		return dev_err_probe(&client->dev, PTR_ERR(bd->reset),
+				     "unable to request reset GPIO\n");
 
 	memset(&props, 0, sizeof(props));
 	props.type = BACKLIGHT_RAW;

@@ -164,10 +161,9 @@ static int bd6107_probe(struct i2c_client *client,
 					      dev_name(&client->dev),
 					      &bd->client->dev, bd,
 					      &bd6107_backlight_ops, &props);
-	if (IS_ERR(backlight)) {
-		dev_err(&client->dev, "failed to register backlight\n");
-		return PTR_ERR(backlight);
-	}
+	if (IS_ERR(backlight))
+		return dev_err_probe(&client->dev, PTR_ERR(backlight),
+				     "failed to register backlight\n");
 
 	backlight_update_status(backlight);
 	i2c_set_clientdata(client, backlight);

-- 
2.25.1