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

Peter Rosin
2017-12-11T13:26:11Z

On 2017-12-11 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@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 ?

Keep the name as PCA954x, just add support for PCA9846-9 without touching
either file names, config options or variables prefixes. The only rename
that is appropriate is to PCA9540, but since the x is already there, PCA954x
stays as it is.

The config option CONFIG_I2C_MUX_PCA954x will do nicely for PCA9846-9 too,
all that is needed is to explain what is going on in the help text, as
indicated by Rodolfo.

So, don't split the file and don't get distracted by the driver covering
more hardware than indicated by the driver names.

Cheers,
Peter