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@xxxxxxxxxxxxxxx
> > > > > Signed-off-by: Johan Hovold <johan@xxxxxxxxxx>
> > > > > ---
> > > > > 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".
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
