From: Sergio Paracuellos <sergio.paracuellos@gmail.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: "open list:STAGING SUBSYSTEM" <devel@driverdev.osuosl.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@vger.kernel.org>, Jason Yan <yanaijie@huawei.com>,
Greg KH <gregkh@linuxfoundation.org>,
Linus Walleij <linus.walleij@linaro.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
Rob Herring <robh+dt@kernel.org>
Subject: Re: [PATCH 2/3] pinctrl: ralink: add a pinctrl driver for the rt2880 family
Date: Tue, 8 Dec 2020 12:48:32 +0100 [thread overview]
Message-ID: <CAMhs-H8G0A54J6csPLSuxs2Q-3vZQt363cZdui2SeJK4C2Yajw@mail.gmail.com> (raw)
In-Reply-To: <20201208101728.GZ2767@kadam>
Hi Dan,
Thanks for the review.
On Tue, Dec 8, 2020 at 11:17 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, Dec 07, 2020 at 08:21:03PM +0100, Sergio Paracuellos wrote:
> > +static struct pinctrl_desc rt2880_pctrl_desc = {
> > + .owner = THIS_MODULE,
> > + .name = "rt2880-pinmux",
> > + .pctlops = &rt2880_pctrl_ops,
> > + .pmxops = &rt2880_pmx_group_ops,
> > +};
> > +
> > +static struct rt2880_pmx_func gpio_func = {
> > + .name = "gpio",
> > +};
> > +
> > +static int rt2880_pinmux_index(struct rt2880_priv *p)
>
>
> This function name is not great. I assumed that it would return the
> index.
>
> > +{
> > + struct rt2880_pmx_func **f;
>
> Get rid of this "f" variable and use "p->func" instead.
>
> > + struct rt2880_pmx_group *mux = p->groups;
> > + int i, j, c = 0;
> > +
> > + /* count the mux functions */
> > + while (mux->name) {
> > + p->group_count++;
> > + mux++;
> > + }
> > +
> > + /* allocate the group names array needed by the gpio function */
> > + p->group_names = devm_kcalloc(p->dev, p->group_count,
> > + sizeof(char *), GFP_KERNEL);
> > + if (!p->group_names)
> > + return -1;
>
> Return proper error codes in this function. s/-1/-ENOMEM/
>
> > +
> > + for (i = 0; i < p->group_count; i++) {
> > + p->group_names[i] = p->groups[i].name;
> > + p->func_count += p->groups[i].func_count;
> > + }
> > +
> > + /* we have a dummy function[0] for gpio */
> > + p->func_count++;
> > +
> > + /* allocate our function and group mapping index buffers */
> > + f = p->func = devm_kcalloc(p->dev,
> > + p->func_count,
> > + sizeof(*p->func),
> > + GFP_KERNEL);
> > + gpio_func.groups = devm_kcalloc(p->dev, p->group_count, sizeof(int),
> > + GFP_KERNEL);
> > + if (!f || !gpio_func.groups)
> > + return -1;
> > +
> > + /* add a backpointer to the function so it knows its group */
> > + gpio_func.group_count = p->group_count;
> > + for (i = 0; i < gpio_func.group_count; i++)
> > + gpio_func.groups[i] = i;
> > +
> > + f[c] = &gpio_func;
> > + c++;
> > +
> > + /* add remaining functions */
> > + for (i = 0; i < p->group_count; i++) {
> > + for (j = 0; j < p->groups[i].func_count; j++) {
> > + f[c] = &p->groups[i].func[j];
> > + f[c]->groups = devm_kzalloc(p->dev, sizeof(int),
> > + GFP_KERNEL);
>
> Add a NULL check.
>
> > + f[c]->groups[0] = i;
> > + f[c]->group_count = 1;
> > + c++;
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > +static int rt2880_pinmux_pins(struct rt2880_priv *p)
> > +{
> > + int i, j;
> > +
> > + /*
> > + * loop over the functions and initialize the pins array.
> > + * also work out the highest pin used.
> > + */
> > + for (i = 0; i < p->func_count; i++) {
> > + int pin;
> > +
> > + if (!p->func[i]->pin_count)
> > + continue;
> > +
> > + p->func[i]->pins = devm_kcalloc(p->dev,
> > + p->func[i]->pin_count,
> > + sizeof(int),
> > + GFP_KERNEL);
>
> This can fit on two lines.
>
> p->func[i]->pins = devm_kcalloc(p->dev, p->func[i]->pin_count,
> sizeof(int), GFP_KERNEL);
>
> > + for (j = 0; j < p->func[i]->pin_count; j++)
> > + p->func[i]->pins[j] = p->func[i]->pin_first + j;
> > +
> > + pin = p->func[i]->pin_first + p->func[i]->pin_count;
> > + if (pin > p->max_pins)
> > + p->max_pins = pin;
> > + }
> > +
> > + /* the buffer that tells us which pins are gpio */
> > + p->gpio = devm_kcalloc(p->dev, p->max_pins, sizeof(u8), GFP_KERNEL);
> > + /* the pads needed to tell pinctrl about our pins */
> > + p->pads = devm_kcalloc(p->dev, p->max_pins,
> > + sizeof(struct pinctrl_pin_desc), GFP_KERNEL);
> > + if (!p->pads || !p->gpio) {
> > + dev_err(p->dev, "Failed to allocate gpio data\n");
>
> Delete this error message. #checkpatch.pl
>
> > + return -ENOMEM;
> > + }
> > +
> > + memset(p->gpio, 1, sizeof(u8) * p->max_pins);
> > + for (i = 0; i < p->func_count; i++) {
> > + if (!p->func[i]->pin_count)
> > + continue;
> > +
> > + for (j = 0; j < p->func[i]->pin_count; j++)
> > + p->gpio[p->func[i]->pins[j]] = 0;
> > + }
> > +
> > + /* pin 0 is always a gpio */
> > + p->gpio[0] = 1;
> > +
> > + /* set the pads */
> > + for (i = 0; i < p->max_pins; i++) {
> > + /* strlen("ioXY") + 1 = 5 */
> > + char *name = devm_kzalloc(p->dev, 5, GFP_KERNEL);
> > +
>
> char *name;
>
> name = kasprintf(GFP_KERNEL, "io%d", i);
>
>
> > + if (!name)
> > + return -ENOMEM;
> > + snprintf(name, 5, "io%d", i);
> > + p->pads[i].number = i;
> > + p->pads[i].name = name;
> > + }
> > + p->desc->pins = p->pads;
> > + p->desc->npins = p->max_pins;
> > +
> > + return 0;
> > +}
> > +
> > +static int rt2880_pinmux_probe(struct platform_device *pdev)
> > +{
> > + struct rt2880_priv *p;
> > + struct pinctrl_dev *dev;
> > +
> > + if (!rt2880_pinmux_data)
> > + return -ENOTSUPP;
> > +
> > + /* setup the private data */
> > + p = devm_kzalloc(&pdev->dev, sizeof(struct rt2880_priv), GFP_KERNEL);
> > + if (!p)
> > + return -ENOMEM;
> > +
> > + p->dev = &pdev->dev;
> > + p->desc = &rt2880_pctrl_desc;
> > + p->groups = rt2880_pinmux_data;
> > + platform_set_drvdata(pdev, p);
> > +
> > + /* init the device */
> > + if (rt2880_pinmux_index(p)) {
> > + dev_err(&pdev->dev, "failed to load index\n");
> > + return -EINVAL;
>
> Preserve the error code:
>
> err = rt2880_pinmux_index(p);
> if (err) {
> dev_err(&pdev->dev, "failed to load index\n");
> return err;
> }
>
>
> > + }
> > + if (rt2880_pinmux_pins(p)) {
> > + dev_err(&pdev->dev, "failed to load pins\n");
> > + return -EINVAL;
>
> Same.
>
> > + }
> > + dev = pinctrl_register(p->desc, &pdev->dev, p);
> > + if (IS_ERR(dev))
> > + return PTR_ERR(dev);
> > +
> > + return 0;
> > +}
This is already applied but needs more work in its correct place
afterwards. So I will take into account all of these comments and will
send patches to properly address all of them.
>
> regards,
> dan carpenter
Thanks again.
Best regards,
Sergio Paracuellos
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2020-12-08 11:48 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-07 19:21 [PATCH 0/3] pinctrl: ralink: pinctrl driver for the rt2880 family Sergio Paracuellos
2020-12-07 19:21 ` [PATCH 1/3] dt-bindings: pinctrl: rt2880: add binding document Sergio Paracuellos
2020-12-07 22:42 ` Linus Walleij
2020-12-08 6:46 ` Sergio Paracuellos
2020-12-07 19:21 ` [PATCH 2/3] pinctrl: ralink: add a pinctrl driver for the rt2880 family Sergio Paracuellos
2020-12-07 23:00 ` Linus Walleij
2020-12-08 6:53 ` Sergio Paracuellos
2020-12-08 10:17 ` Dan Carpenter
2020-12-08 11:48 ` Sergio Paracuellos [this message]
2020-12-07 19:21 ` [PATCH 3/3] staging: mt7621-pinctrl: remove driver from staging Sergio Paracuellos
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=CAMhs-H8G0A54J6csPLSuxs2Q-3vZQt363cZdui2SeJK4C2Yajw@mail.gmail.com \
--to=sergio.paracuellos@gmail.com \
--cc=dan.carpenter@oracle.com \
--cc=devel@driverdev.osuosl.org \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=yanaijie@huawei.com \
/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).