| Message ID | 20170524045319.29926-1-joel@jms.id.au |
|---|---|
| State | Superseded |
| Headers | show |
Hi Joel, On Wed, 2017-05-24 at 14:53 +1000, Joel Stanley wrote: > This adds the hooks for an optional reset controller in the 8250 device > tree node. > > Signed-off-by: Joel Stanley <joel@jms.id.au> > --- > Documentation/devicetree/bindings/serial/8250.txt | 1 + > drivers/tty/serial/8250/8250_of.c | 16 ++++++++++++++++ > 2 files changed, 17 insertions(+) > > diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt > index 10276a46ecef..63e32393f82b 100644 > --- a/Documentation/devicetree/bindings/serial/8250.txt > +++ b/Documentation/devicetree/bindings/serial/8250.txt > @@ -45,6 +45,7 @@ Optional properties: > property. > - tx-threshold: Specify the TX FIFO low water indication for parts with > programmable TX FIFO thresholds. > +- resets : phandle + reset specifier pairs > > Note: > * fsl,ns16550: > diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c > index 1cbadafc6889..f34dd23376f4 100644 > --- a/drivers/tty/serial/8250/8250_of.c > +++ b/drivers/tty/serial/8250/8250_of.c > @@ -19,11 +19,13 @@ > #include <linux/of_irq.h> > #include <linux/of_platform.h> > #include <linux/clk.h> > +#include <linux/reset.h> > > #include "8250.h" > > struct of_serial_info { > struct clk *clk; > + struct reset_control *rst; > int type; > int line; > }; > @@ -132,6 +134,18 @@ static int of_platform_serial_setup(struct platform_device *ofdev, > } > } > > + info->rst = devm_reset_control_get_optional(&ofdev->dev, NULL); Please use devm_reset_control_get_optional_shared instead. This looks like shared (clock-like) reset use, where you just have to make sure that the reset is deasserted while the module is in use, but you don't care whether it is actually asserted all the time otherwise. > + if (IS_ERR(info->rst)) { > + ret = PTR_ERR(info->rst); > + if (ret == -EPROBE_DEFER) > + goto out; > + info->rst = NULL; devm_reset_control_get_optional returns NULL if no reset is specified in the device tree specifically so we don't have to do this in all the drivers. If an error is returned, just goto out unconditionally, ... > + } else { > + ret = reset_control_deassert(info->rst); > + if (ret) > + goto out; > + } ... then reset_control_deassert can be called unconditionally, too. > + > port->type = type; > port->uartclk = clk; > port->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_IOREMAP > @@ -231,6 +245,8 @@ static int of_platform_serial_remove(struct platform_device *ofdev) > > if (info->clk) > clk_disable_unprepare(info->clk); > + if (info->rst) Not necessary, reset_control_assert checks this, too. > + reset_control_assert(info->rst); > kfree(info); > return 0; > } regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 24, 2017 at 6:18 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Hi Joel, > > On Wed, 2017-05-24 at 14:53 +1000, Joel Stanley wrote: >> @@ -132,6 +134,18 @@ static int of_platform_serial_setup(struct platform_device *ofdev, >> } >> } >> >> + info->rst = devm_reset_control_get_optional(&ofdev->dev, NULL); > > Please use devm_reset_control_get_optional_shared instead. > > This looks like shared (clock-like) reset use, where you just have to > make sure that the reset is deasserted while the module is in use, but > you don't care whether it is actually asserted all the time otherwise. Thanks for the review. The new reset API is much nicer, and it worked for me once I realised the changes came in for 4.11 (my test setup is based on 4.10). Cheers, Joel -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt index 10276a46ecef..63e32393f82b 100644 --- a/Documentation/devicetree/bindings/serial/8250.txt +++ b/Documentation/devicetree/bindings/serial/8250.txt @@ -45,6 +45,7 @@ Optional properties: property. - tx-threshold: Specify the TX FIFO low water indication for parts with programmable TX FIFO thresholds. +- resets : phandle + reset specifier pairs Note: * fsl,ns16550: diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c index 1cbadafc6889..f34dd23376f4 100644 --- a/drivers/tty/serial/8250/8250_of.c +++ b/drivers/tty/serial/8250/8250_of.c @@ -19,11 +19,13 @@ #include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/clk.h> +#include <linux/reset.h> #include "8250.h" struct of_serial_info { struct clk *clk; + struct reset_control *rst; int type; int line; }; @@ -132,6 +134,18 @@ static int of_platform_serial_setup(struct platform_device *ofdev, } } + info->rst = devm_reset_control_get_optional(&ofdev->dev, NULL); + if (IS_ERR(info->rst)) { + ret = PTR_ERR(info->rst); + if (ret == -EPROBE_DEFER) + goto out; + info->rst = NULL; + } else { + ret = reset_control_deassert(info->rst); + if (ret) + goto out; + } + port->type = type; port->uartclk = clk; port->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_IOREMAP @@ -231,6 +245,8 @@ static int of_platform_serial_remove(struct platform_device *ofdev) if (info->clk) clk_disable_unprepare(info->clk); + if (info->rst) + reset_control_assert(info->rst); kfree(info); return 0; }
This adds the hooks for an optional reset controller in the 8250 device tree node. Signed-off-by: Joel Stanley <joel@jms.id.au> --- Documentation/devicetree/bindings/serial/8250.txt | 1 + drivers/tty/serial/8250/8250_of.c | 16 ++++++++++++++++ 2 files changed, 17 insertions(+) -- 2.11.0 -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html