Re: [PATCH v5 03/15] pinctrl: bcm: add bcm63xx base code

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: "Álvaro Fernández Rojas" <noltari@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Michael Walle <michael@walle.cc>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	bcm-kernel-feedback-list <bcm-kernel-feedback-list@broadcom.com>,
	Jonas Gorski <jonas.gorski@gmail.com>,
	Necip Fazil Yildiran <fazilyildiran@gmail.com>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	devicetree <devicetree@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v5 03/15] pinctrl: bcm: add bcm63xx base code
Date: Sun, 7 Mar 2021 21:05:19 +0200	[thread overview]
Message-ID: <CAHp75VdwqpL0UScR5s+Tf4z7RZQfyo+625uXZtfWV3=xQr6Z2Q@mail.gmail.com> (raw)
In-Reply-To: <20210306155712.4298-4-noltari@gmail.com>

On Sat, Mar 6, 2021 at 5:57 PM Álvaro Fernández Rojas <noltari@gmail.com> wrote:
>
> Add a helper for registering BCM63XX pin controllers.

Thanks for this, but I think we may use the fwnode API. See below.

...

> +#include <linux/gpio/regmap.h>
> +#include <linux/mfd/syscon.h>

> +#include <linux/of.h>

+ property.h
+ mod_devicetable.h

> +#include <linux/platform_device.h>
> +
> +#include "pinctrl-bcm63xx.h"

> +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 int bcm63xx_gpio_probe(struct device *dev, struct device_node *node,

device_node *node -> fwnode_handle *fwnode

> +                             const struct bcm63xx_pinctrl_soc *soc,
> +                             struct bcm63xx_pinctrl *pc)
> +{
> +       struct gpio_regmap_config grc = {0};
> +
> +       grc.parent = dev;

> +       grc.fwnode = &node->fwnode;

grc.fwnode = fwnode;

> +       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))

fwnode_property_read_u32()

> +               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))

Ditto.

> +               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;

struct fwnode_handle *fwnode;

> +       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) {

device_for_each_child_node(dev, fwnode) {

> +               if (of_match_node(bcm63xx_gpio_of_match, node)) {

// for now, since we have not an analogue (yet)
node ==> to_of_node(fwnode)

> +                       err = bcm63xx_gpio_probe(dev, node, soc, pc);

...(dev, fwnode, soc, pc);

> +                       if (err) {
> +                               dev_err(dev, "could not add GPIO chip\n");

> +                               of_node_put(node);

fwnode_handle_put(fwnode);

> +                               return err;
> +                       }
> +               }
> +       }
> +
> +       return 0;
> +}
> diff --git a/drivers/pinctrl/bcm/pinctrl-bcm63xx.h b/drivers/pinctrl/bcm/pinctrl-bcm63xx.h
> new file mode 100644
> index 000000000000..3bdb50021f1b
> --- /dev/null
> +++ b/drivers/pinctrl/bcm/pinctrl-bcm63xx.h
> @@ -0,0 +1,43 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2021 Álvaro Fernández Rojas <noltari@gmail.com>
> + * Copyright (C) 2016 Jonas Gorski <jonas.gorski@gmail.com>
> + */
> +
> +#ifndef __PINCTRL_BCM63XX_H__
> +#define __PINCTRL_BCM63XX_H__
> +
> +#include <linux/pinctrl/pinctrl.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
>

--
With Best Regards,
Andy Shevchenko