On Sat, Jun 18, 2022 at 11:40 PM Aidan MacDonald
<aidanmacdonald.0x0@xxxxxxxxx> 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.
...
> +config PINCTRL_AXP192
> + tristate "X-Powers AXP192 PMIC pinctrl and GPIO Support"
> + depends on MFD_AXP20X
> + depends on OF
Why?
> + select PINMUX
> + select GENERIC_PINCONF
> + select GPIOLIB
...
> +#include <linux/of.h>
> +#include <linux/of_device.h>
Why?
...
> +struct axp192_pctl_function {
> + const char *name;
> + /* Mux value written to the control register to select the function (-1 if unsupported) */
Comment is misleading. -1 can't be a value of unsigned type.
> + const u8 *muxvals;
> + const char * const *groups;
> + unsigned int ngroups;
> +};
...
> +struct axp192_pctl_desc {
> + unsigned int npins;
> + const struct pinctrl_pin_desc *pins;
> + /* Description of the function control register for each pin */
> + const struct axp192_pctl_reg_info *ctrl_regs;
> + /* Description of the output signal register for each pin */
> + const struct axp192_pctl_reg_info *out_regs;
> + /* Description of the input signal register for each pin */
> + const struct axp192_pctl_reg_info *in_regs;
> + /* Description of the pull down resistor config register for each pin */
Can you just convert these comments to a kernel-doc?
> + const struct axp192_pctl_reg_info *pull_down_regs;
> +
> + unsigned int nfunctions;
> + const struct axp192_pctl_function *functions;
> +};
...
> +
> +
One blank line is enough.
...
> + switch (param) {
> + case PIN_CONFIG_BIAS_DISABLE:
> + ret = axp192_pinconf_get_pull_down(pctldev, pin);
> + if (ret < 0)
> + return ret;
> + else if (ret != 0)
1. Redundant 'else'
2. if (ret > 0)
> + return -EINVAL;
> + break;
> +
> + case PIN_CONFIG_BIAS_PULL_DOWN:
> + ret = axp192_pinconf_get_pull_down(pctldev, pin);
> + if (ret < 0)
> + return ret;
> + else if (ret == 0)
Ditto.
Looking at this I would rather expect the function to return something
defined, than 0, non-0.
> + return -EINVAL;
> + break;
> + default:
> + return -ENOTSUPP;
> + }
...
> + for (cfg = 0; cfg < num_configs; ++cfg) {
cfg++ will work the same way and easier to read.
> + 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;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static const struct pinconf_ops axp192_conf_ops = {
> + .is_generic = true,
> + .pin_config_get = axp192_pinconf_get,
> + .pin_config_set = axp192_pinconf_set,
> + .pin_config_group_get = axp192_pinconf_get,
> + .pin_config_group_set = axp192_pinconf_set,
> +};
> +
> +static int axp192_pmx_set(struct pinctrl_dev *pctldev, unsigned int offset, u8 config)
> +{
> + struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
> + const struct axp192_pctl_reg_info *reginfo = &pctl->desc->ctrl_regs[offset];
> + unsigned int regval = config << (ffs(reginfo->mask) - 1);
> +
> + return regmap_update_bits(pctl->regmap, reginfo->reg, reginfo->mask, regval);
> +}
> +
> +static int axp192_pmx_func_cnt(struct pinctrl_dev *pctldev)
> +{
> + struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
> +
> + return pctl->desc->nfunctions;
> +}
> +
> +static const char *axp192_pmx_func_name(struct pinctrl_dev *pctldev, unsigned int selector)
> +{
> + struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
> +
> + return pctl->desc->functions[selector].name;
> +}
> +
> +static int axp192_pmx_func_groups(struct pinctrl_dev *pctldev, unsigned int selector,
> + const char * const **groups, unsigned int *num_groups)
> +{
> + struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
> +
> + *groups = pctl->desc->functions[selector].groups;
> + *num_groups = pctl->desc->functions[selector].ngroups;
> +
> + return 0;
> +}
> +
> +static int axp192_pmx_set_mux(struct pinctrl_dev *pctldev,
> + unsigned int function, unsigned int group)
> +{
> + struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
> + const u8 *muxvals = pctl->desc->functions[function].muxvals;
> +
> + if (muxvals[group] == U8_MAX)
> + return -EINVAL;
> +
> + /*
> + * Switching to LDO or PWM function will enable LDO/PWM output, so it's
> + * better to ignore these requests and let the regulator or PWM drivers
> + * handle muxing to avoid interfering with them.
> + */
> + if (function == AXP192_FUNC_LDO || function == AXP192_FUNC_PWM)
> + return 0;
> +
> + return axp192_pmx_set(pctldev, group, muxvals[group]);
> +}
> +
> +static int axp192_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
> + struct pinctrl_gpio_range *range,
> + unsigned int offset, bool input)
> +{
> + struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
> + const u8 *muxvals = input ? pctl->desc->functions[AXP192_FUNC_INPUT].muxvals
> + : pctl->desc->functions[AXP192_FUNC_OUTPUT].muxvals;
> +
> + return axp192_pmx_set(pctldev, offset, muxvals[offset]);
> +}
> +
> +static const struct pinmux_ops axp192_pmx_ops = {
> + .get_functions_count = axp192_pmx_func_cnt,
> + .get_function_name = axp192_pmx_func_name,
> + .get_function_groups = axp192_pmx_func_groups,
> + .set_mux = axp192_pmx_set_mux,
> + .gpio_set_direction = axp192_pmx_gpio_set_direction,
> + .strict = true,
> +};
> +
> +static int axp192_groups_cnt(struct pinctrl_dev *pctldev)
> +{
> + struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
> +
> + return pctl->desc->npins;
> +}
> +
> +static const char *axp192_group_name(struct pinctrl_dev *pctldev, unsigned int selector)
> +{
> + struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
> +
> + return pctl->desc->pins[selector].name;
> +}
> +
> +static int axp192_group_pins(struct pinctrl_dev *pctldev, unsigned int selector,
> + const unsigned int **pins, unsigned int *num_pins)
> +{
> + struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
> +
> + *pins = &pctl->desc->pins[selector].number;
> + *num_pins = 1;
> +
> + return 0;
> +}
> +
> +static const struct pinctrl_ops axp192_pctrl_ops = {
> + .dt_node_to_map = pinconf_generic_dt_node_to_map_group,
> + .dt_free_map = pinconf_generic_dt_free_map,
> + .get_groups_count = axp192_groups_cnt,
> + .get_group_name = axp192_group_name,
> + .get_group_pins = axp192_group_pins,
> +};
> +
> +static int axp192_pctl_probe(struct platform_device *pdev)
> +{
> + struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> + struct axp192_pctl *pctl;
> + struct pinctrl_desc *pctrl_desc;
> + int ret, i;
> +
> + pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
> + if (!pctl)
> + return -ENOMEM;
> +
> + pctl->desc = device_get_match_data(&pdev->dev);
> + pctl->regmap = axp20x->regmap;
> + pctl->regmap_irqc = axp20x->regmap_irqc;
> + pctl->dev = &pdev->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 = &pdev->dev;
> + pctl->chip.label = dev_name(&pdev->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(&pdev->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(&pdev->dev, sizeof(*pctrl_desc), GFP_KERNEL);
> + if (!pctrl_desc)
> + return -ENOMEM;
> +
> + pctrl_desc->name = dev_name(&pdev->dev);
> + pctrl_desc->owner = THIS_MODULE;
> + pctrl_desc->pins = pctl->desc->pins;
> + pctrl_desc->npins = pctl->desc->npins;
> + pctrl_desc->pctlops = &axp192_pctrl_ops;
> + pctrl_desc->pmxops = &axp192_pmx_ops;
> + pctrl_desc->confops = &axp192_conf_ops;
> +
> + pctl->pctl_dev = devm_pinctrl_register(&pdev->dev, pctrl_desc, pctl);
> + if (IS_ERR(pctl->pctl_dev))
> + dev_err_probe(&pdev->dev, PTR_ERR(pctl->pctl_dev),
> + "couldn't register pinctrl driver\n");
> +
> + ret = devm_gpiochip_add_data(&pdev->dev, &pctl->chip, pctl);
> + if (ret)
> + dev_err_probe(&pdev->dev, ret, "Failed to register GPIO chip\n");
> +
> + return 0;
> +}
> +
> +static const struct of_device_id axp192_pctl_match[] = {
> + { .compatible = "x-powers,axp192-gpio", .data = &axp192_data, },
> + { }
> +};
--
With Best Regards,
Andy Shevchenko
