Re: [PATCH v3 09/11] pinctrl: qcom-wcd934x: Add support to wcd934x pinctrl driver.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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?
I agree, will fix this in next spin.

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'.

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.




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux