Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 16 Oct 2020 17:34:15 +0300 | Subject | Re: [PATCH v6 2/3] pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO |
| |
On Wed, Oct 14, 2020 at 6:25 PM Lars Povlsen <lars.povlsen@microchip.com> wrote: > > This adds a pinctrl driver for the Microsemi/Microchip Serial GPIO > (SGPIO) device used in various SoC's.
...
> +#define PIN_NAM_SZ (sizeof("SGPIO_D_pXXbY")+1)
+1 for what?
...
> +#define __shf(x) (__builtin_ffsll(x) - 1) > +#define __BF_PREP(bf, x) (bf & ((x) << __shf(bf))) > +#define __BF_GET(bf, x) (((x & bf) >> __shf(bf)))
This smells like bitfield.h.
...
> +static int sgpio_input_get(struct sgpio_priv *priv, > + struct sgpio_port_addr *addr) > +{
> + int ret; > + > + ret = !!(sgpio_readl(priv, REG_INPUT_DATA, addr->bit) & > + BIT(addr->port)); > + > + return ret;
Sounds like one line.
> +}
> +static int sgpio_get_functions_count(struct pinctrl_dev *pctldev) > +{
> + return 1;
I didn't get why it's not a pure GPIO driver? It has only one function (no pinmux). I didn't find any pin control features either.
What did I miss?
...
> +static int microchip_sgpio_get_value(struct gpio_chip *gc, unsigned int gpio) > +{ > + struct sgpio_bank *bank = gpiochip_get_data(gc); > + struct sgpio_priv *priv = bank->priv; > + struct sgpio_port_addr addr;
> + int ret;
No need.
> + > + sgpio_pin_to_addr(priv, gpio, &addr); > + > + if (bank->is_input) > + ret = sgpio_input_get(priv, &addr); > + else > + ret = sgpio_output_get(priv, &addr); > + > + return ret; > +}
...
> + ret = devm_gpiochip_add_data(dev, gc, bank); > + if (ret == 0)
> + dev_info(dev, "Registered %d GPIOs\n", ngpios);
No noise.
> + else > + dev_err(dev, "Failed to register: ret %d\n", ret); > +
...
> + /* Get register map */ > + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + priv->regs = devm_ioremap_resource(dev, regs);
devm_platform_ioremap_resource();
> + if (IS_ERR(priv->regs)) > + return PTR_ERR(priv->regs);
> + priv->properties = of_device_get_match_data(dev);
It's interesting you have a mix between OF APIs and device property APIs. Choose one. If you stick with OF, use of_property_ and so, otherwise replace of_*() by corresponding device_*() or generic calls.
Can you use gpio-regmap APIs?
-- With Best Regards, Andy Shevchenko
| |