Re: [PATCH] i2c: Add support for NXP PCA984x family.

[Rodolfo Giometti <giometti@xxxxxxxxxxxx>]

On 11/12/17 14:15, Adrian Fiergolski wrote:
> On 11.12.2017 at 13:51, Peter Rosin wrote:
> > On 2017-12-11 12:25, Rodolfo Giometti wrote:
> > > On 11/12/17 12:10, Adrian Fiergolski wrote:
> > > > This patch exetends the current i2c-mux-pca954x driver and adds suuport for
> > > > a newer PCA984x family of the I2C switches and multiplexers from NXP.
> > > > In probe function, the patch supports device ID function, introduced in the
> > > > new family, and checks it against configuration provided by the
> > > > devicetree. Moreover, it also performs software reset which is now
> > > > available for the new devices.
> > > > The reset of the code remains common with the legacy PCA954x devices.
> > > >
> > > > Signed-off-by: Adrian Fiergolski <adrian.fiergolski@xxxxxxx>
> > > > ---
> > > >    .../{i2c-mux-pca954x.txt => i2c-mux-pca9x4x.txt}   |   5 +-
> > > >    arch/arm/configs/aspeed_g4_defconfig               |   2 +-
> > > >    arch/arm/configs/aspeed_g5_defconfig               |   2 +-
> > > >    arch/arm/configs/multi_v7_defconfig                |   2 +-
> > > >    arch/arm/configs/pxa_defconfig                     |   2 +-
> > > >    arch/arm/configs/tegra_defconfig                   |   2 +-
> > > >    arch/arm64/configs/defconfig                       |   2 +-
> > > >    arch/powerpc/configs/85xx-hw.config                |   2 +-
> > > >    drivers/i2c/muxes/Kconfig                          |   6 +-
> > > >    drivers/i2c/muxes/Makefile                         |   2 +-
> > > >    drivers/i2c/muxes/i2c-mux-pca9541.c                |   4 +-
> > > >    .../muxes/{i2c-mux-pca954x.c => i2c-mux-pca9x4x.c} | 295 ++++++++++++++++-----
> > > >    .../linux/platform_data/{pca954x.h => pca9x4x.h}   |  15 +-
> > > >    13 files changed, 246 insertions(+), 95 deletions(-)
> > > >    rename Documentation/devicetree/bindings/i2c/{i2c-mux-pca954x.txt => i2c-mux-pca9x4x.txt} (91%)
> > > >    rename drivers/i2c/muxes/{i2c-mux-pca954x.c => i2c-mux-pca9x4x.c} (56%)
> > > >    rename include/linux/platform_data/{pca954x.h => pca9x4x.h} (80%)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
> > > > similarity index 91%
> > > > rename from Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> > > > rename to Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
> > > > index aa097045a10e..cf9a075ca1dd 100644
> > > > --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> > > > +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca9x4x.txt
> > > > @@ -1,10 +1,11 @@
> > > > -* NXP PCA954x I2C bus switch
> > > > +* NXP PCA9x4x I2C bus switch
> > > >    
> > > >    Required Properties:
> > > >    
> > > >      - compatible: Must contain one of the following.
> > > >        "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544",
> > > > -    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548"
> > > > +    "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548",
> > > > +    "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849"
> > > >    
> > > >      - reg: The I2C address of the device.
> > > >    
> > > > diff --git a/arch/arm/configs/aspeed_g4_defconfig b/arch/arm/configs/aspeed_g4_defconfig
> > > > index d23b9d56a88b..a461ad3cf63d 100644
> > > > --- a/arch/arm/configs/aspeed_g4_defconfig
> > > > +++ b/arch/arm/configs/aspeed_g4_defconfig
> > > > @@ -116,7 +116,7 @@ CONFIG_I2C=y
> > > >    CONFIG_I2C_CHARDEV=y
> > > >    CONFIG_I2C_MUX=y
> > > >    CONFIG_I2C_MUX_PCA9541=y
> > > > -CONFIG_I2C_MUX_PCA954x=y
> > > > +CONFIG_I2C_MUX_PCA9x4x=y
> > > Nak.
> > >
> > > I'm not sure you should break backward compatibility. You should keep the
> > > CONFIG_I2C_MUX_PCA954x configuration setting and the current name convention.
> > Right, definitely avoid the mass rename. In addition to the pointless
> > churn, pca9x4x is just plain wrong since it matches e.g. pca9745 which
> > has nothing whatsoever to do with this driver.
> >
> > I'll add more comments once the rename noise is gone.
> >
> > Cheers,
> > Peter
> Thanks for comments.
> What do you suggest then ? I could keep CONFIG_I2C_MUX_PCA954x, but
> then, since there is a common source, if enabled, the PCA985x family
> support would be provided as well anyway. I could try to move PCA985x
> features to a separate file which would be included by i2c-mux-pca954x.c
> if a dedicated CONFIG_I2C_MUX_PCA984x is enabled, but it doesn't seem to
> be an elegant solution to me.
>
> I understand, that your main concern regards CONFIG_I2C_MUX_PCA954x
> only. The fact that internal data types and files have been renamed to a
> more generic and actual pca9x5x style is fine, right ?

None. Just drop the mass rename and add support for the new family. Also don't 
forget to mention the new support into documentation/configuration files.

Ciao,

Rodolfo

--

HCE Engineering                      e-mail: giometti@xxxxxxxxxxxxxxxxxx
GNU/Linux Solutions                          giometti@xxxxxxxxxxxx
Linux Device Driver                          giometti@xxxxxxxx
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Cosino Project - the quick prototyping embedded system - www.cosino.it
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it