| Message ID | 20240603123710.649549-1-hui.wang@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] dt-bindings: serial: sc16is7xx: add reset-gpios | expand |
On Mon, 3 Jun 2024 20:37:10 +0800 Hui Wang <hui.wang@canonical.com> wrote: Hi Hui, > Certain designs connect a gpio to the reset pin, and the reset pin > needs to be setup correctly before accessing the chip. > > Here adding a function to handle the reset pin. This change has no > impact if there is no reset_gpios defined in the device tree. > > Signed-off-by: Hui Wang <hui.wang@canonical.com> > --- > drivers/tty/serial/sc16is7xx.c | 22 ++++++++++++++++++++++ > drivers/tty/serial/sc16is7xx.h | 2 ++ > drivers/tty/serial/sc16is7xx_i2c.c | 2 ++ > drivers/tty/serial/sc16is7xx_spi.c | 2 ++ > 4 files changed, 28 insertions(+) > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > index bf0065d1c8e9..53bfb603b03c 100644 > --- a/drivers/tty/serial/sc16is7xx.c > +++ b/drivers/tty/serial/sc16is7xx.c > @@ -19,6 +19,7 @@ > #include <linux/kthread.h> > #include <linux/mod_devicetable.h> > #include <linux/module.h> > +#include <linux/of_gpio.h> > #include <linux/property.h> > #include <linux/regmap.h> > #include <linux/sched.h> > @@ -1467,6 +1468,27 @@ static const struct serial_rs485 sc16is7xx_rs485_supported = { > .delay_rts_after_send = 1, /* Not supported but keep returning -EINVAL */ > }; > > +void sc16is7xx_setup_reset_pin(struct device *dev) Potentially rename to sc16is7xx_reset() based on my comments below about software reset. > +{ > + struct device_node *np = dev->of_node; > + int reset_gpio, err; > + > + reset_gpio = of_get_named_gpio(np, "reset-gpios", 0); Maybe use devm_gpiod_get_optional() to simplify and avoid OF-specific code. > + if (!gpio_is_valid(reset_gpio)) > + return; > + > + err = devm_gpio_request_one(dev, reset_gpio, GPIOF_OUT_INIT_LOW, > + "sc16is7xx-reset"); > + if (err) { > + dev_err(dev, "failed to request sc16is7xx-reset-gpios: %d\n", err); > + return; When this error happens, you return no error to the calling function, why? If you specify a reset GPIO in your device tree, and you cannot use it, seems like an error worth reporting. > + } > + > + /* Deassert the reset pin */ Do you respect the manufacturer's minimum reset pulse width? The datasheet states that its 3 us, so maybe add a delay before deassertion. > + gpio_set_value_cansleep(reset_gpio, 1); > +} > +EXPORT_SYMBOL_GPL(sc16is7xx_setup_reset_pin); > + > int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, > struct regmap *regmaps[], int irq) > { > diff --git a/drivers/tty/serial/sc16is7xx.h b/drivers/tty/serial/sc16is7xx.h > index afb784eaee45..f4ae114cc41a 100644 > --- a/drivers/tty/serial/sc16is7xx.h > +++ b/drivers/tty/serial/sc16is7xx.h > @@ -33,6 +33,8 @@ const char *sc16is7xx_regmap_name(u8 port_id); > > unsigned int sc16is7xx_regmap_port_mask(unsigned int port_id); > > +void sc16is7xx_setup_reset_pin(struct device *dev); > + > int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, > struct regmap *regmaps[], int irq); > > diff --git a/drivers/tty/serial/sc16is7xx_i2c.c b/drivers/tty/serial/sc16is7xx_i2c.c > index 3ed47c306d85..9833c3b935c2 100644 > --- a/drivers/tty/serial/sc16is7xx_i2c.c > +++ b/drivers/tty/serial/sc16is7xx_i2c.c > @@ -21,6 +21,8 @@ static int sc16is7xx_i2c_probe(struct i2c_client *i2c) > if (!devtype) > return dev_err_probe(&i2c->dev, -ENODEV, "Failed to match device\n"); > > + sc16is7xx_setup_reset_pin(&i2c->dev); Move this call inside sc16is7xx_probe() function, since it is common to both i2c and spi interfaces. Also, you will see in sc16is7xx_probe() that we already issue a software reset. If you specify a hardware reset pin, then you shouldn't issue the software reset. > + > memcpy(®cfg, &sc16is7xx_regcfg, sizeof(struct regmap_config)); > > for (i = 0; i < devtype->nr_uart; i++) { > diff --git a/drivers/tty/serial/sc16is7xx_spi.c b/drivers/tty/serial/sc16is7xx_spi.c > index 73df36f8a7fd..ce38561faaf0 100644 > --- a/drivers/tty/serial/sc16is7xx_spi.c > +++ b/drivers/tty/serial/sc16is7xx_spi.c > @@ -38,6 +38,8 @@ static int sc16is7xx_spi_probe(struct spi_device *spi) > if (!devtype) > return dev_err_probe(&spi->dev, -ENODEV, "Failed to match device\n"); > > + sc16is7xx_setup_reset_pin(&spi->dev); > + > memcpy(®cfg, &sc16is7xx_regcfg, sizeof(struct regmap_config)); > > for (i = 0; i < devtype->nr_uart; i++) { > -- > 2.34.1 > > >
On 03/06/2024 14:37, Hui Wang wrote: > In some designs, the chip reset pin is connected to a gpio, this > gpio needs to be set correctly before probing the driver, so adding > a reset-gpios in the device tree. > > Signed-off-by: Hui Wang <hui.wang@canonical.com> > --- > Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml | 4 ++++ > 1 file changed, 4 insertions(+) Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel. Tools like b4 or scripts/get_maintainer.pl provide you proper list of people, so fix your workflow. Tools might also fail if you work on some ancient tree (don't, instead use mainline), work on fork of kernel (don't, instead use mainline) or you ignore some maintainers (really don't). Just use b4 and everything should be fine, although remember about `b4 prep --auto-to-cc` if you added new patches to the patchset. You missed at least devicetree list (maybe more), so this won't be tested by automated tooling. Performing review on untested code might be a waste of time. Please kindly resend and include all necessary To/Cc entries. Best regards, Krzysztof
Hi Hugo, Will address all your comment in the v2. Thanks, Hui. On 6/3/24 21:11, Hugo Villeneuve wrote: > On Mon, 3 Jun 2024 20:37:10 +0800 > Hui Wang <hui.wang@canonical.com> wrote: > > Hi Hui, > >> Certain designs connect a gpio to the reset pin, and the reset pin >> needs to be setup correctly before accessing the chip. >> >> Here adding a function to handle the reset pin. This change has no >> impact if there is no reset_gpios defined in the device tree. >> >> Signed-off-by: Hui Wang <hui.wang@canonical.com> >> --- >> drivers/tty/serial/sc16is7xx.c | 22 ++++++++++++++++++++++ >> drivers/tty/serial/sc16is7xx.h | 2 ++ >> drivers/tty/serial/sc16is7xx_i2c.c | 2 ++ >> drivers/tty/serial/sc16is7xx_spi.c | 2 ++ >> 4 files changed, 28 insertions(+) >> >> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c >> index bf0065d1c8e9..53bfb603b03c 100644 >> --- a/drivers/tty/serial/sc16is7xx.c >> +++ b/drivers/tty/serial/sc16is7xx.c >> @@ -19,6 +19,7 @@ >> #include <linux/kthread.h> >> #include <linux/mod_devicetable.h> >> #include <linux/module.h> >> +#include <linux/of_gpio.h> >> #include <linux/property.h> >> #include <linux/regmap.h> >> #include <linux/sched.h> >> @@ -1467,6 +1468,27 @@ static const struct serial_rs485 sc16is7xx_rs485_supported = { >> .delay_rts_after_send = 1, /* Not supported but keep returning -EINVAL */ >> }; >> >> +void sc16is7xx_setup_reset_pin(struct device *dev) > Potentially rename to sc16is7xx_reset() based on my comments below > about software reset. > >> +{ >> + struct device_node *np = dev->of_node; >> + int reset_gpio, err; >> + >> + reset_gpio = of_get_named_gpio(np, "reset-gpios", 0); > Maybe use devm_gpiod_get_optional() to simplify and avoid OF-specific > code. > >> + if (!gpio_is_valid(reset_gpio)) >> + return; >> + >> + err = devm_gpio_request_one(dev, reset_gpio, GPIOF_OUT_INIT_LOW, >> + "sc16is7xx-reset"); >> + if (err) { >> + dev_err(dev, "failed to request sc16is7xx-reset-gpios: %d\n", err); >> + return; > When this error happens, you return no error to the calling function, > why? If you specify a reset GPIO in your device tree, and you cannot use > it, seems like an error worth reporting. > >> + } >> + >> + /* Deassert the reset pin */ > Do you respect the manufacturer's minimum reset pulse width? The > datasheet states that its 3 us, so maybe add a delay before deassertion. > >> + gpio_set_value_cansleep(reset_gpio, 1); >> +} >> +EXPORT_SYMBOL_GPL(sc16is7xx_setup_reset_pin); >> + >> int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, >> struct regmap *regmaps[], int irq) >> { >> diff --git a/drivers/tty/serial/sc16is7xx.h b/drivers/tty/serial/sc16is7xx.h >> index afb784eaee45..f4ae114cc41a 100644 >> --- a/drivers/tty/serial/sc16is7xx.h >> +++ b/drivers/tty/serial/sc16is7xx.h >> @@ -33,6 +33,8 @@ const char *sc16is7xx_regmap_name(u8 port_id); >> >> unsigned int sc16is7xx_regmap_port_mask(unsigned int port_id); >> >> +void sc16is7xx_setup_reset_pin(struct device *dev); >> + >> int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, >> struct regmap *regmaps[], int irq); >> >> diff --git a/drivers/tty/serial/sc16is7xx_i2c.c b/drivers/tty/serial/sc16is7xx_i2c.c >> index 3ed47c306d85..9833c3b935c2 100644 >> --- a/drivers/tty/serial/sc16is7xx_i2c.c >> +++ b/drivers/tty/serial/sc16is7xx_i2c.c >> @@ -21,6 +21,8 @@ static int sc16is7xx_i2c_probe(struct i2c_client *i2c) >> if (!devtype) >> return dev_err_probe(&i2c->dev, -ENODEV, "Failed to match device\n"); >> >> + sc16is7xx_setup_reset_pin(&i2c->dev); > Move this call inside sc16is7xx_probe() function, since it is common to > both i2c and spi interfaces. Also, you will see in sc16is7xx_probe() > that we already issue a software reset. If you > specify a hardware reset pin, then you shouldn't issue the software > reset. > >> + >> memcpy(®cfg, &sc16is7xx_regcfg, sizeof(struct regmap_config)); >> >> for (i = 0; i < devtype->nr_uart; i++) { >> diff --git a/drivers/tty/serial/sc16is7xx_spi.c b/drivers/tty/serial/sc16is7xx_spi.c >> index 73df36f8a7fd..ce38561faaf0 100644 >> --- a/drivers/tty/serial/sc16is7xx_spi.c >> +++ b/drivers/tty/serial/sc16is7xx_spi.c >> @@ -38,6 +38,8 @@ static int sc16is7xx_spi_probe(struct spi_device *spi) >> if (!devtype) >> return dev_err_probe(&spi->dev, -ENODEV, "Failed to match device\n"); >> >> + sc16is7xx_setup_reset_pin(&spi->dev); >> + >> memcpy(®cfg, &sc16is7xx_regcfg, sizeof(struct regmap_config)); >> >> for (i = 0; i < devtype->nr_uart; i++) { >> -- >> 2.34.1 >> >> >> >
On 6/3/24 21:18, Rob Herring wrote: > On Mon, Jun 03, 2024 at 08:37:09PM +0800, Hui Wang wrote: >> In some designs, the chip reset pin is connected to a gpio, this >> gpio needs to be set correctly before probing the driver, so adding >> a reset-gpios in the device tree. >> >> Signed-off-by: Hui Wang <hui.wang@canonical.com> >> --- >> Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml >> index 5dec15b7e7c3..62aff6e034cb 100644 >> --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml >> +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml >> @@ -28,6 +28,9 @@ properties: >> clocks: >> maxItems: 1 >> >> + reset-gpios: >> + maxItems: 1 >> + >> clock-frequency: >> description: >> When there is no clock provider visible to the platform, this >> @@ -120,6 +123,7 @@ examples: >> compatible = "nxp,sc16is752"; >> reg = <0x54>; >> clocks = <&clk20m>; >> + reset-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>; > Missing the header for the define. > > Test your bindings before sending. OK, got it. Thanks. > >> interrupt-parent = <&gpio3>; >> interrupts = <7 IRQ_TYPE_EDGE_FALLING>; >> nxp,modem-control-line-ports = <0 1>; /* Ports 0 and 1 as modem control lines */ >> -- >> 2.34.1 >>
On 6/4/24 14:54, Krzysztof Kozlowski wrote: > On 03/06/2024 14:37, Hui Wang wrote: >> In some designs, the chip reset pin is connected to a gpio, this >> gpio needs to be set correctly before probing the driver, so adding >> a reset-gpios in the device tree. >> >> Signed-off-by: Hui Wang <hui.wang@canonical.com> >> --- >> Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml | 4 ++++ >> 1 file changed, 4 insertions(+) > Please use scripts/get_maintainers.pl to get a list of necessary people > and lists to CC. It might happen, that command when run on an older > kernel, gives you outdated entries. Therefore please be sure you base > your patches on recent Linux kernel. > > Tools like b4 or scripts/get_maintainer.pl provide you proper list of > people, so fix your workflow. Tools might also fail if you work on some > ancient tree (don't, instead use mainline), work on fork of kernel > (don't, instead use mainline) or you ignore some maintainers (really > don't). Just use b4 and everything should be fine, although remember > about `b4 prep --auto-to-cc` if you added new patches to the patchset. > > You missed at least devicetree list (maybe more), so this won't be > tested by automated tooling. Performing review on untested code might be > a waste of time. > > Please kindly resend and include all necessary To/Cc entries. > > Best regards, > Krzysztof > Got it. thanks.
diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml index 5dec15b7e7c3..62aff6e034cb 100644 --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml @@ -28,6 +28,9 @@ properties: clocks: maxItems: 1 + reset-gpios: + maxItems: 1 + clock-frequency: description: When there is no clock provider visible to the platform, this @@ -120,6 +123,7 @@ examples: compatible = "nxp,sc16is752"; reg = <0x54>; clocks = <&clk20m>; + reset-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>; interrupt-parent = <&gpio3>; interrupts = <7 IRQ_TYPE_EDGE_FALLING>; nxp,modem-control-line-ports = <0 1>; /* Ports 0 and 1 as modem control lines */
In some designs, the chip reset pin is connected to a gpio, this gpio needs to be set correctly before probing the driver, so adding a reset-gpios in the device tree. Signed-off-by: Hui Wang <hui.wang@canonical.com> --- Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml | 4 ++++ 1 file changed, 4 insertions(+)