On Tue, Dec 06, 2016 at 10:45:55AM -0600, Grygorii Strashko wrote:
> On 12/06/2016 07:40 AM, Richard Cochran wrote:
> > [ BTW, resetting the timecounter here makes no sense either. Why
> > reset the clock just because the interface goes down? ]
> >
>
> Huh. This is how it works now (even before my changes) - this is just
> refactoring!
> (really new thing is the only cpts_calc_mult_shift()).
The cpts_register() used to be called from probe(), but this changed
without any review in f280e89ad. That wasn't your fault, but still...
> Also there are additional questions such as:
> - is there guarantee that cpsw port will be connected to the same network
> after ifup?
The network is not relevant. PTP time is a global standard, just like
with NTP. We don't reset the NTP clock with ifup/down, do we?
> - should there be possibility to reset cc.mult if it's value will be kept
> from the previous run?
cc.mult will change naturally according to the operation of the user
space PTP stack. There is no need to reset it that I can see.
> > Secondly, you have made the initialization order of these fields hard
> > to follow. With the whole series applied:
> >
> > probe()
> > cpts_create()
> > cpts_of_parse()
> > {
> > /* Set cc_mult but not cc.mult! */
> > set cc_mult
> > set cc.shift
> > }
> > cpts_calc_mult_shift()
> > {
> > /* Set them both. */
> > cpts->cc_mult = mult;
> > cpts->cc.mult = mult;
>
> ^^ this assignment of cpts->cc.mult not required.
You wrote the code, not me.
> > cpts->cc.shift = shift;
>
>
> only in case there were not set in DT before
> (I have a requirement to support both - DT and cpts_calc_mult_shift and
> that introduces a bit of complexity)
>
> Also, I've tried not to add more fields in struct cpts.
>
> > }
> > /* later on */
> > cpts_register()
> > cpts->cc.mult = cpts->cc_mult;
> >
> > There is no need for such complexity. Simply set cc.mult in
> > cpts_create() _once_, immediately after the call to
> > cpts_calc_mult_shift().
> >
> > You can remove the assignment from cpts_calc_mult_shift() and
> > cpts_register().
>
> Just to clarify: do you propose to get rid of cpts->cc_mult at all?
No. Read what I said before:
There is no need for such complexity. Simply set cc.mult in
cpts_create() _once_, immediately after the call to
cpts_calc_mult_shift().
> Honestly, i'd not prefer to change functional behavior of ptp clock as part of
> this series.
Fair enough. The bogus clock reset existed before your series.
But please don't obfuscate simple initialization routines. The way
you set cc.mult and cc_mult is illogical and convoluted.
Thanks,
Richard
