On 30/06/2020 10:29, Michael Tretter wrote:
> 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.
Can you post a v2? This series is ready to be merged except for this issue.
Regards,
Hans
