[PATCH 0/2] Add support of the FXL6408 GPIO expander
Add support of the Fairchild's FXL6408 i2c gpio expander and enable this driver for Toradex Colibri iMX8QXP SoM.
Oleksandr Suvorov (2): GPIO: fxl6408: Add support for FXL6408 GPIO expander colibri-imx8x: add on-module gpio expander fxl6408
arch/arm/dts/fsl-imx8qxp-colibri.dts | 27 ++ configs/colibri-imx8x_defconfig | 1 + drivers/gpio/Kconfig | 7 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-fxl6408.c | 371 +++++++++++++++++++++++++++ 5 files changed, 407 insertions(+) create mode 100644 drivers/gpio/gpio-fxl6408.c
Initial support for Fairchild's 8 bit I2C gpio expander FXL6408. The CONFIG_FXL6408_GPIO define enables support for such devices.
Based on: https://patchwork.kernel.org/patch/9148419/
Signed-off-by: Oleksandr Suvorov oleksandr.suvorov@toradex.com ---
drivers/gpio/Kconfig | 7 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-fxl6408.c | 371 ++++++++++++++++++++++++++++++++++++ 3 files changed, 379 insertions(+) create mode 100644 drivers/gpio/gpio-fxl6408.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 0817b12c5f..5883582a7f 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -123,6 +123,13 @@ config DA8XX_GPIO help This driver supports the DA8xx GPIO controller
+config FXL6408_GPIO + bool "FXL6408 I2C GPIO driver" + depends on DM_GPIO && DM_I2C + help + Support for Fairchild Semiconductor FXL6408 I2C 8-bit GPIO + expander. + config INTEL_BROADWELL_GPIO bool "Intel Broadwell GPIO driver" depends on DM diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 16b09fb1b5..a71f3879a2 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -16,6 +16,7 @@ obj-$(CONFIG_AT91_GPIO) += at91_gpio.o obj-$(CONFIG_ATMEL_PIO4) += atmel_pio4.o obj-$(CONFIG_BCM6345_GPIO) += bcm6345_gpio.o obj-$(CONFIG_CORTINA_GPIO) += cortina_gpio.o +obj-$(CONFIG_FXL6408_GPIO) += gpio-fxl6408.o obj-$(CONFIG_INTEL_GPIO) += intel_gpio.o obj-$(CONFIG_INTEL_ICH6_GPIO) += intel_ich6_gpio.o obj-$(CONFIG_INTEL_BROADWELL_GPIO) += intel_broadwell_gpio.o diff --git a/drivers/gpio/gpio-fxl6408.c b/drivers/gpio/gpio-fxl6408.c new file mode 100644 index 0000000000..282ec0a69c --- /dev/null +++ b/drivers/gpio/gpio-fxl6408.c @@ -0,0 +1,371 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2021 Toradex + * Copyright (C) 2016 Broadcom + */ + +/** + * DOC: FXL6408 I2C to GPIO expander. + * + * This chip has 8 GPIO lines out of it, and is controlled by an I2C + * bus (a pair of lines), providing 4x expansion of GPIO lines. It + * also provides an interrupt line out for notifying of state changes. + * + * Any preconfigured state will be left in place until the GPIO lines + * get activated. At power on, everything is treated as an input, + * default input is HIGH and pulled-up, all interrupts are masked. + * + * Documentation can be found at: + * https://www.fairchildsemi.com/datasheets/FX/FXL6408.pdf + * + * This driver bases on: + * - the original driver by Eric Anholt eric@anholt.net: + * https://patchwork.kernel.org/patch/9148419/ + * - the Toradex version by Max Krummenacher max.krummenacher@toradex.com: + * http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/gpio/gpio-fxl6408... + * - the U-boot PCA953x driver by Peng Fan van.freenix@gmail.com: + * drivers/gpio/pca953x_gpio.c + * + * TODO: Add interrupts support + */ + +#include <common.h> +#include <dm.h> +#include <dm/device_compat.h> +#include <i2c.h> +#include <log.h> +#include <asm-generic/gpio.h> +#include <asm/global_data.h> +#include <linux/bitops.h> +#include <dt-bindings/gpio/gpio.h> + +#define FXL6408_DEVID_CTRL 0x01 +# define FXL6408_SW_RST BIT(0) +# define FXL6408_RST_INT BIT(1) + +/* 3-bit manufacturer ID */ +# define FXL6408_MF_MASK GENMASK(7, 5) +# define FXL6408_MF_ID(devid) (((devid) & FXL6408_MF_MASK) >> 5) +/* 0b101 is for Fairchild assigned by Nokia */ +# define FXL6408_FAIRCHILD_MF 5 + +/* 3-bit firmware revision */ +# define FXL6408_FW_MASK GENMASK(4, 2) +# define FXL6408_FW_REV(devid) (((devid) & FXL6408_FW_MASK) >> 2) + +/* + * Bits set here indicate that the GPIO is an output. + */ +#define FXL6408_DIRECTION 0x03 + +enum { + FXL6408_DIRECTION_IN, + FXL6408_DIRECTION_OUT, +}; + +/* + * Bits set here, when the corresponding bit of IO_DIR is set, drive + * the output high instead of low. + */ +#define FXL6408_OUTPUT 0x05 + +/* + * Bits here make the output High-Z, instead of the OUTPUT value. + */ +#define FXL6408_OUTPUT_HIGH_Z 0x07 + +/* + * Bits here define the expected input state of the GPIO. + * INTERRUPT_STATUS bits will be set when the INPUT transitions away + * from this value. + */ +#define FXL6408_INPUT_DEF_STATE 0x09 + +/* + * Bits here enable either pull up or pull down according to + * INPUT_PULL_STATE. + */ +#define FXL6408_INPUT_PULL_ENABLE 0x0b + +/* + * Bits set here selects a pull-up/pull-down state of pin, which + * is configured as Input and the corresponding PULL_ENABLE bit is + * set. + */ +#define FXL6408_INPUT_PULL_STATE 0x0d + +/* + * Returns the current status (1 = HIGH) of the input pins. + */ +#define FXL6408_INPUT 0x0f + +/* + * Mask of pins which can generate interrupts. + */ +#define FXL6408_INTERRUPT_MASK 0x11 +/* + * Mask of pins which have generated an interrupt. + * Cleared on read. + */ +#define FXL6408_INTERRUPT_STATUS 0x13 + +/* + * struct fxl6408_info - Data for fxl6408 + * + * @dev: udevice structure for the device + * @addr: i2c slave address + * @reg_output: hold the value of output register + * @reg_direction: hold the value of direction register + */ +struct fxl6408_info { + struct udevice *dev; + int addr; + u8 device_id; + u8 reg_io_dir; + u8 reg_output; +}; + +static int fxl6408_write(struct udevice *dev, int reg, u8 val) +{ + int ret; + + ret = dm_i2c_write(dev, reg, &val, 1); + if (ret) + dev_err(dev, "%s error: %d\n", __func__, ret); + + return ret; +} + +static int fxl6408_read(struct udevice *dev, int reg, u8 *val) +{ + int ret; + u8 tmp; + + ret = dm_i2c_read(dev, reg, &tmp, 1); + if (!ret) + *val = tmp; + else + dev_err(dev, "%s error: %d\n", __func__, ret); + + return ret; +} + +/* + * fxl6408_is_output() - check whether the gpio configures as either + * output or input. + * Return: 0 - input, 1 - output. + */ +static int fxl6408_is_output(struct udevice *dev, int offset) +{ + struct fxl6408_info *info = dev_get_plat(dev); + + return !!(info->reg_io_dir & BIT(offset)); +} + +static int fxl6408_get_value(struct udevice *dev, uint offset) +{ + int ret, reg = fxl6408_is_output(dev, offset) ? FXL6408_OUTPUT : FXL6408_INPUT; + u8 val = 0; + + ret = fxl6408_read(dev, reg, &val); + if (IS_ERR_VALUE(ret)) + return ret; + + return !!(val & BIT(offset)); +} + +static int fxl6408_set_value(struct udevice *dev, uint offset, int value) +{ + struct fxl6408_info *info = dev_get_plat(dev); + u8 val; + int ret; + + if (value) + val = info->reg_output | BIT(offset); + else + val = info->reg_output & ~BIT(offset); + + ret = fxl6408_write(dev, FXL6408_OUTPUT, val); + if (!IS_ERR_VALUE(ret)) + info->reg_output = val; + + return ret; +} + +static int fxl6408_set_direction(struct udevice *dev, uint offset, int dir) +{ + struct fxl6408_info *info = dev_get_plat(dev); + u8 val; + int ret; + + if (dir == FXL6408_DIRECTION_IN) + val = info->reg_io_dir & ~BIT(offset); + else + val = info->reg_io_dir | BIT(offset); + + ret = fxl6408_write(dev, FXL6408_DIRECTION, val); + if (!IS_ERR_VALUE(ret)) + info->reg_io_dir = val; + + return ret; +} + +static int fxl6408_direction_input(struct udevice *dev, uint offset) +{ + return fxl6408_set_direction(dev, offset, FXL6408_DIRECTION_IN); +} + +static int fxl6408_direction_output(struct udevice *dev, uint offset, int value) +{ + int ret; + + /* Configure output value. */ + ret = fxl6408_set_value(dev, offset, value); + if (!IS_ERR_VALUE(ret)) + /* Configure direction as output. */ + fxl6408_set_direction(dev, offset, FXL6408_DIRECTION_OUT); + + return ret; +} + +static int fxl6408_get_function(struct udevice *dev, uint offset) +{ + if (fxl6408_is_output(dev, offset)) + return GPIOF_OUTPUT; + else + return GPIOF_INPUT; +} + +static int fxl6408_xlate(struct udevice *dev, struct gpio_desc *desc, + struct ofnode_phandle_args *args) +{ + desc->offset = args->args[0]; + desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0; + + return 0; +} + +static const struct dm_gpio_ops fxl6408_ops = { + .direction_input = fxl6408_direction_input, + .direction_output = fxl6408_direction_output, + .get_value = fxl6408_get_value, + .set_value = fxl6408_set_value, + .get_function = fxl6408_get_function, + .xlate = fxl6408_xlate, +}; + +static int fxl6408_probe(struct udevice *dev) +{ + struct fxl6408_info *info = dev_get_plat(dev); + struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev); + char name[32], label[32], *str; + int addr; + int ret; + int size; + const u8 *tmp; + u8 val; + u32 val32; + + addr = dev_read_addr(dev); + if (addr == 0) + return -ENODEV; + info->addr = addr; + + /* Check the device ID register to see if it's responding. + * This also clears RST_INT as a side effect, so we won't get + * the "we've been power cycled" interrupt once we enable + * interrupts. + */ + ret = fxl6408_read(dev, FXL6408_DEVID_CTRL, &val); + if (IS_ERR_VALUE(ret)) { + dev_err(dev, "FXL6408 probe returned %d\n", ret); + return ret; + } + + if (FXL6408_MF_ID(val) != FXL6408_FAIRCHILD_MF) { + dev_err(dev, "FXL6408 probe returned DID: 0x%02x\n", val); + return -ENODEV; + } + info->device_id = val; + + /* + * Disable High-Z of outputs, so that the OUTPUT updates + * actually take effect. + */ + ret = fxl6408_write(dev, FXL6408_OUTPUT_HIGH_Z, (u8)0); + if (IS_ERR_VALUE(ret)) { + dev_err(dev, "Error writing High-Z register\n"); + return ret; + } + + /* + * If configured, set initial output state and direction, + * otherwise read them from the chip. + */ + if (IS_ERR_VALUE(dev_read_u32(dev, "initial_io_dir", &val32))) { + ret = fxl6408_read(dev, FXL6408_DIRECTION, &info->reg_io_dir); + if (IS_ERR_VALUE(ret)) { + dev_err(dev, "Error reading direction register\n"); + return ret; + } + } else { + info->reg_io_dir = val32 & 0xFF; + ret = fxl6408_write(dev, FXL6408_DIRECTION, info->reg_io_dir); + if (IS_ERR_VALUE(ret)) { + dev_err(dev, "Error setting direction register\n"); + return ret; + } + } + + if (IS_ERR_VALUE(dev_read_u32(dev, "initial_output", &val32))) { + ret = fxl6408_read(dev, FXL6408_OUTPUT, &info->reg_output); + if (IS_ERR_VALUE(ret)) { + dev_err(dev, "Error reading output register\n"); + return ret; + } + } else { + info->reg_output = val32 & 0xFF; + ret = fxl6408_write(dev, FXL6408_OUTPUT, info->reg_output); + if (IS_ERR_VALUE(ret)) { + dev_err(dev, "Error setting output register\n"); + return ret; + } + } + + tmp = dev_read_prop(dev, "label", &size); + if (tmp) { + size = min(size, (int)sizeof(label) - 1); + memcpy(label, tmp, size); + label[size] = '\0'; + snprintf(name, sizeof(name), "%s@%x_", label, info->addr); + } else { + snprintf(name, sizeof(name), "gpio@%x_", info->addr); + } + + str = strdup(name); + if (!str) + return -ENOMEM; + + uc_priv->bank_name = str; + uc_priv->gpio_count = dev_get_driver_data(dev); + uc_priv->gpio_base = -1; + + dev_dbg(dev, "%s (FW rev. %ld) is ready\n", str, + FXL6408_FW_REV(info->device_id)); + + return 0; +} + +static const struct udevice_id fxl6408_ids[] = { + { .compatible = "fcs,fxl6408", .data = 8 }, + { } +}; + +U_BOOT_DRIVER(fxl6408_gpio) = { + .name = "fxl6408_gpio", + .id = UCLASS_GPIO, + .ops = &fxl6408_ops, + .probe = fxl6408_probe, + .of_match = fxl6408_ids, + .plat_auto = sizeof(struct fxl6408_info), +};
The FXL6408 GPIO expander manages critical devices, including on-module USB hub. Configure the expander to switch the USB hub into bypass mode, allowing to use on-carrier-board USB hub.
Signed-off-by: Oleksandr Suvorov oleksandr.suvorov@toradex.com ---
arch/arm/dts/fsl-imx8qxp-colibri.dts | 27 +++++++++++++++++++++++++++ configs/colibri-imx8x_defconfig | 1 + 2 files changed, 28 insertions(+)
diff --git a/arch/arm/dts/fsl-imx8qxp-colibri.dts b/arch/arm/dts/fsl-imx8qxp-colibri.dts index 11ece34c02..df992ac639 100644 --- a/arch/arm/dts/fsl-imx8qxp-colibri.dts +++ b/arch/arm/dts/fsl-imx8qxp-colibri.dts @@ -129,6 +129,14 @@ >; };
+ /* On Module I2C */ + pinctrl_i2c0: i2c0grp { + fsl,pins = < + SC_P_MIPI_CSI0_GPIO0_00_ADMA_I2C0_SCL 0x06000021 + SC_P_MIPI_CSI0_GPIO0_01_ADMA_I2C0_SDA 0x06000021 + >; + }; + /* Off Module I2C */ pinctrl_i2c1: i2c1grp { fsl,pins = < @@ -298,6 +306,25 @@ }; };
+&i2c0 { + #address-cells = <1>; + #size-cells = <0>; + clock-frequency = <100000>; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_i2c0>; + status = "okay"; + + /* GPIO expander */ + gpio_expander_43: gpio-expander@43 { + compatible = "fcs,fxl6408"; + gpio-controller; + #gpio-cells = <2>; + reg = <0x43>; + initial_io_dir = <0xff>; + initial_output = <0x05>; + }; +}; + &i2c1 { #address-cells = <1>; #size-cells = <0>; diff --git a/configs/colibri-imx8x_defconfig b/configs/colibri-imx8x_defconfig index a0816acc27..fbe6e575a3 100644 --- a/configs/colibri-imx8x_defconfig +++ b/configs/colibri-imx8x_defconfig @@ -38,6 +38,7 @@ CONFIG_TFTP_BLOCKSIZE=4096 CONFIG_TFTP_TSIZE=y CONFIG_CLK_IMX8=y CONFIG_CPU=y +CONFIG_FXL6408_GPIO=y CONFIG_MXC_GPIO=y CONFIG_DM_I2C=y CONFIG_SYS_I2C_IMX_LPI2C=y
Hi Oleksandr,
On Wed, 21 Jul 2021 at 06:21, Oleksandr Suvorov oleksandr.suvorov@toradex.com wrote:
Initial support for Fairchild's 8 bit I2C gpio expander FXL6408. The CONFIG_FXL6408_GPIO define enables support for such devices.
Based on: https://patchwork.kernel.org/patch/9148419/
Signed-off-by: Oleksandr Suvorov oleksandr.suvorov@toradex.com
drivers/gpio/Kconfig | 7 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-fxl6408.c | 371 ++++++++++++++++++++++++++++++++++++ 3 files changed, 379 insertions(+) create mode 100644 drivers/gpio/gpio-fxl6408.c
Reviewed-by: Simon Glass sjg@chromium.org
Lots of nits below
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 0817b12c5f..5883582a7f 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -123,6 +123,13 @@ config DA8XX_GPIO help This driver supports the DA8xx GPIO controller
+config FXL6408_GPIO
bool "FXL6408 I2C GPIO driver"depends on DM_GPIO && DM_I2ChelpSupport for Fairchild Semiconductor FXL6408 I2C 8-bit GPIOexpander.
Please add some more details here.
[..]
diff --git a/drivers/gpio/gpio-fxl6408.c b/drivers/gpio/gpio-fxl6408.c new file mode 100644 index 0000000000..282ec0a69c --- /dev/null +++ b/drivers/gpio/gpio-fxl6408.c @@ -0,0 +1,371 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright (C) 2021 Toradex
- Copyright (C) 2016 Broadcom
- */
+/**
- DOC: FXL6408 I2C to GPIO expander.
- This chip has 8 GPIO lines out of it, and is controlled by an I2C
- bus (a pair of lines), providing 4x expansion of GPIO lines. It
- also provides an interrupt line out for notifying of state changes.
- Any preconfigured state will be left in place until the GPIO lines
- get activated. At power on, everything is treated as an input,
- default input is HIGH and pulled-up, all interrupts are masked.
- Documentation can be found at:
- This driver bases on:
- the original driver by Eric Anholt eric@anholt.net:
- the Toradex version by Max Krummenacher max.krummenacher@toradex.com:
- the U-boot PCA953x driver by Peng Fan van.freenix@gmail.com:
- drivers/gpio/pca953x_gpio.c
- TODO: Add interrupts support
- */
+#include <common.h> +#include <dm.h> +#include <dm/device_compat.h> +#include <i2c.h> +#include <log.h> +#include <asm-generic/gpio.h> +#include <asm/global_data.h> +#include <linux/bitops.h> +#include <dt-bindings/gpio/gpio.h>
+#define FXL6408_DEVID_CTRL 0x01
Do you need the FXL6408 prefix?
+# define FXL6408_SW_RST BIT(0) +# define FXL6408_RST_INT BIT(1)
+/* 3-bit manufacturer ID */ +# define FXL6408_MF_MASK GENMASK(7, 5) +# define FXL6408_MF_ID(devid) (((devid) & FXL6408_MF_MASK) >> 5) +/* 0b101 is for Fairchild assigned by Nokia */ +# define FXL6408_FAIRCHILD_MF 5
+/* 3-bit firmware revision */ +# define FXL6408_FW_MASK GENMASK(4, 2) +# define FXL6408_FW_REV(devid) (((devid) & FXL6408_FW_MASK) >> 2)
This is only used once, so why not include that code inline below?
+/*
- Bits set here indicate that the GPIO is an output.
single-line comment style is /* ... */
- */
+#define FXL6408_DIRECTION 0x03
Then call it DIR_MASK ?
+enum {
FXL6408_DIRECTION_IN,FXL6408_DIRECTION_OUT,+};
+/*
- Bits set here, when the corresponding bit of IO_DIR is set, drive
- the output high instead of low.
- */
+#define FXL6408_OUTPUT 0x05
+/*
- Bits here make the output High-Z, instead of the OUTPUT value.
- */
+#define FXL6408_OUTPUT_HIGH_Z 0x07
+/*
- Bits here define the expected input state of the GPIO.
- INTERRUPT_STATUS bits will be set when the INPUT transitions away
- from this value.
- */
+#define FXL6408_INPUT_DEF_STATE 0x09
+/*
- Bits here enable either pull up or pull down according to
- INPUT_PULL_STATE.
- */
+#define FXL6408_INPUT_PULL_ENABLE 0x0b
+/*
- Bits set here selects a pull-up/pull-down state of pin, which
- is configured as Input and the corresponding PULL_ENABLE bit is
- set.
- */
+#define FXL6408_INPUT_PULL_STATE 0x0d
+/*
- Returns the current status (1 = HIGH) of the input pins.
- */
+#define FXL6408_INPUT 0x0f
+/*
- Mask of pins which can generate interrupts.
- */
+#define FXL6408_INTERRUPT_MASK 0x11 +/*
- Mask of pins which have generated an interrupt.
- Cleared on read.
- */
+#define FXL6408_INTERRUPT_STATUS 0x13
+/*
- struct fxl6408_info - Data for fxl6408
- @dev: udevice structure for the device
- @addr: i2c slave address
Please check; these don't match the struct.
- @reg_output: hold the value of output register
- @reg_direction: hold the value of direction register
- */
+struct fxl6408_info {
struct udevice *dev;int addr;u8 device_id;u8 reg_io_dir;u8 reg_output;+};
+static int fxl6408_write(struct udevice *dev, int reg, u8 val) +{
int ret;ret = dm_i2c_write(dev, reg, &val, 1);if (ret)dev_err(dev, "%s error: %d\n", __func__, ret);return ret;+}
+static int fxl6408_read(struct udevice *dev, int reg, u8 *val)
You could return the value, or -ve for error, saving a parameter.
+{
int ret;u8 tmp;ret = dm_i2c_read(dev, reg, &tmp, 1);if (!ret)*val = tmp;elsedev_err(dev, "%s error: %d\n", __func__, ret);return ret;+}
+/*
- fxl6408_is_output() - check whether the gpio configures as either
output or input.
- Return: 0 - input, 1 - output.
- */
+static int fxl6408_is_output(struct udevice *dev, int offset)
s/int/bool/ and you can drop the !! here and below
+{
struct fxl6408_info *info = dev_get_plat(dev);return !!(info->reg_io_dir & BIT(offset));+}
+static int fxl6408_get_value(struct udevice *dev, uint offset) +{
int ret, reg = fxl6408_is_output(dev, offset) ? FXL6408_OUTPUT : FXL6408_INPUT;u8 val = 0;ret = fxl6408_read(dev, reg, &val);if (IS_ERR_VALUE(ret))return ret;return !!(val & BIT(offset));+}
+static int fxl6408_set_value(struct udevice *dev, uint offset, int value) +{
struct fxl6408_info *info = dev_get_plat(dev);u8 val;int ret;if (value)val = info->reg_output | BIT(offset);elseval = info->reg_output & ~BIT(offset);ret = fxl6408_write(dev, FXL6408_OUTPUT, val);if (!IS_ERR_VALUE(ret))
if (ret < 0) is good enough here
We are not going to change that, and the macros obfuscates things IMO.
info->reg_output = val;return ret;+}
+static int fxl6408_set_direction(struct udevice *dev, uint offset, int dir) +{
struct fxl6408_info *info = dev_get_plat(dev);u8 val;int ret;if (dir == FXL6408_DIRECTION_IN)
Then shouldn't 'dir' be an enum?
val = info->reg_io_dir & ~BIT(offset);elseval = info->reg_io_dir | BIT(offset);ret = fxl6408_write(dev, FXL6408_DIRECTION, val);if (!IS_ERR_VALUE(ret))info->reg_io_dir = val;return ret;+}
+static int fxl6408_direction_input(struct udevice *dev, uint offset) +{
return fxl6408_set_direction(dev, offset, FXL6408_DIRECTION_IN);+}
+static int fxl6408_direction_output(struct udevice *dev, uint offset, int value) +{
int ret;/* Configure output value. */ret = fxl6408_set_value(dev, offset, value);if (!IS_ERR_VALUE(ret))/* Configure direction as output. */fxl6408_set_direction(dev, offset, FXL6408_DIRECTION_OUT);return ret;+}
+static int fxl6408_get_function(struct udevice *dev, uint offset) +{
if (fxl6408_is_output(dev, offset))return GPIOF_OUTPUT;elsereturn GPIOF_INPUT;+}
+static int fxl6408_xlate(struct udevice *dev, struct gpio_desc *desc,
struct ofnode_phandle_args *args)+{
desc->offset = args->args[0];desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0;return 0;+}
+static const struct dm_gpio_ops fxl6408_ops = {
.direction_input = fxl6408_direction_input,.direction_output = fxl6408_direction_output,.get_value = fxl6408_get_value,.set_value = fxl6408_set_value,.get_function = fxl6408_get_function,.xlate = fxl6408_xlate,+};
Can you put that down below with the other struct?
+static int fxl6408_probe(struct udevice *dev) +{
struct fxl6408_info *info = dev_get_plat(dev);struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);char name[32], label[32], *str;int addr;int ret;int size;const u8 *tmp;u8 val;u32 val32;addr = dev_read_addr(dev);if (addr == 0)return -ENODEV;
This has a special meaning in drive model. Try -EINVAL instead for errors reading the DT.
info->addr = addr;/* Check the device ID register to see if it's responding.
Multi-line comment style is:
/* * Check the * .. */
* This also clears RST_INT as a side effect, so we won't get* the "we've been power cycled" interrupt once we enable* interrupts.*/ret = fxl6408_read(dev, FXL6408_DEVID_CTRL, &val);if (IS_ERR_VALUE(ret)) {dev_err(dev, "FXL6408 probe returned %d\n", ret);return ret;}if (FXL6408_MF_ID(val) != FXL6408_FAIRCHILD_MF) {dev_err(dev, "FXL6408 probe returned DID: 0x%02x\n", val);return -ENODEV;
Again you cannot return this, as there is a device. How about -ENXIO?
}info->device_id = val;/** Disable High-Z of outputs, so that the OUTPUT updates* actually take effect.*/ret = fxl6408_write(dev, FXL6408_OUTPUT_HIGH_Z, (u8)0);if (IS_ERR_VALUE(ret)) {dev_err(dev, "Error writing High-Z register\n");return ret;}/** If configured, set initial output state and direction,* otherwise read them from the chip.*/if (IS_ERR_VALUE(dev_read_u32(dev, "initial_io_dir", &val32))) {ret = fxl6408_read(dev, FXL6408_DIRECTION, &info->reg_io_dir);if (IS_ERR_VALUE(ret)) {dev_err(dev, "Error reading direction register\n");return ret;}} else {info->reg_io_dir = val32 & 0xFF;ret = fxl6408_write(dev, FXL6408_DIRECTION, info->reg_io_dir);if (IS_ERR_VALUE(ret)) {dev_err(dev, "Error setting direction register\n");return ret;}}if (IS_ERR_VALUE(dev_read_u32(dev, "initial_output", &val32))) {ret = fxl6408_read(dev, FXL6408_OUTPUT, &info->reg_output);if (IS_ERR_VALUE(ret)) {dev_err(dev, "Error reading output register\n");return ret;}} else {info->reg_output = val32 & 0xFF;ret = fxl6408_write(dev, FXL6408_OUTPUT, info->reg_output);if (IS_ERR_VALUE(ret)) {dev_err(dev, "Error setting output register\n");return ret;}}tmp = dev_read_prop(dev, "label", &size);if (tmp) {size = min(size, (int)sizeof(label) - 1);memcpy(label, tmp, size);label[size] = '\0';snprintf(name, sizeof(name), "%s@%x_", label, info->addr);} else {snprintf(name, sizeof(name), "gpio@%x_", info->addr);}str = strdup(name);if (!str)return -ENOMEM;uc_priv->bank_name = str;uc_priv->gpio_count = dev_get_driver_data(dev);uc_priv->gpio_base = -1;dev_dbg(dev, "%s (FW rev. %ld) is ready\n", str,FXL6408_FW_REV(info->device_id));return 0;+}
+static const struct udevice_id fxl6408_ids[] = {
{ .compatible = "fcs,fxl6408", .data = 8 },{ }+};
+U_BOOT_DRIVER(fxl6408_gpio) = {
.name = "fxl6408_gpio",.id = UCLASS_GPIO,.ops = &fxl6408_ops,.probe = fxl6408_probe,.of_match = fxl6408_ids,.plat_auto = sizeof(struct fxl6408_info),+};
2.31.1
Regards, Simon
Hi Simon,
On Sun, Jul 25, 2021 at 1:01 AM Simon Glass sjg@chromium.org wrote:
Hi Oleksandr,
On Wed, 21 Jul 2021 at 06:21, Oleksandr Suvorov oleksandr.suvorov@toradex.com wrote:
Initial support for Fairchild's 8 bit I2C gpio expander FXL6408. The CONFIG_FXL6408_GPIO define enables support for such devices.
Based on: https://patchwork.kernel.org/patch/9148419/
Signed-off-by: Oleksandr Suvorov oleksandr.suvorov@toradex.com
drivers/gpio/Kconfig | 7 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-fxl6408.c | 371 ++++++++++++++++++++++++++++++++++++ 3 files changed, 379 insertions(+) create mode 100644 drivers/gpio/gpio-fxl6408.c
Reviewed-by: Simon Glass sjg@chromium.org
Lots of nits below
Thanks for your detailed and extremely useful review!
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 0817b12c5f..5883582a7f 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -123,6 +123,13 @@ config DA8XX_GPIO help This driver supports the DA8xx GPIO controller
+config FXL6408_GPIO
bool "FXL6408 I2C GPIO driver"depends on DM_GPIO && DM_I2ChelpSupport for Fairchild Semiconductor FXL6408 I2C 8-bit GPIOexpander.Please add some more details here.
Some details are being added to the next version of the patch set.
[..]
diff --git a/drivers/gpio/gpio-fxl6408.c b/drivers/gpio/gpio-fxl6408.c new file mode 100644 index 0000000000..282ec0a69c --- /dev/null +++ b/drivers/gpio/gpio-fxl6408.c @@ -0,0 +1,371 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright (C) 2021 Toradex
- Copyright (C) 2016 Broadcom
- */
+/**
- DOC: FXL6408 I2C to GPIO expander.
- This chip has 8 GPIO lines out of it, and is controlled by an I2C
- bus (a pair of lines), providing 4x expansion of GPIO lines. It
- also provides an interrupt line out for notifying of state changes.
- Any preconfigured state will be left in place until the GPIO lines
- get activated. At power on, everything is treated as an input,
- default input is HIGH and pulled-up, all interrupts are masked.
- Documentation can be found at:
- This driver bases on:
- the original driver by Eric Anholt eric@anholt.net:
- the Toradex version by Max Krummenacher max.krummenacher@toradex.com:
- the U-boot PCA953x driver by Peng Fan van.freenix@gmail.com:
- drivers/gpio/pca953x_gpio.c
- TODO: Add interrupts support
- */
+#include <common.h> +#include <dm.h> +#include <dm/device_compat.h> +#include <i2c.h> +#include <log.h> +#include <asm-generic/gpio.h> +#include <asm/global_data.h> +#include <linux/bitops.h> +#include <dt-bindings/gpio/gpio.h>
+#define FXL6408_DEVID_CTRL 0x01
Do you need the FXL6408 prefix?
Actually, no :) Thanks, I renamed all registers in a straight manner. They'll be introduced in the next patchset version.
+# define FXL6408_SW_RST BIT(0) +# define FXL6408_RST_INT BIT(1)
+/* 3-bit manufacturer ID */ +# define FXL6408_MF_MASK GENMASK(7, 5) +# define FXL6408_MF_ID(devid) (((devid) & FXL6408_MF_MASK) >> 5) +/* 0b101 is for Fairchild assigned by Nokia */ +# define FXL6408_FAIRCHILD_MF 5
+/* 3-bit firmware revision */ +# define FXL6408_FW_MASK GENMASK(4, 2) +# define FXL6408_FW_REV(devid) (((devid) & FXL6408_FW_MASK) >> 2)
This is only used once, so why not include that code inline below?
This way is clearer to read as for me. Moreover, if we include only FW_REV()'s code inline, the shifting and the purpose of such shifting were in different places. Don't you mind leaving it as is? (with some names simplifying)
+/*
- Bits set here indicate that the GPIO is an output.
single-line comment style is /* ... */
Thanks, fixed!
- */
+#define FXL6408_DIRECTION 0x03
Then call it DIR_MASK ?
Formally, it is not a mask but a register. All register names will be tuned in the next patchset version.
+enum {
FXL6408_DIRECTION_IN,FXL6408_DIRECTION_OUT,+};
+/*
- Bits set here, when the corresponding bit of IO_DIR is set, drive
- the output high instead of low.
- */
+#define FXL6408_OUTPUT 0x05
+/*
- Bits here make the output High-Z, instead of the OUTPUT value.
- */
+#define FXL6408_OUTPUT_HIGH_Z 0x07
+/*
- Bits here define the expected input state of the GPIO.
- INTERRUPT_STATUS bits will be set when the INPUT transitions away
- from this value.
- */
+#define FXL6408_INPUT_DEF_STATE 0x09
+/*
- Bits here enable either pull up or pull down according to
- INPUT_PULL_STATE.
- */
+#define FXL6408_INPUT_PULL_ENABLE 0x0b
+/*
- Bits set here selects a pull-up/pull-down state of pin, which
- is configured as Input and the corresponding PULL_ENABLE bit is
- set.
- */
+#define FXL6408_INPUT_PULL_STATE 0x0d
+/*
- Returns the current status (1 = HIGH) of the input pins.
- */
+#define FXL6408_INPUT 0x0f
+/*
- Mask of pins which can generate interrupts.
- */
+#define FXL6408_INTERRUPT_MASK 0x11 +/*
- Mask of pins which have generated an interrupt.
- Cleared on read.
- */
+#define FXL6408_INTERRUPT_STATUS 0x13
+/*
- struct fxl6408_info - Data for fxl6408
- @dev: udevice structure for the device
- @addr: i2c slave address
Please check; these don't match the struct.
Thanks! Fixed.
- @reg_output: hold the value of output register
- @reg_direction: hold the value of direction register
- */
+struct fxl6408_info {
struct udevice *dev;int addr;u8 device_id;u8 reg_io_dir;u8 reg_output;+};
+static int fxl6408_write(struct udevice *dev, int reg, u8 val) +{
int ret;ret = dm_i2c_write(dev, reg, &val, 1);if (ret)dev_err(dev, "%s error: %d\n", __func__, ret);return ret;+}
+static int fxl6408_read(struct udevice *dev, int reg, u8 *val)
You could return the value, or -ve for error, saving a parameter.
Good shot! Reimplemented.
+{
int ret;u8 tmp;ret = dm_i2c_read(dev, reg, &tmp, 1);if (!ret)*val = tmp;elsedev_err(dev, "%s error: %d\n", __func__, ret);return ret;+}
+/*
- fxl6408_is_output() - check whether the gpio configures as either
output or input.
- Return: 0 - input, 1 - output.
- */
+static int fxl6408_is_output(struct udevice *dev, int offset)
s/int/bool/ and you can drop the !! here and below
Done, thanks!
+{
struct fxl6408_info *info = dev_get_plat(dev);return !!(info->reg_io_dir & BIT(offset));+}
+static int fxl6408_get_value(struct udevice *dev, uint offset) +{
int ret, reg = fxl6408_is_output(dev, offset) ? FXL6408_OUTPUT : FXL6408_INPUT;u8 val = 0;ret = fxl6408_read(dev, reg, &val);if (IS_ERR_VALUE(ret))return ret;return !!(val & BIT(offset));+}
+static int fxl6408_set_value(struct udevice *dev, uint offset, int value) +{
struct fxl6408_info *info = dev_get_plat(dev);u8 val;int ret;if (value)val = info->reg_output | BIT(offset);elseval = info->reg_output & ~BIT(offset);ret = fxl6408_write(dev, FXL6408_OUTPUT, val);if (!IS_ERR_VALUE(ret))if (ret < 0) is good enough here
We are not going to change that, and the macros obfuscates things IMO.
Replaced, thanks.
info->reg_output = val;return ret;+}
+static int fxl6408_set_direction(struct udevice *dev, uint offset, int dir) +{
struct fxl6408_info *info = dev_get_plat(dev);u8 val;int ret;if (dir == FXL6408_DIRECTION_IN)Then shouldn't 'dir' be an enum?
Good point! Done.
val = info->reg_io_dir & ~BIT(offset);elseval = info->reg_io_dir | BIT(offset);ret = fxl6408_write(dev, FXL6408_DIRECTION, val);if (!IS_ERR_VALUE(ret))info->reg_io_dir = val;return ret;+}
+static int fxl6408_direction_input(struct udevice *dev, uint offset) +{
return fxl6408_set_direction(dev, offset, FXL6408_DIRECTION_IN);+}
+static int fxl6408_direction_output(struct udevice *dev, uint offset, int value) +{
int ret;/* Configure output value. */ret = fxl6408_set_value(dev, offset, value);if (!IS_ERR_VALUE(ret))/* Configure direction as output. */fxl6408_set_direction(dev, offset, FXL6408_DIRECTION_OUT);return ret;+}
+static int fxl6408_get_function(struct udevice *dev, uint offset) +{
if (fxl6408_is_output(dev, offset))return GPIOF_OUTPUT;elsereturn GPIOF_INPUT;+}
+static int fxl6408_xlate(struct udevice *dev, struct gpio_desc *desc,
struct ofnode_phandle_args *args)+{
desc->offset = args->args[0];desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0;return 0;+}
+static const struct dm_gpio_ops fxl6408_ops = {
.direction_input = fxl6408_direction_input,.direction_output = fxl6408_direction_output,
I've just found out these callbacks are deprecated. So I'll add set_flags and get_flags callback's implementations in the next version.
.get_value = fxl6408_get_value,.set_value = fxl6408_set_value,.get_function = fxl6408_get_function,.xlate = fxl6408_xlate,+};
Can you put that down below with the other struct?
Done.
+static int fxl6408_probe(struct udevice *dev) +{
struct fxl6408_info *info = dev_get_plat(dev);struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);char name[32], label[32], *str;int addr;int ret;int size;const u8 *tmp;u8 val;u32 val32;addr = dev_read_addr(dev);if (addr == 0)return -ENODEV;This has a special meaning in drive model. Try -EINVAL instead for errors reading the DT.
Thanks! Fixed.
info->addr = addr;/* Check the device ID register to see if it's responding.Multi-line comment style is:
Thanks, fixed!
/*
- Check the
- ..
*/
* This also clears RST_INT as a side effect, so we won't get* the "we've been power cycled" interrupt once we enable* interrupts.*/ret = fxl6408_read(dev, FXL6408_DEVID_CTRL, &val);if (IS_ERR_VALUE(ret)) {dev_err(dev, "FXL6408 probe returned %d\n", ret);return ret;}if (FXL6408_MF_ID(val) != FXL6408_FAIRCHILD_MF) {dev_err(dev, "FXL6408 probe returned DID: 0x%02x\n", val);return -ENODEV;Again you cannot return this, as there is a device. How about -ENXIO?
Sounds logical. Fixed.
}info->device_id = val;/** Disable High-Z of outputs, so that the OUTPUT updates* actually take effect.*/ret = fxl6408_write(dev, FXL6408_OUTPUT_HIGH_Z, (u8)0);if (IS_ERR_VALUE(ret)) {dev_err(dev, "Error writing High-Z register\n");return ret;}/** If configured, set initial output state and direction,* otherwise read them from the chip.*/if (IS_ERR_VALUE(dev_read_u32(dev, "initial_io_dir", &val32))) {ret = fxl6408_read(dev, FXL6408_DIRECTION, &info->reg_io_dir);if (IS_ERR_VALUE(ret)) {dev_err(dev, "Error reading direction register\n");return ret;}} else {info->reg_io_dir = val32 & 0xFF;ret = fxl6408_write(dev, FXL6408_DIRECTION, info->reg_io_dir);if (IS_ERR_VALUE(ret)) {dev_err(dev, "Error setting direction register\n");return ret;}}if (IS_ERR_VALUE(dev_read_u32(dev, "initial_output", &val32))) {ret = fxl6408_read(dev, FXL6408_OUTPUT, &info->reg_output);if (IS_ERR_VALUE(ret)) {dev_err(dev, "Error reading output register\n");return ret;}} else {info->reg_output = val32 & 0xFF;ret = fxl6408_write(dev, FXL6408_OUTPUT, info->reg_output);if (IS_ERR_VALUE(ret)) {dev_err(dev, "Error setting output register\n");return ret;}}tmp = dev_read_prop(dev, "label", &size);if (tmp) {size = min(size, (int)sizeof(label) - 1);memcpy(label, tmp, size);label[size] = '\0';snprintf(name, sizeof(name), "%s@%x_", label, info->addr);} else {snprintf(name, sizeof(name), "gpio@%x_", info->addr);}str = strdup(name);if (!str)return -ENOMEM;uc_priv->bank_name = str;uc_priv->gpio_count = dev_get_driver_data(dev);uc_priv->gpio_base = -1;dev_dbg(dev, "%s (FW rev. %ld) is ready\n", str,FXL6408_FW_REV(info->device_id));return 0;+}
+static const struct udevice_id fxl6408_ids[] = {
{ .compatible = "fcs,fxl6408", .data = 8 },{ }+};
+U_BOOT_DRIVER(fxl6408_gpio) = {
.name = "fxl6408_gpio",.id = UCLASS_GPIO,.ops = &fxl6408_ops,.probe = fxl6408_probe,.of_match = fxl6408_ids,.plat_auto = sizeof(struct fxl6408_info),+};
2.31.1
Regards, Simon
Best regards Oleksandr Suvorov
Toradex AG Ebenaustrasse 10 | 6048 Horw | Switzerland | T: +41 41 500 48 00
Hi Oleksandr,
On Sun, 25 Jul 2021 at 16:19, Oleksandr Suvorov oleksandr.suvorov@toradex.com wrote:
Hi Simon,
On Sun, Jul 25, 2021 at 1:01 AM Simon Glass sjg@chromium.org wrote:
Hi Oleksandr,
On Wed, 21 Jul 2021 at 06:21, Oleksandr Suvorov oleksandr.suvorov@toradex.com wrote:
Initial support for Fairchild's 8 bit I2C gpio expander FXL6408. The CONFIG_FXL6408_GPIO define enables support for such devices.
Based on: https://patchwork.kernel.org/patch/9148419/
Signed-off-by: Oleksandr Suvorov oleksandr.suvorov@toradex.com
drivers/gpio/Kconfig | 7 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-fxl6408.c | 371 ++++++++++++++++++++++++++++++++++++ 3 files changed, 379 insertions(+) create mode 100644 drivers/gpio/gpio-fxl6408.c
Reviewed-by: Simon Glass sjg@chromium.org
Lots of nits below
Thanks for your detailed and extremely useful review!
[..]
+/* 3-bit firmware revision */ +# define FXL6408_FW_MASK GENMASK(4, 2) +# define FXL6408_FW_REV(devid) (((devid) & FXL6408_FW_MASK) >> 2)
This is only used once, so why not include that code inline below?
This way is clearer to read as for me. Moreover, if we include only FW_REV()'s code inline, the shifting and the purpose of such shifting were in different places. Don't you mind leaving it as is? (with some names simplifying)
Yes that's fine, you have my review tag for the next version. FYI we tend to do things like:
enum { FW_MASK = GENMASK(4, 2), FW_SHIFT = 2, };
then open-code the assignment or reading. Often there is only one read and one write in the source code, since registers are typically handled in only a few places:
reg_val = (old_val & ~FW_MASK) | (val << FW_SHIFT);
val = (reg_val & FW_MASK) >> FW_SHIFT;
Regards, Simon
participants (2)
-
Oleksandr Suvorov -
Simon Glass