Re: [PATCH v3 2/9] i2c: mlxbf: Use dev_err_probe in probe function

[Krzysztof Kozlowski]

On 08/08/2023 13:47, Andi Shyti wrote:
> Hi Krzysztof,
> 
> On Tue, Aug 08, 2023 at 01:31:31PM +0200, Krzysztof Kozlowski wrote:
>> On 08/08/2023 13:29, Andi Shyti wrote:
>>> Hi Krzysztof,
>>>
>>> On Tue, Aug 08, 2023 at 10:36:40AM +0200, Krzysztof Kozlowski wrote:
>>>> On 08/08/2023 03:29, Liao Chang wrote:
>>>>> Use the dev_err_probe function instead of dev_err in the probe function
>>>>> so that the printed messge includes the return value and also handles
>>>>> -EPROBE_DEFER nicely.
>>>>>
>>>>> Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxx>
>>>>> Signed-off-by: Liao Chang <liaochang1@xxxxxxxxxx>
>>>>
>>>> ...
>>>>
>>>>> @@ -2413,10 +2399,8 @@ static int mlxbf_i2c_probe(struct platform_device *pdev)
>>>>>  	ret = devm_request_irq(dev, irq, mlxbf_i2c_irq,
>>>>>  			       IRQF_SHARED | IRQF_PROBE_SHARED,
>>>>>  			       dev_name(dev), priv);
>>>>> -	if (ret < 0) {
>>>>> -		dev_err(dev, "Cannot get irq %d\n", irq);
>>>>> -		return ret;
>>>>> -	}
>>>>> +	if (ret < 0)
>>>>> +		return dev_err_probe(dev, ret, "Cannot get irq %d\n", irq);
>>>>
>>>> I don't think this is needed:
>>>> https://lore.kernel.org/all/20230721094641.77189-1-frank.li@xxxxxxxx/
>>>
>>> Hmm, that's a bit borderline, I'd say. The change to
>>
>> What's borderline exactly? devm_request_threaded_irq_probe() is coming,
>> right? If it is accepted this hunk is useless and soon should be
>> replaced with proper one.
> 
> Such change is out of the scope of this series, there are two
> options that I'd prefer (in the listed order):
> 
>  1. accept the patch as it is, this patch is not sent today the
>     first time and at the current state it's correct.
>  2. not accept a change on this line

The 2 is what I commented here. This change should not be made and
instead we should just switch all such users to new API, because this is
preferred for all error messages, when applicable and does not result in
lost context. If there was no such API, sure, but we have this API coming.

> 
> Replacing devm_request_irq belongs to another series and,
> besides, I don't want to ask Liao to hold on this series for such
> trivialities.

So the comment about this redundant and unneeded change, thus switching
to new API you call 'triviality' but a comment of yours of changing the
tone of error message to 'please' is appropriate.
https://lore.kernel.org/all/20230807231320.svssge6uymw3jiho@intel.intel/

That's double standards.

Best regards,
Krzysztof