Re: [PATCH v5] i2c: imx: make bus recovery through pinctrl optional

From: Leo Li
Date: Tue Sep 06 2016 - 19:14:51 EST


On Tue, Sep 6, 2016 at 4:07 PM, Uwe Kleine-KÃnig
<u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> On Tue, Sep 06, 2016 at 03:06:41PM -0500, Leo Li wrote:
>> On Tue, Sep 6, 2016 at 1:58 PM, Uwe Kleine-KÃnig
>> <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
>> > On Fri, Aug 19, 2016 at 05:05:22PM -0500, Li Yang wrote:
>> >> Since commit 1c4b6c3bcf30 ("i2c: imx: implement bus recovery") the
>> >> driver starts to use gpio/pinctrl to do i2c bus recovery. But pinctrl
>> >> is not always available for platforms with this controller such as ls1021a
>> >> and ls1043a, and the device tree binding also mentioned this gpio based
>> >> recovery mechanism as optional. The patch makes it really optional that
>> >> the probe function won't bailout but just disable the bus recovery function
>> >> when pinctrl is not available.
>> >>
>> >> Signed-off-by: Li Yang <leoyang.li@xxxxxxx>
>> >> Cc: Gao Pan <pandy.gao@xxxxxxx>
>> >> Cc: Uwe Kleine-KÃnig <u.kleine-koenig@xxxxxxxxxxxxxx>
>> >> Acked-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
>> >> ---
>> >> v5:
>> >> Revert the last minute change of recovery info initialization timing, it
>> >> will cause problem if initialized after i2c_add_numbered_adapter()
>> >>
>> >> v4:
>> >> Remove the use of IS_ERR_OR_NULL
>> >> Move the condition judgement to i2c_imx_init_recovery_info()
>> >> Change the timing of recovery initialization to be after bus registration
>> >>
>> >> v3:
>> >> Rebased to Wolfram's for-next branch
>> >> Added acked-by from Linus Walleij
>> >> Update to use new nxp email addresses due to company merge
>> >>
>> >> drivers/i2c/busses/i2c-imx.c | 15 ++++++++++++++-
>> >> 1 file changed, 14 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
>> >> index 1844bc9..7ae7992 100644
>> >> --- a/drivers/i2c/busses/i2c-imx.c
>> >> +++ b/drivers/i2c/busses/i2c-imx.c
>> >> @@ -989,6 +989,15 @@ static void i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
>> >> {
>> >> struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
>> >>
>> >> + /* if pinctrl is not supported on the system */
>> >> + if (IS_ERR(i2c_imx->pinctrl))
>> >> + i2c_imx->pinctrl = NULL;
>> >> +
>> >> + if (!i2c_imx->pinctrl) {
>> >> + dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
>> >> + return;
>> >> + }
>> >> +
>> >> i2c_imx->pinctrl_pins_default = pinctrl_lookup_state(i2c_imx->pinctrl,
>> >> PINCTRL_STATE_DEFAULT);
>> >> i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
>> >> @@ -1081,8 +1090,11 @@ static int i2c_imx_probe(struct platform_device *pdev)
>> >> return ret;
>> >> }
>> >>
>> >> + /* optional bus recovery feature through pinctrl */
>> >> i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
>> >> - if (IS_ERR(i2c_imx->pinctrl)) {
>> >> + /* bailout on -ENOMEM or -EPROBE_DEFER, continue for other errors */
>> >> + if (PTR_ERR(i2c_imx->pinctrl) == -ENOMEM ||
>> >> + PTR_ERR(i2c_imx->pinctrl) == -EPROBE_DEFER) {
>> >> ret = PTR_ERR(i2c_imx->pinctrl);
>> >> goto clk_disable;
>> >> }
>> >
>> > devm_pinctrl_get might return the following error-valued pointers:
>> > - -EINVAL
>> > - -ENOMEM
>> > - -ENODEV
>> > - -EPROBE_DEFER
>> >
>> > There are several error paths returning -EINVAL, one is when an invalid
>> > phandle is used. Do you really want to ignore that?
>> >
>> > IMO error handling is better done with inverse logic, that is continue
>> > on some explicit error, bail out on all unknown stuff. This tends to be
>> > more robust. Also the comment should be improved to not explain that for
>> > -ENOMEM and -EPROBE_DEFER we bail out (which should be obvious for
>> > anyone who can read C) but to explain why.
>>
>> What you said is true for normal error handling, but in this scenario
>> it is intentional to ignore all pinctrl related errors except critical
>> ones because failing to have pinctrl for an optional feature shouldn't
>> impact the function of normal i2c. We choose to catch -ENOMEM because
>> the error could also cause problem for i2c probe, and -EPROBE_DEFER
>> because it's possible that the pinctrl will be ready later and we want
>> to give it a chance. The i2c driver really don't care why the pinctrl
>> was not usable. I thought I added comment before the
>> devm_pinctrl_get() to explain that the pinctrl is optional. Hopefully
>> you are not suggesting to explain what -ENOMEM and -EPROBE_DEFER mean
>> in the comment. :)
>
> You wrote
>
> /* optional bus recovery feature through pinctrl */
>
> there. I'd expect to read something like:
>
> /*
> * If pinctrl is available it might be possible to switch the
> * SDA and SCL lines to their GPIO functions and do a recovery
> * procedure in this mode. If there is anything wrong with
> * pinctrl we cannot do this and simply skip it.
> */

Thanks. I will use this. This does looks more clear. And more verbose too. :)

>
>> > Also I'd put
>> >
>> > i2c_imx->pinctrl = NULL;
>> >
>> > directly after devm_pinctrl_get() which I consider the more obvious
>> > place for this.
>>
>> On the other hand, put it together with the actually judgement can
>> make it clear that it is catching both IS_ERR and NULL returns from
>> devm_pinctrl_get()?
>
> When you do it immediately, you have:
>
> if (i2c_imx->pinctrl)
> use_it();
> else
> ignore_it();
>

But you suggested the other way last time "Or maybe make
i2c_imx_init_recovery_info aware of this situation to keep the caller
simple?" :) Either way I think it is just personal preferences with
both subtle pros and cons. I don't think we should spend too much
time on this.

Regards,
Leo