Re: [PATCH v6 2/3] pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO

[Lars Povlsen]

Hi Andy!

Andy Shevchenko writes:

> 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?
>

Reverse fencepost :-). I'll remove it.

> ...
>
>> +#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.
>

Yes, and I would use it if I could, just bitfield.h don't like anything
but constexpr. The driver support 3 SoC variants which (unfortunately)
have different register layouts in some areas.

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

Yes, I'll change that.

>> +}
>
>> +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?

The hardware has more functions, which are planned to be added
later. This has already been agreed with Linux Walleij.

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

Ok, I'll trim it.

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

OK, gone.

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

Yes, I'll replace with that.

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

Sure. I will change the device_property_read_u32() with
of_property_read_u32().

>
> Can you use gpio-regmap APIs?

No, I think the sgpio hardware is a little too odd for that
(of_gpio_n_cells == 3). And then there's alternate pinctrl functions.

Thank you for your comments, they are very much appreciated. Let me know
if I missed anything.

I will refresh the series shortly (on v5.10-rc1).

---Lars

-- 
Lars Povlsen,
Microchip