Message-ID:
 <TYUPR06MB5876BB28E3C30EEB9BB05997EF15A@TYUPR06MB5876.apcprd06.prod.outlook.com>
Date: Mon, 15 Sep 2025 07:09:01 +0000
From: Gary Yang <gary.yang@...tech.com>
To: Linus Walleij <linus.walleij@...aro.org>
CC: "robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, cix-kernel-upstream
	<cix-kernel-upstream@...tech.com>
Subject:
 回复: [v2 1/3] pinctrl: cix: Add pin-controller support for sky1

Hi Linus

Thanks for your review and suggestions below

> EXTERNAL EMAIL
> 
> 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.
> 

OK, we will add more description as following, How about it?

The pin-controller is used to control Soc pins. It contains two parts: pinmux and pinconfig.
Pinmux is used to select pin function and pinconfig is used to configure pin. It includes
Pull-up,pull-down,and drive strength. There are two pin-controllers on Cix Sky1 platform. 
one is used under S0 state, the other one is used under S0 and S5 state.

> > 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.

Yes, it looks better. I agree this change

> 
> > +#define SKY1_MUX_SHIFT         (0x7)
> > +#define SKY1_PULLCONF_MASK     (0x60)
> 
> Same idea here.

Same things

> 
> > +#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.
> 

Same things

> > +#define SKY1_DS_MASK           (0x0f)
> 
> Use GENMASK()
> 

Same things

> > +#define CIX_GET_PIN_NO(x) ((x) >> 8)
> > +#define CIX_GET_PIN_FUNC(x) ((x) & 0xf)
> 
> Maybe define 8 and 0xf as shifts?
> 

Same things

> > +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;
> 
> > +};

First, I understand your thinkings. please pay some attention to the type of func->name, it's a pointer, 

Checking whether this pointer is a null pointer is generally acceptable. A name maps to a mux value.

I think that it is safe. Of course, your suggestion is also a good idea. If you think this is not safe, we will 

change codes as your suggestions.

> 
> 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.
> 

First, let us review the struct sky1_pin_desc, it contains two members, one is the struct pinctrl_pin_desc.

struct pinctrl_pin_desc {
        unsigned int number;
        const char *name;
        void *drv_data;
};

PINCTRL_PIN is used to initialize this struct in kernel. It locates in include/linux/pinctrl/pinctrl.h

#define PINCTRL_PIN(a, b) { .number = a, .name = b }

PINCTRL_PIN(0, "GPIO1") defines a pin, its number is 0, its name is "GPIO1".

Second, I ever think that use SKY_PINFUNCTION macro to define pins, when a pin has serval functions,

more than one function, we need define a function array for the pin and then pass the array to 

SKY_PINFUNCTION. Maybe it doesn't save anything, it takes more time in reading these codes to understand. 

What's your opinion?

> > +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!
> 

First maybe you ignore that fact the struct sky1_pinctrl_group is different from 

the struct group_desc. It only saves the config of a pin from dts. it doesn't include 

pin function part. As we know, a pin only has a config at the same time. One group map to a pin.

The pin function part is included in the struct sky1_function_desc. One pin can map

serval functions. There are four functions on sky1. We define the sky1_gpio_functions array.

It is used to select the pin functions. 

Second, you are right. the pinctrl driver support new scheme is seldom in kernel. You take mt723 as

an example before. Some codes come from the pinctrl driver on MTK. We modify them to adopt our platform.

If I misunderstand or miss anything, please let me know.

> The pinconf part of the driver looks very good to me.
> 
> Look over these things, and keep posting updates!
> 
> Yours,
> Linus Walleij

Best wishes
Gary

Powered by blists - more mailing lists