Messages in this thread | | | From | Lars Povlsen <> | Subject | Re: [PATCH v6 2/3] pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO | Date | Mon, 26 Oct 2020 15:41:48 +0100 |
| |
Hi Andy!
Andy Shevchenko writes:
> 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? >
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
| |