Date:   Thu, 5 May 2022 18:15:07 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Arun.Ramadoss@...rochip.com
Cc:     songliubraving@...com, linux-kernel@...r.kernel.org,
        bpf@...r.kernel.org, daniel@...earbox.net, ast@...nel.org,
        robh+dt@...nel.org, andrew@...n.ch, devicetree@...r.kernel.org,
        linux@...linux.org.uk, andrii@...nel.org,
        UNGLinuxDriver@...rochip.com, john.fastabend@...il.com,
        vivien.didelot@...il.com, f.fainelli@...il.com, kuba@...nel.org,
        kpsingh@...nel.org, edumazet@...gle.com, pabeni@...hat.com,
        netdev@...r.kernel.org, kafai@...com, yhs@...com,
        krzysztof.kozlowski+dt@...aro.org, davem@...emloft.net,
        Woojung.Huh@...rochip.com
Subject: Re: [Patch net-next v13 07/13] net: dsa: microchip: add LAN937x SPI
 driver

On Thu, May 05, 2022 at 10:32:17AM +0000, Arun.Ramadoss@...rochip.com wrote:
> > >  static int lan937x_switch_init(struct ksz_device *dev)
> > >  {
> > > +     int ret;
> > > +
> > >       dev->ds->ops = &lan937x_switch_ops;
> > > 
> > > +     /* Check device tree */
> > > +     ret = lan937x_check_device_id(dev);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > 
> > Can't this be called from lan937x_spi_probe() directly, why do you
> > need
> > to go through lan937x_switch_register() first?
> 
> lan937x_check_device_id function compares the dev->chip_id with the
> lan937x_switch_chip array and populate the some of the parameters of
> struct ksz_dev. The dev->chip_id is populated using the dev->dev_ops-
> >detect in the ksz_switch_register function. If lan937x_check_device_id
> needs to be called in spi_probe, then chip_id has to be identified as
> part of spi_probe function. Since ksz_switch_register handles the
> identifying the chip_id, checking the device_id is part of switch_init.
>  
>  if (dev->dev_ops->detect(dev))
>              return -EINVAL;
>  
>  ret = dev->dev_ops->init(dev);
>  if (ret)
>             return ret;

Whatever you do, please use a common pattern for all of ksz9477, ksz8,
and your lan937x. This includes validation of chip id, placement of the
chip_data and dev_ops structures, and reuse as much logic as possible.
The key is to limit the chip-specific information to structured data
(tables) wherever possible and let common code deal with them.

For example there is no reason why struct ksz_chip_data is redefined for
every switch, why copying from "chip" to "dev" is duplicated for every
switch, and yet, why every other switch copies from "chip" to "dev" in
the "switch_init" function yet lan937x does it from "check_device_id".
So much boilerplate, yet different in subtle ways, makes the code very
unpleasant to review.

I'm sure you'll find a straightforward way to code up a probing function.

> As per the comment, enable_spi_indirect_access function called twice
> https://lore.kernel.org/netdev/20220408232557.b62l3lksotq5vuvm@skbuf/
> I have removed the enable_spi_indirect_access in the lan937x_setup
> function in v13 patch 6. But it actually failed our regression.
> The SPI indirect is required for accessing the Internal phy registers.
> We have enabled it in lan937x_init before registering the
> mdio_register. We need it for reading the phy id.
> And another place enabled in lan937x_setup after lan937x_switch_reset
> function. When I removed enabling in setup function, switch_reset
> disables the spi indirecting addressing. Because of that further phy
> register r/w fails. In Summary, we need to enable spi indirect access
> in both the places, one for mdio_register and another after
> switch_reset. 
> 
> Can I enable it both the places? Kindly suggest. 

So you call lan937x_reset_switch() as the first thing in ds->ops->setup(),
and this momentarily breaks the earlier MDIO bus setup done from probe
-> ksz_switch_register() -> dev_ops->init().

So why don't you move the lan937x_enable_spi_indirect_access() and
lan937x_mdio_register() calls to ds->ops->setup(), _after_ the switch
soft reset, then?

Powered by blists - more mailing lists