lkml.org 
[lkml]   [2018]   [Aug]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] gpio: Add driver for PC Engines APU2/APU3 GPIOs
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

\
 
 \ /
  Last update: 2018-08-07 13:48    [W:0.166 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site