Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Robert Hancock <hancockrwd@gmail.com>]

On 05/10/2010 03:51 AM, Sergei Shtylyov wrote:
> Hello.
>
> Graeme Russ wrote:
>
>> Sergei Shtylyov wrote:
>>> Hello.
>>>
>>> Graeme Russ wrote:
>>>
>>>> [Added linux-ide back onto distribution list]
>>> Right, I didn't intend to exclude it -- probably didn't press all the
>>> keys at once for the reply-to-all function.
>>>
>>> [...]
>>>>> You should have also taught the symmetric ide-platfrom driver the same
>>>>> trick. However, IDE core's data transfer methods has a different
>>>>> prototype.

The IDE subsystem is deprecated and in maintenance mode, it shouldn't be 
growing support for new hardware, which I assume this is.

>>>> I did think about the other drivers (OF Platform etc) but I have no
>>>> way of
>>>> testing them.
>>> pata_of_platform is not easily extensible in this way. It gets all the
>>> information about the device from the device tree -- and you can't
>>> encode all your complications there, unless you invent a new OF device.
>>>
>>>>> I suggest to rather add a new flag, indicating 8-bit data I/O, and
>>>>> have
>>>>> the alternate sff_data_xfer() method defined inside the driver.
>>
>> The vast majority of implementations are 16-bit (no one has complained
>> about the lack of 8-bit support to date). I don't think the majority of
>> users should be carrying around the extra code for a tiny minority.
>> Yes, it
>> could be wrapped around an #ifdef but then things start to get ugly (why
>> not just ditch the flag and #ifdef the 8-bit transfers entirely, hack
>> Kconfig etc) eeewwwww....
>
> I didn't propose any of this. Anyway, this is not an option anymore now
> that we know enough about your hardware.
>
>>>> other devices on the bus). By overriding the data transfer function
>>>> I can
>>>> arbitrate access to the bus and switch the bus timings based on the
>>>> peripheral being accessed. This cannot be done be a generic data
>>>> transfer
>>>> function.
>>> I disagree with your approach of overriding the libata methods at the
>>> board level, so I suggest to write a new driver. This is too complicated
>>> stuff for poor old pata_platform. :-)
>>
>> My custom function to date looks like:
>>
>> unsigned int ata_enet_data_xfer(struct ata_device *dev, unsigned char
>> *buf,
>> unsigned int buflen, int rw)
>> {
>> struct ata_port *ap = dev->link->ap;
>> void __iomem *data_addr = ap->ioaddr.data_addr;
>>
>> set_gp_bus_slow();
>> /* Transfer bytes */
>> if (rw == READ)
>> ioread8_rep(data_addr, buf, buflen);
>> else
>> iowrite8_rep(data_addr, buf, buflen);
>>
>> set_gp_bus_fast();
>> return buflen;
>> }
>>
>> set_gp_bus_slow() and set_gp_bus_fast() (at the moment) simply set a few
>> config registers to set the GP bus timing (no arbitration yet, but these
>> functions will also handle that using a mutex). I don't see the point in
>> re-writing the entire PATA Platform driver when the existing driver
>> appears
>> to be perfectly capable with a very minor extension hook.
>
> As I said, we *can't* implement the driver methods at the board level.
> Especially if they involve messing with timings -- that's the point
> where the ATA driver stops being generic, like pata_platform, and there
> arises a need for the dedicated driver. Also, your patch would bring in
> disparity with the ide-platfrom driver (which should be interchangeable
> with pata_platfrom). For me, the need of a separate driver is clear now,
> so I'll remain opposed to your patch. Of course, the maintainer (Jeff
> Garzik) will decide but if I could veto this patch I would.

I think there's a case to be made for doing some refactoring to allow 
splitting the code related to this hardware into a different file or 
something. However, creating an entirely different driver when the only 
thing different from pata_platform is that function seems excessive.