On 17/06/2020 13:45, Michael Tretter wrote:
> Add a send/notify abstraction for the mailbox and separate the message
> handling in the driver from the code to read and write message to the
> mailbox.
>
> This untangles how mails are written into the MCU's SRAM and signaled to
> the MCU from the protocol between the driver and the firmware.
>
> Signed-off-by: Michael Tretter <m.tretter@xxxxxxxxxxxxxx>
> ---
> .../staging/media/allegro-dvt/allegro-core.c | 150 +++++++++++-------
> 1 file changed, 92 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
> index 70f133a842dd..447b15cc235c 100644
> --- a/drivers/staging/media/allegro-dvt/allegro-core.c
> +++ b/drivers/staging/media/allegro-dvt/allegro-core.c
> @@ -105,9 +105,11 @@ struct allegro_buffer {
> struct list_head head;
> };
>
> +struct allegro_dev;
> struct allegro_channel;
>
> struct allegro_mbox {
> + struct allegro_dev *dev;
> unsigned int head;
> unsigned int tail;
> unsigned int data;
> @@ -134,8 +136,8 @@ struct allegro_dev {
> struct completion init_complete;
>
> /* The mailbox interface */
> - struct allegro_mbox mbox_command;
> - struct allegro_mbox mbox_status;
> + struct allegro_mbox *mbox_command;
> + struct allegro_mbox *mbox_status;
>
> /*
> * The downstream driver limits the users to 64 users, thus I can use
> @@ -583,12 +585,20 @@ static void allegro_free_buffer(struct allegro_dev *dev,
> * Mailbox interface to send messages to the MCU.
> */
>
> -static int allegro_mbox_init(struct allegro_dev *dev,
> - struct allegro_mbox *mbox,
> - unsigned int base, size_t size)
> +static void allegro_mcu_interrupt(struct allegro_dev *dev);
> +static void allegro_handle_message(struct allegro_dev *dev,
> + union mcu_msg_response *msg);
> +
> +static struct allegro_mbox *allegro_mbox_init(struct allegro_dev *dev,
> + unsigned int base, size_t size)
> {
> + struct allegro_mbox *mbox;
> +
> + mbox = devm_kmalloc(&dev->plat_dev->dev, sizeof(*mbox), GFP_KERNEL);
> if (!mbox)
> - return -EINVAL;
> + return ERR_PTR(-ENOMEM);
> +
> + mbox->dev = dev;
>
> mbox->head = base;
> mbox->tail = base + 0x4;
> @@ -599,7 +609,7 @@ static int allegro_mbox_init(struct allegro_dev *dev,
> regmap_write(dev->sram, mbox->head, 0);
> regmap_write(dev->sram, mbox->tail, 0);
>
> - return 0;
> + return mbox;
> }
>
> static int allegro_mbox_write(struct allegro_dev *dev,
> @@ -713,9 +723,55 @@ static ssize_t allegro_mbox_read(struct allegro_dev *dev,
> return size;
> }
>
> -static void allegro_mcu_interrupt(struct allegro_dev *dev)
> +/**
> + * allegro_mbox_send() - Send a message via the mailbox
> + * @mbox: the mailbox which is used to send the message
> + * @msg: the message to send
> + */
> +static int allegro_mbox_send(struct allegro_mbox *mbox, void *msg)
> {
> - regmap_write(dev->regmap, AL5_MCU_INTERRUPT, BIT(0));
> + struct allegro_dev *dev = mbox->dev;
> + struct mcu_msg_header *header = msg;
> + ssize_t size = sizeof(*header) + header->length;
> + int err;
> +
> + err = allegro_mbox_write(dev, mbox, msg, size);
> + if (err)
> + goto out;
> +
> + allegro_mcu_interrupt(dev);
> +
> +out:
> + return err;
> +}
> +
> +/**
> + * allegro_mbox_notify() - Notify the mailbox about a new message
> + * @mbox: The allegro_mbox to notify
> + */
> +static int allegro_mbox_notify(struct allegro_mbox *mbox)
> +{
> + struct allegro_dev *dev = mbox->dev;
> + union mcu_msg_response *msg;
> + ssize_t size;
> + int err;
Shouldn't this be 'err = 0;'?
smatch gives me an error for this:
media-git/drivers/staging/media/allegro-dvt/allegro-core.c:786 allegro_mbox_notify() error: uninitialized symbol 'err'.
Regards,
Hans
> +
> + msg = kmalloc(sizeof(*msg), GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
> +
> + size = allegro_mbox_read(dev, mbox, msg, sizeof(*msg));
> + if (size < 0) {
> + err = size;
> + goto out;
> + }
> +
> + allegro_handle_message(dev, msg);
> +
> +out:
> + kfree(msg);
> +
> + return err;
> }
>
> static void allegro_mcu_send_init(struct allegro_dev *dev,
> @@ -736,8 +792,7 @@ static void allegro_mcu_send_init(struct allegro_dev *dev,
> msg.l2_cache[1] = -1;
> msg.l2_cache[2] = -1;
>
> - allegro_mbox_write(dev, &dev->mbox_command, &msg, sizeof(msg));
> - allegro_mcu_interrupt(dev);
> + allegro_mbox_send(dev->mbox_command, &msg);
> }
>
> static u32 v4l2_pixelformat_to_mcu_format(u32 pixelformat)
> @@ -946,8 +1001,7 @@ static int allegro_mcu_send_create_channel(struct allegro_dev *dev,
>
> fill_create_channel_param(channel, &msg.param);
>
> - allegro_mbox_write(dev, &dev->mbox_command, &msg, sizeof(msg));
> - allegro_mcu_interrupt(dev);
> + allegro_mbox_send(dev->mbox_command, &msg);
>
> return 0;
> }
> @@ -964,8 +1018,7 @@ static int allegro_mcu_send_destroy_channel(struct allegro_dev *dev,
>
> msg.channel_id = channel->mcu_channel_id;
>
> - allegro_mbox_write(dev, &dev->mbox_command, &msg, sizeof(msg));
> - allegro_mcu_interrupt(dev);
> + allegro_mbox_send(dev->mbox_command, &msg);
>
> return 0;
> }
> @@ -991,8 +1044,7 @@ static int allegro_mcu_send_put_stream_buffer(struct allegro_dev *dev,
> /* copied to mcu_msg_encode_frame_response */
> msg.stream_id = stream_id;
>
> - allegro_mbox_write(dev, &dev->mbox_command, &msg, sizeof(msg));
> - allegro_mcu_interrupt(dev);
> + allegro_mbox_send(dev->mbox_command, &msg);
>
> return 0;
> }
> @@ -1021,8 +1073,7 @@ static int allegro_mcu_send_encode_frame(struct allegro_dev *dev,
> msg.ep2 = 0x0;
> msg.ep2_v = to_mcu_addr(dev, msg.ep2);
>
> - allegro_mbox_write(dev, &dev->mbox_command, &msg, sizeof(msg));
> - allegro_mcu_interrupt(dev);
> + allegro_mbox_send(dev->mbox_command, &msg);
>
> return 0;
> }
> @@ -1084,12 +1135,8 @@ static int allegro_mcu_push_buffer_internal(struct allegro_channel *channel,
> buffer++;
> }
>
> - err = allegro_mbox_write(dev, &dev->mbox_command, msg, size);
> - if (err)
> - goto out;
> - allegro_mcu_interrupt(dev);
> + err = allegro_mbox_send(dev->mbox_command, msg);
>
> -out:
> kfree(msg);
> return err;
> }
> @@ -1681,51 +1728,28 @@ allegro_handle_encode_frame(struct allegro_dev *dev,
> return 0;
> }
>
> -static int allegro_receive_message(struct allegro_dev *dev)
> +static void allegro_handle_message(struct allegro_dev *dev,
> + union mcu_msg_response *msg)
> {
> - union mcu_msg_response *msg;
> - ssize_t size;
> - int err = 0;
> -
> - msg = kmalloc(sizeof(*msg), GFP_KERNEL);
> - if (!msg)
> - return -ENOMEM;
> -
> - size = allegro_mbox_read(dev, &dev->mbox_status, msg, sizeof(*msg));
> - if (size < sizeof(msg->header)) {
> - v4l2_err(&dev->v4l2_dev,
> - "invalid mbox message (%zd): must be at least %zu\n",
> - size, sizeof(msg->header));
> - err = -EINVAL;
> - goto out;
> - }
> -
> switch (msg->header.type) {
> case MCU_MSG_TYPE_INIT:
> - err = allegro_handle_init(dev, &msg->init);
> + allegro_handle_init(dev, &msg->init);
> break;
> case MCU_MSG_TYPE_CREATE_CHANNEL:
> - err = allegro_handle_create_channel(dev, &msg->create_channel);
> + allegro_handle_create_channel(dev, &msg->create_channel);
> break;
> case MCU_MSG_TYPE_DESTROY_CHANNEL:
> - err = allegro_handle_destroy_channel(dev,
> - &msg->destroy_channel);
> + allegro_handle_destroy_channel(dev, &msg->destroy_channel);
> break;
> case MCU_MSG_TYPE_ENCODE_FRAME:
> - err = allegro_handle_encode_frame(dev, &msg->encode_frame);
> + allegro_handle_encode_frame(dev, &msg->encode_frame);
> break;
> default:
> v4l2_warn(&dev->v4l2_dev,
> "%s: unknown message %s\n",
> __func__, msg_type_name(msg->header.type));
> - err = -EINVAL;
> break;
> }
> -
> -out:
> - kfree(msg);
> -
> - return err;
> }
>
> static irqreturn_t allegro_hardirq(int irq, void *data)
> @@ -1746,7 +1770,7 @@ static irqreturn_t allegro_irq_thread(int irq, void *data)
> {
> struct allegro_dev *dev = data;
>
> - allegro_receive_message(dev);
> + allegro_mbox_notify(dev->mbox_status);
>
> return IRQ_HANDLED;
> }
> @@ -1895,6 +1919,11 @@ static int allegro_mcu_reset(struct allegro_dev *dev)
> return allegro_mcu_wait_for_sleep(dev);
> }
>
> +static void allegro_mcu_interrupt(struct allegro_dev *dev)
> +{
> + regmap_write(dev->regmap, AL5_MCU_INTERRUPT, BIT(0));
> +}
> +
> static void allegro_destroy_channel(struct allegro_channel *channel)
> {
> struct allegro_dev *dev = channel->dev;
> @@ -2887,10 +2916,15 @@ static int allegro_mcu_hw_init(struct allegro_dev *dev,
> {
> int err;
>
> - allegro_mbox_init(dev, &dev->mbox_command,
> - info->mailbox_cmd, info->mailbox_size);
> - allegro_mbox_init(dev, &dev->mbox_status,
> - info->mailbox_status, info->mailbox_size);
> + dev->mbox_command = allegro_mbox_init(dev, info->mailbox_cmd,
> + info->mailbox_size);
> + dev->mbox_status = allegro_mbox_init(dev, info->mailbox_status,
> + info->mailbox_size);
> + if (!dev->mbox_command || !dev->mbox_status) {
> + v4l2_err(&dev->v4l2_dev,
> + "failed to initialize mailboxes\n");
> + return -EIO;
> + }
>
> allegro_mcu_enable_interrupts(dev);
>
>
