Re: [PATCH v4 1/2] Input: DaVinci Keypad Driver

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Tue, Oct 13, 2009 at 12:21:47PM -0400, H Hartley Sweeten wrote:
> On Monday, October 12, 2009 10:47 PM, Dmitry Torokhov wrote:
> > Hi Miguel,
> > 
> > On Fri, Oct 09, 2009 at 11:27:05AM -0600, miguel.aguilar@ridgerun.com wrote:
> >> From: Miguel Aguilar <miguel.aguilar@ridgerun.com>
> >> 
> >> Adds the driver for enabling keypad support for DaVinci platforms.
> >> 
> >> DM365 is the only platform that uses this driver at the moment.
> >> 
> >
> > Looks pretty good, I have one question:
> >
> >> +static void davinci_ks_write(struct davinci_ks *davinci_ks, u32 val, u32 addr)
> >> +{
> >> +	u32 base = (u32)davinci_ks->base;
> >> +
> >> +	__raw_writel(val,(u32 *)(base + addr));
> >> +}
> >
> > Do you really need these casts? I'd think that bare __raw_writel would
> > work just fine.
> 
> The address for __raw_{write/read}* should be void __iomem *, davinci_ks->base
> should already be the correct type due to ioremap().
> 
> You can probably just change the whole thing to:
> 
> +static void davinci_ks_write(struct davinci_ks *davinci_ks, u32 val, u32 addr)
> +{
> +	__raw_writel(val, davinci_ks->base + addr);
> +}
> +
> +static u32 davinci_ks_read(struct davinci_ks *davinci_ks, u32 addr)
> +{
> +	return __raw_readl(davinci_ks->base + addr);
> +}
> 

Better yet, use __raw_readl() and __raw_writel() directly - there is no
need for wrappers that just rename existig functions not adding any
additional functionality.

-- 
Dmitry