[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANr-f5x-Vif5RJ5JBM+8L28byPb-E6-d0J1j5njhFiReTvXdZw@mail.gmail.com>
Date: Sun, 10 Apr 2022 14:32:32 +0200
From: Gerhard Engleder <gerhard@...leder-embedded.com>
To: Richard Cochran <richardcochran@...il.com>, yangbo.lu@....com
Cc: Vinicius Costa Gomes <vinicius.gomes@...el.com>,
David Miller <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, mlichvar@...hat.com,
netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 1/5] ptp: Add cycles support for virtual clocks
> > @@ -225,6 +233,21 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
> > mutex_init(&ptp->n_vclocks_mux);
> > init_waitqueue_head(&ptp->tsev_wq);
> >
> > + if (!ptp->info->getcycles64 && !ptp->info->getcyclesx64) {
>
> Please swap blocks, using non-negated logical test:
>
> if (ptp->info->getcycles64 || ptp->info->getcyclesx64)
I will change it as suggested.
> > + /* Free running cycle counter not supported, use time. */
> > + ptp->info->getcycles64 = ptp_getcycles64;
> > +
> > + if (ptp->info->gettimex64)
> > + ptp->info->getcyclesx64 = ptp->info->gettimex64;
> > +
> > + if (ptp->info->getcrosststamp)
> > + ptp->info->getcrosscycles = ptp->info->getcrosststamp;
> > + } else {
> > + ptp->has_cycles = true;
> > + if (!ptp->info->getcycles64 && ptp->info->getcyclesx64)
> > + ptp->info->getcycles64 = ptp_getcycles64;
> > + }
> > +
> > if (ptp->info->do_aux_work) {
> > kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker);
> > ptp->kworker = kthread_create_worker(0, "ptp%d", ptp->index);
>
>
> > @@ -231,10 +231,12 @@ static ssize_t n_vclocks_store(struct device *dev,
> > *(ptp->vclock_index + ptp->n_vclocks - i) = -1;
> > }
> >
> > - if (num == 0)
> > - dev_info(dev, "only physical clock in use now\n");
> > - else
> > - dev_info(dev, "guarantee physical clock free running\n");
> > + if (!ptp->has_cycles) {
>
> Not sure what this test means ...
I thought these dev_info() are useless if the free running cycle
counter is supported,
because the behavior of the physical clock does not change in this case.
> > + if (num == 0)
> > + dev_info(dev, "only physical clock in use now\n");
>
> Shouldn't this one print even if has_cycles == false?
It will print if has_cycles == false.
In my opinion this dev_info() tells the user that the physical clock can be used
again. So it does not carry any interesting information if has_cycles == true.
> > + else
> > + dev_info(dev, "guarantee physical clock free running\n");
> > + }
> >
> > ptp->n_vclocks = num;
> > mutex_unlock(&ptp->n_vclocks_mux);
Thank you!
Gerhard
Powered by blists - more mailing lists