On Wed, Aug 23, 2023 at 10:25:18PM +0530, Ayush Singh wrote:
> Signed-off-by: Ayush Singh <ayushdevel1325@xxxxxxxxx>
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
_______________________________________________
greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx
To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx
|