| Message ID | 20240608141633.2562-1-laurent.pinchart@ideasonboard.com |
|---|---|
| Headers | show |
| Series | ADP5585 GPIO expander, PWM and keypad controller support | expand |
Sat, Jun 08, 2024 at 05:16:32PM +0300, Laurent Pinchart kirjoitti: > From: Haibo Chen <haibo.chen@nxp.com> > > The ADP5585 is a 10/11 input/output port expander with a built in keypad > matrix decoder, programmable logic, reset generator, and PWM generator. > This driver supports the GPIO function using the platform device > registered by the core MFD driver. > > The driver is derived from an initial implementation from NXP, available > in commit 451f61b46b76 ("MLK-25917-2 gpio: adp5585-gpio: add > adp5585-gpio support") in their BSP kernel tree. It has been extensively > rewritten. ... > +static const struct platform_device_id adp5585_gpio_id_table[] = { > + { "adp5585-gpio" }, > + { /* Sentinel */ }, Drop the comma. > +};
On Mon, Jun 10, 2024 at 06:15:40PM +0300, Andy Shevchenko wrote: > Sat, Jun 08, 2024 at 05:16:32PM +0300, Laurent Pinchart kirjoitti: > > From: Haibo Chen <haibo.chen@nxp.com> > > > > The ADP5585 is a 10/11 input/output port expander with a built in keypad > > matrix decoder, programmable logic, reset generator, and PWM generator. > > This driver supports the GPIO function using the platform device > > registered by the core MFD driver. > > > > The driver is derived from an initial implementation from NXP, available > > in commit 451f61b46b76 ("MLK-25917-2 gpio: adp5585-gpio: add > > adp5585-gpio support") in their BSP kernel tree. It has been extensively > > rewritten. > > ... > > > +static const struct platform_device_id adp5585_gpio_id_table[] = { > > + { "adp5585-gpio" }, > > > + { /* Sentinel */ }, > > Drop the comma. I prefer keeping it. > > +};
On Mon, Jun 10, 2024 at 6:26 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Mon, Jun 10, 2024 at 06:15:40PM +0300, Andy Shevchenko wrote: > > Sat, Jun 08, 2024 at 05:16:32PM +0300, Laurent Pinchart kirjoitti: ... > > > +static const struct platform_device_id adp5585_gpio_id_table[] = { > > > + { "adp5585-gpio" }, > > > > > + { /* Sentinel */ }, > > > > Drop the comma. > > I prefer keeping it. For what reason? The sentinel should be runtime and compile time one. Why should we make our lives worse by neglecting help from a compiler? > > > +};
Hello Andy, On Mon, Jun 10, 2024 at 07:29:09PM +0300, Andy Shevchenko wrote: > On Mon, Jun 10, 2024 at 6:26 PM Laurent Pinchart wrote: > > On Mon, Jun 10, 2024 at 06:15:40PM +0300, Andy Shevchenko wrote: > > > Sat, Jun 08, 2024 at 05:16:32PM +0300, Laurent Pinchart kirjoitti: > > ... > > > > > +static const struct platform_device_id adp5585_gpio_id_table[] = { > > > > + { "adp5585-gpio" }, > > > > > > > + { /* Sentinel */ }, > > > > > > Drop the comma. > > > > I prefer keeping it. > > For what reason? > The sentinel should be runtime and compile time one. Why should we > make our lives worse by neglecting help from a compiler? Do you really think there's a risk here and that this will make a difference ? I do appreciate most of your review comments, even pendantic ones, as they can help making the code better, but we also all need a little bit of space to breathe when it comes to coding style. > > > > +};
On Fri, Jul 19, 2024 at 3:55 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Mon, Jun 10, 2024 at 07:29:09PM +0300, Andy Shevchenko wrote: > > On Mon, Jun 10, 2024 at 6:26 PM Laurent Pinchart wrote: > > > On Mon, Jun 10, 2024 at 06:15:40PM +0300, Andy Shevchenko wrote: > > > > Sat, Jun 08, 2024 at 05:16:32PM +0300, Laurent Pinchart kirjoitti: ... > > > > > +static const struct platform_device_id adp5585_gpio_id_table[] = { > > > > > + { "adp5585-gpio" }, > > > > > > > > > + { /* Sentinel */ }, > > > > > > > > Drop the comma. > > > > > > I prefer keeping it. > > > > For what reason? > > The sentinel should be runtime and compile time one. Why should we > > make our lives worse by neglecting help from a compiler? > > Do you really think there's a risk here and that this will make a > difference ? There are two aspects (or more?): 1) potential mis-rebase or other thing that makes possible to have an entry _after_ the terminator and having it being compiled successfully, while we may prevent this from happening on the compilation phase (as you noticed this is quite unlikely to happen IRL); 2) educational part, as somebody may use your code as a good standard. > I do appreciate most of your review comments, even > pendantic ones, as they can help making the code better, but we also all > need a little bit of space to breathe when it comes to coding style. Some of the coding style decisions can be considered slightly better than others. I have a rationale for this case. But of course, it's up to you and the subsystem maintainer on how to proceed with this. I wouldn't take your breath away. > > > > > +};