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
