* [PATCH] can: mcba_usb: Fix termination command argument
@ 2022-11-23 19:44 Yasushi SHOJI
2022-11-23 22:34 ` Marc Kleine-Budde
2022-11-24 0:53 ` Vincent Mailhol
0 siblings, 2 replies; 7+ messages in thread
From: Yasushi SHOJI @ 2022-11-23 19:44 UTC (permalink / raw)
To: Yasushi SHOJI, Wolfgang Grandegger, Marc Kleine-Budde,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: linux-can, netdev, linux-kernel
Microchip USB Analyzer can be set with termination setting ON or OFF.
As I've observed, both with my oscilloscope and USB packet capture
below, you must send "0" to turn it ON, and "1" to turn it OFF.
Reverse the argument value to fix this.
These are the two commands sequence, ON then OFF.
> No. Time Source Destination Protocol Length Info
> 1 0.000000 host 1.3.1 USB 46 URB_BULK out
>
> Frame 1: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
> USB URB
> Leftover Capture Data: a80000000000000000000000000000000000a8
>
> No. Time Source Destination Protocol Length Info
> 2 4.372547 host 1.3.1 USB 46 URB_BULK out
>
> Frame 2: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
> USB URB
> Leftover Capture Data: a80100000000000000000000000000000000a9
Signed-off-by: Yasushi SHOJI <yashi@spacecubics.com>
---
drivers/net/can/usb/mcba_usb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
index 218b098b261d..67beff1a3876 100644
--- a/drivers/net/can/usb/mcba_usb.c
+++ b/drivers/net/can/usb/mcba_usb.c
@@ -785,9 +785,9 @@ static int mcba_set_termination(struct net_device *netdev, u16 term)
};
if (term == MCBA_TERMINATION_ENABLED)
- usb_msg.termination = 1;
- else
usb_msg.termination = 0;
+ else
+ usb_msg.termination = 1;
mcba_usb_xmit_cmd(priv, (struct mcba_usb_msg *)&usb_msg);
--
2.38.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] can: mcba_usb: Fix termination command argument
2022-11-23 19:44 [PATCH] can: mcba_usb: Fix termination command argument Yasushi SHOJI
@ 2022-11-23 22:34 ` Marc Kleine-Budde
2022-11-24 9:50 ` Yasushi SHOJI
2022-11-24 0:53 ` Vincent Mailhol
1 sibling, 1 reply; 7+ messages in thread
From: Marc Kleine-Budde @ 2022-11-23 22:34 UTC (permalink / raw)
To: Yasushi SHOJI, Remigiusz Kołłątaj
Cc: Yasushi SHOJI, Wolfgang Grandegger, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-can, netdev,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]
Let's take the original driver author into the loop.
On 24.11.2022 04:44:06, Yasushi SHOJI wrote:
> Microchip USB Analyzer can be set with termination setting ON or OFF.
> As I've observed, both with my oscilloscope and USB packet capture
> below, you must send "0" to turn it ON, and "1" to turn it OFF.
>
> Reverse the argument value to fix this.
>
> These are the two commands sequence, ON then OFF.
>
> > No. Time Source Destination Protocol Length Info
> > 1 0.000000 host 1.3.1 USB 46 URB_BULK out
> >
> > Frame 1: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
> > USB URB
> > Leftover Capture Data: a80000000000000000000000000000000000a8
> >
> > No. Time Source Destination Protocol Length Info
> > 2 4.372547 host 1.3.1 USB 46 URB_BULK out
> >
> > Frame 2: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
> > USB URB
> > Leftover Capture Data: a80100000000000000000000000000000000a9
Is this the USB data after applying the patch?
Can you measure the resistance between CAN-H and CAN-L to verify that
your patch fixes the problem?
> Signed-off-by: Yasushi SHOJI <yashi@spacecubics.com>
> ---
> drivers/net/can/usb/mcba_usb.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> index 218b098b261d..67beff1a3876 100644
> --- a/drivers/net/can/usb/mcba_usb.c
> +++ b/drivers/net/can/usb/mcba_usb.c
> @@ -785,9 +785,9 @@ static int mcba_set_termination(struct net_device *netdev, u16 term)
> };
>
> if (term == MCBA_TERMINATION_ENABLED)
> - usb_msg.termination = 1;
> - else
> usb_msg.termination = 0;
> + else
> + usb_msg.termination = 1;
>
> mcba_usb_xmit_cmd(priv, (struct mcba_usb_msg *)&usb_msg);
What about the static void mcba_usb_process_ka_usb() function? Do you
need to convert this, too?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] can: mcba_usb: Fix termination command argument
2022-11-23 22:34 ` Marc Kleine-Budde
@ 2022-11-24 9:50 ` Yasushi SHOJI
0 siblings, 0 replies; 7+ messages in thread
From: Yasushi SHOJI @ 2022-11-24 9:50 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Remigiusz Kołłątaj, Yasushi SHOJI,
Wolfgang Grandegger, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, linux-can, netdev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2753 bytes --]
Hi,
On Thu, Nov 24, 2022 at 7:34 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> Let's take the original driver author into the loop.
>
> On 24.11.2022 04:44:06, Yasushi SHOJI wrote:
> > Microchip USB Analyzer can be set with termination setting ON or OFF.
> > As I've observed, both with my oscilloscope and USB packet capture
> > below, you must send "0" to turn it ON, and "1" to turn it OFF.
> >
> > Reverse the argument value to fix this.
> >
> > These are the two commands sequence, ON then OFF.
> >
> > > No. Time Source Destination Protocol Length Info
> > > 1 0.000000 host 1.3.1 USB 46 URB_BULK out
> > >
> > > Frame 1: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
> > > USB URB
> > > Leftover Capture Data: a80000000000000000000000000000000000a8
> > >
> > > No. Time Source Destination Protocol Length Info
> > > 2 4.372547 host 1.3.1 USB 46 URB_BULK out
> > >
> > > Frame 2: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
> > > USB URB
> > > Leftover Capture Data: a80100000000000000000000000000000000a9
>
> Is this the USB data after applying the patch?
That's not from Linux.
> Can you measure the resistance between CAN-H and CAN-L to verify that
> your patch fixes the problem?
Sure. The command I'm using on my Linux is:
sudo ip link set can0 up type can bitrate 100000 termination X
where X is either 0 or 120.
With Debian Sid stock kernel: linux-image-6.0.0-4-amd64
- termination 0: 135.4 Ohms
- termination 120: 17.82 Ohms
With my patch on v6.1-rc6
- termination 0: 22.20 Ohms
- termination 120: 134.2 Ohms
> > Signed-off-by: Yasushi SHOJI <yashi@spacecubics.com>
> > ---
> > drivers/net/can/usb/mcba_usb.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> > index 218b098b261d..67beff1a3876 100644
> > --- a/drivers/net/can/usb/mcba_usb.c
> > +++ b/drivers/net/can/usb/mcba_usb.c
> > @@ -785,9 +785,9 @@ static int mcba_set_termination(struct net_device *netdev, u16 term)
> > };
> >
> > if (term == MCBA_TERMINATION_ENABLED)
> > - usb_msg.termination = 1;
> > - else
> > usb_msg.termination = 0;
> > + else
> > + usb_msg.termination = 1;
> >
> > mcba_usb_xmit_cmd(priv, (struct mcba_usb_msg *)&usb_msg);
>
> What about the static void mcba_usb_process_ka_usb() function? Do you
> need to convert this, too?
Ah, yes. Thanks.
Attaching a compressed patch.
Let me know if I need to resend it as an email.
Best,
--
yashi
[-- Attachment #2: 0001-can-mcba_usb-Fix-termination-command-argument.patch.gz --]
[-- Type: application/gzip, Size: 995 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] can: mcba_usb: Fix termination command argument
2022-11-23 19:44 [PATCH] can: mcba_usb: Fix termination command argument Yasushi SHOJI
2022-11-23 22:34 ` Marc Kleine-Budde
@ 2022-11-24 0:53 ` Vincent Mailhol
2022-11-24 9:52 ` Yasushi SHOJI
1 sibling, 1 reply; 7+ messages in thread
From: Vincent Mailhol @ 2022-11-24 0:53 UTC (permalink / raw)
To: Yasushi SHOJI
Cc: Yasushi SHOJI, Wolfgang Grandegger, Marc Kleine-Budde,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-can, netdev, linux-kernel
On Thu. 24 Nov. 2022 at 04:53, Yasushi SHOJI <yasushi.shoji@gmail.com> wrote:
> Microchip USB Analyzer can be set with termination setting ON or OFF.
> As I've observed, both with my oscilloscope and USB packet capture
> below, you must send "0" to turn it ON, and "1" to turn it OFF.
>
> Reverse the argument value to fix this.
>
> These are the two commands sequence, ON then OFF.
>
> > No. Time Source Destination Protocol Length Info
> > 1 0.000000 host 1.3.1 USB 46 URB_BULK out
> >
> > Frame 1: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
> > USB URB
> > Leftover Capture Data: a80000000000000000000000000000000000a8
> >
> > No. Time Source Destination Protocol Length Info
> > 2 4.372547 host 1.3.1 USB 46 URB_BULK out
> >
> > Frame 2: 46 bytes on wire (368 bits), 46 bytes captured (368 bits)
> > USB URB
> > Leftover Capture Data: a80100000000000000000000000000000000a9
>
> Signed-off-by: Yasushi SHOJI <yashi@spacecubics.com>
> ---
> drivers/net/can/usb/mcba_usb.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> index 218b098b261d..67beff1a3876 100644
> --- a/drivers/net/can/usb/mcba_usb.c
> +++ b/drivers/net/can/usb/mcba_usb.c
> @@ -785,9 +785,9 @@ static int mcba_set_termination(struct net_device *netdev, u16 term)
> };
>
> if (term == MCBA_TERMINATION_ENABLED)
> - usb_msg.termination = 1;
> - else
> usb_msg.termination = 0;
> + else
> + usb_msg.termination = 1;
>
> mcba_usb_xmit_cmd(priv, (struct mcba_usb_msg *)&usb_msg);
Nitpick: does it make sense to rename the field to something like
usb_msg.termination_disable or usb_msg.termination_off? This would
make it more explicit that this is a "reverse" boolean.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] can: mcba_usb: Fix termination command argument
2022-11-24 0:53 ` Vincent Mailhol
@ 2022-11-24 9:52 ` Yasushi SHOJI
2022-11-24 9:54 ` Vincent Mailhol
2022-11-24 14:38 ` Marc Kleine-Budde
0 siblings, 2 replies; 7+ messages in thread
From: Yasushi SHOJI @ 2022-11-24 9:52 UTC (permalink / raw)
To: Vincent Mailhol
Cc: Yasushi SHOJI, Wolfgang Grandegger, Marc Kleine-Budde,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-can, netdev, linux-kernel
On Thu, Nov 24, 2022 at 9:53 AM Vincent Mailhol
<vincent.mailhol@gmail.com> wrote:
> > diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> > index 218b098b261d..67beff1a3876 100644
> > --- a/drivers/net/can/usb/mcba_usb.c
> > +++ b/drivers/net/can/usb/mcba_usb.c
> > @@ -785,9 +785,9 @@ static int mcba_set_termination(struct net_device *netdev, u16 term)
> > };
> >
> > if (term == MCBA_TERMINATION_ENABLED)
> > - usb_msg.termination = 1;
> > - else
> > usb_msg.termination = 0;
> > + else
> > + usb_msg.termination = 1;
> >
> > mcba_usb_xmit_cmd(priv, (struct mcba_usb_msg *)&usb_msg);
>
> Nitpick: does it make sense to rename the field to something like
> usb_msg.termination_disable or usb_msg.termination_off? This would
> make it more explicit that this is a "reverse" boolean.
I'd rather define the values like
#define TERMINATION_ON (0)
#define TERMINATION_OFF (1)
So the block becomes
if (term == MCBA_TERMINATION_ENABLED)
usb_msg.termination = TERMINATION_ON;
else
usb_msg.termination = TERMINATION_OFF;
--
yashi
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] can: mcba_usb: Fix termination command argument
2022-11-24 9:52 ` Yasushi SHOJI
@ 2022-11-24 9:54 ` Vincent Mailhol
2022-11-24 14:38 ` Marc Kleine-Budde
1 sibling, 0 replies; 7+ messages in thread
From: Vincent Mailhol @ 2022-11-24 9:54 UTC (permalink / raw)
To: Yasushi SHOJI
Cc: Yasushi SHOJI, Wolfgang Grandegger, Marc Kleine-Budde,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-can, netdev, linux-kernel
On Thu. 24 Nov. 2022 at 18:52, Yasushi SHOJI <yasushi.shoji@gmail.com> wrote:
> On Thu, Nov 24, 2022 at 9:53 AM Vincent Mailhol
> <vincent.mailhol@gmail.com> wrote:
> > > diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> > > index 218b098b261d..67beff1a3876 100644
> > > --- a/drivers/net/can/usb/mcba_usb.c
> > > +++ b/drivers/net/can/usb/mcba_usb.c
> > > @@ -785,9 +785,9 @@ static int mcba_set_termination(struct net_device *netdev, u16 term)
> > > };
> > >
> > > if (term == MCBA_TERMINATION_ENABLED)
> > > - usb_msg.termination = 1;
> > > - else
> > > usb_msg.termination = 0;
> > > + else
> > > + usb_msg.termination = 1;
> > >
> > > mcba_usb_xmit_cmd(priv, (struct mcba_usb_msg *)&usb_msg);
> >
> > Nitpick: does it make sense to rename the field to something like
> > usb_msg.termination_disable or usb_msg.termination_off? This would
> > make it more explicit that this is a "reverse" boolean.
>
> I'd rather define the values like
>
> #define TERMINATION_ON (0)
> #define TERMINATION_OFF (1)
>
> So the block becomes
>
> if (term == MCBA_TERMINATION_ENABLED)
> usb_msg.termination = TERMINATION_ON;
> else
> usb_msg.termination = TERMINATION_OFF;
That also works! Thank you.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] can: mcba_usb: Fix termination command argument
2022-11-24 9:52 ` Yasushi SHOJI
2022-11-24 9:54 ` Vincent Mailhol
@ 2022-11-24 14:38 ` Marc Kleine-Budde
1 sibling, 0 replies; 7+ messages in thread
From: Marc Kleine-Budde @ 2022-11-24 14:38 UTC (permalink / raw)
To: Yasushi SHOJI
Cc: Vincent Mailhol, Yasushi SHOJI, Wolfgang Grandegger,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-can, netdev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1684 bytes --]
On 24.11.2022 18:52:14, Yasushi SHOJI wrote:
> On Thu, Nov 24, 2022 at 9:53 AM Vincent Mailhol
> <vincent.mailhol@gmail.com> wrote:
> > > diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> > > index 218b098b261d..67beff1a3876 100644
> > > --- a/drivers/net/can/usb/mcba_usb.c
> > > +++ b/drivers/net/can/usb/mcba_usb.c
> > > @@ -785,9 +785,9 @@ static int mcba_set_termination(struct net_device *netdev, u16 term)
> > > };
> > >
> > > if (term == MCBA_TERMINATION_ENABLED)
> > > - usb_msg.termination = 1;
> > > - else
> > > usb_msg.termination = 0;
> > > + else
> > > + usb_msg.termination = 1;
> > >
> > > mcba_usb_xmit_cmd(priv, (struct mcba_usb_msg *)&usb_msg);
> >
> > Nitpick: does it make sense to rename the field to something like
> > usb_msg.termination_disable or usb_msg.termination_off? This would
> > make it more explicit that this is a "reverse" boolean.
>
> I'd rather define the values like
>
> #define TERMINATION_ON (0)
> #define TERMINATION_OFF (1)
>
> So the block becomes
>
> if (term == MCBA_TERMINATION_ENABLED)
> usb_msg.termination = TERMINATION_ON;
> else
> usb_msg.termination = TERMINATION_OFF;
Please send a v2 patch, using git send-email, as you did with the first
version. (No compressed attached patches please.)
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-11-24 14:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-23 19:44 [PATCH] can: mcba_usb: Fix termination command argument Yasushi SHOJI
2022-11-23 22:34 ` Marc Kleine-Budde
2022-11-24 9:50 ` Yasushi SHOJI
2022-11-24 0:53 ` Vincent Mailhol
2022-11-24 9:52 ` Yasushi SHOJI
2022-11-24 9:54 ` Vincent Mailhol
2022-11-24 14:38 ` Marc Kleine-Budde
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).