linux-kernel - Re: [PATCH v4 12/15] pinctrl: Add AXP192 pin control driver

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <37d40cf2-4512-754f-2e44-ee1449bc2e9f@sholland.org>
Date:   Sat, 2 Jul 2022 10:11:43 -0500
From:   Samuel Holland <samuel@...lland.org>
To:     Aidan MacDonald <aidanmacdonald.0x0@...il.com>, wens@...e.org
Cc:     linus.walleij@...aro.org, brgl@...ev.pl, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, jic23@...nel.org,
        sre@...nel.org, lee.jones@...aro.org, lgirdwood@...il.com,
        broonie@...nel.org, lars@...afoo.de, quic_gurus@...cinc.com,
        sebastian.reichel@...labora.com, andy.shevchenko@...il.com,
        michael@...le.cc, rdunlap@...radead.org,
        linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
        linux-pm@...r.kernel.org
Subject: Re: [PATCH v4 12/15] pinctrl: Add AXP192 pin control driver

On 6/29/22 9:30 AM, Aidan MacDonald wrote:
> The AXP192 PMIC's GPIO registers are much different from the GPIO
> registers of the AXP20x and AXP813 PMICs supported by the existing
> pinctrl-axp209 driver. It makes more sense to add a new driver for
> the AXP192, rather than add support in the existing axp20x driver.
> 
> The pinctrl-axp192 driver is considerably more flexible in terms of
> register layout and should be able to support other X-Powers PMICs.
> Interrupts and pull down resistor configuration are supported too.

I am planning to implement gpio/pinctrl support for AXP152[1], which is
somewhere between AXP20x and AXP192 in terms of GPIO capability.

Which driver should I add it to? How much work would it be to convert AXP20x
variants to the new driver? And if supporting other X-Powers PMICs is the plan,
would it make sense to convert the existing driver in-place to avoid dealing
with Kconfig migrations?

[1]: https://linux-sunxi.org/AXP152

> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@...il.com>
> ---
>  drivers/pinctrl/Kconfig          |  13 +
>  drivers/pinctrl/Makefile         |   1 +
>  drivers/pinctrl/pinctrl-axp192.c | 598 +++++++++++++++++++++++++++++++
>  3 files changed, 612 insertions(+)
>  create mode 100644 drivers/pinctrl/pinctrl-axp192.c
> [...]
> +static int axp192_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin, unsigned long *config)
> +{
> +	enum pin_config_param param = pinconf_to_config_param(*config);
> +	unsigned int arg = 1;
> +	bool pull_down;
> +	int ret;
> +
> +	switch (param) {
> +	case PIN_CONFIG_BIAS_DISABLE:
> +		ret = axp192_pinconf_get_pull_down(pctldev, pin, &pull_down);
> +		if (ret)
> +			return ret;
> +		if (pull_down)
> +			return -EINVAL;
> +		break;
> +
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +		ret = axp192_pinconf_get_pull_down(pctldev, pin, &pull_down);
> +		if (ret)
> +			return ret;
> +		if (!pull_down)
> +			return -EINVAL;
> +		break;
> +
> +	default:
> +		return -ENOTSUPP;
> +	}
> +
> +	*config = pinconf_to_config_packed(param, arg);
> +	return 0;
> +}
> +
> +static int axp192_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> +			      unsigned long *configs, unsigned int num_configs)
> +{
> +	int ret;
> +	unsigned int cfg;
> +
> +	for (cfg = 0; cfg < num_configs; cfg++) {
> +		switch (pinconf_to_config_param(configs[cfg])) {
> +		case PIN_CONFIG_BIAS_DISABLE:
> +			ret = axp192_pinconf_set_pull_down(pctldev, pin, 0);
> +			if (ret)
> +				return ret;
> +			break;
> +
> +		case PIN_CONFIG_BIAS_PULL_DOWN:
> +			ret = axp192_pinconf_set_pull_down(pctldev, pin, 1);
> +			if (ret)
> +				return ret;
> +			break;
> +
> +		default:
> +			return -ENOTSUPP;

The GPIO outputs are always open-drain. It looks like this needs to handle
PIN_CONFIG_DRIVE_OPEN_DRAIN, or gpiolib will try to emulate it.

And I would suggest returning -EINVAL for PIN_CONFIG_DRIVE_PUSH_PULL, but
gpiolib does not check the return value when setting that.

> +		}
> +	}
> +
> +	return 0;
> +}
> +
> [...]
> +
> +static int axp192_pctl_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct axp20x_dev *axp20x = dev_get_drvdata(dev->parent);
> +	struct axp192_pctl *pctl;
> +	struct pinctrl_desc *pctrl_desc;
> +	int ret, i;
> +
> +	pctl = devm_kzalloc(dev, sizeof(*pctl), GFP_KERNEL);
> +	if (!pctl)
> +		return -ENOMEM;
> +
> +	pctl->desc = device_get_match_data(dev);
> +	pctl->regmap = axp20x->regmap;
> +	pctl->regmap_irqc = axp20x->regmap_irqc;
> +	pctl->dev = dev;
> +
> +	pctl->chip.base			= -1;
> +	pctl->chip.can_sleep		= true;
> +	pctl->chip.request		= gpiochip_generic_request;
> +	pctl->chip.free			= gpiochip_generic_free;
> +	pctl->chip.parent		= dev;
> +	pctl->chip.label		= dev_name(dev);
> +	pctl->chip.owner		= THIS_MODULE;
> +	pctl->chip.get			= axp192_gpio_get;
> +	pctl->chip.get_direction	= axp192_gpio_get_direction;
> +	pctl->chip.set			= axp192_gpio_set;
> +	pctl->chip.direction_input	= axp192_gpio_direction_input;
> +	pctl->chip.direction_output	= axp192_gpio_direction_output;
> +	pctl->chip.to_irq		= axp192_gpio_to_irq;
> +	pctl->chip.ngpio		= pctl->desc->npins;
> +
> +	pctl->irqs = devm_kcalloc(dev, pctl->desc->npins, sizeof(int), GFP_KERNEL);
> +	if (!pctl->irqs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < pctl->desc->npins; i++) {
> +		ret = platform_get_irq_byname_optional(pdev, pctl->desc->pins[i].name);
> +		if (ret > 0)
> +			pctl->irqs[i] = ret;
> +	}
> +
> +	platform_set_drvdata(pdev, pctl);
> +
> +	pctrl_desc = devm_kzalloc(dev, sizeof(*pctrl_desc), GFP_KERNEL);
> +	if (!pctrl_desc)
> +		return -ENOMEM;

This can go inside struct axp192_pctl. It does not need a separate allocation.

Regards,
Samuel