On Thu, Feb 10, 2022 at 06:13:15PM +0100, Max Staudt wrote:
> --- /dev/null
> +++ b/drivers/net/can/elmcan.c
> @@ -0,0 +1,1299 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* elmcan.c - ELM327 based CAN interface driver
> + * (tty line discipline)
> + *
> + * This driver started as a derivative of linux/drivers/net/can/slcan.c
> + * and my thanks go to the original authors for their inspiration.
> + *
> + * elmcan.c Author : Max Staudt <max@xxxxxxxxx>
> + * slcan.c Author : Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
> + * slip.c Authors : Laurence Culhane <loz@xxxxxxxxxxxxxxxxxx>
> + * Fred N. van Kempen <waltje@xxxxxxxxxxxxxxxxxxx>
> + *
> + * This code barely bears any resemblance to slcan anymore, and whatever
> + * may be left is Linux specific boilerplate anyway, however I am leaving
> + * the GPL-2.0 identifier at the top just to be sure.
> + *
> + * Please feel free to use my own code, especially the ELM327 communication
> + * logic, in accordance with SPDX-License-Identifier BSD-3-Clause to port
> + * this driver to other systems.
> + * - Max
That is too hard to unwind and determine by anyone. Please don't do
stuff like that, it just gives lawyers nightmares :(
> + *
> + */
> +
> +#define pr_fmt(fmt) "[elmcan] " fmt
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +
> +#include <linux/atomic.h>
> +#include <linux/bitops.h>
> +#include <linux/ctype.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/if_ether.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/netdevice.h>
> +#include <linux/skbuff.h>
> +#include <linux/spinlock.h>
> +#include <linux/string.h>
> +#include <linux/tty.h>
> +#include <linux/tty_ldisc.h>
> +#include <linux/workqueue.h>
> +
> +#include <uapi/linux/tty.h>
> +
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +#include <linux/can/led.h>
> +#include <linux/can/rx-offload.h>
> +
> +MODULE_ALIAS_LDISC(N_ELMCAN);
> +MODULE_DESCRIPTION("ELM327 based CAN interface");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Max Staudt <max@xxxxxxxxx>");
> +
> +/* If this is enabled, we'll try to make the best of the situation
> + * even if we receive unexpected characters on the line.
> + * No guarantees.
> + * Handle with care, it's likely your hardware is unreliable!
> + */
> +static bool accept_flaky_uart;
> +module_param_named(accept_flaky_uart, accept_flaky_uart, bool, 0444);
> +MODULE_PARM_DESC(accept_flaky_uart, "Don't bail at the first invalid character. Behavior undefined.");
Module parameters are from the 1990's, please no. This is a per-code
setting, when you want it to be a per-device setting, right? Please
just drop this.
> + /***********************************************************************
> + * ELM327: Reception -> netdev glue *
> + * *
> + * (assumes elm->lock taken) *
> + ***********************************************************************/
> +
> +static void elm327_feed_frame_to_netdev(struct elmcan *elm,
Please use normal kernel function comment formats. kernel-doc for
global functions, simpler stuff for static functions.
> +static void elm327_parse_error(struct elmcan *elm, int len)
> +{
> + struct can_frame frame;
> +
> + memset(&frame, 0, sizeof(frame));
> + frame.can_id = CAN_ERR_FLAG;
> + frame.can_dlc = CAN_ERR_DLC;
> +
> + switch (len) {
> + case 17:
> + if (!memcmp(elm->rxbuf, "UNABLE TO CONNECT", 17)) {
hard-coded numbers here and everywhere. If you have these strings as a
define or array somewhere, you can calculate the length automatically.
> +/* Dummy needed to use bitrate_const */
> +static int elmcan_do_set_bittiming(struct net_device *netdev)
> +{
> + (void)netdev;
No need for this line.
> +
> + return 0;
> +}
...
> +static int __init elmcan_init(void)
> +{
> + int status;
> +
> + pr_info("ELM327 based best effort CAN interface driver\n");
> + pr_info("This device is severely limited as a CAN interface, see documentation.\n");
When all works properly, kernel drivers are quiet. No need for these
two lines at all.
> +
> + status = tty_register_ldisc(&elmcan_ldisc);
> + if (status)
> + pr_err("Can't register line discipline\n");
> +
> + return status;
> +}
> +
> +static void __exit elmcan_exit(void)
> +{
> + /* This will only be called when all channels have been closed by
> + * userspace - tty_ldisc.c takes care of the module's refcount.
> + */
> + tty_unregister_ldisc(&elmcan_ldisc);
> +}
> +
> +module_init(elmcan_init);
> +module_exit(elmcan_exit);
> diff --git a/include/uapi/linux/tty.h b/include/uapi/linux/tty.h
> index a58deb3061eb..4d3ad2569641 100644
> --- a/include/uapi/linux/tty.h
> +++ b/include/uapi/linux/tty.h
> @@ -39,5 +39,6 @@
> #define N_SPEAKUP 26 /* Speakup communication with synths */
> #define N_NULL 27 /* Null ldisc used for error handling */
> #define N_MCTP 28 /* MCTP-over-serial */
> +#define N_ELMCAN 29 /* Serial / USB serial OBD-II Interfaces */
We are now full, no more new ones to ever add! :)
This looks fine, we can always bump up the number if we want more.
thanks,
greg k-h
