Hi!
> > Hi Bartolomiej!
> Hi,
> > --- Bartlomiej Zolnierkiewicz <[email protected]>
wrote:
> > > Hi,
> > >
> > > Can you please drop all code-style changes (such as foo() -> foo
())
> >
> > sorry about that, I ran Lindent on it...
> Please read Documentation/CodingStyle instead ;-).
I've read it and I got the impression that a code cleanup is in order
but nevermind, I left it as wrongly indented as it was.
> > Please excuse me for sending this patch as an attachment,
> > but as my mail account is Yahoo! and I'm too lazy to find a better
> > sollution, I cannot get the patch through the web interface without
> > breaking the lines.
> Okay.
> > This patch integrates the CMD640 chipset support in the 2.4.22
> > kernel. I was using it succesfully in the 2.2.x kernel series, but
> > got no result in the 2.4.x kernels. After comparing the 2 versions,
> > I noticed errors in the new version (outb_p() instead of outl_p())
> > and also some useless code (the wrapers: __put_cmd640_reg() and
> > __get_cmd640_reg() - which I removed and placed the locks where
needed;
> It seems Alexander already covered removal of wrappers...
> > the pci_conf1() and pci_conf2() functions).
> You can't remove them.
> /* Find out what kind of PCI probing is supported otherwise
> we break some Adaptec cards... */
OK, you are right, i am wrong. This functions are back.
> > I also removed the CONFIG_BLK_DEV_CMD640_ENHANCED config option,
as
> > it
> > makes little difference for the kernel size.
> > The init_hwif_cmd640() function had to be rewritten because it is
> > called once for each ide interface found, so the old way of
addressing
> > all the drives in one run was no longer working. Therefore, to not
> > break all the code, came the need for a function that computes the
> > index from the ide_drive_t* : calculate_index().
> ide_probe_for_cmd640x() should be still be used instead.
I disagree.
> By removing setup_device_ptrs() and moving this driver to generic PCI
layer,
> you broke support for VLB version of CMD640.
I don't have a VLB version to test it on, but by reading the code I
think
that it will work just fine.
Anyway, if I'm wrong I should get a prize for breaking something that
was
allready broken :-)))) .
> Also there is a comment in a cmd640.c:
> /*
> * The CMD640x chip does not support DWORD config write cycles, but
some
> * of the BIOSes use them to implement the config services.
> */
> which worries me that it might be not safe to move this driver to
generic
> IDE PCI layer (at least for now).
Don't worry man, just read the code and you shall find peace of
mind....
> > The code that handles PIO settings was rearanged in a new
function:
> > cmd640_tuneproc().
> Is this really necessary, it is even harder to read it now...
It is necessary, unless the purpose of this piece of code is
readability.
> Stefan, please rework your patch. Thanks.
If you say that the only way I will get this driver fixed is to keep
it
ugly then I will send you a lame patch that does just that.
> cheers,
> --bartlomiej
later,
=====
Stefan Talpalaru
__________________________________
Do you Yahoo!?
Protect your identity with Yahoo! Mail AddressGuard
http://antispam.yahoo.com/whatsnewfree
On Monday 10 of November 2003 14:00, Stefan Talpalaru wrote:
> > > I also removed the CONFIG_BLK_DEV_CMD640_ENHANCED config option, as it
> > > makes little difference for the kernel size.
> > > The init_hwif_cmd640() function had to be rewritten because it is
> > > called once for each ide interface found, so the old way of addressing
> > > all the drives in one run was no longer working. Therefore, to not
> > > break all the code, came the need for a function that computes the
> > > index from the ide_drive_t* : calculate_index().
> >
> > ide_probe_for_cmd640x() should be still be used instead.
>
> I disagree.
>
> > By removing setup_device_ptrs() and moving this driver to generic PCI
> > layer, you broke support for VLB version of CMD640.
>
> I don't have a VLB version to test it on, but by reading the code I think
> that it will work just fine.
No, it won't be even probed. Note that ide_probe_for_cmd640x() was detecting
both VLB and PCI chipsets. After moving detection to generic IDE PCI layer,
only PCI version of the chipset will be probed (and only after Device/Vendor
ID matching). You are of course free to disagree as much as you want...
> Anyway, if I'm wrong I should get a prize for breaking something that was
> allready broken :-)))) .
>
> > Also there is a comment in a cmd640.c:
> > /*
> > * The CMD640x chip does not support DWORD config write cycles, but some
> > * of the BIOSes use them to implement the config services.
> > */
> > which worries me that it might be not safe to move this driver to generic
> > IDE PCI layer (at least for now).
>
> Don't worry man, just read the code and you shall find peace of mind....
Get serious, piece of mind after reading drivers/ide code? Nah.
> > > The code that handles PIO settings was rearanged in a new function:
> > > cmd640_tuneproc().
> >
> > Is this really necessary, it is even harder to read it now...
>
> It is necessary, unless the purpose of this piece of code is readability.
>
> > Stefan, please rework your patch. Thanks.
>
> If you say that the only way I will get this driver fixed is to keep it
> ugly then I will send you a lame patch that does just that.
Minimize changes, then next time when you need to fix this driver (say in 2.7)
you will spend much less time on tracking changes 2.0.x->2.7.x.
[ Uhh.. mail was sent too early thanks to KMail's "feature". ]
On Tuesday 11 of November 2003 17:50, Bartlomiej Zolnierkiewicz wrote:
> > > > The code that handles PIO settings was rearanged in a new function:
> > > > cmd640_tuneproc().
> > >
> > > Is this really necessary, it is even harder to read it now...
> >
> > It is necessary, unless the purpose of this piece of code is
> > readability.
No, but I want to know WHY it is necessary.
> > > Stefan, please rework your patch. Thanks.
> >
> > If you say that the only way I will get this driver fixed is to keep it
> > ugly then I will send you a lame patch that does just that.
>
> Minimize changes, then next time when you need to fix this driver (say in
> 2.7) you will spend much less time on tracking changes 2.0.x->2.7.x.
I've get report that fixing outb->outl issue is sufficient to get working driver.
Still dunno about second channel support.
Waiting for a lame patch... :)
--bartlomiej