| Message ID | 77f71974-541e-7e06-d37d-c52b9623ed25@pignat.org |
|---|---|
| State | New |
| Headers | show |
On Thu, Feb 9, 2017 at 11:54 AM, Marc Pignat <marc@pignat.org> wrote: I looped in Bjorn Helgaas to have a look at how this thing probes. Please keep him on CC for future reposts. I'd like William to look at this driver too, he's the authority on EISA type cards and port-mapped GPIO IO. Please keep him on CC too. Please look a bit at drivers/gpio/gpio-it87.c, and gpio-f7188x.c it seems to me that this is a simlar or identical chip to one of these. In that case, you should augment and extend an existing driver instead of writing a new one. But I haven't drilled into the problem and I don't know EISA well enough. > +++ b/drivers/gpio/gpio-nct5104d.c > @@ -0,0 +1,438 @@ > +/* > + * GPIO driver for NCT5104D > + * > + * Author: Tasanakorn Phaipool <tasanakorn@gmail.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/platform_device.h> > +#include <linux/io.h> > +#include <linux/gpio.h> Use #include <linux/gpio/driver.h> only > +#define DRVNAME "gpio-nct5104d" Usually I prefer to just hardcode the string when used. > +/* > + * Super-I/O registers > + */ > +#define SIO_LDSEL 0x07 /* Logical device select */ > +#define SIO_CHIPID 0x20 /* Chaip ID (2 bytes) */ > +#define SIO_GPIO_ENABLE 0x30 /* GPIO enable */ > +#define SIO_GPIO1_MODE 0xE0 /* GPIO1 Mode OpenDrain/Push-Pull */ > +#define SIO_GPIO2_MODE 0xE1 /* GPIO2 Mode OpenDrain/Push-Pull */ > + > +#define SIO_LD_GPIO 0x07 /* GPIO logical device */ > +#define SIO_LD_GPIO_MODE 0x0F /* GPIO mode control device */ > +#define SIO_UNLOCK_KEY 0x87 /* Key to enable Super-I/O */ > +#define SIO_LOCK_KEY 0xAA /* Key to disable Super-I/O */ > + > +#define SIO_NCT5104D_ID 0x1061 /* Chip ID */ > +#define SIO_PCENGINES_APU_NCT5104D_ID 0xc452 /* Chip ID */ > + > +enum chips { nct5104d }; > + > +static const char * const nct5104d_names[] = { > + "nct5104d" > +}; This enum and name array seems a bit overkill? Are you already planning to add support for a bunch of other chips? > +/* > + * GPIO chip. > + */ > + > +static int nct5104d_gpio_direction_in(struct gpio_chip *chip, > + unsigned int offset); > +static int nct5104d_gpio_get(struct gpio_chip *chip, unsigned int offset); > +static int nct5104d_gpio_direction_out(struct gpio_chip *chip, > + unsigned int offset, int value); > +static void nct5104d_gpio_set(struct gpio_chip *chip, unsigned int offset, > + int value); Do you really have to forward-declare all this? I strongly prefer if you move code around so as to avoid it. > +#define NCT5104D_GPIO_BANK(_base, _ngpio, _regbase) \ > + { \ > + .chip = { \ > + .label = DRVNAME, \ > + .owner = THIS_MODULE, \ > + .direction_input = nct5104d_gpio_direction_in, \ > + .get = nct5104d_gpio_get, \ > + .direction_output = nct5104d_gpio_direction_out,\ > + .set = nct5104d_gpio_set, \ > + .base = _base, \ > + .ngpio = _ngpio, \ > + .can_sleep = true, \ > + }, \ > + .regbase = _regbase, \ > + } Please also implement .get_direction(), it is very helpful to have. > +static int nct5104d_gpio_direction_in(struct gpio_chip *chip, > + unsigned int offset) > +{ > + int err; > + struct nct5104d_gpio_bank *bank = > + container_of(chip, struct nct5104d_gpio_bank, chip); > + struct nct5104d_sio *sio = bank->data->sio; > + u8 dir; > + > + err = superio_enter(sio->addr); > + if (err) > + return err; > + superio_select(sio->addr, SIO_LD_GPIO); > + > + dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); > + dir |= (1 << offset); Please use: #include <linux/bitops.h> dir |= BIT(offset); This applies to all sites using this pattern. > + err = gpiochip_add(&bank->chip); Can you use devm_gpiochip_add_data() (you can pass NULL as data) so you do not need the .remove() call below at all? > +err_gpiochip: > + for (i = i - 1; i >= 0; i--) { > + struct nct5104d_gpio_bank *bank = &data->bank[i]; > + > + gpiochip_remove(&bank->chip); Then conversely this needs devm_gpiochip_remove(). > +static int nct5104d_gpio_remove(struct platform_device *pdev) And this could go away. > +static int __init nct5104d_find(int addr, struct nct5104d_sio *sio) > +{ > + int err; > + u16 devid; > + u8 gpio_cfg; > + > + err = superio_enter(addr); > + if (err) > + return err; > + > + err = -ENODEV; > + > + devid = superio_inw(addr, SIO_CHIPID); > + switch (devid) { > + case SIO_NCT5104D_ID: > + case SIO_PCENGINES_APU_NCT5104D_ID: > + sio->type = nct5104d; > + /* enable GPIO0 and GPIO1 */ > + superio_select(addr, SIO_LD_GPIO); > + gpio_cfg = superio_inb(addr, SIO_GPIO_ENABLE); > + gpio_cfg |= 0x03; > + superio_outb(addr, SIO_GPIO_ENABLE, gpio_cfg); > + break; > + default: > + pr_info(DRVNAME ": Unsupported device 0x%04x\n", devid); > + goto err; > + } > + sio->addr = addr; > + err = 0; > + > + pr_info(DRVNAME ": Found %s at %#x chip id 0x%04x\n", > + nct5104d_names[sio->type], > + (unsigned int) addr, > + (int) superio_inw(addr, SIO_CHIPID)); > + > + superio_select(sio->addr, SIO_LD_GPIO_MODE); > + superio_outb(sio->addr, SIO_GPIO1_MODE, 0x0); > + superio_outb(sio->addr, SIO_GPIO2_MODE, 0x0); > + > +err: > + superio_exit(addr); > + return err; > +} > + > +static struct platform_device *nct5104d_gpio_pdev; > + > +static int __init > +nct5104d_gpio_device_add(const struct nct5104d_sio *sio) > +{ > + int err; > + > + nct5104d_gpio_pdev = platform_device_alloc(DRVNAME, -1); > + if (!nct5104d_gpio_pdev) > + pr_err(DRVNAME ": Error platform_device_alloc\n"); > + if (!nct5104d_gpio_pdev) > + return -ENOMEM; > + > + err = platform_device_add_data(nct5104d_gpio_pdev, > + sio, sizeof(*sio)); > + if (err) { > + pr_err(DRVNAME "Platform data allocation failed\n"); > + goto err; > + } > + > + err = platform_device_add(nct5104d_gpio_pdev); > + if (err) { > + pr_err(DRVNAME "Device addition failed\n"); > + goto err; > + } > + pr_info(DRVNAME ": Device added\n"); > + return 0; > + > +err: > + platform_device_put(nct5104d_gpio_pdev); > + > + return err; > +} > + > +/* > + */ > + > +static struct platform_driver nct5104d_gpio_driver = { > + .driver = { > + .owner = THIS_MODULE, > + .name = DRVNAME, > + }, > + .probe = nct5104d_gpio_probe, > + .remove = nct5104d_gpio_remove, > +}; > + > +static int __init nct5104d_gpio_init(void) > +{ > + int err; > + struct nct5104d_sio sio; > + > + if (nct5104d_find(0x2e, &sio) && > + nct5104d_find(0x4e, &sio)) > + return -ENODEV; > + > + err = platform_driver_register(&nct5104d_gpio_driver); > + if (!err) { > + pr_info(DRVNAME ": platform_driver_register\n"); > + err = nct5104d_gpio_device_add(&sio); > + if (err) > + platform_driver_unregister(&nct5104d_gpio_driver); > + } > + > + return err; > +} > +subsys_initcall(nct5104d_gpio_init); > + > +static void __exit nct5104d_gpio_exit(void) > +{ > + platform_device_unregister(nct5104d_gpio_pdev); > + platform_driver_unregister(&nct5104d_gpio_driver); > +} > +module_exit(nct5104d_gpio_exit); I'm not thrilled by this "plug-and-play" that seems very far from autodetection. I need help reviewing this from someone who knows how to deal with this kind of platform drivers on port-mapped I/O. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 22, 2017 at 03:52:05PM +0100, Linus Walleij wrote: >On Thu, Feb 9, 2017 at 11:54 AM, Marc Pignat <marc@pignat.org> wrote: > >I looped in Bjorn Helgaas to have a look at how this thing probes. Please >keep him on CC for future reposts. > >I'd like William to look at this driver too, he's the authority on >EISA type cards >and port-mapped GPIO IO. Please keep him on CC too. > >Please look a bit at drivers/gpio/gpio-it87.c, and gpio-f7188x.c it seems to me >that this is a simlar or identical chip to one of these. In that case, >you should >augment and extend an existing driver instead of writing a new one. >But I haven't >drilled into the problem and I don't know EISA well enough. > Hi Linus, This looks like a Super I/O chip (correct me if I'm mistaken Marc). Super I/O chips typically communicate via the LPC bus (which is software compatible with the ISA bus) hence the ioport operations. The F7188x and IT87xx drivers handle respective Super I/O chips as well. It appears that a lot of code duplication is occurring between these three drivers; this makes sense since the functionality would be similar between these devices. >> +++ b/drivers/gpio/gpio-nct5104d.c >> @@ -0,0 +1,438 @@ >> +/* >> + * GPIO driver for NCT5104D >> + * >> + * Author: Tasanakorn Phaipool <tasanakorn@gmail.com> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> + >> +#include <linux/module.h> >> +#include <linux/init.h> >> +#include <linux/platform_device.h> >> +#include <linux/io.h> >> +#include <linux/gpio.h> > >Use #include <linux/gpio/driver.h> only > >> +#define DRVNAME "gpio-nct5104d" > >Usually I prefer to just hardcode the string when used. > >> +/* >> + * Super-I/O registers >> + */ >> +#define SIO_LDSEL 0x07 /* Logical device select */ >> +#define SIO_CHIPID 0x20 /* Chaip ID (2 bytes) */ >> +#define SIO_GPIO_ENABLE 0x30 /* GPIO enable */ >> +#define SIO_GPIO1_MODE 0xE0 /* GPIO1 Mode OpenDrain/Push-Pull */ >> +#define SIO_GPIO2_MODE 0xE1 /* GPIO2 Mode OpenDrain/Push-Pull */ >> + >> +#define SIO_LD_GPIO 0x07 /* GPIO logical device */ >> +#define SIO_LD_GPIO_MODE 0x0F /* GPIO mode control device */ >> +#define SIO_UNLOCK_KEY 0x87 /* Key to enable Super-I/O */ >> +#define SIO_LOCK_KEY 0xAA /* Key to disable Super-I/O */ >> + >> +#define SIO_NCT5104D_ID 0x1061 /* Chip ID */ >> +#define SIO_PCENGINES_APU_NCT5104D_ID 0xc452 /* Chip ID */ >> + >> +enum chips { nct5104d }; >> + >> +static const char * const nct5104d_names[] = { >> + "nct5104d" >> +}; > >This enum and name array seems a bit overkill? Are you already planning >to add support for a bunch of other chips? > >> +/* >> + * GPIO chip. >> + */ >> + >> +static int nct5104d_gpio_direction_in(struct gpio_chip *chip, >> + unsigned int offset); >> +static int nct5104d_gpio_get(struct gpio_chip *chip, unsigned int offset); >> +static int nct5104d_gpio_direction_out(struct gpio_chip *chip, >> + unsigned int offset, int value); >> +static void nct5104d_gpio_set(struct gpio_chip *chip, unsigned int offset, >> + int value); > >Do you really have to forward-declare all this? > >I strongly prefer if you move code around so as to avoid it. > >> +#define NCT5104D_GPIO_BANK(_base, _ngpio, _regbase) \ >> + { \ >> + .chip = { \ >> + .label = DRVNAME, \ >> + .owner = THIS_MODULE, \ >> + .direction_input = nct5104d_gpio_direction_in, \ >> + .get = nct5104d_gpio_get, \ >> + .direction_output = nct5104d_gpio_direction_out,\ >> + .set = nct5104d_gpio_set, \ >> + .base = _base, \ >> + .ngpio = _ngpio, \ >> + .can_sleep = true, \ >> + }, \ >> + .regbase = _regbase, \ >> + } > >Please also implement .get_direction(), it is very helpful to have. > >> +static int nct5104d_gpio_direction_in(struct gpio_chip *chip, >> + unsigned int offset) >> +{ >> + int err; >> + struct nct5104d_gpio_bank *bank = >> + container_of(chip, struct nct5104d_gpio_bank, chip); >> + struct nct5104d_sio *sio = bank->data->sio; >> + u8 dir; >> + >> + err = superio_enter(sio->addr); >> + if (err) >> + return err; >> + superio_select(sio->addr, SIO_LD_GPIO); >> + >> + dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); >> + dir |= (1 << offset); > >Please use: >#include <linux/bitops.h> > >dir |= BIT(offset); > >This applies to all sites using this pattern. > >> + err = gpiochip_add(&bank->chip); > >Can you use devm_gpiochip_add_data() (you can pass NULL as data) >so you do not need the .remove() call below at all? > >> +err_gpiochip: >> + for (i = i - 1; i >= 0; i--) { >> + struct nct5104d_gpio_bank *bank = &data->bank[i]; >> + >> + gpiochip_remove(&bank->chip); > >Then conversely this needs devm_gpiochip_remove(). > >> +static int nct5104d_gpio_remove(struct platform_device *pdev) > >And this could go away. > >> +static int __init nct5104d_find(int addr, struct nct5104d_sio *sio) >> +{ >> + int err; >> + u16 devid; >> + u8 gpio_cfg; >> + >> + err = superio_enter(addr); >> + if (err) >> + return err; >> + >> + err = -ENODEV; >> + >> + devid = superio_inw(addr, SIO_CHIPID); >> + switch (devid) { >> + case SIO_NCT5104D_ID: >> + case SIO_PCENGINES_APU_NCT5104D_ID: >> + sio->type = nct5104d; >> + /* enable GPIO0 and GPIO1 */ >> + superio_select(addr, SIO_LD_GPIO); >> + gpio_cfg = superio_inb(addr, SIO_GPIO_ENABLE); >> + gpio_cfg |= 0x03; >> + superio_outb(addr, SIO_GPIO_ENABLE, gpio_cfg); >> + break; >> + default: >> + pr_info(DRVNAME ": Unsupported device 0x%04x\n", devid); >> + goto err; >> + } >> + sio->addr = addr; >> + err = 0; >> + >> + pr_info(DRVNAME ": Found %s at %#x chip id 0x%04x\n", >> + nct5104d_names[sio->type], >> + (unsigned int) addr, >> + (int) superio_inw(addr, SIO_CHIPID)); >> + >> + superio_select(sio->addr, SIO_LD_GPIO_MODE); >> + superio_outb(sio->addr, SIO_GPIO1_MODE, 0x0); >> + superio_outb(sio->addr, SIO_GPIO2_MODE, 0x0); >> + >> +err: >> + superio_exit(addr); >> + return err; >> +} >> + >> +static struct platform_device *nct5104d_gpio_pdev; >> + >> +static int __init >> +nct5104d_gpio_device_add(const struct nct5104d_sio *sio) >> +{ >> + int err; >> + >> + nct5104d_gpio_pdev = platform_device_alloc(DRVNAME, -1); >> + if (!nct5104d_gpio_pdev) >> + pr_err(DRVNAME ": Error platform_device_alloc\n"); >> + if (!nct5104d_gpio_pdev) >> + return -ENOMEM; >> + >> + err = platform_device_add_data(nct5104d_gpio_pdev, >> + sio, sizeof(*sio)); >> + if (err) { >> + pr_err(DRVNAME "Platform data allocation failed\n"); >> + goto err; >> + } >> + >> + err = platform_device_add(nct5104d_gpio_pdev); >> + if (err) { >> + pr_err(DRVNAME "Device addition failed\n"); >> + goto err; >> + } >> + pr_info(DRVNAME ": Device added\n"); >> + return 0; >> + >> +err: >> + platform_device_put(nct5104d_gpio_pdev); >> + >> + return err; >> +} >> + >> +/* >> + */ >> + >> +static struct platform_driver nct5104d_gpio_driver = { >> + .driver = { >> + .owner = THIS_MODULE, >> + .name = DRVNAME, >> + }, >> + .probe = nct5104d_gpio_probe, >> + .remove = nct5104d_gpio_remove, >> +}; >> + >> +static int __init nct5104d_gpio_init(void) >> +{ >> + int err; >> + struct nct5104d_sio sio; >> + >> + if (nct5104d_find(0x2e, &sio) && >> + nct5104d_find(0x4e, &sio)) >> + return -ENODEV; >> + >> + err = platform_driver_register(&nct5104d_gpio_driver); >> + if (!err) { >> + pr_info(DRVNAME ": platform_driver_register\n"); >> + err = nct5104d_gpio_device_add(&sio); >> + if (err) >> + platform_driver_unregister(&nct5104d_gpio_driver); >> + } >> + >> + return err; >> +} >> +subsys_initcall(nct5104d_gpio_init); >> + >> +static void __exit nct5104d_gpio_exit(void) >> +{ >> + platform_device_unregister(nct5104d_gpio_pdev); >> + platform_driver_unregister(&nct5104d_gpio_driver); >> +} >> +module_exit(nct5104d_gpio_exit); > Marc, I recommend utilizing the isa_driver since you are interfacing a super I/O chip. This should simplify your __init and __exit code by removing the need for all those platform_driver setup calls you make; instead you would simply call isa_register_driver. Check out the respective __init, __exit, probe, and remove code in drivers/watchdog/ebc-c384_wdt.c file a simple example of how to use the isa_driver calls. If you need more specific help, let me know and I'll go into more detail. >I'm not thrilled by this "plug-and-play" that seems very far from autodetection. > Linus, I believe the best course of action forward would be the development of a "super_io_driver" to handle the detection of these type of devices. Super I/O chips typically have a ioport base address at 0x2E; this is why you see all of these Super I/O chip drivers probing that address. Since device IDs can be found at the standard Super I/O base address, it would be useful to have a super_io_driver structure with an id_table member filled with possible device IDs of devices supported by the respective driver. I imagine that this would enable automatic driver loads and device probes such as we have with the PCI bus driver, where a super_io_driver structure can be populated by the driver author and then passed to a super_io_register_driver call. Furthermore, a Super I/O driver could provide the common read/write operations we see across the existing Super I/O drivers, thus eliminating the code duplication (superio_outb, superio_inw, etc. become standardized). This plan would obviously require more time to get right, so if you decide to merge this particular driver now, I suggest at least using the isa_driver idioms for now to ease the transition to a possible future super_io_driver. William Breathitt Gray >I need help reviewing this from someone who knows how to deal with >this kind of platform drivers on port-mapped I/O. > >Yours, >Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear all, Thank you for your feedback! On 02/22/2017 10:04 PM, William Breathitt Gray wrote: > On Wed, Feb 22, 2017 at 03:52:05PM +0100, Linus Walleij wrote: >> On Thu, Feb 9, 2017 at 11:54 AM, Marc Pignat <marc@pignat.org> wrote: ... >> Please look a bit at drivers/gpio/gpio-it87.c, and gpio-f7188x.c it seems to me >> that this is a simlar or identical chip to one of these. In that case, Looking at gpio-f7188x.c is probably the best feedback. In fact the patch I proposed is just a copy+modifications of an old version of this file. So someone (who has put his name in the author field) has copied the gpio-f7188x.c, changed something like 20 lines (excluding identifiers renaming) and pushed it to github. I took it from github, tested it and passed it through checkpatch.pl and proposed it for inclusion into the mainline kernel. >> you should >> augment and extend an existing driver instead of writing a new one. Sure! >> But I haven't ... > > This plan would obviously require more time to get right, so if you > decide to merge this particular driver now, I suggest at least using the > isa_driver idioms for now to ease the transition to a possible future > super_io_driver. super_io_driver seems a good idea, in the meantime, I will try to merge the nct5104 code into gpio-f7188x.c, without the isa_driver idiom. Best regards Marc -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi all, Here is a v2, with fixes or at least responses to your comments. On 02/22/2017 10:04 PM, William Breathitt Gray wrote: > On Wed, Feb 22, 2017 at 03:52:05PM +0100, Linus Walleij wrote: >> On Thu, Feb 9, 2017 at 11:54 AM, Marc Pignat <marc@pignat.org> wrote: >> ... >> Please look a bit at drivers/gpio/gpio-it87.c, and gpio-f7188x.c it seems to me >> that this is a simlar or identical chip to one of these. In that case, >> you should gpio-f7188x is really similar, but I failed to add nct5104d into it, the registers are not at the same addresses, input/output is inverted and open drain management is different. So I restarted the gpio-nct5104d.c driver using gpio-f7188x.c. ... ... >> >> This enum and name array seems a bit overkill? Are you already planning >> to add support for a bunch of other chips? No, removed. >> >>> +/* >>> + * GPIO chip. >>> + */ >>> + >>> +static int nct5104d_gpio_direction_in(struct gpio_chip *chip, >>> + unsigned int offset); >>> +static int nct5104d_gpio_get(struct gpio_chip *chip, unsigned int offset); >>> +static int nct5104d_gpio_direction_out(struct gpio_chip *chip, >>> + unsigned int offset, int value); >>> +static void nct5104d_gpio_set(struct gpio_chip *chip, unsigned int offset, >>> + int value); >> >> Do you really have to forward-declare all this? No, but was like that in gpio-f7188x.c, fixed. >> >> I strongly prefer if you move code around so as to avoid it. >> >>> +#define NCT5104D_GPIO_BANK(_base, _ngpio, _regbase) \ >>> + { \ >>> + .chip = { \ >>> + .label = DRVNAME, \ >>> + .owner = THIS_MODULE, \ >>> + .direction_input = nct5104d_gpio_direction_in, \ >>> + .get = nct5104d_gpio_get, \ >>> + .direction_output = nct5104d_gpio_direction_out,\ >>> + .set = nct5104d_gpio_set, \ >>> + .base = _base, \ >>> + .ngpio = _ngpio, \ >>> + .can_sleep = true, \ >>> + }, \ >>> + .regbase = _regbase, \ >>> + } >> >> Please also implement .get_direction(), it is very helpful to have. done >> >>> +static int nct5104d_gpio_direction_in(struct gpio_chip *chip, >>> + unsigned int offset) >>> +{ >>> + int err; >>> + struct nct5104d_gpio_bank *bank = >>> + container_of(chip, struct nct5104d_gpio_bank, chip); >>> + struct nct5104d_sio *sio = bank->data->sio; >>> + u8 dir; >>> + >>> + err = superio_enter(sio->addr); >>> + if (err) >>> + return err; >>> + superio_select(sio->addr, SIO_LD_GPIO); >>> + >>> + dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); >>> + dir |= (1 << offset); >> >> Please use: >> #include <linux/bitops.h> >> >> dir |= BIT(offset); >> >> This applies to all sites using this pattern. done >> >>> + err = gpiochip_add(&bank->chip); >> >> Can you use devm_gpiochip_add_data() (you can pass NULL as data) >> so you do not need the .remove() call below at all? done >> >>> +err_gpiochip: >>> + for (i = i - 1; i >= 0; i--) { >>> + struct nct5104d_gpio_bank *bank = &data->bank[i]; >>> + >>> + gpiochip_remove(&bank->chip); >> >> Then conversely this needs devm_gpiochip_remove(). >> >>> +static int nct5104d_gpio_remove(struct platform_device *pdev) >> >> And this could go away. done ... >> > > Marc, > > I recommend utilizing the isa_driver since you are interfacing a super > I/O chip. This should simplify your __init and __exit code by removing > the need for all those platform_driver setup calls you make; instead you > would simply call isa_register_driver. > > Check out the respective __init, __exit, probe, and remove code in > drivers/watchdog/ebc-c384_wdt.c file a simple example of how to use the > isa_driver calls. If you need more specific help, let me know and I'll > go into more detail. > >> I'm not thrilled by this "plug-and-play" that seems very far from autodetection. Sure ISA driver seems a little more clean, but it seems recent kernel are not compiled with CONFIG_ISA_BUS_API. Best regards Marc -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 28, 2017 at 9:14 AM Marc Pignat <marc@pignat.org> wrote: > On 02/22/2017 10:04 PM, William Breathitt Gray wrote: > > On Wed, Feb 22, 2017 at 03:52:05PM +0100, Linus Walleij wrote: > >> On Thu, Feb 9, 2017 at 11:54 AM, Marc Pignat <marc@pignat.org> wrote: > >> I'm not thrilled by this "plug-and-play" that seems very far from autodetection. > > Sure ISA driver seems a little more clean, but it seems recent kernel are > not compiled with CONFIG_ISA_BUS_API. Has this changed these days so we can get this driver in using the proper ISA abstractions? Yours, Linus Walleij
On Thu, Mar 25, 2021 at 09:23:17AM +0100, Linus Walleij wrote: > On Tue, Feb 28, 2017 at 9:14 AM Marc Pignat <marc@pignat.org> wrote: > > On 02/22/2017 10:04 PM, William Breathitt Gray wrote: > > > On Wed, Feb 22, 2017 at 03:52:05PM +0100, Linus Walleij wrote: > > >> On Thu, Feb 9, 2017 at 11:54 AM, Marc Pignat <marc@pignat.org> wrote: > > > >> I'm not thrilled by this "plug-and-play" that seems very far from autodetection. > > > > Sure ISA driver seems a little more clean, but it seems recent kernel are > > not compiled with CONFIG_ISA_BUS_API. > > Has this changed these days so we can get this driver in using > the proper ISA abstractions? > > Yours, > Linus Walleij It's been some years so I don't quite remember how we had things back then, but at least nowadays we typically have a "select ISA_BUS_API" line for the driver's Kconfig block. I suspect this is no longer an issue then because CONFIG_ISA_BUS_API will be compiled along with the drivers that select it. William Breathitt Gray
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index d5d3654..dd5ff27 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -602,6 +602,16 @@ config GPIO_IT87 To compile this driver as a module, choose M here: the module will be called gpio_it87 +config GPIO_NCT5104D + tristate "NCT5104D GPIO support" + help + Say yes here to support GPIO functionality of NCT5104D Super I/O chips. + + This driver is tested with Nuvoton NCT5104D Super I/O chip. + + To compile this driver as a module, choose M here: the module will + be called gpio_nct5104d + config GPIO_SCH tristate "Intel SCH/TunnelCreek/Centerton/Quark X1000 GPIO" depends on (X86 || COMPILE_TEST) && PCI diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index a7676b8..88ea5d8 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -85,6 +85,7 @@ obj-$(CONFIG_GPIO_MSIC) += gpio-msic.o obj-$(CONFIG_GPIO_MVEBU) += gpio-mvebu.o obj-$(CONFIG_GPIO_MXC) += gpio-mxc.o obj-$(CONFIG_GPIO_MXS) += gpio-mxs.o +obj-$(CONFIG_GPIO_NCT5104D) += gpio-nct5104d.o obj-$(CONFIG_GPIO_OCTEON) += gpio-octeon.o obj-$(CONFIG_GPIO_OMAP) += gpio-omap.o obj-$(CONFIG_GPIO_PCA953X) += gpio-pca953x.o diff --git a/drivers/gpio/gpio-nct5104d.c b/drivers/gpio/gpio-nct5104d.c new file mode 100644 index 0000000..127d468 --- /dev/null +++ b/drivers/gpio/gpio-nct5104d.c @@ -0,0 +1,438 @@ +/* + * GPIO driver for NCT5104D + * + * Author: Tasanakorn Phaipool <tasanakorn@gmail.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <linux/module.h> +#include <linux/init.h> +#include <linux/platform_device.h> +#include <linux/io.h> +#include <linux/gpio.h> + +#define DRVNAME "gpio-nct5104d" + +/* + * Super-I/O registers + */ +#define SIO_LDSEL 0x07 /* Logical device select */ +#define SIO_CHIPID 0x20 /* Chaip ID (2 bytes) */ +#define SIO_GPIO_ENABLE 0x30 /* GPIO enable */ +#define SIO_GPIO1_MODE 0xE0 /* GPIO1 Mode OpenDrain/Push-Pull */ +#define SIO_GPIO2_MODE 0xE1 /* GPIO2 Mode OpenDrain/Push-Pull */ + +#define SIO_LD_GPIO 0x07 /* GPIO logical device */ +#define SIO_LD_GPIO_MODE 0x0F /* GPIO mode control device */ +#define SIO_UNLOCK_KEY 0x87 /* Key to enable Super-I/O */ +#define SIO_LOCK_KEY 0xAA /* Key to disable Super-I/O */ + +#define SIO_NCT5104D_ID 0x1061 /* Chip ID */ +#define SIO_PCENGINES_APU_NCT5104D_ID 0xc452 /* Chip ID */ + +enum chips { nct5104d }; + +static const char * const nct5104d_names[] = { + "nct5104d" +}; + +struct nct5104d_sio { + int addr; + enum chips type; +}; + +struct nct5104d_gpio_bank { + struct gpio_chip chip; + unsigned int regbase; + struct nct5104d_gpio_data *data; +}; + +struct nct5104d_gpio_data { + struct nct5104d_sio *sio; + int nr_bank; + struct nct5104d_gpio_bank *bank; +}; + +/* + * Super-I/O functions. + */ + +static inline int superio_inb(int base, int reg) +{ + outb(reg, base); + return inb(base + 1); +} + +static int superio_inw(int base, int reg) +{ + int val; + + outb(reg++, base); + val = inb(base + 1) << 8; + outb(reg, base); + val |= inb(base + 1); + + return val; +} + +static inline void superio_outb(int base, int reg, int val) +{ + outb(reg, base); + outb(val, base + 1); +} + +static inline int superio_enter(int base) +{ + /* Don't step on other drivers' I/O space by accident. */ + if (!request_muxed_region(base, 2, DRVNAME)) { + pr_err(DRVNAME "I/O address 0x%04x already in use\n", base); + return -EBUSY; + } + + /* According to the datasheet the key must be send twice. */ + outb(SIO_UNLOCK_KEY, base); + outb(SIO_UNLOCK_KEY, base); + + return 0; +} + +static inline void superio_select(int base, int ld) +{ + outb(SIO_LDSEL, base); + outb(ld, base + 1); +} + +static inline void superio_exit(int base) +{ + outb(SIO_LOCK_KEY, base); + release_region(base, 2); +} + +/* + * GPIO chip. + */ + +static int nct5104d_gpio_direction_in(struct gpio_chip *chip, + unsigned int offset); +static int nct5104d_gpio_get(struct gpio_chip *chip, unsigned int offset); +static int nct5104d_gpio_direction_out(struct gpio_chip *chip, + unsigned int offset, int value); +static void nct5104d_gpio_set(struct gpio_chip *chip, unsigned int offset, + int value); + +#define NCT5104D_GPIO_BANK(_base, _ngpio, _regbase) \ + { \ + .chip = { \ + .label = DRVNAME, \ + .owner = THIS_MODULE, \ + .direction_input = nct5104d_gpio_direction_in, \ + .get = nct5104d_gpio_get, \ + .direction_output = nct5104d_gpio_direction_out,\ + .set = nct5104d_gpio_set, \ + .base = _base, \ + .ngpio = _ngpio, \ + .can_sleep = true, \ + }, \ + .regbase = _regbase, \ + } + +#define gpio_dir(base) (base + 0) +#define gpio_data(base) (base + 1) + +static struct nct5104d_gpio_bank nct5104d_gpio_bank[] = { + NCT5104D_GPIO_BANK(0, 8, 0xE0), + NCT5104D_GPIO_BANK(10, 8, 0xE4) +}; + +static int nct5104d_gpio_direction_in(struct gpio_chip *chip, + unsigned int offset) +{ + int err; + struct nct5104d_gpio_bank *bank = + container_of(chip, struct nct5104d_gpio_bank, chip); + struct nct5104d_sio *sio = bank->data->sio; + u8 dir; + + err = superio_enter(sio->addr); + if (err) + return err; + superio_select(sio->addr, SIO_LD_GPIO); + + dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); + dir |= (1 << offset); + superio_outb(sio->addr, gpio_dir(bank->regbase), dir); + + superio_exit(sio->addr); + + return 0; +} + +static int nct5104d_gpio_get(struct gpio_chip *chip, unsigned int offset) +{ + int err; + struct nct5104d_gpio_bank *bank = + container_of(chip, struct nct5104d_gpio_bank, chip); + struct nct5104d_sio *sio = bank->data->sio; + u8 data; + + err = superio_enter(sio->addr); + if (err) + return err; + superio_select(sio->addr, SIO_LD_GPIO); + + data = superio_inb(sio->addr, gpio_data(bank->regbase)); + + superio_exit(sio->addr); + + return !!(data & 1 << offset); +} + +static int nct5104d_gpio_direction_out(struct gpio_chip *chip, + unsigned int offset, int value) +{ + int err; + struct nct5104d_gpio_bank *bank = + container_of(chip, struct nct5104d_gpio_bank, chip); + struct nct5104d_sio *sio = bank->data->sio; + u8 dir, data_out; + + err = superio_enter(sio->addr); + if (err) + return err; + superio_select(sio->addr, SIO_LD_GPIO); + + data_out = superio_inb(sio->addr, gpio_data(bank->regbase)); + if (value) + data_out |= (1 << offset); + else + data_out &= ~(1 << offset); + superio_outb(sio->addr, gpio_data(bank->regbase), data_out); + + dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); + dir &= ~(1 << offset); + superio_outb(sio->addr, gpio_dir(bank->regbase), dir); + + superio_exit(sio->addr); + + return 0; +} + +static void nct5104d_gpio_set(struct gpio_chip *chip, unsigned int offset, + int value) +{ + int err; + struct nct5104d_gpio_bank *bank = + container_of(chip, struct nct5104d_gpio_bank, chip); + struct nct5104d_sio *sio = bank->data->sio; + u8 data_out; + + err = superio_enter(sio->addr); + if (err) + return; + superio_select(sio->addr, SIO_LD_GPIO); + + data_out = superio_inb(sio->addr, gpio_data(bank->regbase)); + if (value) + data_out |= (1 << offset); + else + data_out &= ~(1 << offset); + superio_outb(sio->addr, gpio_data(bank->regbase), data_out); + + superio_exit(sio->addr); +} + +/* + * Platform device and driver. + */ + +static int nct5104d_gpio_probe(struct platform_device *pdev) +{ + int err; + int i; + struct nct5104d_sio *sio = pdev->dev.platform_data; + struct nct5104d_gpio_data *data; + + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + switch (sio->type) { + case nct5104d: + data->nr_bank = ARRAY_SIZE(nct5104d_gpio_bank); + data->bank = nct5104d_gpio_bank; + break; + default: + return -ENODEV; + } + data->sio = sio; + + platform_set_drvdata(pdev, data); + + /* For each GPIO bank, register a GPIO chip. */ + for (i = 0; i < data->nr_bank; i++) { + struct nct5104d_gpio_bank *bank = &data->bank[i]; + + bank->chip.parent = &pdev->dev; + bank->data = data; + + err = gpiochip_add(&bank->chip); + if (err) { + dev_err(&pdev->dev, + "Failed to register gpiochip %d: %d\n", + i, err); + goto err_gpiochip; + } + } + + return 0; + +err_gpiochip: + for (i = i - 1; i >= 0; i--) { + struct nct5104d_gpio_bank *bank = &data->bank[i]; + + gpiochip_remove(&bank->chip); + } + + return err; +} + +static int nct5104d_gpio_remove(struct platform_device *pdev) +{ + int i; + struct nct5104d_gpio_data *data = platform_get_drvdata(pdev); + + for (i = 0; i < data->nr_bank; i++) { + struct nct5104d_gpio_bank *bank = &data->bank[i]; + + gpiochip_remove(&bank->chip); + } + + return 0; +} + +static int __init nct5104d_find(int addr, struct nct5104d_sio *sio) +{ + int err; + u16 devid; + u8 gpio_cfg; + + err = superio_enter(addr); + if (err) + return err; + + err = -ENODEV; + + devid = superio_inw(addr, SIO_CHIPID); + switch (devid) { + case SIO_NCT5104D_ID: + case SIO_PCENGINES_APU_NCT5104D_ID: + sio->type = nct5104d; + /* enable GPIO0 and GPIO1 */ + superio_select(addr, SIO_LD_GPIO); + gpio_cfg = superio_inb(addr, SIO_GPIO_ENABLE); + gpio_cfg |= 0x03; + superio_outb(addr, SIO_GPIO_ENABLE, gpio_cfg); + break; + default: + pr_info(DRVNAME ": Unsupported device 0x%04x\n", devid); + goto err; + } + sio->addr = addr; + err = 0; + + pr_info(DRVNAME ": Found %s at %#x chip id 0x%04x\n", + nct5104d_names[sio->type], + (unsigned int) addr, + (int) superio_inw(addr, SIO_CHIPID)); + + superio_select(sio->addr, SIO_LD_GPIO_MODE); + superio_outb(sio->addr, SIO_GPIO1_MODE, 0x0); + superio_outb(sio->addr, SIO_GPIO2_MODE, 0x0); + +err: + superio_exit(addr); + return err; +} + +static struct platform_device *nct5104d_gpio_pdev; + +static int __init +nct5104d_gpio_device_add(const struct nct5104d_sio *sio) +{ + int err; + + nct5104d_gpio_pdev = platform_device_alloc(DRVNAME, -1); + if (!nct5104d_gpio_pdev) + pr_err(DRVNAME ": Error platform_device_alloc\n"); + if (!nct5104d_gpio_pdev) + return -ENOMEM; + + err = platform_device_add_data(nct5104d_gpio_pdev, + sio, sizeof(*sio)); + if (err) { + pr_err(DRVNAME "Platform data allocation failed\n"); + goto err; + } + + err = platform_device_add(nct5104d_gpio_pdev); + if (err) { + pr_err(DRVNAME "Device addition failed\n"); + goto err; + } + pr_info(DRVNAME ": Device added\n"); + return 0; + +err: + platform_device_put(nct5104d_gpio_pdev); + + return err; +} + +/* + */ + +static struct platform_driver nct5104d_gpio_driver = { + .driver = { + .owner = THIS_MODULE, + .name = DRVNAME, + }, + .probe = nct5104d_gpio_probe, + .remove = nct5104d_gpio_remove, +}; + +static int __init nct5104d_gpio_init(void) +{ + int err; + struct nct5104d_sio sio; + + if (nct5104d_find(0x2e, &sio) && + nct5104d_find(0x4e, &sio)) + return -ENODEV; + + err = platform_driver_register(&nct5104d_gpio_driver); + if (!err) { + pr_info(DRVNAME ": platform_driver_register\n"); + err = nct5104d_gpio_device_add(&sio); + if (err) + platform_driver_unregister(&nct5104d_gpio_driver); + } + + return err; +} +subsys_initcall(nct5104d_gpio_init); + +static void __exit nct5104d_gpio_exit(void) +{ + platform_device_unregister(nct5104d_gpio_pdev); + platform_driver_unregister(&nct5104d_gpio_driver); +} +module_exit(nct5104d_gpio_exit); + +MODULE_DESCRIPTION("GPIO driver for Super-I/O chips NCT5104D"); +MODULE_AUTHOR("Tasanakorn Phaipool <tasanakorn@gmail.com>"); +MODULE_LICENSE("GPL"); +