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 CC1352co-processor acts as Greybus SVC.
Fix indentation.
To compile this code as a module, chose M here: the modulewill be called gb-beagleplay.koconfig 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