Re: [PATCH 19/22] dt/bindings: add micrel,rmii_ref_clk_sel_25_mhz to eth-phy binding

[Johan Hovold <johan@kernel.org>]

On Thu, Nov 13, 2014 at 09:09:27AM +0100, Sascha Hauer wrote:
> On Wed, Nov 12, 2014 at 10:19:20AM +0100, Johan Hovold wrote:
> > On Wed, Nov 12, 2014 at 08:01:27AM +0100, Sascha Hauer wrote:
> > > On Tue, Nov 11, 2014 at 07:18:25PM +0100, Johan Hovold wrote:
> > > > On Tue, Nov 11, 2014 at 05:57:42PM +0000, Mark Rutland wrote:
> > > > > On Tue, Nov 11, 2014 at 05:37:37PM +0000, Johan Hovold wrote:
> > > > > > Add "micrel,rmii_ref_clk_sel_25_mhz" to Micrel ethernet PHY binding
> > > > > > documentation.
> > > > > > 
> > > > > > Cc: devicetree@vger.kernel.org
> > > > > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/net/micrel.txt | 5 +++++
> > > > > >  1 file changed, 5 insertions(+)
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt
> > > > > > index a1bab5eaae02..9b08dd6551dd 100644
> > > > > > --- a/Documentation/devicetree/bindings/net/micrel.txt
> > > > > > +++ b/Documentation/devicetree/bindings/net/micrel.txt
> > > > > > @@ -19,6 +19,11 @@ Optional properties:
> > > > > >  
> > > > > >                See the respective PHY datasheet for the mode values.
> > > > > >  
> > > > > > + - micrel,rmii_ref_clk_sel_25_mhz: rmii_ref_clk_sel bit selects 25 MHz mode
> > > > > > +
> > > > > > +		Whether 25 MHz (rather than 50 Mhz) clock mode is selected
> > > > > > +		when the rmii_ref_clk_sel bit is set.
> > > > > 
> > > > > s/_/-/ in property names please.
> > > > 
> > > > Ouch, copied from variable name, sorry.
> > > > 
> > > > > That said, I don't follow the meaning. Does this cause the kernel to do
> > > > > something different, or is is simply that a 25MHz ref clock is wired up?
> > > > 
> > > > Yes, the driver currently sets this configuration bit based on a common
> > > > clock binding.
> > > > 
> > > > However, it turns out the meaning of the bit is reversed on some PHY
> > > > variants. On most PHYs 50 MHz mode is selected by setting this bit,
> > > > whereas on the PHYs that need this new property, setting it selects 25
> > > > MHz mode instead.
> > > 
> > > Maybe rename the property to something like rmii-ref-clk-25mhz-active-high
> > > then? Also you should probably make it more explicit that this is a
> > > hardware property and not for adjusting the clock.
> > 
> > You're right, but how about calling it
> > 
> > 	micrel,rmii-reference-clock-select-inverted
> > 
> > Then no one will set it believing it will change the clock mode and
> > without reading the binding doc first.
> > 
> > The description could then read something like
> > 
> > 	micrel,rmii-reference-clock-select-inverted: RMII Reference
> > 		Clock Select bit is inverted
> > 
> > 	The RMII Reference Clock Select bit is inverted so that setting
> > 	it selects 25 MHz rather than 50 MHz clock mode. 
> > 
> > 	Note that this is only needed for PHY variants that has this bit
> > 	inverted and that a clock reference ("rmii-ref" below) is always
> > 	needed to select the actual mode.
> 
> "Inverted" only has a meaning when everybody agrees what's the normal
> case. Since that not the case I really prefer talking about
> "active-high" or "active-low".

Yes, but I'm reluctant to using "active-high" and "active-low" as this
is not a signal (or IO pin) we're dealing with.

It's a configuration bit, which (if set) selects 25 MHz mode. Hence I
still think something like 

	micrel,rmii_ref_clk_sel_25_mhz

or

	micrel,rmii_reference_clock_select_selects_25_mhz

is preferred. The binding documentation will still make it clear that a
clocks reference is also needed.

Johan