From: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> To: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> Cc: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>, Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>, David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Wim Van Sebroeck <wim-IQzOog9fTRqzQB+pC5nmwQ@public.gmane.org>, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>, Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>, Debjit Ghosh <dghosh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>, Georgi Vlaev <gvlaev-3r7Miqu9kMnR7s880joybQ@public.gmane.org>, Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>, JawaharBalaji Thirumalaisamy <jawaharb-3r7Miqu9kMnR7s880joybQ@public.gmane.org>, Rajat Jain <rajatjain-3r7Miqu9kMnR7s880joybQ@public.gmane.org>, "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-gpio@vg> Subject: Re: [PATCH 07/10] gpio: ptxpmb-cpld: Add support for PTXPMB CPLD's GPIO Date: Fri, 21 Oct 2016 00:24:42 +0200 [thread overview] Message-ID: <CACRpkdaYVgwBS_QR+S4CafP0jFrn8jw-Ewc_m8LDXUK5sMMwTw@mail.gmail.com> (raw) In-Reply-To: <1475853451-22121-8-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> On Fri, Oct 7, 2016 at 5:17 PM, Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote: > From: Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org> > > Support the GPIO block which is located in PTXPMB CPLDs > on relevant Juniper platforms. > > Signed-off-by: Georgi Vlaev <gvlaev-3r7Miqu9kMnR7s880joybQ@public.gmane.org> > Signed-off-by: Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org> > Signed-off-by: Rajat Jain <rajatjain-3r7Miqu9kMnR7s880joybQ@public.gmane.org> > [Ported from Juniper kernel] > Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> Nice with preserved credits. > +config GPIO_PTXPMB_CPLD > + tristate "PTXPMB CPLD GPIO" > + depends on MFD_JUNIPER_CPLD > + default y if MFD_JUNIPER_CPLD Can't you do something like default MFD_JUNIPER_CPLD so it will become a module if the MFD driver is a module and compiled-in if the MFD driver is compiled in? Please check if that works. > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/pci.h> > +#include <linux/gpio.h> Just: #include <linux/gpio/driver.h> Should work. > +#include <linux/errno.h> > +#include <linux/of_device.h> > +#include <linux/of_platform.h> > +#include <linux/of_gpio.h> Strange that a PCI driver need so much OF stuff. Really? But OK, do you really use all these includes? > +#include <linux/io.h> > +#include <linux/module.h> > + > +#include <linux/mfd/ptxpmb_cpld.h> Is this include deserving a separate whitespace in front of it? > +static u8 *ptxpmb_cpld_gpio_get_addr(struct pmb_boot_cpld *cpld, > + unsigned int nr) > +{ > + if (nr < 8) /* 0..7: reset */ > + return &cpld->reset; > + else if (nr < 16) /* 8..15: control */ > + return &cpld->control; > + else if (nr < 24) /* 16..23: gpio1 */ > + return &cpld->gpio_1; > + else if (nr < 32) /* 24..31: gpio2 */ > + return &cpld->gpio_2; > + else if (nr < 40) /* 32..39: gp_reset1 */ > + return &cpld->gp_reset1; > + return &cpld->thermal_status; /* 40..41: thermal status */ > +} Reset, control, gp_reset and *especially* thermal status does not seem to be GPIO at all. Rather these seem to be special purpose lines rather than general purpose. Can you explain why the GPIO driver should even poke at them? It seems other subdrivers should use these registers... > +static void ptxpmb_cpld_gpio_set(struct gpio_chip *gpio, unsigned int nr, > + int val) > +{ > + u32 reg; > + u8 bit = 1 << (nr & 7); Use this: #include <linux/bitops.h> Then inline BIT() instead of making a local variable like this. See below... > + struct ptxpmb_cpld_gpio *chip = > + container_of(gpio, struct ptxpmb_cpld_gpio, gpio); Use: struct ptxpmb_cpld_gpio *cpldg = gpiochip_get_data(gpio); Please don't name it "chip" it is confusing with the gpio chip proper. > + u8 *addr = ptxpmb_cpld_gpio_get_addr(chip->base, nr); > + > + mutex_lock(&chip->lock); > + reg = ioread8(addr); > + if (val) > + reg |= bit; > + else > + reg &= ~bit; So instead: if (val) reg |= BIT(nr); else reg &= ~BIT(nr); > +static int ptxpmb_cpld_gpio_get(struct gpio_chip *gpio, unsigned int nr) > +{ > + struct ptxpmb_cpld_gpio *chip = container_of(gpio, > + struct ptxpmb_cpld_gpio, > + gpio); Same comment as before. > + u8 *addr = ptxpmb_cpld_gpio_get_addr(chip->base, nr); > + u8 bit = 1 << (nr & 7); > + > + return !!(ioread8(addr) & bit); Same comment on using BIT() > +static int ptxpmb_cpld_gpio_direction_output(struct gpio_chip *gpio, > + unsigned int nr, int val) > +{ > + return 0; > +} > + > +static int ptxpmb_cpld_gpio_direction_input(struct gpio_chip *gpio, > + unsigned int nr) > +{ > + return 0; > +} Oh yeah? Can you explain how this works electronically? If these lines are open drain you should also implement .set_single_ended(). > +static void ptxpmb_cpld_gpio_setup(struct ptxpmb_cpld_gpio *chip) Again rename from chip. > +static int ptxpmb_cpld_gpio_probe(struct platform_device *pdev) > +{ > + int ret; > + struct ptxpmb_cpld_gpio *chip; Rename from chip. > + struct resource *res; > + struct device *dev = &pdev->dev; > + > + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); > + if (chip == NULL) > + return -ENOMEM; > + > + chip->dev = dev; > + platform_set_drvdata(pdev, chip); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENODEV; > + > + chip->base = devm_ioremap_nocache(dev, res->start, resource_size(res)); > + if (!chip->base) > + return -ENOMEM; > + > + mutex_init(&chip->lock); > + ptxpmb_cpld_gpio_setup(chip); > + ret = gpiochip_add(&chip->gpio); Use devm_gpiochip_add_data() so you can use gpiochip_get_data() in the callbacks and get rid of container_of(). > + if (ret) { > + dev_err(dev, "CPLD gpio: Failed to register GPIO\n"); > + return ret; > + } > + return 0; > +} > + > +static int ptxpmb_cpld_gpio_remove(struct platform_device *pdev) > +{ > + struct ptxpmb_cpld_gpio *chip = platform_get_drvdata(pdev); > + > + gpiochip_remove(&chip->gpio); > + > + return 0; > +} If you use devm_gpiochip_add_data() I don't think this function is even needed. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: Linus Walleij <linus.walleij@linaro.org> To: Pantelis Antoniou <pantelis.antoniou@konsulko.com> Cc: Lee Jones <lee.jones@linaro.org>, Rob Herring <robh+dt@kernel.org>, Alexandre Courbot <gnurou@gmail.com>, Mark Rutland <mark.rutland@arm.com>, Frank Rowand <frowand.list@gmail.com>, Wolfram Sang <wsa@the-dreams.de>, David Woodhouse <dwmw2@infradead.org>, Brian Norris <computersforpeace@gmail.com>, Wim Van Sebroeck <wim@iguana.be>, Guenter Roeck <linux@roeck-us.net>, Peter Rosin <peda@axentia.se>, Debjit Ghosh <dghosh@juniper.net>, Georgi Vlaev <gvlaev@juniper.net>, Guenter Roeck <groeck@juniper.net>, JawaharBalaji Thirumalaisamy <jawaharb@juniper.net>, Rajat Jain <rajatjain@juniper.net>, "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>, "linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>, "linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>, linux-watchdog@vger.kernel.org Subject: Re: [PATCH 07/10] gpio: ptxpmb-cpld: Add support for PTXPMB CPLD's GPIO Date: Fri, 21 Oct 2016 00:24:42 +0200 [thread overview] Message-ID: <CACRpkdaYVgwBS_QR+S4CafP0jFrn8jw-Ewc_m8LDXUK5sMMwTw@mail.gmail.com> (raw) In-Reply-To: <1475853451-22121-8-git-send-email-pantelis.antoniou@konsulko.com> On Fri, Oct 7, 2016 at 5:17 PM, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote: > From: Guenter Roeck <groeck@juniper.net> > > Support the GPIO block which is located in PTXPMB CPLDs > on relevant Juniper platforms. > > Signed-off-by: Georgi Vlaev <gvlaev@juniper.net> > Signed-off-by: Guenter Roeck <groeck@juniper.net> > Signed-off-by: Rajat Jain <rajatjain@juniper.net> > [Ported from Juniper kernel] > Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com> Nice with preserved credits. > +config GPIO_PTXPMB_CPLD > + tristate "PTXPMB CPLD GPIO" > + depends on MFD_JUNIPER_CPLD > + default y if MFD_JUNIPER_CPLD Can't you do something like default MFD_JUNIPER_CPLD so it will become a module if the MFD driver is a module and compiled-in if the MFD driver is compiled in? Please check if that works. > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/pci.h> > +#include <linux/gpio.h> Just: #include <linux/gpio/driver.h> Should work. > +#include <linux/errno.h> > +#include <linux/of_device.h> > +#include <linux/of_platform.h> > +#include <linux/of_gpio.h> Strange that a PCI driver need so much OF stuff. Really? But OK, do you really use all these includes? > +#include <linux/io.h> > +#include <linux/module.h> > + > +#include <linux/mfd/ptxpmb_cpld.h> Is this include deserving a separate whitespace in front of it? > +static u8 *ptxpmb_cpld_gpio_get_addr(struct pmb_boot_cpld *cpld, > + unsigned int nr) > +{ > + if (nr < 8) /* 0..7: reset */ > + return &cpld->reset; > + else if (nr < 16) /* 8..15: control */ > + return &cpld->control; > + else if (nr < 24) /* 16..23: gpio1 */ > + return &cpld->gpio_1; > + else if (nr < 32) /* 24..31: gpio2 */ > + return &cpld->gpio_2; > + else if (nr < 40) /* 32..39: gp_reset1 */ > + return &cpld->gp_reset1; > + return &cpld->thermal_status; /* 40..41: thermal status */ > +} Reset, control, gp_reset and *especially* thermal status does not seem to be GPIO at all. Rather these seem to be special purpose lines rather than general purpose. Can you explain why the GPIO driver should even poke at them? It seems other subdrivers should use these registers... > +static void ptxpmb_cpld_gpio_set(struct gpio_chip *gpio, unsigned int nr, > + int val) > +{ > + u32 reg; > + u8 bit = 1 << (nr & 7); Use this: #include <linux/bitops.h> Then inline BIT() instead of making a local variable like this. See below... > + struct ptxpmb_cpld_gpio *chip = > + container_of(gpio, struct ptxpmb_cpld_gpio, gpio); Use: struct ptxpmb_cpld_gpio *cpldg = gpiochip_get_data(gpio); Please don't name it "chip" it is confusing with the gpio chip proper. > + u8 *addr = ptxpmb_cpld_gpio_get_addr(chip->base, nr); > + > + mutex_lock(&chip->lock); > + reg = ioread8(addr); > + if (val) > + reg |= bit; > + else > + reg &= ~bit; So instead: if (val) reg |= BIT(nr); else reg &= ~BIT(nr); > +static int ptxpmb_cpld_gpio_get(struct gpio_chip *gpio, unsigned int nr) > +{ > + struct ptxpmb_cpld_gpio *chip = container_of(gpio, > + struct ptxpmb_cpld_gpio, > + gpio); Same comment as before. > + u8 *addr = ptxpmb_cpld_gpio_get_addr(chip->base, nr); > + u8 bit = 1 << (nr & 7); > + > + return !!(ioread8(addr) & bit); Same comment on using BIT() > +static int ptxpmb_cpld_gpio_direction_output(struct gpio_chip *gpio, > + unsigned int nr, int val) > +{ > + return 0; > +} > + > +static int ptxpmb_cpld_gpio_direction_input(struct gpio_chip *gpio, > + unsigned int nr) > +{ > + return 0; > +} Oh yeah? Can you explain how this works electronically? If these lines are open drain you should also implement .set_single_ended(). > +static void ptxpmb_cpld_gpio_setup(struct ptxpmb_cpld_gpio *chip) Again rename from chip. > +static int ptxpmb_cpld_gpio_probe(struct platform_device *pdev) > +{ > + int ret; > + struct ptxpmb_cpld_gpio *chip; Rename from chip. > + struct resource *res; > + struct device *dev = &pdev->dev; > + > + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); > + if (chip == NULL) > + return -ENOMEM; > + > + chip->dev = dev; > + platform_set_drvdata(pdev, chip); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENODEV; > + > + chip->base = devm_ioremap_nocache(dev, res->start, resource_size(res)); > + if (!chip->base) > + return -ENOMEM; > + > + mutex_init(&chip->lock); > + ptxpmb_cpld_gpio_setup(chip); > + ret = gpiochip_add(&chip->gpio); Use devm_gpiochip_add_data() so you can use gpiochip_get_data() in the callbacks and get rid of container_of(). > + if (ret) { > + dev_err(dev, "CPLD gpio: Failed to register GPIO\n"); > + return ret; > + } > + return 0; > +} > + > +static int ptxpmb_cpld_gpio_remove(struct platform_device *pdev) > +{ > + struct ptxpmb_cpld_gpio *chip = platform_get_drvdata(pdev); > + > + gpiochip_remove(&chip->gpio); > + > + return 0; > +} If you use devm_gpiochip_add_data() I don't think this function is even needed. Yours, Linus Walleij
next prev parent reply other threads:[~2016-10-20 22:24 UTC|newest] Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-10-07 15:17 [PATCH 00/10] Introduce Juniper PTXPMB CPLD driver Pantelis Antoniou 2016-10-07 15:17 ` Pantelis Antoniou 2016-10-07 15:17 ` [PATCH 01/10] mfd: Juniper PTXPMB CPLD Multi-function core driver Pantelis Antoniou 2016-10-07 15:17 ` Pantelis Antoniou 2016-10-26 13:50 ` Lee Jones 2016-10-26 13:50 ` Lee Jones 2016-10-26 13:50 ` Lee Jones 2016-10-07 15:17 ` [PATCH 02/10] mfd: ptxpmb-cpld: Add documentation for PTXPMB CPLD Pantelis Antoniou 2016-10-07 15:17 ` Pantelis Antoniou 2016-10-10 17:38 ` Rob Herring 2016-10-10 17:38 ` Rob Herring 2016-10-07 15:17 ` [PATCH 03/10] watchdog: Add support for PTXPMB CPLD watchdog Pantelis Antoniou 2016-10-07 15:17 ` Pantelis Antoniou 2016-10-07 15:17 ` [PATCH 04/10] watchdog: ptxpmb-wdt: Add ptxpmb-wdt device tree bindings Pantelis Antoniou 2016-10-07 15:17 ` Pantelis Antoniou [not found] ` <1475853451-22121-5-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> 2016-10-10 17:41 ` Rob Herring 2016-10-10 17:41 ` Rob Herring 2016-10-07 15:17 ` [PATCH 07/10] gpio: ptxpmb-cpld: Add support for PTXPMB CPLD's GPIO Pantelis Antoniou 2016-10-07 15:17 ` Pantelis Antoniou [not found] ` <1475853451-22121-8-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> 2016-10-20 22:24 ` Linus Walleij [this message] 2016-10-20 22:24 ` Linus Walleij 2016-10-07 15:17 ` [PATCH 08/10] gpio: ptxpmb-cpld: Document bindings of PTXPMB's CPLD GPIO Pantelis Antoniou 2016-10-07 15:17 ` Pantelis Antoniou 2016-10-10 18:08 ` Rob Herring 2016-10-10 18:08 ` Rob Herring 2016-10-07 15:17 ` [PATCH 09/10] mtd: devices: Add driver for memory mapped NVRAM on FPC Pantelis Antoniou 2016-10-07 15:17 ` Pantelis Antoniou 2016-10-07 15:17 ` [PATCH 10/10] mtd: ngpmb_nvram: Add bindings for Juniper's ngpmb NVRAM Pantelis Antoniou 2016-10-07 15:17 ` Pantelis Antoniou [not found] ` <1475853451-22121-11-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> 2016-10-10 18:18 ` Rob Herring 2016-10-10 18:18 ` Rob Herring [not found] ` <1475853451-22121-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> 2016-10-07 15:17 ` [PATCH 05/10] i2c/muxes: Juniper's PTXPMB CPLD I2C multiplexer Pantelis Antoniou 2016-10-07 15:17 ` Pantelis Antoniou 2016-10-07 15:17 ` [PATCH 06/10] i2c: i2c-mux-ptxpmb-cpld: Add device tree bindings Pantelis Antoniou 2016-10-07 15:17 ` Pantelis Antoniou [not found] ` <1475853451-22121-7-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> 2016-10-10 17:45 ` Rob Herring 2016-10-10 17:45 ` Rob Herring 2016-10-11 7:36 ` Peter Rosin 2016-10-11 7:36 ` Peter Rosin 2016-10-07 23:39 ` [PATCH 00/10] Introduce Juniper PTXPMB CPLD driver Frank Rowand 2016-10-07 23:39 ` Frank Rowand [not found] ` <57F8324C.2040804-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-10-08 2:17 ` Frank Rowand 2016-10-08 2:17 ` Frank Rowand 2016-10-20 13:42 ` Linus Walleij 2016-10-20 13:42 ` Linus Walleij 2016-10-20 13:43 ` Pantelis Antoniou 2016-10-20 13:43 ` Pantelis Antoniou
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CACRpkdaYVgwBS_QR+S4CafP0jFrn8jw-Ewc_m8LDXUK5sMMwTw@mail.gmail.com \ --to=linus.walleij-qsej5fyqhm4dnm+yrofe0a@public.gmane.org \ --cc=computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \ --cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=dghosh-3r7Miqu9kMnR7s880joybQ@public.gmane.org \ --cc=dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org \ --cc=frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \ --cc=gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \ --cc=groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org \ --cc=gvlaev-3r7Miqu9kMnR7s880joybQ@public.gmane.org \ --cc=jawaharb-3r7Miqu9kMnR7s880joybQ@public.gmane.org \ --cc=lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \ --cc=linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org \ --cc=linux-gpio@vg \ --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=mark.rutland-5wv7dgnIgG8@public.gmane.org \ --cc=pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org \ --cc=peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org \ --cc=rajatjain-3r7Miqu9kMnR7s880joybQ@public.gmane.org \ --cc=robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \ --cc=wim-IQzOog9fTRqzQB+pC5nmwQ@public.gmane.org \ --cc=wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.