Re: [PATCH v2 2/7] gpio: Add Nuvoton NCT6694 GPIO support

[Ming Yu]

Dear Bartosz,

Thank you for your comments,

Bartosz Golaszewski <brgl@xxxxxxxx> 於 2024年11月21日 週四 下午4:28寫道：
>
> > +struct nct6694_gpio_data {
> > +       struct nct6694 *nct6694;
> > +       struct gpio_chip gpio;
> > +       struct mutex lock;
> > +       /* Protect irq operation */
> > +       struct mutex irq_lock;
> > +
> > +       unsigned char xmit_buf;
> > +       unsigned char irq_trig_falling;
> > +       unsigned char irq_trig_rising;
> > +
> > +       /* Current gpio group */
> > +       unsigned char group;
> > +
> > +       /* GPIO line names */
> > +       char **names;
>
> You only use this in probe() and after assigning it to gc->names, you
> never reference it again. You don't need this field here, it can be a
> local variable in probe().
>

Understood. I will modify it in the next patch.

> > +};
...
> > +
> > +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +       if (!data)
> > +               return -ENOMEM;
> > +
> > +       data->names = devm_kzalloc(dev, sizeof(char *) * NCT6694_NR_GPIO,
>
> devm_kcalloc()?
>

Okay, fix it in v3.

> > +                                  GFP_KERNEL);
...
> > +       mutex_init(&data->irq_lock);
> > +
> > +       platform_set_drvdata(pdev, data);
>
> There is no corresponding platform_get_drvdata() so you don't need this.
>

Okay, I'll drop it.

> > +
...
> > +module_platform_driver(nct6694_gpio_driver);
> > +
> > +MODULE_DESCRIPTION("USB-GPIO controller driver for NCT6694");
> > +MODULE_AUTHOR("Ming Yu <tmyu0@xxxxxxxxxxx>");
> > +MODULE_LICENSE("GPL");
>
> It's an MFD device, don't you need a MODULE_ALIAS() for this module to load?
>

I will add MODULE_ALIAS() for each child driver.


Best regards,
Ming