[greybus-dev] Re: [PATCH 2/4] Add beagleplay greybus driver

On Wed, Aug 23, 2023 at 10:25:18PM +0530, Ayush Singh wrote:

Signed-off-by: Ayush Singh ayushdevel1325@gmail.com

I can't take patches without any changelog text at all (neither do you want me to.)

Writing the changelog is usually the hardest part. Take a look at the link my patch bot sent you and read that to see what to do.

---

.../greybus/beagleplay-greybus-driver.c | 264 ++++++++++++++++++

This is a really really really long name for a driver.

Why not beagle_gb.c?

.../greybus/beagleplay-greybus-driver.h | 28 ++

Again, no need for .h files for a simple .c file.

+#define BEAGLEPLAY_GREYBUS_DRV_VERSION "0.1.0"

driver versions make no sense in the kernel tree, just drop this.

+#define BEAGLEPLAY_GREYBUS_DRV_NAME "bcfgreybus"

KBUILD_MODNAME?

+#define BEAGLEPLAY_GB_MAX_CPORTS 32

Where does this number come from?

+static const struct of_device_id beagleplay_greybus_of_match[] = {

• {

• .compatible = "beagle,beagleplaygreybus",
  

Are you sure this will work? You need to get your dt changes properly reviewed first.

• },
• {},

+}; +MODULE_DEVICE_TABLE(of, beagleplay_greybus_of_match);

+static int beagleplay_greybus_serdev_write_locked(void *drvdata,

• 				  const unsigned char *buf,
  

• 				  size_t buf_len)
  

+{

• struct beagleplay_greybus *beagleplay_greybus;
• beagleplay_greybus = drvdata;
• return serdev_device_write_buf(beagleplay_greybus->serdev, buf,

• 		       buf_len);
  

+}

+static void +beagleplay_greybus_handle_greybus_frame(struct beagleplay_greybus *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(u16));

Did you run checkpatch on the code before sending it? Please do so.

• if (hdr->result) {

• dev_warn(
  

• 	&bg->serdev->dev,
  

• 	"Failed Greybus Operation %u of Type %X on CPort %u with Status %u",
  

• 	hdr->operation_id, hdr->type, cport_id, hdr->result);
  

• } else {

• dev_dbg(&bg->serdev->dev,
  

• 	"Successful Greybus Operation %u of Type %X on CPort %u",
  

• 	hdr->operation_id, hdr->type, cport_id);
  

• }
• greybus_data_rcvd(bg->gb_host_device, cport_id, buffer, buffer_len);

+}

+static void beagleplay_greybus_handle_dbg_frame(struct beagleplay_greybus *bg,

• 				const char *buffer,
  

• 				size_t buffer_len)
  

+{

• char buf[RX_HDLC_PAYLOAD];

Are you sure you can do all of that on the stack?

• memcpy(buf, buffer, buffer_len);
• buf[buffer_len] = 0;
• dev_dbg(&bg->serdev->dev, "CC1352 Debug: %s", buf);

Why are you using a stack buffer for a debug log?

And no need for prefixes on a debug message, that comes for free from the dynamic debug infrastructure.

and are you sure this buffer is a string?

+}

+static void beagleplay_greybus_handle_hdlc_rx_frame(void *drv_data, u8 *buffer,

• 				    size_t buffer_len,
  

• 				    uint8_t address)
  

+{

• struct beagleplay_greybus *beagleplay_greybus;
• beagleplay_greybus = drv_data;

This is usually just one line: struct beagleplay_greybus *beagleplay_greybus = drv_data;

but really, use less characters, how about: struct beagle_gb *beagle_gb = data;

or something like that.

• switch (address) {
• case ADDRESS_DBG:

• beagleplay_greybus_handle_dbg_frame(beagleplay_greybus, buffer,
  

• 				    buffer_len);
  

very long function names, you can make them smaller :)

• break;
  

• case ADDRESS_GREYBUS:

• beagleplay_greybus_handle_greybus_frame(beagleplay_greybus,
  

• 					buffer, buffer_len);
  

• break;
  

• default:

• dev_warn(&beagleplay_greybus->serdev->dev,
  

• 	 "Got Unknown Frame %u", address);
  

• }

+}

+static int beagleplay_greybus_tty_receive(struct serdev_device *serdev,

• 			  const unsigned char *data,
  

• 			  size_t count)
  

+{

• struct beagleplay_greybus *beagleplay_greybus;
• beagleplay_greybus = serdev_device_get_drvdata(serdev);
• return hdlc_rx(beagleplay_greybus->hdlc_drv, data, count);

+}

+static void beagleplay_greybus_tty_wakeup(struct serdev_device *serdev) +{

• struct beagleplay_greybus *beagleplay_greybus;
• beagleplay_greybus = serdev_device_get_drvdata(serdev);
• hdlc_tx_wakeup(beagleplay_greybus->hdlc_drv);

+}

+static struct serdev_device_ops beagleplay_greybus_ops = {

• .receive_buf = beagleplay_greybus_tty_receive,
• .write_wakeup = beagleplay_greybus_tty_wakeup,

+};

+static int gb_message_send(struct gb_host_device *hd, u16 cport_id,

• 	   struct gb_message *message, gfp_t gfp_mask)
  

+{

• struct beagleplay_greybus *beagleplay_greybus;
• char block_payload[HDLC_MAX_BLOCK_SIZE];
• dev_dbg(&hd->dev,

• "Sending Greybus message with Operation %u, Type: %X on Cport %u",
  

• message->header->operation_id, message->header->type, cport_id);
  

• if (message->header->size > HDLC_MAX_BLOCK_SIZE) {

• dev_err(&hd->dev, "Greybus message too big");
  

• return -1;
  

Please always use real error values, not made up negative numbers. -ETOBIG?

• }
• beagleplay_greybus = dev_get_drvdata(&hd->dev);
• memcpy(message->header->pad, &cport_id, sizeof(u16));
• memcpy(&block_payload, message->header,

•       sizeof(struct gb_operation_msg_hdr));
  

• memcpy(&block_payload[sizeof(struct gb_operation_msg_hdr)],

•       message->payload, message->payload_size);
  

• hdlc_send_async(beagleplay_greybus->hdlc_drv, message->header->size,

• 	&block_payload, ADDRESS_GREYBUS, 0x03);
  

• greybus_message_sent(beagleplay_greybus->gb_host_device, message, 0);
• return 0;

+}

+static void gb_message_cancel(struct gb_message *message) +{ +}

Why is an empty function needed? That feels wrong.

+static struct gb_hd_driver gb_hdlc_driver = { .message_send = gb_message_send,

• 			      .message_cancel =
  

• 				      gb_message_cancel };
  

Formatting can be fixed up here.

+static void beagleplay_greybus_start_svc(struct beagleplay_greybus *bg) +{

• const u8 command[1] = { CONTROL_SVC_START };

Are you sure this is allowed on the stack?

• hdlc_send_async(bg->hdlc_drv, sizeof(command), command, ADDRESS_CONTROL,

• 	0x03);
  

+}

+static void beagleplay_greybus_stop_svc(struct beagleplay_greybus *bg) +{

• const u8 command = CONTROL_SVC_STOP;
• hdlc_send_async(bg->hdlc_drv, 1, &command, ADDRESS_CONTROL, 0x03);

+}

+static int beagleplay_greybus_probe(struct serdev_device *serdev) +{

• struct beagleplay_greybus *bg;
• u32 speed = 115200;
• int ret = 0;
• bg = devm_kmalloc(&serdev->dev, sizeof(struct beagleplay_greybus),

• 	  GFP_KERNEL);
  

• bg->serdev = serdev;
• serdev_device_set_drvdata(serdev, bg);
• serdev_device_set_client_ops(serdev, &beagleplay_greybus_ops);
• ret = serdev_device_open(serdev);
• if (ret) {

• dev_err(&bg->serdev->dev, "Unable to Open Device");
  

• return ret;
  

• }
• speed = serdev_device_set_baudrate(serdev, speed);
• dev_info(&bg->serdev->dev, "Using baudrate %u\n", speed);
• serdev_device_set_flow_control(serdev, false);
• // Init HDLC
• bg->hdlc_drv =

• hdlc_init(&serdev->dev, beagleplay_greybus_serdev_write_locked,
  

• 	  beagleplay_greybus_handle_hdlc_rx_frame);
  

• if (!bg->hdlc_drv) {

• dev_err(&serdev->dev, "Failed to initialize HDLC");
  

• return -ENOMEM;
  

• }
• // Greybus setup
• bg->gb_host_device =

• gb_hd_create(&gb_hdlc_driver, &serdev->dev, TX_CIRC_BUF_SIZE,
  

• 	     BEAGLEPLAY_GB_MAX_CPORTS);
  

• if (IS_ERR(bg->gb_host_device)) {

• dev_err(&bg->serdev->dev,
  

• 	"Unable to create Greybus Host Device");
  

• return -1;
  

How about returning the error given to you?

• }
• ret = gb_hd_add(bg->gb_host_device);
• if (ret) {

• dev_err(&serdev->dev, "Failed to add Greybus Host Device");
  

• return ret;
  

Did you just leak memory?

Anyway, these are all tiny things, it's great to see this work happening, it's getting close!

greg k-h