Messages in this thread | | | Date | Tue, 04 Dec 2018 11:17:37 +0100 | From | Florian Eckert <> | Subject | Re: [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
| |