i Michal,
On Thu, Sep 16, 2021 at 10:05 AM Michal Simek <michal.si...@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.si...@xilinx.com>
> > wrote:
> >>
> >>
> >>
> >> On 9/9/21 10:44 PM, Oleksandr Suvorov wrote:
> >>> From: Oleksandr Suvorov <oleksandr.suvo...@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.suvo...@toradex.com>
> >>> Reviewed-by: Simon Glass <s...@chromium.org>
> >>> Tested-by: Marcel Ziswiler <marcel.ziswi...@toradex.com>
> >>> Signed-off-by: Oleksandr Suvorov <cryo...@gmail.com>
> >>> Signed-off-by: Oleksandr Suvorov <oleksandr.suvo...@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_I2C
> >>> + help
> >>> + This driver supports the Fairchild FXL6408 device. FXL6408 is a
> >>> + fully 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
>
--
Best regards
Oleksandr
Oleksandr Suvorov
cryo...@gmail.com
