diff mbox series

[3/3] can: esd_usb: Improved decoding for ESD_EV_CAN_ERROR_EXT messages

Message ID 20221219212717.1298282-2-frank.jungclaus@esd.eu
State New
Headers
Series can: esd_usb: Some more preparation for supporting esd CAN-USB/3 |

Commit Message

Frank Jungclaus Dec. 19, 2022, 9:27 p.m. UTC 
  As suggested by Marc there now is a union plus a struct ev_can_err_ext
for easier decoding of an ESD_EV_CAN_ERROR_EXT event message (which
simply is a rx_msg with some dedicated data).

Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Link: https://lore.kernel.org/linux-can/20220621071152.ggyhrr5sbzvwpkpx@pengutronix.de/
Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
---
 drivers/net/can/usb/esd_usb.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Comments

Vincent Mailhol Dec. 20, 2022, 5:27 a.m. UTC | #1 
Le mar. 20 déc. 2022 à 06:28, Frank Jungclaus <frank.jungclaus@esd.eu> a écrit :
>
> As suggested by Marc there now is a union plus a struct ev_can_err_ext
> for easier decoding of an ESD_EV_CAN_ERROR_EXT event message (which
> simply is a rx_msg with some dedicated data).
>
> Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Link: https://lore.kernel.org/linux-can/20220621071152.ggyhrr5sbzvwpkpx@pengutronix.de/
> Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
> ---
>  drivers/net/can/usb/esd_usb.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index 09745751f168..f90bb2c0ba15 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -127,7 +127,15 @@ struct rx_msg {
>         u8 dlc;
>         __le32 ts;
>         __le32 id; /* upper 3 bits contain flags */
> -       u8 data[8];
> +       union {
> +               u8 data[8];
> +               struct {
> +                       u8 status; /* CAN Controller Status */
> +                       u8 ecc;    /* Error Capture Register */
> +                       u8 rec;    /* RX Error Counter */
> +                       u8 tec;    /* TX Error Counter */
> +               } ev_can_err_ext;  /* For ESD_EV_CAN_ERROR_EXT */
> +       };
>  };
>
>  struct tx_msg {
> @@ -229,10 +237,10 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
>         u32 id = le32_to_cpu(msg->msg.rx.id) & ESD_IDMASK;
>
>         if (id == ESD_EV_CAN_ERROR_EXT) {
> -               u8 state = msg->msg.rx.data[0];
> -               u8 ecc = msg->msg.rx.data[1];
> -               u8 rxerr = msg->msg.rx.data[2];
> -               u8 txerr = msg->msg.rx.data[3];
> +               u8 state = msg->msg.rx.ev_can_err_ext.status;
> +               u8 ecc = msg->msg.rx.ev_can_err_ext.ecc;
> +               u8 rxerr = msg->msg.rx.ev_can_err_ext.rec;
> +               u8 txerr = msg->msg.rx.ev_can_err_ext.tec;

I do not like how you have to write msg->msg.rx.something. I think it
would be better to make the union within struct esd_usb_msg anonymous:

  https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/esd_usb.c#L169

That said, this is not a criticism of this patch but more something to
be addressed in a separate clean-up patch.

>                 netdev_dbg(priv->netdev,
>                            "CAN_ERR_EV_EXT: dlc=%#02x state=%02x ecc=%02x rec=%02x tec=%02x\n",
> --
> 2.25.1
>
Vincent Mailhol Dec. 20, 2022, 8:53 a.m. UTC | #2 
On Tue. 20 Dec. 2022 at 14:27, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> Le mar. 20 déc. 2022 à 06:28, Frank Jungclaus <frank.jungclaus@esd.eu> a écrit :
> > As suggested by Marc there now is a union plus a struct ev_can_err_ext
> > for easier decoding of an ESD_EV_CAN_ERROR_EXT event message (which
> > simply is a rx_msg with some dedicated data).
> >
> > Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > Link: https://lore.kernel.org/linux-can/20220621071152.ggyhrr5sbzvwpkpx@pengutronix.de/
> > Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
> > ---
> >  drivers/net/can/usb/esd_usb.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> > index 09745751f168..f90bb2c0ba15 100644
> > --- a/drivers/net/can/usb/esd_usb.c
> > +++ b/drivers/net/can/usb/esd_usb.c
> > @@ -127,7 +127,15 @@ struct rx_msg {
> >         u8 dlc;
> >         __le32 ts;
> >         __le32 id; /* upper 3 bits contain flags */
> > -       u8 data[8];
> > +       union {
> > +               u8 data[8];
> > +               struct {
> > +                       u8 status; /* CAN Controller Status */
> > +                       u8 ecc;    /* Error Capture Register */
> > +                       u8 rec;    /* RX Error Counter */
> > +                       u8 tec;    /* TX Error Counter */
> > +               } ev_can_err_ext;  /* For ESD_EV_CAN_ERROR_EXT */
> > +       };
> >  };
> >
> >  struct tx_msg {
> > @@ -229,10 +237,10 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> >         u32 id = le32_to_cpu(msg->msg.rx.id) & ESD_IDMASK;
> >
> >         if (id == ESD_EV_CAN_ERROR_EXT) {
> > -               u8 state = msg->msg.rx.data[0];
> > -               u8 ecc = msg->msg.rx.data[1];
> > -               u8 rxerr = msg->msg.rx.data[2];
> > -               u8 txerr = msg->msg.rx.data[3];
> > +               u8 state = msg->msg.rx.ev_can_err_ext.status;
> > +               u8 ecc = msg->msg.rx.ev_can_err_ext.ecc;
> > +               u8 rxerr = msg->msg.rx.ev_can_err_ext.rec;
> > +               u8 txerr = msg->msg.rx.ev_can_err_ext.tec;
>
> I do not like how you have to write msg->msg.rx.something. I think it
> would be better to make the union within struct esd_usb_msg anonymous:
>
>   https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/esd_usb.c#L169

Or maybe just declare esd_usb_msg as an union instead of a struct:

  union esd_usb_msg {
          struct header_msg hdr;
          struct version_msg version;
          struct version_reply_msg version_reply;
          struct rx_msg rx;
          struct tx_msg tx;
          struct tx_done_msg txdone;
          struct set_baudrate_msg setbaud;
          struct id_filter_msg filter;
  };
Marc Kleine-Budde Dec. 20, 2022, 9:05 a.m. UTC | #3 
On 20.12.2022 17:53:28, Vincent MAILHOL wrote:
> > >  struct tx_msg {
> > > @@ -229,10 +237,10 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> > >         u32 id = le32_to_cpu(msg->msg.rx.id) & ESD_IDMASK;
> > >
> > >         if (id == ESD_EV_CAN_ERROR_EXT) {
> > > -               u8 state = msg->msg.rx.data[0];
> > > -               u8 ecc = msg->msg.rx.data[1];
> > > -               u8 rxerr = msg->msg.rx.data[2];
> > > -               u8 txerr = msg->msg.rx.data[3];
> > > +               u8 state = msg->msg.rx.ev_can_err_ext.status;
> > > +               u8 ecc = msg->msg.rx.ev_can_err_ext.ecc;
> > > +               u8 rxerr = msg->msg.rx.ev_can_err_ext.rec;
> > > +               u8 txerr = msg->msg.rx.ev_can_err_ext.tec;
> >
> > I do not like how you have to write msg->msg.rx.something. I think it
> > would be better to make the union within struct esd_usb_msg anonymous:
> >
> >   https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/esd_usb.c#L169
> 
> Or maybe just declare esd_usb_msg as an union instead of a struct:

+1

>   union esd_usb_msg {
>           struct header_msg hdr;
>           struct version_msg version;
>           struct version_reply_msg version_reply;
>           struct rx_msg rx;
>           struct tx_msg tx;
>           struct tx_done_msg txdone;
>           struct set_baudrate_msg setbaud;
>           struct id_filter_msg filter;
>   };

Marc
Frank Jungclaus Dec. 21, 2022, 6:01 p.m. UTC | #4 
On Tue, 2022-12-20 at 17:53 +0900, Vincent MAILHOL wrote:
> On Tue. 20 Dec. 2022 at 14:27, Vincent MAILHOL
> <mailhol.vincent@wanadoo.fr> wrote:
> > Le mar. 20 déc. 2022 à 06:28, Frank Jungclaus <frank.jungclaus@esd.eu> a écrit :
> > > As suggested by Marc there now is a union plus a struct ev_can_err_ext
> > > for easier decoding of an ESD_EV_CAN_ERROR_EXT event message (which
> > > simply is a rx_msg with some dedicated data).
> > > 
> > > Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > > Link: https://lore.kernel.org/linux-can/20220621071152.ggyhrr5sbzvwpkpx@pengutronix.de/
> > > Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
> > > ---
> > >  drivers/net/can/usb/esd_usb.c | 18 +++++++++++++-----
> > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> > > index 09745751f168..f90bb2c0ba15 100644
> > > --- a/drivers/net/can/usb/esd_usb.c
> > > +++ b/drivers/net/can/usb/esd_usb.c
> > > @@ -127,7 +127,15 @@ struct rx_msg {
> > >         u8 dlc;
> > >         __le32 ts;
> > >         __le32 id; /* upper 3 bits contain flags */
> > > -       u8 data[8];
> > > +       union {
> > > +               u8 data[8];
> > > +               struct {
> > > +                       u8 status; /* CAN Controller Status */
> > > +                       u8 ecc;    /* Error Capture Register */
> > > +                       u8 rec;    /* RX Error Counter */
> > > +                       u8 tec;    /* TX Error Counter */
> > > +               } ev_can_err_ext;  /* For ESD_EV_CAN_ERROR_EXT */
> > > +       };
> > >  };
> > > 
> > >  struct tx_msg {
> > > @@ -229,10 +237,10 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> > >         u32 id = le32_to_cpu(msg->msg.rx.id) & ESD_IDMASK;
> > > 
> > >         if (id == ESD_EV_CAN_ERROR_EXT) {
> > > -               u8 state = msg->msg.rx.data[0];
> > > -               u8 ecc = msg->msg.rx.data[1];
> > > -               u8 rxerr = msg->msg.rx.data[2];
> > > -               u8 txerr = msg->msg.rx.data[3];
> > > +               u8 state = msg->msg.rx.ev_can_err_ext.status;
> > > +               u8 ecc = msg->msg.rx.ev_can_err_ext.ecc;
> > > +               u8 rxerr = msg->msg.rx.ev_can_err_ext.rec;
> > > +               u8 txerr = msg->msg.rx.ev_can_err_ext.tec;
> > 
> > I do not like how you have to write msg->msg.rx.something. I think it
> > would be better to make the union within struct esd_usb_msg anonymous:
> > 
> >   https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/esd_usb.c#L169
> 
> Or maybe just declare esd_usb_msg as an union instead of a struct:
> 
>   union esd_usb_msg {
>           struct header_msg hdr;
>           struct version_msg version;
>           struct version_reply_msg version_reply;
>           struct rx_msg rx;
>           struct tx_msg tx;
>           struct tx_done_msg txdone;
>           struct set_baudrate_msg setbaud;
>           struct id_filter_msg filter;
>   };

Apart from the fact that this change would probably require several
dozen lines of code to be adjusted, I like the idea ;)
Frank Jungclaus Jan. 23, 2023, 3:51 p.m. UTC | #5 
On Tue, 2022-12-20 at 10:05 +0100, Marc Kleine-Budde wrote:
> On 20.12.2022 17:53:28, Vincent MAILHOL wrote:
> > > >  struct tx_msg {
> > > > @@ -229,10 +237,10 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> > > >         u32 id = le32_to_cpu(msg->msg.rx.id) & ESD_IDMASK;
> > > > 
> > > >         if (id == ESD_EV_CAN_ERROR_EXT) {
> > > > -               u8 state = msg->msg.rx.data[0];
> > > > -               u8 ecc = msg->msg.rx.data[1];
> > > > -               u8 rxerr = msg->msg.rx.data[2];
> > > > -               u8 txerr = msg->msg.rx.data[3];
> > > > +               u8 state = msg->msg.rx.ev_can_err_ext.status;
> > > > +               u8 ecc = msg->msg.rx.ev_can_err_ext.ecc;
> > > > +               u8 rxerr = msg->msg.rx.ev_can_err_ext.rec;
> > > > +               u8 txerr = msg->msg.rx.ev_can_err_ext.tec;
> > > 
> > > I do not like how you have to write msg->msg.rx.something. I think it
> > > would be better to make the union within struct esd_usb_msg anonymous:
> > > 
> > >   https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/esd_usb.c#L169
> > 
> > Or maybe just declare esd_usb_msg as an union instead of a struct:
> 
> +1

Accepted ;)
I'll try to address this in a separate code-clean-up patch.

> 
> >   union esd_usb_msg {
> >           struct header_msg hdr;
> >           struct version_msg version;
> >           struct version_reply_msg version_reply;
> >           struct rx_msg rx;
> >           struct tx_msg tx;
> >           struct tx_done_msg txdone;
> >           struct set_baudrate_msg setbaud;
> >           struct id_filter_msg filter;
> >   };
> 
> Marc
>
diff mbox series

Patch

diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index 09745751f168..f90bb2c0ba15 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -127,7 +127,15 @@  struct rx_msg {
 	u8 dlc;
 	__le32 ts;
 	__le32 id; /* upper 3 bits contain flags */
-	u8 data[8];
+	union {
+		u8 data[8];
+		struct {
+			u8 status; /* CAN Controller Status */
+			u8 ecc;    /* Error Capture Register */
+			u8 rec;    /* RX Error Counter */
+			u8 tec;    /* TX Error Counter */
+		} ev_can_err_ext;  /* For ESD_EV_CAN_ERROR_EXT */
+	};
 };
 
 struct tx_msg {
@@ -229,10 +237,10 @@  static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
 	u32 id = le32_to_cpu(msg->msg.rx.id) & ESD_IDMASK;
 
 	if (id == ESD_EV_CAN_ERROR_EXT) {
-		u8 state = msg->msg.rx.data[0];
-		u8 ecc = msg->msg.rx.data[1];
-		u8 rxerr = msg->msg.rx.data[2];
-		u8 txerr = msg->msg.rx.data[3];
+		u8 state = msg->msg.rx.ev_can_err_ext.status;
+		u8 ecc = msg->msg.rx.ev_can_err_ext.ecc;
+		u8 rxerr = msg->msg.rx.ev_can_err_ext.rec;
+		u8 txerr = msg->msg.rx.ev_can_err_ext.tec;
 
 		netdev_dbg(priv->netdev,
 			   "CAN_ERR_EV_EXT: dlc=%#02x state=%02x ecc=%02x rec=%02x tec=%02x\n",