Re: [PATCH net-next 05/16] net: pcs: xpcs: Move native device ID macro to linux/pcs/pcs-xpcs.h

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Dec 05, 2023 at 02:14:34PM +0300, Serge Semin wrote:
> On Tue, Dec 05, 2023 at 10:45:34AM +0000, Russell King (Oracle) wrote:
> > On Tue, Dec 05, 2023 at 01:35:26PM +0300, Serge Semin wrote:
> > > Generic MDIO-device driver will support setting a custom device ID for the
> > > particular MDIO-device.
> > 
> > Why future tense? I don't see anything later in this patch set adding
> > this.
> 
> After the next patch is applied
> [PATCH net-next 10/16] net: pcs: xpcs: Add generic DW XPCS MDIO-device support
> the DW XPCS driver _will_ support setting custom IDs based on the
> platform data and the DT compatibles.

What is confusing is that the sentence makes it sound like it's some
generic driver that can be used for any PCS, whereas in reality it is
_this_ XPCS driver which is not generic.

"This driver will support setting a custom device ID in a future patch."
or explicitly state the summary line of the patch concerned so one can
refer to it. Future references are difficult to find whether they're in
email and especially once they're merged into git.

> It can be used for instance to
> fix the already available SJ1105 and SJ1110 MDIO bus implementations,
> so instead of substituting the XPCS IDs on the PHYSID CSR reads the
> driver could just pass the device ID and PMA ID via the device
> platform data.
> 
> If my patch log text looks unclear anyway, just say so. I'll change it
> accordingly. I guess it would be enough to say that moving is required
> just to collect all the IDs in a single place.

You need to adjust your attitude - I did exactly that. There was
something which I didn't understand, so I raised the issue. Sorry
for spotting a problem, but do you always get arsey when a reviewer
picks up on something wrong? If that's your attitude, then for this
entire series: NAK.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!