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

[Hui Wang <hui.wang@canonical.com>]

On 6/12/24 21:56, Lech Perczak wrote:
> Hi,
> Some comments inline.
>
> W dniu 12.06.2024 o 15:14, Hui Wang pisze:
>> Some boards connect a GPIO to the reset pin, and the reset pin needs
>> to be setup 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, othwerwise do software
>> reset as before.
>>
>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>> ---
>> In the v3:
>>   - drop Reviewed-by
>>   - adjust the sequence of if, else if and else
>>   - replace PTR_ERR(reset_gpiod) with dev_err_probe
>>   - change GPIOD_OUT_LOW to GPIOD_OUT_HIGH, this will assert the reset pin after requesting the GPIO
>>   - change the 2nd parameter struct regmap *regmap[] to struct regmap *regmap
>>   - address all spelling and description issues in the v2
>>
>>   drivers/tty/serial/sc16is7xx.c | 33 +++++++++++++++++++++++++++++----
>>   1 file changed, 29 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
>> index bf0065d1c8e9..8c7e0fe76049 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,30 @@ 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 GPIO is ACTIVE_LOW, flag GPIOD_OUT_HIGH asserts the reset GPIO */
> s/reset GPIO is ACTIVE_LOW /reset input is active low/ - because we're referring to the chip input here, and this is reflected in DT bindings.
>
> Also, it isn't clear from the comment that GPIO is asserted immediately.
Got it.
>
>
>> +       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. */
>> +               udelay(5);
> Prefer usleep_range() over that, since maximum reset time isn't all that critical.
Got it.
>
>
>> +               gpiod_set_value_cansleep(reset_gpio, 0);
>> +               /* Deassert GPIO */
> This comment should go one line above or be removed entirely.

Got it.

Thanks.

>
>
>> +       } 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)
>>   {
>> @@ -1527,6 +1552,10 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>>          s->devtype = devtype;
>>          dev_set_drvdata(dev, s);
>>
>> +       ret = sc16is7xx_reset(dev, regmaps[0]);
>> +       if (ret)
>> +               goto out_clk;
>> +
>>          kthread_init_worker(&s->kworker);
>>          s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
>>                                        "sc16is7xx");
>> @@ -1536,10 +1565,6 @@ 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);
>> -
>>          /* Mark each port line and status as uninitialised. */
>>          for (i = 0; i < devtype->nr_uart; ++i) {
>>                  s->p[i].port.line = SC16IS7XX_MAX_DEVS;
>> --
>> 2.34.1