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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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