Re: [PATCH v5 2/3] greybus: Add BeaglePlay Linux Driver

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 24/09/2023 13:36, Ayush Singh wrote:
> Add the Greybus host driver for BeaglePlay board by BeagleBoard.org.
> 
> Current beagleconnect setup involves running SVC in a user-space
> application (GBridge) and using netlink to communicate with kernel
> space. GBridge itself uses wpanusb kernel driver for communication with
> CC1325, so the greybus messages travel from kernel space (gb_netlink) to
> user-space (GBridge) and then back to kernel space (wpanusb) before
> reaching CC1352.
> 

Thank you for your patch. There is something to discuss/improve.

>  
>  GREYBUS SUBSYSTEM
>  M:	Johan Hovold <johan@kernel.org>
> diff --git a/drivers/greybus/Kconfig b/drivers/greybus/Kconfig
> index 78ba3c3083d5..07e3119e2faa 100644
> --- a/drivers/greybus/Kconfig
> +++ b/drivers/greybus/Kconfig
> @@ -17,6 +17,16 @@ menuconfig GREYBUS
>  
>  if GREYBUS
>  
> +config GREYBUS_BEAGLEPLAY
> +	tristate "Greybus BeaglePlay driver"
> +	depends on TTY
> +	help
> +	  Select this option if you have a BeaglePlay where CC1352
> +		co-processor acts as Greybus SVC.

Fix indentation.

> +
> +	  To compile this code as a module, chose M here: the module
> +	  will be called gb-beagleplay.ko
> +
>  config GREYBUS_ES2
>  	tristate "Greybus ES3 USB host controller"
>  	depends on USB
> diff --git a/drivers/greybus/Makefile b/drivers/greybus/Makefile
> index 9bccdd229aa2..15a84a83788d 100644
> --- a/drivers/greybus/Makefile
> +++ b/drivers/greybus/Makefile
> @@ -18,9 +18,9 @@ obj-$(CONFIG_GREYBUS)		+= greybus.o
>  # needed for trace events
>  ccflags-y += -I$(src)
>  
> +obj-$(CONFIG_GREYBUS_BEAGLEPLAY)	+= gb-beagleplay.o
> +
>  # Greybus Host controller drivers
>  gb-es2-y := es2.o
>  
>  obj-$(CONFIG_GREYBUS_ES2)	+= gb-es2.o
> -
> -

Does not look related to your patch.

> diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
> new file mode 100644
> index 000000000000..39d87ef3b8fc
> --- /dev/null
> +++ b/drivers/greybus/gb-beagleplay.c
> @@ -0,0 +1,526 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Beagleplay Linux Driver for Greybus
> + *
> + * Copyright (c) 2023 Ayush Singh <ayushdevel1325@gmail.com>
> + * Copyright (c) 2023  BeagleBoard.org Foundation
> + */
> +
> +#include <linux/gfp.h>
> +#include <linux/greybus.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/printk.h>
> +#include <linux/serdev.h>
> +#include <linux/tty.h>
> +#include <linux/tty_driver.h>
> +#include <linux/greybus/hd.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/crc-ccitt.h>
> +#include <linux/circ_buf.h>
> +#include <linux/types.h>
> +#include <linux/workqueue.h>
> +
> +#define RX_HDLC_PAYLOAD 1024
> +#define CRC_LEN 2
> +#define MAX_RX_HDLC (1 + RX_HDLC_PAYLOAD + CRC_LEN)
> +#define TX_CIRC_BUF_SIZE 1024
> +
> +#define ADDRESS_GREYBUS 0x01
> +#define ADDRESS_DBG 0x02
> +#define ADDRESS_CONTROL 0x03
> +
> +#define HDLC_FRAME 0x7E
> +#define HDLC_ESC 0x7D
> +#define HDLC_XOR 0x20
> +
> +#define CONTROL_SVC_START 0x01
> +#define CONTROL_SVC_STOP 0x02
> +
> +/* The maximum number of CPorts supported by Greybus Host Device */
> +#define BEAGLEPLAY_GB_MAX_CPORTS 32
> +
> +/*

Use kerneldoc.

> + * BeaglePlay Greybus driver
> + *
> + * @serdev: Serdev device
> + *
> + * @gb_host_device: greybud host device
> + *
> + * @tx_work: transmit work
> + * @tx_producer_lock: transmit producer lock
> + * @tx_consumer_lock: transmit consumer lock
> + * @tx_circ_buf: transmit circular buffer
> + * @tx_crc: HDCL CRC
> + * @tx_ack_seq: current TX ACK sequence number
> + *
> + * @rx_buffer_len: Rx buffer length
> + * @rx_in_esc: Rx Flag to indicate if ESC
> + * @rx_buffer: Rx buffer

This is absolutely useless comment. We know it is RX buffer because
member is called "rx_buffer".

> + */
> +struct gb_beagleplay {
> +	struct serdev_device *serdev;
> +
> +	struct gb_host_device *gb_host_device;
> +
> +	struct work_struct tx_work;
> +	/* tx_producer_lock: HDLC producer lock */

Do not comment in two places - kerneldoc and in-line. Only one place.

> +	spinlock_t tx_producer_lock;
> +	/* tx_consumer_lock: HDLC consumer lock */
> +	spinlock_t tx_consumer_lock;
> +	struct circ_buf tx_circ_buf;
> +	u16 tx_crc;
> +	u8 tx_ack_seq;
> +
> +	u16 rx_buffer_len;
> +	u8 rx_in_esc;
> +	u8 rx_buffer[MAX_RX_HDLC];
> +};
> +
> +struct hdlc_payload {
> +	u16 length;
> +	void *payload;
> +};
> +

...

> +
> +static int gb_serdev_init(struct gb_beagleplay *bg)
> +{
> +	u32 speed = 115200;
> +	int ret;
> +
> +	serdev_device_set_drvdata(bg->serdev, bg);
> +	serdev_device_set_client_ops(bg->serdev, &gb_beagleplay_ops);
> +	ret = serdev_device_open(bg->serdev);
> +	if (ret) {
> +		return dev_err_probe(&bg->serdev->dev, ret,
> +				     "Unable to Open Serial Device");
> +	}

Please run scripts/checkpatch.pl --strict and fix reported warnings.
Some warnings can be ignored, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.

> +	speed = serdev_device_set_baudrate(bg->serdev, speed);
> +	serdev_device_set_flow_control(bg->serdev, false);
> +
> +	return 0;
> +}
> +
> +static int gb_beagleplay_probe(struct serdev_device *serdev)
> +{
> +	int ret = 0;
> +	struct gb_beagleplay *bg =
> +		devm_kmalloc(&serdev->dev, sizeof(*bg), GFP_KERNEL);

Don't mix dynamic memory allocation with variable definition. That's not
readable.

> +

There is never blank line between allocation and the check.

> +	if (!bg)
> +		return -ENOMEM;
> +
> +	bg->serdev = serdev;
> +	ret = gb_serdev_init(bg);
> +	if (ret)
> +		return ret;
> +
> +	ret = hdlc_init(bg);
> +	if (ret)
> +		goto free_serdev;
> +
> +	ret = gb_greybus_init(bg);
> +	if (ret)
> +		goto free_hdlc;
> +
> +	gb_beagleplay_start_svc(bg);
> +
> +	return 0;
> +
> +free_hdlc:
> +	hdlc_deinit(bg);
> +free_serdev:
> +	gb_serdev_deinit(bg);
> +	return ret;
> +}
> +
> +static void gb_beagleplay_remove(struct serdev_device *serdev)
> +{
> +	struct gb_beagleplay *bg = serdev_device_get_drvdata(serdev);
> +
> +	gb_greybus_deinit(bg);
> +	gb_beagleplay_stop_svc(bg);
> +	hdlc_deinit(bg);
> +	gb_serdev_deinit(bg);
> +}
> +
> +static const struct of_device_id gb_beagleplay_of_match[] = {
> +	{
> +		.compatible = "beagle,play-cc1352",
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, gb_beagleplay_of_match);
> +
> +static struct serdev_device_driver gb_beagleplay_driver = {
> +	.probe = gb_beagleplay_probe,
> +	.remove = gb_beagleplay_remove,
> +	.driver = {
> +	      .name = "gb_beagleplay",
> +	      .of_match_table = gb_beagleplay_of_match,

This is still wrongly aligned. Spaces after tab. Are you sure checkpatch
does not complain bout it?

> +	    },

Align with .driver, so only one tab.

> +};
> +
> +module_serdev_device_driver(gb_beagleplay_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Ayush Singh <ayushdevel1325@gmail.com>");
> +MODULE_DESCRIPTION("A Greybus driver for BeaglePlay");

Best regards,
Krzysztof