Re: [PATCH V2] i2c-designware: Change readl to readw and writel to writew

[Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>]

Hi Rajeev,

On Thu, Oct 27, 2011 at 11:01:34AM +0530, Rajeev kumar wrote:
> On 10/24/2011 5:53 PM, Baruch Siach wrote:
> >On Mon, Oct 24, 2011 at 04:38:26PM +0530, Rajeev kumar wrote:
> >>On 10/24/2011 4:03 PM, Baruch Siach wrote:
> >>>On Mon, Oct 24, 2011 at 03:28:02PM +0530, Rajeev Kumar wrote:
> >>>>Since I2C designware registers are 16 bit wide and so we should use
> >>>>readw/writew.
> >>>>
> >>>>Signed-off-by: Rajeev Kumar<rajeev-dlh.kumar-qxv4g6HH51o@public.gmane.org>
> >>>>---
> >>>>  drivers/i2c/busses/i2c-designware.c |  104 +++++++++++++++++-----------------
> >>>>  1 files changed, 52 insertions(+), 52 deletions(-)
> >>>>
> >>>>diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
> >>>>index 6eaa681..5149a10 100644
> >>>>--- a/drivers/i2c/busses/i2c-designware.c
> >>>>+++ b/drivers/i2c/busses/i2c-designware.c
> >>>>@@ -216,11 +216,11 @@ struct dw_i2c_dev {
> >>>>  	u32			abort_source;
> >>>>  	int			irq;
> >>>>  	struct i2c_adapter	adapter;
> >>>>-	unsigned int		tx_fifo_depth;
> >>>>-	unsigned int		rx_fifo_depth;
> >>>>+	u16			tx_fifo_depth;
> >>>>+	u16			rx_fifo_depth;
> >>>>  };
> >>>
> >>>This looks wrong. The {tx,rx}_fifo_depth fields do not represent bit fields,
> >>>but numbers. So unsigned int should be better here.
> >>
> >>Yes, I agree with you, but I do not see any possibility of value of
> >>{tx,rx}_fifo_depth fields greater than 2^^16 - 1. So, would not it
> >>be better to keep them as u16 and save just 4 bytes.
> >
> >Well, if you are after these 4 bytes just make them 'unsigned short'.
> 
> Sorry, I could not understand preference of unsigned short over u16.
> 
> Although, its not a big deal to keep either unsigned short or u16 or
> unsigned int. More or less all are fine. But, should we not keep
> what is most appropriate?
> 
> Is it not correct that u16 will always be 16 bit , but unsigned short
> may not be guaranteed to be 16 bit on every platform?

Since this patch deals with matching variables width to registers size of the 
hardware, changes for reducing the size of struct dw_i2c_dev shouldn't be part 
of it, in my opinion.  But this is just nitpicking, I don't feel too strongly 
about it.

Acked-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -