Re: [PATCH net-next v3 12/14] onsemi: s2500: Add driver support for TS2500 MAC-PHY

[Andrew Lunn <andrew@lunn.ch>]

On Tue, Jun 02, 2026 at 02:30:02AM +0000, Selvamani Rajagopal wrote:
> > 
> > > +#define S2500_MMS_MII (S2500_OA_TC6_MACPHY_MMS0 << 16)
> > > +#define S2500_MMS_MAC (S2500_OA_TC6_MAC_MMS1 << 16)
> > > +#define S2500_MMS_PCS (OA_TC6_PHY_C45_PCS_MMS2 << 16)
> > > +#define S2500_MMS_PMDPMA (OA_TC6_PHY_C45_PMA_PMD_MMS3 << 16)
> > > +#define S2500_MMS_VS1 (S2500_OA_TC6_VS1_MMS12 << 16)
> > > +#define S2500_MMS_VS2 (OA_TC6_PHY_C45_VS_PLCA_MMS4 << 16)
> > 
> > This kind of suggests we should rethink the API for reading/writing
> > registers. It should be possible to pass MMS and register as seperate
> > values, and let the core combine them.
> 
> 
> I agree. In fact, onsemi's original implementation had MMS and address as separate parameter. 
> It is going to touch all the files. Let me know whether you want to do, If so, when.

I already suggested adding _mms() versions for register read/write. We
can leave any using MMS 0 alone, and only use these wrappers for !
0. That should make the patch smaller.

> > > + /* Convert the OID in host byte order */
> > > + for (i = 2; i >= 0; --i) {
> > > + addr[i] = 0;
> > > + for (j = 0; j < 8; ++j) {
> > > + addr[i] |= (val & 1) << (7 - j);
> > > + val >>= 1;
> > > + }
> > > + }
> > 
> > That seems pretty complex. What exactly is it doing?
> 
> Could have been well commented. According to datesheet, phy-id-reg[16:31] mapped to OUI[2:17] and 
> phy-id-reg[8:15] mapped to OUI[18:23]. I will add this to the comment.

What you might want to use FIELD_PREP, etc. See if that makes the code
more readable.

	Andrew