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@xxxxxxxxxx>
> 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@xxxxxxxxx>
> + * 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@xxxxxxxxx>");
> +MODULE_DESCRIPTION("A Greybus driver for BeaglePlay");
Best regards,
Krzysztof
