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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Thu, 2018-08-09 at 15:03 +0200, Florian Eckert wrote:
> Add a new device driver "gpio-apu" which will handle the GPIOs on APU2
> and APU3 devices from PC Engines.
> 
> APU2 (https://pcengines.ch/schema/apu2c.pdf page 7):
> - G32 is "button_reset" connected to the smd-button on the frontpanel
> - G50 is "mpcie2_reset" connected to mPCIe2 reset line
> - G51 is "mpcie3_reset" connected to mPCIe3 reset line
> 
> APU3 (https://pcengines.ch/schema/apu3c.pdf page 7):
> - G32 is "button_reset" connected to the smd-button on the frontpanel
> - G50 is "mpcie2_reset" connected to mPCIe2 reset line
> - G51 is "mpcie3_reset" connected to mPCIe3 reset line
> - G33 is "simswap" connected to SIM switch IC to swap the SIM between
> mPCIe2
>   and mPCIe3 slot

Thanks for an update, my comments below.

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

Comment style issue.

> +#define APU_FCH_ACPI_MMIO_BASE 0xFED80000
> +#define APU_FCH_GPIO_BASE      (APU_FCH_ACPI_MMIO_BASE + 0x1500)

I think it's still unanswered.

Why do you have this hardcoded? What the issue with your firmware?
Why it's not provided via ACPI?

> +struct apu_gpio_pdata {
> +	struct platform_device *pdev;
> +	struct gpio_chip *chip;
> +	unsigned long *offset;
> +	void __iomem **addr;

> +	int iosize; /* for devm_ioremap() */

Why?

> +	spinlock_t lock;
> +};
> +

> +static struct apu_gpio_pdata *apu_gpio;

Why this is global variable?

> +static void __iomem *apu2_gpio_addr[APU2_NUM_GPIO] = {NULL, NULL,
> NULL};

Ditto.

> +static void __iomem *apu3_gpio_addr[APU3_NUM_GPIO] = {NULL, NULL,
> NULL, NULL};

Ditto.

> +static int gpio_apu_get_dir(struct gpio_chip *chip, unsigned offset)
> +{
> +	u32 val;
> +
> +	spin_lock(&apu_gpio->lock);
> +
> +	val = ~ioread32(apu_gpio->addr[offset]);

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

It seems you ignored my previous comments.

Why this is under spin lock?

So, I stopped here and waiting for next one which answers above and
previous comments.

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html