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

[Álvaro Fernández Rojas]

Hi Andy,

> El 7 mar 2021, a las 20:05, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> escribió:
> 
> On Sat, Mar 6, 2021 at 5:57 PM Álvaro Fernández Rojas <noltari@xxxxxxxxx> 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)

So you want me to convert everything to fwnode, but then I would need to use of_node here…
It makes more sense to me to use of_node for now and convert it to fwnode in the future…
@Linus, what do you think?

> 
>> +                       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@xxxxxxxxx>
>> + * Copyright (C) 2016 Jonas Gorski <jonas.gorski@xxxxxxxxx>
>> + */
>> +
>> +#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

Best regards,
Álvaro.