* [PATCH 0/2] Add support of the FXL6408 GPIO expander
@ 2021-07-21 12:20 Oleksandr Suvorov
2021-07-21 12:20 ` [PATCH 1/2] GPIO: fxl6408: Add support for " Oleksandr Suvorov
0 siblings, 1 reply; 6+ messages in thread
From: Oleksandr Suvorov @ 2021-07-21 12:20 UTC (permalink / raw)
To: u-boot
Cc: Oleksandr Suvorov, Adam Ford, Fabio Estevam, Harm Berntsen,
Heiko Schocher, Igor Opaniuk, Linus Walleij, Michal Simek,
NXP i.MX U-Boot Team, Philippe Schenker, Rayagonda Kokatanur,
Simon Glass, Stefan Bosch, Stefan Roese, Stefano Babic,
Stephan Gerhold, Suneel Garapati, Tero Kristo, Weijie Gao
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
--
2.31.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] GPIO: fxl6408: Add support for FXL6408 GPIO expander
2021-07-21 12:20 [PATCH 0/2] Add support of the FXL6408 GPIO expander Oleksandr Suvorov
@ 2021-07-21 12:20 ` Oleksandr Suvorov
2021-07-21 12:20 ` [PATCH 2/2] colibri-imx8x: add on-module gpio expander fxl6408 Oleksandr Suvorov
2021-07-24 22:01 ` [PATCH 1/2] GPIO: fxl6408: Add support for FXL6408 GPIO expander Simon Glass
0 siblings, 2 replies; 6+ messages in thread
From: Oleksandr Suvorov @ 2021-07-21 12:20 UTC (permalink / raw)
To: u-boot
Cc: Oleksandr Suvorov, Harm Berntsen, Heiko Schocher, Linus Walleij,
Michal Simek, Rayagonda Kokatanur, Simon Glass, Stefan Bosch,
Stefan Roese, Stephan Gerhold, Suneel Garapati, Weijie Gao
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.c?h=toradex_5.4-2.3.x-imx
+ * - 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),
+};
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] colibri-imx8x: add on-module gpio expander fxl6408
2021-07-21 12:20 ` [PATCH 1/2] GPIO: fxl6408: Add support for " Oleksandr Suvorov
@ 2021-07-21 12:20 ` Oleksandr Suvorov
2021-07-24 22:01 ` [PATCH 1/2] GPIO: fxl6408: Add support for FXL6408 GPIO expander Simon Glass
1 sibling, 0 replies; 6+ messages in thread
From: Oleksandr Suvorov @ 2021-07-21 12:20 UTC (permalink / raw)
To: u-boot
Cc: Oleksandr Suvorov, Adam Ford, Fabio Estevam, Igor Opaniuk,
NXP i.MX U-Boot Team, Philippe Schenker, Stefano Babic,
Tero Kristo
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
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] GPIO: fxl6408: Add support for FXL6408 GPIO expander
2021-07-21 12:20 ` [PATCH 1/2] GPIO: fxl6408: Add support for " Oleksandr Suvorov
2021-07-21 12:20 ` [PATCH 2/2] colibri-imx8x: add on-module gpio expander fxl6408 Oleksandr Suvorov
@ 2021-07-24 22:01 ` Simon Glass
2021-07-25 22:19 ` Oleksandr Suvorov
1 sibling, 1 reply; 6+ messages in thread
From: Simon Glass @ 2021-07-24 22:01 UTC (permalink / raw)
To: Oleksandr Suvorov
Cc: U-Boot Mailing List, Harm Berntsen, Heiko Schocher,
Linus Walleij, Michal Simek, Rayagonda Kokatanur, Stefan Bosch,
Stefan Roese, Stephan Gerhold, Suneel Garapati, Weijie Gao
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_I2C
> + help
> + Support for Fairchild Semiconductor FXL6408 I2C 8-bit GPIO
> + expander.
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:
> + * 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.c?h=toradex_5.4-2.3.x-imx
> + * - 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;
> + 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)
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);
> + else
> + val = 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);
> + 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,
> +};
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] GPIO: fxl6408: Add support for FXL6408 GPIO expander
2021-07-24 22:01 ` [PATCH 1/2] GPIO: fxl6408: Add support for FXL6408 GPIO expander Simon Glass
@ 2021-07-25 22:19 ` Oleksandr Suvorov
2021-07-26 14:06 ` Simon Glass
0 siblings, 1 reply; 6+ messages in thread
From: Oleksandr Suvorov @ 2021-07-25 22:19 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Harm Berntsen, Heiko Schocher,
Linus Walleij, Michal Simek, Rayagonda Kokatanur, Stefan Bosch,
Stefan Roese, Stephan Gerhold, Suneel Garapati, Weijie Gao
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_I2C
> > + help
> > + Support for Fairchild Semiconductor FXL6408 I2C 8-bit GPIO
> > + expander.
>
> 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:
> > + * 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.c?h=toradex_5.4-2.3.x-imx
> > + * - 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;
> > + 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)
>
> 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);
> > + else
> > + val = 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);
> > + 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,
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] GPIO: fxl6408: Add support for FXL6408 GPIO expander
2021-07-25 22:19 ` Oleksandr Suvorov
@ 2021-07-26 14:06 ` Simon Glass
0 siblings, 0 replies; 6+ messages in thread
From: Simon Glass @ 2021-07-26 14:06 UTC (permalink / raw)
To: Oleksandr Suvorov
Cc: U-Boot Mailing List, Harm Berntsen, Heiko Schocher,
Linus Walleij, Michal Simek, Rayagonda Kokatanur, Stefan Bosch,
Stefan Roese, Stephan Gerhold, Suneel Garapati, Weijie Gao
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-26 14:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-21 12:20 [PATCH 0/2] Add support of the FXL6408 GPIO expander Oleksandr Suvorov
2021-07-21 12:20 ` [PATCH 1/2] GPIO: fxl6408: Add support for " Oleksandr Suvorov
2021-07-21 12:20 ` [PATCH 2/2] colibri-imx8x: add on-module gpio expander fxl6408 Oleksandr Suvorov
2021-07-24 22:01 ` [PATCH 1/2] GPIO: fxl6408: Add support for FXL6408 GPIO expander Simon Glass
2021-07-25 22:19 ` Oleksandr Suvorov
2021-07-26 14:06 ` Simon Glass
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).