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

From: Andy Shevchenko
Date: Fri Aug 03 2018 - 15:08:31 EST


On Wed, Aug 1, 2018 at 2:12 PM, Florian Eckert <fe@xxxxxxxxxx> wrote:

Thanks for the patch, my comments below.

> Add a new device driver "gpio-apu" which will now handle the GPIOs on
> APU2 and APU3 devices from PC Engines.

> - APU2/APU3 -> front button reset support
> - APU3 -> SIM switch support

Good.
Can we see some specification for those platforms?

> +/* PC Engines APU2/APU3 GPIO device driver
> + *
> + * Copyright (C) 2018 Florian Eckert <fe@xxxxxxxxxx>

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version
> + *
> + * This program is distributed in the hope that it will be useful
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * 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!

> + */

> +#include <linux/gpio.h>
> +#include <linux/gpio_keys.h>

These both looks very strange in here.

> +#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` ?

> +#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.

> +#define APU_IOSIZE sizeof(u32)

This is usual for x86 stuff, no need to have a definition, I think.

> +struct apu_gpio_pdata {
> + struct platform_device *pdev;
> + struct gpio_chip *chip;
> + unsigned long *offset;
> + void __iomem **addr;
> + int iosize; /* for devm_ioremap() */
> + spinlock_t lock;
> +};
> +
> +static struct apu_gpio_pdata *apu_gpio;
> +static struct platform_device *keydev;
> +
> +/* APU2 */
> +static unsigned long apu2_gpio_offset[APU2_NUM_GPIO] = {
> + APU_FCH_GPIO_BASE + 89 * APU_IOSIZE, //KEY
> +};
> +static void __iomem *apu2_gpio_addr[APU2_NUM_GPIO] = {NULL};
> +
> +/* APU3 */
> +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.

> +{
> + 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.

> + val = (val >> APU_GPIO_BIT_DIR) & 1;

Do you need this under spin lock?

> +
> + spin_unlock(&apu_gpio->lock);
> +
> + return val;
> +}


> +static int gpio_apu_get_data (struct gpio_chip *chip, unsigned offset)
> +{
> + u32 val;
> +
> + spin_lock(&apu_gpio->lock);
> +
> + val = ioread32(apu_gpio->addr[offset]);
> + val = (val >> APU_GPIO_BIT_READ) & 1;
> +
> + spin_unlock(&apu_gpio->lock);
> +
> + return val;
> +}

Similar comments as per _get_dir().

> +static struct gpio_keys_button apu_gpio_keys[] = {

> +};
> +
> +static void register_gpio_keys_polled(int id, unsigned poll_interval,
> + unsigned nbuttons,
> + struct gpio_keys_button *buttons)
> +{

> +}

Above must not be here.

> + if (dmi_match(DMI_PRODUCT_NAME, "APU3")) {
> + apu_gpio->offset = apu3_gpio_offset;
> + apu_gpio->addr = apu3_gpio_addr;
> + apu_gpio->iosize = APU_IOSIZE;
> + apu_gpio->chip->ngpio = ARRAY_SIZE(apu3_gpio_offset);
> + for( i = 0; i < ARRAY_SIZE(apu3_gpio_offset); i++) {
> + apu3_gpio_addr[i] = devm_ioremap(&pdev->dev,
> + apu_gpio->offset[i], apu_gpio->iosize);
> + if (!apu3_gpio_addr[i]) {
> + return -ENOMEM;
> + }
> + }
> + } else if (dmi_match(DMI_BOARD_NAME, "APU2") ||
> + dmi_match(DMI_BOARD_NAME, "apu2") ||
> + dmi_match(DMI_BOARD_NAME, "PC Engines apu2")) {
> + apu_gpio->offset = apu2_gpio_offset;
> + apu_gpio->addr = apu2_gpio_addr;
> + apu_gpio->iosize = APU_IOSIZE;
> + 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.

> + ret = gpiochip_add(&gpio_apu_chip);

devm_

> + if (ret) {

> + pr_err("Adding gpiochip failed\n");

dev_err(), but I consider this message completely useless.

> + }

> +
> + register_gpio_keys_polled(-1, 20, ARRAY_SIZE(apu_gpio_keys), apu_gpio_keys);
> +

Not part of this driver. Remove.

> + return ret;
> +}

> +module_init(apu_gpio_init);
> +module_exit(apu_gpio_exit);

Consider to use module_platform_driver() and accompanying data
structures and functions.

--
With Best Regards,
Andy Shevchenko