lkml.org 
[lkml]   [2018]   [Dec]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 1/2] gpio: Add driver for PC Engines APU boards


> /*
> * Multi-line comments
> * have this style
> */

fixed


>> +#include <linux/kernel.h>
>
> kbuild bot complains for absence of
>
> #include <linux/mod_devicetable.h>
>
> here.
>

fixed

>> +static int gpio_apu_get_dir(struct gpio_chip *chip, unsigned int
>> offset)
>> +{
>> + u32 val;
>> + struct apu_gpio_pdata *apu_gpio = gpiochip_get_data(chip);
>> +
>> + spin_lock(&apu_gpio->lock);
>> +
>
>> + val = ~ioread32(apu_gpio->addr[offset]);
>
> There is no need to do ~ under spin lock.
>

fixed

>> +
>> + spin_unlock(&apu_gpio->lock);
>> +
>> + return !!(val & BIT(APU_GPIO_BIT_DIR));
>> +}
>
>> + if (dmi_check_system(apu3_gpio_dmi_table)) {
>
> (1)
>
>> + apu_gpio->addr = devm_kzalloc(&pdev->dev,
>> + sizeof(apu3_gpio_offset),
>> + GFP_KERNEL);
>
>> +
>
> No need to have this blank line. Same for the other cases.
>

fixed

>> + if (!apu_gpio->addr)
>> + return -ENOMEM;
>
>> + } else if (dmi_check_system(apu2_gpio_dmi_table)) {
>
> (2)
>
> I think I have already told about (1) and (2). You may create two
> callbacks and utilize .callback member in DMI table.
>

Done but I do not seen any advantage. I used the following driver as
basis.
https://github.com/torvalds/linux/blob/master/drivers/leds/leds-clevo-mail.c

>> + }
>
>> +static int __init apu_gpio_init(void)
>> +{
>
>> + if (!(dmi_check_system(apu2_gpio_dmi_table)) &&
>> + !(dmi_check_system(apu3_gpio_dmi_table))) {
>> + pr_err("No PC Engines board detected\n");
>> + return -ENODEV;
>> + }
>
> I don't think we need this.
>

see below


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

>
> After removing unneeded checks why not to simple use
> module_platform_driver()
> ?

I have fixed all the above hints from you now but using
"module_platform_driver" is no option.
I played around with them but the driver does not find any device. So I
need the init function
to add a platform device. Only if I do this way driver and device will
find and match. And I
see the gpios under /sys/class/gpio. So I think I need this?
I have not find any driver who has the same problems
I used the following drivers as my basis:
https://github.com/torvalds/linux/blob/master/drivers/leds/leds-apu.c
https://github.com/torvalds/linux/blob/master/drivers/leds/leds-clevo-mail.c
They all use dmi and need init/exit for platform device register and
unregister



\
 
 \ /
  Last update: 2018-12-04 11:18    [W:0.075 / U:0.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site