| Message ID | 20221123194406.80575-1-yashi@spacecubics.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: mcba_usb: Fix termination command argument | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
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
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.
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
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
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.
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
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);