Re: [PATCH v6 06/13] gpio: mxc: replace of_device_is_compatible() with hwdata flags

[Andy Shevchenko]

On Mon, Oct 12, 2026 at 12:54 PM Peng Fan <peng.fan@xxxxxxxxxxx> wrote:
> On Mon, Oct 12, 2026 at 12:25:41PM +0300, Andy Shevchenko wrote:
> >On Sun, Oct 11, 2026 at 2:54 PM Peng Fan (OSS) <peng.fan@xxxxxxxxxxx> wrote:

...

> >>  struct mxc_gpio_hwdata {
> >>         unsigned dr_reg;
> >
> >>         unsigned high_level;
> >>         unsigned rise_edge;
> >>         unsigned fall_edge;
> >> +       unsigned int flags;
> >
> >Obviously you need another precursor patch to move from 'unsigned' to
> >'unsigned int' in the whole driver.
>
> Could we keep as it is? I try to avoid adding more changes if not critical.

No, be consistent. You can add a new member with just 'unsigned' type,
but it's discouraged, sooner or later you should provide such a change
anyway.

> >>  };

...

> >> +#define MXC_GPIO_HW_DATA_COMMON        \
> >> +       .dr_reg         = 0x00, \
> >> +       .gdir_reg       = 0x04, \
> >> +       .psr_reg        = 0x08, \
> >> +       .icr1_reg       = 0x0c, \
> >> +       .icr2_reg       = 0x10, \
> >> +       .imr_reg        = 0x14, \
> >> +       .isr_reg        = 0x18, \
> >> +       .edge_sel_reg   = 0x1c, \
> >> +       .low_level      = 0x00, \
> >> +       .high_level     = 0x01, \
> >> +       .rise_edge      = 0x02, \
> >> +       .fall_edge      = 0x03
> >
> >Why?! Just to save a few lines? This makes it harder to follow the data part.
>
> If I expand the macro the imx7d, imx8qm, imx8qxp hwdata, 24 lines will be
> duplicated. I would keep as it is.
>
> The current hwdata as below:
> static struct mxc_gpio_hwdata imx7d_gpio_hwdata = {
>         MXC_GPIO_HW_DATA_COMMON,
>         .flags = MXC_GPIO_HAS_POWER_OFF,
> };
>
> static struct mxc_gpio_hwdata imx8qm_gpio_hwdata = {
>         MXC_GPIO_HW_DATA_COMMON,
>         .flags = MXC_GPIO_FALL_EDGE_WAKEUP_BROKEN | MXC_GPIO_HAS_PAD_WAKEUP,
> };
>
> static struct mxc_gpio_hwdata imx8qxp_gpio_hwdata = {
>         MXC_GPIO_HW_DATA_COMMON,
>         .flags = MXC_GPIO_HAS_PAD_WAKEUP,
> };

Yes, I saw it in the next patch(es) and commented there that what
should be done is split of the common part to another data structure
(or be wrapped into one with a .flags member).

-- 
With Best Regards,
Andy Shevchenko