On 24/09/2023 14:32, Ayush Singh wrote: >>> + >>> + 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. > 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@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. > 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