Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

[Bartlomiej Zolnierkiewicz]

On Tue, Feb 8, 2011 at 3:12 PM, Alan Cox <alan@xxxxxxxxxxxxxxxxxxx> wrote:
>> respect some of my time spent on all this burdensome silly little
>> driver differences comparisons and read the whole patch set before
>> making comments on individual changes (which you certainly haven't
>> done given timing of your review mails and complexity of changes)..
>
> I was hoping you'd improved but apparently not.

I'm not the person who doesn't change..

> Any untested change is dangerous. An untested change that merges drivers
> together simply means you can break lots of stuff for no gain at all.

This is why patches were posted to mailing list with a request for a
real hardware testing:

"All testing was done using QEMU's PIIX3 controller emulation so any testing
with real EFAR, IT8213, old PIIX, RDC and Radisys R82600 PATA controllers
would be really appreciated.."

instead of request for a merge.  It was all there in initial mail.

Besides decreased complexity,1400 LOC gone and 5 drivers removed
cannot be really called "no gain at all".

> If these were all PCI card devices it might make some sense but given
> they are all motherboard chipsets putting them into one driver merely
> increases memory use as well.

We can improve situation here by doing generic ATA or SCSI changes
instead, not worth keeping separate drivers when you make similar
effect but more maintainable and generic changes.

> As far as stuff like
>
>    unsigned int has_sitre  = (dev->vendor != 0x8086 ||
>                                   dev->device != 0x1230);
>
> and the even worse mess you generate with the added patch all the other
> PIIX code does this by flags, and if you had a HAS_SITRE (or NO_SITRE)
> flag in the device data it would be obvious to anyone reading stuff how
> it all fitted together.

Now that's what is a real review work.  It will be changed in the next
revision, thanks!

-- Bartlomiej
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/