| Message ID | 20201014100707.2728637-1-lars.povlsen@microchip.com |
|---|---|
| Headers | show |
| Series | Adding support for Microchip/Microsemi serial GPIO controller | expand |
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?
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
The series add support for the serial GPIO controller used by Microchip Sparx5, as well as (MSCC) ocelot/jaguar2 SoCs. v6 changes: - Use "bus-frequency" instead of "microchip,sgpio-frequency". Drop '$ref'. (Robh) - Added "ngpios" description, bumped minimum to 32. (Linus) - Added "#size-cells" description. (Linus) - Changed "bus-frequency" validation in driver to reflect the YAML description. v5 changes (driver comments from Linus): - Collect bank data in sgpio_bank struct - Add is_input boolean to sgpio_bank struct - Use single-bit bitmasks in sgpio_output_set() and sgpio_output_get() - Eliminate superfluous struct pinctrl_dev *pctl_dev in bank data - Fix wrong ngpio consistency check v4 changes (binding comments from Rob): - microchip,sgpio-port-ranges changed to uint32-matrix so tuples can be represented properly. - gpio controller node name changed to "gpio@[0-1]" - whitespace fixes - DT files updated as per schema changes v3 changes: - Renamed all usage of "mchp" abbrevation with "microchip". - Split the in/output directions into (two) separate banks. - Eliminated the bindings include file (from above) - Changed SPDX license to "GPL-2.0-or-later" - Change -ENOTSUPP to -EOPNOTSUPP - Minor type/symbol naming changes v2 changes: - Adds both in and output modes. - Use direct adressing of the individual banks (#gpio-cells = <4>), also osoleting need for addressing macros in bindings include file. - Property 'microchip,sgpio-ports' (uint32, bitmask) replaced by proper range set (array of [start,end]) 'microchip,sgpio-port-ranges'. - Fixes whitespace issues in Kconfig file Lars Povlsen (3): dt-bindings: pinctrl: Add bindings for pinctrl-microchip-sgpio driver pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO arm64: dts: sparx5: Add SGPIO devices Lars Povlsen (3): dt-bindings: pinctrl: Add bindings for pinctrl-microchip-sgpio driver pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO arm64: dts: sparx5: Add SGPIO devices .../pinctrl/microchip,sparx5-sgpio.yaml | 145 ++++ MAINTAINERS | 1 + arch/arm64/boot/dts/microchip/sparx5.dtsi | 91 +++ .../boot/dts/microchip/sparx5_pcb125.dts | 5 + .../dts/microchip/sparx5_pcb134_board.dtsi | 258 +++++++ .../dts/microchip/sparx5_pcb135_board.dtsi | 55 ++ drivers/pinctrl/Kconfig | 18 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-microchip-sgpio.c | 667 ++++++++++++++++++ 9 files changed, 1241 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml create mode 100644 drivers/pinctrl/pinctrl-microchip-sgpio.c -- 2.25.1