| Message ID | 1436841787-7734-1-git-send-email-yamada.masahiro@socionext.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 14, 2015 at 4:43 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > This GPIO controller device is used on UniPhier SoCs. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > > Changes in v3: > - Use module_platform_driver() > > Changes in v2: > - Fix typos in the comment block OK why no device tree bindings? Are they in a separate patch? > +/* > + * Unfortunately, the hardware specification adopts weird GPIO pin labeling. > + * The ports are named as > + * PORT00, PORT01, PORT02, ..., PORT07, > + * PORT10, PORT11, PORT12, ..., PORT17, > + * PORT20, PORT21, PORT22, ..., PORT27, > + * ... > + * PORT90, PORT91, PORT92, ..., PORT97, > + * PORT100, PORT101, PORT102, ..., PORT107, > + * ... > + * > + * The PORTs with 8 or 9 in the one's place are missing, i.e. the one's place > + * is octal, while the other places are decimal. If we handle the port numbers > + * as seen in the hardware documents, the GPIO offsets must be non-contiguous. > + * It is possible to have sparse GPIO pins, but not handy for GPIO range > + * mappings, register accessing, etc. > + * > + * To make things simpler (for driver and device tree implementation), this > + * driver takes contiguously-numbered GPIO offsets. GPIO consumers should make > + * sure to convert the PORT number into the one that fits in this driver. > + * The conversion logic is very easy math, for example, > + * PORT15 --> GPIO offset 13 (8 * 1 + 5) > + * PORT123 --> GPIO offset 99 (8 * 12 + 3) > + */ > +#define UNIPHIER_GPIO_PORTS_PER_BANK 8 > +#define UNIPHIER_GPIO_BANK_MASK \ > + ((1UL << (UNIPHIER_GPIO_PORTS_PER_BANK)) - 1) > + > +#define UNIPHIER_GPIO_REG_DATA 0 /* data */ > +#define UNIPHIER_GPIO_REG_DIR 4 /* direction (1:in, 0:out) */ > + > +struct uniphier_gpio_priv { > + struct of_mm_gpio_chip mmchip; > + spinlock_t lock; > +}; > + > +static unsigned uniphier_gpio_bank_to_reg(unsigned bank, unsigned reg_type) > +{ > + unsigned reg; > + > + reg = (bank + 1) * 8 + reg_type; > + > + /* > + * Unfortunately, there is a register hole at offset 0x90-0x9f. > + * Add 0x10 when crossing the hole. > + */ > + if (reg >= 0x90) > + reg += 0x10; > + > + return reg; > +} > + > +static void uniphier_gpio_bank_write(struct gpio_chip *chip, > + unsigned bank, unsigned reg_type, > + unsigned mask, unsigned value) > +{ > + struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip); > + struct uniphier_gpio_priv *priv; > + unsigned long flags; > + unsigned reg; > + u32 tmp; > + > + if (!mask) > + return; > + > + priv = container_of(mmchip, struct uniphier_gpio_priv, mmchip); > + > + reg = uniphier_gpio_bank_to_reg(bank, reg_type); > + > + /* > + * Note > + * regmap_update_bits() should not be used here. > + * > + * The DATA registers return the current readback of pins, not the > + * previously written data when they are configured as "input". > + * The DATA registers must be overwritten even if the data you are > + * going to write is the same as what readl() has returned. > + * > + * regmap_update_bits() does not write back if the data is not changed. > + */ Why is this mentioned when the driver doesn't even use regmap? Development artifact? > +static int uniphier_gpio_get_direction(struct gpio_chip *chip, unsigned offset) > +{ > + return uniphier_gpio_offset_read(chip, UNIPHIER_GPIO_REG_DIR, offset) ? > + GPIOF_DIR_IN : GPIOF_DIR_OUT; Just use return !!uniphier_gpio_offset_read(chip, UNIPHIER_GPIO_REG_DIR, offset); > +static int uniphier_gpio_get(struct gpio_chip *chip, unsigned offset) > +{ > + return uniphier_gpio_offset_read(chip, offset, UNIPHIER_GPIO_REG_DATA); return !!uniphier_gpio_offset_read(chip, offset, UNIPHIER_GPIO_REG_DATA); > +static void uniphier_gpio_set_multiple(struct gpio_chip *chip, > + unsigned long *mask, > + unsigned long *bits) > +{ > + unsigned bank, shift, bank_mask, bank_bits; > + int i; > + > + for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_PORTS_PER_BANK) { > + bank = i / UNIPHIER_GPIO_PORTS_PER_BANK; > + shift = i % BITS_PER_LONG; > + bank_mask = (mask[BIT_WORD(i)] >> shift) & > + UNIPHIER_GPIO_BANK_MASK; > + bank_bits = bits[BIT_WORD(i)] >> shift; > + > + uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_REG_DATA, > + bank_mask, bank_bits); > + } This looks like a piece of algorithm that we could make generic like in a static function in drivers/gpio/gpiolib.h or so, that it may be shared with other drivers. Do you see some clear way to break out the core of this? Or is it as generic as I think? > +static int uniphier_gpio_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct uniphier_gpio_priv *priv; > + u32 ngpio; > + int ret; > + > + ret = of_property_read_u32(dev->of_node, "ngpio", &ngpio); > + if (ret) { > + dev_err(dev, "failed to get ngpio property\n"); > + return ret; > + } This needs to be documented, plus I don't see why it's needed. The driver for this very specific hardware should already know how many GPIOs there are in this hardware, it should not come from the device tree. ngpio is typically for the case where the hardware has 32 bits for GPIO pins, but only the first 11 are actually used in the hardware. > +static const struct of_device_id uniphier_gpio_match[] = { > + { .compatible = "socionext,uniphier-gpio" }, > + { /* sentinel */ } > +}; Needs a binding document. Apart from this it looks nice! Yours, Linus Walleij
Hi Linus, 2015-07-15 23:15 GMT+09:00 Linus Walleij <linus.walleij@linaro.org>: > On Tue, Jul 14, 2015 at 4:43 AM, Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: > >> This GPIO controller device is used on UniPhier SoCs. >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >> --- >> >> Changes in v3: >> - Use module_platform_driver() >> >> Changes in v2: >> - Fix typos in the comment block > > OK why no device tree bindings? Are they in a separate patch? Sorry, I was planning to do it later. OK. I will come back with Documentation/devicetree/bindings/gpio/uniphier-gpio.txt in binding info in it. >> +/* >> + * Unfortunately, the hardware specification adopts weird GPIO pin labeling. >> + * The ports are named as >> + * PORT00, PORT01, PORT02, ..., PORT07, >> + * PORT10, PORT11, PORT12, ..., PORT17, >> + * PORT20, PORT21, PORT22, ..., PORT27, >> + * ... >> + * PORT90, PORT91, PORT92, ..., PORT97, >> + * PORT100, PORT101, PORT102, ..., PORT107, >> + * ... >> + * >> + * The PORTs with 8 or 9 in the one's place are missing, i.e. the one's place >> + * is octal, while the other places are decimal. If we handle the port numbers >> + * as seen in the hardware documents, the GPIO offsets must be non-contiguous. >> + * It is possible to have sparse GPIO pins, but not handy for GPIO range >> + * mappings, register accessing, etc. >> + * >> + * To make things simpler (for driver and device tree implementation), this >> + * driver takes contiguously-numbered GPIO offsets. GPIO consumers should make >> + * sure to convert the PORT number into the one that fits in this driver. >> + * The conversion logic is very easy math, for example, >> + * PORT15 --> GPIO offset 13 (8 * 1 + 5) >> + * PORT123 --> GPIO offset 99 (8 * 12 + 3) >> + */ >> +#define UNIPHIER_GPIO_PORTS_PER_BANK 8 >> +#define UNIPHIER_GPIO_BANK_MASK \ >> + ((1UL << (UNIPHIER_GPIO_PORTS_PER_BANK)) - 1) > > > >> + >> +#define UNIPHIER_GPIO_REG_DATA 0 /* data */ >> +#define UNIPHIER_GPIO_REG_DIR 4 /* direction (1:in, 0:out) */ >> + >> +struct uniphier_gpio_priv { >> + struct of_mm_gpio_chip mmchip; >> + spinlock_t lock; >> +}; >> + >> +static unsigned uniphier_gpio_bank_to_reg(unsigned bank, unsigned reg_type) >> +{ >> + unsigned reg; >> + >> + reg = (bank + 1) * 8 + reg_type; >> + >> + /* >> + * Unfortunately, there is a register hole at offset 0x90-0x9f. >> + * Add 0x10 when crossing the hole. >> + */ >> + if (reg >= 0x90) >> + reg += 0x10; >> + >> + return reg; >> +} >> + >> +static void uniphier_gpio_bank_write(struct gpio_chip *chip, >> + unsigned bank, unsigned reg_type, >> + unsigned mask, unsigned value) >> +{ >> + struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip); >> + struct uniphier_gpio_priv *priv; >> + unsigned long flags; >> + unsigned reg; >> + u32 tmp; >> + >> + if (!mask) >> + return; >> + >> + priv = container_of(mmchip, struct uniphier_gpio_priv, mmchip); >> + >> + reg = uniphier_gpio_bank_to_reg(bank, reg_type); >> + >> + /* >> + * Note >> + * regmap_update_bits() should not be used here. >> + * >> + * The DATA registers return the current readback of pins, not the >> + * previously written data when they are configured as "input". >> + * The DATA registers must be overwritten even if the data you are >> + * going to write is the same as what readl() has returned. >> + * >> + * regmap_update_bits() does not write back if the data is not changed. >> + */ > > Why is this mentioned when the driver doesn't even use regmap? > Development artifact? At first, I thought regmap_update_bits() might be useful, but it tuned out a bad idea. Anyway, it did not use regmap in this driver, so this comment sounds a bit weird. I will delete it in v4. >> +static int uniphier_gpio_get_direction(struct gpio_chip *chip, unsigned offset) >> +{ >> + return uniphier_gpio_offset_read(chip, UNIPHIER_GPIO_REG_DIR, offset) ? >> + GPIOF_DIR_IN : GPIOF_DIR_OUT; > > Just use > return !!uniphier_gpio_offset_read(chip, UNIPHIER_GPIO_REG_DIR, offset); OK, will fix. >> +static int uniphier_gpio_get(struct gpio_chip *chip, unsigned offset) >> +{ >> + return uniphier_gpio_offset_read(chip, offset, UNIPHIER_GPIO_REG_DATA); > > return !!uniphier_gpio_offset_read(chip, offset, UNIPHIER_GPIO_REG_DATA); Likewise. >> +static void uniphier_gpio_set_multiple(struct gpio_chip *chip, >> + unsigned long *mask, >> + unsigned long *bits) >> +{ >> + unsigned bank, shift, bank_mask, bank_bits; >> + int i; >> + >> + for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_PORTS_PER_BANK) { >> + bank = i / UNIPHIER_GPIO_PORTS_PER_BANK; >> + shift = i % BITS_PER_LONG; >> + bank_mask = (mask[BIT_WORD(i)] >> shift) & >> + UNIPHIER_GPIO_BANK_MASK; >> + bank_bits = bits[BIT_WORD(i)] >> shift; >> + >> + uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_REG_DATA, >> + bank_mask, bank_bits); >> + } > > This looks like a piece of algorithm that we could make generic like in a > static function in drivers/gpio/gpiolib.h or so, that it may be shared with > other drivers. Do you see some clear way to break out the core of this? > Or is it as generic as I think? I assume this comment has no intention to block my patch. We already have a generic handling in gpio_chip_set_multiple() in case .set_multiple() is not supported. Given that not many drivers support .set_multiple, I am not sure if we should add another generalized helper func. >> +static int uniphier_gpio_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct uniphier_gpio_priv *priv; >> + u32 ngpio; >> + int ret; >> + >> + ret = of_property_read_u32(dev->of_node, "ngpio", &ngpio); >> + if (ret) { >> + dev_err(dev, "failed to get ngpio property\n"); >> + return ret; >> + } > > This needs to be documented, plus I don't see why it's needed. > The driver for this very specific hardware should already know > how many GPIOs there are in this hardware, it should not come > from the device tree. I want to use this driver on various SoCs, but the number of GPIO pins varies by SoC. ngpio == 248 for some SoCs, and ngpio == 136 for some, etc. I will document it. > ngpio is typically for the case where the hardware has 32 bits > for GPIO pins, but only the first 11 are actually used in the hardware. > >> +static const struct of_device_id uniphier_gpio_match[] = { >> + { .compatible = "socionext,uniphier-gpio" }, >> + { /* sentinel */ } >> +}; > > Needs a binding document. > > Apart from this it looks nice! > > Yours, > Linus Walleij > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Jul 16, 2015 at 5:44 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2015-07-15 23:15 GMT+09:00 Linus Walleij <linus.walleij@linaro.org>: >>> + for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_PORTS_PER_BANK) { >>> + bank = i / UNIPHIER_GPIO_PORTS_PER_BANK; >>> + shift = i % BITS_PER_LONG; >>> + bank_mask = (mask[BIT_WORD(i)] >> shift) & >>> + UNIPHIER_GPIO_BANK_MASK; >>> + bank_bits = bits[BIT_WORD(i)] >> shift; >>> + >>> + uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_REG_DATA, >>> + bank_mask, bank_bits); >>> + } >> >> This looks like a piece of algorithm that we could make generic like in a >> static function in drivers/gpio/gpiolib.h or so, that it may be shared with >> other drivers. Do you see some clear way to break out the core of this? >> Or is it as generic as I think? > > I assume this comment has no intention to block my patch. Nah. Just thinking the code looks neat. >>> + ret = of_property_read_u32(dev->of_node, "ngpio", &ngpio); >>> + if (ret) { >>> + dev_err(dev, "failed to get ngpio property\n"); >>> + return ret; >>> + } >> >> This needs to be documented, plus I don't see why it's needed. >> The driver for this very specific hardware should already know >> how many GPIOs there are in this hardware, it should not come >> from the device tree. > > I want to use this driver on various SoCs, but > the number of GPIO pins varies by SoC. > > ngpio == 248 for some SoCs, > and ngpio == 136 for some, etc. That is the wrong way to handle different SoC. That should be handled by different compatible strings, and then you select the number of GPIOs for the version corresponding to that compatibe string. >>> +static const struct of_device_id uniphier_gpio_match[] = { >>> + { .compatible = "socionext,uniphier-gpio" }, >>> + { /* sentinel */ } >>> +}; i.e. you should use the .data field of of_device_id to carry variant-specific information. Yours, Linus Walleij
Hi Linus, 2015-07-16 16:07 GMT+09:00 Linus Walleij <linus.walleij@linaro.org>: > On Thu, Jul 16, 2015 at 5:44 AM, Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: >> 2015-07-15 23:15 GMT+09:00 Linus Walleij <linus.walleij@linaro.org>: > >>>> + for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_PORTS_PER_BANK) { >>>> + bank = i / UNIPHIER_GPIO_PORTS_PER_BANK; >>>> + shift = i % BITS_PER_LONG; >>>> + bank_mask = (mask[BIT_WORD(i)] >> shift) & >>>> + UNIPHIER_GPIO_BANK_MASK; >>>> + bank_bits = bits[BIT_WORD(i)] >> shift; >>>> + >>>> + uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_REG_DATA, >>>> + bank_mask, bank_bits); >>>> + } >>> >>> This looks like a piece of algorithm that we could make generic like in a >>> static function in drivers/gpio/gpiolib.h or so, that it may be shared with >>> other drivers. Do you see some clear way to break out the core of this? >>> Or is it as generic as I think? >> >> I assume this comment has no intention to block my patch. > > Nah. Just thinking the code looks neat. > >>>> + ret = of_property_read_u32(dev->of_node, "ngpio", &ngpio); >>>> + if (ret) { >>>> + dev_err(dev, "failed to get ngpio property\n"); >>>> + return ret; >>>> + } >>> >>> This needs to be documented, plus I don't see why it's needed. >>> The driver for this very specific hardware should already know >>> how many GPIOs there are in this hardware, it should not come >>> from the device tree. >> >> I want to use this driver on various SoCs, but >> the number of GPIO pins varies by SoC. >> >> ngpio == 248 for some SoCs, >> and ngpio == 136 for some, etc. > > That is the wrong way to handle different SoC. That should be handled > by different compatible strings, and then you select the number of GPIOs > for the version corresponding to that compatibe string. > >>>> +static const struct of_device_id uniphier_gpio_match[] = { >>>> + { .compatible = "socionext,uniphier-gpio" }, >>>> + { /* sentinel */ } >>>> +}; > > i.e. you should use the .data field of of_device_id to carry variant-specific > information. Currently, I want to use this driver on 7 SoCs PH1-sLD3: ngpio == 136 PH1-LD4 : ngpio == 136 PH1-Pro4: ngpio == 248 PH1-sLD8: ngpio == 136 PH1-Pro5: ngpio == 248 ProXstream2: ngpio == 232 PH1-LD6b: ngpio == 232 So, should I describe the OF match table like this? static const struct of_device_id uniphier_gpio_match[] = { { .compatible = "socionext,ph1-sld3-gpio" .data = (void *)136 }, { .compatible = "socionext,ph1-ld4-gpio" .data = (void *)136 }, { .compatible = "socionext,ph1-pro4-gpio" .data = (void *)248 }, { .compatible = "socionext,ph1-sld8-gpio" .data = (void *)136 }, { .compatible = "socionext,ph1-pro5-gpio" .data = (void *)248 }, { .compatible = "socionext,proxstream2-gpio", .data = (void *)232 }, { .compatible = "socionext,ph1-ld6b-gpio", .data = (void *)232 }, { /* sentinel */ } }; One disadvantage for this way is that I need to touch the driver file every time I add a new SoC support. If I could support "ngpio" property, I would only have to add a device tree for a new SoC.
On Thu, Jul 16, 2015 at 9:35 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2015-07-16 16:07 GMT+09:00 Linus Walleij <linus.walleij@linaro.org>: >>> ngpio == 248 for some SoCs, >>> and ngpio == 136 for some, etc. >> >> That is the wrong way to handle different SoC. That should be handled >> by different compatible strings, and then you select the number of GPIOs >> for the version corresponding to that compatibe string. >> >>>>> +static const struct of_device_id uniphier_gpio_match[] = { >>>>> + { .compatible = "socionext,uniphier-gpio" }, >>>>> + { /* sentinel */ } >>>>> +}; >> >> i.e. you should use the .data field of of_device_id to carry variant-specific >> information. > > > Currently, I want to use this driver on 7 SoCs > > PH1-sLD3: ngpio == 136 > PH1-LD4 : ngpio == 136 > PH1-Pro4: ngpio == 248 > PH1-sLD8: ngpio == 136 > PH1-Pro5: ngpio == 248 > ProXstream2: ngpio == 232 > PH1-LD6b: ngpio == 232 > > So, should I describe the OF match table like this? > > static const struct of_device_id uniphier_gpio_match[] = { > { .compatible = "socionext,ph1-sld3-gpio" .data = (void *)136 }, > { .compatible = "socionext,ph1-ld4-gpio" .data = (void *)136 }, > { .compatible = "socionext,ph1-pro4-gpio" .data = (void *)248 }, > { .compatible = "socionext,ph1-sld8-gpio" .data = (void *)136 }, > { .compatible = "socionext,ph1-pro5-gpio" .data = (void *)248 }, > { .compatible = "socionext,proxstream2-gpio", .data = (void *)232 }, > { .compatible = "socionext,ph1-ld6b-gpio", .data = (void *)232 }, > { /* sentinel */ } > }; Yes. > One disadvantage for this way is that > I need to touch the driver file every time I add a new SoC support. That is appropriate since it is a new hardware. This is the same as the fact that we touch the kernel to add new USB IDs and PCI IDs every time a new hardware comes out for x86, we should know what hardware we are toying with. Since you seem to have a pin controller in parallel anyways I see it as natural to do this at the same time as you do that anyways. Yours, Linus Walleij
Hi Linus, 2015-07-16 16:42 GMT+09:00 Linus Walleij <linus.walleij@linaro.org>: > On Thu, Jul 16, 2015 at 9:35 AM, Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: >> 2015-07-16 16:07 GMT+09:00 Linus Walleij <linus.walleij@linaro.org>: > >>>> ngpio == 248 for some SoCs, >>>> and ngpio == 136 for some, etc. >>> >>> That is the wrong way to handle different SoC. That should be handled >>> by different compatible strings, and then you select the number of GPIOs >>> for the version corresponding to that compatibe string. >>> >>>>>> +static const struct of_device_id uniphier_gpio_match[] = { >>>>>> + { .compatible = "socionext,uniphier-gpio" }, >>>>>> + { /* sentinel */ } >>>>>> +}; >>> >>> i.e. you should use the .data field of of_device_id to carry variant-specific >>> information. >> >> >> Currently, I want to use this driver on 7 SoCs >> >> PH1-sLD3: ngpio == 136 >> PH1-LD4 : ngpio == 136 >> PH1-Pro4: ngpio == 248 >> PH1-sLD8: ngpio == 136 >> PH1-Pro5: ngpio == 248 >> ProXstream2: ngpio == 232 >> PH1-LD6b: ngpio == 232 >> >> So, should I describe the OF match table like this? >> >> static const struct of_device_id uniphier_gpio_match[] = { >> { .compatible = "socionext,ph1-sld3-gpio" .data = (void *)136 }, >> { .compatible = "socionext,ph1-ld4-gpio" .data = (void *)136 }, >> { .compatible = "socionext,ph1-pro4-gpio" .data = (void *)248 }, >> { .compatible = "socionext,ph1-sld8-gpio" .data = (void *)136 }, >> { .compatible = "socionext,ph1-pro5-gpio" .data = (void *)248 }, >> { .compatible = "socionext,proxstream2-gpio", .data = (void *)232 }, >> { .compatible = "socionext,ph1-ld6b-gpio", .data = (void *)232 }, >> { /* sentinel */ } >> }; > > Yes. > >> One disadvantage for this way is that >> I need to touch the driver file every time I add a new SoC support. > > That is appropriate since it is a new hardware. This is the same as > the fact that we touch the kernel to add new USB IDs and PCI IDs > every time a new hardware comes out for x86, we should know > what hardware we are toying with. > > Since you seem to have a pin controller in parallel anyways I see > it as natural to do this at the same time as you do that anyways. > OK, I will do it in v4.
Hi Linus, 2015-07-16 16:48 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>: > Hi Linus, > > OK, I will do it in v4. I has been away from my GPIO driver upstreaming for a long time for some reason. I sent v3 ten months ago. http://patchwork.ozlabs.org/patch/494860/ In the meantime, there have been various updates in the GPIO frame-work. So, I'd like to know the recommended driver coding style based on 4.7-rc1. Diving into the git-log so far, I came up the following list: [1] The "dev" member of gpiochip was renamed to "parent" [2] The most recommended register function is now devm_gpiochip_add_data() [3] Pass the driver private data to the 3rd argument of devm_gpiochip_add_data(). [4] Do not use container_of() to covert from gpiochip to driver private data. Instead, gpiochip_get_data() should be used. (I used of_mm_gpio_chip in v3, but I needed to use container_of() to convert from gpiochip to of_mm_gpio_chip. I am wondering if you are happy with it or not.) [5] gpiochip.owner is being deprecated. So, platform drivers need not set "gpiochip.owner = THIS_MODULE". Correct? Is there anything else missing? Any advice is appreciated.
On Fri, May 20, 2016 at 6:30 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > So, I'd like to know the recommended driver coding style > based on 4.7-rc1. Sure, best is to look at other recently added drivers I guess. > Diving into the git-log so far, I came up the following list: > > [1] The "dev" member of gpiochip was renamed to "parent" Yes. > [2] The most recommended register function is now > devm_gpiochip_add_data() Yes, saves some code. > [3] Pass the driver private data to the > 3rd argument of devm_gpiochip_add_data(). Yes. > [4] Do not use container_of() to covert > from gpiochip to driver private data. > Instead, gpiochip_get_data() should be used. Yes. > (I used of_mm_gpio_chip in v3, > but I needed to use container_of() to > convert from gpiochip to of_mm_gpio_chip. > I am wondering if you are happy with it or not.) We will see when we look at the code ;) I guess it is OK. The alternative is to use gpio-generic instead, see commit 42178e2a1e42b480ada954750f248b53d3fb5940 "drivers/gpio: Switch gpio-mpc8xxx to use gpio-generic" for an example of that approach. > [5] gpiochip.owner is being deprecated. > So, platform drivers need not set > "gpiochip.owner = THIS_MODULE". Yes. > Correct? Yes. > Is there anything else missing? I advice to add a .get_direction() callback if possible and also .set_single_ended() if the GPIO chip supports open drain / open source in hardware. Yours, Linus Walleij
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 4c9fa58..37cb4f2 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -462,6 +462,12 @@ config GPIO_TZ1090_PDC help Say yes here to support Toumaz Xenif TZ1090 PDC GPIOs. +config GPIO_UNIPHIER + tristate "UniPhier GPIO" + depends on ARCH_UNIPHIER && OF_GPIO + help + Say yes here to support UniPhier GPIOs. + config GPIO_VF610 def_bool y depends on ARCH_MXC && SOC_VF610 diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index f82cd67..3538b0c 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -102,6 +102,7 @@ obj-$(CONFIG_GPIO_TWL6040) += gpio-twl6040.o obj-$(CONFIG_GPIO_TZ1090) += gpio-tz1090.o obj-$(CONFIG_GPIO_TZ1090_PDC) += gpio-tz1090-pdc.o obj-$(CONFIG_GPIO_UCB1400) += gpio-ucb1400.o +obj-$(CONFIG_GPIO_UNIPHIER) += gpio-uniphier.o obj-$(CONFIG_GPIO_VF610) += gpio-vf610.o obj-$(CONFIG_GPIO_VIPERBOARD) += gpio-viperboard.o obj-$(CONFIG_GPIO_VR41XX) += gpio-vr41xx.o diff --git a/drivers/gpio/gpio-uniphier.c b/drivers/gpio/gpio-uniphier.c new file mode 100644 index 0000000..1afacaa --- /dev/null +++ b/drivers/gpio/gpio-uniphier.c @@ -0,0 +1,262 @@ +/* + * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro@socionext.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. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/gpio/driver.h> +#include <linux/of_gpio.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> + +/* + * Unfortunately, the hardware specification adopts weird GPIO pin labeling. + * The ports are named as + * PORT00, PORT01, PORT02, ..., PORT07, + * PORT10, PORT11, PORT12, ..., PORT17, + * PORT20, PORT21, PORT22, ..., PORT27, + * ... + * PORT90, PORT91, PORT92, ..., PORT97, + * PORT100, PORT101, PORT102, ..., PORT107, + * ... + * + * The PORTs with 8 or 9 in the one's place are missing, i.e. the one's place + * is octal, while the other places are decimal. If we handle the port numbers + * as seen in the hardware documents, the GPIO offsets must be non-contiguous. + * It is possible to have sparse GPIO pins, but not handy for GPIO range + * mappings, register accessing, etc. + * + * To make things simpler (for driver and device tree implementation), this + * driver takes contiguously-numbered GPIO offsets. GPIO consumers should make + * sure to convert the PORT number into the one that fits in this driver. + * The conversion logic is very easy math, for example, + * PORT15 --> GPIO offset 13 (8 * 1 + 5) + * PORT123 --> GPIO offset 99 (8 * 12 + 3) + */ +#define UNIPHIER_GPIO_PORTS_PER_BANK 8 +#define UNIPHIER_GPIO_BANK_MASK \ + ((1UL << (UNIPHIER_GPIO_PORTS_PER_BANK)) - 1) + +#define UNIPHIER_GPIO_REG_DATA 0 /* data */ +#define UNIPHIER_GPIO_REG_DIR 4 /* direction (1:in, 0:out) */ + +struct uniphier_gpio_priv { + struct of_mm_gpio_chip mmchip; + spinlock_t lock; +}; + +static unsigned uniphier_gpio_bank_to_reg(unsigned bank, unsigned reg_type) +{ + unsigned reg; + + reg = (bank + 1) * 8 + reg_type; + + /* + * Unfortunately, there is a register hole at offset 0x90-0x9f. + * Add 0x10 when crossing the hole. + */ + if (reg >= 0x90) + reg += 0x10; + + return reg; +} + +static void uniphier_gpio_bank_write(struct gpio_chip *chip, + unsigned bank, unsigned reg_type, + unsigned mask, unsigned value) +{ + struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip); + struct uniphier_gpio_priv *priv; + unsigned long flags; + unsigned reg; + u32 tmp; + + if (!mask) + return; + + priv = container_of(mmchip, struct uniphier_gpio_priv, mmchip); + + reg = uniphier_gpio_bank_to_reg(bank, reg_type); + + /* + * Note + * regmap_update_bits() should not be used here. + * + * The DATA registers return the current readback of pins, not the + * previously written data when they are configured as "input". + * The DATA registers must be overwritten even if the data you are + * going to write is the same as what readl() has returned. + * + * regmap_update_bits() does not write back if the data is not changed. + */ + spin_lock_irqsave(&priv->lock, flags); + tmp = readl(mmchip->regs + reg); + tmp &= ~mask; + tmp |= mask & value; + writel(tmp, mmchip->regs + reg); + spin_unlock_irqrestore(&priv->lock, flags); +} + +static void uniphier_gpio_offset_write(struct gpio_chip *chip, unsigned offset, + unsigned reg_type, int value) +{ + unsigned bank = offset / UNIPHIER_GPIO_PORTS_PER_BANK; + unsigned bit = offset % UNIPHIER_GPIO_PORTS_PER_BANK; + + uniphier_gpio_bank_write(chip, bank, reg_type, BIT(bit), value << bit); +} + +static int uniphier_gpio_offset_read(struct gpio_chip *chip, unsigned offset, + unsigned reg_type) +{ + struct of_mm_gpio_chip *mmchip = to_of_mm_gpio_chip(chip); + unsigned bank = offset / UNIPHIER_GPIO_PORTS_PER_BANK; + unsigned bit = offset % UNIPHIER_GPIO_PORTS_PER_BANK; + unsigned reg; + + reg = uniphier_gpio_bank_to_reg(bank, reg_type); + + return readl(mmchip->regs + reg) & BIT(bit) ? 1 : 0; +} + +static int uniphier_gpio_request(struct gpio_chip *chip, unsigned offset) +{ + return pinctrl_request_gpio(chip->base + offset); +} + +static void uniphier_gpio_free(struct gpio_chip *chip, unsigned offset) +{ + pinctrl_free_gpio(chip->base + offset); +} + +static int uniphier_gpio_get_direction(struct gpio_chip *chip, unsigned offset) +{ + return uniphier_gpio_offset_read(chip, UNIPHIER_GPIO_REG_DIR, offset) ? + GPIOF_DIR_IN : GPIOF_DIR_OUT; +} + +static int uniphier_gpio_direction_input(struct gpio_chip *chip, + unsigned offset) +{ + uniphier_gpio_offset_write(chip, offset, UNIPHIER_GPIO_REG_DIR, 1); + + return 0; +} + +static int uniphier_gpio_direction_output(struct gpio_chip *chip, + unsigned offset, int value) +{ + uniphier_gpio_offset_write(chip, offset, UNIPHIER_GPIO_REG_DATA, value); + uniphier_gpio_offset_write(chip, offset, UNIPHIER_GPIO_REG_DIR, 0); + + return 0; +} + +static int uniphier_gpio_get(struct gpio_chip *chip, unsigned offset) +{ + return uniphier_gpio_offset_read(chip, offset, UNIPHIER_GPIO_REG_DATA); +} + +static void uniphier_gpio_set(struct gpio_chip *chip, + unsigned offset, int value) +{ + uniphier_gpio_offset_write(chip, offset, UNIPHIER_GPIO_REG_DATA, value); +} + +static void uniphier_gpio_set_multiple(struct gpio_chip *chip, + unsigned long *mask, + unsigned long *bits) +{ + unsigned bank, shift, bank_mask, bank_bits; + int i; + + for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_PORTS_PER_BANK) { + bank = i / UNIPHIER_GPIO_PORTS_PER_BANK; + shift = i % BITS_PER_LONG; + bank_mask = (mask[BIT_WORD(i)] >> shift) & + UNIPHIER_GPIO_BANK_MASK; + bank_bits = bits[BIT_WORD(i)] >> shift; + + uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_REG_DATA, + bank_mask, bank_bits); + } +} + +static int uniphier_gpio_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct uniphier_gpio_priv *priv; + u32 ngpio; + int ret; + + ret = of_property_read_u32(dev->of_node, "ngpio", &ngpio); + if (ret) { + dev_err(dev, "failed to get ngpio property\n"); + return ret; + } + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + spin_lock_init(&priv->lock); + + priv->mmchip.gc.dev = dev; + priv->mmchip.gc.owner = THIS_MODULE; + priv->mmchip.gc.request = uniphier_gpio_request; + priv->mmchip.gc.free = uniphier_gpio_free; + priv->mmchip.gc.get_direction = uniphier_gpio_get_direction; + priv->mmchip.gc.direction_input = uniphier_gpio_direction_input; + priv->mmchip.gc.direction_output = uniphier_gpio_direction_output; + priv->mmchip.gc.get = uniphier_gpio_get; + priv->mmchip.gc.set = uniphier_gpio_set; + priv->mmchip.gc.set_multiple = uniphier_gpio_set_multiple; + priv->mmchip.gc.ngpio = ngpio; + + ret = of_mm_gpiochip_add(dev->of_node, &priv->mmchip); + if (ret) { + dev_err(dev, "failed to add memory mapped gpiochip\n"); + return ret; + } + + platform_set_drvdata(pdev, priv); + + return 0; +} + +static int uniphier_gpio_remove(struct platform_device *pdev) +{ + struct uniphier_gpio_priv *priv = platform_get_drvdata(pdev); + + of_mm_gpiochip_remove(&priv->mmchip); + + return 0; +} + +static const struct of_device_id uniphier_gpio_match[] = { + { .compatible = "socionext,uniphier-gpio" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, uniphier_gpio_match); + +static struct platform_driver uniphier_gpio_driver = { + .probe = uniphier_gpio_probe, + .remove = uniphier_gpio_remove, + .driver = { + .name = "uniphier-gpio", + .of_match_table = uniphier_gpio_match, + }, +}; +module_platform_driver(uniphier_gpio_driver); + +MODULE_AUTHOR("Masahiro Yamada <yamada.masahiro@socionext.com>"); +MODULE_DESCRIPTION("UniPhier GPIO driver"); +MODULE_LICENSE("GPL");
This GPIO controller device is used on UniPhier SoCs. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- Changes in v3: - Use module_platform_driver() Changes in v2: - Fix typos in the comment block drivers/gpio/Kconfig | 6 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-uniphier.c | 262 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 269 insertions(+) create mode 100644 drivers/gpio/gpio-uniphier.c