| 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
| ||
|
Message-ID: <CAMRc=Md-CzrG3QPtnh0OxYaHTAYZ2aUfMKhkAOeRm2Zn30qE0A@mail.gmail.com>
Date: Mon, 15 May 2023 17:36:46 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Jerome Neanne <jneanne@...libre.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Tony Lindgren <tony@...mide.com>, Lee Jones <lee@...nel.org>,
linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-omap@...r.kernel.org,
Jonathan Cormier <jcormier@...ticallink.com>
Subject: Re: [PATCH v2 1/2] gpio: tps65219: add GPIO support for TPS65219 PMIC
On Thu, May 11, 2023 at 4:09 PM Jerome Neanne <jneanne@...libre.com> wrote:
>
> Add support for TPS65219 PMICs GPIO interface.
>
> 3 GPIO pins:
> - GPIO0 only is IO but input mode reserved for MULTI_DEVICE_ENABLE usage
> - GPIO1 and GPIO2 are Output only and referred as GPO1 and GPO2 in spec
>
> GPIO0 is statically configured as input or output prior to Linux boot.
> it is used for MULTI_DEVICE_ENABLE function.
> This setting is statically configured by NVM.
> GPIO0 can't be used as a generic GPIO (specification Table 8-34).
> It's either a GPO when MULTI_DEVICE_EN=0,
> or a GPI when MULTI_DEVICE_EN=1.
>
> Datasheet describes specific usage for non standard GPIO.
> Link: https://www.ti.com/lit/ds/symlink/tps65219.pdf
>
> Co-developed-by: Jonathan Cormier <jcormier@...ticallink.com>
> Signed-off-by: Jonathan Cormier <jcormier@...ticallink.com>
> Signed-off-by: Jerome Neanne <jneanne@...libre.com>
> ---
> MAINTAINERS | 1 +
> drivers/gpio/Kconfig | 17 +++++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-tps65219.c | 173 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 192 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c0cde28c62c6..d912b7465e84 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15398,6 +15398,7 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git
> F: arch/arm/configs/omap2plus_defconfig
> F: arch/arm/mach-omap2/
> F: drivers/bus/ti-sysc.c
> +F: drivers/gpio/gpio-tps65219.c
> F: drivers/i2c/busses/i2c-omap.c
> F: drivers/irqchip/irq-omap-intc.c
> F: drivers/mfd/*omap*.c
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 5521f060d58e..f4e37881d01a 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1440,6 +1440,23 @@ config GPIO_TPS65218
> Select this option to enable GPIO driver for the TPS65218
> chip family.
>
> +config GPIO_TPS65219
> + tristate "TPS65219 GPIO"
> + depends on MFD_TPS65219
> + default MFD_TPS65219
> + help
> + Select this option to enable GPIO driver for the TPS65219
> + chip family.
> + GPIO0 is statically configured as input or output prior to Linux boot.
> + it is used for MULTI_DEVICE_ENABLE function.
> + This setting is statically configured by NVM.
> + GPIO0 can't be used as a generic GPIO.
> + It's either a GPO when MULTI_DEVICE_EN=0,
> + or a GPI when MULTI_DEVICE_EN=1.
> +
> + This driver can also be built as a module. If so, the
> + module will be called gpio_tps65219.
> +
> config GPIO_TPS6586X
> bool "TPS6586X GPIO"
> depends on MFD_TPS6586X
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 20036af3acb1..7843b16f5d59 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -160,6 +160,7 @@ obj-$(CONFIG_GPIO_TN48M_CPLD) += gpio-tn48m.o
> obj-$(CONFIG_GPIO_TPIC2810) += gpio-tpic2810.o
> obj-$(CONFIG_GPIO_TPS65086) += gpio-tps65086.o
> obj-$(CONFIG_GPIO_TPS65218) += gpio-tps65218.o
> +obj-$(CONFIG_GPIO_TPS65219) += gpio-tps65219.o
> obj-$(CONFIG_GPIO_TPS6586X) += gpio-tps6586x.o
> obj-$(CONFIG_GPIO_TPS65910) += gpio-tps65910.o
> obj-$(CONFIG_GPIO_TPS65912) += gpio-tps65912.o
> diff --git a/drivers/gpio/gpio-tps65219.c b/drivers/gpio/gpio-tps65219.c
> new file mode 100644
> index 000000000000..42bbd142e4b6
> --- /dev/null
> +++ b/drivers/gpio/gpio-tps65219.c
> @@ -0,0 +1,173 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * GPIO driver for TI TPS65219 PMICs
> + *
> + * Copyright (C) 2022 Texas Instruments Incorporated - http://www.ti.com/
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/gpio/driver.h>
> +#include <linux/mfd/tps65219.h>
> +
Please keep all includes together and ordered alphabetically. I see
you probably wanted to split them thematically but we don't really do
this.
> +#define TPS65219_GPIO0_DIR_MASK BIT(3)
> +#define TPS65219_GPIO0_OFFSET 2
> +#define TPS65219_GPIO0_IDX 0
> +#define TPS65219_GPIO_DIR_IN 1
> +#define TPS65219_GPIO_DIR_OUT 0
> +
> +struct tps65219_gpio {
> + struct gpio_chip gpio_chip;
> + struct tps65219 *tps;
> +};
> +
> +static int tps65219_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct tps65219_gpio *gpio = gpiochip_get_data(gc);
> + int ret, val;
> +
> + if (offset != TPS65219_GPIO0_IDX)
> + return GPIO_LINE_DIRECTION_OUT;
> +
> + ret = regmap_read(gpio->tps->regmap, TPS65219_REG_MFP_1_CONFIG, &val);
> + if (ret)
> + return ret;
> +
> + return !!(val & TPS65219_GPIO0_DIR_MASK);
> +}
> +
> +static int tps65219_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct tps65219_gpio *gpio = gpiochip_get_data(gc);
> + int ret, val;
> +
> + if (offset != TPS65219_GPIO0_IDX) {
> + dev_err(gpio->tps->dev,
> + "GPIO%d is output only, cannot get\n",
> + offset);
> + return -EOPNOTSUPP;
> + }
> +
> + ret = regmap_read(gpio->tps->regmap, TPS65219_REG_MFP_CTRL, &val);
> + if (ret)
> + return ret;
Add newline here.
> + dev_warn(gpio->tps->dev,
> + "GPIO%d = %d, used for MULTI_DEVICE_ENABLE, not as standard GPIO\n",
> + offset, !!(val & BIT(TPS65219_MFP_GPIO_STATUS_MASK)));
> +
> + /* depends on NVM config return error if dir output else the GPIO0 status bit */
> + if (tps65219_gpio_get_direction(gc, offset) == TPS65219_GPIO_DIR_OUT)
> + return -EOPNOTSUPP;
Add newline here.
> + return !!(val & BIT(TPS65219_MFP_GPIO_STATUS_MASK));
> +}
> +
> +static void tps65219_gpio_set(struct gpio_chip *gc, unsigned int offset,
> + int value)
> +{
> + struct tps65219_gpio *gpio = gpiochip_get_data(gc);
> + int v, mask, bit;
> +
> + bit = (offset == TPS65219_GPIO0_IDX) ? TPS65219_GPIO0_OFFSET : offset - 1;
> +
> + mask = BIT(bit);
> + v = value ? mask : 0;
> +
> + regmap_update_bits(gpio->tps->regmap,
> + TPS65219_REG_GENERAL_CONFIG,
> + mask, v);
This can fail - I suggest emitting an error message as regmap won't do it.
> +}
> +
> +static int tps65219_gpio_change_direction(struct gpio_chip *gc, unsigned int offset,
> + unsigned int direction)
> +{
> + struct tps65219_gpio *gpio = gpiochip_get_data(gc);
> +
> + /* Documentation is stating that GPIO0 direction must not be changed in Linux:
> + * Table 8-34. MFP_1_CONFIG(3): MULTI_DEVICE_ENABLE,
> + * Should only be changed in INITIALIZE state (prior to ON Request).
> + * Set statically by NVM, changing direction in application can cause a hang.
> + * Below can be used for test purpose only:
> + */
> +
> +#if 0
> + int ret = regmap_update_bits(gpio->tps->regmap, TPS65219_REG_MFP_1_CONFIG,
> + TPS65219_GPIO0_DIR_MASK, direction);
> + if (ret)
> + return ret;
> +#endif
Agree with Linus here on enabling it for DEBUG.
And a newline here.
> + dev_err(gpio->tps->dev,
> + "GPIO%d direction set by NVM, change to %u failed, not allowed by specification\n",
> + offset, direction);
And before return.
> + return -EOPNOTSUPP;
> +}
> +
> +static int tps65219_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct tps65219_gpio *gpio = gpiochip_get_data(gc);
> +
> + if (offset != TPS65219_GPIO0_IDX) {
> + dev_err(gpio->tps->dev,
> + "GPIO%d is output only, cannot change to input\n",
> + offset);
> + return -EOPNOTSUPP;
> + }
Newline here.
> + if (tps65219_gpio_get_direction(gc, offset) == TPS65219_GPIO_DIR_IN)
> + return 0;
and here.
> + return tps65219_gpio_change_direction(gc, offset, TPS65219_GPIO_DIR_IN);
> +}
> +
> +static int tps65219_gpio_direction_output(struct gpio_chip *gc, unsigned int offset,
> + int value)
> +{
> + tps65219_gpio_set(gc, offset, value);
> + if (offset != TPS65219_GPIO0_IDX)
> + return 0;
> +
> + if (tps65219_gpio_get_direction(gc, offset) == TPS65219_GPIO_DIR_OUT)
> + return 0;
and here.
> + return tps65219_gpio_change_direction(gc, offset, TPS65219_GPIO_DIR_OUT);
> +}
> +
> +static const struct gpio_chip tps65219_gpio_chip = {
> + .label = "tps65219-gpio",
> + .owner = THIS_MODULE,
> + .get_direction = tps65219_gpio_get_direction,
> + .direction_input = tps65219_gpio_direction_input,
> + .direction_output = tps65219_gpio_direction_output,
> + .get = tps65219_gpio_get,
> + .set = tps65219_gpio_set,
> + .base = -1,
> + .ngpio = 3,
> + .can_sleep = true,
> +};
> +
> +static int tps65219_gpio_probe(struct platform_device *pdev)
> +{
> + struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
> + struct tps65219_gpio *gpio;
> +
> + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
> +
> + gpio->tps = tps;
> + gpio->gpio_chip = tps65219_gpio_chip;
Aren't you getting any warnings here about dropping the 'const' from
the global structure?
> + gpio->gpio_chip.parent = tps->dev;
> +
> + return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio_chip, gpio);
> +}
> +
> +static struct platform_driver tps65219_gpio_driver = {
> + .driver = {
> + .name = "tps65219-gpio",
> + },
> + .probe = tps65219_gpio_probe,
> +};
> +module_platform_driver(tps65219_gpio_driver);
> +
> +MODULE_ALIAS("platform:tps65219-gpio");
> +MODULE_AUTHOR("Jonathan Cormier <jcormier@...ticallink.com>");
> +MODULE_DESCRIPTION("TPS65219 GPIO driver");
> +MODULE_LICENSE("GPL");
>
Otherwise looks good, just a couple nits.
Bart
> --
> 2.34.1
>
Powered by blists - more mailing lists