Re: [PATCH net-next v3 6/8] net: phy: mscc: timestamping and PHC support

[Antoine Tenart]

Hi Quentin,

Quoting Quentin Schulz (2020-06-21 19:35:20)
> On 2020-06-19 14:22, Antoine Tenart wrote:
> [...]
> > @@ -999,9 +1553,35 @@ int vsc8584_ptp_probe(struct phy_device *phydev)
> >       if (!vsc8531->ptp)
> >               return -ENOMEM;
> > 
> > +     mutex_init(&vsc8531->phc_lock);
> >       mutex_init(&vsc8531->ts_lock);
> > 
> > +     /* Retrieve the shared load/save GPIO. Request it as non exclusive as
> > +      * the same GPIO can be requested by all the PHYs of the same 
> > package.
> > +      * Ths GPIO must be used with the phc_lock taken (the lock is shared
> 
> Typo + wrong lock named in the comment, instead:
> 
>          * This GPIO must be used with the gpio_lock taken (the lock is shared
> 
> Though technically both are taken when access to the GPIO is requested 
> AFAICT.

That's right, thanks for pointing this out! I'll fix it for v4.

> Also on another note, maybe we could actually make vsc8531->base_addr
> be a part of vsc85xx_shared_private structure.
> 
> We would still need to compute it to pass it to devm_phy_package_join
> but it can easily be returned by vsc8584_get_base_addr instead of the
> current void and it'd put all the things used for all PHYs in the
> package at the same place.

We actually do not use directly the base_addr anymore from within the
driver, thanks to the shared package conversion. We're now using
__phy_package_write/__phy_package_read which are using the base address.

So the move could be to remove it from the vsc8531_private. If we were
to do it, we would need to find a clean solution: it's still part of a
structure as multiple values are computed in vsc8584_get_base_addr, and
it's a lot easier and cleaner to have them all in the same struct. Also,
that have nothing to do with the current series :)

Thanks,
Antoine

-- 
Antoine TÃnart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com