On Wed, Oct 14, 2020 at 6:25 PM Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx> 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
