Re: [PATCH] ARM: dts: NSP: Use hardware I2C for BCM958625HR

[Florian Fainelli]

On 12/13/2019 2:11 PM, Ray Jui wrote:
> 
> 
> On 2019-12-13 11:51 a.m., Florian Fainelli wrote:
>> Now that the i2c-bcm-iproc driver has been fixed to permit reading more
>> than 63 bytes in a single transaction with commit fd01eecdf959 ("i2c:
>> iproc: Fix i2c master read more than 63 bytes") we no longer need to
>> bitbang i2c over GPIOs which was necessary before to allow the
>> PHYLINK/SFP subsystems to read SFP modules.
>>
> 
> This is good to hear!
> 
>> Signed-off-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
>> ---
>> Â arch/arm/boot/dts/bcm958625hr.dts | 15 +++++----------
>> Â 1 file changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/bcm958625hr.dts
>> b/arch/arm/boot/dts/bcm958625hr.dts
>> index a2c9de35ddfb..536fb24f38bb 100644
>> --- a/arch/arm/boot/dts/bcm958625hr.dts
>> +++ b/arch/arm/boot/dts/bcm958625hr.dts
>> @@ -55,18 +55,9 @@
>> ÂÂÂÂÂÂÂÂÂ priority = <200>;
>> ÂÂÂÂÂ };
>> Â -ÂÂÂ /* Hardware I2C block cannot do more than 63 bytes per transfer,
>> -ÂÂÂÂ * which would prevent reading from a SFP's EEPROM (256 byte).
>> -ÂÂÂÂ */
>> -ÂÂÂ i2c1: i2c {
>> -ÂÂÂÂÂÂÂ compatible = "i2c-gpio";
>> -ÂÂÂÂÂÂÂ sda-gpios = <&gpioa 5 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>> -ÂÂÂÂÂÂÂ scl-gpios = <&gpioa 4 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>> -ÂÂÂ };
>> -
> 
> So I suppose GPIO 4 and 5 from the 'gpioa' controller are tied to the
> same SCL/SDA pins from i2c0 and they are internally muxed, right?

Correct.

> 
> Is the mux to GPIO done automatically when pins are configured as GPIO,
> and therefore you don't require any additional changes to pinmux to make
> this work, after changing it back to use real I2C0 block below?

Yes indeed.

> 
>> ÂÂÂÂÂ sfp: sfp {
>> ÂÂÂÂÂÂÂÂÂ compatible = "sff,sfp";
>> -ÂÂÂÂÂÂÂ i2c-bus = <&i2c1>;
>> +ÂÂÂÂÂÂÂ i2c-bus = <&i2c0>;
>> ÂÂÂÂÂÂÂÂÂ mod-def0-gpios = <&gpioa 28 GPIO_ACTIVE_LOW>;
>> ÂÂÂÂÂÂÂÂÂ los-gpios = <&gpioa 24 GPIO_ACTIVE_HIGH>;
>> ÂÂÂÂÂÂÂÂÂ tx-fault-gpios = <&gpioa 30 GPIO_ACTIVE_HIGH>;
>> @@ -74,6 +65,10 @@
>> ÂÂÂÂÂ };
>> Â };
>> Â +&i2c0 {
>> +ÂÂÂ status = "okay";
>> +};
>> +
>> Â &amac0 {
>> ÂÂÂÂÂ status = "okay";
>> Â };
>>
> 
> Change looks good to me.
> 
> Reviewed-by: Ray Jui <ray.jui@xxxxxxxxxxxx>

Thanks!
-- 
Florian