| Message ID | cover.1676042188.git.asmaa@nvidia.com |
|---|---|
| Headers | show |
| Series | Add NVIDIA BlueField-3 GPIO driver and pin controller | expand |
Support the BlueField-3 SoC GPIO driver for handling interrupts and providing the option to change the direction and value of a GPIO. Support the BlueField-3 SoC pin controller driver for allowing a select number of GPIO pins to be manipulated from userspace or the kernel. The gpio-mlxbf3.c driver handles hardware registers and logic that are different from gpio-mlxbf.c and gpio-mlxbf2.c. For that reason, we have separate drivers for each generation. Changes from v3->v4: gpio-mlxbf3.c: - Update the Kconfig file so that it is conform with checkpatch - Remove unncessary headers and add missing header inclusions - Make irq_chip struct static and const - Replace generic_handle_irq(irq_find_mapping) with generic_handle_domain_irq - Simplify logic in irq_set_type - Replace valid_mask with gpio-reserved-ranges - Cleanup code pinctrl-mlxbf.c: - Cleanup code - Update the Kconfig file so that it is conform with checkpatch - Remove unncessary headers and add missing header inclusions Asmaa Mnebhi (2): gpio: gpio-mlxbf3: Add gpio driver support pinctrl: pinctrl-mlxbf: Add pinctrl driver support drivers/gpio/Kconfig | 12 ++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-mlxbf3.c | 230 ++++++++++++++++++++++ drivers/pinctrl/Kconfig | 14 ++ drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-mlxbf.c | 338 ++++++++++++++++++++++++++++++++ 6 files changed, 596 insertions(+) create mode 100644 drivers/gpio/gpio-mlxbf3.c create mode 100644 drivers/pinctrl/pinctrl-mlxbf.c
On Fri, Feb 17, 2023 at 11:27 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote: > > NVIDIA BlueField-3 SoC has a few pins that can be used as GPIOs > or take the default hardware functionality. Add a driver for > the pinmuxing. pin muxing ... > +++ b/drivers/pinctrl/pinctrl-mlxbf.c Wondering if it would be better to match the GPIO driver naming schema, i.e. by adding 3. In this case the additional explanation in Kconfig help won't be necessary. ... > +#define MLXBF_GPIO0_FW_CONTROL_SET 0 > +#define MLXBF_GPIO0_FW_CONTROL_CLEAR 0x14 > +#define MLXBF_GPIO1_FW_CONTROL_SET 0x80 > +#define MLXBF_GPIO1_FW_CONTROL_CLEAR 0x94 Unclear if these are commands or register offsets. If they are of the same type (semantically), make them fixed width, e.g., 0x00. ... > +enum { > + MLXBF_GPIO_HW_MODE, > + MLXBF_GPIO_SW_MODE I would leave a comma here as it might be extended in the future. > +}; ... > +static const char * const mlxbf_pinctrl_single_group_names[] = { > + "gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6", > + "gpio7", "gpio8", "gpio9", "gpio10", "gpio11", "gpio12", "gpio13", > + "gpio14", "gpio15", "gpio16", "gpio17", "gpio18", "gpio19", "gpio20", > + "gpio21", "gpio22", "gpio23", "gpio24", "gpio25", "gpio26", "gpio27", > + "gpio28", "gpio29", "gpio30", "gpio31", "gpio32", "gpio33", "gpio34", > + "gpio35", "gpio36", "gpio37", "gpio38", "gpio39", "gpio40", "gpio41", > + "gpio42", "gpio43", "gpio44", "gpio45", "gpio46", "gpio47", "gpio48", > + "gpio49", "gpio50", "gpio51", "gpio52", "gpio53", "gpio54", "gpio55" Ditto. Can you group by 8? > +}; > + > +/* Set of pin numbers for single-pin groups */ > +static const unsigned int mlxbf_pinctrl_single_group_pins[] = { > + 0, 1, 2, 3, 4, 5, 6, > + 7, 8, 9, 10, 11, 12, 13, > + 14, 15, 16, 17, 18, 19, 20, > + 21, 22, 23, 24, 25, 26, 27, > + 28, 29, 30, 31, 32, 33, 34, > + 35, 36, 37, 38, 39, 40, 41, > + 42, 43, 44, 45, 46, 47, 48, > + 49, 50, 51, 52, 53, 54, 55, Group by 8 which is the more natural length of subarray per line. Is it just 1:1 to the index? If so, why do you need this table at all? > +}; ... > +static const struct { > + const char *name; > + const char * const *group_names; Use this instead https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n215 and this https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n222 > +} mlxbf_pmx_funcs[] = { > +}; ... > +{ > + struct mlxbf_pinctrl *priv = pinctrl_dev_get_drvdata(pctldev); > + > + /* disable GPIO functionality by giving control back to hardware */ > + if (offset < MLXBF_NGPIOS_GPIO0) > + writel(BIT(offset), priv->base + MLXBF_GPIO0_FW_CONTROL_CLEAR); > + else > + writel(BIT(offset % MLXBF_NGPIOS_GPIO0), priv->base + MLXBF_GPIO1_FW_CONTROL_CLEAR); > + Redundant blank line. > +} ... > +static_assert(ARRAY_SIZE(mlxbf_pinctrl_single_group_names) == > + ARRAY_SIZE(mlxbf_pinctrl_single_group_pins)); I would put it on a single line, but it's up to you. ... > + struct resource *res; Useless? ... > + /* This resource is shared so use devm_ioremap */ Can you elaborate on who actually requests the region? And why is it not _this_ driver? > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENODEV; ... > + ret = devm_pinctrl_register_and_init(priv->dev, Is the priv->dev different from dev? > + &mlxbf_pin_desc, > + priv, > + &priv->pctl); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to register pinctrl\n"); ... > + pinctrl_add_gpio_range(priv->pctl, &mlxbf_pinctrl_gpio_ranges[0]); > + pinctrl_add_gpio_range(priv->pctl, &mlxbf_pinctrl_gpio_ranges[1]); pinctrl_add_gpio_ranges() ? -- With Best Regards, Andy Shevchenko
> > +static const struct { > > + const char *name; > > + const char * const *group_names; > > Use this instead > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux- > pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n215 > and this > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux- > pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n222 > > > +} mlxbf_pmx_funcs[] = { > > > +}; so copy that struct definition and macro to my driver? (I don’t see these code changes in master) > > > + /* This resource is shared so use devm_ioremap */ > > Can you elaborate on who actually requests the region? And why is it not > _this_ driver? This resource is shared with the gpio-mlxbf3.c driver. The gpio-mlxbf3.c driver does not access the same offsets as the pinctrl-mlxbf3.c driver, but it accesses several other registers offsets in between.
On Thu, Feb 23, 2023 at 11:07 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote: > > > > +static const struct { > > > + const char *name; > > > + const char * const *group_names; > > > > Use this instead > > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux- > > pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n215 > > and this > > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux- > > pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n222 > > > > > +} mlxbf_pmx_funcs[] = { > > > > > +}; > > so copy that struct definition and macro to my driver? (I don’t see these code changes in master) Which master? First of all, you should do your development based on the "for-next" of the respective subsystem (okay, for pin control Linus Walleij called his published branch "devel"). So, the above mentioned functionality was there a while ago. Second, a couple of days ago Linus Torvalds pulled PR, so it's part of upstream now. TL;DR: just use those types and macros in your code. > > > + /* This resource is shared so use devm_ioremap */ > > > > Can you elaborate on who actually requests the region? And why is it not > > _this_ driver? > > This resource is shared with the gpio-mlxbf3.c driver. The gpio-mlxbf3.c driver does not access the same offsets as the pinctrl-mlxbf3.c driver, but it accesses several other registers offsets in between. Okay, so in such a case you need a common denominator that actually does this all for you via, for example, regmap. If the region is not requested, bad things may happen.
> > > > + /* This resource is shared so use devm_ioremap */ > > > > > > Can you elaborate on who actually requests the region? And why is it > > > not _this_ driver? > > > > This resource is shared with the gpio-mlxbf3.c driver. The gpio-mlxbf3.c > driver does not access the same offsets as the pinctrl-mlxbf3.c driver, but it > accesses several other registers offsets in between. > > Okay, so in such a case you need a common denominator that actually does > this all for you via, for example, regmap. If the region is not requested, bad > things may happen. Hi Andy, I have taken a look at the regmap driver and decided we will maintain the Current implementation with devm_platform_ioremap_resource. The main reason is for backward compatibility with older kernels. Although we Do support the latest kernel, we have some customers who are still using old Kernels which don’t have regmap support. But I have found a solution which Involves using devm_platform_ioremap_resource here as you initially suggested. patch v5 coming soon! Thanks. Asmaa