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
