Thanks Stephen for reviewing this patch.
On 30/10/2019 14:50, Stephen Boyd wrote:
Quoting Srinivas Kandagatla (2019-10-29 04:26:58)
From: Yeleswarapu Nagaradhesh nagaradh@codeaurora.org
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?
I agree, will fix this in next spin.
Signed-off-by: Yeleswarapu Nagaradhesh nagaradh@codeaurora.org Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org
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);elseret = (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'.
I can add ret in the err message.
I did not knew about "%#x".. nice, I will use this in future!
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.
I will give that a go in next version!
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?
Am not sure why its not used her, but I can do that change in next version.
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()?
Good idea, will do that in next spin.
+}
+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.