From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Stephen Boyd <swboyd@chromium.org>,
broonie@kernel.org, lee.jones@linaro.org,
linus.walleij@linaro.org, robh@kernel.org
Cc: vinod.koul@linaro.org, alsa-devel@alsa-project.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
spapothi@codeaurora.org, bgoswami@codeaurora.org,
linux-gpio@vger.kernel.org,
Yeleswarapu Nagaradhesh <nagaradh@codeaurora.org>
Subject: Re: [PATCH v3 09/11] pinctrl: qcom-wcd934x: Add support to wcd934x pinctrl driver.
Date: Thu, 31 Oct 2019 10:31:19 +0000 [thread overview]
Message-ID: <85aa859f-9d20-2a13-caba-d4901f18829c@linaro.org> (raw)
In-Reply-To: <5db9a34f.1c69fb81.23dfc.7ea5@mx.google.com>
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.
>
next prev parent reply other threads:[~2019-10-31 10:31 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-29 11:26 [PATCH v3 00/11] ASoC: Add support to WCD9340/WCD9341 codec Srinivas Kandagatla
2019-10-29 11:26 ` [PATCH v3 01/11] ASoC: dt-bindings: add dt bindings for WCD9340/WCD9341 audio codec Srinivas Kandagatla
2019-11-05 19:31 ` Rob Herring
2019-11-06 10:08 ` Srinivas Kandagatla
2019-11-07 17:40 ` Rob Herring
2019-10-29 11:26 ` [PATCH v3 02/11] mfd: wcd934x: add support to wcd9340/wcd9341 codec Srinivas Kandagatla
2019-11-11 11:18 ` Lee Jones
2019-11-11 12:48 ` Srinivas Kandagatla
2019-11-11 13:36 ` Lee Jones
2019-10-29 11:26 ` [PATCH v3 03/11] ASoC: " Srinivas Kandagatla
2019-10-29 11:26 ` [PATCH v3 04/11] ASoC: wcd934x: add basic controls Srinivas Kandagatla
2019-10-29 11:26 ` [PATCH v3 05/11] ASoC: wcd934x: add playback dapm widgets Srinivas Kandagatla
2019-10-29 11:26 ` [PATCH v3 06/11] ASoC: wcd934x: add capture " Srinivas Kandagatla
2019-10-29 11:26 ` [PATCH v3 07/11] ASoC: wcd934x: add audio routings Srinivas Kandagatla
2019-10-29 11:26 ` [PATCH v3 08/11] dt-bindings: pinctrl: qcom-wcd934x: Add bindings for gpio Srinivas Kandagatla
2019-11-03 23:19 ` Linus Walleij
2019-11-04 9:35 ` Srinivas Kandagatla
2019-11-05 13:25 ` Linus Walleij
2019-11-05 13:27 ` Srinivas Kandagatla
2019-11-05 18:49 ` Rob Herring
2019-10-29 11:26 ` [PATCH v3 09/11] pinctrl: qcom-wcd934x: Add support to wcd934x pinctrl driver Srinivas Kandagatla
2019-10-30 14:50 ` Stephen Boyd
2019-10-31 10:31 ` Srinivas Kandagatla [this message]
2019-10-29 11:26 ` [PATCH v3 10/11] ASoC: qcom: dt-bindings: Add compatible for DB845c and Lenovo Yoga Srinivas Kandagatla
2019-10-29 11:27 ` [PATCH v3 11/11] ASoC: qcom: sdm845: add support to " Srinivas Kandagatla
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=85aa859f-9d20-2a13-caba-d4901f18829c@linaro.org \
--to=srinivas.kandagatla@linaro.org \
--cc=alsa-devel@alsa-project.org \
--cc=bgoswami@codeaurora.org \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=lee.jones@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nagaradh@codeaurora.org \
--cc=robh@kernel.org \
--cc=spapothi@codeaurora.org \
--cc=swboyd@chromium.org \
--cc=vinod.koul@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).