On Wed, Aug 23, 2023 at 10:25:19PM +0530, Ayush Singh wrote:
> This file provides functions that allow sending and recieving async HDLC
> frames over any transport. Currently only tested with UART.
>
> I am not quite sure where these files should go so I have put them close
> to Beagleplay greybus driver for now.
This shouldn't be in a changelog text :)
You can put comments below the --- line.
And to answer this question, no, it probably shouldn't be here in
drivers/staging/ it should be in the "real" part of the kernel as it is
a real driver. drivers/staging/ is for stuff that still needs work to
do to get it out of that part of the kernel, do the work ahead of time
and then you don't have to mess with that at all.
>
> Signed-off-by: Ayush Singh <ayushdevel1325@xxxxxxxxx>
> ---
> drivers/staging/greybus/hdlc.c | 229 +++++++++++++++++++++++++++++++++
> drivers/staging/greybus/hdlc.h | 137 ++++++++++++++++++++
No need for a .h file for a simple .c file, just put it all together
into one file please.
> 2 files changed, 366 insertions(+)
> create mode 100644 drivers/staging/greybus/hdlc.c
> create mode 100644 drivers/staging/greybus/hdlc.h
>
> diff --git a/drivers/staging/greybus/hdlc.c b/drivers/staging/greybus/hdlc.c
> new file mode 100644
> index 000000000000..079d4c10e476
> --- /dev/null
> +++ b/drivers/staging/greybus/hdlc.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2023 Ayush Singh <ayushdevel1325@xxxxxxxxx>
> + */
> +
> +#include "hdlc.h"
> +#include <linux/device.h>
> +#include <linux/crc-ccitt.h>
> +#include <linux/serdev.h>
> +
> +static void hdlc_write_locked(struct hdlc_driver *drv)
> +{
> + // must be locked already
What must be locked? I don't understand this comment, sorry.
> + int head = smp_load_acquire(&drv->tx_circ_buf.head);
> + int tail = drv->tx_circ_buf.tail;
> + int count = CIRC_CNT_TO_END(head, tail, TX_CIRC_BUF_SIZE);
> + int written;
> +
> + if (count >= 1) {
> + written = drv->hdlc_send_frame_cb(dev_get_drvdata(drv->parent),
> + &drv->tx_circ_buf.buf[tail],
> + count);
> +
> + /* Finish consuming HDLC data */
> + smp_store_release(&drv->tx_circ_buf.tail,
> + (tail + written) & (TX_CIRC_BUF_SIZE - 1));
> + }
> +}
> +
> +static void hdlc_append(struct hdlc_driver *drv, u8 value)
> +{
> + // must be locked already
Again, I don't understand.
> + int head, tail;
> +
> + head = drv->tx_circ_buf.head;
> +
> + while (true) {
> + tail = READ_ONCE(drv->tx_circ_buf.tail);
> +
> + if (CIRC_SPACE(head, tail, TX_CIRC_BUF_SIZE) >= 1) {
> + drv->tx_circ_buf.buf[head] = value;
> +
> + /* Finish producing HDLC byte */
> + smp_store_release(&drv->tx_circ_buf.head,
> + (head + 1) & (TX_CIRC_BUF_SIZE - 1));
> + return;
> + }
> + dev_warn(drv->parent, "Tx circ buf full\n");
> + usleep_range(3000, 5000);
> + }
> +}
> +
> +static void hdlc_append_escaped(struct hdlc_driver *drv, u8 value)
> +{
> + if (value == HDLC_FRAME || value == HDLC_ESC) {
> + hdlc_append(drv, HDLC_ESC);
> + value ^= HDLC_XOR;
> + }
> + hdlc_append(drv, value);
> +}
> +
> +static void hdlc_append_tx_frame(struct hdlc_driver *drv)
> +{
> + drv->tx_crc = 0xFFFF;
> + hdlc_append(drv, HDLC_FRAME);
> +}
> +
> +static void hdlc_append_tx_u8(struct hdlc_driver *drv, u8 value)
> +{
> + drv->tx_crc = crc_ccitt(drv->tx_crc, &value, 1);
> + hdlc_append_escaped(drv, value);
> +}
> +
> +static void hdlc_append_tx_buffer(struct hdlc_driver *drv, const void *buffer,
> + size_t len)
> +{
> + size_t i;
> +
> + for (i = 0; i < len; i++)
> + hdlc_append_tx_u8(drv, ((u8 *)buffer)[i]);
> +}
> +
> +static void hdlc_append_tx_crc(struct hdlc_driver *drv)
> +{
> + drv->tx_crc ^= 0xffff;
> + hdlc_append_escaped(drv, drv->tx_crc & 0xff);
> + hdlc_append_escaped(drv, (drv->tx_crc >> 8) & 0xff);
> +}
> +
> +static void hdlc_handle_rx_frame(struct hdlc_driver *drv)
> +{
> + u8 address = drv->rx_buffer[0];
> + char *buf = &drv->rx_buffer[2];
> + size_t buf_len = drv->rx_buffer_len - 4;
> +
> + drv->hdlc_process_frame_cb(dev_get_drvdata(drv->parent), buf, buf_len,
> + address);
> +}
> +
> +static void hdlc_transmit(struct work_struct *work)
> +{
> + struct hdlc_driver *drv =
> + container_of(work, struct hdlc_driver, tx_work);
> +
> + spin_lock_bh(&drv->tx_consumer_lock);
> + hdlc_write_locked(drv);
> + spin_unlock_bh(&drv->tx_consumer_lock);
> +}
> +
> +static void hdlc_send_s_frame_ack(struct hdlc_driver *drv)
> +{
> + u8 address = drv->rx_buffer[0];
> +
> + hdlc_send_async(drv, 0, NULL, address, (drv->rx_buffer[1] >> 1) & 0x7);
> +}
> +
> +int hdlc_rx(struct hdlc_driver *drv, const unsigned char *data, size_t count)
Why is this a global function?
> +void hdlc_send_async(struct hdlc_driver *drv, u16 length, const void *buffer,
> + u8 address, u8 control)
> +{
Same here, why is this global?
Who calls these new functions? There seems to be something missing here
as if this is never called, there's no need for it?
confused,
greg k-h
_______________________________________________
greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx
To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx
|