Add a helper for registering BCM63XX pin controllers.
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
Signed-off-by: Jonas Gorski <[email protected]>
---
v4: drop IRQ selects from Kconfig and add missing of_node_put()
v3: add new patch with shared code
drivers/pinctrl/bcm/Kconfig | 7 ++
drivers/pinctrl/bcm/Makefile | 1 +
drivers/pinctrl/bcm/pinctrl-bcm63xx.c | 114 ++++++++++++++++++++++++++
drivers/pinctrl/bcm/pinctrl-bcm63xx.h | 46 +++++++++++
4 files changed, 168 insertions(+)
create mode 100644 drivers/pinctrl/bcm/pinctrl-bcm63xx.c
create mode 100644 drivers/pinctrl/bcm/pinctrl-bcm63xx.h
diff --git a/drivers/pinctrl/bcm/Kconfig b/drivers/pinctrl/bcm/Kconfig
index 0ed14de0134c..882f19bdc243 100644
--- a/drivers/pinctrl/bcm/Kconfig
+++ b/drivers/pinctrl/bcm/Kconfig
@@ -29,6 +29,13 @@ config PINCTRL_BCM2835
help
Say Y here to enable the Broadcom BCM2835 GPIO driver.
+config PINCTRL_BCM63XX
+ bool
+ select GENERIC_PINCONF
+ select GPIO_REGMAP
+ select PINCONF
+ select PINMUX
+
config PINCTRL_IPROC_GPIO
bool "Broadcom iProc GPIO (with PINCONF) driver"
depends on OF_GPIO && (ARCH_BCM_IPROC || COMPILE_TEST)
diff --git a/drivers/pinctrl/bcm/Makefile b/drivers/pinctrl/bcm/Makefile
index 79d5e49fdd9a..0e3cf9b15c65 100644
--- a/drivers/pinctrl/bcm/Makefile
+++ b/drivers/pinctrl/bcm/Makefile
@@ -3,6 +3,7 @@
obj-$(CONFIG_PINCTRL_BCM281XX) += pinctrl-bcm281xx.o
obj-$(CONFIG_PINCTRL_BCM2835) += pinctrl-bcm2835.o
+obj-$(CONFIG_PINCTRL_BCM63XX) += pinctrl-bcm63xx.o
obj-$(CONFIG_PINCTRL_IPROC_GPIO) += pinctrl-iproc-gpio.o
obj-$(CONFIG_PINCTRL_CYGNUS_MUX) += pinctrl-cygnus-mux.o
obj-$(CONFIG_PINCTRL_NS) += pinctrl-ns.o
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm63xx.c b/drivers/pinctrl/bcm/pinctrl-bcm63xx.c
new file mode 100644
index 000000000000..9a2d68c1ad26
--- /dev/null
+++ b/drivers/pinctrl/bcm/pinctrl-bcm63xx.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Driver for BCM63xx GPIO unit (pinctrl + GPIO)
+ *
+ * Copyright (C) 2021 Álvaro Fernández Rojas <[email protected]>
+ * Copyright (C) 2016 Jonas Gorski <[email protected]>
+ */
+
+#include <linux/gpio/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include "pinctrl-bcm63xx.h"
+
+#define BCM63XX_BANK_SIZE 4
+
+#define BCM63XX_DIROUT_REG 0x04
+#define BCM63XX_DATA_REG 0x0c
+
+static int bcm63xx_reg_mask_xlate(struct gpio_regmap *gpio,
+ unsigned int base, unsigned int offset,
+ unsigned int *reg, unsigned int *mask)
+{
+ unsigned int line = offset % BCM63XX_BANK_GPIOS;
+ unsigned int stride = offset / BCM63XX_BANK_GPIOS;
+
+ *reg = base - stride * BCM63XX_BANK_SIZE;
+ *mask = BIT(line);
+
+ return 0;
+}
+
+static const struct of_device_id bcm63xx_gpio_of_match[] = {
+ { .compatible = "brcm,bcm6318-gpio", },
+ { .compatible = "brcm,bcm6328-gpio", },
+ { .compatible = "brcm,bcm6358-gpio", },
+ { .compatible = "brcm,bcm6362-gpio", },
+ { .compatible = "brcm,bcm6368-gpio", },
+ { .compatible = "brcm,bcm63268-gpio", },
+ { /* sentinel */ },
+};
+
+static int bcm63xx_gpio_probe(struct device *dev, struct device_node *node,
+ const struct bcm63xx_pinctrl_soc *soc,
+ struct bcm63xx_pinctrl *pc)
+{
+ struct gpio_regmap_config grc = {0};
+
+ grc.parent = dev;
+ grc.of_node = node;
+ grc.ngpio = soc->ngpios;
+ grc.ngpio_per_reg = BCM63XX_BANK_GPIOS;
+ grc.regmap = pc->regs;
+ grc.reg_mask_xlate = bcm63xx_reg_mask_xlate;
+
+ if (of_property_read_u32(node, "data", &grc.reg_dat_base))
+ grc.reg_dat_base = BCM63XX_DATA_REG;
+ grc.reg_set_base = grc.reg_dat_base;
+
+ if (of_property_read_u32(node, "dirout", &grc.reg_dir_out_base))
+ grc.reg_dir_out_base = BCM63XX_DIROUT_REG;
+
+ return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &grc));
+}
+
+int bcm63xx_pinctrl_probe(struct platform_device *pdev,
+ const struct bcm63xx_pinctrl_soc *soc,
+ void *driver_data)
+{
+ struct device *dev = &pdev->dev;
+ struct bcm63xx_pinctrl *pc;
+ struct device_node *node;
+ int err;
+
+ pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
+ if (!pc)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, pc);
+
+ pc->dev = dev;
+ pc->driver_data = driver_data;
+
+ pc->regs = syscon_node_to_regmap(dev->parent->of_node);
+ if (IS_ERR(pc->regs))
+ return PTR_ERR(pc->regs);
+
+ pc->pctl_desc.name = dev_name(dev);
+ pc->pctl_desc.pins = soc->pins;
+ pc->pctl_desc.npins = soc->npins;
+ pc->pctl_desc.pctlops = soc->pctl_ops;
+ pc->pctl_desc.pmxops = soc->pmx_ops;
+ pc->pctl_desc.owner = THIS_MODULE;
+
+ pc->pctl_dev = devm_pinctrl_register(dev, &pc->pctl_desc, pc);
+ if (IS_ERR(pc->pctl_dev))
+ return PTR_ERR(pc->pctl_dev);
+
+ for_each_child_of_node(dev->of_node, node) {
+ if (of_match_node(bcm63xx_gpio_of_match, node)) {
+ err = bcm63xx_gpio_probe(dev, node, soc, pc);
+ if (err) {
+ dev_err(dev, "could not add GPIO chip\n");
+ of_node_put(node);
+ return err;
+ }
+ }
+ }
+
+ dev_info(dev, "registered\n");
+
+ return 0;
+}
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm63xx.h b/drivers/pinctrl/bcm/pinctrl-bcm63xx.h
new file mode 100644
index 000000000000..cb2bfca7d174
--- /dev/null
+++ b/drivers/pinctrl/bcm/pinctrl-bcm63xx.h
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2021 Álvaro Fernández Rojas <[email protected]>
+ * Copyright (C) 2016 Jonas Gorski <[email protected]>
+ */
+
+#ifndef __PINCTRL_BCM63XX_H__
+#define __PINCTRL_BCM63XX_H__
+
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/regmap.h>
+
+#include "../core.h"
+
+#define BCM63XX_BANK_GPIOS 32
+
+struct bcm63xx_pinctrl_soc {
+ struct pinctrl_ops *pctl_ops;
+ struct pinmux_ops *pmx_ops;
+
+ const struct pinctrl_pin_desc *pins;
+ unsigned npins;
+
+ unsigned int ngpios;
+};
+
+struct bcm63xx_pinctrl {
+ struct device *dev;
+ struct regmap *regs;
+
+ struct pinctrl_desc pctl_desc;
+ struct pinctrl_dev *pctl_dev;
+
+ void *driver_data;
+};
+
+static inline unsigned int bcm63xx_bank_pin(unsigned int pin)
+{
+ return pin % BCM63XX_BANK_GPIOS;
+}
+
+int bcm63xx_pinctrl_probe(struct platform_device *pdev,
+ const struct bcm63xx_pinctrl_soc *soc,
+ void *driver_data);
+
+#endif /* __PINCTRL_BCM63XX_H__ */
--
2.20.1
Hi Andy
> El 4 mar 2021, a las 11:43, Andy Shevchenko <[email protected]> escribió:
>
> On Thu, Mar 4, 2021 at 10:57 AM Álvaro Fernández Rojas
> <[email protected]> wrote:
>>
>> Add a helper for registering BCM63XX pin controllers.
>>
>> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
>> Signed-off-by: Jonas Gorski <[email protected]>
>
> This SoB is in a strange place.
Why?
Can’t we both sign the patches?
>
> The order is wrong taking into account the From header (committer). So,
> it's not clear who is the author, who is a co-developer, and who is
> the committer (one person may utilize few roles).
> Check for the rest of the series as well (basically this is the rule
> of thumb to recheck entire code for the comment you have got at any
> single place of it).
Jonas was the original author of this patches (sent back in 2016) and I’m just continuing his work and trying to get those patches upstreamed.
I don’t know how to do it correctly, so a little hint would be appreciated.
>
> ...
>
>> +static const struct of_device_id bcm63xx_gpio_of_match[] = {
>> + { .compatible = "brcm,bcm6318-gpio", },
>> + { .compatible = "brcm,bcm6328-gpio", },
>> + { .compatible = "brcm,bcm6358-gpio", },
>> + { .compatible = "brcm,bcm6362-gpio", },
>> + { .compatible = "brcm,bcm6368-gpio", },
>> + { .compatible = "brcm,bcm63268-gpio", },
>
>> + { /* sentinel */ },
>
> Comma is not needed in terminator line
Ok, I will remove it.
>
>> +};
>
> ...
>
>> + dev_info(dev, "registered\n");
>
> Unneeded noise.
Ok, I will remove it.
>
> ...
>
>> +#include <linux/pinctrl/pinctrl.h>
>> +#include <linux/regmap.h>
>
> The rule of thumb is to include only the headers that the below code
> is direct user of.
Ok, so I will move them to pinctrl-bcm63xx.c.
I added them because they were needed for pinctrl_desc.
>
> The above are not used anyhow, while missed types.h and several
> forward declarations.
… so I should include linux/types.h and I don’t know what you mean by “several forward declarations”…
>
>> +#include "../core.h"
>
> Seems the same.
Ok, I will move it to pinctrl-bcm63xx.c.
>
>> +#define BCM63XX_BANK_GPIOS 32
>> +
>> +struct bcm63xx_pinctrl_soc {
>> + struct pinctrl_ops *pctl_ops;
>> + struct pinmux_ops *pmx_ops;
>> +
>> + const struct pinctrl_pin_desc *pins;
>> + unsigned npins;
>> +
>> + unsigned int ngpios;
>> +};
>> +
>> +struct bcm63xx_pinctrl {
>> + struct device *dev;
>> + struct regmap *regs;
>> +
>> + struct pinctrl_desc pctl_desc;
>> + struct pinctrl_dev *pctl_dev;
>> +
>> + void *driver_data;
>> +};
>
>
> --
> With Best Regards,
> Andy Shevchenko
Best regards,
Álvaro.
On Thu, Mar 4, 2021 at 2:28 PM Álvaro Fernández Rojas <[email protected]> wrote:
> > El 4 mar 2021, a las 13:09, Andy Shevchenko <[email protected]> escribió:
> > On Thu, Mar 4, 2021 at 1:17 PM Álvaro Fernández Rojas <[email protected]> wrote:
> >>> El 4 mar 2021, a las 11:43, Andy Shevchenko <[email protected]> escribió:
> >>> On Thu, Mar 4, 2021 at 10:57 AM Álvaro Fernández Rojas
> >>> <[email protected]> wrote:
> >>>>
> >>>> Add a helper for registering BCM63XX pin controllers.
> >>>>
> >>>> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
> >>>> Signed-off-by: Jonas Gorski <[email protected]>
> >>>
> >>> This SoB is in a strange place.
> >>
> >> Why?
> >> Can’t we both sign the patches?
> >
> > You can, but you have to follow the rules (see chapters 11-13 in the [1]).
> >
> >>> The order is wrong taking into account the From header (committer). So,
> >>> it's not clear who is the author, who is a co-developer, and who is
> >>> the committer (one person may utilize few roles).
> >>> Check for the rest of the series as well (basically this is the rule
> >>> of thumb to recheck entire code for the comment you have got at any
> >>> single place of it).
> >>
> >> Jonas was the original author of this patches (sent back in 2016) and I’m just continuing his work and trying to get those patches upstreamed.
> >> I don’t know how to do it correctly, so a little hint would be appreciated.
> >
> > There are two ways (depends on the amount of work you have done):
> > - leave him as an original author (so Author field will have his name,
> > not yours) and apply yours with Co-developed-by tag and SoB since you
> > are co-developed and committed
> > - other way around
>
> So I will move his SoB to the top, add a Co-developed-by referencing him before that and then leave my SoB as the last one.
Yes, if it is what you, guys, agreed on and want to have.
Something like this.
Co-developed-by: Jonas
SoB: Jonas
SoB: you (and From in the email should be yours as one who submitted the change)
> @Jonas are you OK with that?
> > [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
--
With Best Regards,
Andy Shevchenko
On Thu, Mar 4, 2021 at 10:57 AM Álvaro Fernández Rojas
<[email protected]> wrote:
>
> Add a helper for registering BCM63XX pin controllers.
>
> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
> Signed-off-by: Jonas Gorski <[email protected]>
This SoB is in a strange place.
The order is wrong taking into account the From header (committer). So,
it's not clear who is the author, who is a co-developer, and who is
the committer (one person may utilize few roles).
Check for the rest of the series as well (basically this is the rule
of thumb to recheck entire code for the comment you have got at any
single place of it).
...
> +static const struct of_device_id bcm63xx_gpio_of_match[] = {
> + { .compatible = "brcm,bcm6318-gpio", },
> + { .compatible = "brcm,bcm6328-gpio", },
> + { .compatible = "brcm,bcm6358-gpio", },
> + { .compatible = "brcm,bcm6362-gpio", },
> + { .compatible = "brcm,bcm6368-gpio", },
> + { .compatible = "brcm,bcm63268-gpio", },
> + { /* sentinel */ },
Comma is not needed in terminator line
> +};
...
> + dev_info(dev, "registered\n");
Unneeded noise.
...
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/regmap.h>
The rule of thumb is to include only the headers that the below code
is direct user of.
The above are not used anyhow, while missed types.h and several
forward declarations.
> +#include "../core.h"
Seems the same.
> +#define BCM63XX_BANK_GPIOS 32
> +
> +struct bcm63xx_pinctrl_soc {
> + struct pinctrl_ops *pctl_ops;
> + struct pinmux_ops *pmx_ops;
> +
> + const struct pinctrl_pin_desc *pins;
> + unsigned npins;
> +
> + unsigned int ngpios;
> +};
> +
> +struct bcm63xx_pinctrl {
> + struct device *dev;
> + struct regmap *regs;
> +
> + struct pinctrl_desc pctl_desc;
> + struct pinctrl_dev *pctl_dev;
> +
> + void *driver_data;
> +};
--
With Best Regards,
Andy Shevchenko
On Thu, Mar 4, 2021 at 1:17 PM Álvaro Fernández Rojas <[email protected]> wrote:
> > El 4 mar 2021, a las 11:43, Andy Shevchenko <[email protected]> escribió:
> > On Thu, Mar 4, 2021 at 10:57 AM Álvaro Fernández Rojas
> > <[email protected]> wrote:
> >>
> >> Add a helper for registering BCM63XX pin controllers.
> >>
> >> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
> >> Signed-off-by: Jonas Gorski <[email protected]>
> >
> > This SoB is in a strange place.
>
> Why?
> Can’t we both sign the patches?
You can, but you have to follow the rules (see chapters 11-13 in the [1]).
> > The order is wrong taking into account the From header (committer). So,
> > it's not clear who is the author, who is a co-developer, and who is
> > the committer (one person may utilize few roles).
> > Check for the rest of the series as well (basically this is the rule
> > of thumb to recheck entire code for the comment you have got at any
> > single place of it).
>
> Jonas was the original author of this patches (sent back in 2016) and I’m just continuing his work and trying to get those patches upstreamed.
> I don’t know how to do it correctly, so a little hint would be appreciated.
There are two ways (depends on the amount of work you have done):
- leave him as an original author (so Author field will have his name,
not yours) and apply yours with Co-developed-by tag and SoB since you
are co-developed and committed
- other way around
[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
...
> >> +#include <linux/pinctrl/pinctrl.h>
> >> +#include <linux/regmap.h>
> >
> > The rule of thumb is to include only the headers that the below code
> > is direct user of.
>
> Ok, so I will move them to pinctrl-bcm63xx.c.
> I added them because they were needed for pinctrl_desc.
Ah, for that yes, you need a header.
> > The above are not used anyhow, while missed types.h and several
> > forward declarations.
>
> … so I should include linux/types.h and I don’t know what you mean by “several forward declarations”…
Like
struct regmap;
which effectively tells the compiler that "hey, this type is defined
somewhere else".
--
With Best Regards,
Andy Shevchenko
Hi Andy,
> El 4 mar 2021, a las 13:09, Andy Shevchenko <[email protected]> escribió:
>
> On Thu, Mar 4, 2021 at 1:17 PM Álvaro Fernández Rojas <[email protected]> wrote:
>>> El 4 mar 2021, a las 11:43, Andy Shevchenko <[email protected]> escribió:
>>> On Thu, Mar 4, 2021 at 10:57 AM Álvaro Fernández Rojas
>>> <[email protected]> wrote:
>>>>
>>>> Add a helper for registering BCM63XX pin controllers.
>>>>
>>>> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
>>>> Signed-off-by: Jonas Gorski <[email protected]>
>>>
>>> This SoB is in a strange place.
>>
>> Why?
>> Can’t we both sign the patches?
>
> You can, but you have to follow the rules (see chapters 11-13 in the [1]).
>
>>> The order is wrong taking into account the From header (committer). So,
>>> it's not clear who is the author, who is a co-developer, and who is
>>> the committer (one person may utilize few roles).
>>> Check for the rest of the series as well (basically this is the rule
>>> of thumb to recheck entire code for the comment you have got at any
>>> single place of it).
>>
>> Jonas was the original author of this patches (sent back in 2016) and I’m just continuing his work and trying to get those patches upstreamed.
>> I don’t know how to do it correctly, so a little hint would be appreciated.
>
> There are two ways (depends on the amount of work you have done):
> - leave him as an original author (so Author field will have his name,
> not yours) and apply yours with Co-developed-by tag and SoB since you
> are co-developed and committed
> - other way around
So I will move his SoB to the top, add a Co-developed-by referencing him before that and then leave my SoB as the last one.
@Jonas are you OK with that?
>
> [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
>
> ...
>
>>>> +#include <linux/pinctrl/pinctrl.h>
>>>> +#include <linux/regmap.h>
>>>
>>> The rule of thumb is to include only the headers that the below code
>>> is direct user of.
>>
>> Ok, so I will move them to pinctrl-bcm63xx.c.
>> I added them because they were needed for pinctrl_desc.
>
> Ah, for that yes, you need a header.
>
>>> The above are not used anyhow, while missed types.h and several
>>> forward declarations.
>>
>> … so I should include linux/types.h and I don’t know what you mean by “several forward declarations”…
>
> Like
>
> struct regmap;
>
> which effectively tells the compiler that "hey, this type is defined
> somewhere else".
>
> --
> With Best Regards,
> Andy Shevchenko
Best regards,
Álvaro.