Jassi Brar <[email protected]> writes: > On Wed, Apr 29, 2015 at 1:19 AM, Eric Anholt <[email protected]> wrote: >> Jassi Brar <[email protected]> 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.
signature.asc
Description: PGP signature
