| Message ID | 20230816095015.23705-1-yann@sionneau.net |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] i2c: designware: add support for pinctrl for recovery | expand |
Hi On 8/16/23 12:50, Yann Sionneau wrote: > From: Yann Sionneau <ysionneau@kalray.eu> > > Currently if the SoC needs pinctrl to switch the SCL and SDA > from the I2C function to GPIO function, the recovery won't work. > > scl-gpio = <>; > sda-gpio = <>; > > Are not enough for some SoCs to have a working recovery. > Some need: > > scl-gpio = <>; > sda-gpio = <>; > pinctrl-names = "default", "recovery"; > pinctrl-0 = <&i2c_pins_hw>; > pinctrl-1 = <&i2c_pins_gpio>; > > The driver was not filling rinfo->pinctrl with the device node > pinctrl data which is needed by generic recovery code. > > Tested-by: Yann Sionneau <ysionneau@kalray.eu> > Signed-off-by: Yann Sionneau <ysionneau@kalray.eu> Tested-by from author is needless. Expectation is that author has tested the patch while not always true :-) > @@ -905,6 +906,15 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev) > return PTR_ERR(gpio); > rinfo->sda_gpiod = gpio; > > + rinfo->pinctrl = devm_pinctrl_get(dev->dev); > + if (IS_ERR(rinfo->pinctrl)) { > + if (PTR_ERR(rinfo->pinctrl) == -EPROBE_DEFER) > + return PTR_ERR(rinfo->pinctrl); > + > + rinfo->pinctrl = NULL; > + dev_info(dev->dev, "can't get pinctrl, bus recovery might not work\n"); I think dev_dbg() suits better here or is it needed at all? End user may not be able to do anything when sees this in dmesg. I.e. more like development time dev_dbg() information. Does i2c-core-base.c: i2c_gpio_init_pinctrl_recovery() already do dev_info() print when pinctrl & GPIO are set properly making above also kind of needless? Jarkko
Hi Le 17/08/2023 à 10:07, Jarkko Nikula a écrit : > Hi > > On 8/16/23 12:50, Yann Sionneau wrote: >> From: Yann Sionneau <ysionneau@kalray.eu> >> >> Currently if the SoC needs pinctrl to switch the SCL and SDA >> from the I2C function to GPIO function, the recovery won't work. >> >> scl-gpio = <>; >> sda-gpio = <>; >> >> Are not enough for some SoCs to have a working recovery. >> Some need: >> >> scl-gpio = <>; >> sda-gpio = <>; >> pinctrl-names = "default", "recovery"; >> pinctrl-0 = <&i2c_pins_hw>; >> pinctrl-1 = <&i2c_pins_gpio>; >> >> The driver was not filling rinfo->pinctrl with the device node >> pinctrl data which is needed by generic recovery code. >> >> Tested-by: Yann Sionneau <ysionneau@kalray.eu> >> Signed-off-by: Yann Sionneau <ysionneau@kalray.eu> > > Tested-by from author is needless. Expectation is that author has > tested the patch while not always true :-) Ok, I just wanted to emphasize the fact that I have the device and I tested the change with the device. Ack! > >> @@ -905,6 +906,15 @@ static int i2c_dw_init_recovery_info(struct >> dw_i2c_dev *dev) >> return PTR_ERR(gpio); >> rinfo->sda_gpiod = gpio; >> + rinfo->pinctrl = devm_pinctrl_get(dev->dev); >> + if (IS_ERR(rinfo->pinctrl)) { >> + if (PTR_ERR(rinfo->pinctrl) == -EPROBE_DEFER) >> + return PTR_ERR(rinfo->pinctrl); >> + >> + rinfo->pinctrl = NULL; >> + dev_info(dev->dev, "can't get pinctrl, bus recovery might >> not work\n"); > > I think dev_dbg() suits better here or is it needed at all? End user > may not be able to do anything when sees this in dmesg. I.e. more like > development time dev_dbg() information. I agree dev_dbg() is a better idea. > > Does i2c-core-base.c: i2c_gpio_init_pinctrl_recovery() already do > dev_info() print when pinctrl & GPIO are set properly making above > also kind of needless? Thanks for the review. In fact I had to use gdb to understand why the recovery was not working. Because as you said, it only prints something to say "everything looks ok!". I kind of prefer when it prints when something goes wrong. But I let you decide what you think is the best.
On 8/17/23 17:27, Yann Sionneau wrote: > Hi > > Le 17/08/2023 à 10:07, Jarkko Nikula a écrit : >> Hi >> >> On 8/16/23 12:50, Yann Sionneau wrote: >>> From: Yann Sionneau <ysionneau@kalray.eu> >>> >>> Currently if the SoC needs pinctrl to switch the SCL and SDA >>> from the I2C function to GPIO function, the recovery won't work. >>> >>> scl-gpio = <>; >>> sda-gpio = <>; >>> >>> Are not enough for some SoCs to have a working recovery. >>> Some need: >>> >>> scl-gpio = <>; >>> sda-gpio = <>; >>> pinctrl-names = "default", "recovery"; >>> pinctrl-0 = <&i2c_pins_hw>; >>> pinctrl-1 = <&i2c_pins_gpio>; >>> >>> The driver was not filling rinfo->pinctrl with the device node >>> pinctrl data which is needed by generic recovery code. >>> >>> Tested-by: Yann Sionneau <ysionneau@kalray.eu> >>> Signed-off-by: Yann Sionneau <ysionneau@kalray.eu> >> >> Tested-by from author is needless. Expectation is that author has >> tested the patch while not always true :-) > Ok, I just wanted to emphasize the fact that I have the device and I > tested the change with the device. Ack! >> >>> @@ -905,6 +906,15 @@ static int i2c_dw_init_recovery_info(struct >>> dw_i2c_dev *dev) >>> return PTR_ERR(gpio); >>> rinfo->sda_gpiod = gpio; >>> + rinfo->pinctrl = devm_pinctrl_get(dev->dev); >>> + if (IS_ERR(rinfo->pinctrl)) { >>> + if (PTR_ERR(rinfo->pinctrl) == -EPROBE_DEFER) >>> + return PTR_ERR(rinfo->pinctrl); >>> + >>> + rinfo->pinctrl = NULL; >>> + dev_info(dev->dev, "can't get pinctrl, bus recovery might >>> not work\n"); >> >> I think dev_dbg() suits better here or is it needed at all? End user >> may not be able to do anything when sees this in dmesg. I.e. more like >> development time dev_dbg() information. > I agree dev_dbg() is a better idea. >> >> Does i2c-core-base.c: i2c_gpio_init_pinctrl_recovery() already do >> dev_info() print when pinctrl & GPIO are set properly making above >> also kind of needless? > > Thanks for the review. In fact I had to use gdb to understand why the > recovery was not working. Because as you said, it only prints something > to say "everything looks ok!". > > I kind of prefer when it prints when something goes wrong. > > But I let you decide what you think is the best. > Fair enough, dev_dbg() is justified when it makes developer's life easier :-)
Hi, August 20, 2023 1:02 PM, "Andi Shyti" <andi.shyti@kernel.org> wrote: > Hi, > > On Thu, Aug 17, 2023 at 04:27:26PM +0200, Yann Sionneau wrote: > >> Hi >> >> Le 17/08/2023 à 10:07, Jarkko Nikula a écrit : >> Hi >> >> On 8/16/23 12:50, Yann Sionneau wrote: > > From: Yann Sionneau <ysionneau@kalray.eu> > > Currently if the SoC needs pinctrl to switch the SCL and SDA > from the I2C function to GPIO function, the recovery won't work. > > scl-gpio = <>; > sda-gpio = <>; > > Are not enough for some SoCs to have a working recovery. > Some need: > > scl-gpio = <>; > sda-gpio = <>; > pinctrl-names = "default", "recovery"; > pinctrl-0 = <&i2c_pins_hw>; > pinctrl-1 = <&i2c_pins_gpio>; > > The driver was not filling rinfo->pinctrl with the device node > pinctrl data which is needed by generic recovery code. > > Tested-by: Yann Sionneau <ysionneau@kalray.eu> > Signed-off-by: Yann Sionneau <ysionneau@kalray.eu> >> Tested-by from author is needless. Expectation is that author has tested >> the patch while not always true :-) >> Ok, I just wanted to emphasize the fact that I have the device and I tested >> the change with the device. Ack! > > @@ -905,6 +906,15 @@ static int i2c_dw_init_recovery_info(struct > dw_i2c_dev *dev) > return PTR_ERR(gpio); > rinfo->sda_gpiod = gpio; > + rinfo->pinctrl = devm_pinctrl_get(dev->dev); > + if (IS_ERR(rinfo->pinctrl)) { > + if (PTR_ERR(rinfo->pinctrl) == -EPROBE_DEFER) > + return PTR_ERR(rinfo->pinctrl); > + > + rinfo->pinctrl = NULL; > + dev_info(dev->dev, "can't get pinctrl, bus recovery might > not work\n"); >> I think dev_dbg() suits better here or is it needed at all? End user may >> not be able to do anything when sees this in dmesg. I.e. more like >> development time dev_dbg() information. >> I agree dev_dbg() is a better idea. >> >> Does i2c-core-base.c: i2c_gpio_init_pinctrl_recovery() already do >> dev_info() print when pinctrl & GPIO are set properly making above also >> kind of needless? >> >> Thanks for the review. In fact I had to use gdb to understand why the >> recovery was not working. Because as you said, it only prints something to >> say "everything looks ok!". >> >> I kind of prefer when it prints when something goes wrong. >> But I let you decide what you think is the best. > > You need to differentiate here between an error and not an error. > If the return value is an ENOMEM, then this is an error. Although > I think you should not return, but the message needs to be an > dev_err(). Ack. > On the other hand, if the return value is a '0', then I think > dev_info() is correct. Ack. > Either remove the logging or make it correct. I'll print a dev_err in case of error (except for -EPROBE_DEFER) and a dev_info in case of NULL. > > One more note, the sentence "can't get pinctrl,... " sounds like > an error. If the pinctrl is not connected on your system, maybe > it's because your system is not designed to have recovery. Please > write a message that doesn't sound like an error (or suppress the > logging). I insist that I would have liked to see a message in case of pinctrl setup failure whatever the reason, even if it's because CONFIG_PINCTRL is not set. That would be a "bug" for the developer, in case he/she wants to have recovery working. Sometimes developer just forget to enable the correct config or to put the correct stuff in DTB and printing debug messages really helps to understand what's going on. Regards,
Hi Yann, > >> Tested-by from author is needless. Expectation is that author has tested > >> the patch while not always true :-) > >> Ok, I just wanted to emphasize the fact that I have the device and I tested > >> the change with the device. Ack! > > > > @@ -905,6 +906,15 @@ static int i2c_dw_init_recovery_info(struct > > dw_i2c_dev *dev) > > return PTR_ERR(gpio); > > rinfo->sda_gpiod = gpio; > > + rinfo->pinctrl = devm_pinctrl_get(dev->dev); > > + if (IS_ERR(rinfo->pinctrl)) { > > + if (PTR_ERR(rinfo->pinctrl) == -EPROBE_DEFER) > > + return PTR_ERR(rinfo->pinctrl); > > + > > + rinfo->pinctrl = NULL; > > + dev_info(dev->dev, "can't get pinctrl, bus recovery might > > not work\n"); > >> I think dev_dbg() suits better here or is it needed at all? End user may > >> not be able to do anything when sees this in dmesg. I.e. more like > >> development time dev_dbg() information. > >> I agree dev_dbg() is a better idea. > >> > >> Does i2c-core-base.c: i2c_gpio_init_pinctrl_recovery() already do > >> dev_info() print when pinctrl & GPIO are set properly making above also > >> kind of needless? > >> > >> Thanks for the review. In fact I had to use gdb to understand why the > >> recovery was not working. Because as you said, it only prints something to > >> say "everything looks ok!". > >> > >> I kind of prefer when it prints when something goes wrong. > >> But I let you decide what you think is the best. > > > > You need to differentiate here between an error and not an error. > > If the return value is an ENOMEM, then this is an error. Although > > I think you should not return, but the message needs to be an > > dev_err(). > > Ack. > > On the other hand, if the return value is a '0', then I think > > dev_info() is correct. > > Ack. > > > Either remove the logging or make it correct. > > I'll print a dev_err in case of error (except for -EPROBE_DEFER) and a dev_info in case of NULL. Maybe dev_warn() is the best if there is a failure but you keep going anyway. > > One more note, the sentence "can't get pinctrl,... " sounds like > > an error. If the pinctrl is not connected on your system, maybe > > it's because your system is not designed to have recovery. Please > > write a message that doesn't sound like an error (or suppress the > > logging). > > I insist that I would have liked to see a message in case of pinctrl setup failure whatever the reason, even if it's because CONFIG_PINCTRL is not set. That would be a "bug" for the developer, in case he/she wants to have recovery working. > Sometimes developer just forget to enable the correct config or to put the correct stuff in DTB and printing debug messages really helps to understand what's going on. Please, read more carefully. I am not suggesting to remove the message; I'm suggesting to reword it so that it doesn't sound as an error if it's not an error. But this is a note, i.e. a minor review. Andi
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index 3bfd7a2232db..b45accbff915 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -17,6 +17,7 @@ #include <linux/interrupt.h> #include <linux/io.h> #include <linux/module.h> +#include <linux/pinctrl/consumer.h> #include <linux/pm_runtime.h> #include <linux/regmap.h> #include <linux/reset.h> @@ -905,6 +906,15 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev) return PTR_ERR(gpio); rinfo->sda_gpiod = gpio; + rinfo->pinctrl = devm_pinctrl_get(dev->dev); + if (IS_ERR(rinfo->pinctrl)) { + if (PTR_ERR(rinfo->pinctrl) == -EPROBE_DEFER) + return PTR_ERR(rinfo->pinctrl); + + rinfo->pinctrl = NULL; + dev_info(dev->dev, "can't get pinctrl, bus recovery might not work\n"); + } + rinfo->recover_bus = i2c_generic_scl_recovery; rinfo->prepare_recovery = i2c_dw_prepare_recovery; rinfo->unprepare_recovery = i2c_dw_unprepare_recovery;