[U-Boot] [PATCH] serial: add BCM283x mini UART driver

[U-Boot] [PATCH] serial: add BCM283x mini UART driver

[Stephen Warren]

The RPi3 typically uses the regular UART for high-speed communication with
the Bluetooth device, leaving us the mini UART to use for the serial
console. Add support for this UART so we can use it.

Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
---
(This will be a dependency for the RPi 3 patches, so it'd be good if it
could make it into mainline pretty quickly if acceptable.)

 drivers/serial/Makefile                      |   1 +
 drivers/serial/serial_bcm283x_mu.c           | 138 +++++++++++++++++++++++++++
 include/dm/platform_data/serial_bcm283x_mu.h |  22 +++++
 3 files changed, 161 insertions(+)
 create mode 100644 drivers/serial/serial_bcm283x_mu.c
 create mode 100644 include/dm/platform_data/serial_bcm283x_mu.h

diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
index 05bdf56c6fe9..ee7147a77abc 100644
--- a/drivers/serial/Makefile
+++ b/drivers/serial/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_UNIPHIER_SERIAL) += serial_uniphier.o
 obj-$(CONFIG_STM32_SERIAL) += serial_stm32.o
 obj-$(CONFIG_PIC32_SERIAL) += serial_pic32.o
 obj-$(CONFIG_STM32X7_SERIAL) += serial_stm32x7.o
+obj-$(CONFIG_BCM283X_MU_SERIAL) += serial_bcm283x_mu.o
 
 ifndef CONFIG_SPL_BUILD
 obj-$(CONFIG_USB_TTY) += usbtty.o
diff --git a/drivers/serial/serial_bcm283x_mu.c b/drivers/serial/serial_bcm283x_mu.c
new file mode 100644
index 000000000000..97b57cc52163
--- /dev/null
+++ b/drivers/serial/serial_bcm283x_mu.c
@@ -0,0 +1,138 @@
+/*
+ * (C) Copyright 2016 Stephen Warren <swarren@wwwdotorg.org>
+ *
+ * Derived from pl01x code:
+ *
+ * (C) Copyright 2000
+ * Rob Taylor, Flying Pig Systems. robt at flyingpig.com.
+ *
+ * (C) Copyright 2004
+ * ARM Ltd.
+ * Philippe Robin, <philippe.robin@arm.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/* Simple U-Boot driver for the BCM283x mini UART */
+
+#include <common.h>
+#include <dm.h>
+#include <errno.h>
+#include <watchdog.h>
+#include <asm/io.h>
+#include <serial.h>
+#include <dm/platform_data/serial_bcm283x_mu.h>
+#include <linux/compiler.h>
+#include <fdtdec.h>
+
+struct bcm283x_mu_regs {
+	u32 io;
+	u32 iir;
+	u32 ier;
+	u32 lcr;
+	u32 mcr;
+	u32 lsr;
+	u32 msr;
+	u32 scratch;
+	u32 cntl;
+	u32 stat;
+	u32 baud;
+};
+
+#define BCM283X_MU_LCR_DATA_SIZE_8	3
+
+#define BCM283X_MU_LSR_TX_IDLE		BIT(6)
+/* This actually means not full, but is named not empty in the docs */
+#define BCM283X_MU_LSR_TX_EMPTY		BIT(5)
+#define BCM283X_MU_LSR_RX_READY		BIT(0)
+
+struct bcm283x_mu_priv {
+	struct bcm283x_mu_regs *regs;
+};
+
+static int bcm283x_mu_serial_setbrg(struct udevice *dev, int baudrate)
+{
+	struct bcm283x_mu_priv *priv = dev_get_priv(dev);
+	struct bcm283x_mu_regs *regs = priv->regs;
+	/* FIXME: Get this from plat data later */
+	u32 clock_rate = 250000000;
+	u32 divider;
+
+	divider = clock_rate / (baudrate * 8);
+
+	writel(BCM283X_MU_LCR_DATA_SIZE_8, &regs->lcr);
+	writel(divider - 1, &regs->baud);
+
+	return 0;
+}
+
+static int bcm283x_mu_serial_probe(struct udevice *dev)
+{
+	struct bcm283x_mu_serial_platdata *plat = dev_get_platdata(dev);
+	struct bcm283x_mu_priv *priv = dev_get_priv(dev);
+
+	priv->regs = (struct bcm283x_mu_regs *)plat->base;
+
+	return 0;
+}
+
+static int bcm283x_mu_serial_getc(struct udevice *dev)
+{
+	struct bcm283x_mu_priv *priv = dev_get_priv(dev);
+	struct bcm283x_mu_regs *regs = priv->regs;
+	u32 data;
+
+	/* Wait until there is data in the FIFO */
+	if (!(readl(&regs->lsr) & BCM283X_MU_LSR_RX_READY))
+		return -EAGAIN;
+
+	data = readl(&regs->io);
+
+	return (int)data;
+}
+
+static int bcm283x_mu_serial_putc(struct udevice *dev, const char data)
+{
+	struct bcm283x_mu_priv *priv = dev_get_priv(dev);
+	struct bcm283x_mu_regs *regs = priv->regs;
+
+	/* Wait until there is space in the FIFO */
+	if (!(readl(&regs->lsr) & BCM283X_MU_LSR_TX_EMPTY))
+		return -EAGAIN;
+
+	/* Send the character */
+	writel(data, &regs->io);
+
+	return 0;
+}
+
+static int bcm283x_mu_serial_pending(struct udevice *dev, bool input)
+{
+	struct bcm283x_mu_priv *priv = dev_get_priv(dev);
+	struct bcm283x_mu_regs *regs = priv->regs;
+	unsigned int lsr = readl(&regs->lsr);
+
+	if (input) {
+		WATCHDOG_RESET();
+		return lsr & BCM283X_MU_LSR_RX_READY;
+	} else {
+		return !(lsr & BCM283X_MU_LSR_TX_IDLE);
+	}
+}
+
+static const struct dm_serial_ops bcm283x_mu_serial_ops = {
+	.putc = bcm283x_mu_serial_putc,
+	.pending = bcm283x_mu_serial_pending,
+	.getc = bcm283x_mu_serial_getc,
+	.setbrg = bcm283x_mu_serial_setbrg,
+};
+
+U_BOOT_DRIVER(serial_bcm283x_mu) = {
+	.name = "serial_bcm283x_mu",
+	.id = UCLASS_SERIAL,
+	.platdata_auto_alloc_size = sizeof(struct bcm283x_mu_serial_platdata),
+	.probe = bcm283x_mu_serial_probe,
+	.ops = &bcm283x_mu_serial_ops,
+	.flags = DM_FLAG_PRE_RELOC,
+	.priv_auto_alloc_size = sizeof(struct bcm283x_mu_priv),
+};
diff --git a/include/dm/platform_data/serial_bcm283x_mu.h b/include/dm/platform_data/serial_bcm283x_mu.h
new file mode 100644
index 000000000000..441594579549
--- /dev/null
+++ b/include/dm/platform_data/serial_bcm283x_mu.h
@@ -0,0 +1,22 @@
+/*
+ * (C) Copyright 2016 Stephen Warren <swarren@wwwdotorg.org>
+ *
+ * Derived from pl01x code:
+ * Copyright (c) 2014 Google, Inc
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#ifndef __serial_bcm283x_mu_h
+#define __serial_bcm283x_mu_h
+
+/*
+ *Information about a serial port
+ *
+ * @base: Register base address
+ */
+struct bcm283x_mu_serial_platdata {
+	unsigned long base;
+};
+
+#endif
-- 
2.7.3

[U-Boot] [PATCH] serial: add BCM283x mini UART driver

[Simon Glass]

Hi Stephen,

On 16 March 2016 at 21:46, Stephen Warren <swarren@wwwdotorg.org> wrote:
> The RPi3 typically uses the regular UART for high-speed communication with
> the Bluetooth device, leaving us the mini UART to use for the serial
> console. Add support for this UART so we can use it.
>
> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
> (This will be a dependency for the RPi 3 patches, so it'd be good if it
> could make it into mainline pretty quickly if acceptable.)

Reviewed-by: Simon Glass <sjg@chromium.org>

Not sure if this went in already. But see comment below.

>
>  drivers/serial/Makefile                      |   1 +
>  drivers/serial/serial_bcm283x_mu.c           | 138 +++++++++++++++++++++++++++
>  include/dm/platform_data/serial_bcm283x_mu.h |  22 +++++
>  3 files changed, 161 insertions(+)
>  create mode 100644 drivers/serial/serial_bcm283x_mu.c
>  create mode 100644 include/dm/platform_data/serial_bcm283x_mu.h
>
> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> index 05bdf56c6fe9..ee7147a77abc 100644
> --- a/drivers/serial/Makefile
> +++ b/drivers/serial/Makefile
> @@ -38,6 +38,7 @@ obj-$(CONFIG_UNIPHIER_SERIAL) += serial_uniphier.o
>  obj-$(CONFIG_STM32_SERIAL) += serial_stm32.o
>  obj-$(CONFIG_PIC32_SERIAL) += serial_pic32.o
>  obj-$(CONFIG_STM32X7_SERIAL) += serial_stm32x7.o
> +obj-$(CONFIG_BCM283X_MU_SERIAL) += serial_bcm283x_mu.o
>
>  ifndef CONFIG_SPL_BUILD
>  obj-$(CONFIG_USB_TTY) += usbtty.o
> diff --git a/drivers/serial/serial_bcm283x_mu.c b/drivers/serial/serial_bcm283x_mu.c
> new file mode 100644
> index 000000000000..97b57cc52163
> --- /dev/null
> +++ b/drivers/serial/serial_bcm283x_mu.c
> @@ -0,0 +1,138 @@
> +/*
> + * (C) Copyright 2016 Stephen Warren <swarren@wwwdotorg.org>
> + *
> + * Derived from pl01x code:
> + *
> + * (C) Copyright 2000
> + * Rob Taylor, Flying Pig Systems. robt at flyingpig.com.
> + *
> + * (C) Copyright 2004
> + * ARM Ltd.
> + * Philippe Robin, <philippe.robin@arm.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +/* Simple U-Boot driver for the BCM283x mini UART */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <watchdog.h>
> +#include <asm/io.h>
> +#include <serial.h>
> +#include <dm/platform_data/serial_bcm283x_mu.h>
> +#include <linux/compiler.h>
> +#include <fdtdec.h>
> +
> +struct bcm283x_mu_regs {
> +       u32 io;
> +       u32 iir;
> +       u32 ier;
> +       u32 lcr;
> +       u32 mcr;
> +       u32 lsr;
> +       u32 msr;
> +       u32 scratch;
> +       u32 cntl;
> +       u32 stat;
> +       u32 baud;
> +};
> +
> +#define BCM283X_MU_LCR_DATA_SIZE_8     3
> +
> +#define BCM283X_MU_LSR_TX_IDLE         BIT(6)
> +/* This actually means not full, but is named not empty in the docs */
> +#define BCM283X_MU_LSR_TX_EMPTY                BIT(5)
> +#define BCM283X_MU_LSR_RX_READY                BIT(0)
> +
> +struct bcm283x_mu_priv {
> +       struct bcm283x_mu_regs *regs;
> +};
> +
> +static int bcm283x_mu_serial_setbrg(struct udevice *dev, int baudrate)
> +{
> +       struct bcm283x_mu_priv *priv = dev_get_priv(dev);
> +       struct bcm283x_mu_regs *regs = priv->regs;
> +       /* FIXME: Get this from plat data later */

Or device tree?

> +       u32 clock_rate = 250000000;
> +       u32 divider;
> +
> +       divider = clock_rate / (baudrate * 8);
> +
> +       writel(BCM283X_MU_LCR_DATA_SIZE_8, &regs->lcr);
> +       writel(divider - 1, &regs->baud);
> +
> +       return 0;
> +}
> +
> +static int bcm283x_mu_serial_probe(struct udevice *dev)
> +{
> +       struct bcm283x_mu_serial_platdata *plat = dev_get_platdata(dev);
> +       struct bcm283x_mu_priv *priv = dev_get_priv(dev);
> +
> +       priv->regs = (struct bcm283x_mu_regs *)plat->base;
> +
> +       return 0;
> +}
> +
> +static int bcm283x_mu_serial_getc(struct udevice *dev)
> +{
> +       struct bcm283x_mu_priv *priv = dev_get_priv(dev);
> +       struct bcm283x_mu_regs *regs = priv->regs;
> +       u32 data;
> +
> +       /* Wait until there is data in the FIFO */
> +       if (!(readl(&regs->lsr) & BCM283X_MU_LSR_RX_READY))
> +               return -EAGAIN;
> +
> +       data = readl(&regs->io);
> +
> +       return (int)data;
> +}
> +
> +static int bcm283x_mu_serial_putc(struct udevice *dev, const char data)
> +{
> +       struct bcm283x_mu_priv *priv = dev_get_priv(dev);
> +       struct bcm283x_mu_regs *regs = priv->regs;
> +
> +       /* Wait until there is space in the FIFO */
> +       if (!(readl(&regs->lsr) & BCM283X_MU_LSR_TX_EMPTY))
> +               return -EAGAIN;
> +
> +       /* Send the character */
> +       writel(data, &regs->io);
> +
> +       return 0;
> +}
> +
> +static int bcm283x_mu_serial_pending(struct udevice *dev, bool input)
> +{
> +       struct bcm283x_mu_priv *priv = dev_get_priv(dev);
> +       struct bcm283x_mu_regs *regs = priv->regs;
> +       unsigned int lsr = readl(&regs->lsr);
> +
> +       if (input) {
> +               WATCHDOG_RESET();
> +               return lsr & BCM283X_MU_LSR_RX_READY;
> +       } else {
> +               return !(lsr & BCM283X_MU_LSR_TX_IDLE);

These look like flags - be care to return 1 if there is an unknown
number of characters, rather than (e.g. 4). The latter might cause the
uclass to expect 4 characters to be present.

Suggest putting ? 1 : 0 or ? 0 : 1 on the end.

> +       }
> +}
> +
> +static const struct dm_serial_ops bcm283x_mu_serial_ops = {
> +       .putc = bcm283x_mu_serial_putc,
> +       .pending = bcm283x_mu_serial_pending,
> +       .getc = bcm283x_mu_serial_getc,
> +       .setbrg = bcm283x_mu_serial_setbrg,
> +};
> +
> +U_BOOT_DRIVER(serial_bcm283x_mu) = {
> +       .name = "serial_bcm283x_mu",
> +       .id = UCLASS_SERIAL,
> +       .platdata_auto_alloc_size = sizeof(struct bcm283x_mu_serial_platdata),
> +       .probe = bcm283x_mu_serial_probe,
> +       .ops = &bcm283x_mu_serial_ops,
> +       .flags = DM_FLAG_PRE_RELOC,
> +       .priv_auto_alloc_size = sizeof(struct bcm283x_mu_priv),
> +};
> diff --git a/include/dm/platform_data/serial_bcm283x_mu.h b/include/dm/platform_data/serial_bcm283x_mu.h
> new file mode 100644
> index 000000000000..441594579549
> --- /dev/null
> +++ b/include/dm/platform_data/serial_bcm283x_mu.h
> @@ -0,0 +1,22 @@
> +/*
> + * (C) Copyright 2016 Stephen Warren <swarren@wwwdotorg.org>
> + *
> + * Derived from pl01x code:
> + * Copyright (c) 2014 Google, Inc
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#ifndef __serial_bcm283x_mu_h
> +#define __serial_bcm283x_mu_h
> +
> +/*
> + *Information about a serial port
> + *
> + * @base: Register base address
> + */
> +struct bcm283x_mu_serial_platdata {
> +       unsigned long base;
> +};
> +
> +#endif
> --
> 2.7.3
>

Regards,
Simon

[U-Boot] [PATCH] serial: add BCM283x mini UART driver

[Stephen Warren]

On 04/09/2016 12:34 PM, Simon Glass wrote:
> Hi Stephen,
>
> On 16 March 2016 at 21:46, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> The RPi3 typically uses the regular UART for high-speed communication with
>> the Bluetooth device, leaving us the mini UART to use for the serial
>> console. Add support for this UART so we can use it.
>>
>> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
>> ---
>> (This will be a dependency for the RPi 3 patches, so it'd be good if it
>> could make it into mainline pretty quickly if acceptable.)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Not sure if this went in already. But see comment below.

It has, but I can send a fix.

>> diff --git a/drivers/serial/serial_bcm283x_mu.c b/drivers/serial/serial_bcm283x_mu.c

>> +static int bcm283x_mu_serial_setbrg(struct udevice *dev, int baudrate)
>> +{
>> +       struct bcm283x_mu_priv *priv = dev_get_priv(dev);
>> +       struct bcm283x_mu_regs *regs = priv->regs;
>> +       /* FIXME: Get this from plat data later */
>> +       u32 clock_rate = 250000000;
>
> Or device tree?

Well even if DT were used on this platform, the code right here would 
get the clock rate from platform data. Now, whether the platform data 
came from a board file or was parsed from DT is another matter.

>> +static int bcm283x_mu_serial_pending(struct udevice *dev, bool input)
>> +{
>> +       struct bcm283x_mu_priv *priv = dev_get_priv(dev);
>> +       struct bcm283x_mu_regs *regs = priv->regs;
>> +       unsigned int lsr = readl(&regs->lsr);
>> +
>> +       if (input) {
>> +               WATCHDOG_RESET();
>> +               return lsr & BCM283X_MU_LSR_RX_READY;
>> +       } else {
>> +               return !(lsr & BCM283X_MU_LSR_TX_IDLE);
>
> These look like flags - be care to return 1 if there is an unknown
> number of characters, rather than (e.g. 4). The latter might cause the
> uclass to expect 4 characters to be present.
>
> Suggest putting ? 1 : 0 or ? 0 : 1 on the end.

Luckily BCM283X_MU_LSR_RX_READY is BIT(0) so there's no practical issue. 
A !! would make this more obvious at this point in the code though. Do 
you think that warrants a patch? The ! on the second return also ensure 
the correct return value.

[U-Boot] [PATCH] serial: add BCM283x mini UART driver

[Tom Rini]

On Sat, Apr 09, 2016 at 09:45:36PM -0600, Stephen Warren wrote:
> On 04/09/2016 12:34 PM, Simon Glass wrote:
> >Hi Stephen,
> >
> >On 16 March 2016 at 21:46, Stephen Warren <swarren@wwwdotorg.org> wrote:
> >>The RPi3 typically uses the regular UART for high-speed communication with
> >>the Bluetooth device, leaving us the mini UART to use for the serial
> >>console. Add support for this UART so we can use it.
> >>
> >>Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> >>---
> >>(This will be a dependency for the RPi 3 patches, so it'd be good if it
> >>could make it into mainline pretty quickly if acceptable.)
> >
> >Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> >Not sure if this went in already. But see comment below.
> 
> It has, but I can send a fix.
> 
> >>diff --git a/drivers/serial/serial_bcm283x_mu.c b/drivers/serial/serial_bcm283x_mu.c
> 
> >>+static int bcm283x_mu_serial_setbrg(struct udevice *dev, int baudrate)
> >>+{
> >>+       struct bcm283x_mu_priv *priv = dev_get_priv(dev);
> >>+       struct bcm283x_mu_regs *regs = priv->regs;
> >>+       /* FIXME: Get this from plat data later */
> >>+       u32 clock_rate = 250000000;
> >
> >Or device tree?
> 
> Well even if DT were used on this platform, the code right here
> would get the clock rate from platform data. Now, whether the
> platform data came from a board file or was parsed from DT is
> another matter.
> 
> >>+static int bcm283x_mu_serial_pending(struct udevice *dev, bool input)
> >>+{
> >>+       struct bcm283x_mu_priv *priv = dev_get_priv(dev);
> >>+       struct bcm283x_mu_regs *regs = priv->regs;
> >>+       unsigned int lsr = readl(&regs->lsr);
> >>+
> >>+       if (input) {
> >>+               WATCHDOG_RESET();
> >>+               return lsr & BCM283X_MU_LSR_RX_READY;
> >>+       } else {
> >>+               return !(lsr & BCM283X_MU_LSR_TX_IDLE);
> >
> >These look like flags - be care to return 1 if there is an unknown
> >number of characters, rather than (e.g. 4). The latter might cause the
> >uclass to expect 4 characters to be present.
> >
> >Suggest putting ? 1 : 0 or ? 0 : 1 on the end.
> 
> Luckily BCM283X_MU_LSR_RX_READY is BIT(0) so there's no practical
> issue. A !! would make this more obvious at this point in the code
> though. Do you think that warrants a patch? The ! on the second
> return also ensure the correct return value.

I hate the '!!' operator, how bad would it be to spell things out in the
code in a bit less compact way but such that the compiler will still
optimize things well.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160410/14e68d74/attachment.sig>

[U-Boot] [PATCH] serial: add BCM283x mini UART driver

[Simon Glass]

Hi Stephen,

On 9 April 2016 at 21:45, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/09/2016 12:34 PM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 16 March 2016 at 21:46, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>
>>> The RPi3 typically uses the regular UART for high-speed communication
>>> with
>>> the Bluetooth device, leaving us the mini UART to use for the serial
>>> console. Add support for this UART so we can use it.
>>>
>>> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
>>> ---
>>> (This will be a dependency for the RPi 3 patches, so it'd be good if it
>>> could make it into mainline pretty quickly if acceptable.)
>>
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> Not sure if this went in already. But see comment below.
>
>
> It has, but I can send a fix.
>
>>> diff --git a/drivers/serial/serial_bcm283x_mu.c
>>> b/drivers/serial/serial_bcm283x_mu.c
>
>
>>> +static int bcm283x_mu_serial_setbrg(struct udevice *dev, int baudrate)
>>> +{
>>> +       struct bcm283x_mu_priv *priv = dev_get_priv(dev);
>>> +       struct bcm283x_mu_regs *regs = priv->regs;
>>> +       /* FIXME: Get this from plat data later */
>>> +       u32 clock_rate = 250000000;
>>
>>
>> Or device tree?
>
>
> Well even if DT were used on this platform, the code right here would get
> the clock rate from platform data. Now, whether the platform data came from
> a board file or was parsed from DT is another matter.
>
>>> +static int bcm283x_mu_serial_pending(struct udevice *dev, bool input)
>>> +{
>>> +       struct bcm283x_mu_priv *priv = dev_get_priv(dev);
>>> +       struct bcm283x_mu_regs *regs = priv->regs;
>>> +       unsigned int lsr = readl(&regs->lsr);
>>> +
>>> +       if (input) {
>>> +               WATCHDOG_RESET();
>>> +               return lsr & BCM283X_MU_LSR_RX_READY;
>>> +       } else {
>>> +               return !(lsr & BCM283X_MU_LSR_TX_IDLE);
>>
>>
>> These look like flags - be care to return 1 if there is an unknown
>> number of characters, rather than (e.g. 4). The latter might cause the
>> uclass to expect 4 characters to be present.
>>
>> Suggest putting ? 1 : 0 or ? 0 : 1 on the end.
>
>
> Luckily BCM283X_MU_LSR_RX_READY is BIT(0) so there's no practical issue. A
> !! would make this more obvious at this point in the code though. Do you
> think that warrants a patch? The ! on the second return also ensure the
> correct return value.

I think it is worth a patch, in case perhaps someone else copies that
code later. Agreed not a high priority though.

Regards,
Simon