From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robert Hancock Subject: Re: [PATCH] Add hook for custom xfer function in PATA Platform driver Date: Mon, 10 May 2010 18:25:34 -0600 Message-ID: <4BE8A3FE.4070402@gmail.com> References: <1273382493-5859-1-git-send-email-graeme.russ@gmail.com> <4BE6910D.9070504@ru.mvista.com> <4BE69C81.2070703@gmail.com> <4BE6BA74.10200@mvista.com> <4BE74EEE.5060307@gmail.com> <4BE7D714.9010006@ru.mvista.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mail-gw0-f46.google.com ([74.125.83.46]:60118 "EHLO mail-gw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752736Ab0EKAZi (ORCPT ); Mon, 10 May 2010 20:25:38 -0400 Received: by gwj19 with SMTP id 19so2369576gwj.19 for ; Mon, 10 May 2010 17:25:37 -0700 (PDT) In-Reply-To: <4BE7D714.9010006@ru.mvista.com> Sender: linux-ide-owner@vger.kernel.org List-Id: linux-ide@vger.kernel.org To: Sergei Shtylyov Cc: Graeme Russ , linux-ide@vger.kernel.org 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.