On Tue, Jan 16, 2018 at 10:42:54AM +0100, Linus Walleij wrote: > On Mon, Jan 15, 2018 at 4:13 AM, Jonathan Neuschäfer > <j.neuschaefer@xxxxxxx> wrote: > > > This patch is based on code developed by Albert Herranz and the GameCube > > Linux Team, file arch/powerpc/platforms/embedded6xx/hlwd-gpio.c, > > available at https://github.com/DeltaResero/GC-Wii-Linux-Kernels, but > > has grown quite dissimilar. > > I'm impressed by this effort. As with all reverse engineering. > > > This driver currently uses __raw_readl and __raw_writel to access the > > GPIO controller's MMIO registers. I wonder if readl/writel plus explicit > > byte-swapping would be more correct, because it could be independent of > > the CPU's endianness. That said, this hardware only exists in two > > big-endian machines (Wii and Wii U). > > I don't know about PPC but I think you're supposed to use > ioread32be() and iowrite32be() to do explicit BE access. Ah, that's the name! I didn't find ioread32*/iowrite32* in the documentation or source code. > But when I look at it, I think you can just use the gpio-mmio library > for this driver and cut down code cosiderably. I'll look into it. So far it looks good (drivers/gpio/gpio-iop.c has just 60 lines). > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > Can't you just save a pointer to struct device *dev in the > state container and use dev_info(state->dev, ...) etc instead > of this? Makes sense. I'll try this out. > > +#include <linux/of_gpio.h> > > This include should not be needed. Okay. > > +/* > > + * Update the bit with the given bit offset in the given register to a given > > + * value > > + */ > > +static void hlwd_gpio_update_bit(struct gpio_chip *gc, unsigned int reg, > > + int offset, int value) > > +{ > > + struct hlwd_gpio *hlwd = gpiochip_get_data(gc); > > + unsigned long flags; > > + u32 bit = 1UL << offset; > > #include <linux/bitops.h> > > u32 bit = BIT(offset); > > > + u32 tmp; > > + > > + spin_lock_irqsave(&hlwd->lock, flags); > > + tmp = __raw_readl(hlwd->regs + reg); > > + if (value) > > + __raw_writel(tmp | bit, hlwd->regs + reg); > > + else > > + __raw_writel(tmp & ~bit, hlwd->regs + reg); > > + spin_unlock_irqrestore(&hlwd->lock, flags); > > +} > > This looks very much like it is reimplementing the stuff we already > have in drivers/gpio/gpio-mmio.h. > > There is even a big endian access flag for the library. > And you get so much for free with gpio-mmio. > > select GPIO_GENERIC > in Kconfig > > the helpers come in from <linux/gpio/driver.h> > > Look at other drivers for inspiration: > git grep bgpio_init > > If you need IRQ support you should probably have your own file > for this driver, but it will be just a few lines of wrapper using > bgpio_init() and BGPIOF_BIG_ENDIAN and/or possibly > BGPIOF_BIG_ENDIAN_BYTE_ORDER. Yes, I plan to add IRQ support in a later patch. > > See the other drivers. Yep, gpio-mmio looks like a good option, thanks for the pointer! Thanks, Jonathan Neuschäfer
Attachment:
signature.asc
Description: PGP signature
