On Fri, Oct 2, 2015 at 5:05 AM, YD Tseng <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
