Message-ID: <CACRpkdYgTjerG5mks_+3sjhKKYtCsFY=1NWhgw_YEuib7gZm3g@mail.gmail.com>
Date: Fri, 12 Sep 2025 13:59:00 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: Gary Yang <gary.yang@...tech.com>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	linux-gpio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	cix-kernel-upstream@...tech.com
Subject: Re: [v2 1/3] pinctrl: cix: Add pin-controller support for sky1

Hi Gary,

thanks for your patch!

On Fri, Sep 12, 2025 at 8:06 AM Gary Yang <gary.yang@...tech.com> wrote:

> Add the pin-controller driver for Sky1 platform
>

Add some more description of the pin control on the SoC here please.

> Signed-off-by: Gary Yang <gary.yang@...tech.com>
(...)

Config structure in Kconfig looks good!

> +++ b/drivers/pinctrl/cix/pinctrl-sky1-base.c
> @@ -0,0 +1,581 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +//
> +// Author: Jerry Zhu <Jerry.Zhu@...tech.com>
> +
> +#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>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/platform_device.h>
> +#include <linux/seq_file.h>
> +#include <linux/slab.h>
> +
> +#include "../core.h"
> +#include "../pinconf.h"
> +#include "../pinctrl-utils.h"
> +#include "../pinmux.h"
> +#include "pinctrl-sky1.h"
> +
> +#define SKY1_PIN_SIZE          (0x4)
> +#define SKY1_MUX_MASK          (0x180)

For masks do this:

#include <linux/bits.h>

#define SKY1_MUX_MASK GENMASK(8, 7)

GENMASK generates a bitmask from bit 7 to 8, 0x180.

If its a 32bit register you can use GENMASK_U32() to be
even clearer, etc.

> +#define SKY1_MUX_SHIFT         (0x7)
> +#define SKY1_PULLCONF_MASK     (0x60)

Same idea here.

> +#define SKY1_PULLUP_SHIFT      (0x6)
> +#define SKY1_PULLDN_SHIFT      (0x5)

I would probably do this:

#include <linus/bits.h>

#define SKY1_PULLDN BIT(5)
#define SKY1_PULLUP BIT(6)

Using simple bit references to define what each bit
is for.

> +#define SKY1_DS_MASK           (0x0f)

Use GENMASK()

> +#define CIX_GET_PIN_NO(x) ((x) >> 8)
> +#define CIX_GET_PIN_FUNC(x) ((x) & 0xf)

Maybe define 8 and 0xf as shifts?

> +static const struct sky1_function_desc *sky1_pctrl_find_function_by_pin(
> +               struct sky1_pinctrl *spctl, u32 pin_num, u32 fnum)
> +{
> +       const struct sky1_pin_desc *pin = spctl->info->pins + pin_num;
> +       const struct sky1_function_desc *func = pin->functions;
> +
> +       while (func && func->name) {
> +               if (func->muxval == fnum)
> +                       return func;
> +               func++;
> +       }

Using a NULL func->name to terminate the array looks a bit dangerous.

What about adding:

> +struct sky1_function_desc {
> +       unsigned char muxval;
> +       const char *name;

const char * const *functions;
size_t nfuncs;

> +};

Then you can use nfuncs to iterate over the array of
function names, and define a macro like this:

#define SKY_PINFUNCTION(_muxval, _functions, _nfunctions)   \
(struct sky1_function_desc) {                                  \
                .muxval = (muxval),                        \
                .functions = (_functions),                    \
                .nfuncs = (_nfunctions),                  \
        }

And then this:

+static const struct sky1_pin_desc sky1_pinctrl_s5_pads[] = {
> +       {
> +               .pin = PINCTRL_PIN(0, "GPIO1"),
> +               .functions = {
> +                       [0] = {0, "GPIO1"},
> +               },
> +       },
> +       {
> +               .pin = PINCTRL_PIN(1, "GPIO2"),
> +               .functions = {
> +                       [0] = {0, "GPIO2"},
> +               },

> +       },

becomes

static const struct sky1_pin_desc sky1_pinctrl_s5_pads[] = {
    SKY_PINFUNCTION(PINCTRL_PIN(0, "GPIO1"),  "GPIO1", 1),
    SKY_PINFUNCTION(PINCTRL_PIN(1, "GPIO2"),  "GPIO2", 1),

I don't know about using the PINCTRL_PIN() macro here though,
can't you just put in 0, 1...?

Anyway I think you get the idea.

> +static bool sky1_pctrl_is_function_valid(struct sky1_pinctrl *spctl,
> +               u32 pin_num, u32 fnum)
> +{
> +       int i;
> +
> +       for (i = 0; i < spctl->info->npins; i++) {
> +               const struct sky1_pin_desc *pin = spctl->info->pins + i;
> +
> +               if (pin->pin.number == pin_num) {
> +                       const struct sky1_function_desc *func =
> +                                       pin->functions;
> +
> +                       while (func && func->name) {

So here you could just for (i = 0; i++; i < func->nfuncs)

(etc everywhere)

> +static int sky1_pctrl_dt_node_to_map_func(struct sky1_pinctrl *spctl,
> +               u32 pin, u32 fnum, struct sky1_pinctrl_group *grp,
> +               struct pinctrl_map **map, unsigned int *reserved_maps,
> +               unsigned int *num_maps)
> +{
> +       bool ret;
> +
> +       if (*num_maps == *reserved_maps)
> +               return -ENOSPC;
> +
> +       (*map)[*num_maps].type = PIN_MAP_TYPE_MUX_GROUP;
> +       (*map)[*num_maps].data.mux.group = grp->name;
> +
> +       ret = sky1_pctrl_is_function_valid(spctl, pin, fnum);
> +       if (!ret) {
> +               dev_err(spctl->dev, "invalid function %d on pin %d .\n",
> +                               fnum, pin);
> +               return -EINVAL;
> +       }
> +
> +       (*map)[*num_maps].data.mux.function = sky1_gpio_functions[fnum];
> +       (*num_maps)++;
> +
> +       return 0;
> +}
> +
> +static struct sky1_pinctrl_group *
> +sky1_pctrl_find_group_by_pin(struct sky1_pinctrl *spctl, u32 pin)
> +{
> +       int i;
> +
> +       for (i = 0; i < spctl->info->npins; i++) {
> +               struct sky1_pinctrl_group *grp =
> +                       (struct sky1_pinctrl_group *)spctl->groups + i;
> +
> +               if (grp->pin == pin)
> +                       return grp;
> +       }
> +
> +       return NULL;
> +}

And this:

> +struct sky1_pinctrl_group {
> +       const char *name;
> +       unsigned long config;
> +       unsigned int pin;
> +};

it's a bit conceptually weird.

Usually a pin can be member of many groups.

The only time this works is when the pin controller is of the type
where every pin is placed in a single group with only that pin in it.

And that seems to be the case, because:

> +static int sky1_pctrl_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> +       struct sky1_pinctrl *spctl = pinctrl_dev_get_drvdata(pctldev);
> +
> +       return spctl->info->npins;
> +}

If this is the implied pattern for this driver, write as a comment to
the above function that this pin controller place all pins into a
single group with one pin and that this is why this works.

The normal (as can be seen from the pin control documentation
https://docs.kernel.org/driver-api/pin-control.html )
is to group pins, so e.g.

uart0_rx_tx_grp = { pin1, pin2 };
i2c0_sda_scl_grp = { pin1, pin2 };

Then this is combined with functions such as uart0 and i2c0:

function, group
("uart0", uart0_rx_tx_grp)
("i2c0", i2c0_sda_scl_grp)

Here you see the two pins are used for uart in the first case
and for i2c in the second case, it's the same pins, but members
of two different groups, and these groups are then used with
a function.

The possible functions for a group are then defined somewhere
so these settings can be applied.

Maybe this pattern is something you have in your driver
because the code was copied from some other driver
which use one group per pin, it's not certain that this is the
best layout for the cix SoC so look it over!

The pinconf part of the driver looks very good to me.

Look over these things, and keep posting updates!

Yours,
Linus Walleij

Powered by blists - more mailing lists