[RESEND,v5,5/5] drm/bridge: lvds-codec: simplify error handling code
diff mbox series

Message ID 20200624114127.3016-6-a.hajda@samsung.com
State New
Headers show
Series
  • [RESEND,v5,1/5] driver core: add probe_err log helper
Related show

Commit Message

Andrzej Hajda June 24, 2020, 11:41 a.m. UTC
Using probe_err code has following advantages:
- shorter code,
- recorded defer probe reason for debugging,
- uniform error code logging.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/bridge/lvds-codec.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

Comments

Laurent Pinchart June 24, 2020, 1:33 p.m. UTC | #1
Hi Andrzej,

On Wed, Jun 24, 2020 at 01:41:27PM +0200, Andrzej Hajda wrote:
> Using probe_err code has following advantages:
> - shorter code,
> - recorded defer probe reason for debugging,
> - uniform error code logging.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/gpu/drm/bridge/lvds-codec.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> index 24fb1befdfa2..c76fa0239e39 100644
> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> @@ -71,13 +71,8 @@ static int lvds_codec_probe(struct platform_device *pdev)
>  	lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
>  	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
>  							     GPIOD_OUT_HIGH);
> -	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> -		int err = PTR_ERR(lvds_codec->powerdown_gpio);
> -
> -		if (err != -EPROBE_DEFER)
> -			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> -		return err;
> -	}
> +	if (IS_ERR(lvds_codec->powerdown_gpio))
> +		return probe_err(dev, lvds_codec->powerdown_gpio, "powerdown GPIO failure\n");

Line wrap please.

It bothers me that the common pattern of writing the error code at the
end of the message isn't possible anymore. Maybe I'll get used to it,
but it removes some flexibility.

>  
>  	/* Locate the panel DT node. */
>  	panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
Andrzej Hajda June 24, 2020, 2:03 p.m. UTC | #2
On 24.06.2020 15:33, Laurent Pinchart wrote:
> Hi Andrzej,
>
> On Wed, Jun 24, 2020 at 01:41:27PM +0200, Andrzej Hajda wrote:
>> Using probe_err code has following advantages:
>> - shorter code,
>> - recorded defer probe reason for debugging,
>> - uniform error code logging.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>>   drivers/gpu/drm/bridge/lvds-codec.c | 9 ++-------
>>   1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
>> index 24fb1befdfa2..c76fa0239e39 100644
>> --- a/drivers/gpu/drm/bridge/lvds-codec.c
>> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
>> @@ -71,13 +71,8 @@ static int lvds_codec_probe(struct platform_device *pdev)
>>   	lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
>>   	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
>>   							     GPIOD_OUT_HIGH);
>> -	if (IS_ERR(lvds_codec->powerdown_gpio)) {
>> -		int err = PTR_ERR(lvds_codec->powerdown_gpio);
>> -
>> -		if (err != -EPROBE_DEFER)
>> -			dev_err(dev, "powerdown GPIO failure: %d\n", err);
>> -		return err;
>> -	}
>> +	if (IS_ERR(lvds_codec->powerdown_gpio))
>> +		return probe_err(dev, lvds_codec->powerdown_gpio, "powerdown GPIO failure\n");
> Line wrap please.


I hoped that with latest checkpatch line length limit increase from 80 
to 100 it is acceptable :) but apparently not [1].

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144


>
> It bothers me that the common pattern of writing the error code at the
> end of the message isn't possible anymore. Maybe I'll get used to it,
> but it removes some flexibility.


Yes, but it gives uniformity :) and now with %pe printk format it 
changes anyway.


Regards

Andrzej


>
>>   
>>   	/* Locate the panel DT node. */
>>   	panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
Laurent Pinchart June 24, 2020, 9:18 p.m. UTC | #3
Hi Andrzej,

On Wed, Jun 24, 2020 at 04:03:30PM +0200, Andrzej Hajda wrote:
> On 24.06.2020 15:33, Laurent Pinchart wrote:
> > On Wed, Jun 24, 2020 at 01:41:27PM +0200, Andrzej Hajda wrote:
> >> Using probe_err code has following advantages:
> >> - shorter code,
> >> - recorded defer probe reason for debugging,
> >> - uniform error code logging.
> >>
> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> >> ---
> >>   drivers/gpu/drm/bridge/lvds-codec.c | 9 ++-------
> >>   1 file changed, 2 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> >> index 24fb1befdfa2..c76fa0239e39 100644
> >> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> >> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> >> @@ -71,13 +71,8 @@ static int lvds_codec_probe(struct platform_device *pdev)
> >>   	lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
> >>   	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> >>   							     GPIOD_OUT_HIGH);
> >> -	if (IS_ERR(lvds_codec->powerdown_gpio)) {
> >> -		int err = PTR_ERR(lvds_codec->powerdown_gpio);
> >> -
> >> -		if (err != -EPROBE_DEFER)
> >> -			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> >> -		return err;
> >> -	}
> >> +	if (IS_ERR(lvds_codec->powerdown_gpio))
> >> +		return probe_err(dev, lvds_codec->powerdown_gpio, "powerdown GPIO failure\n");
> >
> > Line wrap please.
> 
> I hoped that with latest checkpatch line length limit increase from 80 
> to 100 it is acceptable :) but apparently not [1].

I'm all for using longer lines when that improves readability, but in
this case I think it's easy enough to split the line. A longer line
limit doesn't mean we're forced to generate longer lines :-)

On a side note, I've been working on a C++ userspace project where we
had to decide on a coding style. Line length was one parameter, and we
went for a soft limit of 80 columns, and a hard limit of 120 columns.
This works quite well so far. The only pain point is that clang-format
(we use it, wrapped in a python script, to detect coding style
violations) doesn't understand soft and hard limits for line lengths.

> [1]: 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
>
> > It bothers me that the common pattern of writing the error code at the
> > end of the message isn't possible anymore. Maybe I'll get used to it,
> > but it removes some flexibility.
> 
> Yes, but it gives uniformity :) and now with %pe printk format it 
> changes anyway.
> 
> >>   	/* Locate the panel DT node. */
> >>   	panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);

Patch
diff mbox series

diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
index 24fb1befdfa2..c76fa0239e39 100644
--- a/drivers/gpu/drm/bridge/lvds-codec.c
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -71,13 +71,8 @@  static int lvds_codec_probe(struct platform_device *pdev)
 	lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
 	lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
 							     GPIOD_OUT_HIGH);
-	if (IS_ERR(lvds_codec->powerdown_gpio)) {
-		int err = PTR_ERR(lvds_codec->powerdown_gpio);
-
-		if (err != -EPROBE_DEFER)
-			dev_err(dev, "powerdown GPIO failure: %d\n", err);
-		return err;
-	}
+	if (IS_ERR(lvds_codec->powerdown_gpio))
+		return probe_err(dev, lvds_codec->powerdown_gpio, "powerdown GPIO failure\n");
 
 	/* Locate the panel DT node. */
 	panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);