[PATCH v3 1/2] GPIO: fxl6408: Add support for FXL6408 GPIO expander
Michal Simek
michal.simek at xilinx.com
Thu Sep 16 09:05:47 CEST 2021
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 at xilinx.com> wrote:
>>
>>
>>
>> On 9/9/21 10:44 PM, Oleksandr Suvorov wrote:
>>> From: Oleksandr Suvorov <oleksandr.suvorov at 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 at toradex.com>
>>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>> Tested-by: Marcel Ziswiler <marcel.ziswiler at toradex.com>
>>> Signed-off-by: Oleksandr Suvorov <cryosay at gmail.com>
>>> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov at 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 /**.
<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.
Thanks,
Michal
More information about the U-Boot
mailing list