| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 12 May 2023 20:07:59 +0300
From: andy.shevchenko@...il.com
To: Esteban Blanc <eblanc@...libre.com>
Cc: linus.walleij@...aro.org, lgirdwood@...il.com, broonie@...nel.org,
a.zummo@...ertech.it, alexandre.belloni@...tlin.com,
linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-rtc@...r.kernel.org, jpanis@...libre.com,
jneanne@...libre.com, aseketeli@...libre.com, sterzik@...com,
u-kumar1@...com
Subject: Re: [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl
and GPIOs
Fri, May 12, 2023 at 04:17:54PM +0200, Esteban Blanc kirjoitti:
> TI TPS6594 PMIC has 11 GPIOs which can be used
> for different functions.
>
> This patch adds a pinctrl and GPIO drivers in
> order to use those functions.
...
> +config PINCTRL_THUNDERBAY
Is it correct name? To me sounds not. The problem is that you use platform name
for the non-platform-wide pin control, i.e. for PMIC exclusively.
Did I miss anything?
> + tristate "Generic pinctrl and GPIO driver for Intel Thunder Bay SoC"
> + depends on ARCH_THUNDERBAY || (ARM64 && COMPILE_TEST)
This doesn't look correct, but I remember some Kconfig options that are using
this way of dependency.
> + depends on HAS_IOMEM
> + select PINMUX
> + select PINCONF
> + select GENERIC_PINCONF
> + select GENERIC_PINCTRL_GROUPS
> + select GENERIC_PINMUX_FUNCTIONS
> + select GPIOLIB
> + select GPIOLIB_IRQCHIP
> + select GPIO_GENERIC
> + help
> + This selects pin control driver for the Intel Thunder Bay SoC.
> + It provides pin config functions such as pull-up, pull-down,
> + interrupt, drive strength, sec lock, Schmitt trigger, slew
> + rate control and direction control. This module will be
> + called as pinctrl-thunderbay.
Ah, the above simply a mistake. right?
Why is it in this patch?
> +config PINCTRL_TPS6594
> + tristate "Pinctrl and GPIO driver for TI TPS6594 PMIC"
> + depends on MFD_TPS6594
> + default MFD_TPS6594
> + select PINMUX
> + select GPIOLIB
> + select REGMAP
> + select GPIO_REGMAP
> + help
> + This driver supports GPIOs and pinmuxing for the TPS6594
> + PMICs chip family.
Module name?
...
> +obj-$(CONFIG_PINCTRL_THUNDERBAY) += pinctrl-thunderbay.o
Huh?!
> +obj-$(CONFIG_PINCTRL_TPS6594) += pinctrl-tps6594.o
...
> +#include <linux/gpio/regmap.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pinctrl/pinmux.h>
Ordered?
...
> +static const char *groups_name[TPS6594_PINCTRL_PINS_NB] = {
> + "GPIO0", "GPIO1", "GPIO2", "GPIO3", "GPIO4", "GPIO5",
> + "GPIO6", "GPIO7", "GPIO8", "GPIO9", "GPIO10"
Leave trailing comma even for known size.
> +};
...
> +struct tps6594_pinctrl_function {
> + const char *name;
> + u8 muxval;
> + const char **groups;
> + unsigned long ngroups;
We have struct pinfunction. Use it here (as embedded).
> +};
...
> +static const struct tps6594_pinctrl_function pinctrl_functions[] = {
> + { "gpio", TPS6594_PINCTRL_GPIO_FUNCTION, groups_name,
> + TPS6594_PINCTRL_PINS_NB },
Here and further use PINCTRL_PINFUNCTION() macro.
> +};
...
> +static int tps6594_group_pins(struct pinctrl_dev *pctldev,
> + unsigned int selector, const unsigned int **pins,
> + unsigned int *num_pins)
> +{
> + struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> + *pins = (unsigned int *)&pinctrl->pins[selector];
Why casting?
> + *num_pins = 1;
> +
> + return 0;
> +}
...
> + pinctrl->pctl_dev =
> + devm_pinctrl_register(&pdev->dev, pctrl_desc, pinctrl);
One line?
> + if (IS_ERR(pinctrl->pctl_dev)) {
> + dev_err(&pdev->dev, "Couldn't register pinctrl driver\n");
> + return PTR_ERR(pinctrl->pctl_dev);
return dev_err_probe(...);
> + }
...
> + pinctrl->gpio_regmap = devm_gpio_regmap_register(&pdev->dev, &config);
> + if (IS_ERR(pinctrl->gpio_regmap)) {
> + dev_err(&pdev->dev, "Couldn't register gpio_regmap driver\n");
> + return PTR_ERR(pinctrl->pctl_dev);
Ditto.
> + }
> +
> + return 0;
> +}
...
> -#define TPS6594_REG_GPIOX_CONF(gpio_inst) (0x31 + (gpio_inst))
> +#define TPS6594_REG_GPIO1_CONF 0x31
> +#define TPS6594_REG_GPIOX_CONF(gpio_inst) (TPS6594_REG_GPIO1_CONF + (gpio_inst))
Why? The original code with parameter 0 will issue the same.
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists