Re: PATCH: CMD640 IDE chipset

[Bartlomiej Zolnierkiewicz]

On Wednesday 29 of October 2003 13:12, Stefan Talpalaru wrote:
> Hi Bartolomiej!

Hi,

> --- Bartlomiej Zolnierkiewicz <B.Zolnierkiewicz@xxxxxxxxxxxxxx> 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 ;-).

>    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...  */

>   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.
By removing setup_device_ptrs() and moving this driver to generic PCI layer,
you broke support for VLB version of CMD640.

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).

>   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...

Stefan, please rework your patch.  Thanks.

cheers,
--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/