[PATCH 2/3 v5] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

Jassi Brar <jassisinghbrar at gmail.com> writes:

> On Wed, Apr 29, 2015 at 1:19 AM, Eric Anholt <eric at anholt.net> wrote:
>> Jassi Brar <jassisinghbrar at gmail.com> writes:
>>
>>>> +
>>>> +struct bcm2835_mbox {
>>>> +       struct device *dev;
>>>> +       void __iomem *regs;
>>>> +       spinlock_t lock;
>>>> +       bool started;
>>>> +       struct mbox_controller controller;
>>>> +};
>>>> +
>>>> +struct bcm2835_mbox *mbox;
>>>> +
>>> Bad omen. You assume any platform will ever have just one instance of
>>> the mailbox controller. Which may come true, but still is a taboo to
>>> think of.
>>
>> On the other hand, when I've submitted to other subsystems people have
>> complained about how I have these extra lookups/container_ofs, instead
>> of just storing the obviously-only-one-of-them thing in a global.
>>
>> I think a global makes definite sense for this driver.
>>
> 0) Why is 'mbox' not even static?

Typo.

> 1) It makes sense for system wide entities like stack/protocol
> instance. But this is _one_ controller instance with _one_ mailbox
> instance. You think any platform will ever only need one mailbox or
> use two classes of controllers?

Yes, I think there will only ever be one or zero instances of this
driver.  I give it 10:1 odds.

>>>> +       if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) {
>>>> +               ret = -EBUSY;
>>>> +               goto end;
>>>> +       }
>>>>
>>> This check is unnecessary too. API would have already called
>>> last_tx_done() already to make sure this never hits.
>>
>> Dropped.
>>
> I see it's not yet.

Misread this while going through the ->stopped removals.  Fixed now.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150429/bc4157e5/attachment.sig>