Re: [PATCH 4/6] can: gs_usb: add ability to enable / disable berr rerporting

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2674 bytes --]

On 09.10.2022 00:11:49, Jeroen Hofstee wrote:
> Hi,
> 
> On 10/6/22 18:36, Marc Kleine-Budde wrote:
> > On 06.10.2022 18:24:50, Marc Kleine-Budde wrote:
> > > From: Jeroen Hofstee <jhofstee@victronenergy.com>
> > > 
> > > The open source firmware candleLight report bus errors
> > > unconditionally. This adds support to enable / disable bus error
> > > reporting with the standard netlink property.
> > I haven't checked the candleLight firmware, yet.
> > 
> > If the unmodified firmware sends bus errors per default and we introduce
> > BERR_REPORTING as suggested in this patch, we have to modify the default
> > behavior for bus errors: By default the firmware will not send bus
> > errors, but only if GS_CAN_MODE_BERR_REPORTING is requested during
> > open().
> > 
> > I'm not sure if we want to change the default behavior of the
> > firmware....To work around this backwards compatibility issue we can
> > explicitly turn BERR reporting on or off during open via
> > GS_CAN_MODE_BERR_REPORTING_ON or GS_CAN_MODE_BERR_REPORTING_OFF.
> 
> It would look like this, untested:
> 
> subject: can: gs_usb: add ability to enable / disable berr rerporting
> 
> The open source firmware candleLight report bus errors
> unconditionally. This adds support to enable / disable bus error
> reporting with the standard netlink property.
> 
> Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/can/usb/gs_usb.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 7c1f13a90419..039e8d91ad88 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -135,6 +135,13 @@ struct gs_device_config {
>  /* GS_CAN_FEATURE_BT_CONST_EXT BIT(10) */
>  /* GS_CAN_FEATURE_TERMINATION BIT(11) */
> 
> +/* Previous versions of this driver didn't support the berr-reporting flag,
> + * this flag tells if berr-reporting is supported. When not set, the device
> + * can default to its own preference to be backwards compatible.
> + */
> +#define GS_CAN_MODE_BERR_REPORTING_SET BIT(12)
> +#define GS_CAN_MODE_BERR_REPORTING BIT(13)

You've convinced me :) This is overengineered, and it "breaks" the nice
1-to-1 mapping of feature and mode bits :)

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 --]