Re: [RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code
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
> >> ---
> >> 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);
--
Regards,
Laurent Pinchart
Re: [RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code
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
>> ---
>> 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);
Re: [RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code
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
> ---
> 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);
--
Regards,
Laurent Pinchart
[RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code
Using probe_err code has following advantages:
- shorter code,
- recorded defer probe reason for debugging,
- uniform error code logging.
Signed-off-by: Andrzej Hajda
---
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");
/* Locate the panel DT node. */
panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
--
2.17.1
