Messages in this thread | | | Subject | Re: [PATCH v3 09/11] pinctrl: qcom-wcd934x: Add support to wcd934x pinctrl driver. | From | Srinivas Kandagatla <> | Date | Thu, 31 Oct 2019 10:31:19 +0000 |
| |
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); >> + 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. >
| |