| Message ID | 20200624114127.3016-6-a.hajda@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,v5,1/5] driver core: add probe_err log helper | expand |
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);
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);
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);
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);
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(-)