From: Min Li <[email protected]>
Use div_s64 so that the neg_adj is not needed.
Signed-off-by: Min Li <[email protected]>
---
drivers/ptp/ptp_idt82p33.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/ptp/ptp_idt82p33.c b/drivers/ptp/ptp_idt82p33.c
index b1528a0..e970379d 100644
--- a/drivers/ptp/ptp_idt82p33.c
+++ b/drivers/ptp/ptp_idt82p33.c
@@ -320,7 +320,6 @@ static int _idt82p33_adjfine(struct idt82p33_channel *channel, long scaled_ppm)
{
struct idt82p33 *idt82p33 = channel->idt82p33;
unsigned char buf[5] = {0};
- int neg_adj = 0;
int err, i;
s64 fcw;
@@ -340,16 +339,9 @@ static int _idt82p33_adjfine(struct idt82p33_channel *channel, long scaled_ppm)
* FCW = -------------
* 168 * 2^4
*/
- if (scaled_ppm < 0) {
- neg_adj = 1;
- scaled_ppm = -scaled_ppm;
- }
fcw = scaled_ppm * 244140625ULL;
- fcw = div_u64(fcw, 2688);
-
- if (neg_adj)
- fcw = -fcw;
+ fcw = div_s64(fcw, 2688);
for (i = 0; i < 5; i++) {
buf[i] = fcw & 0xff;
--
2.7.4
On Wed, Nov 04, 2020 at 11:01:49AM -0500, [email protected] wrote:
> From: Min Li <[email protected]>
>
> Use div_s64 so that the neg_adj is not needed.
Back in the day, I coded the neg_adj because there was some issue with
signed 64 bit division that I can't recall now. Either div_s64 didn't
exist or it was buggy on some archs... there was _some_ reason.
So unless you are sure that this works on all platforms, I would leave
it alone.
Thanks,
Richard
> Signed-off-by: Min Li <[email protected]>
> ---
> drivers/ptp/ptp_idt82p33.c | 10 +---------
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/drivers/ptp/ptp_idt82p33.c b/drivers/ptp/ptp_idt82p33.c
> index b1528a0..e970379d 100644
> --- a/drivers/ptp/ptp_idt82p33.c
> +++ b/drivers/ptp/ptp_idt82p33.c
> @@ -320,7 +320,6 @@ static int _idt82p33_adjfine(struct idt82p33_channel *channel, long scaled_ppm)
> {
> struct idt82p33 *idt82p33 = channel->idt82p33;
> unsigned char buf[5] = {0};
> - int neg_adj = 0;
> int err, i;
> s64 fcw;
>
> @@ -340,16 +339,9 @@ static int _idt82p33_adjfine(struct idt82p33_channel *channel, long scaled_ppm)
> * FCW = -------------
> * 168 * 2^4
> */
> - if (scaled_ppm < 0) {
> - neg_adj = 1;
> - scaled_ppm = -scaled_ppm;
> - }
>
> fcw = scaled_ppm * 244140625ULL;
> - fcw = div_u64(fcw, 2688);
> -
> - if (neg_adj)
> - fcw = -fcw;
> + fcw = div_s64(fcw, 2688);
>
> for (i = 0; i < 5; i++) {
> buf[i] = fcw & 0xff;
> --
> 2.7.4
>
On Wed, Nov 04, 2020 at 08:46:57AM -0800, Richard Cochran wrote:
> On Wed, Nov 04, 2020 at 11:01:49AM -0500, [email protected] wrote:
> > From: Min Li <[email protected]>
> >
> > Use div_s64 so that the neg_adj is not needed.
>
> Back in the day, I coded the neg_adj because there was some issue with
> signed 64 bit division that I can't recall now. Either div_s64 didn't
> exist or it was buggy on some archs... there was _some_ reason.
>
> So unless you are sure that this works on all platforms, I would leave
> it alone.
On the other hand and with all due respect, saying that it may have been
'buggy on some archs back in the day' and then not bringing any evidence
is a bit of a strange claim to make.
I am actively using div_s64 in drivers/net/dsa/sja1105/sja1105_ptp.c
successfully on arm and arm64.
We may keep the ptp_clock_info::adjfine procedure as is, and to be
copied by everyone, because we can't make sure that it works "on all
platforms" (aka "cargo cult"). Or we could waste a few hours from
somebody's time, until he figures out how to bisect the IDT 82P33 PTP
driver (a driver with 3 patches, and 3 more with Min's series) to find a
1-line change, and then we could find out what the problem you were
seeing was. I say waste that guy's time :)
On Thu, Nov 05, 2020 at 02:35:56AM +0200, Vladimir Oltean wrote:
> On the other hand and with all due respect, saying that it may have been
> 'buggy on some archs back in the day' and then not bringing any evidence
> is a bit of a strange claim to make.
You're right. I made the effort to look back into the days of v3.0,
and the only thing I could find is that the 32 bit implementation of
div_s64 does extra operations and invokes an additional function call.
But the difference in performance, if any, is probably not very large.
> I am actively using div_s64 in drivers/net/dsa/sja1105/sja1105_ptp.c
> successfully on arm and arm64.
Yeah, I see div_s64 has found its way into the ntp code, too, so it
must be fine.
Thanks,
Richard