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

[Ayush Singh <ayushdevel1325@xxxxxxxxx>]

> > +
> > +	  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.
> > 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?
> > +	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

_______________________________________________
greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx
To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx