netdev - Re: [PATCH net-next 2/2] net: phy: microchip_t1s: add support for Microchip LAN865x Rev.B0 PHYs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d4649b51-b06e-42f3-88d3-e269c304d0ac@lunn.ch>
Date:   Wed, 26 Apr 2023 23:43:32 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Ramón Nordin Rodriguez 
        <ramon.nordin.rodriguez@...roamp.se>
Cc:     Parthiban Veerasooran <Parthiban.Veerasooran@...rochip.com>,
        netdev@...r.kernel.org, davem@...emloft.net,
        jan.huber@...rochip.com, thorsten.kummermehr@...rochip.com
Subject: Re: [PATCH net-next 2/2] net: phy: microchip_t1s: add support for
 Microchip LAN865x Rev.B0 PHYs

> > +/* indirect read pseudocode from AN1760
> > + * write_register(0x4, 0x00D8, addr)
> > + * write_register(0x4, 0x00DA, 0x2)
> > + * return (int8)(read_register(0x4, 0x00D9))
> > + */
> 
> I suggest this comment block is slightly changed to
> 
> /* Pulled from AN1760 describing 'indirect read'
>  *
>  * write_register(0x4, 0x00D8, addr)
>  * write_register(0x4, 0x00DA, 0x2)
>  * return (int8)(read_register(0x4, 0x00D9))
>  *
>  * 0x4 refers to memory map selector 4, which maps to MDIO_MMD_VEND2
>  */
> 
> > +static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr)
> > +{
> > +	int ret;
> > +
> > +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xD8, addr);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xDA, 0x0002);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0xD9);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return ret;
> > +}

It is unclear to me how 0x4 maps to MDIO_MMD_VEND2, which is 31.

Why not just describe it in terms of MMD read/write?

> The func itself might get a bit more readable by naming the magic regs
> and value, below is a suggestion.
> 
> static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr)
> {
> 	int ret;
> 	static const u16 fixup_w0_reg = 0x00D8;
> 	static const u16 fixup_r0_reg = 0x00D9;
> 	static const u16 fixup_w1_val = 0x0002;
> 	static const u16 fixup_w1_reg = 0x00DA;

#defines would be normal, not variables.

And i guess 0x0002 is actually BIT(1). And it probably means something
like START? 0xD8 is some sort of address register, so i would put ADDR
in the name. 0xD9 appears to be a control register, so CTRL. And 0xDA
is a data register? So these could be give more descriptive names,
just by my reverse engineering it. With the actual data sheet, i'm
expect somebody could do better.

> > +static int lan865x_revb0_config_init(struct phy_device *phydev)
> > +{
> > +	int addr;
> > +	int value;
> > +	int ret;
> > +
> > +	/* As per AN1760, the below configuration applies only to the LAN8650/1
> > +	 * hardware revision Rev.B0.
> > +	 */
> 
> I think this is implied by having it the device specific init func, you
> can probably drop this comment.

A reference to AN1760 somewhere in the driver would be good, to help
people understand why this magic is needed. Does AN1760 actually
explain the magic, or just say you need to do this to make it work, Trust Us(tm).

	    Andrew