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

[Adrian Fiergolski <Adrian.Fiergolski@cern.ch>]

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@cern.ch>
>>> ---
>>>   .../{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 ?

Cheers,
Adrian