> On 6/5/24 18:30, Maarten Brock wrote:
> >> 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
> Hi Maarten,
>
> My understanding is when calling devm_gpiod_get_optional(dev, "reset",
> GPIOD_OUT_LOW) and returning a valid (gpio_desc *), the flag
> GPIOD_OUT_LOW guarantees the GPIO is set to output and low (assert the
> reset pin).
Ah, right. Sorry, I missed that.
So GPIOD_OUT_LOW disregards the inversion from GPIO_ACTIVE_LOW.
And gpiod_set_value_cansleep(reset_gpiod, 0) uses the inversion to make the pin high.
Looks fine to me now.
> Thanks,
>
> Hui.
Maarten
