[4/5] pata: Update experimental tags

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 drivers/ata/Kconfig |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 11/17/2009 09:51 AM, Alan Cox wrote:
> Signed-off-by: Alan Cox<alan@linux.intel.com>
> ---
>
>   drivers/ata/Kconfig |    8 ++++----
>   1 files changed, 4 insertions(+), 4 deletions(-)
>
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index f2df6e2..36931e0 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -374,7 +374,7 @@ config PATA_HPT366
>   	  If unsure, say N.
>
>   config PATA_HPT37X
> -	tristate "HPT 370/370A/371/372/374/302 PATA support (Experimental)"
> +	tristate "HPT 370/370A/371/372/374/302 PATA support"
>   	depends on PCI&&  EXPERIMENTAL


When you change the help string, you must also change the 'depends' line.

	Jeff




--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tuesday 17 November 2009 15:51:39 Alan Cox wrote:
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> ---
> 
>  drivers/ata/Kconfig |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index f2df6e2..36931e0 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -374,7 +374,7 @@ config PATA_HPT366
>  	  If unsure, say N.
>  
>  config PATA_HPT37X
> -	tristate "HPT 370/370A/371/372/374/302 PATA support (Experimental)"
> +	tristate "HPT 370/370A/371/372/374/302 PATA support"
>  	depends on PCI && EXPERIMENTAL
>  	help
>  	  This option enables support for the majority of the later HPT
> @@ -383,7 +383,7 @@ config PATA_HPT37X
>  	  If unsure, say N.
>  
>  config PATA_HPT3X2N
> -	tristate "HPT 372N/302N PATA support (Experimental)"
> +	tristate "HPT 372N/302N PATA support"
>  	depends on PCI && EXPERIMENTAL
>  	help
>  	  This option enables support for the N variant HPT PATA

Maybe they are 'stable' but when it comes to features they are behind hpt366
(i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
to understand and much smaller..

 1609 drivers/ide/hpt366.c

  432 drivers/ata/pata_hpt366.c
 1041 drivers/ata/pata_hpt37x.c
  594 drivers/ata/pata_hpt3x2n.c
 2067 total

(we can still easily cut more than 100 LOC from hpt366)

Having separate drivers wasn't the best decisions from the maintainability
point-of-view.   It added needless complexity (different chips share the same
PCI IDs which make detection across multiple drivers extremely painful) and
confusion (i.e. would you have guessed that HPT302 is supported by pata_hpt37x
while HPT302N by pata_hpt3x2n?).

> Maybe they are 'stable' but when it comes to features they are behind hpt366
> (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
> to understand and much smaller..

37x and 3xn lack PCI PM.  Added to the TODO list.

The smaller size is a bit questionable given its mostly comments, and
three drivers versus one.

> Having separate drivers wasn't the best decisions from the maintainability
> point-of-view.   It added needless complexity (different chips share the same

It was most definitely a good decision, having maintained both sets of
drivers at different times. It also makes it possible to do things the
way highpoint does rather than trying to make stuff common which HPT
themselves don't keep common. Even more importantly we get to break *one*
type of device at a time.

> PCI IDs which make detection across multiple drivers extremely painful) and
> confusion (i.e. would you have guessed that HPT302 is supported by pata_hpt37x
> while HPT302N by pata_hpt3x2n?).

I love highpoint too for their ever weirder and more confusingly labelled
and identified chips. I still think the split is worth it, and the 'wtf
device am I' logic is needed in both cases - either to pick a driver or
pick a set of methods.

We have the it821x driver for it8211/2 but not 8213 ..

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello.

Alan Cox wrote:

  >>Having separate drivers wasn't the best decisions from the maintainability
>>point-of-view.   It added needless complexity (different chips share the same

> It was most definitely a good decision, having maintained both sets of

    Separating HPT36x was grounded enough decision but I can't say the same 
of the separation of HPT3xxN.

> drivers at different times. It also makes it possible to do things the
> way highpoint does

    Oh, don't remind me of that stupid code mostly not worth copying from...

>>PCI IDs which make detection across multiple drivers extremely painful) and
>>confusion (i.e. would you have guessed that HPT302 is supported by pata_hpt37x
>>while HPT302N by pata_hpt3x2n?).

    How about HPT371N? ;-)

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wednesday 18 November 2009 19:41:25 Alan Cox wrote:
> > Maybe they are 'stable' but when it comes to features they are behind hpt366
> > (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
> > to understand and much smaller..
> 
> 37x and 3xn lack PCI PM.  Added to the TODO list.
> 
> The smaller size is a bit questionable given its mostly comments, and
> three drivers versus one.

I wonder what you find 'questionable' in the numbers given..

> > Having separate drivers wasn't the best decisions from the maintainability
> > point-of-view.   It added needless complexity (different chips share the same
> 
> It was most definitely a good decision, having maintained both sets of
> drivers at different times. It also makes it possible to do things the
> > way highpoint does rather than trying to make stuff common which HPT

Interesting, because all other people involved in HPT PATA support
seem to disagree with you and they certainly wouldn't say that doing
all things the way HPT did is a good thing..

> themselves don't keep common. Even more importantly we get to break *one*
> type of device at a time.

I prefer to not break anything, but hey my way of doing things is not
very much welcomed here..

I also like to think that by sharing code we get better testing coverage
and are in reality able to fix problems much faster because of this..

> > PCI IDs which make detection across multiple drivers extremely painful) and
> > confusion (i.e. would you have guessed that HPT302 is supported by pata_hpt37x
> > while HPT302N by pata_hpt3x2n?).
> 
> I love highpoint too for their ever weirder and more confusingly labelled
> and identified chips. I still think the split is worth it, and the 'wtf
> device am I' logic is needed in both cases - either to pick a driver or
> pick a set of methods.
> 
> We have the it821x driver for it8211/2 but not 8213 ..

it821x and it8213 share absolutely no common logic (the first chip
has ITE logic and the second one is based on Intel ICH) while in case
of HPT family there are sometimes large similarities..

pata_hpt37x.c:

...
static struct hpt_clock hpt37x_timings_66[] = {
	{ XFER_UDMA_6,		0x1c869c62 },
	{ XFER_UDMA_5,		0x1cae9c62 },	/* 0x1c8a9c62 */
	{ XFER_UDMA_4,		0x1c8a9c62 },
	{ XFER_UDMA_3,		0x1c8e9c62 },
	{ XFER_UDMA_2,		0x1c929c62 },
	{ XFER_UDMA_1,		0x1c9a9c62 },
	{ XFER_UDMA_0,		0x1c829c62 },

	{ XFER_MW_DMA_2,	0x2c829c62 },
	{ XFER_MW_DMA_1,	0x2c829c66 },
	{ XFER_MW_DMA_0,	0x2c829d2e },

	{ XFER_PIO_4,		0x0c829c62 },
	{ XFER_PIO_3,		0x0c829c84 },
	{ XFER_PIO_2,		0x0c829ca6 },
	{ XFER_PIO_1,		0x0d029d26 },
	{ XFER_PIO_0,		0x0d029d5e }
};
...
/**
 *	hpt372_set_piomode		-	PIO setup
 *	@ap: ATA interface
 *	@adev: device on the interface
 *
 *	Perform PIO mode setup.
 */

static void hpt372_set_piomode(struct ata_port *ap, struct ata_device *adev)
{
	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
	u32 addr1, addr2;
	u32 reg;
	u32 mode;
	u8 fast;

	addr1 = 0x40 + 4 * (adev->devno + 2 * ap->port_no);
	addr2 = 0x51 + 4 * ap->port_no;

	/* Fast interrupt prediction disable, hold off interrupt disable */
	pci_read_config_byte(pdev, addr2, &fast);
	fast &= ~0x07;
	pci_write_config_byte(pdev, addr2, fast);

	pci_read_config_dword(pdev, addr1, &reg);
	mode = hpt37x_find_mode(ap, adev->pio_mode);

	printk("Find mode for %d reports %X\n", adev->pio_mode, mode);
	mode &= ~0x80000000;	/* No FIFO in PIO */
	mode &= ~0x30070000;	/* Leave config bits alone */
	reg &= 0x30070000;	/* Strip timing bits */
	pci_write_config_dword(pdev, addr1, reg | mode);
}

/**
 *	hpt372_set_dmamode		-	DMA timing setup
 *	@ap: ATA interface
 *	@adev: Device being configured
 *
 *	Set up the channel for MWDMA or UDMA modes. Much the same as with
 *	PIO, load the mode number and then set MWDMA or UDMA flag.
 */

static void hpt372_set_dmamode(struct ata_port *ap, struct ata_device *adev)
{
	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
	u32 addr1, addr2;
	u32 reg;
	u32 mode;
	u8 fast;

	addr1 = 0x40 + 4 * (adev->devno + 2 * ap->port_no);
	addr2 = 0x51 + 4 * ap->port_no;

	/* Fast interrupt prediction disable, hold off interrupt disable */
	pci_read_config_byte(pdev, addr2, &fast);
	fast &= ~0x07;
	pci_write_config_byte(pdev, addr2, fast);

	pci_read_config_dword(pdev, addr1, &reg);
	mode = hpt37x_find_mode(ap, adev->dma_mode);
	printk("Find mode for DMA %d reports %X\n", adev->dma_mode, mode);
	mode &= ~0xC0000000;	/* Leave config bits alone */
	mode |= 0x80000000;	/* FIFO in MWDMA or UDMA */
	reg &= 0xC0000000;	/* Strip timing bits */
	pci_write_config_dword(pdev, addr1, reg | mode);
}

/**
 *	hpt37x_bmdma_end		-	DMA engine stop
 *	@qc: ATA command
 *
 *	Clean up after the HPT372 and later DMA engine
 */

static void hpt37x_bmdma_stop(struct ata_queued_cmd *qc)
{
	struct ata_port *ap = qc->ap;
	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
	int mscreg = 0x50 + 4 * ap->port_no;
	u8 bwsr_stat, msc_stat;

	pci_read_config_byte(pdev, 0x6A, &bwsr_stat);
	pci_read_config_byte(pdev, mscreg, &msc_stat);
	if (bwsr_stat & (1 << ap->port_no))
		pci_write_config_byte(pdev, mscreg, msc_stat | 0x30);
	ata_bmdma_stop(qc);
}
...
/**
 *	hpt37x_calibrate_dpll		-	Calibrate the DPLL loop
 *	@dev: PCI device
 *
 *	Perform a calibration cycle on the HPT37x DPLL. Returns 1 if this
 *	succeeds
 */

static int hpt37x_calibrate_dpll(struct pci_dev *dev)
{
	u8 reg5b;
	u32 reg5c;
	int tries;

	for(tries = 0; tries < 0x5000; tries++) {
		udelay(50);
		pci_read_config_byte(dev, 0x5b, &reg5b);
		if (reg5b & 0x80) {
			/* See if it stays set */
			for(tries = 0; tries < 0x1000; tries ++) {
				pci_read_config_byte(dev, 0x5b, &reg5b);
				/* Failed ? */
				if ((reg5b & 0x80) == 0)
					return 0;
			}
			/* Turn off tuning, we have the DPLL set */
			pci_read_config_dword(dev, 0x5c, &reg5c);
			pci_write_config_dword(dev, 0x5c, reg5c & ~ 0x100);
			return 1;
		}
	}
	/* Never went stable */
	return 0;
}
...


pata_hpt3x2n.c:

...
/* 66MHz DPLL clocks */

static struct hpt_clock hpt3x2n_clocks[] = {
	{	XFER_UDMA_7,	0x1c869c62	},
	{	XFER_UDMA_6,	0x1c869c62	},
	{	XFER_UDMA_5,	0x1c8a9c62	},
	{	XFER_UDMA_4,	0x1c8a9c62	},
	{	XFER_UDMA_3,	0x1c8e9c62	},
	{	XFER_UDMA_2,	0x1c929c62	},
	{	XFER_UDMA_1,	0x1c9a9c62	},
	{	XFER_UDMA_0,	0x1c829c62	},

	{	XFER_MW_DMA_2,	0x2c829c62	},
	{	XFER_MW_DMA_1,	0x2c829c66	},
	{	XFER_MW_DMA_0,	0x2c829d2c	},

	{	XFER_PIO_4,	0x0c829c62	},
	{	XFER_PIO_3,	0x0c829c84	},
	{	XFER_PIO_2,	0x0c829ca6	},
	{	XFER_PIO_1,	0x0d029d26	},
	{	XFER_PIO_0,	0x0d029d5e	},
	{	0,		0x0d029d5e	}
};
...
/**
 *	hpt3x2n_set_piomode		-	PIO setup
 *	@ap: ATA interface
 *	@adev: device on the interface
 *
 *	Perform PIO mode setup.
 */

static void hpt3x2n_set_piomode(struct ata_port *ap, struct ata_device *adev)
{
	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
	u32 addr1, addr2;
	u32 reg;
	u32 mode;
	u8 fast;

	addr1 = 0x40 + 4 * (adev->devno + 2 * ap->port_no);
	addr2 = 0x51 + 4 * ap->port_no;

	/* Fast interrupt prediction disable, hold off interrupt disable */
	pci_read_config_byte(pdev, addr2, &fast);
	fast &= ~0x07;
	pci_write_config_byte(pdev, addr2, fast);

	pci_read_config_dword(pdev, addr1, &reg);
	mode = hpt3x2n_find_mode(ap, adev->pio_mode);
	mode &= ~0x8000000;	/* No FIFO in PIO */
	mode &= ~0x30070000;	/* Leave config bits alone */
	reg &= 0x30070000;	/* Strip timing bits */
	pci_write_config_dword(pdev, addr1, reg | mode);
}

/**
 *	hpt3x2n_set_dmamode		-	DMA timing setup
 *	@ap: ATA interface
 *	@adev: Device being configured
 *
 *	Set up the channel for MWDMA or UDMA modes. Much the same as with
 *	PIO, load the mode number and then set MWDMA or UDMA flag.
 */

static void hpt3x2n_set_dmamode(struct ata_port *ap, struct ata_device *adev)
{
	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
	u32 addr1, addr2;
	u32 reg;
	u32 mode;
	u8 fast;

	addr1 = 0x40 + 4 * (adev->devno + 2 * ap->port_no);
	addr2 = 0x51 + 4 * ap->port_no;

	/* Fast interrupt prediction disable, hold off interrupt disable */
	pci_read_config_byte(pdev, addr2, &fast);
	fast &= ~0x07;
	pci_write_config_byte(pdev, addr2, fast);

	pci_read_config_dword(pdev, addr1, &reg);
	mode = hpt3x2n_find_mode(ap, adev->dma_mode);
	mode |= 0x8000000;	/* FIFO in MWDMA or UDMA */
	mode &= ~0xC0000000;	/* Leave config bits alone */
	reg &= 0xC0000000;	/* Strip timing bits */
	pci_write_config_dword(pdev, addr1, reg | mode);
}

/**
 *	hpt3x2n_bmdma_end		-	DMA engine stop
 *	@qc: ATA command
 *
 *	Clean up after the HPT3x2n and later DMA engine
 */

static void hpt3x2n_bmdma_stop(struct ata_queued_cmd *qc)
{
	struct ata_port *ap = qc->ap;
	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
	int mscreg = 0x50 + 2 * ap->port_no;
	u8 bwsr_stat, msc_stat;

	pci_read_config_byte(pdev, 0x6A, &bwsr_stat);
	pci_read_config_byte(pdev, mscreg, &msc_stat);
	if (bwsr_stat & (1 << ap->port_no))
		pci_write_config_byte(pdev, mscreg, msc_stat | 0x30);
	ata_bmdma_stop(qc);
}
...
/**
 *	hpt3xn_calibrate_dpll		-	Calibrate the DPLL loop
 *	@dev: PCI device
 *
 *	Perform a calibration cycle on the HPT3xN DPLL. Returns 1 if this
 *	succeeds
 */

static int hpt3xn_calibrate_dpll(struct pci_dev *dev)
{
	u8 reg5b;
	u32 reg5c;
	int tries;

	for(tries = 0; tries < 0x5000; tries++) {
		udelay(50);
		pci_read_config_byte(dev, 0x5b, &reg5b);
		if (reg5b & 0x80) {
			/* See if it stays set */
			for(tries = 0; tries < 0x1000; tries ++) {
				pci_read_config_byte(dev, 0x5b, &reg5b);
				/* Failed ? */
				if ((reg5b & 0x80) == 0)
					return 0;
			}
			/* Turn off tuning, we have the DPLL set */
			pci_read_config_dword(dev, 0x5c, &reg5c);
			pci_write_config_dword(dev, 0x5c, reg5c & ~ 0x100);
			return 1;
		}
	}
	/* Never went stable */
	return 0;
}
...

etc.

On Wednesday 18 November 2009 19:47:25 Sergei Shtylyov wrote:
> Hello.
> 
> Alan Cox wrote:
> 
>   >>Having separate drivers wasn't the best decisions from the maintainability
> >>point-of-view.   It added needless complexity (different chips share the same
> 
> > It was most definitely a good decision, having maintained both sets of
> 
>     Separating HPT36x was grounded enough decision but I can't say the same 
> of the separation of HPT3xxN.

When it comes to HPT36x I would agree if we would be developing completely
from scratch but due to historical reasons we share UDMA blacklists between
HPT36x and HPT37x.  It is not worth it in terms of potential regressions to
try to untangle them nowadays.

> > drivers at different times. It also makes it possible to do things the
> > way highpoint does
> 
>     Oh, don't remind me of that stupid code mostly not worth copying from...
> 
> >>PCI IDs which make detection across multiple drivers extremely painful) and
> >>confusion (i.e. would you have guessed that HPT302 is supported by pata_hpt37x
> >>while HPT302N by pata_hpt3x2n?).
> 
>     How about HPT371N? ;-)

Not a problem, this one is unsupported according to help entries. ;)

On Wednesday 18 November 2009 19:41:25 Alan Cox wrote:

> > PCI IDs which make detection across multiple drivers extremely painful) and
> > confusion (i.e. would you have guessed that HPT302 is supported by pata_hpt37x
> > while HPT302N by pata_hpt3x2n?).
> 
> I love highpoint too for their ever weirder and more confusingly labelled
> and identified chips. I still think the split is worth it, and the 'wtf
> device am I' logic is needed in both cases - either to pick a driver or
> pick a set of methods.

While in case of pata_hpt366.c it doesn't look that bad in case of two other
drivers it does..

pata_hpt366.c:
...
/**
 *	hpt36x_init_one		-	Initialise an HPT366/368
 *	@dev: PCI device
 *	@id: Entry in match table
 *
 *	Initialise an HPT36x device. There are some interesting complications
 *	here. Firstly the chip may report 366 and be one of several variants.
 *	Secondly all the timings depend on the clock for the chip which we must
 *	detect and look up
 *
 *	This is the known chip mappings. It may be missing a couple of later
 *	releases.
 *
 *	Chip version		PCI		Rev	Notes
 *	HPT366			4 (HPT366)	0	UDMA66
 *	HPT366			4 (HPT366)	1	UDMA66
 *	HPT368			4 (HPT366)	2	UDMA66
 *	HPT37x/30x		4 (HPT366)	3+	Other driver
 *
 */

static int hpt36x_init_one(struct pci_dev *dev, const struct pci_device_id *id)
{
	static const struct ata_port_info info_hpt366 = {
		.flags = ATA_FLAG_SLAVE_POSS,
		.pio_mask = ATA_PIO4,
		.mwdma_mask = ATA_MWDMA2,
		.udma_mask = ATA_UDMA4,
		.port_ops = &hpt366_port_ops
	};
	const struct ata_port_info *ppi[] = { &info_hpt366, NULL };

	void *hpriv = NULL;
	u32 class_rev;
	u32 reg1;
	int rc;

	rc = pcim_enable_device(dev);
	if (rc)
		return rc;

	pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
	class_rev &= 0xFF;

	/* May be a later chip in disguise. Check */
	/* Newer chips are not in the HPT36x driver. Ignore them */
	if (class_rev > 2)
			return -ENODEV;
...

pata_hpt37x.c:
...
/**
 *	hpt37x_init_one		-	Initialise an HPT37X/302
 *	@dev: PCI device
 *	@id: Entry in match table
 *
 *	Initialise an HPT37x device. There are some interesting complications
 *	here. Firstly the chip may report 366 and be one of several variants.
 *	Secondly all the timings depend on the clock for the chip which we must
 *	detect and look up
 *
 *	This is the known chip mappings. It may be missing a couple of later
 *	releases.
 *
 *	Chip version		PCI		Rev	Notes
 *	HPT366			4 (HPT366)	0	Other driver
 *	HPT366			4 (HPT366)	1	Other driver
 *	HPT368			4 (HPT366)	2	Other driver
 *	HPT370			4 (HPT366)	3	UDMA100
 *	HPT370A			4 (HPT366)	4	UDMA100
 *	HPT372			4 (HPT366)	5	UDMA133 (1)
 *	HPT372N			4 (HPT366)	6	Other driver
 *	HPT372A			5 (HPT372)	1	UDMA133 (1)
 *	HPT372N			5 (HPT372)	2	Other driver
 *	HPT302			6 (HPT302)	1	UDMA133
 *	HPT302N			6 (HPT302)	2	Other driver
 *	HPT371			7 (HPT371)	*	UDMA133
 *	HPT374			8 (HPT374)	*	UDMA133 4 channel
 *	HPT372N			9 (HPT372N)	*	Other driver
 *
 *	(1) UDMA133 support depends on the bus clock
 */

static int hpt37x_init_one(struct pci_dev *dev, const struct pci_device_id *id)
{
	/* HPT370 - UDMA100 */
	static const struct ata_port_info info_hpt370 = {
		.flags = ATA_FLAG_SLAVE_POSS,
		.pio_mask = ATA_PIO4,
		.mwdma_mask = ATA_MWDMA2,
		.udma_mask = ATA_UDMA5,
		.port_ops = &hpt370_port_ops
	};
	/* HPT370A - UDMA100 */
	static const struct ata_port_info info_hpt370a = {
		.flags = ATA_FLAG_SLAVE_POSS,
		.pio_mask = ATA_PIO4,
		.mwdma_mask = ATA_MWDMA2,
		.udma_mask = ATA_UDMA5,
		.port_ops = &hpt370a_port_ops
	};
	/* HPT370 - UDMA100 */
	static const struct ata_port_info info_hpt370_33 = {
		.flags = ATA_FLAG_SLAVE_POSS,
		.pio_mask = ATA_PIO4,
		.mwdma_mask = ATA_MWDMA2,
		.udma_mask = ATA_UDMA5,
		.port_ops = &hpt370_port_ops
	};
	/* HPT370A - UDMA100 */
	static const struct ata_port_info info_hpt370a_33 = {
		.flags = ATA_FLAG_SLAVE_POSS,
		.pio_mask = ATA_PIO4,
		.mwdma_mask = ATA_MWDMA2,
		.udma_mask = ATA_UDMA5,
		.port_ops = &hpt370a_port_ops
	};
	/* HPT371, 372 and friends - UDMA133 */
	static const struct ata_port_info info_hpt372 = {
		.flags = ATA_FLAG_SLAVE_POSS,
		.pio_mask = ATA_PIO4,
		.mwdma_mask = ATA_MWDMA2,
		.udma_mask = ATA_UDMA6,
		.port_ops = &hpt372_port_ops
	};
	/* HPT374 - UDMA100, function 1 uses different prereset method */
	static const struct ata_port_info info_hpt374_fn0 = {
		.flags = ATA_FLAG_SLAVE_POSS,
		.pio_mask = ATA_PIO4,
		.mwdma_mask = ATA_MWDMA2,
		.udma_mask = ATA_UDMA5,
		.port_ops = &hpt372_port_ops
	};
	static const struct ata_port_info info_hpt374_fn1 = {
		.flags = ATA_FLAG_SLAVE_POSS,
		.pio_mask = ATA_PIO4,
		.mwdma_mask = ATA_MWDMA2,
		.udma_mask = ATA_UDMA5,
		.port_ops = &hpt374_fn1_port_ops
	};

	static const int MHz[4] = { 33, 40, 50, 66 };
	void *private_data = NULL;
	const struct ata_port_info *ppi[] = { NULL, NULL };

	u8 irqmask;
	u32 class_rev;
	u8 mcr1;
	u32 freq;
	int prefer_dpll = 1;

	unsigned long iobase = pci_resource_start(dev, 4);

	const struct hpt_chip *chip_table;
	int clock_slot;
	int rc;

	rc = pcim_enable_device(dev);
	if (rc)
		return rc;

	pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
	class_rev &= 0xFF;

	if (dev->device == PCI_DEVICE_ID_TTI_HPT366) {
		/* May be a later chip in disguise. Check */
		/* Older chips are in the HPT366 driver. Ignore them */
		if (class_rev < 3)
			return -ENODEV;
		/* N series chips have their own driver. Ignore */
		if (class_rev == 6)
			return -ENODEV;

		switch(class_rev) {
			case 3:
				ppi[0] = &info_hpt370;
				chip_table = &hpt370;
				prefer_dpll = 0;
				break;
			case 4:
				ppi[0] = &info_hpt370a;
				chip_table = &hpt370a;
				prefer_dpll = 0;
				break;
			case 5:
				ppi[0] = &info_hpt372;
				chip_table = &hpt372;
				break;
			default:
				printk(KERN_ERR "pata_hpt37x: Unknown HPT366 subtype please report (%d).\n", class_rev);
				return -ENODEV;
		}
	} else {
		switch(dev->device) {
			case PCI_DEVICE_ID_TTI_HPT372:
				/* 372N if rev >= 2*/
				if (class_rev >= 2)
					return -ENODEV;
				ppi[0] = &info_hpt372;
				chip_table = &hpt372a;
				break;
			case PCI_DEVICE_ID_TTI_HPT302:
				/* 302N if rev > 1 */
				if (class_rev > 1)
					return -ENODEV;
				ppi[0] = &info_hpt372;
				/* Check this */
				chip_table = &hpt302;
				break;
			case PCI_DEVICE_ID_TTI_HPT371:
				if (class_rev > 1)
					return -ENODEV;
				ppi[0] = &info_hpt372;
				chip_table = &hpt371;
				/* Single channel device, master is not present
				   but the BIOS (or us for non x86) must mark it
				   absent */
				pci_read_config_byte(dev, 0x50, &mcr1);
				mcr1 &= ~0x04;
				pci_write_config_byte(dev, 0x50, mcr1);
				break;
			case PCI_DEVICE_ID_TTI_HPT374:
				chip_table = &hpt374;
				if (!(PCI_FUNC(dev->devfn) & 1))
					*ppi = &info_hpt374_fn0;
				else
					*ppi = &info_hpt374_fn1;
				break;
			default:
				printk(KERN_ERR "pata_hpt37x: PCI table is bogus please report (%d).\n", dev->device);
				return -ENODEV;
		}
	}
...

pata_hpt3x2n.c:
...
/**
 *	hpt3x2n_init_one		-	Initialise an HPT37X/302
 *	@dev: PCI device
 *	@id: Entry in match table
 *
 *	Initialise an HPT3x2n device. There are some interesting complications
 *	here. Firstly the chip may report 366 and be one of several variants.
 *	Secondly all the timings depend on the clock for the chip which we must
 *	detect and look up
 *
 *	This is the known chip mappings. It may be missing a couple of later
 *	releases.
 *
 *	Chip version		PCI		Rev	Notes
 *	HPT372			4 (HPT366)	5	Other driver
 *	HPT372N			4 (HPT366)	6	UDMA133
 *	HPT372			5 (HPT372)	1	Other driver
 *	HPT372N			5 (HPT372)	2	UDMA133
 *	HPT302			6 (HPT302)	*	Other driver
 *	HPT302N			6 (HPT302)	> 1	UDMA133
 *	HPT371			7 (HPT371)	*	Other driver
 *	HPT371N			7 (HPT371)	> 1	UDMA133
 *	HPT374			8 (HPT374)	*	Other driver
 *	HPT372N			9 (HPT372N)	*	UDMA133
 *
 *	(1) UDMA133 support depends on the bus clock
 *
 *	To pin down		HPT371N
 */

static int hpt3x2n_init_one(struct pci_dev *dev, const struct pci_device_id *id)
{
	/* HPT372N and friends - UDMA133 */
	static const struct ata_port_info info = {
		.flags = ATA_FLAG_SLAVE_POSS,
		.pio_mask = ATA_PIO4,
		.mwdma_mask = ATA_MWDMA2,
		.udma_mask = ATA_UDMA6,
		.port_ops = &hpt3x2n_port_ops
	};
	const struct ata_port_info *ppi[] = { &info, NULL };

	u8 irqmask;
	u32 class_rev;

	unsigned int pci_mhz;
	unsigned int f_low, f_high;
	int adjust;
	unsigned long iobase = pci_resource_start(dev, 4);
	void *hpriv = NULL;
	int rc;

	rc = pcim_enable_device(dev);
	if (rc)
		return rc;

	pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
	class_rev &= 0xFF;

	switch(dev->device) {
		case PCI_DEVICE_ID_TTI_HPT366:
			if (class_rev < 6)
				return -ENODEV;
			break;
		case PCI_DEVICE_ID_TTI_HPT371:
			if (class_rev < 2)
				return -ENODEV;
			/* 371N if rev > 1 */
			break;
		case PCI_DEVICE_ID_TTI_HPT372:
			/* 372N if rev >= 2*/
			if (class_rev < 2)
				return -ENODEV;
			break;
		case PCI_DEVICE_ID_TTI_HPT302:
			if (class_rev < 2)
				return -ENODEV;
			break;
		case PCI_DEVICE_ID_TTI_HPT372N:
			break;
		default:
			printk(KERN_ERR "pata_hpt3x2n: PCI table is bogus please report (%d).\n", dev->device);
			return -ENODEV;
	}
...


and now for the comparison hpt366.c:


...
static const struct hpt_info hpt36x __devinitdata = {
	.chip_name	= "HPT36x",
	.chip_type	= HPT36x,
	.udma_mask	= HPT366_ALLOW_ATA66_3 ? (HPT366_ALLOW_ATA66_4 ? ATA_UDMA4 : ATA_UDMA3) : ATA_UDMA2,
	.dpll_clk	= 0,	/* no DPLL */
	.timings	= &hpt36x_timings
};

static const struct hpt_info hpt370 __devinitdata = {
	.chip_name	= "HPT370",
	.chip_type	= HPT370,
	.udma_mask	= HPT370_ALLOW_ATA100_5 ? ATA_UDMA5 : ATA_UDMA4,
	.dpll_clk	= 48,
	.timings	= &hpt37x_timings
};

static const struct hpt_info hpt370a __devinitdata = {
	.chip_name	= "HPT370A",
	.chip_type	= HPT370A,
	.udma_mask	= HPT370_ALLOW_ATA100_5 ? ATA_UDMA5 : ATA_UDMA4,
	.dpll_clk	= 48,
	.timings	= &hpt37x_timings
};

static const struct hpt_info hpt374 __devinitdata = {
	.chip_name	= "HPT374",
	.chip_type	= HPT374,
	.udma_mask	= ATA_UDMA5,
	.dpll_clk	= 48,
	.timings	= &hpt37x_timings
};

static const struct hpt_info hpt372 __devinitdata = {
	.chip_name	= "HPT372",
	.chip_type	= HPT372,
	.udma_mask	= HPT372_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
	.dpll_clk	= 55,
	.timings	= &hpt37x_timings
};

static const struct hpt_info hpt372a __devinitdata = {
	.chip_name	= "HPT372A",
	.chip_type	= HPT372A,
	.udma_mask	= HPT372_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
	.dpll_clk	= 66,
	.timings	= &hpt37x_timings
};

static const struct hpt_info hpt302 __devinitdata = {
	.chip_name	= "HPT302",
	.chip_type	= HPT302,
	.udma_mask	= HPT302_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
	.dpll_clk	= 66,
	.timings	= &hpt37x_timings
};

static const struct hpt_info hpt371 __devinitdata = {
	.chip_name	= "HPT371",
	.chip_type	= HPT371,
	.udma_mask	= HPT371_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
	.dpll_clk	= 66,
	.timings	= &hpt37x_timings
};

static const struct hpt_info hpt372n __devinitdata = {
	.chip_name	= "HPT372N",
	.chip_type	= HPT372N,
	.udma_mask	= HPT372_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
	.dpll_clk	= 77,
	.timings	= &hpt37x_timings
};

static const struct hpt_info hpt302n __devinitdata = {
	.chip_name	= "HPT302N",
	.chip_type	= HPT302N,
	.udma_mask	= HPT302_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
	.dpll_clk	= 77,
	.timings	= &hpt37x_timings
};

static const struct hpt_info hpt371n __devinitdata = {
	.chip_name	= "HPT371N",
	.chip_type	= HPT371N,
	.udma_mask	= HPT371_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
	.dpll_clk	= 77,
	.timings	= &hpt37x_timings
};
...
/**
 *	hpt366_init_one	-	called when an HPT366 is found
 *	@dev: the hpt366 device
 *	@id: the matching pci id
 *
 *	Called when the PCI registration layer (or the IDE initialization)
 *	finds a device matching our IDE device tables.
 */
static int __devinit hpt366_init_one(struct pci_dev *dev, const struct pci_device_id *id)
{
	const struct hpt_info *info = NULL;
	struct hpt_info *dyn_info;
	struct pci_dev *dev2 = NULL;
	struct ide_port_info d;
	u8 idx = id->driver_data;
	u8 rev = dev->revision;
	int ret;

	if ((idx == 0 || idx == 4) && (PCI_FUNC(dev->devfn) & 1))
		return -ENODEV;

	switch (idx) {
	case 0:
		if (rev < 3)
			info = &hpt36x;
		else {
			switch (min_t(u8, rev, 6)) {
			case 3: info = &hpt370;  break;
			case 4: info = &hpt370a; break;
			case 5: info = &hpt372;  break;
			case 6: info = &hpt372n; break;
			}
			idx++;
		}
		break;
	case 1:
		info = (rev > 1) ? &hpt372n : &hpt372a;
		break;
	case 2:
		info = (rev > 1) ? &hpt302n : &hpt302;
		break;
	case 3:
		hpt371_init(dev);
		info = (rev > 1) ? &hpt371n : &hpt371;
		break;
	case 4:
		info = &hpt374;
		break;
	case 5:
		info = &hpt372n;
		break;
	}
...

On Wednesday 18 November 2009 20:07:07 Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 18 November 2009 19:41:25 Alan Cox wrote:
> > > Maybe they are 'stable' but when it comes to features they are behind hpt366
> > > (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
> > > to understand and much smaller..
> > 
> > 37x and 3xn lack PCI PM.  Added to the TODO list.
> > 
> > The smaller size is a bit questionable given its mostly comments, and
> > three drivers versus one.
> 
> I wonder what you find 'questionable' in the numbers given..

BTW we can immediately reclaim 300 LOC by simply merging drivers back..

> > themselves don't keep common. Even more importantly we get to break *one*
> > type of device at a time.
> 
> I prefer to not break anything, but hey my way of doing things is not
> very much welcomed here..
> 
> I also like to think that by sharing code we get better testing coverage
> and are in reality able to fix problems much faster because of this..

Wait, you seem to be right!

Only pata_hpt37x has broken cable detection while pata_hpt3x2n is okay. ;)

On Wednesday 18 November 2009 20:34:19 Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 18 November 2009 19:41:25 Alan Cox wrote:
> 
> > > PCI IDs which make detection across multiple drivers extremely painful) and
> > > confusion (i.e. would you have guessed that HPT302 is supported by pata_hpt37x
> > > while HPT302N by pata_hpt3x2n?).
> > 
> > I love highpoint too for their ever weirder and more confusingly labelled
> > and identified chips. I still think the split is worth it, and the 'wtf
> > device am I' logic is needed in both cases - either to pick a driver or
> > pick a set of methods.
> 
> While in case of pata_hpt366.c it doesn't look that bad in case of two other
> drivers it does..
> 
> pata_hpt366.c:
> ...
> /**
>  *	hpt36x_init_one		-	Initialise an HPT366/368
>  *	@dev: PCI device
>  *	@id: Entry in match table
>  *
>  *	Initialise an HPT36x device. There are some interesting complications
>  *	here. Firstly the chip may report 366 and be one of several variants.
>  *	Secondly all the timings depend on the clock for the chip which we must
>  *	detect and look up
>  *
>  *	This is the known chip mappings. It may be missing a couple of later
>  *	releases.
>  *
>  *	Chip version		PCI		Rev	Notes
>  *	HPT366			4 (HPT366)	0	UDMA66
>  *	HPT366			4 (HPT366)	1	UDMA66
>  *	HPT368			4 (HPT366)	2	UDMA66
>  *	HPT37x/30x		4 (HPT366)	3+	Other driver
>  *
>  */
> 
> static int hpt36x_init_one(struct pci_dev *dev, const struct pci_device_id *id)
> {
> 	static const struct ata_port_info info_hpt366 = {
> 		.flags = ATA_FLAG_SLAVE_POSS,
> 		.pio_mask = ATA_PIO4,
> 		.mwdma_mask = ATA_MWDMA2,
> 		.udma_mask = ATA_UDMA4,
> 		.port_ops = &hpt366_port_ops
> 	};
> 	const struct ata_port_info *ppi[] = { &info_hpt366, NULL };
> 
> 	void *hpriv = NULL;
> 	u32 class_rev;
> 	u32 reg1;
> 	int rc;
> 
> 	rc = pcim_enable_device(dev);
> 	if (rc)
> 		return rc;
> 
> 	pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
> 	class_rev &= 0xFF;
> 
> 	/* May be a later chip in disguise. Check */
> 	/* Newer chips are not in the HPT36x driver. Ignore them */
> 	if (class_rev > 2)
> 			return -ENODEV;
> ...
> 
> pata_hpt37x.c:
> ...
> /**
>  *	hpt37x_init_one		-	Initialise an HPT37X/302
>  *	@dev: PCI device
>  *	@id: Entry in match table
>  *
>  *	Initialise an HPT37x device. There are some interesting complications
>  *	here. Firstly the chip may report 366 and be one of several variants.
>  *	Secondly all the timings depend on the clock for the chip which we must
>  *	detect and look up
>  *
>  *	This is the known chip mappings. It may be missing a couple of later
>  *	releases.
>  *
>  *	Chip version		PCI		Rev	Notes
>  *	HPT366			4 (HPT366)	0	Other driver
>  *	HPT366			4 (HPT366)	1	Other driver
>  *	HPT368			4 (HPT366)	2	Other driver
>  *	HPT370			4 (HPT366)	3	UDMA100
>  *	HPT370A			4 (HPT366)	4	UDMA100
>  *	HPT372			4 (HPT366)	5	UDMA133 (1)
>  *	HPT372N			4 (HPT366)	6	Other driver
>  *	HPT372A			5 (HPT372)	1	UDMA133 (1)
>  *	HPT372N			5 (HPT372)	2	Other driver
>  *	HPT302			6 (HPT302)	1	UDMA133
>  *	HPT302N			6 (HPT302)	2	Other driver
>  *	HPT371			7 (HPT371)	*	UDMA133
>  *	HPT374			8 (HPT374)	*	UDMA133 4 channel
>  *	HPT372N			9 (HPT372N)	*	Other driver
>  *
>  *	(1) UDMA133 support depends on the bus clock
>  */
> 
> static int hpt37x_init_one(struct pci_dev *dev, const struct pci_device_id *id)
> {
> 	/* HPT370 - UDMA100 */
> 	static const struct ata_port_info info_hpt370 = {
> 		.flags = ATA_FLAG_SLAVE_POSS,
> 		.pio_mask = ATA_PIO4,
> 		.mwdma_mask = ATA_MWDMA2,
> 		.udma_mask = ATA_UDMA5,
> 		.port_ops = &hpt370_port_ops
> 	};
> 	/* HPT370A - UDMA100 */
> 	static const struct ata_port_info info_hpt370a = {
> 		.flags = ATA_FLAG_SLAVE_POSS,
> 		.pio_mask = ATA_PIO4,
> 		.mwdma_mask = ATA_MWDMA2,
> 		.udma_mask = ATA_UDMA5,
> 		.port_ops = &hpt370a_port_ops
> 	};
> 	/* HPT370 - UDMA100 */
> 	static const struct ata_port_info info_hpt370_33 = {
> 		.flags = ATA_FLAG_SLAVE_POSS,
> 		.pio_mask = ATA_PIO4,
> 		.mwdma_mask = ATA_MWDMA2,
> 		.udma_mask = ATA_UDMA5,
> 		.port_ops = &hpt370_port_ops
> 	};
> 	/* HPT370A - UDMA100 */
> 	static const struct ata_port_info info_hpt370a_33 = {
> 		.flags = ATA_FLAG_SLAVE_POSS,
> 		.pio_mask = ATA_PIO4,
> 		.mwdma_mask = ATA_MWDMA2,
> 		.udma_mask = ATA_UDMA5,
> 		.port_ops = &hpt370a_port_ops
> 	};
> 	/* HPT371, 372 and friends - UDMA133 */
> 	static const struct ata_port_info info_hpt372 = {
> 		.flags = ATA_FLAG_SLAVE_POSS,
> 		.pio_mask = ATA_PIO4,
> 		.mwdma_mask = ATA_MWDMA2,
> 		.udma_mask = ATA_UDMA6,
> 		.port_ops = &hpt372_port_ops
> 	};
> 	/* HPT374 - UDMA100, function 1 uses different prereset method */
> 	static const struct ata_port_info info_hpt374_fn0 = {
> 		.flags = ATA_FLAG_SLAVE_POSS,
> 		.pio_mask = ATA_PIO4,
> 		.mwdma_mask = ATA_MWDMA2,
> 		.udma_mask = ATA_UDMA5,
> 		.port_ops = &hpt372_port_ops
> 	};
> 	static const struct ata_port_info info_hpt374_fn1 = {
> 		.flags = ATA_FLAG_SLAVE_POSS,
> 		.pio_mask = ATA_PIO4,
> 		.mwdma_mask = ATA_MWDMA2,
> 		.udma_mask = ATA_UDMA5,
> 		.port_ops = &hpt374_fn1_port_ops
> 	};
> 
> 	static const int MHz[4] = { 33, 40, 50, 66 };
> 	void *private_data = NULL;
> 	const struct ata_port_info *ppi[] = { NULL, NULL };
> 
> 	u8 irqmask;
> 	u32 class_rev;
> 	u8 mcr1;
> 	u32 freq;
> 	int prefer_dpll = 1;
> 
> 	unsigned long iobase = pci_resource_start(dev, 4);
> 
> 	const struct hpt_chip *chip_table;
> 	int clock_slot;
> 	int rc;
> 
> 	rc = pcim_enable_device(dev);
> 	if (rc)
> 		return rc;
> 
> 	pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
> 	class_rev &= 0xFF;
> 
> 	if (dev->device == PCI_DEVICE_ID_TTI_HPT366) {
> 		/* May be a later chip in disguise. Check */
> 		/* Older chips are in the HPT366 driver. Ignore them */
> 		if (class_rev < 3)
> 			return -ENODEV;
> 		/* N series chips have their own driver. Ignore */
> 		if (class_rev == 6)
> 			return -ENODEV;
> 
> 		switch(class_rev) {
> 			case 3:
> 				ppi[0] = &info_hpt370;
> 				chip_table = &hpt370;
> 				prefer_dpll = 0;
> 				break;
> 			case 4:
> 				ppi[0] = &info_hpt370a;
> 				chip_table = &hpt370a;
> 				prefer_dpll = 0;
> 				break;
> 			case 5:
> 				ppi[0] = &info_hpt372;
> 				chip_table = &hpt372;
> 				break;
> 			default:
> 				printk(KERN_ERR "pata_hpt37x: Unknown HPT366 subtype please report (%d).\n", class_rev);
> 				return -ENODEV;
> 		}
> 	} else {
> 		switch(dev->device) {
> 			case PCI_DEVICE_ID_TTI_HPT372:
> 				/* 372N if rev >= 2*/
> 				if (class_rev >= 2)
> 					return -ENODEV;
> 				ppi[0] = &info_hpt372;
> 				chip_table = &hpt372a;
> 				break;
> 			case PCI_DEVICE_ID_TTI_HPT302:
> 				/* 302N if rev > 1 */
> 				if (class_rev > 1)
> 					return -ENODEV;
> 				ppi[0] = &info_hpt372;
> 				/* Check this */
> 				chip_table = &hpt302;
> 				break;
> 			case PCI_DEVICE_ID_TTI_HPT371:
> 				if (class_rev > 1)
> 					return -ENODEV;
> 				ppi[0] = &info_hpt372;
> 				chip_table = &hpt371;
> 				/* Single channel device, master is not present
> 				   but the BIOS (or us for non x86) must mark it
> 				   absent */
> 				pci_read_config_byte(dev, 0x50, &mcr1);
> 				mcr1 &= ~0x04;
> 				pci_write_config_byte(dev, 0x50, mcr1);
> 				break;
> 			case PCI_DEVICE_ID_TTI_HPT374:
> 				chip_table = &hpt374;
> 				if (!(PCI_FUNC(dev->devfn) & 1))
> 					*ppi = &info_hpt374_fn0;
> 				else
> 					*ppi = &info_hpt374_fn1;
> 				break;
> 			default:
> 				printk(KERN_ERR "pata_hpt37x: PCI table is bogus please report (%d).\n", dev->device);
> 				return -ENODEV;
> 		}
> 	}
> ...
> 
> pata_hpt3x2n.c:
> ...
> /**
>  *	hpt3x2n_init_one		-	Initialise an HPT37X/302
>  *	@dev: PCI device
>  *	@id: Entry in match table
>  *
>  *	Initialise an HPT3x2n device. There are some interesting complications
>  *	here. Firstly the chip may report 366 and be one of several variants.
>  *	Secondly all the timings depend on the clock for the chip which we must
>  *	detect and look up
>  *
>  *	This is the known chip mappings. It may be missing a couple of later
>  *	releases.
>  *
>  *	Chip version		PCI		Rev	Notes
>  *	HPT372			4 (HPT366)	5	Other driver
>  *	HPT372N			4 (HPT366)	6	UDMA133
>  *	HPT372			5 (HPT372)	1	Other driver
>  *	HPT372N			5 (HPT372)	2	UDMA133
>  *	HPT302			6 (HPT302)	*	Other driver
>  *	HPT302N			6 (HPT302)	> 1	UDMA133
>  *	HPT371			7 (HPT371)	*	Other driver
>  *	HPT371N			7 (HPT371)	> 1	UDMA133
>  *	HPT374			8 (HPT374)	*	Other driver
>  *	HPT372N			9 (HPT372N)	*	UDMA133
>  *
>  *	(1) UDMA133 support depends on the bus clock
>  *
>  *	To pin down		HPT371N
>  */
> 
> static int hpt3x2n_init_one(struct pci_dev *dev, const struct pci_device_id *id)
> {
> 	/* HPT372N and friends - UDMA133 */
> 	static const struct ata_port_info info = {
> 		.flags = ATA_FLAG_SLAVE_POSS,
> 		.pio_mask = ATA_PIO4,
> 		.mwdma_mask = ATA_MWDMA2,
> 		.udma_mask = ATA_UDMA6,
> 		.port_ops = &hpt3x2n_port_ops
> 	};
> 	const struct ata_port_info *ppi[] = { &info, NULL };
> 
> 	u8 irqmask;
> 	u32 class_rev;
> 
> 	unsigned int pci_mhz;
> 	unsigned int f_low, f_high;
> 	int adjust;
> 	unsigned long iobase = pci_resource_start(dev, 4);
> 	void *hpriv = NULL;
> 	int rc;
> 
> 	rc = pcim_enable_device(dev);
> 	if (rc)
> 		return rc;
> 
> 	pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
> 	class_rev &= 0xFF;
> 
> 	switch(dev->device) {
> 		case PCI_DEVICE_ID_TTI_HPT366:
> 			if (class_rev < 6)
> 				return -ENODEV;
> 			break;
> 		case PCI_DEVICE_ID_TTI_HPT371:
> 			if (class_rev < 2)
> 				return -ENODEV;
> 			/* 371N if rev > 1 */
> 			break;
> 		case PCI_DEVICE_ID_TTI_HPT372:
> 			/* 372N if rev >= 2*/
> 			if (class_rev < 2)
> 				return -ENODEV;
> 			break;
> 		case PCI_DEVICE_ID_TTI_HPT302:
> 			if (class_rev < 2)
> 				return -ENODEV;
> 			break;
> 		case PCI_DEVICE_ID_TTI_HPT372N:
> 			break;
> 		default:
> 			printk(KERN_ERR "pata_hpt3x2n: PCI table is bogus please report (%d).\n", dev->device);
> 			return -ENODEV;
> 	}
> ...
> 
> 
> and now for the comparison hpt366.c:
> 
> 
> ...
> static const struct hpt_info hpt36x __devinitdata = {
> 	.chip_name	= "HPT36x",
> 	.chip_type	= HPT36x,
> 	.udma_mask	= HPT366_ALLOW_ATA66_3 ? (HPT366_ALLOW_ATA66_4 ? ATA_UDMA4 : ATA_UDMA3) : ATA_UDMA2,
> 	.dpll_clk	= 0,	/* no DPLL */
> 	.timings	= &hpt36x_timings
> };
> 
> static const struct hpt_info hpt370 __devinitdata = {
> 	.chip_name	= "HPT370",
> 	.chip_type	= HPT370,
> 	.udma_mask	= HPT370_ALLOW_ATA100_5 ? ATA_UDMA5 : ATA_UDMA4,
> 	.dpll_clk	= 48,
> 	.timings	= &hpt37x_timings
> };
> 
> static const struct hpt_info hpt370a __devinitdata = {
> 	.chip_name	= "HPT370A",
> 	.chip_type	= HPT370A,
> 	.udma_mask	= HPT370_ALLOW_ATA100_5 ? ATA_UDMA5 : ATA_UDMA4,
> 	.dpll_clk	= 48,
> 	.timings	= &hpt37x_timings
> };
> 
> static const struct hpt_info hpt374 __devinitdata = {
> 	.chip_name	= "HPT374",
> 	.chip_type	= HPT374,
> 	.udma_mask	= ATA_UDMA5,
> 	.dpll_clk	= 48,
> 	.timings	= &hpt37x_timings
> };
> 
> static const struct hpt_info hpt372 __devinitdata = {
> 	.chip_name	= "HPT372",
> 	.chip_type	= HPT372,
> 	.udma_mask	= HPT372_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
> 	.dpll_clk	= 55,
> 	.timings	= &hpt37x_timings
> };
> 
> static const struct hpt_info hpt372a __devinitdata = {
> 	.chip_name	= "HPT372A",
> 	.chip_type	= HPT372A,
> 	.udma_mask	= HPT372_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
> 	.dpll_clk	= 66,
> 	.timings	= &hpt37x_timings
> };
> 
> static const struct hpt_info hpt302 __devinitdata = {
> 	.chip_name	= "HPT302",
> 	.chip_type	= HPT302,
> 	.udma_mask	= HPT302_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
> 	.dpll_clk	= 66,
> 	.timings	= &hpt37x_timings
> };
> 
> static const struct hpt_info hpt371 __devinitdata = {
> 	.chip_name	= "HPT371",
> 	.chip_type	= HPT371,
> 	.udma_mask	= HPT371_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
> 	.dpll_clk	= 66,
> 	.timings	= &hpt37x_timings
> };
> 
> static const struct hpt_info hpt372n __devinitdata = {
> 	.chip_name	= "HPT372N",
> 	.chip_type	= HPT372N,
> 	.udma_mask	= HPT372_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
> 	.dpll_clk	= 77,
> 	.timings	= &hpt37x_timings
> };
> 
> static const struct hpt_info hpt302n __devinitdata = {
> 	.chip_name	= "HPT302N",
> 	.chip_type	= HPT302N,
> 	.udma_mask	= HPT302_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
> 	.dpll_clk	= 77,
> 	.timings	= &hpt37x_timings
> };
> 
> static const struct hpt_info hpt371n __devinitdata = {
> 	.chip_name	= "HPT371N",
> 	.chip_type	= HPT371N,
> 	.udma_mask	= HPT371_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
> 	.dpll_clk	= 77,
> 	.timings	= &hpt37x_timings
> };
> ...
> /**
>  *	hpt366_init_one	-	called when an HPT366 is found
>  *	@dev: the hpt366 device
>  *	@id: the matching pci id
>  *
>  *	Called when the PCI registration layer (or the IDE initialization)
>  *	finds a device matching our IDE device tables.
>  */
> static int __devinit hpt366_init_one(struct pci_dev *dev, const struct pci_device_id *id)
> {
> 	const struct hpt_info *info = NULL;
> 	struct hpt_info *dyn_info;
> 	struct pci_dev *dev2 = NULL;
> 	struct ide_port_info d;
> 	u8 idx = id->driver_data;
> 	u8 rev = dev->revision;
> 	int ret;
> 
> 	if ((idx == 0 || idx == 4) && (PCI_FUNC(dev->devfn) & 1))
> 		return -ENODEV;
> 
> 	switch (idx) {
> 	case 0:
> 		if (rev < 3)
> 			info = &hpt36x;
> 		else {
> 			switch (min_t(u8, rev, 6)) {
> 			case 3: info = &hpt370;  break;
> 			case 4: info = &hpt370a; break;
> 			case 5: info = &hpt372;  break;
> 			case 6: info = &hpt372n; break;
> 			}
> 			idx++;
> 		}
> 		break;
> 	case 1:
> 		info = (rev > 1) ? &hpt372n : &hpt372a;
> 		break;
> 	case 2:
> 		info = (rev > 1) ? &hpt302n : &hpt302;
> 		break;
> 	case 3:
> 		hpt371_init(dev);
> 		info = (rev > 1) ? &hpt371n : &hpt371;
> 		break;
> 	case 4:
> 		info = &hpt374;
> 		break;
> 	case 5:
> 		info = &hpt372n;
> 		break;
> 	}
> ...

On the second look it seems pata_ drivers also have something similar
to struct hpt_info (it is called struct hpt_chip) so in reality only
hpt366_init_one() code matters.

> Only pata_hpt37x has broken cable detection while pata_hpt3x2n is
> okay. ;)

I think you have that backwards according to the vendor drivers. 3x2n
looks like it needs adjusting.
 

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> BTW we can immediately reclaim 300 LOC by simply merging drivers back..

LoC - lines of comments ? (at least half of them).

Been testing a bit before fixing up the pata_hpt drivers. Resume from S2R
doesn't work with drivers/ide/hpt366.c. So it may have code for resume
but it doesn't actually work for all the chips.

Reason is fairly obvious - the PCI speed detection logic is broken except
for the case of a PC boot where the ROM BIOS for the HPT3xx is run. That
doesn't occur on a resume from RAM so a 66MHz clocked motherboard device
comes back with the speed guessed wrongly.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Cox wrote:

>>BTW we can immediately reclaim 300 LOC by simply merging drivers back..

> LoC - lines of comments ? (at least half of them).

> Been testing a bit before fixing up the pata_hpt drivers. Resume from S2R
> doesn't work with drivers/ide/hpt366.c. So it may have code for resume
> but it doesn't actually work for all the chips.

> Reason is fairly obvious - the PCI speed detection logic is broken except
> for the case of a PC boot where the ROM BIOS for the HPT3xx is run. That

    Point out the breakage please. The driver has been developed and tested 
on non-x86 machine with 66 MHz PCI.

> doesn't occur on a resume from RAM so a 66MHz clocked motherboard device
> comes back with the speed guessed wrongly.

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thursday 19 November 2009 14:16:36 Alan Cox wrote:
> > Only pata_hpt37x has broken cable detection while pata_hpt3x2n is
> > okay. ;)
> 
> I think you have that backwards according to the vendor drivers. 3x2n
> looks like it needs adjusting.

lol, just fix the damn pata_hpt37x driver _lacking_ ->cable_detect method..

On Thursday 19 November 2009 15:02:59 Alan Cox wrote:
> > BTW we can immediately reclaim 300 LOC by simply merging drivers back..
> 
> LoC - lines of comments ? (at least half of them).

lol, I _only_ counted whole functions + their documentation to come up with
this fairly _conservative_ number...

>     Point out the breakage please. The driver has been developed and tested 
> on non-x86 machine with 66 MHz PCI.

static int init_chipset_hpt366(struct pci_dev *dev)

        if (chip_type >= HPT370) {

               if ((temp & 0xFFFFF000) != 0xABCDE000) {

it then falls through and gets a 33MHz answer from the tests.


If I stash the MHz answer from boot then it works. So I imagine the boot
time value just needs to be dumped into a static for resume to use.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 19 Nov 2009 15:17:14 +0100
Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:

> On Thursday 19 November 2009 14:16:36 Alan Cox wrote:
> > > Only pata_hpt37x has broken cable detection while pata_hpt3x2n is
> > > okay. ;)
> > 
> > I think you have that backwards according to the vendor drivers. 3x2n
> > looks like it needs adjusting.
> 
> lol, just fix the damn pata_hpt37x driver _lacking_ ->cable_detect method..

It doesn't need one. It still sets the cable type in the pre reset
function as the older drivers that haven't converted to a cable_detect
method do.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Cox wrote:

>>    Point out the breakage please. The driver has been developed and tested 
>>on non-x86 machine with 66 MHz PCI.

> static int init_chipset_hpt366(struct pci_dev *dev)
> 
>         if (chip_type >= HPT370) {
> 
>                if ((temp & 0xFFFFF000) != 0xABCDE000) {

> it then falls through and gets a 33MHz answer from the tests.

> If I stash the MHz answer from boot then it works. So I imagine the boot
> time value just needs to be dumped into a static for resume to use.

    Ah, that... sure, this test only works at boot if there was no smart 
enough BIOS around to save the f_CNT -- then the DPLL gets reprogrammed, and 
doesn't reflect the default clock anymore...

> Alan

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thursday 19 November 2009 15:33:51 Alan Cox wrote:
> On Thu, 19 Nov 2009 15:17:14 +0100
> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> 
> > On Thursday 19 November 2009 14:16:36 Alan Cox wrote:
> > > > Only pata_hpt37x has broken cable detection while pata_hpt3x2n is
> > > > okay. ;)
> > > 
> > > I think you have that backwards according to the vendor drivers. 3x2n
> > > looks like it needs adjusting.
> > 
> > lol, just fix the damn pata_hpt37x driver _lacking_ ->cable_detect method..
> 
> It doesn't need one. It still sets the cable type in the pre reset
> function as the older drivers that haven't converted to a cable_detect
> method do.

If you know about other drivers still using ->pre_reset for cable detection
please let us know because they need fixing ASAP.

->cable_detect method is there for a reason, by knowingly not using it you
already have a buggy cable detection (since ->pre_reset ignores the mandatory
by spec part of cable detection which is probing slave before master) and
make it more likely to cause breakages in the future when some other developer
do a change under assumption that all host drivers use API correctly.

On Thursday 19 November 2009 15:50:25 Bartlomiej Zolnierkiewicz wrote:
> On Thursday 19 November 2009 15:33:51 Alan Cox wrote:
> > On Thu, 19 Nov 2009 15:17:14 +0100
> > Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> > 
> > > On Thursday 19 November 2009 14:16:36 Alan Cox wrote:
> > > > > Only pata_hpt37x has broken cable detection while pata_hpt3x2n is
> > > > > okay. ;)
> > > > 
> > > > I think you have that backwards according to the vendor drivers. 3x2n
> > > > looks like it needs adjusting.
> > > 
> > > lol, just fix the damn pata_hpt37x driver _lacking_ ->cable_detect method..
> > 
> > It doesn't need one. It still sets the cable type in the pre reset
> > function as the older drivers that haven't converted to a cable_detect
> > method do.
> 
> If you know about other drivers still using ->pre_reset for cable detection
> please let us know because they need fixing ASAP.

No need for it any longer.  I'll send patches later.

> If you know about other drivers still using ->pre_reset for cable detection
> please let us know because they need fixing ASAP.
> 
> ->cable_detect method is there for a reason,

Given that I added it I may know more about it than you do. In fact from
the rubbish you spout below it seems I do.

> already have a buggy cable detection (since ->pre_reset ignores the mandatory

Wrong.

> by spec part of cable detection which is probing slave before master) and

Have a free hint. If the host detects the cable type then we don't ask
the drive. See the standard if you don't understand why. Even if we
didn't the code would still be correct because we properly evaluate
the speed configuration from all the data sources.

> make it more likely to cause breakages in the future when some other developer
> do a change under assumption that all host drivers use API correctly.

To quote Bartlomiej "Talk to the maintainer" (or use grep).

Alan

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> > If you know about other drivers still using ->pre_reset for cable detection
> > please let us know because they need fixing ASAP.
> 
> No need for it any longer.  I'll send patches later.

Works for me - not a bug fix as you make out but it's a clean up that is
worth doing.
 
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thursday 19 November 2009 16:22:45 Alan Cox wrote:
> > If you know about other drivers still using ->pre_reset for cable detection
> > please let us know because they need fixing ASAP.
> > 
> > ->cable_detect method is there for a reason,
> 
> Given that I added it I may know more about it than you do. In fact from
> the rubbish you spout below it seems I do.

lol, I always heard that debugging other people's code is harder than
writing it in the first place..

> > already have a buggy cable detection (since ->pre_reset ignores the mandatory
> 
> Wrong.

You cannot know it unless you know how chip operates internally.  That's it.

You are taking chances that the controller does what most of similar hardware
do.  Unfortunately we have seen so many counterexamples of this in the past
(i.e. I wouldn't be so surprised if some hosts just snoop IDENTIFY data to get
their cable info) that I prefer to stick to safe approaches.

Especially since it cost us nothing and provides additional benefit of having
coherent API.

> > by spec part of cable detection which is probing slave before master) and
> 
> Have a free hint. If the host detects the cable type then we don't ask
> the drive. See the standard if you don't understand why. Even if we
> didn't the code would still be correct because we properly evaluate
> the speed configuration from all the data sources.

Please spare your 'free hints' and preaching tone.   You've completely failed
over four years span to out do the messy code even with like ~1.5 year handicap
to finalize the hostile takeover.

I'm completely fed up with the process and I'm simply fixing up your mess now,
50+ patches and counting.  Turns out to be order of magnitude more productive
than even trying to discuss things with you and/or your influential friends.

> > > already have a buggy cable detection (since ->pre_reset ignores the mandatory
> > 
> > Wrong.
> 
> You cannot know it unless you know how chip operates internally.  That's it.

Read the code. How the chip operates is irrelevant.

> You are taking chances that the controller does what most of similar hardware
> do.  Unfortunately we have seen so many counterexamples of this in the past
> (i.e. I wouldn't be so surprised if some hosts just snoop IDENTIFY data to get
> their cable info) that I prefer to stick to safe approaches.

I would be very suprised indeed because neither IDE stack nor the
reference drivers would work in that case.

Even then switch to cable_detect would make no difference. Read the code.

> > Have a free hint. If the host detects the cable type then we don't ask
> > the drive. See the standard if you don't understand why. Even if we
> > didn't the code would still be correct because we properly evaluate
> > the speed configuration from all the data sources.
> 
> Please spare your 'free hints' and preaching tone.

If you don't know or follow the standards you'll get bugs. Thats why I
went to so much trouble researching and digging out the rules on cable
detect in detail, and fixing them in libata so that unlike the old IDE
stack they got them right (and more stuff probes correctly). (And Davem -
those did partly - drive ordering at least get pushed into old IDE too)

The reason it doesn't matter beyond style is this. Libata doesn't mangle
drive and host cable state into one thing. The ap->cbl field reflects the
cable type as measured by the controller. The forty wire decision is made
later on a combination of both controller and drive information, trusting
the controller first because the controller is the correct source if it
knows the information (as per the spec). [1]

Thus all the rubbish you spouted earlier doesn't matter at all. The
cable_detect logic was very carefully added so it didn't break back
compatibility with old drivers or re-order stuff wrongly.

For the other stuf "Talk to the maintainer" to quote yourself. I've
been working on other things for the past couple of years.

Alan
[1] The one specific exception here is the SATA cable setting when a
drive reports that it is SATA. There isn't an obvious way to de-uglify
that aspect of it.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thursday 19 November 2009 17:27:38 Alan Cox wrote:

> For the other stuf "Talk to the maintainer" to quote yourself. I've
> been working on other things for the past couple of years.

Indeed.

I wonder for how long we will be recovering from your help in TTY...

On 11/18/2009 01:19 PM, Bartlomiej Zolnierkiewicz wrote:
> On Tuesday 17 November 2009 15:51:39 Alan Cox wrote:
>> Signed-off-by: Alan Cox<alan@linux.intel.com>
>> ---
>>
>>   drivers/ata/Kconfig |    8 ++++----
>>   1 files changed, 4 insertions(+), 4 deletions(-)
>>
>>
>> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
>> index f2df6e2..36931e0 100644
>> --- a/drivers/ata/Kconfig
>> +++ b/drivers/ata/Kconfig
>> @@ -374,7 +374,7 @@ config PATA_HPT366
>>   	  If unsure, say N.
>>
>>   config PATA_HPT37X
>> -	tristate "HPT 370/370A/371/372/374/302 PATA support (Experimental)"
>> +	tristate "HPT 370/370A/371/372/374/302 PATA support"
>>   	depends on PCI&&  EXPERIMENTAL
>>   	help
>>   	  This option enables support for the majority of the later HPT
>> @@ -383,7 +383,7 @@ config PATA_HPT37X
>>   	  If unsure, say N.
>>
>>   config PATA_HPT3X2N
>> -	tristate "HPT 372N/302N PATA support (Experimental)"
>> +	tristate "HPT 372N/302N PATA support"
>>   	depends on PCI&&  EXPERIMENTAL
>>   	help
>>   	  This option enables support for the N variant HPT PATA
>
> Maybe they are 'stable' but when it comes to features they are behind hpt366
> (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
> to understand and much smaller..

That sounds like an ACK to Alan's patch, to me ;-)

A libata driver can be stable, yet not have all the features of drivers/ide.

That said, I do agree that libata drivers need to have the features 
found in the drivers/ide/ drivers.

	Jeff



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 11/18/2009 02:56 PM, Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 18 November 2009 20:07:07 Bartlomiej Zolnierkiewicz wrote:
>> On Wednesday 18 November 2009 19:41:25 Alan Cox wrote:
>>>> Maybe they are 'stable' but when it comes to features they are behind hpt366
>>>> (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
>>>> to understand and much smaller..
>>>
>>> 37x and 3xn lack PCI PM.  Added to the TODO list.
>>>
>>> The smaller size is a bit questionable given its mostly comments, and
>>> three drivers versus one.
>>
>> I wonder what you find 'questionable' in the numbers given..
>
> BTW we can immediately reclaim 300 LOC by simply merging drivers back..

Merging drivers is a big pain for distributions and end users, even with 
the new module_alias facility.

Absent stronger justification, libata remains with separate drivers, 
even if that means some code duplication.

	Jeff



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thursday 19 November 2009 22:36:50 Jeff Garzik wrote:
> On 11/18/2009 01:19 PM, Bartlomiej Zolnierkiewicz wrote:
> > On Tuesday 17 November 2009 15:51:39 Alan Cox wrote:
> >> Signed-off-by: Alan Cox<alan@linux.intel.com>
> >> ---
> >>
> >>   drivers/ata/Kconfig |    8 ++++----
> >>   1 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >>
> >> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> >> index f2df6e2..36931e0 100644
> >> --- a/drivers/ata/Kconfig
> >> +++ b/drivers/ata/Kconfig
> >> @@ -374,7 +374,7 @@ config PATA_HPT366
> >>   	  If unsure, say N.
> >>
> >>   config PATA_HPT37X
> >> -	tristate "HPT 370/370A/371/372/374/302 PATA support (Experimental)"
> >> +	tristate "HPT 370/370A/371/372/374/302 PATA support"
> >>   	depends on PCI&&  EXPERIMENTAL
> >>   	help
> >>   	  This option enables support for the majority of the later HPT
> >> @@ -383,7 +383,7 @@ config PATA_HPT37X
> >>   	  If unsure, say N.
> >>
> >>   config PATA_HPT3X2N
> >> -	tristate "HPT 372N/302N PATA support (Experimental)"
> >> +	tristate "HPT 372N/302N PATA support"
> >>   	depends on PCI&&  EXPERIMENTAL
> >>   	help
> >>   	  This option enables support for the N variant HPT PATA
> >
> > Maybe they are 'stable' but when it comes to features they are behind hpt366
> > (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
> > to understand and much smaller..
> 
> That sounds like an ACK to Alan's patch, to me ;-)
> 
> A libata driver can be stable, yet not have all the features of drivers/ide.
> 
> That said, I do agree that libata drivers need to have the features 
> found in the drivers/ide/ drivers.

Feel free to add them Mr. Maintainer. :)

On Thursday 19 November 2009 22:38:39 Jeff Garzik wrote:
> On 11/18/2009 02:56 PM, Bartlomiej Zolnierkiewicz wrote:
> > On Wednesday 18 November 2009 20:07:07 Bartlomiej Zolnierkiewicz wrote:
> >> On Wednesday 18 November 2009 19:41:25 Alan Cox wrote:
> >>>> Maybe they are 'stable' but when it comes to features they are behind hpt366
> >>>> (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
> >>>> to understand and much smaller..
> >>>
> >>> 37x and 3xn lack PCI PM.  Added to the TODO list.
> >>>
> >>> The smaller size is a bit questionable given its mostly comments, and
> >>> three drivers versus one.
> >>
> >> I wonder what you find 'questionable' in the numbers given..
> >
> > BTW we can immediately reclaim 300 LOC by simply merging drivers back..
> 
> Merging drivers is a big pain for distributions and end users, even with 
> the new module_alias facility.

Could please explain in more detail 'a big pain' part?

> Absent stronger justification, libata remains with separate drivers, 
> even if that means some code duplication.

I can always hold my own tree with such changes if needed.

On 11/19/2009 04:42 PM, Bartlomiej Zolnierkiewicz wrote:
> On Thursday 19 November 2009 22:36:50 Jeff Garzik wrote:
>> On 11/18/2009 01:19 PM, Bartlomiej Zolnierkiewicz wrote:
>>> On Tuesday 17 November 2009 15:51:39 Alan Cox wrote:
>>>> Signed-off-by: Alan Cox<alan@linux.intel.com>
>>>> ---
>>>>
>>>>    drivers/ata/Kconfig |    8 ++++----
>>>>    1 files changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>>
>>>> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
>>>> index f2df6e2..36931e0 100644
>>>> --- a/drivers/ata/Kconfig
>>>> +++ b/drivers/ata/Kconfig
>>>> @@ -374,7 +374,7 @@ config PATA_HPT366
>>>>    	  If unsure, say N.
>>>>
>>>>    config PATA_HPT37X
>>>> -	tristate "HPT 370/370A/371/372/374/302 PATA support (Experimental)"
>>>> +	tristate "HPT 370/370A/371/372/374/302 PATA support"
>>>>    	depends on PCI&&   EXPERIMENTAL
>>>>    	help
>>>>    	  This option enables support for the majority of the later HPT
>>>> @@ -383,7 +383,7 @@ config PATA_HPT37X
>>>>    	  If unsure, say N.
>>>>
>>>>    config PATA_HPT3X2N
>>>> -	tristate "HPT 372N/302N PATA support (Experimental)"
>>>> +	tristate "HPT 372N/302N PATA support"
>>>>    	depends on PCI&&   EXPERIMENTAL
>>>>    	help
>>>>    	  This option enables support for the N variant HPT PATA
>>>
>>> Maybe they are 'stable' but when it comes to features they are behind hpt366
>>> (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
>>> to understand and much smaller..
>>
>> That sounds like an ACK to Alan's patch, to me ;-)
>>
>> A libata driver can be stable, yet not have all the features of drivers/ide.
>>
>> That said, I do agree that libata drivers need to have the features
>> found in the drivers/ide/ drivers.
>
> Feel free to add them Mr. Maintainer. :)

Oh, I'm happy to wait until one of two scenarios occurs:

1) Bart annoys Alan sufficiently to motivate Alan to add feature X
2) Alan annoys Bart sufficiently to motivate Bart to add feature X

Personally, I think my two PATA co-maintainers are doing an excellent 
job of finding and killing bugs, and adding features, even if the 
mailing list traffic is contentious.

I'm happy as long as the users win in the end...

	Jeff



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 11/19/2009 04:49 PM, Bartlomiej Zolnierkiewicz wrote:
> On Thursday 19 November 2009 22:38:39 Jeff Garzik wrote:
>> On 11/18/2009 02:56 PM, Bartlomiej Zolnierkiewicz wrote:
>>> On Wednesday 18 November 2009 20:07:07 Bartlomiej Zolnierkiewicz wrote:
>>>> On Wednesday 18 November 2009 19:41:25 Alan Cox wrote:
>>>>>> Maybe they are 'stable' but when it comes to features they are behind hpt366
>>>>>> (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
>>>>>> to understand and much smaller..
>>>>>
>>>>> 37x and 3xn lack PCI PM.  Added to the TODO list.
>>>>>
>>>>> The smaller size is a bit questionable given its mostly comments, and
>>>>> three drivers versus one.
>>>>
>>>> I wonder what you find 'questionable' in the numbers given..
>>>
>>> BTW we can immediately reclaim 300 LOC by simply merging drivers back..
>>
>> Merging drivers is a big pain for distributions and end users, even with
>> the new module_alias facility.
>
> Could please explain in more detail 'a big pain' part?

It makes issue tracking (bug reports, feature requests, etc.) more 
difficult.  The "where did my driver go?" issue, which is largely a 
communication/education not technical problem.  Manually specifying 
drivers in /etc/modprobe.conf remains supported.


>> Absent stronger justification, libata remains with separate drivers,
>> even if that means some code duplication.
>
> I can always hold my own tree with such changes if needed.

If it really bugs you, you could take the drivers/ata/sata_promise.h 
approach, or do multi-file modules.

	Jeff



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> Oh, I'm happy to wait until one of two scenarios occurs:
> 
> 1) Bart annoys Alan sufficiently to motivate Alan to add feature X

I'm rather hard to annoy but I'm happy Bart is going over the drivers.
Whatever his motivation I am very sure he'll find more bugs in the
process. He'll even be able to correct blame me for a few I am sure.

> 2) Alan annoys Bart sufficiently to motivate Bart to add feature X

Well latest score is Bart fixes libata bug, Alan fixes the IDE tree
version. Not quite the expected outcome but who cares ;)

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> > Merging drivers is a big pain for distributions and end users, even with 
> > the new module_alias facility.
> 
> Could please explain in more detail 'a big pain' part?

It might confuse the Fedora intall a bit but it shouldn't cause much
confusion.

> > Absent stronger justification, libata remains with separate drivers, 
> > even if that means some code duplication.
> 
> I can always hold my own tree with such changes if needed.

My view is that they should be separate because of all the stuff like the
hpt3x2n clocking. I've got some updates to hpt3x2n pending which I'll
post as untested too.

If you are going to try merging them then the proof will be in what the
merge looks like. If you are going to do it anyway it's got to be worth
making that decision after the merge is done.

Alan

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thursday 19 November 2009 11:03:29 pm Jeff Garzik wrote:
> On 11/19/2009 04:49 PM, Bartlomiej Zolnierkiewicz wrote:

> >> Absent stronger justification, libata remains with separate drivers,
> >> even if that means some code duplication.
> >
> > I can always hold my own tree with such changes if needed.
> 
> If it really bugs you, you could take the drivers/ata/sata_promise.h 
> approach, or do multi-file modules.

One of the main problems with separate drivers is a needless complexity
of the detection logic (different chips share same PCI IDs) and relevant
code parts presenting the issue were already posted in this thread.

PS It would bug me really if I were the author or the maintainer of said
code but since I'm not I'll get to fixing it only when I'm bored enough..

--
Bartlomiej Zolnierkiewicz
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html