From: "Sören Brinkmann" <soren.brinkmann@xilinx.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Bjorn Andersson <Bjorn.Andersson@sonymobile.com>,
"Ivan T. Ivanov" <iivanov@mm-sol.com>,
Michal Simek <michal.simek@xilinx.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
Alessandro Rubini <rubini@unipv.it>,
Heiko Stuebner <heiko@sntech.de>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
<linux-rockchip@lists.infradead.org>,
"linux-sh@vger.kernel.org" <linux-sh@vger.kernel.org>
Subject: Re: [PATCH 3/7] pinctrl: pinconf-generic: Allow driver to specify DT params
Date: Thu, 27 Nov 2014 09:53:14 -0800 [thread overview]
Message-ID: <3656bd45ce11485ebe7dfa6b64ca87fe@BN1BFFO11FD023.protection.gbl> (raw)
In-Reply-To: <CACRpkdYfj2ydLQ4q_FKwXo73jzNsvj9u2Aem9EiuEw-OCvurrg@mail.gmail.com>
Hi Linus,
On Tue, 2014-11-11 at 03:53PM +0100, Linus Walleij wrote:
> On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
> <soren.brinkmann@xilinx.com> wrote:
>
> > Additionally to the generic DT parameters, allow drivers to provide
> > driver-specific DT parameters to be used with the generic parser
> > infrastructure.
> >
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>
> I like the looks of this, but the patch description is a bit terse.
> I'd like it to describe some of the refactorings being done
> to the intrinsics, because I have a hard time following the patch.
I'll be a little more verbose :)
>
> First please rebase onto the "devel" branch in the pin control
> tree, and notice that drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> which is merged there is actually doing this already:
>
>
> for_each_child_of_node(np_config, np) {
> ret = pinconf_generic_dt_subnode_to_map(pctldev, np, map,
> &reserv, nmaps, type);
> if (ret)
> break;
>
> ret = pmic_gpio_dt_subnode_to_map(pctldev, np, map, &reserv,
> nmaps, type);
> if (ret)
> break;
> }
>
> So it should be patched to illustrate the point of this code.
I'll look into this.
>
> I'd like feedback from Ivan+Björn on the code too if possible.
>
> > - ret = pinconf_generic_parse_dt_config(np, &configs, &nconfigs);
> > + ret = pinconf_generic_parse_dt_config(np, pctldev, &configs, &nconfigs);
> > if (nconfigs)
> > has_config = 1;
> > np_config = of_parse_phandle(np, "ste,config", 0);
> > if (np_config) {
> > - ret = pinconf_generic_parse_dt_config(np_config, &configs,
> > - &nconfigs);
> > + ret = pinconf_generic_parse_dt_config(np_config, pctldev,
> > + &configs, &nconfigs);
>
> This code is patched upstream so that ABx500 only uses generic config.
> Again rebase on "devel"
Yeah, causes a conflict, but seems to be pretty much the same.
>
> > -void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
> > - struct seq_file *s, unsigned pin)
> > +static void _pinconf_generic_dump(struct pinctrl_dev *pctldev,
> > + struct seq_file *s, const char *gname,
> > + unsigned pin,
> > + const struct pin_config_item *items,
> > + int nitems)
>
> Don't use functions named _foo, actually the underscore is for
> preprocessor and compiler things in my book, just give it an intuitive
> name instead. Like pinconf_generic_dump_one() if that is suitable
> or whatever.
>
> This changes the function signature from something quite intuitively
> understood to something pretty hard to understand, so you need to
> add kerneldoc to it. (That also enhance my understanding of the
> patch.)
I'll rename it and add some documentation.
>
> > -void pinconf_generic_dump_group(struct pinctrl_dev *pctldev,
> > - struct seq_file *s, const char *gname)
> > +static void pinconf_generic_dump(struct pinctrl_dev *pctldev,
> > + struct seq_file *s, const char *gname,
> > + unsigned pin)
>
> This looks intuitive and nice.
>
> > + _pinconf_generic_dump(pctldev, s, gname, pin,
> > + conf_items, ARRAY_SIZE(conf_items));
> > + if (pctldev->desc->num_dt_params) {
> > + BUG_ON(!pctldev->desc->conf_items);
>
> Don't use BUG_ON() like that, it's nasty. Always try to
> recover and bail out instead.
I merge the condition into the if.
>
> > +void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
> > + struct seq_file *s, unsigned pin)
> > +{
> > + pinconf_generic_dump(pctldev, s, NULL, pin);
> > +}
> > +
> > +void pinconf_generic_dump_group(struct pinctrl_dev *pctldev,
> > + struct seq_file *s, const char *gname)
> > +{
> > + pinconf_generic_dump(pctldev, s, gname, 0);
> > +}
>
> Do you really need these helpers? Isn't it simpler just
> to call the generic function with the different arguments?
I'll remove the helpers and patch the users of these functions.
>
> > @@ -148,17 +132,22 @@ void pinconf_generic_dump_config(struct pinctrl_dev *pctldev,
> > seq_printf(s, "%s: 0x%x", conf_items[i].display,
> > pinconf_to_config_argument(config));
> > }
> > +
> > + if (!pctldev->desc->num_dt_params)
> > + return;
> > +
> > + BUG_ON(!pctldev->desc->conf_items);
>
> No BUG_ON() dev_err() and exit.
As above.
>
> > +static void _parse_dt_cfg(struct device_node *np,
> > + const struct pinconf_generic_dt_params *params,
> > + unsigned int count,
> > + unsigned long *cfg,
> > + unsigned int *ncfg)
>
> Should return an error code right? Kerneldoc doesn't hurt either.
I don't see a need for an error return. It's currently not needed and
this refactoring doesn't change that, IMHO. I'll add kerneldoc.
>
> > +{
> > + int i;
> > +
> > + for (i = 0; i < count; i++) {
> > + u32 val;
> > + int ret;
> > + const struct pinconf_generic_dt_params *par = ¶ms[i];
> > +
> > + ret = of_property_read_u32(np, par->property, &val);
>
> Not checking this return value. Alter the function to return an
> int value on success.
It's checked in the very next statement?! And it's all handled in this
function. No need to report anything to the caller.
>
> > +
> > + /* property not found */
> > + if (ret == -EINVAL)
> > + continue;
> > +
> > + /* use default value, when no value is specified */
> > + if (ret)
> > + val = par->default_value;
> > +
> > + pr_debug("found %s with value %u\n", par->property, val);
> > + cfg[*ncfg] = pinconf_to_config_packed(par->param, val);
> > + (*ncfg)++;
> > + }
> > +}
>
> There is something very unintuitive about this loop. You pass two
> counter indexes (count, ncfg) in basically, that is looking weird,
> does it have to look like that? Especially since there is no
> bounds check on ncfg!
>
> Just use one index in the loop please. Assign *ncfg = ... after
> the loop has *successfully* iterated.
I think this needs to be as is. There are two arrays @cfg and @params.
@params holding the DT params parsed with @count indicating the
boundary. And @cfg, where the parsed options are put with @ncfg being
a write pointer. @nfcg can be passed non-zero into this function. The
caller is responsible to allocate enough memory to hold all possible entries.
>
> > int pinconf_generic_parse_dt_config(struct device_node *np,
> > + struct pinctrl_dev *pctldev,
> > unsigned long **configs,
> > unsigned int *nconfigs)
>
> This is a good refactoring, but no _foo naming!
Will be renamed.
>
> > {
> > unsigned long *cfg;
> > - unsigned int ncfg = 0;
> > + unsigned int max_cfg, ncfg = 0;
> > int ret;
> > - int i;
> > - u32 val;
> >
> > if (!np)
> > return -EINVAL;
> >
> > /* allocate a temporary array big enough to hold one of each option */
> > - cfg = kzalloc(sizeof(*cfg) * ARRAY_SIZE(dt_params), GFP_KERNEL);
> > + max_cfg = ARRAY_SIZE(dt_params);
> > + if (pctldev)
> > + max_cfg += pctldev->desc->num_dt_params;
> > + cfg = kcalloc(max_cfg, sizeof(*cfg), GFP_KERNEL);
>
> Aha this looks good...
>
> > + _parse_dt_cfg(np, dt_params, ARRAY_SIZE(dt_params), cfg, &ncfg);
> > + if (pctldev && pctldev->desc->num_dt_params) {
> > + BUG_ON(!pctldev->desc->params);
>
> No BUG_ON()
as above.
Sören
next prev parent reply other threads:[~2014-11-27 17:53 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-03 19:05 [PATCH 0/7] Pinctrl support for Zynq Soren Brinkmann
2014-11-03 19:05 ` [PATCH 1/7] pinctrl: pinconf-generic: Declare dt_params/conf_items const Soren Brinkmann
2014-11-11 12:00 ` Linus Walleij
2014-11-03 19:05 ` [PATCH 2/7] pinctrl: pinconf-generic: Infer map type from DT property Soren Brinkmann
2014-11-05 13:56 ` Laurent Pinchart
2014-11-05 18:09 ` Sören Brinkmann
2014-11-05 18:17 ` Laurent Pinchart
2014-11-11 12:29 ` Linus Walleij
2014-11-12 18:43 ` Sören Brinkmann
2014-11-11 12:47 ` Linus Walleij
2014-11-12 18:46 ` Sören Brinkmann
2014-11-12 19:38 ` Sören Brinkmann
2014-11-03 19:05 ` [PATCH 3/7] pinctrl: pinconf-generic: Allow driver to specify DT params Soren Brinkmann
2014-11-03 19:12 ` Geert Uytterhoeven
2014-11-11 14:53 ` Linus Walleij
2014-11-18 8:50 ` Ivan T. Ivanov
2014-11-18 17:25 ` Sören Brinkmann
2014-11-19 7:49 ` Ivan T. Ivanov
2014-11-19 15:35 ` Sören Brinkmann
2014-11-20 8:06 ` Ivan T. Ivanov
2014-11-20 16:22 ` Sören Brinkmann
2014-11-21 7:35 ` Ivan T. Ivanov
2014-11-22 16:06 ` Sören Brinkmann
2014-11-24 8:52 ` Ivan T. Ivanov
2014-11-27 17:53 ` Sören Brinkmann [this message]
2014-11-03 19:05 ` [PATCH 4/7] pinctrl: zynq: Document DT binding Soren Brinkmann
2014-11-05 3:35 ` Andreas Färber
2014-11-05 17:07 ` Sören Brinkmann
2014-11-11 15:00 ` Linus Walleij
2014-11-12 18:53 ` Sören Brinkmann
2014-11-27 13:10 ` Linus Walleij
2014-11-03 19:05 ` [PATCH 5/7] pinctrl: Add driver for Zynq Soren Brinkmann
2014-11-05 3:24 ` Andreas Färber
2014-11-05 17:10 ` Sören Brinkmann
2014-11-05 5:12 ` Andreas Färber
2014-11-05 17:14 ` Sören Brinkmann
2014-11-03 19:05 ` [PATCH 6/7] ARM: zynq: Enable pinctrl Soren Brinkmann
2014-11-03 19:05 ` [PATCH 7/7] ARM: zynq: DT: Add pinctrl information Soren Brinkmann
2014-11-05 5:56 ` [PATCH 0/7] Pinctrl support for Zynq Andreas Färber
2014-11-05 17:03 ` Sören Brinkmann
2014-11-06 3:51 ` Andreas Färber
2014-11-06 4:13 ` Sören Brinkmann
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=3656bd45ce11485ebe7dfa6b64ca87fe@BN1BFFO11FD023.protection.gbl \
--to=soren.brinkmann@xilinx.com \
--cc=Bjorn.Andersson@sonymobile.com \
--cc=heiko@sntech.de \
--cc=iivanov@mm-sol.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=linux-sh@vger.kernel.org \
--cc=michal.simek@xilinx.com \
--cc=rubini@unipv.it \
/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).