Hi Geert,
Thanks for your review!
On 14 June 2022 13:43 Geert Uytterhoeven wrote:
> On Fri, May 20, 2022 at 5:41 PM Phil Edworthy wrote:
> > Add support for pin and gpio controller driver for RZ/V2M SoC.
> > Based on the RZ/G2L driver.
> >
> > Note that the DETDO and DETMS dedicated pins are currently not
> > documented in the HW manual as to which pin group they are in.
> > HW team have since said that the output level of V1.8V I/O group 4
> > (for MD0-7, and debugger) is the same as the 1.8V IO group 3.
>
> Thank you, I rediscovered this explanation just before pressing send ;-)
>
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@xxxxxxxxxxx>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> > --- a/drivers/pinctrl/renesas/Kconfig
> > +++ b/drivers/pinctrl/renesas/Kconfig
> > @@ -193,6 +194,18 @@ config PINCTRL_RZG2L
> > This selects GPIO and pinctrl driver for Renesas
> RZ/{G2L,G2UL,V2L}
> > platforms.
> >
> > +config PINCTRL_RZV2M
> > + bool "pin control support for RZ/V2M"
> > + depends on OF
> > + depends on ARCH_R9A09G011 || COMPILE_TEST
> > + select GPIOLIB
> > + select GENERIC_PINCTRL_GROUPS
> > + select GENERIC_PINMUX_FUNCTIONS
> > + select GENERIC_PINCONF
> > + help
> > + This selects GPIO and pinctrl driver for Renesas RZ/V2M
> > + platforms.
> > +
>
> Please preserve sort order.
For a while I couldn't see what's wrong here. It should be ordered on
the text, not the Kconfig symbol, right?
> > config PINCTRL_PFC_R8A77470
> > bool "pin control support for RZ/G1C" if COMPILE_TEST
> > select PINCTRL_SH_PFC
> > diff --git a/drivers/pinctrl/renesas/Makefile
> > b/drivers/pinctrl/renesas/Makefile
> > index 5d936c154a6f..cfa966125c4e 100644
> > --- a/drivers/pinctrl/renesas/Makefile
> > +++ b/drivers/pinctrl/renesas/Makefile
> > @@ -48,6 +48,7 @@ obj-$(CONFIG_PINCTRL_PFC_SHX3) += pfc-
> shx3.o
> > obj-$(CONFIG_PINCTRL_RZA1) += pinctrl-rza1.o
> > obj-$(CONFIG_PINCTRL_RZA2) += pinctrl-rza2.o
> > obj-$(CONFIG_PINCTRL_RZG2L) += pinctrl-rzg2l.o
> > +obj-$(CONFIG_PINCTRL_RZV2M) += pinctrl-rzv2m.o
>
> Please preserve sort order.
Ok
> > obj-$(CONFIG_PINCTRL_RZN1) += pinctrl-rzn1.o
> >
> > ifeq ($(CONFIG_COMPILE_TEST),y)
> > diff --git a/drivers/pinctrl/renesas/pinctrl-rzv2m.c
> > b/drivers/pinctrl/renesas/pinctrl-rzv2m.c
> > new file mode 100644
> > index 000000000000..610fc6b05e15
> > --- /dev/null
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzv2m.c
> > @@ -0,0 +1,1149 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/V2M Pin Control and GPIO driver core
> > + *
> > + * Based on:
> > + * Renesas RZ/G2L Pin Control and GPIO driver core
> > + *
> > + * Copyright (C) 2022 Renesas Electronics Corporation.
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/pinctrl/pinconf-generic.h> #include
> > +<linux/pinctrl/pinconf.h> #include <linux/pinctrl/pinctrl.h> #include
> > +<linux/pinctrl/pinmux.h> #include <linux/spinlock.h>
> > +
> > +#include <dt-bindings/pinctrl/rzv2m-pinctrl.h>
> > +
> > +#include "../core.h"
> > +#include "../pinconf.h"
> > +#include "../pinmux.h"
> > +
> > +#define DRV_NAME "pinctrl-rzv2m"
> > +
> > +/*
> > + * Use 16 lower bits [15:0] for pin identifier
> > + * Use 16 higher bits [31:16] for pin mux function */
> > +#define MUX_PIN_ID_MASK GENMASK(15, 0)
> > +#define MUX_FUNC_MASK GENMASK(31, 16)
> > +#define MUX_FUNC_OFFS 16
> > +#define MUX_FUNC(pinconf) (((pinconf) & MUX_FUNC_MASK) >>
> MUX_FUNC_OFFS)
>
> FIELD_GET(MUX_FUNC_MASK, (pinconf))?
> After that you can remove MUX_FUNC_OFFS.
Ok
> > +
> > +/* PIN capabilities */
> > +#define PIN_CFG_1_8V_GRP2 1
> > +#define PIN_CFG_1_8V_GRP3 2
> > +#define PIN_CFG_SWIO_GRP1 3
> > +#define PIN_CFG_SWIO_GRP2 4
> > +#define PIN_CFG_3_3V_GRP 5
>
> PIN_CFG_GRP_*, and move the definition of PIN_CFG_GRP_MASK here, to make
> it clear they use the same number space as the PIN_CFG_* below?
Ok.
> > +#define PIN_CFG_IE BIT(3)
> > +#define PIN_CFG_BIAS BIT(4)
> > +#define PIN_CFG_DRV BIT(5)
> > +#define PIN_CFG_SLEW BIT(6)
> > +
> > +#define RZV2M_MPXED_PIN_FUNCS (PIN_CFG_IE | \
> > + PIN_CFG_BIAS | \
> > + PIN_CFG_DRV | \
> > + PIN_CFG_SLEW)
> > +
> > +#define PIN_CFG_GRP_MASK 7
>
> GENMASK(2, 0)
Ok
> > +
> > +/*
> > + * n indicates number of pins in the port, a is the register index
> > + * and f is pin configuration capabilities supported.
> > + */
> > +#define RZV2M_GPIO_PORT_PACK(n, a, f) (((n) << 24) | ((a) << 16) |
> > +(f)) #define RZV2M_GPIO_PORT_GET_PINCNT(x) (((x) & GENMASK(31, 24))
> > +>> 24)
>
> FIELD_GET(GENMASK(31, 24), (x)) etc?
Ok
> > +#define RZV2M_GPIO_PORT_GET_INDEX(x) (((x) & GENMASK(23, 16)) >> 16)
> > +#define RZV2M_GPIO_PORT_GET_CFGS(x) ((x) & GENMASK(15, 0))
> > +
> > +#define RZV2M_DEDICATED_PORT_IDX 22
> > +
> > +/*
> > + * BIT(31) indicates dedicated pin, b is the register bits (b * 16)
> > + * and f is the pin configuration capabilities supported.
> > + */
> > +#define RZV2M_SINGLE_PIN BIT(31)
> > +#define RZV2M_SINGLE_PIN_PACK(b, f) (RZV2M_SINGLE_PIN | \
> > + ((RZV2M_DEDICATED_PORT_IDX) <<
> 24) | \
> > + ((b) << 16) | (f))
> > +#define RZV2M_SINGLE_PIN_GET_PORT_OFFSET(x) (((x) & GENMASK(30, 24))
> >> 24)
> > +#define RZV2M_SINGLE_PIN_GET_BIT(x) (((x) & GENMASK(23, 16)) >> 16)
> > +#define RZV2M_SINGLE_PIN_GET_CFGS(x) ((x) & GENMASK(15, 0))
> > +
> > +#define RZV2M_PIN_ID_TO_PORT(id) ((id) / RZV2M_PINS_PER_PORT)
> > +#define RZV2M_PIN_ID_TO_PORT_OFFSET(id) RZV2M_PIN_ID_TO_PORT(id)
>
> Unlike on RZ/G2L, there is no need to distinguish between port and
> port_offset on RZ/V2M, so you can drop RZV2M_PIN_ID_TO_PORT_OFFSET(), and
> always use RZV2M_PIN_ID_TO_PORT().
Ok
> > +#define RZV2M_PIN_ID_TO_PIN(id) ((id) %
> RZV2M_PINS_PER_PORT)
> > +
> > +#define DO(n) (0x00 + (n) * 0x40)
> > +#define OE(n) (0x04 + (n) * 0x40)
> > +#define IE(n) (0x08 + (n) * 0x40)
> > +#define PFC(n) (0x10 + (n) * 0x40)
>
> PFSEL, to match the documentation?
>
> > +#define DI(n) (0x20 + (n) * 0x40)
> > +#define PUPD(n) (0x24 + (n) * 0x40)
> > +#define DRV(n) (0x28 + (n) * 0x40)
> > +#define SR(n) (0x2c + (n) * 0x40)
> > +#define DI_MSK(n) (0x30 + (n) * 0x40)
> > +#define EN_MSK(n) (0x34 + (n) * 0x40)
> > +#define DRV_DEDICATED 0x590
> > +#define SR_DEDICATED 0x594
> > +
> > +#define IE_MASK 0x01
> > +#define PFC_MASK 0x07
> > +#define PUPD_MASK 0x03
> > +#define DRV_MASK 0x03
> > +#define SR_MASK 0x01
>
> Do you need IE_MASK and SR_MASK, for single-bit fields?
> You don't have DO_MASK(), OE_MASK(), and IE_MASK().
Ok, will remove
> > +static const unsigned int drv_1_8V_group2_uA[] = { 1800, 3800, 7800,
> > +11000 }; static const unsigned int drv_1_8V_group3_uA[] = { 1600,
> > +3200, 6400, 9600 }; static const unsigned int
> > +drv_SWIO_group1_3_3V_uA[] = { 2000, 4000, 8000, 12000 }; static const
> > +unsigned int drv_SWIO_group2_3_3V_uA[] = { 9000, 11000, 13000, 18000
> > +}; static const unsigned int drv_3_3V_group_uA[] = { 2000, 4000,
> > +8000, 12000 };
>
> Can drv_SWIO_group1_3_3V_uA[] and drv_3_3V_group_uA[] be shared, as they
> contain the same values?
Yes, will do.
> Note that for the SWIO groups, the actual values are dependent on the I/O
> voltage, too. But I guess it becomes too complicated to handle that (i.e.
> where to get the I/O voltage from)?
Yes, since the values are unique it should be ok.
> > +
> > +/* Helper for registers that have a write enable bit in the upper
> > +word */ static void rzv2m_writel_we(void __iomem *addr, u8 bit, u8
> > +value)
>
> s/bit/shift/
Ok
> > +{
> > + writel((BIT(16) | value) << bit, addr); }
>
> > +static void rzv2m_rmw_pin_config(struct rzv2m_pinctrl *pctrl, u32
> offset,
> > + u8 bit, u32 mask, u32 val)
>
> s/bit/shift/
Ok
> > +{
> > + void __iomem *addr = pctrl->base + offset;
> > + unsigned long flags;
> > + u32 reg;
> > +
> > + spin_lock_irqsave(&pctrl->lock, flags);
> > + reg = readl(addr) & ~(mask << bit);
> > + writel(reg | (val << bit), addr);
> > + spin_unlock_irqrestore(&pctrl->lock, flags); }
> > +
> > +static int rzv2m_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
> > + unsigned int _pin,
> > + unsigned long *config) {
> > + struct rzv2m_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > + enum pin_config_param param = pinconf_to_config_param(*config);
> > + const struct pinctrl_pin_desc *pin = &pctrl->desc.pins[_pin];
> > + unsigned int *pin_data = pin->drv_data;
> > + unsigned int arg = 0;
> > + u32 port_offset;
>
> port?
Ok
> > + u32 cfg = 0;
> > + u8 bit = 0;
>
> No need to pre-initialize cfg and bit to zero.
Ok
> > + u32 val;
> > +
> > + if (!pin_data)
> > + return -EINVAL;
> > +
> > + if (*pin_data & RZV2M_SINGLE_PIN) {
> > + port_offset =
> RZV2M_SINGLE_PIN_GET_PORT_OFFSET(*pin_data);
> > + cfg = RZV2M_SINGLE_PIN_GET_CFGS(*pin_data);
> > + bit = RZV2M_SINGLE_PIN_GET_BIT(*pin_data);
> > + } else {
> > + cfg = RZV2M_GPIO_PORT_GET_CFGS(*pin_data);
> > + port_offset = RZV2M_PIN_ID_TO_PORT_OFFSET(_pin);
> > + bit = RZV2M_PIN_ID_TO_PIN(_pin);
> > +
> > + if (rzv2m_validate_gpio_pin(pctrl, *pin_data,
> RZV2M_PIN_ID_TO_PORT(_pin), bit))
> > + return -EINVAL;
> > + }
> > +
> > + switch (param) {
> > + case PIN_CONFIG_BIAS_DISABLE:
> > + case PIN_CONFIG_BIAS_PULL_UP:
> > + case PIN_CONFIG_BIAS_PULL_DOWN: {
> > + enum pin_config_param bias;
> > +
> > + if (!(cfg & PIN_CFG_BIAS))
> > + return -EINVAL;
> > +
> > + /* PUPD uses 2-bits per pin */
> > + bit <<= 1;
>
> So bit is actually a shift...
> "*= 2" may be easier to read.
Ok
> > +
> > + val = (readl(pctrl->base + PUPD(port_offset)) >> bit) &
> PUPD_MASK;
> > + if (val == 0)
> > + bias = PIN_CONFIG_BIAS_PULL_DOWN;
> > + else if (val == 2)
> > + bias = PIN_CONFIG_BIAS_PULL_UP;
> > + else
> > + bias = PIN_CONFIG_BIAS_DISABLE;
>
> switch (readl(pctrl->base + PUPD(port_offset)) >> bit) & PUPD_MASK) { ...
> }
Ok
> > + if (bias != param)
> > + return -EINVAL;
> > + break;
> > + }
> > +
> > + case PIN_CONFIG_INPUT_ENABLE:
> > + if (!(cfg & PIN_CFG_IE))
> > + return -EINVAL;
> > +
> > + val = (readl(pctrl->base + IE(port_offset)) >> bit) &
> > + IE_MASK;
>
> "& BIT(bit)", or do you want to preserve symmetry with the other cases?
No, "& BIT(bit)" is better
> > + if (!val)
> > + return -EINVAL;
> > + break;
> > +
> > + case PIN_CONFIG_DRIVE_STRENGTH_UA:
> > + if (!(cfg & PIN_CFG_DRV))
> > + return -EINVAL;
> > +
> > + /* DRV uses 2-bits per pin */
> > + bit <<= 1;
> > +
> > + /* Dedicated port is irregularly located to the others
> */
> > + if (port_offset == RZV2M_DEDICATED_PORT_IDX)
> > + val = (readl(pctrl->base + DRV_DEDICATED) >>
> bit) & DRV_MASK;
> > + else
> > + val = (readl(pctrl->base + DRV(port_offset))
> > + >> bit) & DRV_MASK;
>
> You can simplify this, by handling the dedicated port offset in the
> definition of the DRV() macro. Same for SR().
Do you mean put conditional code in the DRV() macro?
> > +
> > + if ((cfg & PIN_CFG_GRP_MASK) == PIN_CFG_1_8V_GRP2)
> > + arg = drv_1_8V_group2_uA[val];
> > + else if ((cfg & PIN_CFG_GRP_MASK) == PIN_CFG_1_8V_GRP3)
> > + arg = drv_1_8V_group3_uA[val];
> > + else if ((cfg & PIN_CFG_GRP_MASK) == PIN_CFG_SWIO_GRP1)
> > + arg = drv_SWIO_group1_3_3V_uA[val];
> > + else if ((cfg & PIN_CFG_GRP_MASK) == PIN_CFG_SWIO_GRP2)
> > + arg = drv_SWIO_group2_3_3V_uA[val];
> > + else if ((cfg & PIN_CFG_GRP_MASK) == PIN_CFG_3_3V_GRP)
> > + arg = drv_3_3V_group_uA[val];
> > + else
> > + return -EINVAL;
>
> switch (cfg & PIN_CFG_GRP_MASK) { ... }
Ok
> > +
> > + break;
> > +
> > + case PIN_CONFIG_SLEW_RATE:
> > + if (!(cfg & PIN_CFG_SLEW))
> > + return -EINVAL;
> > +
> > + /* Dedicated port is irregularly located to the others
> */
> > + if (port_offset == RZV2M_DEDICATED_PORT_IDX)
> > + arg = (readl(pctrl->base + SR_DEDICATED) >>
> > + bit) & SR_MASK;
>
> "& BIT(bit)", or do you want to preserve symmetry with the other cases?
Will change
> > + else
> > + arg = (readl(pctrl->base + SR(port_offset)) >>
> bit) & SR_MASK;
> > + break;
> > +
> > + default:
> > + return -ENOTSUPP;
> > + }
> > +
> > + *config = pinconf_to_config_packed(param, arg);
> > +
> > + return 0;
> > +};
> > +
> > +static int rzv2m_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
> > + unsigned int _pin,
> > + unsigned long *_configs,
> > + unsigned int num_configs) {
> > + struct rzv2m_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > + const struct pinctrl_pin_desc *pin = &pctrl->desc.pins[_pin];
> > + unsigned int *pin_data = pin->drv_data;
> > + enum pin_config_param param;
> > + u32 port_offset;
>
> port?
Ok
> > + unsigned int i;
> > + u32 cfg = 0;
> > + u8 bit = 0;
>
> No need to pre-initialize cfg and bit to zero.
Ok
> > + u32 val;
> > +
> > + if (!pin_data)
> > + return -EINVAL;
> > +
> > + if (*pin_data & RZV2M_SINGLE_PIN) {
> > + port_offset =
> RZV2M_SINGLE_PIN_GET_PORT_OFFSET(*pin_data);
> > + cfg = RZV2M_SINGLE_PIN_GET_CFGS(*pin_data);
> > + bit = RZV2M_SINGLE_PIN_GET_BIT(*pin_data);
> > + } else {
> > + cfg = RZV2M_GPIO_PORT_GET_CFGS(*pin_data);
> > + port_offset = RZV2M_PIN_ID_TO_PORT_OFFSET(_pin);
> > + bit = RZV2M_PIN_ID_TO_PIN(_pin);
> > +
> > + if (rzv2m_validate_gpio_pin(pctrl, *pin_data,
> RZV2M_PIN_ID_TO_PORT(_pin), bit))
> > + return -EINVAL;
> > + }
> > +
> > + for (i = 0; i < num_configs; i++) {
> > + param = pinconf_to_config_param(_configs[i]);
> > + switch (param) {
> > + case PIN_CONFIG_BIAS_DISABLE:
> > + case PIN_CONFIG_BIAS_PULL_UP:
> > + case PIN_CONFIG_BIAS_PULL_DOWN:
> > + if (!(cfg & PIN_CFG_BIAS))
> > + return -EINVAL;
> > +
> > + /* PUPD uses 2-bits per pin */
> > + bit <<= 1;
>
> So bit is actually a shift...
> "*= 2" may be easier to read.
Ok
> > +
> > + if (param == PIN_CONFIG_BIAS_PULL_DOWN)
> > + val = 0;
> > + else if (param == PIN_CONFIG_BIAS_PULL_UP)
> > + val = 2;
> > + else
> > + val = 1;
> i
> switch (param) { ... }
Ok
> > +
> > + rzv2m_rmw_pin_config(pctrl, PUPD(port_offset),
> bit, PUPD_MASK, val);
> > + break;
> > +
> > + case PIN_CONFIG_INPUT_ENABLE: {
> > + unsigned int arg =
> > + pinconf_to_config_argument(_configs[i]);
> > +
> > + if (!(cfg & PIN_CFG_IE))
> > + return -EINVAL;
> > +
> > + rzv2m_writel_we(pctrl->base + OE(port_offset),
> > + bit, !arg);
>
> Should OE be controlled through PIN_CONFIG_OUTPUT_ENABLE instead?
I'll have a look at RZ/A1 output enable.
> > + rzv2m_writel_we(pctrl->base + IE(port_offset),
> bit, !!arg);
> > + break;
> > + }
> > +
> > + case PIN_CONFIG_DRIVE_STRENGTH_UA: {
> > + unsigned int arg =
> pinconf_to_config_argument(_configs[i]);
> > + const unsigned int *drv_strengths;
> > + unsigned int index;
> > +
> > + if (!(cfg & PIN_CFG_DRV))
> > + return -EINVAL;
> > +
> > + if ((cfg & PIN_CFG_GRP_MASK) ==
> PIN_CFG_1_8V_GRP2)
> > + drv_strengths = drv_1_8V_group2_uA;
> > + else if ((cfg & PIN_CFG_GRP_MASK) ==
> PIN_CFG_1_8V_GRP3)
> > + drv_strengths = drv_1_8V_group3_uA;
> > + else if ((cfg & PIN_CFG_GRP_MASK) ==
> PIN_CFG_SWIO_GRP1)
> > + drv_strengths = drv_SWIO_group1_3_3V_uA;
> > + else if ((cfg & PIN_CFG_GRP_MASK) ==
> PIN_CFG_SWIO_GRP2)
> > + drv_strengths = drv_SWIO_group2_3_3V_uA;
> > + else if ((cfg & PIN_CFG_GRP_MASK) ==
> PIN_CFG_3_3V_GRP)
> > + drv_strengths = drv_3_3V_group_uA;
> > + else
> > + return -EINVAL;
>
> switch (cfg & PIN_CFG_GRP_MASK) { ... }
Ok
> > +
> > + for (index = 0; index < 4; index++) {
> > + if (arg == drv_strengths[index])
> > + break;
> > + }
> > + if (index >= 4)
> > + return -EINVAL;
> > +
> > + /* DRV uses 2-bits per pin */
> > + bit <<= 1;
> > +
> > + /* Dedicated port is irregularly located to the
> others */
> > + if (port_offset == RZV2M_DEDICATED_PORT_IDX)
> > + rzv2m_rmw_pin_config(pctrl,
> DRV_DEDICATED, bit, DRV_MASK, index);
> > + else
> > + rzv2m_rmw_pin_config(pctrl,
> DRV(port_offset), bit, DRV_MASK, index);
> > + break;
> > + }
> > +
> > + case PIN_CONFIG_SLEW_RATE: {
> > + unsigned int arg =
> > + pinconf_to_config_argument(_configs[i]);
> > +
> > + if (!(cfg & PIN_CFG_SLEW))
> > + return -EINVAL;
> > +
> > + /* Dedicated port is irregularly located to the
> others */
> > + if (port_offset == RZV2M_DEDICATED_PORT_IDX)
> > + rzv2m_writel_we(pctrl->base +
> SR_DEDICATED, bit, !arg);
> > + else
> > + rzv2m_writel_we(pctrl->base +
> SR(port_offset), bit, !arg);
> > + break;
> > + }
> > +
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > + }
> > +
> > + return 0;
> > +}
>
> > +static void rzv2m_gpio_set_direction(struct rzv2m_pinctrl *pctrl, u32
> port,
> > + u8 bit, bool output) {
> > + if (output == true) {
>
> "if (output)", as output is already a boolean value.
>
> But perhaps just get rid of the if/else, and write "output" resp.
> "!output"
> to the OE resp. IE bits?
Ok
> > + rzv2m_writel_we(pctrl->base + IE(port), bit, 0);
> > + rzv2m_writel_we(pctrl->base + OE(port), bit, 1);
> > + } else {
> > + rzv2m_writel_we(pctrl->base + OE(port), bit, 0);
> > + rzv2m_writel_we(pctrl->base + IE(port), bit, 1);
> > + }
> > +}
>
> > +static void rzv2m_gpio_set(struct gpio_chip *chip, unsigned int offset,
> > + int value)
> > +{
> > + struct rzv2m_pinctrl *pctrl = gpiochip_get_data(chip);
> > + u32 port = RZV2M_PIN_ID_TO_PORT(offset);
> > + u8 bit = RZV2M_PIN_ID_TO_PIN(offset);
> > +
> > + rzv2m_writel_we(pctrl->base + DO(port), bit, value);
>
> While gpiod_direction_output_raw_commit() already does this, I guess it
> doesn't hurt to use "!!value", just in case someone ever passes a non-0/1
> value?
Ok
Thanks!
Phil
