Re: [PATCH v2 2/8] pinctrl: Add driver for the T-Head TH1520 SoC

Andy Shevchenko
2024-05-11T06:07:20Z

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