| Message ID | 20221219212717.1298282-2-frank.jungclaus@esd.eu (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: esd_usb: Some more preparation for supporting esd CAN-USB/3 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
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 >
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; };
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
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 ;)
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 --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",
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(-)