Re: [PATCH v4 3/4] pinctrl: Qualcomm SPMI PMIC GPIO pin controller driver

From: Bjorn Andersson
Date: Wed Sep 24 2014 - 00:18:58 EST

Next message: Bjorn Andersson: "Re: [PATCH v7 0/3] Qualcomm Resource Power Manager driver"
Previous message: Eric W. Biederman: "Re: [PATCH 1/5] rhashtable: Remove gfp_flags from insert and remove functions"
Next in thread: Ivan T. Ivanov: "Re: [PATCH v4 3/4] pinctrl: Qualcomm SPMI PMIC GPIO pin controller driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon 15 Sep 07:44 PDT 2014, Ivan T. Ivanov wrote:

> This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> Qualcomm GPIO sub-function blocks found in the PMIC chips.
>
> Signed-off-by: Ivan T. Ivanov <iivanov@xxxxxxxxxx>

I think this looks pretty good, just some minor comments. Mostly on the future
compatibility of the Kconfig and compatible.

It's much in line with what I hacked up for pm8xxx, I was just hoping to get
something from Thomas regarding irq_read_line() before pushing it again...

> ---
> drivers/pinctrl/qcom/Kconfig | 12 +
> drivers/pinctrl/qcom/Makefile | 1 +
> drivers/pinctrl/qcom/pinctrl-spmi-pmic-gpio.c | 942 ++++++++++++++++++++++++++
> 3 files changed, 955 insertions(+)
> create mode 100644 drivers/pinctrl/qcom/pinctrl-spmi-pmic-gpio.c
>
> diff --git a/drivers/pinctrl/qcom/Kconfig b/drivers/pinctrl/qcom/Kconfig
> index d160a71..123248f 100644
> --- a/drivers/pinctrl/qcom/Kconfig
> +++ b/drivers/pinctrl/qcom/Kconfig
> @@ -39,4 +39,16 @@ config PINCTRL_MSM8X74
> This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> Qualcomm TLMM block found in the Qualcomm 8974 platform.
>
> +config PINCTRL_SPMI_PMIC

As SPMI is a MIPI specification for doing power management I think it's safe to
assume that this is not going to be the only "spmi pmic" pinctrl driver.
So please make this a little bit more specific by throwing in a QCOM here.

> + tristate "Qualcomm SPMI PMIC pin controller driver"
> + depends on GPIOLIB && OF
> + select PINMUX
> + select PINCONF
> + select GENERIC_PINCONF
> + help
> + This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> + Qualcomm GPIO and MPP blocks found in the Qualcomm PMIC's chips,
> + which are using SPMI for communication with SoC. Example PMIC's
> + devices are pm8841, pm8941 and pma8084.
> +
> endif
> diff --git a/drivers/pinctrl/qcom/Makefile b/drivers/pinctrl/qcom/Makefile
> index 2a02602..396130c 100644
> --- a/drivers/pinctrl/qcom/Makefile
> +++ b/drivers/pinctrl/qcom/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_PINCTRL_APQ8064) += pinctrl-apq8064.o
> obj-$(CONFIG_PINCTRL_IPQ8064) += pinctrl-ipq8064.o
> obj-$(CONFIG_PINCTRL_MSM8960) += pinctrl-msm8960.o
> obj-$(CONFIG_PINCTRL_MSM8X74) += pinctrl-msm8x74.o
> +obj-$(CONFIG_PINCTRL_SPMI_PMIC) += pinctrl-spmi-pmic-gpio.o
> diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-pmic-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-pmic-gpio.c
> new file mode 100644
> index 0000000..493f0d3
> --- /dev/null
> +++ b/drivers/pinctrl/qcom/pinctrl-spmi-pmic-gpio.c

This is fine, no need to change as we have qcom in the path...

[..]
> +
> +/**
> + * struct pmic_gpio_pad - keep current GPIO settings

Some indentation in this table would be nice.
Content look sane though.

> + * @base: Address base in SPMI device.
> + * @irq: IRQ number which this GPIO generate.
> + * @is_enabled: Set to false when GPIO should be put in high Z state.
> + * @out_value: Cached pin output value
> + * @have_buffer: Set to true if GPIO output could be configured in push-pull,
> + * open-drain or open-source mode.
> + * @output_enabled: Set to true if GPIO output logic is enabled.
> + * @input_enabled: Set to true if GPIO input buffer logic is enabled.
> + * @num_sources: Number of power-sources supported by this GPIO.
> + * @power_source: Current power-source used.
> + * @buffer_type: Push-pull, open-drain or open-source.
> + * @pullup: Constant current which flow trough GPIO output buffer.
> + * @strength: No, Low, Medium, High
> + * @function: See pmic_gpio_functions[]
> + */
> +struct pmic_gpio_pad {
> + u16 base;
> + int irq;
> + bool is_enabled;
> + bool out_value;
> + bool have_buffer;
> + bool output_enabled;
> + bool input_enabled;
> + unsigned int num_sources;
> + unsigned int power_source;
> + unsigned int buffer_type;
> + unsigned int pullup;
> + unsigned int strength;
> + unsigned int function;
> +};
> +
[..]
> +
> +static int pmic_gpio_parse_dt_config(struct device_node *np,
> + struct pinctrl_dev *pctldev,
> + unsigned long **configs,
> + unsigned int *nconfs)
> +{
> + struct pmic_gpio_bindings *par;
> + unsigned long cfg;
> + int ret, i;
> + u32 val;
> +
> + for (i = 0; i < ARRAY_SIZE(pmic_gpio_bindings); i++) {
> +

Empty line

> + par = &pmic_gpio_bindings[i];
> + ret = of_property_read_u32(np, par->property, &val);
> +
> + /* property not found */
> + if (ret == -EINVAL)
> + continue;
> +
> + /* use zero as default value */
> + if (ret)
> + val = 0;
> +
> + dev_dbg(pctldev->dev, "found %s with value %u\n",
> + par->property, val);
> +
> + cfg = pinconf_to_config_packed(par->param, val);
> +
> + ret = pinctrl_utils_add_config(pctldev, configs, nconfs, cfg);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
[..]
> +
> +static int pmic_gpio_dt_node_to_map(struct pinctrl_dev *pctldev,
> + struct device_node *np_config,
> + struct pinctrl_map **map, unsigned *nmaps)
> +{
> + enum pinctrl_map_type type;
> + struct device_node *np;
> + unsigned reserv;
> + int ret;
> +
> + ret = 0;
> + *map = NULL;
> + *nmaps = 0;
> + reserv = 0;
> + type = PIN_MAP_TYPE_CONFIGS_GROUP;
> +
> + for_each_child_of_node(np_config, np) {
> +

Empty line

> + ret = pinconf_generic_dt_subnode_to_map(pctldev, np, map,
> + &reserv, nmaps, type);
> + if (ret)
> + break;
> +
> + ret = pmic_gpio_dt_subnode_to_map(pctldev, np, map, &reserv,
> + nmaps, type);
> + if (ret)
> + break;
> + }
> +
> + if (ret < 0)
> + pinctrl_utils_dt_free_map(pctldev, *map, *nmaps);
> +
> + return ret;
> +}
> +
[..]
> +
> +static int pmic_gpio_config_get(struct pinctrl_dev *pctldev,
> + unsigned int pin, unsigned long *config)
> +{
> + unsigned param = pinconf_to_config_param(*config);
> + struct pmic_gpio_pad *pad;
> + unsigned arg;
> +
> + pad = pctldev->desc->pins[pin].drv_data;
> +
> + switch (param) {
> + case PIN_CONFIG_DRIVE_PUSH_PULL:
> + arg = pad->buffer_type == PMIC_GPIO_OUT_BUF_CMOS;
> + break;
> + case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> + arg = pad->buffer_type == PMIC_GPIO_OUT_BUF_OPEN_DRAIN_NMOS;
> + break;
> + case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> + arg = pad->buffer_type == PMIC_GPIO_OUT_BUF_OPEN_DRAIN_PMOS;
> + break;
> + case PIN_CONFIG_BIAS_PULL_DOWN:
> + arg = pad->pullup == PMIC_GPIO_PULL_DOWN;
> + break;
> + case PIN_CONFIG_BIAS_DISABLE:
> + arg = pad->pullup = PMIC_GPIO_PULL_DISABLE;
> + break;
> + case PIN_CONFIG_BIAS_PULL_UP:
> + arg = pad->pullup == PMIC_GPIO_PULL_UP_30;
> + break;

Extra break;

> + break;
> + case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> + arg = !pad->is_enabled;
> + break;
> + case PIN_CONFIG_POWER_SOURCE:
> + arg = pad->power_source;
> + break;
> + case PIN_CONFIG_INPUT_ENABLE:
> + arg = pad->input_enabled;
> + break;
> + case PIN_CONFIG_OUTPUT:
> + arg = pad->out_value;
> + break;
> + case PMIC_GPIO_CONF_PULL_UP:
> + arg = pad->pullup;
> + break;
> + case PMIC_GPIO_CONF_STRENGTH:
> + arg = pad->strength;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + *config = pinconf_to_config_packed(param, arg);
> + return 0;
> +}
> +
[..]
> +static int pmic_gpio_direction_input(struct gpio_chip *chip, unsigned pin)
> +{
> + struct pmic_gpio_state *state;
> + unsigned long config;
> +
> + state = container_of(chip, struct pmic_gpio_state, chip);

You use this container_of plenty of times, consider breaking it out to a
to_gpio_state() helper function and you can fit it on the declaration line.

> + config = pinconf_to_config_packed(PIN_CONFIG_INPUT_ENABLE, 1);
> +
> + return pmic_gpio_config_set(state->ctrl, pin, &config, 1);
> +}
> +
[..]
> +
> +static int pmic_gpio_of_xlate(struct gpio_chip *chip,
> + const struct of_phandle_args *gpio_desc,
> + u32 *flags)
> +{
> + if (chip->of_gpio_n_cells < 2)
> + return -EINVAL;
> +
> + if (flags)
> + *flags = gpio_desc->args[1];
> +
> + return gpio_desc->args[0] - PMIC_GPIO_PHYSICAL_OFFSET;
> +}

If you change:
gpiochip_add_pin_range(&state->chip, dev_name(dev), 0, 0, npins);
to:
gpiochip_add_pin_range(&state->chip, dev_name(dev), 1, 0, npins);

And you treat the gpio functions as taking the gpio number instead of pinctrl
number (i.e. subtract 1 in those), then gpiolib will provide this function for
you.

[..]
> +
> +static const struct of_device_id pmic_gpio_of_match[] = {
> + { .compatible = "qcom,spmi-pmic-gpio" },

I think this should be more specific, because hopefully the spmi specification
will outlive the current pmic gpio block.

So I think you need to list the pmic blocks here (e.g. "qcom,pm8941-gpio").

> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(of, pmic_gpio_of_match);
> +
> +static struct platform_driver pmic_gpio_driver = {
> + .driver = {
> + .name = "spmi-pmic-gpio",

The name should include "qcom" as well, to make it less prone to collisions.

> + .owner = THIS_MODULE,

owner filled in for you by module_platform_driver()

> + .of_match_table = pmic_gpio_of_match,
> + },
> + .probe = pmic_gpio_probe,
> + .remove = pmic_gpio_remove,
> +};
> +
> +module_platform_driver(pmic_gpio_driver);
> +

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Bjorn Andersson: "Re: [PATCH v7 0/3] Qualcomm Resource Power Manager driver"
Previous message: Eric W. Biederman: "Re: [PATCH 1/5] rhashtable: Remove gfp_flags from insert and remove functions"
Next in thread: Ivan T. Ivanov: "Re: [PATCH v4 3/4] pinctrl: Qualcomm SPMI PMIC GPIO pin controller driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]