BeagleConnect is both a technology concept and a line of board designs that implement the technology. Born from Greybus, a mechanism for dynamically extending a Linux system with embedded peripherals, BeagleConnect adds two key elements: a 6LoWPAN transport and mikroBUS manifests. The 6LoWPAN transport provides for arbitrary connections, including the IEEE802.15.4g long-range wireless transport supported between BeaglePlay and BeagleConnect Freedom (the first BeagleConnect board design). The mikroBUS manifests provide for rapid prototyping and low-node-count production with sensor boards following the mikroBUS freely-licensable embedded bus standard such that existing Linux drivers can be loaded upon Greybus discovery of the nodes.
This patch set provides the Linux-side hooks required for the 6LoWPAN transport for BeagleConnect on BeaglePlay. Also adds required devicetree additions.
Tested over `next-20230825`.
Link: https://programmershideaway.xyz/tags/gsoc23/ GSoC23 Blog Link: https://git.beagleboard.org/gsoc/greybus/cc1352-firmware Zephyr App Link: https://github.com/Ayush1325/linux/tree/gb-beagleplay GitHub Branch Link: https://lists.linaro.org/archives/list/greybus-dev@lists.linaro.org/thread/G... Patch v4 Link: https://docs.beagleboard.org/latest/boards/beagleconnect/index.html BeagleConnect Link: https://docs.beagleboard.org/latest/boards/beagleplay/index.html BeaglePlay Link: https://github.com/Ayush1325/linux/tree/gb-beagleplay Github Repo
Changes in Patch v5 v4 -> v5: - Move DT Bindings to net - Rename compatible to `beagle,play-cc1352` - Expose CC1352 as MCU - Remove redundant tracing messages - Fix memory leaks
v3 -> v4: - Add DT Bindings - Reorder commits - Improve commit messages
v2 -> v3: - Move gb-beagleplay out of staging
v1 -> v2: - Combine the driver into a single file - Remove redundant code - Fix Checkpatch complaints - Other suggested changes
Ayush Singh (3): dt-bindings: Add beaglecc1352 greybus: Add BeaglePlay Linux Driver dts: ti: k3-am625-beagleplay: Add beaglecc1352
.../bindings/net/beagle,play-cc1352.yaml | 25 + MAINTAINERS | 7 + .../arm64/boot/dts/ti/k3-am625-beagleplay.dts | 4 + drivers/greybus/Kconfig | 10 + drivers/greybus/Makefile | 4 +- drivers/greybus/gb-beagleplay.c | 526 ++++++++++++++++++ 6 files changed, 574 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/beagle,play-cc1352.yaml create mode 100644 drivers/greybus/gb-beagleplay.c
base-commit: 6269320850097903b30be8f07a5c61d9f7592393
Adds DT bindings for BeaglePlay CC1352 co-processor.
BeaglePlay has a CC1352 co-processor connected to the main AM62 (running Linux) over UART. In the BeagleConnect Technology, CC1352 is responsible for handling 6LoWPAN communication with beagleconnect freedom nodes as well as their discovery.
It is used by gb-beagleplay greybus driver.
Signed-off-by: Ayush Singh ayushdevel1325@gmail.com --- .../bindings/net/beagle,play-cc1352.yaml | 25 +++++++++++++++++++ MAINTAINERS | 6 +++++ 2 files changed, 31 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/beagle,play-cc1352.yaml
diff --git a/Documentation/devicetree/bindings/net/beagle,play-cc1352.yaml b/Documentation/devicetree/bindings/net/beagle,play-cc1352.yaml new file mode 100644 index 000000000000..f8536d1a6765 --- /dev/null +++ b/Documentation/devicetree/bindings/net/beagle,play-cc1352.yaml @@ -0,0 +1,25 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/beagle,play-cc1352.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: BeaglePlay CC1352 co-processor + +maintainers: + - Ayush Singh ayushdevel1325@gmail.com + +properties: + compatible: + const: beagle,play-cc1352 + +required: + - compatible + +additionalProperties: false + +examples: + - | + mcu { + compatible = "beagle,play-cc1352"; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 37b9626ee654..9d1b49a6dfad 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8969,6 +8969,12 @@ F: drivers/staging/greybus/sdio.c F: drivers/staging/greybus/spi.c F: drivers/staging/greybus/spilib.c
+GREYBUS BEAGLEPLAY DRIVERS +M: Ayush Singh ayushdevel1325@gmail.com +L: greybus-dev@lists.linaro.org (moderated for non-subscribers) +S: Maintained +F: Documentation/devicetree/bindings/serial/beaglecc1352.yaml + GREYBUS SUBSYSTEM M: Johan Hovold johan@kernel.org M: Alex Elder elder@kernel.org
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.
gb-beagleplay directly communicates with CC1352 (running Zephyr application). Thus, it simplifies the complete beagleconnect setup eliminating user-space GBridge.
This driver is responsible for the following: - Start SVC (CC1352) on driver load. - Send/Receive Greybus messages to/from CC1352 using HDLC over UART. - Print Logs from CC1352. - Stop SVC (CC1352) on driver load.
Signed-off-by: Ayush Singh ayushdevel1325@gmail.com --- MAINTAINERS | 1 + drivers/greybus/Kconfig | 10 + drivers/greybus/Makefile | 4 +- drivers/greybus/gb-beagleplay.c | 526 ++++++++++++++++++++++++++++++++ 4 files changed, 539 insertions(+), 2 deletions(-) create mode 100644 drivers/greybus/gb-beagleplay.c
diff --git a/MAINTAINERS b/MAINTAINERS index 9d1b49a6dfad..3cbf2c87fb14 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8974,6 +8974,7 @@ M: Ayush Singh ayushdevel1325@gmail.com L: greybus-dev@lists.linaro.org (moderated for non-subscribers) S: Maintained F: Documentation/devicetree/bindings/serial/beaglecc1352.yaml +F: drivers/greybus/gb-beagleplay.c
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. + + 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 - - 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 + +/* + * 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 + */ +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 */ + 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 void hdlc_handle_greybus_frame(struct gb_beagleplay *bg, u8 *buffer, + size_t buffer_len) +{ + u16 cport_id; + struct gb_operation_msg_hdr *hdr = + (struct gb_operation_msg_hdr *)buffer; + + memcpy(&cport_id, hdr->pad, sizeof(cport_id)); + + dev_dbg(&bg->serdev->dev, + "Greybus Operation %u of Type %X on CPort %u with Status %u Received", + hdr->operation_id, hdr->type, cport_id, hdr->result); + + greybus_data_rcvd(bg->gb_host_device, cport_id, buffer, buffer_len); +} + +static void hdlc_handle_dbg_frame(struct gb_beagleplay *bg, const char *buffer, + size_t buffer_len) +{ + dev_dbg(&bg->serdev->dev, "CC1352 Debug: %.*s", (int)buffer_len, + buffer); +} + +/* + * Consume HDLC Buffer. This function assumes that consumer lock has been acquired. + */ +static void hdlc_write(struct gb_beagleplay *bg) +{ + /* Start consuning HDLC data */ + int head = smp_load_acquire(&bg->tx_circ_buf.head); + int tail = bg->tx_circ_buf.tail; + int count = CIRC_CNT_TO_END(head, tail, TX_CIRC_BUF_SIZE); + const unsigned char *buf = &bg->tx_circ_buf.buf[tail]; + int written; + + if (count > 0) { + written = serdev_device_write_buf(bg->serdev, buf, count); + + /* Finish consuming HDLC data */ + smp_store_release(&bg->tx_circ_buf.tail, + (tail + written) & (TX_CIRC_BUF_SIZE - 1)); + } +} + +/* + * Queue HDLC data for sending. This function assumes that producer lock as been acquired. + */ +static void hdlc_append(struct gb_beagleplay *bg, u8 value) +{ + int tail, head = bg->tx_circ_buf.head; + + while (true) { + tail = READ_ONCE(bg->tx_circ_buf.tail); + + if (CIRC_SPACE(head, tail, TX_CIRC_BUF_SIZE) >= 1) { + bg->tx_circ_buf.buf[head] = value; + + /* Finish producing HDLC byte */ + smp_store_release(&bg->tx_circ_buf.head, + (head + 1) & (TX_CIRC_BUF_SIZE - 1)); + return; + } + dev_warn(&bg->serdev->dev, "Tx circ buf full"); + usleep_range(3000, 5000); + } +} + +static void hdlc_append_escaped(struct gb_beagleplay *bg, u8 value) +{ + if (value == HDLC_FRAME || value == HDLC_ESC) { + hdlc_append(bg, HDLC_ESC); + value ^= HDLC_XOR; + } + hdlc_append(bg, value); +} + +static void hdlc_append_tx_frame(struct gb_beagleplay *bg) +{ + bg->tx_crc = 0xFFFF; + hdlc_append(bg, HDLC_FRAME); +} + +static void hdlc_append_tx_u8(struct gb_beagleplay *bg, u8 value) +{ + bg->tx_crc = crc_ccitt(bg->tx_crc, &value, 1); + hdlc_append_escaped(bg, value); +} + +static void hdlc_append_tx_buffer(struct gb_beagleplay *bg, const u8 *buffer, + size_t len) +{ + size_t i; + + for (i = 0; i < len; i++) + hdlc_append_tx_u8(bg, buffer[i]); +} + +static void hdlc_append_tx_crc(struct gb_beagleplay *bg) +{ + bg->tx_crc ^= 0xffff; + hdlc_append_escaped(bg, bg->tx_crc & 0xff); + hdlc_append_escaped(bg, (bg->tx_crc >> 8) & 0xff); +} + +static void hdlc_handle_rx_frame(struct gb_beagleplay *bg) +{ + u8 address = bg->rx_buffer[0]; + char *buffer = &bg->rx_buffer[2]; + size_t buffer_len = bg->rx_buffer_len - 4; + + switch (address) { + case ADDRESS_DBG: + hdlc_handle_dbg_frame(bg, buffer, buffer_len); + break; + case ADDRESS_GREYBUS: + hdlc_handle_greybus_frame(bg, buffer, buffer_len); + break; + default: + dev_warn_ratelimited(&bg->serdev->dev, "Got Unknown Frame %u", + address); + } +} + +static void hdlc_transmit(struct work_struct *work) +{ + struct gb_beagleplay *bg = + container_of(work, struct gb_beagleplay, tx_work); + + spin_lock_bh(&bg->tx_consumer_lock); + hdlc_write(bg); + spin_unlock_bh(&bg->tx_consumer_lock); +} + +static void hdlc_send_async(struct gb_beagleplay *bg, u8 address, u8 control, + const struct hdlc_payload payloads[], size_t count) +{ + size_t i; + + /* + * HDLC_FRAME + * 0 address : 0x01 + * 1 control : 0x03 + * contents + * x/y crc + * HDLC_FRAME + */ + + spin_lock(&bg->tx_producer_lock); + + hdlc_append_tx_frame(bg); + hdlc_append_tx_u8(bg, address); + hdlc_append_tx_u8(bg, control); + for (i = 0; i < count; ++i) { + hdlc_append_tx_buffer(bg, payloads[i].payload, + payloads[i].length); + } + hdlc_append_tx_crc(bg); + hdlc_append_tx_frame(bg); + + spin_unlock(&bg->tx_producer_lock); + + schedule_work(&bg->tx_work); +} + +static void hdlc_send_s_frame_ack(struct gb_beagleplay *bg) +{ + hdlc_send_async(bg, bg->rx_buffer[0], (bg->rx_buffer[1] >> 1) & 0x7, + NULL, 0); +} + +static int hdlc_rx(struct gb_beagleplay *bg, const u8 *data, size_t count) +{ + u16 crc_check; + size_t i; + u8 c, ctrl; + + for (i = 0; i < count; ++i) { + c = data[i]; + + switch (c) { + case HDLC_FRAME: + if (bg->rx_buffer_len) { + crc_check = crc_ccitt(0xffff, bg->rx_buffer, + bg->rx_buffer_len); + if (crc_check == 0xf0b8) { + ctrl = bg->rx_buffer[1]; + if ((ctrl & 1) == 0) { + /* I-Frame, send S-Frame ACK */ + hdlc_send_s_frame_ack(bg); + } + hdlc_handle_rx_frame(bg); + } else { + dev_warn_ratelimited( + &bg->serdev->dev, + "CRC Failed from %02x: 0x%04x", + bg->rx_buffer[0], crc_check); + } + } + bg->rx_buffer_len = 0; + break; + case HDLC_ESC: + bg->rx_in_esc = 1; + break; + default: + if (bg->rx_in_esc) { + c ^= 0x20; + bg->rx_in_esc = 0; + } + + if (bg->rx_buffer_len < MAX_RX_HDLC) { + bg->rx_buffer[bg->rx_buffer_len] = c; + bg->rx_buffer_len++; + } else { + dev_err_ratelimited(&bg->serdev->dev, + "RX Buffer Overflow"); + bg->rx_buffer_len = 0; + } + } + } + + return count; +} + +static int hdlc_init(struct gb_beagleplay *bg) +{ + INIT_WORK(&bg->tx_work, hdlc_transmit); + spin_lock_init(&bg->tx_producer_lock); + spin_lock_init(&bg->tx_consumer_lock); + bg->tx_circ_buf.head = 0; + bg->tx_circ_buf.tail = 0; + + bg->tx_circ_buf.buf = + devm_kmalloc(&bg->serdev->dev, TX_CIRC_BUF_SIZE, GFP_KERNEL); + if (!bg->tx_circ_buf.buf) + return -ENOMEM; + + bg->rx_buffer_len = 0; + bg->rx_in_esc = 0; + + return 0; +} + +static void hdlc_deinit(struct gb_beagleplay *bg) +{ + flush_work(&bg->tx_work); +} + +static int gb_beagleplay_tty_receive(struct serdev_device *serdev, + const unsigned char *data, size_t count) +{ + struct gb_beagleplay *bg = serdev_device_get_drvdata(serdev); + + return hdlc_rx(bg, data, count); +} + +static void beagleplay_greybus_tty_wakeup(struct serdev_device *serdev) +{ + struct gb_beagleplay *bg = serdev_device_get_drvdata(serdev); + + schedule_work(&bg->tx_work); +} + +static struct serdev_device_ops gb_beagleplay_ops = { + .receive_buf = gb_beagleplay_tty_receive, + .write_wakeup = beagleplay_greybus_tty_wakeup, +}; + +static int gb_message_send(struct gb_host_device *hd, u16 cport_id, + struct gb_message *msg, gfp_t gfp_mask) +{ + struct gb_beagleplay *bg = dev_get_drvdata(&hd->dev); + struct hdlc_payload payloads[2]; + + dev_dbg(&hd->dev, + "Sending Greybus message with Operation %u, Type: %X on Cport %u", + msg->header->operation_id, msg->header->type, cport_id); + + if (msg->header->size > RX_HDLC_PAYLOAD) { + dev_err(&hd->dev, "Greybus message too big"); + return -E2BIG; + } + + memcpy(msg->header->pad, &cport_id, sizeof(cport_id)); + + payloads[0].payload = msg->header; + payloads[0].length = sizeof(*msg->header); + payloads[1].payload = msg->payload; + payloads[1].length = msg->payload_size; + + hdlc_send_async(bg, ADDRESS_GREYBUS, 0x03, payloads, 2); + greybus_message_sent(bg->gb_host_device, msg, 0); + + return 0; +} + +static void gb_message_cancel(struct gb_message *message) +{ +} + +static struct gb_hd_driver gb_hdlc_driver = { .message_send = gb_message_send, + .message_cancel = + gb_message_cancel }; + +static void gb_beagleplay_start_svc(struct gb_beagleplay *bg) +{ + const u8 command = CONTROL_SVC_START; + const struct hdlc_payload payload = { .length = 1, + .payload = (void *)&command }; + + hdlc_send_async(bg, ADDRESS_CONTROL, 0x03, &payload, 1); +} + +static void gb_beagleplay_stop_svc(struct gb_beagleplay *bg) +{ + const u8 command = CONTROL_SVC_STOP; + const struct hdlc_payload payload = { .length = 1, + .payload = (void *)&command }; + + hdlc_send_async(bg, ADDRESS_CONTROL, 0x03, &payload, 1); +} + +static void gb_greybus_deinit(struct gb_beagleplay *bg) +{ + gb_hd_del(bg->gb_host_device); + gb_hd_put(bg->gb_host_device); +} + +static int gb_greybus_init(struct gb_beagleplay *bg) +{ + int ret; + + bg->gb_host_device = gb_hd_create(&gb_hdlc_driver, &bg->serdev->dev, + TX_CIRC_BUF_SIZE, + BEAGLEPLAY_GB_MAX_CPORTS); + if (IS_ERR(bg->gb_host_device)) { + return dev_err_probe(&bg->serdev->dev, + PTR_ERR(bg->gb_host_device), + "Unable to create Greybus Host Device"); + } + ret = gb_hd_add(bg->gb_host_device); + if (ret) { + dev_err(&bg->serdev->dev, "Failed to add Greybus Host Device"); + goto free_gb_hd; + } + dev_set_drvdata(&bg->gb_host_device->dev, bg); + + return 0; + +free_gb_hd: + gb_greybus_deinit(bg); + return ret; +} + +static void gb_serdev_deinit(struct gb_beagleplay *bg) +{ + serdev_device_close(bg->serdev); +} + +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"); + } + 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); + + 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, + }, +}; + +module_serdev_device_driver(gb_beagleplay_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Ayush Singh ayushdevel1325@gmail.com"); +MODULE_DESCRIPTION("A Greybus driver for BeaglePlay");
The BeaglePlay board by BeagleBoard.org has a CC1352 co-processor. This co-processor is connected to the main AM62 (running Linux) over UART. In the BeagleConnect Technology, CC1352 is responsible for handling 6LoWPAN communication with beagleconnect freedom nodes as well as their discovery.
This mcu is used by gb-beagleplay, a Greybus driver for BeaglePlay.
Signed-off-by: Ayush Singh ayushdevel1325@gmail.com --- arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts index 7cfdf562b53b..5d46f907468f 100644 --- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts +++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts @@ -870,6 +870,10 @@ &main_uart6 { pinctrl-names = "default"; pinctrl-0 = <&wifi_debug_uart_pins_default>; status = "okay"; + + mcu { + compatible = "beagle,play-cc1352"; + }; };
&dss {
On 24/09/2023 13:36, Ayush Singh wrote:
Adds DT bindings for BeaglePlay CC1352 co-processor.
I gave you the link to the exact wording you should use. "Add", not "Adds". The latter is not a correct English sentences, I believe.
BeaglePlay has a CC1352 co-processor connected to the main AM62 (running Linux) over UART. In the BeagleConnect Technology, CC1352 is responsible for handling 6LoWPAN communication with beagleconnect freedom nodes as well as their discovery.
It is used by gb-beagleplay greybus driver.
Signed-off-by: Ayush Singh ayushdevel1325@gmail.com
.../bindings/net/beagle,play-cc1352.yaml | 25 +++++++++++++++++++ MAINTAINERS | 6 +++++ 2 files changed, 31 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/beagle,play-cc1352.yaml
diff --git a/Documentation/devicetree/bindings/net/beagle,play-cc1352.yaml b/Documentation/devicetree/bindings/net/beagle,play-cc1352.yaml new file mode 100644 index 000000000000..f8536d1a6765 --- /dev/null +++ b/Documentation/devicetree/bindings/net/beagle,play-cc1352.yaml @@ -0,0 +1,25 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/beagle,play-cc1352.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#
+title: BeaglePlay CC1352 co-processor
So this is "Texas Instruments Simplelink CC1352P7 wireless MCU"? Since you do not have any fixed feature and run general-purpose OS, then this should be rather compatible matching actual hardware (so ti,cc1352p7).
+maintainers:
- Ayush Singh ayushdevel1325@gmail.com
+properties:
- compatible:
- const: beagle,play-cc1352
+required:
- compatible
Still no resources? I asked about it last time and you did not answer anything.
+additionalProperties: false
+examples:
- |
- mcu {
compatible = "beagle,play-cc1352";- };
diff --git a/MAINTAINERS b/MAINTAINERS index 37b9626ee654..9d1b49a6dfad 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8969,6 +8969,12 @@ F: drivers/staging/greybus/sdio.c F: drivers/staging/greybus/spi.c F: drivers/staging/greybus/spilib.c +GREYBUS BEAGLEPLAY DRIVERS +M: Ayush Singh ayushdevel1325@gmail.com +L: greybus-dev@lists.linaro.org (moderated for non-subscribers) +S: Maintained +F: Documentation/devicetree/bindings/serial/beaglecc1352.yaml
That's not a correct path.
Best regards, Krzysztof
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
So this is "Texas Instruments Simplelink CC1352P7 wireless MCU"? Since you do not have any fixed feature and run general-purpose OS, then this should be rather compatible matching actual hardware (so ti,cc1352p7).
Yes, it is "Texas Instruments Simplelink CC1352P7 wireless MCU". Since the docs suggest that all functionality of the device should be mentioned in the bindings rather than just what is being used, I suppose all the other properties can be defined as optional? The bindings for this device do exist in Zephyr, so I guess that can be re-purposed for Linux kernel. However, I think in that case it should be moved into `soc` instead of `net`?
+maintainers:
- Ayush Singh ayushdevel1325@gmail.com
+properties:
- compatible:
- const: beagle,play-cc1352
+required:
- compatible
Still no resources? I asked about it last time and you did not answer anything.
Sorry, I missed that. By resources, I think you mean pins, peripherals, etc right?
Ayush Singh
To compile this code as a module, chose M here: the modulewill 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.
You are referring to the removal of last 2 newlines, right? In that case, I will fix it.
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.
Thanks, will do that.
- */
+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.
I was getting some errors in checkpatch without those. I guess they will go away if I am using kerneldoc?
- 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.
So I do not actually get any errors here in checkpatch. I am running the follwing:
`scripts/checkpatch.pl --codespell --strict patch/*`
I only get a warning in coverletter due to that path of DT bindings being more than 75 character long and ` Lines should not end with a '('`.
- 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?
Again, it doesn't seem to for me. Am I missing some environment variables or options? Or maybe something wrong with my editor config (neovim)?
Yours Sincerely
Ayush Singh
On 24/09/2023 14:22, Ayush Singh wrote:
So this is "Texas Instruments Simplelink CC1352P7 wireless MCU"? Since you do not have any fixed feature and run general-purpose OS, then this should be rather compatible matching actual hardware (so ti,cc1352p7).
Yes, it is "Texas Instruments Simplelink CC1352P7 wireless MCU". Since the docs suggest that all functionality of the device should be mentioned in the bindings rather than just what is being used, I suppose all the other properties can be defined as optional? The bindings for this device do exist in Zephyr, so I guess that can be re-purposed for Linux kernel. However, I think in that case it should be moved into `soc` instead of `net`?
Zephyr bindings might be entirely different, because they are for the case of running Zephyr OS on this processor.
I still do not fully understand whether you describe here generic MCU used for Greybus or some specific firmware with fixed features. Looks like the first, thus my suggestion about compatible. The location could be arm.
+maintainers:
- Ayush Singh ayushdevel1325@gmail.com
+properties:
- compatible:
- const: beagle,play-cc1352
+required:
- compatible
Still no resources? I asked about it last time and you did not answer anything.
Sorry, I missed that. By resources, I think you mean pins, peripherals, etc right?
By resources I mean whatever is needed to power it up and operate, e.g. clocks, supplies, reset lines.
Best regards, Krzysztof
On 24/09/2023 14:32, Ayush Singh wrote:
To compile this code as a module, chose M here: the modulewill 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.
You are referring to the removal of last 2 newlines, right? In that case, I will fix it.
Yes.
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.
Thanks, will do that.
- */
+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.
I was getting some errors in checkpatch without those. I guess they will go away if I am using kerneldoc?
Check. Anyway your comment does not solve checkpatch problem. Again you duplicate the name of member - tx producer lock is a HLDC producer lock. That's nothing new than name of variable.
- 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.
So I do not actually get any errors here in checkpatch. I am running the follwing:
`scripts/checkpatch.pl --codespell --strict patch/*`
I only get a warning in coverletter due to that path of DT bindings being more than 75 character long and ` Lines should not end with a '('`.
- 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?
Again, it doesn't seem to for me. Am I missing some environment variables or options? Or maybe something wrong with my editor config (neovim)?
You have spaces after tab, so how can this be properly aligned?
Best regards, Krzysztof
- 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.
So I do not actually get any errors here in checkpatch. I am running the follwing:
`scripts/checkpatch.pl --codespell --strict patch/*`
I only get a warning in coverletter due to that path of DT bindings being more than 75 character long and ` Lines should not end with a '('`.
- 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?
Again, it doesn't seem to for me. Am I missing some environment variables or options? Or maybe something wrong with my editor config (neovim)?
You have spaces after tab, so how can this be properly aligned?
Well, I will try to fix all of them. They might be remnants from when I was developing this out-of-tree. Still, it's strange for checkpatch to not complain. Will look into that as well if I get time.
Yours Sincerely,
Ayush Singh
- 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.
So I do not actually get any errors here in checkpatch. I am running the follwing:
`scripts/checkpatch.pl --codespell --strict patch/*`
I only get a warning in coverletter due to that path of DT bindings being more than 75 character long and ` Lines should not end with a '('`.
- 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?
Again, it doesn't seem to for me. Am I missing some environment variables or options? Or maybe something wrong with my editor config (neovim)?
You have spaces after tab, so how can this be properly aligned?
Best regards, Krzysztof
So I just wanted to confirm, but I think spaces after tab are fine for alignment, right? I found this (https://www.mail-archive.com/kernelnewbies@kernelnewbies.org/msg13354.html) message in mailing list stating that it is fine.
It seems clang-format adds spaces for alignment less than 8 spaces. And checkpatch doesn't seem to complain as long as spaces are used for alignment (not indentation).
Sincerely,
Ayush Singh
On 01/10/2023 20:13, Ayush Singh wrote:
+}; +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?
Again, it doesn't seem to for me. Am I missing some environment variables or options? Or maybe something wrong with my editor config (neovim)?
You have spaces after tab, so how can this be properly aligned?
Best regards, Krzysztof
So I just wanted to confirm, but I think spaces after tab are fine for alignment, right? I found this (https://www.mail-archive.com/kernelnewbies@kernelnewbies.org/msg13354.html) message in mailing list stating that it is fine.
Spaces after tab are fine *when needed* for proper alignment. You do not have proper alignment here, so just drop them. Please open any other existing driver (although not something old...).
Best regards, Krzysztof
-
Ayush Singh -
Krzysztof Kozlowski