From: Lars Povlsen <lars.povlsen@microchip.com> To: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Lars Povlsen <lars.povlsen@microchip.com>, Linus Walleij <linus.walleij@linaro.org>, Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>, devicetree <devicetree@vger.kernel.org>, "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>, "linux-arm Mailing List" <linux-arm-kernel@lists.infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Alexandre Belloni <alexandre.belloni@bootlin.com> Subject: Re: [PATCH v9 2/3] pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO Date: Fri, 13 Nov 2020 10:11:26 +0100 [thread overview] Message-ID: <87v9e94o5d.fsf@microchip.com> (raw) In-Reply-To: <CAHp75VfJ7T-ODiyKiMqK-oq5nO3776poSCJag9gvB-aqD3hoMg@mail.gmail.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Lars Povlsen <lars.povlsen@microchip.com> To: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: devicetree <devicetree@vger.kernel.org>, Alexandre Belloni <alexandre.belloni@bootlin.com>, Linus Walleij <linus.walleij@linaro.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>, "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>, Lars Povlsen <lars.povlsen@microchip.com>, linux-arm Mailing List <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH v9 2/3] pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO Date: Fri, 13 Nov 2020 10:11:26 +0100 [thread overview] Message-ID: <87v9e94o5d.fsf@microchip.com> (raw) In-Reply-To: <CAHp75VfJ7T-ODiyKiMqK-oq5nO3776poSCJag9gvB-aqD3hoMg@mail.gmail.com> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-11-13 9:11 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-11-11 12:24 [PATCH v9 0/3] Adding support for Microchip/Microsemi serial GPIO controller Lars Povlsen 2020-11-11 12:24 ` Lars Povlsen 2020-11-11 12:24 ` [PATCH v9 1/3] dt-bindings: pinctrl: Add bindings for pinctrl-microchip-sgpio driver Lars Povlsen 2020-11-11 12:24 ` Lars Povlsen 2020-11-11 12:24 ` [PATCH v9 2/3] pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO Lars Povlsen 2020-11-11 12:24 ` Lars Povlsen 2020-11-11 14:38 ` Andy Shevchenko 2020-11-11 14:38 ` Andy Shevchenko 2020-11-13 9:11 ` Lars Povlsen [this message] 2020-11-13 9:11 ` Lars Povlsen 2020-11-11 12:24 ` [PATCH v9 3/3] arm64: dts: sparx5: Add SGPIO devices Lars Povlsen 2020-11-11 12:24 ` Lars Povlsen
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87v9e94o5d.fsf@microchip.com \ --to=lars.povlsen@microchip.com \ --cc=UNGLinuxDriver@microchip.com \ --cc=alexandre.belloni@bootlin.com \ --cc=andy.shevchenko@gmail.com \ --cc=devicetree@vger.kernel.org \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.