i Michal,
On Thu, Sep 16, 2021 at 10:05 AM Michal Simek michal.simek@xilinx.com wrote:
On 9/15/21 8:35 PM, Oleksandr Suvorov wrote:
Hi Michal,
On Fri, Sep 10, 2021 at 9:35 AM Michal Simek michal.simek@xilinx.com wrote:
On 9/9/21 10:44 PM, Oleksandr Suvorov wrote:
From: Oleksandr Suvorov oleksandr.suvorov@toradex.com
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 Reviewed-by: Simon Glass sjg@chromium.org Tested-by: Marcel Ziswiler marcel.ziswiler@toradex.com Signed-off-by: Oleksandr Suvorov cryosay@gmail.com Signed-off-by: Oleksandr Suvorov oleksandr.suvorov@foundries.io
3 emails for you. Would be the best to choose only one.
Thanks, fixed. That habit to always add '-s' when committing changes :)
Changes in v3:
- fix a warning: ...drivers/gpio/gpio-fxl6408.c:348:15: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 3 has type ‘int’ [-Wformat=]...
- add Tested-by record.
drivers/gpio/Kconfig | 7 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-fxl6408.c | 366 ++++++++++++++++++++++++++++++++++++ 3 files changed, 374 insertions(+) create mode 100644 drivers/gpio/gpio-fxl6408.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 4a89c1a62b..f56e4cc261 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 expander driver"depends on DM_GPIO && DM_I2ChelpThis driver supports the Fairchild FXL6408 device. FXL6408 is afully configurable 8-bit I2C-controlled 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 58f4704f6b..83d8b5c9d8 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..dbc68618f9 --- /dev/null +++ b/drivers/gpio/gpio-fxl6408.c @@ -0,0 +1,366 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright (C) 2021 Toradex
- Copyright (C) 2016 Broadcom
- */
+/**
Below is not kernel-doc format
I don't think so. And without formatting comments of this block as a kernel-doc, the kernel-doc tool shows the warning: drivers/gpio/gpio-fxl6408.c:1: warning: no structured comments found
Because script only checks format starting with /**.
Sure, missed that "little" thing :) Thanks!
<snip>
+};
+/*
/** this should be kernel doc.
Run this ./scripts/kernel-doc -man -v 1>/dev/null *.c
to check that all values are right.
The kernel-doc doesn't found errors.
Because it doesn't find any structure to check. Add here /** and run that script again.
- struct fxl6408_info - Data for fxl6408
- @dev: udevice structure for the device
- @addr: i2c slave address
- @reg_io_dir: hold the value of direction register
- @reg_output: hold the value of output register
- */
+struct fxl6408_info {
struct udevice *dev;int addr;u8 device_id;u8 reg_io_dir;u8 reg_output;+};
+static inline int fxl6408_write(struct udevice *dev, int reg, u8 val) +{
return dm_i2c_write(dev, reg, &val, 1);+}
+static int fxl6408_read(struct udevice *dev, int reg) +{
int ret;u8 tmp;ret = dm_i2c_read(dev, reg, &tmp, 1);if (!ret)ret = tmp;This looks completely wrong. What value are you returning in case of error.
Nope. In case of error, I return an error value stored in "ret".
If ops->xfer is not defined dm_i2c_read returns -ENOSYS. tmp is not initialized and can have random value that's why here in case or error you will return ramdom value.
!(-ENOSYS) == false, thus "if" op fails and doesn't perform "ret = tmp". I don't see any errors here.
You are right. I read it incorrectly.
<snip>
/** If configured, set initial output state and direction,* otherwise read them from the chip.*/if (dev_read_u32(dev, "initial_io_dir", &val32)) {Where do you have dt binding document for this chip? I can't see anything in the linux kernel or in your series.
It is good that you read values from dt but you shouldn't do it in probe. You should create platdata and fill it by information from DT and then in probe just read them from platdata structure. This applied to all dt read in probe.
ret = fxl6408_read(dev, REG_IO_DIR);if (ret < 0) {dev_err(dev, "Error reading direction register\n");return ret;}info->reg_io_dir = ret;} else {info->reg_io_dir = val32 & 0xFF;val32 is not initialized above. dev_read_u32 above fails that's why val32 wont't be initialized and here you do calculation with it. That's quite weird behavior. The same pattern visible below.
Nope. dev_read_u32() returns 0 on success. The logic is correct.
You are right again. I normally use reverse logic.
ret = fxl6408_write(dev, REG_IO_DIR, info->reg_io_dir);if (ret < 0) {dev_err(dev, "Error setting direction register\n");return ret;}}if (dev_read_u32(dev, "initial_output", &val32)) {ret = fxl6408_read(dev, REG_OUT_STATE);if (ret < 0) {dev_err(dev, "Error reading output register\n");return ret;}info->reg_output = ret;} else {info->reg_output = val32 & 0xFF;ret = fxl6408_write(dev, REG_OUT_STATE, info->reg_output);if (ret < 0) {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);}I see some code around labels in gpio-uclass. I would be surprised if it is not there already because it should be core functionality not driver when labels are defined.
This code is not about gpios labeling. It's about the bank name, it just sets the uc_priv->bank_name which is used in gpio-uclass.
Can you please use different variable name then label. If this is bank_name just call it bank name.
It makes sense. I'll rename it in the next revision. Thanks!
Thanks, Michal