Re: [PATCH 2/3] net: fec: implement dtb property phy-reset-durationy

[Stefan Christ <s.christ@phytec.de>]

Hi,

> > Yeah, that would be the obvious and plausible thing to do, but the driver in
> > the linux kernel does it this way and it's documented in the device tree
> > bindings.  So I'm not sure if we can change the default behavior here.
> 
> Yes, we can ;)
> 
> The rationale behind this code may be: The reset duration is
> unreasonably high (>1s), so that it to a sane default (1ms).
> 
> I suggest to drop the check completely. If someone ever starts barebox
> with an insanely high value (maybe he thought the value is in us) then
> he won't notice the mistake if barebox silently falls back to 1ms as
> duration. If instead barebox waits for multiple seconds then he will
> notice and it the device tree which is what we want.

Ok. So what about printing a warn message that case? Then everybody should
notice.

When I send a v2 patch, should the description in the device tree binding be
updated, too?

Mit freundlichen Grüßen / Kind regards,
	Stefan Christ

On Tue, Nov 17, 2015 at 10:32:13AM +0100, Sascha Hauer wrote:
> On Mon, Nov 16, 2015 at 11:17:05AM +0100, Stefan Christ wrote:
> > Hi,
> >   
> > > shouldn't this be msec = 1000.
> > > 
> > > Here you set it to 1ms if it used to be over 1s.
> > 
> > Yeah, that would be the obvious and plausible thing to do, but the driver in
> > the linux kernel does it this way and it's documented in the device tree
> > bindings.  So I'm not sure if we can change the default behavior here.
> 
> Yes, we can ;)
> 
> The rationale behind this code may be: The reset duration is
> unreasonably high (>1s), so that it to a sane default (1ms).
> 
> I suggest to drop the check completely. If someone ever starts barebox
> with an insanely high value (maybe he thought the value is in us) then
> he won't notice the mistake if barebox silently falls back to 1ms as
> duration. If instead barebox waits for multiple seconds then he will
> notice and it the device tree which is what we want.
> 
> Sascha
> 
> 
> > 
> > Mit freundlichen Grüßen / Kind regards,
> > 	Stefan Christ
> > 
> > On Mon, Nov 16, 2015 at 11:03:32AM +0100, Holger Schurig wrote:
> > > 2015-11-16 10:00 GMT+01:00 Stefan Christ <s.christ@phytec.de>:
> > > 
> > > > +               /* A sane reset duration should not be longer than 1s */
> > > > +               if (msec > 1000)
> > > > +                       msec = 1;
> > > >
> > > 
> > > Hi Stefan,
> > > 
> > > shouldn't this be msec = 1000.
> > > 
> > > Here you set it to 1ms if it used to be over 1s.
> > 
> > _______________________________________________
> > barebox mailing list
> > barebox@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/barebox
> 
> -- 
> 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 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox