On Friday 15 May 2009 19:01:11 João Ramos wrote:
[...]
> > +/*
> > + * readl/writel helpers to access internal registers using
> > + * an ioremapped cookie and the specified IDE register offset
> > + */
> > +
> > +static inline u32 ep93xx_readl(unsigned long base, u8 offset)
> > +{
> > + return readl((void __iomem *)(base + offset));
> > +}
> > +
> > +static inline void ep93xx_writel(u32 value, unsigned long base, u8 offset)
> > +{
> > + writel(value, (void __iomem *)(base + offset));
> > +}
> >
> > Hmm, what do we need these wrappers for exactly?
> >
> > Please remove them.
> >
> I just added those to increase code readability, so that I wouldn't have
> to do a '(void __iomem *)(base + offset)' in every readl/writel call.
> But I can remove it, no problem.
If readability is harmed by such casts you can always add a local variable
to alleviate for it, i.e.:
void __iomem *base = (unsigned long)__base;
However I don't think a lot of such tricks would be needed after fixing function
arguments to pass 'void __iomem *base' around and not 'unsigned long base'
> > +/*
> > + * Check whether IORDY is asserted or not.
> > + */
> > +static inline int ep93xx_ide_check_iordy(unsigned long base)
like here
> > +{
> > + u32 reg = ep93xx_readl(base, IDECTRL);
> > +
> > + return (reg & IDECTRL_IORDY) ? 1 : 0;
> > +}
[...]
> > + /*
> > + * fill in ide_io_ports structure.
> > + * we use magic numbers 0x10 for io_addr and 0x0E for ctl_addr,
> > + * hence the lowest 3 bits will give us the real address (ranging from
> > + * 0 to 7) and the subsequent 2 bits will give us the CS1n and CS0n
> > + * values:
> > + * CS1n CS0n A2 A1 A0
> > + * 1 0 x x x -> IO_ADDR (base 0x10)
> > + * 0 1 x x x -> CTL_ADDR (base 0x0E)
> > + */
> > + ide_std_init_ports(&hw, 0x10, 0x0E);
> >
> > Why not just setup the real addresses here instead of using
> > ide_std_init_ports()?
> >
>
> The IDE register's address are specified through bitfields in the EP93xx
> IDECTRL register, as described in the above comment.
> The 'workaround' in the addresses is just to ease manipulation of those
> bitfields while writing the register.
Ok, so there is really no reason to use ide_std_init_ports().
> I suppose I could write the register's address in the hw->io_ports so
> that the value would be directly ORed to the IDECTRL register, if that
> is what you suggest.
Please try it.
> > It seems that it would make driver simpler and get rid of >config_data use.
> >
>
> No. The config_data is used to store the ioremapped cookie of the IDE
> registers base address. I need that address to access the CPUs IDE
> registers.
Please note that ep93xx_ide_{read,write}() are always passed
hwif->config_data as 'unsigned long base' argument so the first
argument should become 'ide_hwif_t *hwif' with:
unsigned long base = hwif->config_data
inside ep93xx_ide_{read,write}().
Also how's about using host->priv_data instead of hwif->config_data?
[ By using ide_host_alloc()+ide_host_register() instead of ide_host_add()
and later obtaining host->priv_data through hwif->host->priv_data. ]
> There's no connection between this address (which maps to CPU's internal
> memory, allowing to acess the IDE registers) and the actual IDE
> addresses, which are defined through bitfields in the IDE control register.
> Yeah, I know, weird IDE host controller... :-)
Indeed. :)
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
