Re: [PATCH v6 10/13] pinctrl: ambarella: add CV75 pin controller

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Sep 04, 2026 at 02:38:17PM +0800, Long Zhao via B4 Relay wrote:

> Add an Ambarella pinmux-only pinctrl driver with CV75 function/group
> tables. GPIO is handled by the PL061 driver.

...

> +#include <linux/kernel.h>

Absolutely no. Follow IWYU principle in all your patches.

...

> +#define CV75_GROUP(_name)						\
> +	{								\
> +		.name = #_name,						\
> +		.pinmux = cv75_##_name##_pinmux,				\
> +		.num_pins = ARRAY_SIZE(cv75_##_name##_pinmux),		\
> +	}
> +
> +#define CV75_FUNCTION(_name)						\
> +	{								\
> +		.name = #_name,						\
> +		.groups = cv75_##_name##_groups,				\
> +		.num_groups = ARRAY_SIZE(cv75_##_name##_groups),		\
> +	}

Don't we have respective macros in linux/pinctrl/pinctrl.h

...

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/bitmap.h>
> +#include <linux/io.h>

> +#include <linux/of.h>
> +#include <linux/of_device.h>



> +#include <linux/slab.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/seq_file.h>

Make it ordered, group linux/pinctrl/ separately after generic linux/*.h

> +#include "pinconf.h"
> +#include "pinctrl-ambarella.h"

...

> +struct amb_pinctrl_soc_data {
> +	struct device			*dev;
> +	const struct amb_pinctrl_data	*data;
> +	void __iomem			*iomux_base;
> +	struct regmap			*ds_regmap;
> +	struct regmap			*pull_regmap;
> +	unsigned int			npins;
> +	unsigned long			used[BITS_TO_LONGS(AMBA_MAX_PINS)];
> +	raw_spinlock_t lock;
> +
> +	struct pinctrl_dev		*pctl;

> +	const struct ambpin_function	*functions;
> +	unsigned int			nr_functions;
> +	struct ambpin_group		*groups;
> +	unsigned int			nr_groups;

Check what linux/pinctrl/pinctrl.h and drivers/pinctrl/core.h provide.

> +};

...

> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +static void amb_pin_dbg_show(struct pinctrl_dev *pctldev,
> +			     struct seq_file *s, unsigned int pin)
> +{
> +	seq_printf(s, " %s", pinctrl_dev_get_devname(pctldev));
> +}
> +#endif

Does this bring any new information?

...

> +/* check if the selector is a valid pin function selector */
> +static int amb_pinmux_request(struct pinctrl_dev *pctldev, unsigned int pin)
> +{
> +	struct amb_pinctrl_soc_data *soc = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (test_and_set_bit(pin, soc->used))
> +		return -EBUSY;
> +
> +	return 0;
> +}
> +
> +/* check if the selector is a valid pin function selector */
> +static int amb_pinmux_free(struct pinctrl_dev *pctldev, unsigned int pin)
> +{
> +	struct amb_pinctrl_soc_data *soc = pinctrl_dev_get_drvdata(pctldev);
> +
> +	clear_bit(pin, soc->used);
> +
> +	return 0;
> +}

Why? Isn't internal pinctrl tracking is not enough?

...

> +static void amb_pinmux_set_altfunc(struct amb_pinctrl_soc_data *soc,
> +				   u32 bank, u32 offset, u32 altfunc)
> +{
> +	u32 i, data;
> +
> +	if (!amb_iomux_accessible(soc))
> +		return;
> +
> +	for (i = 0; i < 3; i++) {

	for (unsigned int i...

> +		data = readl_relaxed(soc->iomux_base + IOMUX_OFFSET(bank, i));
> +		data &= ~BIT(offset);
> +		data |= ((altfunc >> i) & 1U) << offset;
> +		writel_relaxed(data, soc->iomux_base + IOMUX_OFFSET(bank, i));
> +	}
> +}

...

> +static int amb_drive_strength_to_reg(struct amb_pinctrl_soc_data *soc,
> +				     u32 strength)
> +{
> +	if (soc->data->have_ds2) {
> +		switch (strength) {
> +		case 3:
> +			return 0;
> +		case 4:
> +		case 5:
> +			return 1;
> +		case 6:
> +			return 2;
> +		case 7:
> +		case 8:
> +			return 3;
> +		case 9:
> +			return 4;
> +		case 12:
> +			return 5;
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	switch (strength) {
> +	case 2:
> +		return 0;
> +	case 4:
> +		return 1;
> +	case 8:
> +		return 2;
> +	case 12:
> +		return 3;
> +	default:
> +		return -EINVAL;
> +	}
> +}

I would look better in three functions

static int amb_drive_strength_to_reg(struct amb_pinctrl_soc_data *soc,
				     u32 strength)
{
	if (soc->data->have_ds2)
		return amb_drive_strength_to_reg_ds2(soc, strength);

	return amb_drive_strength_to_reg_ds1(soc, strength);

}

> +static int amb_reg_to_drive_strength(struct amb_pinctrl_soc_data *soc, u32 ds)
> +{
> +	static const int ds2_ma[] = { 3, 4, 6, 8, 9, 12 };
> +	static const int ds_ma[] = { 2, 4, 8, 12 };

You have switch cases that use the same arrays, use them for both.

> +	if (soc->data->have_ds2) {
> +		if (ds >= ARRAY_SIZE(ds2_ma))
> +			return -EINVAL;
> +
> +		return ds2_ma[ds];
> +	}
> +
> +	if (ds >= ARRAY_SIZE(ds_ma))
> +		return -EINVAL;
> +
> +	return ds_ma[ds];
> +}

...

I stopped here, this driver is quite far from the modern pin control standards,
please look around, you find a lot of new drivers there, that are written in
more-or-less compact ways, using better APIs and approaches.

When creating a driver, use also helper tools, like `pahole`,
scripts/bloat-o-meter, et cetera. Compile with `make W=1 C=1 ...`
using both compilers (GCC and clang).

...

> +static const struct of_device_id amb_pinctrl_dt_match[] = {
> +	{
> +		.compatible = "ambarella,cv75-pinctrl",
> +		.data = &ambarella_cv75_pinctrl_data,
> +	},
> +	{},

No trailing comma for the terminator entry.

> +};

...

> +static struct platform_driver amb_pinctrl_driver = {
> +	.probe	= amb_pinctrl_probe,
> +	.driver	= {
> +		.name	= "ambarella-pinctrl",
> +		.of_match_table = of_match_ptr(amb_pinctrl_dt_match),

Why of_match_ptr()?!

> +	},
> +};

-- 
With Best Regards,
Andy Shevchenko