Dear all,
Sorry to bother you and we resend it. The new patch will be submitted in
another email.
> >>> 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.
This GPIO controller is located behind PCI-E device, We refer to
drivers/gpio/gpio-amd8111.c.
>
> > +#define PT_DEBOUNCE_REG 0x0C
>
> So the driver should support the .set_debounce() method.
rename PT_DEBOUNCE_REG to PT_CLOCKRATE_REG.
>
> > +#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.
rename PT_VENDOR0_REG to PT_SYNC_REG, remove PT_VENDOR1_REG
>
> > +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.
Modify in the new patch.
>
> > + 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.
It's only used to sync the gpio pins.(avoid reusing the same pin) We modified
some code in pt_gpio_request(), this routine will return EINVAL if a pin is
used.
>
> > + 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).
Modify in the new patch.
>
> But again, I do not believe the real name of that register is
> "VENDER0" and you should implement debounce properly
> and use GPIO_GENERIC
rename PT_VENDOR0_REG to PT_SYNC_REG
>
> Yours,
> Linus Walleij
Best Regards,
YD Tseng
--
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
