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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html