From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Emil Renner Berthing <emil.renner.berthing@canonical.com>
Cc: linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, linux-riscv@lists.infradead.org,
Linus Walleij <linus.walleij@linaro.org>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
Jisheng Zhang <jszhang@kernel.org>, Guo Ren <guoren@kernel.org>,
Fu Wei <wefu@redhat.com>,
Paul Walmsley <paul.walmsley@sifive.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Drew Fustini <dfustini@baylibre.com>
Subject: Re: [PATCH v2 2/8] pinctrl: Add driver for the T-Head TH1520 SoC
Date: Sat, 11 May 2024 09:06:55 +0300 [thread overview]
Message-ID: <Zj8K_0zpI_IAY66R@surfacebook.localdomain> (raw)
In-Reply-To: <20240103132852.298964-3-emil.renner.berthing@canonical.com>
Wed, Jan 03, 2024 at 02:28:39PM +0100, Emil Renner Berthing kirjoitti:
> Add pinctrl driver for the T-Head TH1520 RISC-V SoC.
...
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/seq_file.h>
> +#include <linux/spinlock.h>
+ types.h
...
> +#define TH1520_PAD(_nr, _name, m0, m1, m2, m3, m4, m5, _flags) \
> + { .number = _nr, .name = #_name, .drv_data = (void *)((_flags) | \
> + (TH1520_MUX_##m0 << 0) | (TH1520_MUX_##m1 << 5) | (TH1520_MUX_##m2 << 10) | \
> + (TH1520_MUX_##m3 << 15) | (TH1520_MUX_##m4 << 20) | (TH1520_MUX_##m5 << 25)) }
It's better to read in a form of
#define TH1520_PAD(_nr, _name, m0, m1, m2, m3, m4, m5, _flags) \
{ \
.number = _nr, \
.name = #_name, \
.drv_data = (void *)((_flags) | \
(TH1520_MUX_##m0 << 0) | (TH1520_MUX_##m1 << 5) | (TH1520_MUX_##m2 << 10) | \
(TH1520_MUX_##m3 << 15) | (TH1520_MUX_##m4 << 20) | (TH1520_MUX_##m5 << 25)) \
}
...
> +static void th1520_pin_dbg_show(struct pinctrl_dev *pctldev,
> + struct seq_file *s, unsigned int pin)
static void th1520_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
unsigned int pin)
(it even satisfies 80 characters limit)
...
> + nmaps = 0;
> + for_each_available_child_of_node(np, child) {
You want to use _scoped() variant.
> + int npins = of_property_count_strings(child, "pins");
> +
> + if (npins <= 0) {
Better to decouple definition and assignment
int npins;
npins = of_property_count_strings(child, "pins");
if (npins <= 0) {
Besides that, if npins can hold an error code...
> + of_node_put(child);
> + dev_err(thp->pctl->dev, "no pins selected for %pOFn.%pOFn\n",
> + np, child);
> + return -EINVAL;
...why is it being shadowed?
> + }
> + nmaps += npins;
> + if (of_property_present(child, "function"))
> + nmaps += npins;
> + }
> + map = kcalloc(nmaps, sizeof(*map), GFP_KERNEL);
> + if (!map)
> + return -ENOMEM;
> +
> + nmaps = 0;
> + mutex_lock(&thp->mutex);
> + for_each_available_child_of_node(np, child) {
You want to use _scoped() variant.
> + unsigned int rollback = nmaps;
> + enum th1520_muxtype muxtype;
> + struct property *prop;
> + const char *funcname;
> + const char **pgnames;
> + const char *pinname;
> + int npins;
> +
> + ret = pinconf_generic_parse_dt_config(child, pctldev, &configs, &nconfigs);
> + if (ret) {
> + dev_err(thp->pctl->dev, "%pOFn.%pOFn: error parsing pin config\n",
> + np, child);
> + goto put_child;
> + }
> + if (!of_property_read_string(child, "function", &funcname)) {
Why not traditional pattern, i.e. "errors first"?
if (of_property_read_string(child, "function", &funcname)) {
funcname = NULL;
} else {
...
}
> + muxtype = th1520_muxtype_get(funcname);
> + if (!muxtype) {
> + dev_err(thp->pctl->dev, "%pOFn.%pOFn: unknown function '%s'\n",
> + np, child, funcname);
> + ret = -EINVAL;
> + goto free_configs;
> + }
> +
> + funcname = devm_kasprintf(thp->pctl->dev, GFP_KERNEL, "%pOFn.%pOFn",
> + np, child);
With help of
struct device *dev = thp->pctl->dev;
this and other lines in this function become shorter and easier to read.
> + if (!funcname) {
> + ret = -ENOMEM;
> + goto free_configs;
> + }
> +
> + npins = of_property_count_strings(child, "pins");
> + pgnames = devm_kcalloc(thp->pctl->dev, npins, sizeof(*pgnames), GFP_KERNEL);
> + if (!pgnames) {
> + ret = -ENOMEM;
> + goto free_configs;
> + }
> + } else {
> + funcname = NULL;
> + }
> +
> + npins = 0;
> + of_property_for_each_string(child, "pins", prop, pinname) {
> + unsigned int i;
> +
> + for (i = 0; i < thp->desc.npins; i++) {
> + if (!strcmp(pinname, thp->desc.pins[i].name))
> + break;
> + }
> + if (i == thp->desc.npins) {
> + nmaps = rollback;
> + dev_err(thp->pctl->dev, "%pOFn.%pOFn: unknown pin '%s'\n",
> + np, child, pinname);
> + goto free_configs;
> + }
> +
> + if (nconfigs) {
> + map[nmaps].type = PIN_MAP_TYPE_CONFIGS_PIN;
> + map[nmaps].data.configs.group_or_pin = thp->desc.pins[i].name;
> + map[nmaps].data.configs.configs = configs;
> + map[nmaps].data.configs.num_configs = nconfigs;
> + nmaps += 1;
++ ?
> + }
> + if (funcname) {
> + pgnames[npins++] = thp->desc.pins[i].name;
> + map[nmaps].type = PIN_MAP_TYPE_MUX_GROUP;
> + map[nmaps].data.mux.function = funcname;
> + map[nmaps].data.mux.group = thp->desc.pins[i].name;
> + nmaps += 1;
++ ?
> + }
> + }
> +
> + if (funcname) {
> + ret = pinmux_generic_add_function(pctldev, funcname, pgnames,
> + npins, (void *)muxtype);
> + if (ret < 0) {
> + dev_err(thp->pctl->dev, "error adding function %s\n", funcname);
> + goto put_child;
> + }
> + }
> + }
> +
> + *maps = map;
> + *num_maps = nmaps;
> + mutex_unlock(&thp->mutex);
> + return 0;
> +
> +free_configs:
> + kfree(configs);
> +put_child:
> + of_node_put(child);
> + th1520_pinctrl_dt_free_map(pctldev, map, nmaps);
> + mutex_unlock(&thp->mutex);
> + return ret;
> +}
...
> + if ((uintptr_t)desc->drv_data & TH1520_PAD_NO_PADCFG)
You have a lot of explicit castings here and there, can you reduce the usage
count of them?
> + return -ENOTSUPP;
...
> + if ((uintptr_t)desc->drv_data & TH1520_PAD_NO_PADCFG)
At least this one can be moved to a helper (macro?).
> + return -ENOTSUPP;
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2024-05-11 6:06 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-03 13:28 [PATCH v2 0/8] Add T-Head TH1520 SoC pin control Emil Renner Berthing
2024-01-03 13:28 ` [PATCH v2 1/8] dt-bindings: pinctrl: Add thead,th1520-pinctrl bindings Emil Renner Berthing
2024-01-15 17:36 ` Rob Herring
2024-05-17 12:48 ` Emil Renner Berthing
2024-01-03 13:28 ` [PATCH v2 2/8] pinctrl: Add driver for the T-Head TH1520 SoC Emil Renner Berthing
2024-01-27 23:02 ` Linus Walleij
2024-05-11 6:06 ` Andy Shevchenko [this message]
2024-01-03 13:28 ` [PATCH v2 3/8] riscv: dts: thead: Add TH1520 pin control nodes Emil Renner Berthing
2024-01-08 17:34 ` Conor Dooley
2024-01-09 12:02 ` Emil Renner Berthing
2024-01-09 13:04 ` Conor Dooley
2024-01-09 14:28 ` Emil Renner Berthing
2024-01-09 17:34 ` Conor Dooley
2024-01-09 18:30 ` Drew Fustini
2024-01-03 13:28 ` [PATCH v2 4/8] riscv: dts: thead: Add TH1520 GPIO ranges Emil Renner Berthing
2024-01-03 13:28 ` [PATCH v2 5/8] riscv: dts: thead: Adjust TH1520 GPIO labels Emil Renner Berthing
2024-01-03 13:28 ` [PATCH v2 6/8] riscv: dts: thead: Add Lichee Pi 4M GPIO line names Emil Renner Berthing
2024-01-03 13:28 ` [PATCH v2 7/8] riscv: dts: thead: Add TH1520 pinctrl settings for UART0 Emil Renner Berthing
2024-01-03 13:28 ` [PATCH v2 8/8] riscv: dtb: thead: Add BeagleV Ahead LEDs Emil Renner Berthing
2024-02-06 19:25 ` [PATCH v2 0/8] Add T-Head TH1520 SoC pin control Conor Dooley
2024-05-06 14:28 ` Thomas Bonnefille
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=Zj8K_0zpI_IAY66R@surfacebook.localdomain \
--to=andy.shevchenko@gmail.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dfustini@baylibre.com \
--cc=emil.renner.berthing@canonical.com \
--cc=guoren@kernel.org \
--cc=jszhang@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=robh+dt@kernel.org \
--cc=wefu@redhat.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 read-only IMAP folder(s) and NNTP newsgroup(s).