mbox series

[v4,0/4] ADP5585 GPIO expander, PWM and keypad controller support

Message ID 20240608141633.2562-1-laurent.pinchart@ideasonboard.com
Headers show
Series ADP5585 GPIO expander, PWM and keypad controller support | expand

Message

Laurent Pinchart June 8, 2024, 2:16 p.m. UTC
Hello,

This patch series introduces support for the Analog Devices ADP5585, a
GPIO expander, PWM and keyboard controller. It models the chip as an MFD
device, and includes DT bindings (1/4), an MFD driver (2/4) and drivers
for the GPIO (3/4) and PWM (4/4) functions.

Support for the keypad controller is left out, as I have no means to
test it at the moment. The chip also includes a tiny reset controller,
as well as a 3-bit input programmable logic block, which I haven't tried
to support (and also have no means to test).

The driver is based on an initial version from the NXP BSP kernel, then
extensively and nearly completely rewritten, with added DT bindings. I
have nonetheless retained original authorship. Clark, Haibo, if you
would prefer not being credited and/or listed as authors, please let me
know.

Compared to v3, this version addresses small review comments. I believe
it is ready to go, pending another review of the PWM side (Uwe reviewed
a previous version, and to the best of my knowledge, I've addressed all
his concerns) and the MFD driver. Once the PWM driver gets reviewed, I
think the simplest course of action is to merge the whole series through
the MFD tree.

Clark Wang (1):
  pwm: adp5585: Add Analog Devices ADP5585 support

Haibo Chen (2):
  mfd: adp5585: Add Analog Devices ADP5585 core support
  gpio: adp5585: Add Analog Devices ADP5585 support

Laurent Pinchart (1):
  dt-bindings: mfd: Add Analog Devices ADP5585

 .../devicetree/bindings/mfd/adi,adp5585.yaml  |  90 +++++++
 .../devicetree/bindings/trivial-devices.yaml  |   4 -
 MAINTAINERS                                   |  11 +
 drivers/gpio/Kconfig                          |   7 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-adp5585.c                   | 231 ++++++++++++++++++
 drivers/mfd/Kconfig                           |  12 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/adp5585.c                         | 199 +++++++++++++++
 drivers/pwm/Kconfig                           |   7 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-adp5585.c                     | 189 ++++++++++++++
 include/linux/mfd/adp5585.h                   | 126 ++++++++++
 13 files changed, 875 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
 create mode 100644 drivers/gpio/gpio-adp5585.c
 create mode 100644 drivers/mfd/adp5585.c
 create mode 100644 drivers/pwm/pwm-adp5585.c
 create mode 100644 include/linux/mfd/adp5585.h


base-commit: c3f38fa61af77b49866b006939479069cd451173

Comments

Andy Shevchenko June 10, 2024, 3:15 p.m. UTC | #1
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.

> +};
Laurent Pinchart June 10, 2024, 3:25 p.m. UTC | #2
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.

> > +};
Andy Shevchenko June 10, 2024, 4:29 p.m. UTC | #3
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?

> > > +};
Laurent Pinchart July 19, 2024, 1:55 p.m. UTC | #4
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.

> > > > +};
Andy Shevchenko Aug. 1, 2024, 11:15 p.m. UTC | #5
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.

> > > > > +};