| Message ID | 20240614102952.679806-1-hui.wang@canonical.com |
|---|---|
| State | Needs Review / ACK |
| Headers | show |
| Series | [v5,1/2] dt-bindings: serial: sc16is7xx: add reset-gpios | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
W dniu 14.06.2024 o 12:29, Hui Wang pisze: > In some designs, the chip reset pin is connected to a GPIO, and this > GPIO needs to be set correctly before probing the driver, so add a > reset-gpios in the device tree. > > Acked-by: Conor Dooley <conor.dooley@microchip.com> > Signed-off-by: Hui Wang <hui.wang@canonical.com> > --- > No change in the v5 > > Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml > index 5dec15b7e7c3..88871480018e 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 > @@ -91,6 +94,7 @@ unevaluatedProperties: false > examples: > - | > #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/gpio/gpio.h> > i2c { > #address-cells = <1>; > #size-cells = <0>; > @@ -120,6 +124,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 */ > -- > 2.34.1 Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
On Fri, 14 Jun 2024 18:29:51 +0800 Hui Wang <hui.wang@canonical.com> wrote: > In some designs, the chip reset pin is connected to a GPIO, and this > GPIO needs to be set correctly before probing the driver, so add a > reset-gpios in the device tree. > > Acked-by: Conor Dooley <conor.dooley@microchip.com> > Signed-off-by: Hui Wang <hui.wang@canonical.com> > --- > No change in the v5 > > Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml > index 5dec15b7e7c3..88871480018e 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 > @@ -91,6 +94,7 @@ unevaluatedProperties: false > examples: > - | > #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/gpio/gpio.h> > i2c { > #address-cells = <1>; > #size-cells = <0>; > @@ -120,6 +124,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 */ > -- > 2.34.1 > Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> Tested-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
On Fri, 14 Jun 2024 18:29:52 +0800 Hui Wang <hui.wang@canonical.com> wrote: Hi Hui, > Some boards connect a GPIO to the reset pin, and the reset pin needs > to be set up correctly before accessing the chip. > > Add a function to handle the chip reset. If the reset-gpios is defined > in the DT, do hardware reset through this GPIO, otherwise do software > reset as before. > > Signed-off-by: Hui Wang <hui.wang@canonical.com> > --- > in the V5: > - change setup to set up in the commit header > - change othwerwise to otherwise in the commit header > - change usleep_range(5, 10) to fsleep(5) > > drivers/tty/serial/sc16is7xx.c | 34 +++++++++++++++++++++++++++++++--- > 1 file changed, 31 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > index bf0065d1c8e9..eefa40006c71 100644 > --- a/drivers/tty/serial/sc16is7xx.c > +++ b/drivers/tty/serial/sc16is7xx.c > @@ -14,6 +14,7 @@ > #include <linux/delay.h> > #include <linux/device.h> > #include <linux/export.h> > +#include <linux/gpio/consumer.h> > #include <linux/gpio/driver.h> > #include <linux/idr.h> > #include <linux/kthread.h> > @@ -1467,6 +1468,32 @@ static const struct serial_rs485 sc16is7xx_rs485_supported = { > .delay_rts_after_send = 1, /* Not supported but keep returning -EINVAL */ > }; > > +/* Reset device, purging any pending irq / data */ > +static int sc16is7xx_reset(struct device *dev, struct regmap *regmap) > +{ > + struct gpio_desc *reset_gpio; > + > + /* > + * The reset input is active low, and flag GPIOD_OUT_HIGH ensures the > + * GPIO is low once devm_gpiod_get_optional returns a valid gpio_desc. > + */ I would replace all the above comments with: /* Assert reset GPIO if defined and valid. */ The correct polarity is already defined by the device tree reset-gpios entry, and can be high or low depending on the design (ex: there can be an inverter between the CPU and the chip reset input, etc). > + reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); > + if (IS_ERR(reset_gpio)) > + return dev_err_probe(dev, PTR_ERR(reset_gpio), "Failed to get reset GPIO\n"); > + > + if (reset_gpio) { > + /* The minimum reset pulse width is 3 us. */ > + fsleep(5); > + gpiod_set_value_cansleep(reset_gpio, 0); /* Deassert GPIO */ > + } else { > + /* Software reset */ > + regmap_write(regmap, SC16IS7XX_IOCONTROL_REG, > + SC16IS7XX_IOCONTROL_SRESET_BIT); > + } > + > + return 0; > +} > + > int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, > struct regmap *regmaps[], int irq) > { > @@ -1536,9 +1563,9 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, > } > sched_set_fifo(s->kworker_task); > > - /* reset device, purging any pending irq / data */ > - regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG, > - SC16IS7XX_IOCONTROL_SRESET_BIT); > + ret = sc16is7xx_reset(dev, regmaps[0]); > + if (ret) > + goto out_kthread; > > /* Mark each port line and status as uninitialised. */ > for (i = 0; i < devtype->nr_uart; ++i) { > @@ -1663,6 +1690,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, > uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port); > } > > +out_kthread: > kthread_stop(s->kworker_task); > > out_clk: > -- > 2.34.1 > I could not test the validity of the 3us delay since I do not have an oscilloscope, but testing with a 10s delay instead and a multimeter showed that it works ok. You can add my Tested-by tag: Tested-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> And if you modify the comment as I suggested above, then you can add my R-b tag: Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
On Mon, 17 Jun 2024 18:49:30 +0200 Lech Perczak <lech.perczak@camlingroup.com> wrote: > Hello, > > W dniu 17.06.2024 o 18:03, Hugo Villeneuve pisze: > > On Fri, 14 Jun 2024 18:29:52 +0800 > > Hui Wang <hui.wang@canonical.com> wrote: > > > > Hi Hui, > > > >> Some boards connect a GPIO to the reset pin, and the reset pin needs > >> to be set up correctly before accessing the chip. > >> > >> Add a function to handle the chip reset. If the reset-gpios is defined > >> in the DT, do hardware reset through this GPIO, otherwise do software > >> reset as before. > >> > >> Signed-off-by: Hui Wang <hui.wang@canonical.com> > >> --- > >> in the V5: > >> - change setup to set up in the commit header > >> - change othwerwise to otherwise in the commit header > >> - change usleep_range(5, 10) to fsleep(5) > >> > >> drivers/tty/serial/sc16is7xx.c | 34 +++++++++++++++++++++++++++++++--- > >> 1 file changed, 31 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > >> index bf0065d1c8e9..eefa40006c71 100644 > >> --- a/drivers/tty/serial/sc16is7xx.c > >> +++ b/drivers/tty/serial/sc16is7xx.c > >> @@ -14,6 +14,7 @@ > >> #include <linux/delay.h> > >> #include <linux/device.h> > >> #include <linux/export.h> > >> +#include <linux/gpio/consumer.h> > >> #include <linux/gpio/driver.h> > >> #include <linux/idr.h> > >> #include <linux/kthread.h> > >> @@ -1467,6 +1468,32 @@ static const struct serial_rs485 sc16is7xx_rs485_supported = { > >> .delay_rts_after_send = 1, /* Not supported but keep returning -EINVAL */ > >> }; > >> > >> +/* Reset device, purging any pending irq / data */ > >> +static int sc16is7xx_reset(struct device *dev, struct regmap *regmap) > >> +{ > >> + struct gpio_desc *reset_gpio; > >> + > >> + /* > >> + * The reset input is active low, and flag GPIOD_OUT_HIGH ensures the > >> + * GPIO is low once devm_gpiod_get_optional returns a valid gpio_desc. > >> + */ > > I would replace all the above comments with: > > > > /* Assert reset GPIO if defined and valid. */ > > > > The correct polarity is already defined by the device > > tree reset-gpios entry, and can be high or low depending on the design > > (ex: there can be an inverter between the CPU and the chip reset input, > > etc). > Seconded - this way it's clear for the reader. > >> + reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); > >> + if (IS_ERR(reset_gpio)) > >> + return dev_err_probe(dev, PTR_ERR(reset_gpio), "Failed to get reset GPIO\n"); > >> + > >> + if (reset_gpio) { > >> + /* The minimum reset pulse width is 3 us. */ > >> + fsleep(5); > >> + gpiod_set_value_cansleep(reset_gpio, 0); /* Deassert GPIO */ > >> + } else { > >> + /* Software reset */ > >> + regmap_write(regmap, SC16IS7XX_IOCONTROL_REG, > >> + SC16IS7XX_IOCONTROL_SRESET_BIT); > >> + } > >> + > >> + return 0; > >> +} > >> + > >> int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, > >> struct regmap *regmaps[], int irq) > >> { > >> @@ -1536,9 +1563,9 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, > >> } > >> sched_set_fifo(s->kworker_task); > >> > >> - /* reset device, purging any pending irq / data */ > >> - regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG, > >> - SC16IS7XX_IOCONTROL_SRESET_BIT); > >> + ret = sc16is7xx_reset(dev, regmaps[0]); > >> + if (ret) > >> + goto out_kthread; > >> > >> /* Mark each port line and status as uninitialised. */ > >> for (i = 0; i < devtype->nr_uart; ++i) { > >> @@ -1663,6 +1690,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, > >> uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port); > >> } > >> > >> +out_kthread: > >> kthread_stop(s->kworker_task); > >> > >> out_clk: > >> -- > >> 2.34.1 > >> > > I could not test the validity of the 3us delay since I do not have an > > oscilloscope, but testing with a 10s delay instead and a > > multimeter showed that it works ok. You can add my Tested-by tag: > > > > Tested-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > My hardware doesn't connect this line to the CPU's GPIOs, so I couldn't test this properly - but you can at least have my R-b tag. Hi Lech, just to mention that on my hardware the SC16 reset line is also not connected to the CPU, so I only tested the GPIO assert/deassert logic. Hugo. > > > > And if you modify the comment as I suggested above, then you can add my > > R-b tag: > > > > Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > > > -- > > Hugo Villeneuve > > > > ___ > > This email originated from outside of Camlin Group. Do not click on links or open attachments unless you are confident they are secure. If in doubt, please raise a security incident via the following portal: Camlin Helpdesk – Report an Information Security Incident/Non-Conformance <https://camlin-helpdesk.atlassian.net/servicedesk/customer/portal/2/group/34/create/62> > > -- > Pozdrawiam/With kind regards, > Lech Perczak > > Sr. Software Engineer > Camlin Technologies Poland Limited Sp. z o.o. > Strzegomska 54, > 53-611 Wroclaw > Tel: (+48) 71 75 000 16 > Email: lech.perczak@camlingroup.com > Website: http://www.camlingroup.com
On Mon, Jun 17, 2024 at 6:49 PM Lech Perczak <lech.perczak@camlingroup.com> wrote: > W dniu 17.06.2024 o 18:03, Hugo Villeneuve pisze: > On Fri, 14 Jun 2024 18:29:52 +0800 > Hui Wang <hui.wang@canonical.com> wrote: ... > My hardware doesn't connect this line to the CPU's GPIOs, so I couldn't test this properly - but you can at least have my R-b tag. Lech, you need to provide a formal tag as it's described in Submitting Patches.
W dniu 17.06.2024 o 22:43, Andy Shevchenko pisze: > On Mon, Jun 17, 2024 at 6:49 PM Lech Perczak > <lech.perczak@camlingroup.com> wrote: >> W dniu 17.06.2024 o 18:03, Hugo Villeneuve pisze: >> On Fri, 14 Jun 2024 18:29:52 +0800 >> Hui Wang <hui.wang@canonical.com> wrote: > ... > >> My hardware doesn't connect this line to the CPU's GPIOs, so I couldn't test this properly - but you can at least have my R-b tag. > Lech, you need to provide a formal tag as it's described in Submitting Patches. I already did for v4, but to make it clear for v5 too: Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com> > > > -- > With Best Regards, > Andy Shevchenko
On 6/18/24 00:03, Hugo Villeneuve wrote: > On Fri, 14 Jun 2024 18:29:52 +0800 > Hui Wang <hui.wang@canonical.com> wrote: > > Hi Hui, > >> Some boards connect a GPIO to the reset pin, and the reset pin needs ... >> + struct gpio_desc *reset_gpio; >> + >> + /* >> + * The reset input is active low, and flag GPIOD_OUT_HIGH ensures the >> + * GPIO is low once devm_gpiod_get_optional returns a valid gpio_desc. >> + */ > I would replace all the above comments with: > > /* Assert reset GPIO if defined and valid. */ > > The correct polarity is already defined by the device > tree reset-gpios entry, and can be high or low depending on the design > (ex: there can be an inverter between the CPU and the chip reset input, > etc). > Agree with that, I will change it in the v6. >> + reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); >> + if (IS_ERR(reset_gpio)) >> + return dev_err_probe(dev, PTR_ERR(reset_gpio), "Failed to get reset GPIO\n"); >> + ... >> +out_kthread: >> kthread_stop(s->kworker_task); >> >> out_clk: >> -- >> 2.34.1 >> > I could not test the validity of the 3us delay since I do not have an > oscilloscope, but testing with a 10s delay instead and a > multimeter showed that it works ok. You can add my Tested-by tag: > > Tested-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > And if you modify the comment as I suggested above, then you can add my > R-b tag: > > Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> OK. thanks. >
diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml index 5dec15b7e7c3..88871480018e 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 @@ -91,6 +94,7 @@ unevaluatedProperties: false examples: - | #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/gpio/gpio.h> i2c { #address-cells = <1>; #size-cells = <0>; @@ -120,6 +124,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 */