Re: [PATCH 1/3] PATA host controller driver for ep93xx

[Ryan Mallon <rmallon@gmail.com>]

On 02/04/12 17:52, Rafal Prylowski wrote:

> On 2012-03-30 22:18, Arnd Bergmann wrote:
>>> +static u16 ep93xx_pata_read(struct ep93xx_pata_data *drv_data,
>>> +			    void *addr_io,
>>> +			    const struct ata_timing *t)
>>> +{
>>> +	unsigned long addr = (unsigned long) addr_io & 0x1f;
>>> +	void __iomem *base = drv_data->ide_base;
>>> +	u16 ret;
>>> +
>>> +	writel(IDECtrl_DIOWn | IDECtrl_DIORn | addr, base + IDECtrl);
>>> +	ndelay(t->setup);
>>> +	writel(IDECtrl_DIOWn | addr, base + IDECtrl);
>>
>> The pointer arithmetic that you do here on addr_io looks really evil.
>> Basically your registers are indirect and cannot be accessed through
>> pointer dereference. Maybe you should not be trying to do that then
>> and not use the ata_port->ioaddr structure.
> 
> Yes, I already prepared a version of the driver in which IDE register
> addresses are coded as enums instead of using ata_port->ioaddr structure. 
> I will post updated version, when I get feedback from Ryan if enums
> or defines are preferred.

I would prefer defines, simply because it looks odd having an enum which
has multiple names defined to the same value, and most drivers tend to
use defines rather than enums for registers. It's not a big deal though,
and certainly not a show stopper.

~Ryan