Re: [PATCH v3 2/4] gpio: Add GPIO driver for Nintendo Wii

2018-02-22 Thread Jonathan Neuschäfer
On Thu, Feb 22, 2018 at 01:57:07PM +0100, Linus Walleij wrote:
> On Fri, Feb 9, 2018 at 1:07 PM, Jonathan Neuschäfer
>  wrote:
> 
> > The Nintendo Wii's chipset (called "Hollywood") has a GPIO controller
> > that supports a configurable number of pins (up to 32), interrupts, and
> > some special mechanisms to share the controller between the system's
> > security processor (an ARM926) and the PowerPC CPU. Pin multiplexing is
> > not supported.
> >
> > This patch adds a basic driver for this GPIO controller. Interrupt
> > support will come in a later patch.
> >
> > 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.
> >
> > Signed-off-by: Jonathan Neuschäfer 
> > Cc: Albert Herranz 
> > Cc: Segher Boessenkool 
> > <---
> >
> > v3:
> > - Do some style cleanups, as suggest by Andy Shevchenko
> 
> Patch applied to the GPIO tree for v4.17 with all the review tags.
> 
> I just folded the changelog into the commit message, for new
> drivers it is sometimes useful to keep these around in
> git actually.
> 
> If any further changes are needed we can just patch on top
> of this.
> 
> It's a very pretty driver, good work!

Thanks! :-)


Jonathan Neuschäfer


signature.asc
Description: PGP signature


Re: [PATCH v3 2/4] gpio: Add GPIO driver for Nintendo Wii

2018-02-22 Thread Linus Walleij
On Fri, Feb 9, 2018 at 1:07 PM, Jonathan Neuschäfer
 wrote:

> The Nintendo Wii's chipset (called "Hollywood") has a GPIO controller
> that supports a configurable number of pins (up to 32), interrupts, and
> some special mechanisms to share the controller between the system's
> security processor (an ARM926) and the PowerPC CPU. Pin multiplexing is
> not supported.
>
> This patch adds a basic driver for this GPIO controller. Interrupt
> support will come in a later patch.
>
> 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.
>
> Signed-off-by: Jonathan Neuschäfer 
> Cc: Albert Herranz 
> Cc: Segher Boessenkool 
> <---
>
> v3:
> - Do some style cleanups, as suggest by Andy Shevchenko

Patch applied to the GPIO tree for v4.17 with all the review tags.

I just folded the changelog into the commit message, for new
drivers it is sometimes useful to keep these around in
git actually.

If any further changes are needed we can just patch on top
of this.

It's a very pretty driver, good work!

Yours,
Linus Walleij


Re: [PATCH v3 2/4] gpio: Add GPIO driver for Nintendo Wii

2018-02-09 Thread Jonathan Neuschäfer
On Fri, Feb 09, 2018 at 05:30:55PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 9, 2018 at 2:07 PM, Jonathan Neuschäfer
>  wrote:
> > The Nintendo Wii's chipset (called "Hollywood") has a GPIO controller
> > that supports a configurable number of pins (up to 32), interrupts, and
> > some special mechanisms to share the controller between the system's
> > security processor (an ARM926) and the PowerPC CPU. Pin multiplexing is
> > not supported.
> >
> > This patch adds a basic driver for this GPIO controller. Interrupt
> > support will come in a later patch.
> >
> > 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.
> >
> 
> Fine to me, though one comment below.
> In any case,
> 
> Reviewed-by: Andy Shevchenko 

Thank you.


[...]
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index d6a8e851ad13..47606dfe06cc 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -229,6 +229,15 @@ config GPIO_GRGPIO
> >   Select this to support Aeroflex Gaisler GRGPIO cores from the 
> > GRLIB
> >   VHDL IP core library.
> >
> > +config GPIO_HLWD
> > +   tristate "Nintendo Wii (Hollywood) GPIO"
> 
> > +   depends on OF_GPIO
> 
> You may get rid of it if...

[ Even if this driver isn't switched to the unified device property API,
  I think "depends on OF" would be enough here, because it doesn't use
  the code that's guarded by CONFIG_OF_GPIO (gpiolib-of.c), but this
  applies to other drivers (e.g. gpio-aspeed, gpio-bcm-kona) as well, so
  this would ideally be a bigger cleanup patch. ]


> > +   res = of_property_read_u32(pdev->dev.of_node, "ngpios", );
> 
> ...if you switch to unified device property API.

I don't think this change is worth making, unless/until the of_property
API is deprecated. I'm rather sure this GPIO controller won't appear in
an ACPI-based system.


Thanks,
Jonathan Neuschäfer


signature.asc
Description: PGP signature


Re: [PATCH v3 2/4] gpio: Add GPIO driver for Nintendo Wii

2018-02-09 Thread Jonathan Neuschäfer
On Fri, Feb 09, 2018 at 01:07:29PM +0100, Jonathan Neuschäfer wrote:
> The Nintendo Wii's chipset (called "Hollywood") has a GPIO controller
> that supports a configurable number of pins (up to 32), interrupts, and
> some special mechanisms to share the controller between the system's
> security processor (an ARM926) and the PowerPC CPU. Pin multiplexing is
> not supported.
> 
> This patch adds a basic driver for this GPIO controller. Interrupt
> support will come in a later patch.
> 
> 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.
> 
> Signed-off-by: Jonathan Neuschäfer 
> Cc: Albert Herranz 
> Cc: Segher Boessenkool 
> <---

Ooops, I just noticed that I broke the separator here. This should be a
normal --- line, obviously.


Jonathan Neuschäfer


signature.asc
Description: PGP signature


Re: [PATCH v3 2/4] gpio: Add GPIO driver for Nintendo Wii

2018-02-09 Thread Andy Shevchenko
On Fri, Feb 9, 2018 at 2:07 PM, Jonathan Neuschäfer
 wrote:
> The Nintendo Wii's chipset (called "Hollywood") has a GPIO controller
> that supports a configurable number of pins (up to 32), interrupts, and
> some special mechanisms to share the controller between the system's
> security processor (an ARM926) and the PowerPC CPU. Pin multiplexing is
> not supported.
>
> This patch adds a basic driver for this GPIO controller. Interrupt
> support will come in a later patch.
>
> 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.
>

Fine to me, though one comment below.
In any case,

Reviewed-by: Andy Shevchenko 

> Signed-off-by: Jonathan Neuschäfer 
> Cc: Albert Herranz 
> Cc: Segher Boessenkool 
> <---
>
> v3:
> - Do some style cleanups, as suggest by Andy Shevchenko
>
> v2:
> - Change hlwd_gpio_driver.driver.name to "gpio-hlwd" to match the
>   filename (was "hlwd_gpio")
> - Remove unnecessary include of linux/of_gpio.h, as suggested by Linus
>   Walleij.
> - Add struct device pointer to context struct to make it possible to use
>   dev_info(hlwd->dev, "..."), as suggested by Linus Walleij
> - Use the GPIO_GENERIC library to reduce code size, as suggested by
>   Linus Walleij
> - Use iowrite32be instead of __raw_writel for big-endian MMIO access, as
>   suggested by Linus Walleij
> - Remove commit message paragraph suggesting to diff against the
>   original driver, because it's so different now
> ---
>  drivers/gpio/Kconfig |   9 
>  drivers/gpio/Makefile|   1 +
>  drivers/gpio/gpio-hlwd.c | 115 
> +++
>  3 files changed, 125 insertions(+)
>  create mode 100644 drivers/gpio/gpio-hlwd.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index d6a8e851ad13..47606dfe06cc 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -229,6 +229,15 @@ config GPIO_GRGPIO
>   Select this to support Aeroflex Gaisler GRGPIO cores from the GRLIB
>   VHDL IP core library.
>
> +config GPIO_HLWD
> +   tristate "Nintendo Wii (Hollywood) GPIO"

> +   depends on OF_GPIO

You may get rid of it if...

> +   select GPIO_GENERIC
> +   help
> + Select this to support the GPIO controller of the Nintendo Wii.
> +
> + If unsure, say N.
> +
>  config GPIO_ICH
> tristate "Intel ICH GPIO"
> depends on PCI && X86
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 4bc24febb889..492f62d0eb59 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -54,6 +54,7 @@ obj-$(CONFIG_GPIO_FTGPIO010)  += gpio-ftgpio010.o
>  obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
>  obj-$(CONFIG_GPIO_GPIO_MM) += gpio-gpio-mm.o
>  obj-$(CONFIG_GPIO_GRGPIO)  += gpio-grgpio.o
> +obj-$(CONFIG_GPIO_HLWD)+= gpio-hlwd.o
>  obj-$(CONFIG_HTC_EGPIO)+= gpio-htc-egpio.o
>  obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
>  obj-$(CONFIG_GPIO_INGENIC) += gpio-ingenic.o
> diff --git a/drivers/gpio/gpio-hlwd.c b/drivers/gpio/gpio-hlwd.c
> new file mode 100644
> index ..a63136a68ba3
> --- /dev/null
> +++ b/drivers/gpio/gpio-hlwd.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright (C) 2008-2009 The GameCube Linux Team
> +// Copyright (C) 2008,2009 Albert Herranz
> +// Copyright (C) 2017-2018 Jonathan Neuschäfer
> +//
> +// Nintendo Wii (Hollywood) GPIO driver
> +
> +#include 
> +#include 
> +#include 
> +#include 

> +#include 
> +#include 

...(and using platform device header I suppose)...

> +#include 
> +
> +/*
> + * Register names and offsets courtesy of WiiBrew:
> + * https://wiibrew.org/wiki/Hardware/Hollywood_GPIOs
> + *
> + * Note that for most registers, there are two versions:
> + * - HW_GPIOB_* Is always accessible by the Broadway PowerPC core, but does
> + *   always give access to all GPIO lines
> + * - HW_GPIO_* Is only accessible by the Broadway PowerPC code if the memory
> + *   firewall (AHBPROT) in the Hollywood chipset has been configured to allow
> + *   such access.
> + *
> + * The ownership of each GPIO line can be configured in the HW_GPIO_OWNER
> + * register: A one bit configures the line for access via the HW_GPIOB_*
> + * registers, a zero bit indicates access via HW_GPIO_*. This driver uses
> + * HW_GPIOB_*.
> + */
> +#define HW_GPIOB_OUT   0x00
> +#define HW_GPIOB_DIR   0x04
> +#define HW_GPIOB_IN0x08
> +#define HW_GPIOB_INTLVL0x0c
> +#define HW_GPIOB_INTFLAG   0x10
> +#define HW_GPIOB_INTMASK   0x14
> +#define HW_GPIOB_INMIR 0x18
> +#define HW_GPIO_ENABLE 0x1c
> +#define HW_GPIO_OUT0x20
> +#define 

Re: [PATCH v3 2/4] gpio: Add GPIO driver for Nintendo Wii

2018-02-09 Thread Segher Boessenkool
On Fri, Feb 09, 2018 at 01:07:29PM +0100, Jonathan Neuschäfer wrote:
> The Nintendo Wii's chipset (called "Hollywood") has a GPIO controller
> that supports a configurable number of pins (up to 32), interrupts, and
> some special mechanisms to share the controller between the system's
> security processor (an ARM926) and the PowerPC CPU. Pin multiplexing is
> not supported.
> 
> This patch adds a basic driver for this GPIO controller. Interrupt
> support will come in a later patch.
> 
> 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.
> 
> Signed-off-by: Jonathan Neuschäfer 
> Cc: Albert Herranz 
> Cc: Segher Boessenkool 

Reviewed-by: Segher Boessenkool 

Looks just fine to me :-)


Segher