On Tue, Jun 30, 2020 at 09:36:27AM +0200, Michael Tretter wrote:
> On Wed, Jun 24, 2020 at 04:35:09PM +0200, Hans Verkuil wrote:
> > 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'.
>
> Yes, thank you. I just found out that I was using a really, really old version
> of smatch. Will send a v2.
allegro_mbox_notify() should be void and not return an error code at all. It
is called from the interrupt handler and if there is an error when reading
from the mailbox, we cannot do anything anyway.
Michael
>
> Michael
>
> >
> > 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);
> > >
> > >
> >
> >
