mbox series

[v15,0/4] Add support for Maxim MAX735x/MAX736x variants

Message ID 20230817071056.2125679-1-patrick.rudolph@9elements.com
Headers show
Series Add support for Maxim MAX735x/MAX736x variants | expand

Message

Patrick Rudolph Aug. 17, 2023, 7:10 a.m. UTC 
v15:
- Clarified VDD usage on PCA9846 which has two VDD pins 

v14:
- Added comment for interrupt support

v13:
- Fix dt-binding
- Fixed nits

v12:
- Add separate patch correcting interrupt support in dt-binding
- Fix typo in commit message
- Make vdd-supply non optional

v11:
- Fix dt-binding example

v10:
- Small updates to dt-bindings
- Make vdd-supply optional
- Drop MAX7357 enhanced mode configuration

v9:
- Fix 'then' not aligned with 'if' in dt-bindings
- Split enhanced mode configuration into separate patch
- Add MAX7357/MAX7358 register definitions
- Rename config register defines
- Update comments and explain non default config being applied on MAX7357
- Check for I2C_FUNC_SMBUS_WRITE_BYTE_DATA functionality

v8:
- Move allOf in dt-binding and use double negation

v7:
- Reworked the commit message, comments and renamed a struct
  field. No functional change.

v6:
- Fix typo in dt-bindings

v5:
- Remove optional and make vdd-supply mandatory

v4:
- Add missing maxitems dt-bindings property

v3:
- Merge dt-bindings into i2c-mux-pca954x.yaml

v2:
- Move dt-bindings to separate file
- Added support for MAX736x as they are very similar
- Fixed an issue found by kernel test robot
- Dropped max735x property and custom IRQ check
- Added MAX7357 config register defines instead of magic values
- Renamed vcc-supply to vdd-supply

Patrick Rudolph (4):
  dt-bindings: i2c: pca954x: Correct interrupt support
  dt-bindings: i2c: Add Maxim MAX735x/MAX736x variants
  i2c: muxes: pca954x: Add MAX735x/MAX736x support
  i2c: muxes: pca954x: Add regulator support

 .../bindings/i2c/i2c-mux-pca954x.yaml         | 46 ++++++++--
 drivers/i2c/muxes/Kconfig                     |  6 +-
 drivers/i2c/muxes/i2c-mux-pca954x.c           | 92 +++++++++++++++++--
 3 files changed, 129 insertions(+), 15 deletions(-)

Comments

Peter Rosin Aug. 17, 2023, 4:21 p.m. UTC | #1 
Hi!

2023-08-17 at 09:10, Patrick Rudolph wrote:
> Only some of the PCA954x compatible ICs have interrupt
> capability, but the binding advertises it on all ICs.
> 
> Sync the dt-binding with the driver and only advertise it on:
>  - nxp,pca9542
>  - nxp,pca9543
>  - nxp,pca9544
>  - nxp,pca9545
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Acked-by: Peter Rosin <peda@axentia.se>

Cheers,
Peter
Peter Rosin Aug. 17, 2023, 4:22 p.m. UTC | #2 
Hi!

2023-08-17 at 09:10, Patrick Rudolph wrote:
> Add support for the following Maxim chips using the existing PCA954x
> driver:
> - MAX7356
> - MAX7357
> - MAX7358
> - MAX7367
> - MAX7368
> - MAX7369
> 
> All added Maxim chips behave like the PCA954x, where a single SMBUS byte
> write selects up to 8 channels to be bridged to the primary bus.
> 
> While the MAX7357/MAX7358 have interrupt support, they don't act as
> interrupt controller like the PCA9545 does. Thus don't enable IRQ support
> and handle them like the PCA9548.
> 
> Tested using the MAX7357.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>

Acked-by: Peter Rosin <peda@axentia.se>

/*
 * However, if you find yourself doing a v16 for some reason, please
 * update new multi-line comments to match the preferred coding style.
 */

Cheers,
Peter

> ---
>  drivers/i2c/muxes/Kconfig           |  6 +--
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 64 ++++++++++++++++++++++++++++-
>  2 files changed, 66 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index ea838dbae32e..db1b9057612a 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -65,11 +65,11 @@ config I2C_MUX_PCA9541
>  	  will be called i2c-mux-pca9541.
>  
>  config I2C_MUX_PCA954x
> -	tristate "NXP PCA954x and PCA984x I2C Mux/switches"
> +	tristate "NXP PCA954x/PCA984x and Maxim MAX735x/MAX736x I2C Mux/switches"
>  	depends on GPIOLIB || COMPILE_TEST
>  	help
> -	  If you say yes here you get support for the NXP PCA954x
> -	  and PCA984x I2C mux/switch devices.
> +	  If you say yes here you get support for NXP PCA954x/PCA984x
> +	  and Maxim MAX735x/MAX736x I2C mux/switch devices.
>  
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-mux-pca954x.
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 0ccee2ae5720..968111442625 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -4,6 +4,7 @@
>   *
>   * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
>   * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
> + * Copyright (c) 2022 9elements GmbH <patrick.rudolph@9elements.com>
>   *
>   * This module supports the PCA954x and PCA984x series of I2C multiplexer/switch
>   * chips made by NXP Semiconductors.
> @@ -11,6 +12,12 @@
>   *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547,
>   *	 PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849.
>   *
> + * It's also compatible to Maxims MAX735x I2C switch chips, which are controlled
> + * as the NXP PCA9548 and the MAX736x chips that act like the PCA9544.
> + *
> + * This includes the:
> + *	 MAX7356, MAX7357, MAX7358, MAX7367, MAX7368 and MAX7369
> + *
>   * These chips are all controlled via the I2C bus itself, and all have a
>   * single 8-bit register. The upstream "parent" bus fans out to two,
>   * four, or eight downstream busses or channels; which of these
> @@ -51,6 +58,12 @@
>  #define PCA954X_IRQ_OFFSET 4
>  
>  enum pca_type {
> +	max_7356,
> +	max_7357,
> +	max_7358,
> +	max_7367,
> +	max_7368,
> +	max_7369,
>  	pca_9540,
>  	pca_9542,
>  	pca_9543,
> @@ -90,8 +103,45 @@ struct pca954x {
>  	raw_spinlock_t lock;
>  };
>  
> -/* Provide specs for the PCA954x types we know about */
> +/* Provide specs for the MAX735x, PCA954x and PCA984x types we know about */
>  static const struct chip_desc chips[] = {
> +	[max_7356] = {
> +		.nchans = 8,
> +		.muxtype = pca954x_isswi,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +	},
> +	[max_7357] = {
> +		.nchans = 8,
> +		.muxtype = pca954x_isswi,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +		/* No interrupt controller support.
> +		   The interrupt provides information about stuck channels. */
> +	},
> +	[max_7358] = {
> +		.nchans = 8,
> +		.muxtype = pca954x_isswi,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +		/* No interrupt controller support.
> +		   The interrupt provides information about stuck channels. */
> +	},
> +	[max_7367] = {
> +		.nchans = 4,
> +		.muxtype = pca954x_isswi,
> +		.has_irq = 1,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +	},
> +	[max_7368] = {
> +		.nchans = 4,
> +		.muxtype = pca954x_isswi,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +	},
> +	[max_7369] = {
> +		.nchans = 4,
> +		.enable = 0x4,
> +		.muxtype = pca954x_ismux,
> +		.has_irq = 1,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +	},
>  	[pca_9540] = {
>  		.nchans = 2,
>  		.enable = 0x4,
> @@ -177,6 +227,12 @@ static const struct chip_desc chips[] = {
>  };
>  
>  static const struct i2c_device_id pca954x_id[] = {
> +	{ "max7356", max_7356 },
> +	{ "max7357", max_7357 },
> +	{ "max7358", max_7358 },
> +	{ "max7367", max_7367 },
> +	{ "max7368", max_7368 },
> +	{ "max7369", max_7369 },
>  	{ "pca9540", pca_9540 },
>  	{ "pca9542", pca_9542 },
>  	{ "pca9543", pca_9543 },
> @@ -194,6 +250,12 @@ static const struct i2c_device_id pca954x_id[] = {
>  MODULE_DEVICE_TABLE(i2c, pca954x_id);
>  
>  static const struct of_device_id pca954x_of_match[] = {
> +	{ .compatible = "maxim,max7356", .data = &chips[max_7356] },
> +	{ .compatible = "maxim,max7357", .data = &chips[max_7357] },
> +	{ .compatible = "maxim,max7358", .data = &chips[max_7358] },
> +	{ .compatible = "maxim,max7367", .data = &chips[max_7367] },
> +	{ .compatible = "maxim,max7368", .data = &chips[max_7368] },
> +	{ .compatible = "maxim,max7369", .data = &chips[max_7369] },
>  	{ .compatible = "nxp,pca9540", .data = &chips[pca_9540] },
>  	{ .compatible = "nxp,pca9542", .data = &chips[pca_9542] },
>  	{ .compatible = "nxp,pca9543", .data = &chips[pca_9543] },
Andi Shyti Aug. 20, 2023, 11:23 a.m. UTC | #3 
Hi Patrick,

> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 0ccee2ae5720..968111442625 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -4,6 +4,7 @@
>   *
>   * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
>   * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
> + * Copyright (c) 2022 9elements GmbH <patrick.rudolph@9elements.com>

2023?

One question, why are you adding yourself in the copyright?

>   * This module supports the PCA954x and PCA984x series of I2C multiplexer/switch
>   * chips made by NXP Semiconductors.
> @@ -11,6 +12,12 @@
>   *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547,
>   *	 PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849.
>   *
> + * It's also compatible to Maxims MAX735x I2C switch chips, which are controlled
> + * as the NXP PCA9548 and the MAX736x chips that act like the PCA9544.
> + *
> + * This includes the:
> + *	 MAX7356, MAX7357, MAX7358, MAX7367, MAX7368 and MAX7369
> + *
>   * These chips are all controlled via the I2C bus itself, and all have a
>   * single 8-bit register. The upstream "parent" bus fans out to two,
>   * four, or eight downstream busses or channels; which of these
> @@ -51,6 +58,12 @@
>  #define PCA954X_IRQ_OFFSET 4
>  
>  enum pca_type {
> +	max_7356,
> +	max_7357,
> +	max_7358,
> +	max_7367,
> +	max_7368,
> +	max_7369,
>  	pca_9540,
>  	pca_9542,
>  	pca_9543,
> @@ -90,8 +103,45 @@ struct pca954x {
>  	raw_spinlock_t lock;
>  };
>  
> -/* Provide specs for the PCA954x types we know about */
> +/* Provide specs for the MAX735x, PCA954x and PCA984x types we know about */
>  static const struct chip_desc chips[] = {
> +	[max_7356] = {
> +		.nchans = 8,
> +		.muxtype = pca954x_isswi,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +	},
> +	[max_7357] = {
> +		.nchans = 8,
> +		.muxtype = pca954x_isswi,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +		/* No interrupt controller support.
> +		   The interrupt provides information about stuck channels. */

I'm sorry, Peter already commented on this, can you please fix
the commenting?

		/*
		 * No interrupt controller support.
		 * The interrupt provides information
		 * about stuck channels.
		 */

Or

		/*
		 * No interrupt controller support. The interrupt
		 * provides information * about stuck channels.
		 */

to save one line. (I prefer the latter, your choice)

Rest looks good.

Andi

> +	},
> +	[max_7358] = {
> +		.nchans = 8,
> +		.muxtype = pca954x_isswi,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +		/* No interrupt controller support.
> +		   The interrupt provides information about stuck channels. */
> +	},
> +	[max_7367] = {
> +		.nchans = 4,
> +		.muxtype = pca954x_isswi,
> +		.has_irq = 1,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +	},
> +	[max_7368] = {
> +		.nchans = 4,
> +		.muxtype = pca954x_isswi,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +	},
> +	[max_7369] = {
> +		.nchans = 4,
> +		.enable = 0x4,
> +		.muxtype = pca954x_ismux,
> +		.has_irq = 1,
> +		.id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> +	},
>  	[pca_9540] = {
>  		.nchans = 2,
>  		.enable = 0x4,
> @@ -177,6 +227,12 @@ static const struct chip_desc chips[] = {
>  };
>  
>  static const struct i2c_device_id pca954x_id[] = {
> +	{ "max7356", max_7356 },
> +	{ "max7357", max_7357 },
> +	{ "max7358", max_7358 },
> +	{ "max7367", max_7367 },
> +	{ "max7368", max_7368 },
> +	{ "max7369", max_7369 },
>  	{ "pca9540", pca_9540 },
>  	{ "pca9542", pca_9542 },
>  	{ "pca9543", pca_9543 },
> @@ -194,6 +250,12 @@ static const struct i2c_device_id pca954x_id[] = {
>  MODULE_DEVICE_TABLE(i2c, pca954x_id);
>  
>  static const struct of_device_id pca954x_of_match[] = {
> +	{ .compatible = "maxim,max7356", .data = &chips[max_7356] },
> +	{ .compatible = "maxim,max7357", .data = &chips[max_7357] },
> +	{ .compatible = "maxim,max7358", .data = &chips[max_7358] },
> +	{ .compatible = "maxim,max7367", .data = &chips[max_7367] },
> +	{ .compatible = "maxim,max7368", .data = &chips[max_7368] },
> +	{ .compatible = "maxim,max7369", .data = &chips[max_7369] },
>  	{ .compatible = "nxp,pca9540", .data = &chips[pca_9540] },
>  	{ .compatible = "nxp,pca9542", .data = &chips[pca_9542] },
>  	{ .compatible = "nxp,pca9543", .data = &chips[pca_9543] },
> -- 
> 2.41.0
>
Patrick Rudolph Aug. 21, 2023, 6:20 a.m. UTC | #4 
On Sun, Aug 20, 2023 at 1:23 PM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Patrick,
>
> > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> > index 0ccee2ae5720..968111442625 100644
> > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> > @@ -4,6 +4,7 @@
> >   *
> >   * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
> >   * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
> > + * Copyright (c) 2022 9elements GmbH <patrick.rudolph@9elements.com>
>
> 2023?
>
> One question, why are you adding yourself in the copyright?

Can't remember the reason. It's been a while since I created the first version.
Will drop the line.

>
> >   * This module supports the PCA954x and PCA984x series of I2C multiplexer/switch
> >   * chips made by NXP Semiconductors.
> > @@ -11,6 +12,12 @@
> >   *    PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547,
> >   *    PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849.
> >   *
> > + * It's also compatible to Maxims MAX735x I2C switch chips, which are controlled
> > + * as the NXP PCA9548 and the MAX736x chips that act like the PCA9544.
> > + *
> > + * This includes the:
> > + *    MAX7356, MAX7357, MAX7358, MAX7367, MAX7368 and MAX7369
> > + *
> >   * These chips are all controlled via the I2C bus itself, and all have a
> >   * single 8-bit register. The upstream "parent" bus fans out to two,
> >   * four, or eight downstream busses or channels; which of these
> > @@ -51,6 +58,12 @@
> >  #define PCA954X_IRQ_OFFSET 4
> >
> >  enum pca_type {
> > +     max_7356,
> > +     max_7357,
> > +     max_7358,
> > +     max_7367,
> > +     max_7368,
> > +     max_7369,
> >       pca_9540,
> >       pca_9542,
> >       pca_9543,
> > @@ -90,8 +103,45 @@ struct pca954x {
> >       raw_spinlock_t lock;
> >  };
> >
> > -/* Provide specs for the PCA954x types we know about */
> > +/* Provide specs for the MAX735x, PCA954x and PCA984x types we know about */
> >  static const struct chip_desc chips[] = {
> > +     [max_7356] = {
> > +             .nchans = 8,
> > +             .muxtype = pca954x_isswi,
> > +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> > +     },
> > +     [max_7357] = {
> > +             .nchans = 8,
> > +             .muxtype = pca954x_isswi,
> > +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> > +             /* No interrupt controller support.
> > +                The interrupt provides information about stuck channels. */
>
> I'm sorry, Peter already commented on this, can you please fix
> the commenting?
>
>                 /*
>                  * No interrupt controller support.
>                  * The interrupt provides information
>                  * about stuck channels.
>                  */
>
> Or
>
>                 /*
>                  * No interrupt controller support. The interrupt
>                  * provides information * about stuck channels.
>                  */
>
> to save one line. (I prefer the latter, your choice)
>

I'm sorry, I overlooked this one. Will send a new version.

> Rest looks good.
>
> Andi
>
> > +     },
> > +     [max_7358] = {
> > +             .nchans = 8,
> > +             .muxtype = pca954x_isswi,
> > +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> > +             /* No interrupt controller support.
> > +                The interrupt provides information about stuck channels. */
> > +     },
> > +     [max_7367] = {
> > +             .nchans = 4,
> > +             .muxtype = pca954x_isswi,
> > +             .has_irq = 1,
> > +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> > +     },
> > +     [max_7368] = {
> > +             .nchans = 4,
> > +             .muxtype = pca954x_isswi,
> > +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> > +     },
> > +     [max_7369] = {
> > +             .nchans = 4,
> > +             .enable = 0x4,
> > +             .muxtype = pca954x_ismux,
> > +             .has_irq = 1,
> > +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> > +     },
> >       [pca_9540] = {
> >               .nchans = 2,
> >               .enable = 0x4,
> > @@ -177,6 +227,12 @@ static const struct chip_desc chips[] = {
> >  };
> >
> >  static const struct i2c_device_id pca954x_id[] = {
> > +     { "max7356", max_7356 },
> > +     { "max7357", max_7357 },
> > +     { "max7358", max_7358 },
> > +     { "max7367", max_7367 },
> > +     { "max7368", max_7368 },
> > +     { "max7369", max_7369 },
> >       { "pca9540", pca_9540 },
> >       { "pca9542", pca_9542 },
> >       { "pca9543", pca_9543 },
> > @@ -194,6 +250,12 @@ static const struct i2c_device_id pca954x_id[] = {
> >  MODULE_DEVICE_TABLE(i2c, pca954x_id);
> >
> >  static const struct of_device_id pca954x_of_match[] = {
> > +     { .compatible = "maxim,max7356", .data = &chips[max_7356] },
> > +     { .compatible = "maxim,max7357", .data = &chips[max_7357] },
> > +     { .compatible = "maxim,max7358", .data = &chips[max_7358] },
> > +     { .compatible = "maxim,max7367", .data = &chips[max_7367] },
> > +     { .compatible = "maxim,max7368", .data = &chips[max_7368] },
> > +     { .compatible = "maxim,max7369", .data = &chips[max_7369] },
> >       { .compatible = "nxp,pca9540", .data = &chips[pca_9540] },
> >       { .compatible = "nxp,pca9542", .data = &chips[pca_9542] },
> >       { .compatible = "nxp,pca9543", .data = &chips[pca_9543] },
> > --
> > 2.41.0
> >