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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Fri, Aug 24, 2018 at 08:49:27AM +0200, Florian Eckert wrote:
> Hello Andy,
> 
> thanks for reviewing my driver and sorry for the delayed answer.
> 
> > > +#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?
> 
> As mentioned in my previews mail I have copied this from the leds-apu driver
> under /drivers/leds/leds-apu.c
> 
> I have to look into what mean to get this via ACPI.
> 
> > > +static struct apu_gpio_pdata *apu_gpio;
> > 
> > Why this is global variable?
> 
> I have added the static identifier so it should only be seen in this modul.
> Or what do you mean?
> 
> > 
> > > +static void __iomem *apu2_gpio_addr[APU2_NUM_GPIO] = {NULL, NULL,
> > > NULL};
> > 
> > Ditto.
> 
> Do you mean that this should get declared dynamic witch devm_kzalloc?

That is my question. Usually for such drivers we don't need a global variables
but rather some chunks of memory from a heap.

> > > +static void __iomem *apu3_gpio_addr[APU3_NUM_GPIO] = {NULL, NULL,
> > > NULL, NULL};
> > 
> > Ditto.
> 
> Do you mean that this should get declared dynamic witch devm_kzalloc?
> 
> I have also moved the reset button definition from /arch/x86/platform as
> suggested to /drivers/platform/x86/amd-apu-rbtn.c

Good!

-- 
With Best Regards,
Andy Shevchenko