Re: [PATCH] gpio: Add driver for PC Engines APU2/APU3 GPIOs

[Florian Eckert]

On 2018-08-03 21:08, Andy Shevchenko wrote:
> > - APU2/APU3 -> front button reset support
> > - APU3 -> SIM switch support
>
> Good.
> Can we see some specification for those platforms?

I think the informations from Christian Lamparter are OK?

<https://www.pcengines.ch/pdf/apu1.pdf>
<https://www.pcengines.ch/schema/apu1c.pdf>
<https://www.pcengines.ch/pdf/apu2.pdf>
<http://pcengines.ch/schema/apu2c.pdf>

> > + * GNU General Public License for more details
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see 
> > <http://www.gnu.org/licenses/>
>
> SPDX, please!

I have already updated my patch in my git to use this identifier.
// SPDX-License-Identifier: GPL-2.0
This was a hint from Linus Walleji

> > + */
>
> > +#include <linux/gpio.h>
> > +#include <linux/gpio_keys.h>
>
> These both looks very strange in here.

On the front of the APU2/APU3 there is a SMD-push-button which is 
connected to
one of the GPIOs. This is used as a reset button for the system 
(reboot/factory-reset).
I am also not sure if this is the right place. But for the first review 
i thought this
will be ok.

The geode, the old board from PC-Engines
added the key gpios to this file
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/platform/geode/alix.c#n47
mybe this is the right place too x86/platfrom/apu/apu.c?

> > +#define APU_FCH_ACPI_MMIO_BASE 0xFED80000
> > +#define APU_FCH_GPIO_BASE      (APU_FCH_ACPI_MMIO_BASE + 0x1500)
>
> Wow! Can we see ACPI tables for these boards? Care to share (via some
> file share service) output of `acpidump -o tables.dat` ?

I have copied this from the leds-apu.c driver which is already upstream.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/leds-apu.c#n43

> > +#define APU_GPIO_BIT_WRITE     22
> > +#define APU_GPIO_BIT_READ      16
> > +#define APU_GPIO_BIT_DIR       23
>
> WR and RD looks shorter,
> And please keep them sorted by value.

Ok will fix this.

> > +#define APU_IOSIZE             sizeof(u32)
>
> This is usual for x86 stuff, no need to have a definition, I think.

Ok will fix this.

> > +static unsigned long apu3_gpio_offset[APU3_NUM_GPIO] = {
> > +       APU_FCH_GPIO_BASE + 89 * APU_IOSIZE, //KEY
> > +       APU_FCH_GPIO_BASE + 90 * APU_IOSIZE, //SIM
> > +};
> > +static void __iomem *apu3_gpio_addr[APU3_NUM_GPIO] = {NULL, NULL};
> > +
>
> > +static int gpio_apu_get_dir (struct gpio_chip *chip, unsigned offset)
>
> Style! We do not use space between func and its parameter list.

OK

> > +{
> > +       u32 val;
> > +
> > +       spin_lock(&apu_gpio->lock);
> > +
>
> > +       val = ~ioread32(apu_gpio->addr[offset]);
>
> This is unusual (I mean ~). Better to leave IO alone and do bits
> manipulations latter on.

OK

> > +       val = (val >> APU_GPIO_BIT_DIR) & 1;
>
> Do you need this under spin lock?

No i donÂt.
Will fix this.
Thanks

> > +               apu_gpio->chip->ngpio = ARRAY_SIZE(apu2_gpio_offset);
> > +               for( i = 0; i < ARRAY_SIZE(apu2_gpio_offset); i++) {
> > +                       apu2_gpio_addr[i] = devm_ioremap(&pdev->dev,
> > +                                       apu_gpio->offset[i], 
> > apu_gpio->iosize);
> > +                       if (!apu2_gpio_addr[i]) {
> > +                               return -ENOMEM;
> > +                       }
> > +               }
> > +       }
>
> The above should be part either as callback or driver_data of DMI 
> entries.

I have copied this from the leds-apu.c driver
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/leds-apu.c#n226

> > +       ret = gpiochip_add(&gpio_apu_chip);
>
> devm_

What do you mean with "devm_"?

> > +       if (ret) {
>
> > +               pr_err("Adding gpiochip failed\n");
>
> dev_err(), but I consider this message completely useless.

Thanks will remove this too.

> > +       }
>
> > +
> > +       register_gpio_keys_polled(-1, 20, ARRAY_SIZE(apu_gpio_keys), 
> > apu_gpio_keys);
> > +
>
> Not part of this driver. Remove.

As described above should this go to a file "apu.c" in the directory 
"apu" under
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/platform
?

> > +       return ret;
> > +}
>
> > +module_init(apu_gpio_init);
> > +module_exit(apu_gpio_exit);
>
> Consider to use module_platform_driver() and accompanying data
> structures and functions.

Ok thanks will update this


Will update my patch with your hints thanks