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
