linux-kernel - Re: [PATCH 3/6] gpio: Add GPIO driver for Nintendo Wii

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180116215847.qlygkbnecni7bzog@latitude>
Date:   Tue, 16 Jan 2018 22:58:47 +0100
From:   Jonathan Neuschäfer <j.neuschaefer@....net>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Jonathan Neuschäfer <j.neuschaefer@....net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linuxppc-dev@...ts.ozlabs.org list" <linuxppc-dev@...ts.ozlabs.org>,
        linux-gpio@...r.kernel.org,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Albert Herranz <albert_herranz@...oo.es>,
        Segher Boessenkool <segher@...nel.crashing.org>
Subject: Re: [PATCH 3/6] gpio: Add GPIO driver for Nintendo Wii

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@....net> 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

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)