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.
Signed-off-by: Aidan MacDonald <[email protected]>
---
drivers/pinctrl/Kconfig | 14 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-axp192.c | 589 +++++++++++++++++++++++++++++++
3 files changed, 604 insertions(+)
create mode 100644 drivers/pinctrl/pinctrl-axp192.c
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index f52960d2dfbe..a71e35de333d 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -113,6 +113,20 @@ config PINCTRL_AT91PIO4
Say Y here to enable the at91 pinctrl/gpio driver for Atmel PIO4
controller available on sama5d2 SoC.
+config PINCTRL_AXP192
+ tristate "X-Powers AXP192 PMIC pinctrl and GPIO Support"
+ depends on MFD_AXP20X
+ depends on OF
+ select PINMUX
+ select GENERIC_PINCONF
+ select GPIOLIB
+ help
+ AXP PMICs provides multiple GPIOs that can be muxed for different
+ functions. This driver bundles a pinctrl driver to select the function
+ muxing and a GPIO driver to handle the GPIO when the GPIO function is
+ selected.
+ Say Y to enable pinctrl and GPIO support for the AXP192 PMIC.
+
config PINCTRL_AXP209
tristate "X-Powers AXP209 PMIC pinctrl and GPIO Support"
depends on MFD_AXP20X
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index e76f5cdc64b0..9d2b6420c5dd 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_PINCTRL_ARTPEC6) += pinctrl-artpec6.o
obj-$(CONFIG_PINCTRL_AS3722) += pinctrl-as3722.o
obj-$(CONFIG_PINCTRL_AT91) += pinctrl-at91.o
obj-$(CONFIG_PINCTRL_AT91PIO4) += pinctrl-at91-pio4.o
+obj-$(CONFIG_PINCTRL_AXP192) += pinctrl-axp192.o
obj-$(CONFIG_PINCTRL_AXP209) += pinctrl-axp209.o
obj-$(CONFIG_PINCTRL_BM1880) += pinctrl-bm1880.o
obj-$(CONFIG_PINCTRL_DA850_PUPD) += pinctrl-da850-pupd.o
diff --git a/drivers/pinctrl/pinctrl-axp192.c b/drivers/pinctrl/pinctrl-axp192.c
new file mode 100644
index 000000000000..0ff2d0b84978
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-axp192.c
@@ -0,0 +1,589 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * AXP192 pinctrl and GPIO driver
+ *
+ * Copyright (C) 2022 Aidan MacDonald <[email protected]>
+ */
+
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mfd/axp20x.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+enum {
+ AXP192_FUNC_OUTPUT = 0,
+ AXP192_FUNC_INPUT,
+ AXP192_FUNC_LDO,
+ AXP192_FUNC_PWM,
+ AXP192_FUNC_ADC,
+ AXP192_FUNC_LOW_OUTPUT,
+ AXP192_FUNC_FLOATING,
+ AXP192_FUNC_EXT_CHG_CTL,
+ AXP192_FUNC_LDO_STATUS,
+ AXP192_FUNCS_NB,
+};
+
+struct axp192_pctl_function {
+ const char *name;
+ /* Mux value written to the control register to select the function (-1 if unsupported) */
+ const u8 *muxvals;
+ const char * const *groups;
+ unsigned int ngroups;
+};
+
+struct axp192_pctl_reg_info {
+ u8 reg;
+ u8 mask;
+};
+
+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 */
+ const struct axp192_pctl_reg_info *pull_down_regs;
+
+ unsigned int nfunctions;
+ const struct axp192_pctl_function *functions;
+};
+
+static const struct pinctrl_pin_desc axp192_pins[] = {
+ PINCTRL_PIN(0, "GPIO0"),
+ PINCTRL_PIN(1, "GPIO1"),
+ PINCTRL_PIN(2, "GPIO2"),
+ PINCTRL_PIN(3, "GPIO3"),
+ PINCTRL_PIN(4, "GPIO4"),
+ PINCTRL_PIN(5, "N_RSTO"),
+};
+
+static const char * const axp192_io_groups[] = { "GPIO0", "GPIO1", "GPIO2",
+ "GPIO3", "GPIO4", "N_RSTO" };
+static const char * const axp192_ldo_groups[] = { "GPIO0" };
+static const char * const axp192_pwm_groups[] = { "GPIO1", "GPIO2" };
+static const char * const axp192_adc_groups[] = { "GPIO0", "GPIO1", "GPIO2", "GPIO3" };
+static const char * const axp192_extended_io_groups[] = { "GPIO0", "GPIO1", "GPIO2" };
+static const char * const axp192_ext_chg_ctl_groups[] = { "GPIO3", "GPIO4" };
+static const char * const axp192_ldo_status_groups[] = { "N_RSTO" };
+
+static const u8 axp192_output_muxvals[] = { 0, 0, 0, 1, 1, 2 };
+static const u8 axp192_input_muxvals[] = { 1, 1, 1, 2, 2, 3 };
+static const u8 axp192_ldo_muxvals[] = { 2, -1, -1, -1, -1, -1 };
+static const u8 axp192_pwm_muxvals[] = { -1, 2, 2, -1, -1, -1 };
+static const u8 axp192_adc_muxvals[] = { 4, 4, 4, 3, -1, -1 };
+static const u8 axp192_low_output_muxvals[] = { 5, 5, 5, -1, -1, -1 };
+static const u8 axp192_floating_muxvals[] = { 6, 6, 6, -1, -1, -1 };
+static const u8 axp192_ext_chg_ctl_muxvals[] = { -1, -1, -1, 0, 0, -1 };
+static const u8 axp192_ldo_status_muxvals[] = { -1, -1, -1, -1, -1, 0 };
+
+static const struct axp192_pctl_function axp192_functions[AXP192_FUNCS_NB] = {
+ [AXP192_FUNC_OUTPUT] = {
+ .name = "output",
+ .muxvals = axp192_output_muxvals,
+ .groups = axp192_io_groups,
+ .ngroups = ARRAY_SIZE(axp192_io_groups),
+ },
+ [AXP192_FUNC_INPUT] = {
+ .name = "input",
+ .muxvals = axp192_input_muxvals,
+ .groups = axp192_io_groups,
+ .ngroups = ARRAY_SIZE(axp192_io_groups),
+ },
+ [AXP192_FUNC_LDO] = {
+ .name = "ldo",
+ .muxvals = axp192_ldo_muxvals,
+ .groups = axp192_ldo_groups,
+ .ngroups = ARRAY_SIZE(axp192_ldo_groups),
+ },
+ [AXP192_FUNC_PWM] = {
+ .name = "pwm",
+ .muxvals = axp192_pwm_muxvals,
+ .groups = axp192_pwm_groups,
+ .ngroups = ARRAY_SIZE(axp192_pwm_groups),
+ },
+ [AXP192_FUNC_ADC] = {
+ .name = "adc",
+ .muxvals = axp192_adc_muxvals,
+ .groups = axp192_adc_groups,
+ .ngroups = ARRAY_SIZE(axp192_adc_groups),
+ },
+ [AXP192_FUNC_LOW_OUTPUT] = {
+ .name = "low_output",
+ .muxvals = axp192_low_output_muxvals,
+ .groups = axp192_extended_io_groups,
+ .ngroups = ARRAY_SIZE(axp192_extended_io_groups),
+ },
+ [AXP192_FUNC_FLOATING] = {
+ .name = "floating",
+ .muxvals = axp192_floating_muxvals,
+ .groups = axp192_extended_io_groups,
+ .ngroups = ARRAY_SIZE(axp192_extended_io_groups),
+ },
+ [AXP192_FUNC_EXT_CHG_CTL] = {
+ .name = "ext_chg_ctl",
+ .muxvals = axp192_ext_chg_ctl_muxvals,
+ .groups = axp192_ext_chg_ctl_groups,
+ .ngroups = ARRAY_SIZE(axp192_ext_chg_ctl_groups),
+ },
+ [AXP192_FUNC_LDO_STATUS] = {
+ .name = "ldo_status",
+ .muxvals = axp192_ldo_status_muxvals,
+ .groups = axp192_ldo_groups,
+ .ngroups = ARRAY_SIZE(axp192_ldo_status_groups),
+ },
+};
+
+static const struct axp192_pctl_reg_info axp192_pin_ctrl_regs[] = {
+ { .reg = AXP192_GPIO0_CTRL, .mask = 0x07 },
+ { .reg = AXP192_GPIO1_CTRL, .mask = 0x07 },
+ { .reg = AXP192_GPIO2_CTRL, .mask = 0x07 },
+ { .reg = AXP192_GPIO4_3_CTRL, .mask = 0x03 },
+ { .reg = AXP192_GPIO4_3_CTRL, .mask = 0x0c },
+ { .reg = AXP192_N_RSTO_CTRL, .mask = 0xc0 },
+};
+
+static const struct axp192_pctl_reg_info axp192_pin_in_regs[] = {
+ { .reg = AXP192_GPIO2_0_STATE, .mask = BIT(4) },
+ { .reg = AXP192_GPIO2_0_STATE, .mask = BIT(5) },
+ { .reg = AXP192_GPIO2_0_STATE, .mask = BIT(6) },
+ { .reg = AXP192_GPIO4_3_STATE, .mask = BIT(4) },
+ { .reg = AXP192_GPIO4_3_STATE, .mask = BIT(5) },
+ { .reg = AXP192_N_RSTO_CTRL, .mask = BIT(4) },
+};
+
+static const struct axp192_pctl_reg_info axp192_pin_out_regs[] = {
+ { .reg = AXP192_GPIO2_0_STATE, .mask = BIT(0) },
+ { .reg = AXP192_GPIO2_0_STATE, .mask = BIT(1) },
+ { .reg = AXP192_GPIO2_0_STATE, .mask = BIT(2) },
+ { .reg = AXP192_GPIO4_3_STATE, .mask = BIT(0) },
+ { .reg = AXP192_GPIO4_3_STATE, .mask = BIT(1) },
+ { .reg = AXP192_N_RSTO_CTRL, .mask = BIT(5) },
+};
+
+static const struct axp192_pctl_reg_info axp192_pull_down_regs[] = {
+ { .reg = AXP192_GPIO2_0_PULL, .mask = BIT(0) },
+ { .reg = AXP192_GPIO2_0_PULL, .mask = BIT(1) },
+ { .reg = AXP192_GPIO2_0_PULL, .mask = BIT(2) },
+ { .reg = 0, .mask = 0 /* unsupported */ },
+ { .reg = 0, .mask = 0 /* unsupported */ },
+ { .reg = 0, .mask = 0 /* unsupported */ },
+};
+
+static const struct axp192_pctl_desc axp192_data = {
+ .npins = ARRAY_SIZE(axp192_pins),
+ .pins = axp192_pins,
+ .ctrl_regs = axp192_pin_ctrl_regs,
+ .out_regs = axp192_pin_out_regs,
+ .in_regs = axp192_pin_in_regs,
+ .pull_down_regs = axp192_pull_down_regs,
+
+ .nfunctions = ARRAY_SIZE(axp192_functions),
+ .functions = axp192_functions,
+};
+
+
+struct axp192_pctl {
+ struct gpio_chip chip;
+ struct regmap *regmap;
+ struct pinctrl_dev *pctl_dev;
+ struct device *dev;
+ const struct axp192_pctl_desc *desc;
+ int *irqs;
+};
+
+static int axp192_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+ struct axp192_pctl *pctl = gpiochip_get_data(chip);
+ const struct axp192_pctl_reg_info *reginfo = &pctl->desc->in_regs[offset];
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(pctl->regmap, reginfo->reg, &val);
+ if (ret)
+ return ret;
+
+ return !!(val & reginfo->mask);
+}
+
+static int axp192_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+ struct axp192_pctl *pctl = gpiochip_get_data(chip);
+ const struct axp192_pctl_reg_info *reginfo = &pctl->desc->ctrl_regs[offset];
+ const u8 *input_muxvals = pctl->desc->functions[AXP192_FUNC_INPUT].muxvals;
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(pctl->regmap, reginfo->reg, &val);
+ if (ret)
+ return ret;
+
+ if ((val & reginfo->mask) == (input_muxvals[offset] << (ffs(reginfo->mask) - 1)))
+ return GPIO_LINE_DIRECTION_IN;
+ else
+ return GPIO_LINE_DIRECTION_OUT;
+}
+
+static void axp192_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+ struct axp192_pctl *pctl = gpiochip_get_data(chip);
+ const struct axp192_pctl_reg_info *reginfo = &pctl->desc->out_regs[offset];
+
+ regmap_update_bits(pctl->regmap, reginfo->reg, reginfo->mask, value ? reginfo->mask : 0);
+}
+
+static int axp192_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+ return pinctrl_gpio_direction_input(chip->base + offset);
+}
+
+static int axp192_gpio_direction_output(struct gpio_chip *chip, unsigned int offset, int value)
+{
+ chip->set(chip, offset, value);
+ return 0;
+}
+
+static int axp192_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+ struct axp192_pctl *pctl = gpiochip_get_data(chip);
+
+ return pctl->irqs[offset];
+}
+
+static int axp192_pinconf_get_pull_down(struct pinctrl_dev *pctldev, unsigned int pin)
+{
+ struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
+ const struct axp192_pctl_reg_info *reginfo = &pctl->desc->pull_down_regs[pin];
+ unsigned int val;
+ int ret;
+
+ if (!reginfo->mask)
+ return -EOPNOTSUPP;
+
+ ret = regmap_read(pctl->regmap, reginfo->reg, &val);
+ if (ret)
+ return ret;
+
+ return !!(val & reginfo->mask);
+}
+
+static int axp192_pinconf_set_pull_down(struct pinctrl_dev *pctldev, unsigned int pin, int value)
+{
+ struct axp192_pctl *pctl = pinctrl_dev_get_drvdata(pctldev);
+ const struct axp192_pctl_reg_info *reginfo = &pctl->desc->pull_down_regs[pin];
+
+ if (!reginfo->mask)
+ return -EOPNOTSUPP;
+
+ return regmap_update_bits(pctl->regmap, reginfo->reg, reginfo->mask,
+ value ? reginfo->mask : 0);
+}
+
+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;
+ int ret;
+
+ switch (param) {
+ case PIN_CONFIG_BIAS_DISABLE:
+ ret = axp192_pinconf_get_pull_down(pctldev, pin);
+ if (ret < 0)
+ return ret;
+ else 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)
+ return -EINVAL;
+ break;
+
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ *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:
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ continue;
+ default:
+ return -EOPNOTSUPP;
+ }
+ }
+
+ 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:
+ /* unreachable */
+ break;
+ }
+ }
+
+ 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)-1)
+ 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;
+
+ if (!of_device_is_available(pdev->dev.of_node))
+ return -ENODEV;
+
+ if (!axp20x) {
+ dev_err(&pdev->dev, "Parent drvdata not set\n");
+ return -EINVAL;
+ }
+
+ pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
+ if (!pctl)
+ return -ENOMEM;
+
+ pctl->desc = of_device_get_match_data(&pdev->dev);
+ pctl->regmap = axp20x->regmap;
+ 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] = regmap_irq_get_virq(axp20x->regmap_irqc, 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(&pdev->dev, "couldn't register pinctrl driver\n");
+ return PTR_ERR(pctl->pctl_dev);
+ }
+
+ ret = devm_gpiochip_add_data(&pdev->dev, &pctl->chip, pctl);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to register GPIO chip\n");
+ return ret;
+ }
+
+ ret = gpiochip_add_pin_range(&pctl->chip, dev_name(&pdev->dev),
+ pctl->desc->pins->number,
+ pctl->desc->pins->number,
+ pctl->desc->npins);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to add pin range\n");
+ return ret;
+ }
+
+ dev_info(&pdev->dev, "AXP192 pinctrl and GPIO driver loaded\n");
+
+ return 0;
+}
+
+static const struct of_device_id axp192_pctl_match[] = {
+ { .compatible = "x-powers,axp192-gpio", .data = &axp192_data, },
+ { }
+};
+MODULE_DEVICE_TABLE(of, axp192_pctl_match);
+
+static struct platform_driver axp192_pctl_driver = {
+ .probe = axp192_pctl_probe,
+ .driver = {
+ .name = "axp192-gpio",
+ .of_match_table = axp192_pctl_match,
+ },
+};
+
+module_platform_driver(axp192_pctl_driver);
+
+MODULE_AUTHOR("Aidan MacDonald <[email protected]>");
+MODULE_DESCRIPTION("AXP192 PMIC pinctrl and GPIO driver");
+MODULE_LICENSE("GPL");
--
2.35.1
On Fri, Jun 3, 2022 at 3:56 PM Aidan MacDonald
<[email protected]> 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.
>
> Signed-off-by: Aidan MacDonald <[email protected]>
Looks good to me (TM) but I'd like Michael Walle to take a look
to check if this is one of those drivers that could make use of
gpio-regmap.c CONFIG_GPIO_REGMAP to make it even
simpler.
Yours,
Linus Walleij
On Fri, Jun 3, 2022 at 6:29 PM Aidan MacDonald
<[email protected]> 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.
Thank you for contribution, overall looks good, below some not very
critical comments.
...
> +static const struct axp192_pctl_reg_info axp192_pin_ctrl_regs[] = {
> + { .reg = AXP192_GPIO0_CTRL, .mask = 0x07 },
> + { .reg = AXP192_GPIO1_CTRL, .mask = 0x07 },
> + { .reg = AXP192_GPIO2_CTRL, .mask = 0x07 },
> + { .reg = AXP192_GPIO4_3_CTRL, .mask = 0x03 },
> + { .reg = AXP192_GPIO4_3_CTRL, .mask = 0x0c },
> + { .reg = AXP192_N_RSTO_CTRL, .mask = 0xc0 },
> +};
GENMASK()
...
> + if ((val & reginfo->mask) == (input_muxvals[offset] << (ffs(reginfo->mask) - 1)))
> + return GPIO_LINE_DIRECTION_IN;
> + else
Redundant.
Also applies for the other similar cases in your code. Note, this is
also redundant for 'continue' and 'break' in case of loops.
> + return GPIO_LINE_DIRECTION_OUT;
...
> + if (!reginfo->mask)
> + return -EOPNOTSUPP;
Please, double check that this is used by the pin control subsystem
and not ENOTSUP in your case here.
...
> + default:
> + return -EOPNOTSUPP;
Ditto.
...
> + default:
> + return -EOPNOTSUPP;
Ditto.
...
> + default:
> + /* unreachable */
> + break;
return 0?! Perhaps you need to return an error?
> + }
> + }
> +
> + return 0;
...
> + if (muxvals[group] == (u8)-1)
limits.h and U8_MAX? Or GENMASK()? Choose one which suits you.
> + return -EINVAL;
...
> + if (!of_device_is_available(pdev->dev.of_node))
> + return -ENODEV;
Dead code.
> + if (!axp20x) {
> + dev_err(&pdev->dev, "Parent drvdata not set\n");
> + return -EINVAL;
> + }
Another useless piece of code.
...
> + pctl->desc = of_device_get_match_data(&pdev->dev);
device_get_match_data()
...
> + pctl->chip.to_irq = axp192_gpio_to_irq;
Why a custom method?
...
> + pctl->pctl_dev = devm_pinctrl_register(&pdev->dev, pctrl_desc, pctl);
> + if (IS_ERR(pctl->pctl_dev)) {
> + dev_err(&pdev->dev, "couldn't register pinctrl driver\n");
> + return PTR_ERR(pctl->pctl_dev);
Here and everywhere else in ->probe() and Co, use
return dev_err_probe(...);
pattern.
> + }
...
> + ret = gpiochip_add_pin_range(&pctl->chip, dev_name(&pdev->dev),
> + pctl->desc->pins->number,
> + pctl->desc->pins->number,
> + pctl->desc->npins);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to add pin range\n");
> + return ret;
> + }
We have a specific callback where you may put this, otherwise on some
systems it may not work as expected.
...
> + dev_info(&pdev->dev, "AXP192 pinctrl and GPIO driver loaded\n");
Useless.
...
> +static struct platform_driver axp192_pctl_driver = {
> + .probe = axp192_pctl_probe,
> + .driver = {
> + .name = "axp192-gpio",
> + .of_match_table = axp192_pctl_match,
> + },
> +};
> +
Redundant blank line.
> +module_platform_driver(axp192_pctl_driver);
...
Globally two comments:
1) I also believe that you may utilize gpio-regmap API;
2) try to get rid of OFisms, make it property provider agnostic.
--
With Best Regards,
Andy Shevchenko
Am 15. Juni 2022 15:44:04 OEZ schrieb Linus Walleij <[email protected]>:
>On Fri, Jun 3, 2022 at 3:56 PM Aidan MacDonald
><[email protected]> 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.
>>
>> Signed-off-by: Aidan MacDonald <[email protected]>
>
>Looks good to me (TM) but I'd like Michael Walle to take a look
>to check if this is one of those drivers that could make use of
>gpio-regmap.c CONFIG_GPIO_REGMAP to make it even
>simpler.
>
>Yours,
>Linus Walleij
FWIW, I can look at it at the end of next week. I'm on vacation.
-michael
Andy Shevchenko <[email protected]> writes:
> On Fri, Jun 3, 2022 at 6:29 PM Aidan MacDonald
> <[email protected]> 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.
>
> Thank you for contribution, overall looks good, below some not very
> critical comments.
>
> ...
>
Thanks very much for the review. I'll fix up the issues you spotted
in v3. (v2 doesn't make any changes to the pinctrl driver.)
>> +static const struct axp192_pctl_reg_info axp192_pin_ctrl_regs[] = {
>> + { .reg = AXP192_GPIO0_CTRL, .mask = 0x07 },
>> + { .reg = AXP192_GPIO1_CTRL, .mask = 0x07 },
>> + { .reg = AXP192_GPIO2_CTRL, .mask = 0x07 },
>> + { .reg = AXP192_GPIO4_3_CTRL, .mask = 0x03 },
>> + { .reg = AXP192_GPIO4_3_CTRL, .mask = 0x0c },
>> + { .reg = AXP192_N_RSTO_CTRL, .mask = 0xc0 },
>> +};
>
> GENMASK()
>
> ...
>
>> + if ((val & reginfo->mask) == (input_muxvals[offset] << (ffs(reginfo->mask) - 1)))
>> + return GPIO_LINE_DIRECTION_IN;
>
>> + else
>
> Redundant.
> Also applies for the other similar cases in your code. Note, this is
> also redundant for 'continue' and 'break' in case of loops.
>
Sorry, I'm not sure what you're referring to here. The "else"?
I'm missing the generalization.
>> + return GPIO_LINE_DIRECTION_OUT;
>
> ...
>
>> + if (!reginfo->mask)
>> + return -EOPNOTSUPP;
>
> Please, double check that this is used by the pin control subsystem
> and not ENOTSUP in your case here.
Whoops. You're right, it should be ENOTSUPP.
>
> ...
>
>> + default:
>> + return -EOPNOTSUPP;
>
> Ditto.
>
> ...
>
>> + default:
>> + return -EOPNOTSUPP;
>
> Ditto.
>
> ...
>
>> + default:
>> + /* unreachable */
>> + break;
>
> return 0?! Perhaps you need to return an error?
>
Yeah, that sounds like a good idea for maintainability. I think
there's no need to check that the requested configs are supported
beforehand since the caller must deal with errors in the middle of
the sequence anyway, so I'll drop that check and add ENOTSUPP here.
>> + }
>> + }
>> +
>> + return 0;
>
> ...
>
>> + if (muxvals[group] == (u8)-1)
>
> limits.h and U8_MAX? Or GENMASK()? Choose one which suits you.
>
>> + return -EINVAL;
>
> ...
>
>> + if (!of_device_is_available(pdev->dev.of_node))
>> + return -ENODEV;
>
> Dead code.
>
OK. Did some digging, and this is useless because the parent mfd
device is checking availability.
>> + if (!axp20x) {
>> + dev_err(&pdev->dev, "Parent drvdata not set\n");
>> + return -EINVAL;
>> + }
>
> Another useless piece of code.
>
> ...
>
>> + pctl->desc = of_device_get_match_data(&pdev->dev);
>
> device_get_match_data()
>
> ...
>
>> + pctl->chip.to_irq = axp192_gpio_to_irq;
>
> Why a custom method?
>
> ...
>
The irq chip is part of the mfd device, not the gpio chip. There does
not seem to be any default implementation for this case so I have to
provide one. A similar example is gpio-wm8994.
I did notice I'm doing something wrong by calling regmap_irq_get_virq()
in the probe function, which creates an irq mapping; I think I should be
doing that in the to_irq() callback like the other drivers do.
>> + pctl->pctl_dev = devm_pinctrl_register(&pdev->dev, pctrl_desc, pctl);
>> + if (IS_ERR(pctl->pctl_dev)) {
>> + dev_err(&pdev->dev, "couldn't register pinctrl driver\n");
>> + return PTR_ERR(pctl->pctl_dev);
>
> Here and everywhere else in ->probe() and Co, use
>
> return dev_err_probe(...);
>
> pattern.
>
>> + }
>
> ...
>
>> + ret = gpiochip_add_pin_range(&pctl->chip, dev_name(&pdev->dev),
>> + pctl->desc->pins->number,
>> + pctl->desc->pins->number,
>> + pctl->desc->npins);
>> + if (ret) {
>> + dev_err(&pdev->dev, "failed to add pin range\n");
>> + return ret;
>> + }
>
> We have a specific callback where you may put this, otherwise on some
> systems it may not work as expected.
>
> ...
>
Ah, sorry, I see that function is deprecated. The documentation points
to doing this in the device tree instead. So if I understand correctly
I should follow the example of pinctrl-thunderbay and add gpio-ranges:
pinctrl0: gpio@0 {
compatible = "x-powers,axp192-gpio";
gpio-controller;
#gpio-cells = <2>;
gpio-ranges = <&pinctrl0 0 0 6>;
};
which means I'll have to update the gpio DT bindings. I'm guessing the
callback you mentioned is add_pin_ranges() or of_gpio_ranges_fallback()
but neither of those seem appropriate in this case. The DT node should
be good enough.
>> + dev_info(&pdev->dev, "AXP192 pinctrl and GPIO driver loaded\n");
>
> Useless.
>
> ...
>
>> +static struct platform_driver axp192_pctl_driver = {
>> + .probe = axp192_pctl_probe,
>> + .driver = {
>> + .name = "axp192-gpio",
>> + .of_match_table = axp192_pctl_match,
>> + },
>> +};
>
>> +
>
> Redundant blank line.
>
>> +module_platform_driver(axp192_pctl_driver);
>
> ...
>
> Globally two comments:
> 1) I also believe that you may utilize gpio-regmap API;
> 2) try to get rid of OFisms, make it property provider agnostic.
I wasn't aware of gpio-regmap, will check it out.
Regards,
Aidan
On Fri, Jun 17, 2022 at 2:14 PM Aidan MacDonald
<[email protected]> wrote:
> Andy Shevchenko <[email protected]> writes:
> > On Fri, Jun 3, 2022 at 6:29 PM Aidan MacDonald
> > <[email protected]> wrote:
...
> >> + if ((val & reginfo->mask) == (input_muxvals[offset] << (ffs(reginfo->mask) - 1)))
> >> + return GPIO_LINE_DIRECTION_IN;
> >
> >> + else
> >
> > Redundant.
> > Also applies for the other similar cases in your code. Note, this is
> > also redundant for 'continue' and 'break' in case of loops.
>
> Sorry, I'm not sure what you're referring to here. The "else"?
Yes.
> I'm missing the generalization.
>
> >> + return GPIO_LINE_DIRECTION_OUT;
...
> >> + pctl->chip.to_irq = axp192_gpio_to_irq;
> >
> > Why a custom method?
>
> The irq chip is part of the mfd device, not the gpio chip. There does
> not seem to be any default implementation for this case so I have to
> provide one. A similar example is gpio-wm8994.
>
> I did notice I'm doing something wrong by calling regmap_irq_get_virq()
> in the probe function, which creates an irq mapping; I think I should be
> doing that in the to_irq() callback like the other drivers do.
It may be done using different approaches, but this part should be
carefully reviewed by GPIO / pin control maintainers.
...
> Ah, sorry, I see that function is deprecated. The documentation points
> to doing this in the device tree instead. So if I understand correctly
> I should follow the example of pinctrl-thunderbay and add gpio-ranges:
>
> pinctrl0: gpio@0 {
> compatible = "x-powers,axp192-gpio";
> gpio-controller;
> #gpio-cells = <2>;
> gpio-ranges = <&pinctrl0 0 0 6>;
> };
>
> which means I'll have to update the gpio DT bindings. I'm guessing the
> callback you mentioned is add_pin_ranges() or of_gpio_ranges_fallback()
> but neither of those seem appropriate in this case. The DT node should
> be good enough.
Sounds good.
--
With Best Regards,
Andy Shevchenko