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

Rodolfo Giometti
2017-12-11T13:36:19Z

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@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 ?

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@hce-engineering.it
GNU/Linux Solutions giometti@enneenne.com
Linux Device Driver giometti@linux.it
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