Re: [PATCH v2 2/8] Add Advantech EIO GPIO driver

Hi Ramiro,

thanks for your patch!

On Tue, Jul 14, 2026 at 5:54 PM Ramiro Oliveira
<[email protected]> wrote:


> +static int get_dir(struct gpio_chip *chip, unsigned int offset)
> +{
> +       u8 dir;
> +       int ret;
> +
> +       ret = pmc_read(chip->parent, EIO_GPIO_PIN_DIR, offset, &dir);
> +       if (ret)
> +               return ret;
> +
> +       return dir ? 0 : 1;

Please use:
return dir ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;

> +static int dir_input(struct gpio_chip *chip, unsigned int offset)
> +{
> +       u8 dir = 0;

Use some local define for this stuff like:

#define EIO_GPIO_DIR_IN 0
#define EIO_GPIO_DIR_OUT 1

> +static int dir_output(struct gpio_chip *chip, unsigned int offset, int value)
> +{
> +       u8 dir = 1;

Also here.

> +       u8 val = value;
> +
> +       pmc_write(chip->parent, EIO_GPIO_PIN_DIR, offset, &dir);
> +
> +       return pmc_write(chip->parent, EIO_GPIO_PIN_LEVEL, offset, &val);
> +}
> +
> +static int gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       u8 level;
> +       int ret;
> +
> +       ret = pmc_read(chip->parent, EIO_GPIO_PIN_LEVEL, offset, &level);
> +       if (ret)
> +               return ret;
> +
> +       return level;

return !!level;

so you clamp this to [0, 1]

> +static int check_support(struct device *dev)

Check support for what? Spell it out.

> +{
> +       u8  data;
> +       int ret;
> +
> +       ret = pmc_read(dev, EIO_GPIO_STATUS, 0, &data);
> +       if (ret)
> +               return ret;
> +
> +       if ((data & 0x01) == 0)
> +               return -EOPNOTSUPP;

Use:

#define EIO_GPIO_HAS_SUPPORT_FOR_X BIT(0)

if (!(data & EIO_GPIO_HAS_SUPPORT_FOR_X))
        return -EOPNOTSUPP;

> +static int check_pin(struct device *dev, int pin)
> +{
> +       int ret;
> +       int group, bit;
> +       u16 data;
> +
> +       /* Get pin mapping */
> +       ret = pmc_read(dev, EIO_GPIO_MAPPING, pin, &data);
> +       if (ret)
> +               return ret;
> +
> +       if ((data & 0xFF) > ARRAY_SIZE(group_map))
> +               return -EINVAL;
> +
> +       group = group_map[data & 0xFF].group;
> +       bit   = data >> 8;
> +
> +       /* Check mapped pin */
> +       ret = pmc_read(dev, EIO_GPIO_GROUP_AVAIL, group, &data);
> +       if (ret)
> +               return ret;
> +
> +       return data & BIT(bit) ? 0 : -EOPNOTSUPP;
> +}

So there is some pin muxing going on here. If that should be made dynamic
at some point this should make use of the pin control subsystem, but I guess
it is currently hard-coded?

Yours,
Linus Walleij