On 2023/8/19 22:45, Andi Shyti wrote:
> Hi Russel,
>
>>>>>> i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
>>>>>> - if (!i2c_imx->pinctrl || IS_ERR(i2c_imx->pinctrl)) {
>>>>>> + if (IS_ERR(i2c_imx->pinctrl)) {
>>>>>> dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
>>>>>> return PTR_ERR(i2c_imx->pinctrl);
>>>>>> }
>>>>>
>>>>> I haven't looked at the AT91 version, but... isn't the original code
>>>>> entirely correct?
>>>>>
>>>>> If pinctrl is not available (thus devm_pinctrl_get() returns NULL) then
>>>>> recovery can't work, because we can't switch the I2C pins between the
>>>>> I2C controller and GPIO. So, isn't it quite correct to print
>>>>> "can't get pinctrl, bus recovery not supported" because the I2C bus
>>>>> can't be recovered without pinctrl?
>>>>>
>>>>> The PTR_ERR() is also fine - because if pinctrl is not present and
>>>>> returns NULL, we'll end up returning zero, which is exactly what we
>>>>> want.
>>>>
>>>> Oh, you're probably absolutely right about that.
>>>>
>>>>> The alternative would be to open code that, maybe with a more accurate
>>>>> message:
>>>>>
>>>>> if (!i2c_imx->pinctrl) {
>>>>> dev_info(&pdev->dev, "pinctrl unavailable, bus recovery not supported\n");
>>>>> return 0;
>>>>> }
>>>>> if (IS_ERR(i2c_imx->pinctrl) {
>>>>> ...
>>>>
>>>> This is a way better patch. It makes the implicit explicit.
>>>
>>> we could also use
>>>
>>> if (IS_ERR_OR_NULL(i2c_imx->pinctrl))
>>> ...
>>>
>>> without changing any logic in the driver.
>>
>> IS_ERR_OR_NULL() - is a macro I personally hate, it causes a lot of
>> trouble. I have mutt setup to mark IS_ERR_OR_NULL with a red background
>> so it stands out in patches. It is utterly evil, and I really wish we
>> could get rid of that damn macro.
>>
>> It also looks wrong.
>>
>> if (IS_ERR_OR_NULL(x))
>> return PTR_ERR(x);
>>
>> rings alarm bells for some people, because if x is NULL, then
>> PTR_ERR(x) is zero.
>>
>> While this may be what is intended in this case, for a great many
>> places in the kernel, this is a bug. So I can guarantee that
>> _someone_ will come along and want to "fix" that to make the NULL
>> case return an error code, and in doing so end up breaking the
>> driver.
>>
>> So... no, just don't.
>>
>> This is why having two if() statements are a good idea, and is
>> what Linus means by "making the implicit explicit" - because it
>> then becomes absolutely obvious what we want to do in the NULL
>> case, and what we want to do in the error case.
>>
>> There is none of this ambiguity that I point out above.
>
> Yes, I fully agree, IS_ERR_OR_NULL() shoud be almost never be
> used in an exit path (unless you are in a void function and few
> other cases, like (borderline) this one).
>
> I'm OK also if Ruan goes with what you suggested.
I'll do as what Russel suggested. Thank you!
>
> Andi
