Re: [PATCH] drivers: field for new GPIO driver

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Fri, Oct 2, 2015 at 5:05 AM, YD Tseng <ltyu101@xxxxxxxxx> wrote:

>>> Is this hardware really different or can the old driver be augmented to handle both?

> This hardware is different.  We design a new silicon for AMD.

OK

> +#include <linux/gpio.h>

Should only #include <linux/gpio/driver.h>

> +/* memory mapped register offsets */
> +#define PT_DIRECTION_REG   0x00
> +#define PT_INPUTDATA_REG   0x04
> +#define PT_OUTPUTDATA_REG  0x08

A very simple memory-mapped I/O GPIO, you should use
select GPIO_GENERIC
#include <linux/basic_mmio_gpio.h>

And see for example drivers/gpio/gpio-74xx-mmio.c
on how to use this generic MMIO library right.

> +#define PT_DEBOUNCE_REG    0x0C

So the driver should support the .set_debounce() method.

> +#define PT_VENDER0_REG     0x28
> +#define PT_VENDER1_REG     0x2C

Is it not VENDOR, and what is in these registers really?
>From the code it seems it is pin control registers, and this
driver should then be in drivers/pinctrl/*.

See for example drivers/pinctrl/intel/* for how to do that
right.

> +static int pt_gpio_request(struct gpio_chip *gc, unsigned offset)
> +{
> +       struct pt_gpio_chip *pt_gpio = to_pt_gpio(gc);
> +       struct platform_device *pdev;
> +       void __iomem *v_reg = pt_gpio->reg_base + PT_VENDER0_REG;

Too many local variables. Just used a->b->c directly.
Well this will be using the MMIO library anyway so most stuff
go away.

> +       using_pins = readl(v_reg);
> +       if (using_pins&(1<<offset))
> +               dev_warn(&pdev->dev, "PT GPIO pin %x reconfigured\n", offset);
> +       else
> +               writel(using_pins|(1<<offset), v_reg);

What is this? Pin multiplexing? Then implement this
properly using pin control.

> +       void __iomem *reg;
(...)
> +       /* initialize register setting */
> +       reg = pt_gpio->reg_base + PT_VENDER0_REG;
> +       writel(0, reg);
> +       reg = pt_gpio->reg_base + PT_DEBOUNCE_REG;
> +       writel(0, reg);

Argh no, just writel(0, pt_gpio->reg_base + PT_VENDOR0_REG);
Too many helper variables. The compiler will optimize it out, but
this makes things hard to read (for me).

But again, I do not believe the real name of that register is
"VENDER0" and you should implement debounce properly
and use GPIO_GENERIC

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html