From: Ben Dooks <ben.dooks@codethink.co.uk>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
"linux-kernel@lists.codethink.co.uk"
<linux-kernel@lists.codethink.co.uk>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"f.fainelli@gmail.com" <f.fainelli@gmail.com>
Subject: Re: [PATCH] phy: micrel: add of configuration for LED mode
Date: Tue, 18 Mar 2014 17:21:04 +0100 [thread overview]
Message-ID: <53287270.1020908@codethink.co.uk> (raw)
In-Reply-To: <4129596.cD1CsNElkT@avalon>
On 18/03/14 17:11, Laurent Pinchart wrote:
> Hi Ben,
>
> On Tuesday 18 March 2014 16:56:06 Laurent Pinchart wrote:
>> On Thursday 27 February 2014 10:30:51 Ben Dooks wrote:
>>> On 27/02/14 09:15, Mark Rutland wrote:
>>>> On Wed, Feb 26, 2014 at 11:48:00AM +0000, Ben Dooks wrote:
>>>>> Add support for the led-mode property for the following PHYs
>>>>> which have a single LED mode configuration value.
>>>>>
>>>>> KSZ8001 and KSZ8041 which both use register 0x1e bits 15,14 and
>>>>> KSZ8021, KSZ8031 and KSZ8051 which use register 0x1f bits 5,4
>>>>> to control the LED configuration.
>>>>>
>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>> ---
>>>>>
>>>>> Documentation/devicetree/bindings/net/micrel.txt | 18 +++++++++
>>>>> drivers/net/phy/micrel.c | 49 ++++++++++++++--
>>>>> 2 files changed, 63 insertions(+), 4 deletions(-)
>>>>> create mode 100644 Documentation/devicetree/bindings/net/micrel.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/net/micrel.txt
>>>>> b/Documentation/devicetree/bindings/net/micrel.txt new file mode 100644
>>>>> index 0000000..98a3e61
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/net/micrel.txt
>>>>> @@ -0,0 +1,18 @@
>>>>> +Micrel PHY properties.
>>>>> +
>>>>> +These properties cover the base properties Micrel PHYs.
>>>>> +
>>>>> +Optional properties:
>>>>> +
>>>>> + - micrel,led-mode : LED mode value to set for PHYs with configurable
>>>>> LEDs.
>>>>> +
>>>>> + Configure the LED mode with single value. The list of
>>>>> PHYs and
>>>>> + the bits that are currently supported:
>>>>> +
>>>>> + KSZ8001: register 0x1e, bits 15..14
>>>>> + KSZ8041: register 0x1e, bits 15..14
>>>>> + KSZ8021: register 0x1f, bits 5..4
>>>>> + KSZ8031: register 0x1f, bits 5..4
>>>>> + KSZ8051: register 0x1f, bits 5..4
>>>>> +
>>>>> + See the respective PHY datasheet for the mode values.
>>>>
>>>> What do these mean, roughly,, and why can the kernel not decide how to
>>>> cnofigure these?
>>>
>>> Board specific, in the case of the Lager one of the LEDs is connected
>>> to the ethernet mac block to indicate link, however the default mode
>>> is not for just "Link" so we have to change it.
>>>
>>>> In general we prefer to not place raw register values in the DT, and I'd
>>>> like to know why we'd have to here.
>>>
>>> I could copy out stuff from the data-sheet, but I was trying to avoid a
>>> copy and paste job.
>>
>> I quite agree with Mark here, I would prefer not to list register values in
>> DT bindings. However, the hardware hardware diversity doesn't help us
>> abstracting LED modes.
>>
>> The following table summarizes LED usage options.
>>
>> Device Mode LED usage
>> ----------------------------------------------------------------------------
>> 8001 (LED[3:0]) 00 Collision / Full-Duplex / Speed / Link/Activity
>> 01 Activity / Full-Duplex/Collision / Speed / Link
>> 10 Activity / Full-Duplex / 100Mbps Link / 10Mbps Link
>> 11 Reserved
>> 80[23]1 (LED[0]) 00 Link/Activity
>> 01 Link
>> 10 Reserved
>> 11 Reserved
>> 80[45]1 (LED[1:0]) 00 Speed / Link/Activity
>> 01 Activity / Link
>> 10 Reserved
>> 11 Reserved
>>
>> While LED mode could be described by LED0 mode using "link-activity" or
>> "link" strings for the 80[2345]1 chips, the 8001 makes that slightly more
>> complex and shows that future chips might not conform to any scheme we come
>> up with now.
>>
>> I thus have no strong preference for a string-based mode description over
>> using an integer. However, if we keep the integer value, I wouldn't use the
>> register value directly, but would instead use the mode field value as an
>> integer in the 0-3 range. This would remove knowledge of the PHY control
>> register layout from the DT node content, and bring more consistency to the
>> values.
>
> And I realize that this is what you've done already in the implementation :-/
> I'll send a patch to clarify the DT bindings documentation if you don't mind,
> after hearing Mark's opinion on the issue.
Do we need strings for what is basically a single-setup post reset
which it makes no sense for the user to ever update?
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
next prev parent reply other threads:[~2014-03-18 16:21 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-02-26 11:48 [PATCH] phy: micrel: add of configuration for LED mode Ben Dooks
2014-02-26 22:00 ` David Miller
2014-02-26 22:20 ` Florian Fainelli
2014-02-27 10:22 ` Ben Dooks
2014-02-27 9:15 ` Mark Rutland
2014-02-27 10:30 ` Ben Dooks
[not found] ` <530F13DB.90009-4yDnlxn2s6sWdaTGBSpHTA@public.gmane.org>
2014-03-18 15:56 ` Laurent Pinchart
2014-03-18 16:11 ` Laurent Pinchart
2014-03-18 16:21 ` Ben Dooks [this message]
[not found] ` <53287270.1020908-4yDnlxn2s6sWdaTGBSpHTA@public.gmane.org>
2014-03-18 16:31 ` Laurent Pinchart
2014-03-18 20:39 ` Sergei Shtylyov
2014-03-18 23:15 ` Sergei Shtylyov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53287270.1020908@codethink.co.uk \
--to=ben.dooks@codethink.co.uk \
--cc=devicetree@vger.kernel.org \
--cc=f.fainelli@gmail.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-kernel@lists.codethink.co.uk \
--cc=mark.rutland@arm.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).