RE: [PATCH v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

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

 



> 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




[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