Re: [PATCH v3 2/3] pinctrl: cix: Add pin-controller support for sky1

[Linus Walleij <linus.walleij@linaro.org>]

Hi Gary,

this looks very nice, as long as we finish the bindings we can
soon merge this I hope.

Some small comments!

On Tue, Oct 14, 2025 at 3:57 AM Gary Yang <gary.yang@cixtech.com> wrote:


> There are two pin-controllers on Cix Sky1 platform.
> one is used under S0 state, the other is used under S0 and S5 state.
>
> Signed-off-by: Gary Yang <gary.yang@cixtech.com>
(...)

> +config PINCTRL_SKY1
> +       tristate "Cix Sky1 pinctrl driver"
> +       depends on ARCH_CIX

Maybe depends on ARCH_CIX || COMPILE_TEST so we
get some compile testing in the test farms and also a test
on the rest of the dependencies.

(Makes the bots complain so we can fix all such things!)

> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/pinctrl/machine.h>

Do you really need <linux/pinctrl/machine.h>?

Another thing you might want to consider is whether the
designware GPIO will use this pin controller as
"back-end" for the GPIOs using gpio-ranges in the
GPIO controller nodes, for example (I just made this up):

gpio-ranges = <&pinctrl1 0 20 10>, <&pinctrl2 10 50 20>;

Then you might want to implement the GPIO
accelerator operations in struct pinmux_ops, i.e. these:

 * @gpio_request_enable: requests and enables GPIO on a certain pin.
 *      Implement this only if you can mux every pin individually as GPIO. The
 *      affected GPIO range is passed along with an offset(pin number) into that
 *      specific GPIO range - function selectors and pin groups are orthogonal
 *      to this, the core will however make sure the pins do not collide.
 * @gpio_disable_free: free up GPIO muxing on a certain pin, the reverse of
 *      @gpio_request_enable
 * @gpio_set_direction: Since controllers may need different configurations
 *      depending on whether the GPIO is configured as input or output,
 *      a direction selector function may be implemented as a backing
 *      to the GPIO controllers that need pin muxing.
 * @strict: do not allow simultaneous use of the same pin for GPIO and another
 *      function. Check both gpio_owner and mux_owner strictly before approving
 *      the pin request.

And nowadays it is also worth considering using:

        bool (*function_is_gpio) (struct pinctrl_dev *pctldev,
                                  unsigned int selector);

To make the pinctrl core awara of a certain function selector being the
GPIO function (which makes the accelerated functions work much better
with the strict mode).

This can all be added later in separate patches, but this is a good time
to make sure nothing stands in the way of doing this.

Yours,
Linus Walleij