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

[Aidan MacDonald <aidanmacdonald.0x0@xxxxxxxxx>]

Samuel Holland <samuel@xxxxxxxxxxxx> writes:

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

I'd assume variants getting new GPIO support would be better off using
this driver. The existing one supports only AXP209 and AXP803/813, and
it hardcodes far too many details.

I'll let the maintainers decide whether converting the existing driver
is better than dealing with possible future migrations. If conversion
is the way to go then I'll drop this patch and revisit GPIO support at
a later date.

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

You're right, thanks for pointing that out... guess I should actually
read the documentation sometimes!

>> +		}
>> +	}
>> +
>> +	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