Re: [PATCH 01/15] ARM: OMAP2+: mailbox: Add an API for flushing the FIFO

[Santosh Shilimkar <santosh.shilimkar@ti.com>]

On Sunday 04 November 2012 08:56 PM, Bedia, Vaibhav wrote:
> On Sat, Nov 03, 2012 at 21:33:47, Shilimkar, Santosh wrote:
> [...]
>
>>> +static int omap2_mbox_fifo_needs_flush(struct omap_mbox *mbox)
>>> +{
>>> +	struct omap_mbox2_fifo *fifo =
>>> +		&((struct omap_mbox2_priv *)mbox->priv)->tx_fifo;
>> type casting is generally avoided in linux code.
>
> I was just trying to be consistent with the rest of the mailbox FIFO
> related code :)
>
> Will see how to get rid of these globally in the next version.
>
>>> +	return mbox_read_reg(fifo->msg_stat);
>>> +}
>>> +
>>> +static mbox_msg_t omap2_mbox_fifo_readback(struct omap_mbox *mbox)
>>> +{
>>> +	struct omap_mbox2_fifo *fifo =
>>> +		&((struct omap_mbox2_priv *)mbox->priv)->tx_fifo;
>>> +	return (mbox_msg_t) mbox_read_reg(fifo->msg);
>> same here.
>
> Ok.
>
>>> +}
>>> +
>>>    /* Mailbox IRQ handle functions */
>>>    static void omap2_mbox_enable_irq(struct omap_mbox *mbox,
>>>    		omap_mbox_type_t irq)
>>> @@ -205,19 +219,21 @@ static void omap2_mbox_restore_ctx(struct omap_mbox *mbox)
>>>    }
>>>
>>>    static struct omap_mbox_ops omap2_mbox_ops = {
>>> -	.type		= OMAP_MBOX_TYPE2,
>>> -	.startup	= omap2_mbox_startup,
>>> -	.shutdown	= omap2_mbox_shutdown,
>>> -	.fifo_read	= omap2_mbox_fifo_read,
>>> -	.fifo_write	= omap2_mbox_fifo_write,
>>> -	.fifo_empty	= omap2_mbox_fifo_empty,
>>> -	.fifo_full	= omap2_mbox_fifo_full,
>>> -	.enable_irq	= omap2_mbox_enable_irq,
>>> -	.disable_irq	= omap2_mbox_disable_irq,
>>> -	.ack_irq	= omap2_mbox_ack_irq,
>>> -	.is_irq		= omap2_mbox_is_irq,
>>> -	.save_ctx	= omap2_mbox_save_ctx,
>>> -	.restore_ctx	= omap2_mbox_restore_ctx,
>>> +	.type			= OMAP_MBOX_TYPE2,
>>> +	.startup		= omap2_mbox_startup,
>>> +	.shutdown		= omap2_mbox_shutdown,
>>> +	.fifo_read		= omap2_mbox_fifo_read,
>>> +	.fifo_write		= omap2_mbox_fifo_write,
>>> +	.fifo_empty		= omap2_mbox_fifo_empty,
>>> +	.fifo_full		= omap2_mbox_fifo_full,
>>> +	.fifo_needs_flush	= omap2_mbox_fifo_needs_flush,
>>> +	.fifo_readback		= omap2_mbox_fifo_readback,
>>> +	.enable_irq		= omap2_mbox_enable_irq,
>>> +	.disable_irq		= omap2_mbox_disable_irq,
>>> +	.ack_irq		= omap2_mbox_ack_irq,
>>> +	.is_irq			= omap2_mbox_is_irq,
>>> +	.save_ctx		= omap2_mbox_save_ctx,
>>> +	.restore_ctx		= omap2_mbox_restore_ctx,
>> You should do the indentation fix in another patch.
>>
>
> Ok will split it up.
>
>>>    };
>>>
>>>    /*
>>> diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-omap/include/plat/mailbox.h
>>> index cc3921e..e136529 100644
>>> --- a/arch/arm/plat-omap/include/plat/mailbox.h
>>> +++ b/arch/arm/plat-omap/include/plat/mailbox.h
>>> @@ -29,6 +29,8 @@ struct omap_mbox_ops {
>>>    	void		(*fifo_write)(struct omap_mbox *mbox, mbox_msg_t msg);
>>>    	int		(*fifo_empty)(struct omap_mbox *mbox);
>>>    	int		(*fifo_full)(struct omap_mbox *mbox);
>>> +	int		(*fifo_needs_flush)(struct omap_mbox *mbox);
>>> +	mbox_msg_t	(*fifo_readback)(struct omap_mbox *mbox);
>> Do you think passing the msg structure as an argument and letting the
>> function populate it will be better instead of returning the msg
>> structure ? No strong opinion since from read_foo() point of view
>> what you have done might be right thing. In either case, please
>> get rid of typecasting.
>>
>
> Passing the msg structure looks fine. Will do that in the next version.
>
OK

Regards
Santosh