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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Jassi Brar <jassisinghbrar@xxxxxxxxx> writes:

> On Wed, Apr 29, 2015 at 1:19 AM, Eric Anholt <eric@xxxxxxxxxx> wrote:
>> Jassi Brar <jassisinghbrar@xxxxxxxxx> 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.

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux