| Message ID | 20240828120118.1284420-1-yanzhen@vivo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] cirrus: Simplify with dev_err_probe() | expand |
On 28/08/2024 13:01, Yan Zhen wrote: > Switch to use dev_err_probe() to simplify the error path and > unify a message template. > > Using this helper is totally fine even if err is known to never > be -EPROBE_DEFER. > > The benefit compared to a normal dev_err() is the standardized format > of the error code, it being emitted symbolically and the fact that > the error code is returned which allows more compact error paths. > > Signed-off-by: Yan Zhen <yanzhen@vivo.com> > --- > drivers/irqchip/irq-madera.c | 5 ++--- > drivers/pinctrl/cirrus/pinctrl-madera-core.c | 7 +++---- Split into separate patches for irqchip and pinctrl. There's no dependency between these two changes so they don't need to be applied in the same patch. Also use the correct commit subject prefix for the subsystem and driver that is being patched. Not "cirrus:". > 2 files changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/irqchip/irq-madera.c b/drivers/irqchip/irq-madera.c > index acceb6e7fa95..68ebe09baaf6 100644 > --- a/drivers/irqchip/irq-madera.c > +++ b/drivers/irqchip/irq-madera.c > @@ -199,9 +199,8 @@ static int madera_irq_probe(struct platform_device *pdev) > ret = regmap_update_bits(madera->regmap, MADERA_IRQ1_CTRL, > MADERA_IRQ_POL_MASK, 0); > if (ret) { > - dev_err(&pdev->dev, > - "Failed to set IRQ polarity: %d\n", ret); > - return ret; > + return dev_err_probe(&pdev->dev, ret, > + "Failed to set IRQ polarity: %d\n", ret); dev_err_probe already logs 'ret' so you don't need to log it again in the message. > } > } > > diff --git a/drivers/pinctrl/cirrus/pinctrl-madera-core.c b/drivers/pinctrl/cirrus/pinctrl-madera-core.c > index 898b197c3738..532fa8ac9a94 100644 > --- a/drivers/pinctrl/cirrus/pinctrl-madera-core.c > +++ b/drivers/pinctrl/cirrus/pinctrl-madera-core.c > @@ -1064,10 +1064,9 @@ static int madera_pin_probe(struct platform_device *pdev) > ret = pinctrl_register_mappings(pdata->gpio_configs, > pdata->n_gpio_configs); > if (ret) { > - dev_err(priv->dev, > - "Failed to register pdata mappings (%d)\n", > - ret); > - return ret; > + return dev_err_probe(priv->dev, ret, > + "Failed to register pdata mappings (%d)\n", > + ret); Same here. No need to log 'ret' twice. > } > } >
On Wed, Aug 28, 2024 at 08:01:18PM +0800, Yan Zhen wrote: > Switch to use dev_err_probe() to simplify the error path and > unify a message template. > > Using this helper is totally fine even if err is known to never > be -EPROBE_DEFER. > > The benefit compared to a normal dev_err() is the standardized format > of the error code, it being emitted symbolically and the fact that > the error code is returned which allows more compact error paths. > > Signed-off-by: Yan Zhen <yanzhen@vivo.com> > --- > drivers/irqchip/irq-madera.c | 5 ++--- > drivers/pinctrl/cirrus/pinctrl-madera-core.c | 7 +++---- > 2 files changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/irqchip/irq-madera.c b/drivers/irqchip/irq-madera.c > index acceb6e7fa95..68ebe09baaf6 100644 > --- a/drivers/irqchip/irq-madera.c > +++ b/drivers/irqchip/irq-madera.c > @@ -199,9 +199,8 @@ static int madera_irq_probe(struct platform_device *pdev) > ret = regmap_update_bits(madera->regmap, MADERA_IRQ1_CTRL, > MADERA_IRQ_POL_MASK, 0); > if (ret) { > - dev_err(&pdev->dev, > - "Failed to set IRQ polarity: %d\n", ret); > - return ret; > + return dev_err_probe(&pdev->dev, ret, > + "Failed to set IRQ polarity: %d\n", ret); > } > } > > diff --git a/drivers/pinctrl/cirrus/pinctrl-madera-core.c b/drivers/pinctrl/cirrus/pinctrl-madera-core.c > index 898b197c3738..532fa8ac9a94 100644 > --- a/drivers/pinctrl/cirrus/pinctrl-madera-core.c > +++ b/drivers/pinctrl/cirrus/pinctrl-madera-core.c > @@ -1064,10 +1064,9 @@ static int madera_pin_probe(struct platform_device *pdev) > ret = pinctrl_register_mappings(pdata->gpio_configs, > pdata->n_gpio_configs); > if (ret) { > - dev_err(priv->dev, > - "Failed to register pdata mappings (%d)\n", > - ret); > - return ret; > + return dev_err_probe(priv->dev, ret, > + "Failed to register pdata mappings (%d)\n", > + ret); > } Since we are doing a respin anyway also the brackets on the if should be dropped on both of these as well. Thanks, Charles
On Wed, Aug 28 2024 at 13:27, Charles Keepax wrote: > On Wed, Aug 28, 2024 at 08:01:18PM +0800, Yan Zhen wrote: >> @@ -1064,10 +1064,9 @@ static int madera_pin_probe(struct platform_device *pdev) >> ret = pinctrl_register_mappings(pdata->gpio_configs, >> pdata->n_gpio_configs); >> if (ret) { >> - dev_err(priv->dev, >> - "Failed to register pdata mappings (%d)\n", >> - ret); >> - return ret; >> + return dev_err_probe(priv->dev, ret, >> + "Failed to register pdata mappings (%d)\n", >> + ret); >> } > > Since we are doing a respin anyway also the brackets on the if > should be dropped on both of these as well. No. https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#bracket-rules Thanks, tglx
On 28/08/2024 15:47, Thomas Gleixner wrote: > On Wed, Aug 28 2024 at 13:27, Charles Keepax wrote: >> On Wed, Aug 28, 2024 at 08:01:18PM +0800, Yan Zhen wrote: >>> @@ -1064,10 +1064,9 @@ static int madera_pin_probe(struct platform_device *pdev) >>> ret = pinctrl_register_mappings(pdata->gpio_configs, >>> pdata->n_gpio_configs); >>> if (ret) { >>> - dev_err(priv->dev, >>> - "Failed to register pdata mappings (%d)\n", >>> - ret); >>> - return ret; >>> + return dev_err_probe(priv->dev, ret, >>> + "Failed to register pdata mappings (%d)\n", >>> + ret); >>> } >> >> Since we are doing a respin anyway also the brackets on the if >> should be dropped on both of these as well. > > No. https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#bracket-rules > https://www.kernel.org/doc/html/latest/process/coding-style.html#codingstyle does not say that. It says: "Do not unnecessarily use braces where a single statement will do." Note: single statement, not single line. coding-style.rst is the coding style. If you disagree with the coding style, send a patch to change coding-style.rst. > Thanks, > > tglx
On Wed, Aug 28, 2024 at 03:59:50PM +0100, Richard Fitzgerald wrote: > On 28/08/2024 15:47, Thomas Gleixner wrote: > > On Wed, Aug 28 2024 at 13:27, Charles Keepax wrote: > > > On Wed, Aug 28, 2024 at 08:01:18PM +0800, Yan Zhen wrote: > > > > @@ -1064,10 +1064,9 @@ static int madera_pin_probe(struct platform_device *pdev) > > > > ret = pinctrl_register_mappings(pdata->gpio_configs, > > > > pdata->n_gpio_configs); > > > > if (ret) { > > > > - dev_err(priv->dev, > > > > - "Failed to register pdata mappings (%d)\n", > > > > - ret); > > > > - return ret; > > > > + return dev_err_probe(priv->dev, ret, > > > > + "Failed to register pdata mappings (%d)\n", > > > > + ret); > > > > } > > > > > > Since we are doing a respin anyway also the brackets on the if > > > should be dropped on both of these as well. > > > > No. https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#bracket-rules > > > > https://www.kernel.org/doc/html/latest/process/coding-style.html#codingstyle > > does not say that. It says: > > "Do not unnecessarily use braces where a single statement will do." > > Note: single statement, not single line. > > coding-style.rst is the coding style. If you disagree with the coding > style, send a patch to change coding-style.rst. > I mean certainly having two documents with apparently conflicting guidelines for the coding style is not a helpful situation at all. I would definitely second the call for patching the coding-style doc to be more explicit there. And to be clear I really don't care if we keep the brackets or not, was just trying to follow what I thought was the coding style guidance. Thanks, Charles
On Wed, Aug 28 2024 at 15:59, Richard Fitzgerald wrote: > On 28/08/2024 15:47, Thomas Gleixner wrote: >> On Wed, Aug 28 2024 at 13:27, Charles Keepax wrote: >>> Since we are doing a respin anyway also the brackets on the if >>> should be dropped on both of these as well. >> >> No. https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#bracket-rules > > https://www.kernel.org/doc/html/latest/process/coding-style.html#codingstyle > > does not say that. It says: > > "Do not unnecessarily use braces where a single statement will do." > > Note: single statement, not single line. > > coding-style.rst is the coding style. If you disagree with the coding > style, send a patch to change coding-style.rst. There will never be an agreement about this, but for code which I maintain and goes through the tip tree, the single line rule applies simply because it's my time wasted when I can't parse code fluently. Thanks, tglx
diff --git a/drivers/irqchip/irq-madera.c b/drivers/irqchip/irq-madera.c index acceb6e7fa95..68ebe09baaf6 100644 --- a/drivers/irqchip/irq-madera.c +++ b/drivers/irqchip/irq-madera.c @@ -199,9 +199,8 @@ static int madera_irq_probe(struct platform_device *pdev) ret = regmap_update_bits(madera->regmap, MADERA_IRQ1_CTRL, MADERA_IRQ_POL_MASK, 0); if (ret) { - dev_err(&pdev->dev, - "Failed to set IRQ polarity: %d\n", ret); - return ret; + return dev_err_probe(&pdev->dev, ret, + "Failed to set IRQ polarity: %d\n", ret); } } diff --git a/drivers/pinctrl/cirrus/pinctrl-madera-core.c b/drivers/pinctrl/cirrus/pinctrl-madera-core.c index 898b197c3738..532fa8ac9a94 100644 --- a/drivers/pinctrl/cirrus/pinctrl-madera-core.c +++ b/drivers/pinctrl/cirrus/pinctrl-madera-core.c @@ -1064,10 +1064,9 @@ static int madera_pin_probe(struct platform_device *pdev) ret = pinctrl_register_mappings(pdata->gpio_configs, pdata->n_gpio_configs); if (ret) { - dev_err(priv->dev, - "Failed to register pdata mappings (%d)\n", - ret); - return ret; + return dev_err_probe(priv->dev, ret, + "Failed to register pdata mappings (%d)\n", + ret); } }
Switch to use dev_err_probe() to simplify the error path and unify a message template. Using this helper is totally fine even if err is known to never be -EPROBE_DEFER. The benefit compared to a normal dev_err() is the standardized format of the error code, it being emitted symbolically and the fact that the error code is returned which allows more compact error paths. Signed-off-by: Yan Zhen <yanzhen@vivo.com> --- drivers/irqchip/irq-madera.c | 5 ++--- drivers/pinctrl/cirrus/pinctrl-madera-core.c | 7 +++---- 2 files changed, 5 insertions(+), 7 deletions(-)