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@xxxxxxxxx> > --- > 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
