[PATCH v1] cirrus: Simplify with dev_err_probe()

[PATCH v1] cirrus: Simplify with dev_err_probe()

[Yan Zhen]

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);
 		}
 	}
 
-- 
2.34.1

Re: [PATCH v1] cirrus: Simplify with dev_err_probe()

[Richard Fitzgerald]

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.

>   		}
>   	}
>   

Re: [PATCH v1] cirrus: Simplify with dev_err_probe()

[Charles Keepax]

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

Re: [PATCH v1] cirrus: Simplify with dev_err_probe()

[Thomas Gleixner]

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

Re: [PATCH v1] cirrus: Simplify with dev_err_probe()

[Richard Fitzgerald]

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

Re: [PATCH v1] cirrus: Simplify with dev_err_probe()

[Charles Keepax]

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

Re: [PATCH v1] cirrus: Simplify with dev_err_probe()

[Thomas Gleixner]

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