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).
Thanks,
Hui.
