Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Date:   Wed, 22 Nov 2023 13:50:47 +0200
From:   Andy Shevchenko <andriy.shevchenko@intel.com>
To:     Nikita Shubin <nikita.shubin@maquefel.me>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org,
        Alexander Sverdlin <alexander.sverdlin@gmail.com>
Subject: Re: [PATCH v5 04/39] pinctrl: add a Cirrus ep93xx SoC pin controller
Message-ID: <ZV3rF2QKLVClp1jh@smile.fi.intel.com>
References: <20231122-ep93xx-v5-0-d59a76d5df29@maquefel.me>
 <20231122-ep93xx-v5-4-d59a76d5df29@maquefel.me>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231122-ep93xx-v5-4-d59a76d5df29@maquefel.me>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Wed, Nov 22, 2023 at 11:59:42AM +0300, Nikita Shubin wrote:
> Add a pin control (only multiplexing) driver for ep93xx SoC so
> we can fully convert ep93xx to device tree.
> 
> This driver is capable of muxing ep9301/ep9302/ep9307/ep9312/ep9315
> variants, this is chosen based on "compatible" in device tree.

...

> +config PINCTRL_EP93XX
> +	bool
> +	depends on OF && (ARCH_EP93XX || COMPILE_TEST)

How OF is here?

...

> +#include <linux/of.h>

Ditto.

...

> +#include <linux/slab.h>

+ blank line?

> +#include <linux/soc/cirrus/ep93xx.h>

...

> +/* ep9301, ep9302*/

Missing space.

...

> +static const unsigned int ssp_ep9301_pins[] = {
> +	93, 94, 95, 96

In multi-line definitions like this it makes sense to leave trailing comma.

> +};
> +
> +static const unsigned int ac97_ep9301_pins[] = {
> +	89, 92, 107, 154, 156

Ditto.

And in some other places.

> +};

...

> +	/* Row C*/

Missing space. I noticed in more comments like this, please grep and fix
all of them.

...

> +static const char *ep93xx_get_group_name(struct pinctrl_dev *pctldev,
> +					 unsigned int selector)
> +{
> +	struct ep93xx_pmx *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	switch (pmx->model) {
> +	case EP93XX_9301_PINCTRL:
> +		return ep9301_pin_groups[selector].grp.name;
> +	case EP93XX_9307_PINCTRL:
> +		return ep9307_pin_groups[selector].grp.name;
> +	case EP93XX_9312_PINCTRL:
> +		return ep9312_pin_groups[selector].grp.name;
> +	}
> +
> +	return NULL;

Make it default case.

> +}

...

> +	dev_dbg(pmx->dev,
> +		"before=0x%x, after=0x%x, mask=0x%lx\n",
> +		before, after, PADS_MASK);

At least two first can be on a single line.

...

> +	/* Which bits changed */
> +	before &= PADS_MASK;
> +	after &= PADS_MASK;

> +	expected = before & ~grp->mask;
> +	expected |= grp->value;

Usually we use this pattern:

	expected = (before & ~grp->mask) | (grp->value & grp->mask);

but I don't know the full spectre of the meanings of these pieces, so just
consider it once more.

> +	expected &= PADS_MASK;

...

> +	pmx->model = (int)(id->driver_data);

Wouldn't it warn? Maybe not (it's 32-bit code, right?), but better to use

	pmx->model = (int)(uintptr_t)id->driver_data;

...

> +	/* using parent of_node to match in get_pinctrl_dev_from_of_node() */
> +	dev->of_node = adev->dev.parent->of_node;

device_set_node()

...

> +	pmx->pctl = devm_pinctrl_register(dev, &ep93xx_pmx_desc, pmx);
> +	if (IS_ERR(pmx->pctl)) {

> +		dev_err(dev, "could not register pinmux driver\n");
> +		return PTR_ERR(pmx->pctl);

Why not dev_err_probe() here?

> +	}

-- 
With Best Regards,
Andy Shevchenko