From mboxrd@z Thu Jan  1 00:00:00 1970
From: Joe Hershberger <joe.hershberger@gmail.com>
Date: Wed, 27 Jan 2016 14:11:09 -0600
Subject: [U-Boot] [PATCH v2 2/2] net: phy: Add PHY driver for mv88e61xx
	switches
In-Reply-To: <20160127182843.7f7c13a2@lilith>
References: <1450734319-9515-1-git-send-email-kevin.smith@elecsyscorp.com>
 <1450734319-9515-4-git-send-email-kevin.smith@elecsyscorp.com>
 <CANr=Z=afa_UXY9rF4aVwO+nKt7b4kY-mXXwT-KeZatAHirqkRA@mail.gmail.com>
 <56A8F077.60308@elecsyscorp.com> <20160127182843.7f7c13a2@lilith>
Message-ID: <CANr=Z=btWB-B1r8Gxex3RsA5aqUFN8mJ5Nk4SMB4srUeffRtcA@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Wed, Jan 27, 2016 at 11:28 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Hello Kevin,
>
> On Wed, 27 Jan 2016 16:29:42 +0000, Kevin Smith
> <kevin.smith@elecsyscorp.com> wrote:
>> Hi Joe,
>
>> On 01/26/2016 06:11 PM, Joe Hershberger wrote:
>
>> >> +       /* Replace the bus with the fake device */
>> > Fake how? This is a confusing comment to me as written.
>> The genphy functions assume that they can write to the PHY directly
>> using the MII bus, and the address it uses is the address of a
>> register.  This is not the case for this chip with multiple PHY
>> interfaces, which have to be accessed indirectly.  To handle this, I
>> have created a "fake" mii_dev whose read/write functions are the
>> indirection functions, and stored the actual mdio_bus in the private
>> data for the "fake" device, which is then used by the indirect
>> functions.  This allows this driver to make use of common genphy stuff
>> where appropriate.  Maybe "wrapper" or "indirect bus" is a better name
>> for it.  Let me know if you have a preference or better idea.
>
> "Indirect bus" is better IMO, as it gives a clue about what actually
> happens.

Sounds good to me. May as well use the same nomenclature for the read
and write functions.

>> >> +
>> >> +       mac_addr = phydev->addr;
>> >> +
>> >> +       for (i = 0; i < PORT_COUNT; i++) {
>> >> +               if ((1 << i) & CONFIG_MV88E61XX_PHY_PORTS) {
>> >> +                       phydev->addr = i;
>> >> +                       mv88e61xx_phy_enable(phydev, i);
>> >> +                       mv88e61xx_phy_setup(phydev, i);
>> >> +                       mv88e61xx_phy_config_port(phydev, i);
>> > These all return status, but are ignored.
>> Even if one fails, it seems appropriate to me to continue initializing
>> the others and not bail completely.  Should I catch the error, print a
>> warning and "continue" in the loop?  Or is completely bailing the right
>> thing to do?  If there are some errors, but other successes, what should
>> the function return?
>
> Warn for each port switch initialization failure, bail out if no port
> could be initialized?

I like it.

Cheers,
-Joe