Re: [PATCH v5 2/2] serial: sc16is7xx: hardware reset chip if reset-gpios is defined in DT

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 17 Jun 2024 18:49:30 +0200
Lech Perczak <lech.perczak@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxxx>
> 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@xxxxxxxxxxxx>
> >
> >
> > --
> > 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@xxxxxxxxxxxxxxx
> Website: http://www.camlingroup.com


-- 
Hugo Villeneuve




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux