| Message ID | 20201111122454.6240-1-lars.povlsen@microchip.com |
|---|---|
| Headers | show |
| Series | Adding support for Microchip/Microsemi serial GPIO controller | expand |
On Wed, Nov 11, 2020 at 2: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. > > The driver is added as a pinctrl driver, albeit only having just GPIO > support currently. The hardware supports other functions that will be > added following. Thanks for an update! Seems closer to the final. My comments below. ... > + * Author: <lars.povlsen@microchip.com> No First Name Last Name? ... > +static int sgpio_output_get(struct sgpio_priv *priv, > + struct sgpio_port_addr *addr) > +{ > + u32 val, portval = sgpio_readl(priv, REG_PORT_CONFIG, addr->port); > + unsigned int bit = SGPIO_SRC_BITS * addr->bit; > + > + switch (priv->properties->arch) { > + case SGPIO_ARCH_LUTON: > + val = FIELD_GET(SGPIO_LUTON_BIT_SOURCE, portval); > + break; > + case SGPIO_ARCH_OCELOT: > + val = FIELD_GET(SGPIO_OCELOT_BIT_SOURCE, portval); > + break; > + case SGPIO_ARCH_SPARX5: > + val = FIELD_GET(SGPIO_SPARX5_BIT_SOURCE, portval); > + break; > + default: > + val = 0; Missed break; statement. > + } > + return !!(val & BIT(bit)); > +} ... > +static const struct pinconf_ops sgpio_confops = { > + .is_generic = true, > + .pin_config_get = sgpio_pinconf_get, > + .pin_config_set = sgpio_pinconf_set, > + .pin_config_config_dbg_show = pinconf_generic_dump_config, Do you need this? I mean isn't it default by pin core? > +}; ... > +static int sgpio_gpio_request_enable(struct pinctrl_dev *pctldev, > + struct pinctrl_gpio_range *range, > + unsigned int offset) > +{ > + struct sgpio_bank *bank = pinctrl_dev_get_drvdata(pctldev); > + struct sgpio_priv *priv = bank->priv; > + struct sgpio_port_addr addr; > + > + sgpio_pin_to_addr(priv, offset, &addr); > + > + if ((priv->ports & BIT(addr.port)) == 0) { > + dev_warn(priv->dev, "Request port %d.%d: Port is not enabled\n", > + addr.port, addr.bit); > + } > + > + return 0; I believe this function also does some sanity checks. Perhaps you need to call a generic one. Hence check what should be done in the tear down case. > +} ... > + if (priv->in.gpio.ngpio != priv->out.gpio.ngpio) { > + dev_err(dev, "Banks must have same GPIO count\n"); > + return -EINVAL; -ERANGE? > + } -- With Best Regards, Andy Shevchenko
Andy Shevchenko writes: > On Wed, Nov 11, 2020 at 2: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. >> >> The driver is added as a pinctrl driver, albeit only having just GPIO >> support currently. The hardware supports other functions that will be >> added following. > > Thanks for an update! > Seems closer to the final. My comments below. Well I am certainly glad to hear that! > > ... > >> + * Author: <lars.povlsen@microchip.com> > > No First Name Last Name? > I'll add that. > ... > >> +static int sgpio_output_get(struct sgpio_priv *priv, >> + struct sgpio_port_addr *addr) >> +{ >> + u32 val, portval = sgpio_readl(priv, REG_PORT_CONFIG, addr->port); >> + unsigned int bit = SGPIO_SRC_BITS * addr->bit; >> + >> + switch (priv->properties->arch) { >> + case SGPIO_ARCH_LUTON: >> + val = FIELD_GET(SGPIO_LUTON_BIT_SOURCE, portval); >> + break; >> + case SGPIO_ARCH_OCELOT: >> + val = FIELD_GET(SGPIO_OCELOT_BIT_SOURCE, portval); >> + break; >> + case SGPIO_ARCH_SPARX5: >> + val = FIELD_GET(SGPIO_SPARX5_BIT_SOURCE, portval); >> + break; >> + default: >> + val = 0; > > Missed break; statement. Fine. > >> + } >> + return !!(val & BIT(bit)); >> +} > > ... > >> +static const struct pinconf_ops sgpio_confops = { >> + .is_generic = true, >> + .pin_config_get = sgpio_pinconf_get, >> + .pin_config_set = sgpio_pinconf_set, > >> + .pin_config_config_dbg_show = pinconf_generic_dump_config, > > Do you need this? I mean isn't it default by pin core? No, I see other drivers also setting this up explicitly. > >> +}; > > ... > >> +static int sgpio_gpio_request_enable(struct pinctrl_dev *pctldev, >> + struct pinctrl_gpio_range *range, >> + unsigned int offset) >> +{ >> + struct sgpio_bank *bank = pinctrl_dev_get_drvdata(pctldev); >> + struct sgpio_priv *priv = bank->priv; >> + struct sgpio_port_addr addr; >> + >> + sgpio_pin_to_addr(priv, offset, &addr); >> + >> + if ((priv->ports & BIT(addr.port)) == 0) { >> + dev_warn(priv->dev, "Request port %d.%d: Port is not enabled\n", >> + addr.port, addr.bit); >> + } >> + >> + return 0; > > I believe this function also does some sanity checks. Perhaps you need > to call a generic one. > Hence check what should be done in the tear down case. > This checks whether the requested signal is actually enabled in the bitstream. If it is not, it will trigger a warning message. I recon it should also signal this with the error code, so I'll add that. Generic code does not have knowledge about the bit stream configuration (priv->ports), so it can't check for that. >> +} > > ... > >> + if (priv->in.gpio.ngpio != priv->out.gpio.ngpio) { >> + dev_err(dev, "Banks must have same GPIO count\n"); >> + return -EINVAL; > > -ERANGE? We can do that. > >> + } Thanks, ---Lars -- Lars Povlsen, Microchip
The series add support for the serial GPIO controller used by Microchip Sparx5, as well as (MSCC) ocelot/jaguar2 SoCs. v9 changes (from Andy): - Avoid bitfield duplication (use FIELD_PREP/FIELD_GET) - Introduce SGPIO_SRC_BITS define constant - Use ENOTSUPP instead of EOPNOTSUPP (checkpatch will complain) - Drop dev_err() when using pin for wrong direction - Replaced left-over OF code with device_property_* flavors - Use devm_kasprintf() instead of strnprintf()+devm_strdup() - Minor formatting changes, deleted comments v8 changes (from Andy): - Removed OF dependency/usage entirely. - Trimmed+sorted include files. - Made local variables reverse xmas sorted. - Removed __func__ usage. - Changed some occurences of "if (x) { ..." to early return. - Use dev_err_probe() where possible. - Replace of_device_get_match_data() with device_get_match_data() - Some minor formatting corrections - Do per-pin string allocation as opposed to bulk allocation+chop. v7 changes: - Fixed wrong sizeof in pin string name template. (Andy) - Collapsed sgpio_input_get() to one liner. (Andy) - Eliminated unneeded variable in microchip_sgpio_get_value() - Removed noisy dev_info(). (Andy) - Replaced platform_get_resource()+devm_ioremap_resource() with devm_platform_ioremap_resource(). (Andy) - Replaced device_property_read_u32() with of_property_read_u32(). (Andy) - Replaced __builtin_ffsll() with __builtin_ffs() for MIPS32 targets. 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 .../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 | 16 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-microchip-sgpio.c | 707 ++++++++++++++++++ 9 files changed, 1279 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml create mode 100644 drivers/pinctrl/pinctrl-microchip-sgpio.c -- 2.25.1