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

Samuel Holland
2022-07-02T15:11:54Z

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@gmail.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