Re: [PATCH net] net: dsa: microchip: fix probe of I2C-connected KSZ8563

[Arun.Ramadoss]

Hi Ahmad,
On Fri, 2023-01-20 at 08:57 +0100, Ahmad Fatoum wrote:
> [You don't often get email from a.fatoum@xxxxxxxxxxxxxx. Learn why
> this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Hello Arun,
> 
> On 20.01.23 08:01, Arun.Ramadoss@xxxxxxxxxxxxx wrote:
> > Hi Ahmad,
> > On Thu, 2023-01-19 at 14:10 +0100, Ahmad Fatoum wrote:
> > > [You don't often get email from a.fatoum@xxxxxxxxxxxxxx. Learn
> > > why
> > > this is important at 
> > > https://aka.ms/LearnAboutSenderIdentification ]
> > > 
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > > 
> > > Starting with commit eee16b147121 ("net: dsa: microchip: perform
> > > the
> > > compatibility check for dev probed"), the KSZ switch driver now
> > > bails
> > > out if it thinks the DT compatible doesn't match the actual chip:
> > > 
> > >   ksz9477-switch 1-005f: Device tree specifies chip KSZ9893 but
> > > found
> > >   KSZ8563, please fix it!
> > > 
> > > Problem is that the "microchip,ksz8563" compatible is associated
> > > with ksz_switch_chips[KSZ9893]. Same issue also affected the SPI
> > > driver
> > > for the same switch chip and was fixed in commit b44908095612
> > > ("net: dsa: microchip: add separate struct ksz_chip_data for
> > > KSZ8563
> > > chip").
> > > 
> > > Reuse ksz_switch_chips[KSZ8563] introduced in aforementioned
> > > commit
> > > to get I2C-connected KSZ8563 probing again.
> > > 
> > > Fixes: eee16b147121 ("net: dsa: microchip: perform the
> > > compatibility
> > > check for dev probed")
> > 
> > In this commit, there is no KSZ8563 member in struct
> > ksz_switch_chips.
> > Whether the fixes should be to this commit "net: dsa: microchip:
> > add
> > separate struct ksz_chip_data for KSZ8563" where the member is
> > introduced.
> 
> I disagree. eee16b147121 introduced the check that made my device
> not probe anymore, so that's what's referenced in Fixes:. Commit
> b44908095612 should have had a Fixes: pointing at eee16b147121
> as well, so users don't miss it. But if they miss it, they
> will notice this at build-time anyway.

The KSZ9893, KSZ9563 and KSZ8563 all has the same chip id 0x00989300.
They belong to 3 port switch family. Differentiation is done based on
0x1F register. In the commit eee16b147121, there is no differentiation
based on 0x1F, device is selected based on chip id, all the three chips
will be identified as ksz9893 only. After the commit b44908095612,
KSZ8563 chips is identified based on 0x1F register. 

> 
> Cheers,
> Ahmad
> 
> > 
> > > chip
> > > Signed-off-by: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>
> > > ---
> > >  drivers/net/dsa/microchip/ksz9477_i2c.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c
> > > b/drivers/net/dsa/microchip/ksz9477_i2c.c
> > > index c1a633ca1e6d..e315f669ec06 100644
> > > --- a/drivers/net/dsa/microchip/ksz9477_i2c.c
> > > +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
> > > @@ -104,7 +104,7 @@ static const struct of_device_id
> > > ksz9477_dt_ids[]
> > > = {
> > >         },
> > >         {
> > >                 .compatible = "microchip,ksz8563",
> > > -               .data = &ksz_switch_chips[KSZ9893]
> > > +               .data = &ksz_switch_chips[KSZ8563]
> > >         },
> > >         {
> > >                 .compatible = "microchip,ksz9567",
> > > --
> > > 2.30.2
> > > 
> 
> --
> Pengutronix
> e.K.                           |                             |
> Steuerwalder Str. 21                       | 
> http://www.pengutronix.de/e/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-
> 0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-
> 5555 |
>