> From: Hugo Villeneuve <hugo@xxxxxxxxxxx>
> Sent: Tuesday, 4 June 2024 16:23
<...>
> Add function description from original comment "Reset device,
> purging any pending irq / data", since the comment applies to both
> hardware and software reset,
No it does not (at this moment). See below.
> > +static int sc16is7xx_reset(struct device *dev, struct regmap *regmaps[])
>
> Simply pass "struct regmap *regmap" as the second argument. See
> sc16is7xx_setup_mctrl_ports() for example.
>
> > +{
> > + struct gpio_desc *reset_gpiod;
>
> reset_gpiod -> reset_gpio
>
> > + else if (!IS_ERR(reset_gpiod)) {
> > + /* delay 5 us (at least 3 us) and deassert the gpio to exit the hard
> reset */
>
> You can omit the "delay 5 us" since it is obvious from the code. Maybe
> add that "The minimum reset pulse width is 3 us" as stated in the
> datasheet.
>
> As a general note for your comments: capitalize the first letter,
> ex: "Deassert GPIO" and not "deassert GPIO".
>
>
> > + udelay(5);
> > + gpiod_set_value_cansleep(reset_gpiod, 0);
>
> Move the comment "deassert the gpio to exit the hard reset" here. You
> could also simplify it as "Deassert GPIO.".
The problem here is that this only deasserts the reset if it were asserted before.
Nothing happens if it already was deasserted. This makes the delay also pretty
useless.
To make this a proper reset pulse for the device you must first assert the reset,
then wait >3us, and finally deassert the reset.
Maarten Brock
