devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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

  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).