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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 |
On Thu. 24 Nov. 2022 at 04:53, Yasushi SHOJI <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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. 24 Nov. 2022 at 18:52, Yasushi SHOJI <[email protected]> wrote:
> On Thu, Nov 24, 2022 at 9:53 AM Vincent Mailhol
> <[email protected]> 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 Thu, Nov 24, 2022 at 9:53 AM Vincent Mailhol
<[email protected]> 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 24.11.2022 18:52:14, Yasushi SHOJI wrote:
> On Thu, Nov 24, 2022 at 9:53 AM Vincent Mailhol
> <[email protected]> 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 |