Quoting Srinivas Kandagatla (2019-10-29 04:26:58)
> From: Yeleswarapu Nagaradhesh <nagaradh@xxxxxxxxxxxxxx>
>
> This patch adds support to wcd934x pinctrl block found in
> WCD9340/WC9341 Audio codecs.
>
> [Srini: multiple cleanups to the code]
This goes after the author signoff and before yours. Can you add more
details too?
> Signed-off-by: Yeleswarapu Nagaradhesh <nagaradh@xxxxxxxxxxxxxx>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> ---
> drivers/pinctrl/qcom/Kconfig | 7 +
> drivers/pinctrl/qcom/Makefile | 1 +
> drivers/pinctrl/qcom/pinctrl-wcd934x-gpio.c | 365 ++++++++++++++++++++
> 3 files changed, 373 insertions(+)
> create mode 100644 drivers/pinctrl/qcom/pinctrl-wcd934x-gpio.c
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-wcd934x-gpio.c b/drivers/pinctrl/qcom/pinctrl-wcd934x-gpio.c
> new file mode 100644
> index 000000000000..1aff88d0bcb3
> --- /dev/null
> +++ b/drivers/pinctrl/qcom/pinctrl-wcd934x-gpio.c
> @@ -0,0 +1,365 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2016-2017, The Linux Foundation. All rights reserved.
> +// Copyright (c) 2019, Linaro Limited
> +
> +#include <linux/module.h>
> +#include <linux/gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
> +
> +#include "../core.h"
> +#include "../pinctrl-utils.h"
> +
> +#define WCD_REG_DIR_CTL_OFFSET 0x42
> +#define WCD_REG_VAL_CTL_OFFSET 0x43
> +#define WCD_GPIO_PULL_UP 1
> +#define WCD_GPIO_PULL_DOWN 2
> +#define WCD_GPIO_BIAS_DISABLE 3
> +#define WCD_GPIO_STRING_LEN 20
> +#define WCD934X_NPINS 5
> +
> +/**
> + * struct wcd_gpio_pad - keep current GPIO settings
> + * @offset: offset of gpio.
> + * @is_valid: Set to false, when GPIO in high Z state.
> + * @value: value of a pin
> + * @output_enabled: Set to true if GPIO is output and false if it is input
> + * @pullup: Constant current which flow through GPIO output buffer.
> + * @strength: Drive strength of a pin
> + */
> +struct wcd_gpio_pad {
> + u16 offset;
> + bool is_valid;
> + bool value;
> + bool output_enabled;
> + unsigned int pullup;
> + unsigned int strength;
> +};
> +
> +struct wcd_gpio_priv {
> + struct device *dev;
> + struct regmap *map;
> + struct pinctrl_dev *ctrl;
> + struct gpio_chip chip;
> +};
> +
> +static int wcd_gpio_read(struct wcd_gpio_priv *priv_data,
> + struct wcd_gpio_pad *pad, unsigned int addr)
> +{
> + unsigned int val;
> + int ret;
> +
> + ret = regmap_read(priv_data->map, addr, &val);
> + if (ret < 0)
> + dev_err(priv_data->dev, "%s: read 0x%x failed\n",
> + __func__, addr);
> + else
> + ret = (val >> pad->offset);
> +
> + return ret;
> +}
> +
> +static int wcd_gpio_write(struct wcd_gpio_priv *priv_data,
> + struct wcd_gpio_pad *pad, unsigned int addr,
> + unsigned int val)
> +{
> + int ret;
> +
> + ret = regmap_update_bits(priv_data->map, addr, (1 << pad->offset),
> + val << pad->offset);
> + if (ret < 0)
> + dev_err(priv_data->dev, "write 0x%x failed\n", addr);
Is there value in these error messages? Also, use %#x to get '0x'.
> +
> + return ret;
> +}
[...]
> +
> +static int wcd_pinctrl_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct pinctrl_pin_desc *pindesc;
> + struct pinctrl_desc *pctrldesc;
> + struct wcd_gpio_pad *pad, *pads;
> + struct wcd_gpio_priv *priv_data;
> + u32 npins = WCD934X_NPINS;
> + char **name;
> + int i;
> +
> + priv_data = devm_kzalloc(dev, sizeof(*priv_data), GFP_KERNEL);
> + if (!priv_data)
> + return -ENOMEM;
> +
> + priv_data->dev = dev;
> + priv_data->map = dev_get_regmap(dev->parent, NULL);
> + if (!priv_data->map) {
> + dev_err(dev, "%s: failed to get regmap\n", __func__);
> + return -EINVAL;
> + }
> +
> + pindesc = devm_kcalloc(dev, npins, sizeof(*pindesc), GFP_KERNEL);
> + if (!pindesc)
> + return -ENOMEM;
> +
> + pads = devm_kcalloc(dev, npins, sizeof(*pads), GFP_KERNEL);
> + if (!pads)
> + return -ENOMEM;
Is it possible to put the pad struct around the pindesc struct? It's
sort of sad that we have to allocate a chunk of memory twice for the
pindesc and the pads when we could either use container_of() on the
pindesc or just point the pindesc driver data member to the container
structure for the qcom specific bits.
> +
> + pctrldesc = devm_kzalloc(dev, sizeof(*pctrldesc), GFP_KERNEL);
> + if (!pctrldesc)
> + return -ENOMEM;
> +
> + pctrldesc->pctlops = &wcd_pinctrl_ops;
> + pctrldesc->confops = &wcd_pinconf_ops;
> + pctrldesc->owner = THIS_MODULE;
> + pctrldesc->name = dev_name(dev);
> + pctrldesc->pins = pindesc;
> + pctrldesc->npins = npins;
> +
> + name = devm_kcalloc(dev, npins, sizeof(char *), GFP_KERNEL);
> + if (!name)
> + return -ENOMEM;
> +
> + for (i = 0; i < npins; i++, pindesc++) {
> + name[i] = devm_kzalloc(dev, sizeof(char) * WCD_GPIO_STRING_LEN,
> + GFP_KERNEL);
> + if (!name[i])
> + return -ENOMEM;
> +
> + pad = &pads[i];
> + pindesc->drv_data = pad;
> + pindesc->number = i;
> + snprintf(name[i], (WCD_GPIO_STRING_LEN - 1), "gpio%d", (i+1));
> + pindesc->name = name[i];
Why not use devm_kasprintf()? The 'name' array is also unnecessary?
> + pad->offset = i;
> + pad->is_valid = true;
> + }
> +
> + priv_data->chip = wcd_gpio_chip;
> + priv_data->chip.parent = dev;
> + priv_data->chip.base = -1;
> + priv_data->chip.ngpio = npins;
> + priv_data->chip.label = dev_name(dev);
> + priv_data->chip.of_gpio_n_cells = 2;
> + priv_data->chip.can_sleep = false;
> + platform_set_drvdata(pdev, priv_data);
> +
> + priv_data->ctrl = devm_pinctrl_register(dev, pctrldesc, priv_data);
> + if (IS_ERR(priv_data->ctrl)) {
> + dev_err(dev, "%s: failed to register to pinctrl\n", __func__);
> + return PTR_ERR(priv_data->ctrl);
> + }
> +
> + return gpiochip_add_data(&priv_data->chip, priv_data);
WHy not use devm_gpiochip_add_data()?
> +}
> +
> +static int wcd_pinctrl_remove(struct platform_device *pdev)
> +{
> + struct wcd_gpio_priv *priv_data = platform_get_drvdata(pdev);
> +
> + gpiochip_remove(&priv_data->chip);
> +
> + return 0;
And drop this function?
> +}
> +
> +static const struct of_device_id wcd_pinctrl_of_match[] = {
> + { .compatible = "qcom,wcd9340-pinctrl" },
> + { .compatible = "qcom,wcd9341-pinctrl" },
> + { },
Nitpick: Drop the comma on the sentinel.
> +};
> +
> +MODULE_DEVICE_TABLE(of, wcd_pinctrl_of_match);
Nitpick: Drop the newline between device table and match table.
