Alek Du wrote:
This patch adds Intel SCH chipsets
Looks good, yet there's some nits...
(US15W, US15L, UL11L) PATA controller support.
From the Intel's documentation I got the impression that the part numbers
start with AF82 (82 being the usual Intel's chip number prefix).
Signed-off-by: Alek Du <alek.du@xxxxxxxxx>
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index b693d82..ec589e2 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_ATA) += libata.o
obj-$(CONFIG_SATA_AHCI) += ahci.o
obj-$(CONFIG_SATA_SVW) += sata_svw.o
obj-$(CONFIG_ATA_PIIX) += ata_piix.o
+obj-$(CONFIG_ATA_SCH) += ata_sch.o
The name of the drivers for the pure PATA chips are prefixed with 'pata_',
not 'ata_'.
obj-$(CONFIG_SATA_PROMISE) += sata_promise.o
obj-$(CONFIG_SATA_QSTOR) += sata_qstor.o
obj-$(CONFIG_SATA_SIL) += sata_sil.o
diff --git a/drivers/ata/ata_sch.c b/drivers/ata/ata_sch.c
new file mode 100644
index 0000000..2c7b358
--- /dev/null
+++ b/drivers/ata/ata_sch.c
@@ -0,0 +1,271 @@
[...]
+/* see SCH datasheet page 351 */
+enum {
+ D0TIM = 0x80, /* Device 0 Timing Register */
+ D1TIM = 0x84, /* Device 1 Timing Register */
+ PIO = 0x07, /* PIO Mode Bit Mask */
The SCH datasheet calls this field PM and since you've got all other names
form there, why not call it PM too?
+ MDM = (0x03 << 8), /* Multi-word DMA Mode Bit Mask */
+ UDM = (0x07 << 16), /* Ultra DMA Mode Bit Mask */
+ PPE = (1 << 30), /* Prefetch/Post Enable */
+ USD = (1 << 31), /* Use Synchronous DMA */
+};
+enum sch_controller_ids {
+ /* controller IDs */
+ sch_pata_100, /* SCH up to UDMA 100 */
+};
I don't see why you need the above.
+static unsigned int in_module_init = 1;
+
+static struct ata_port_operations sch_pata_ops = {
+ .inherits = &ata_bmdma_port_ops,
+ .cable_detect = ata_cable_unknown,
+ .set_piomode = sch_set_piomode,
+ .set_dmamode = sch_set_dmamode,
+ .prereset = ata_sff_prereset,
ata_sff_prereset is the default method, why not just inherit it?
+/**
+ * sch_set_piomode - Initialize host controller PATA PIO timings
+ * @ap: Port whose timings we are configuring
+ * @adev: um
+ *
+ * Set PIO mode for device, in host controller PCI config space.
+ *
+ * LOCKING:
+ * None (inherited from caller).
+ */
+
+static void sch_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+ unsigned int pio = adev->pio_mode - XFER_PIO_0;
+ struct pci_dev *dev = to_pci_dev(ap->host->dev);
+ unsigned int port = adev->devno ? D1TIM : D0TIM;
+ unsigned int data;
+
+ /* SCH only support primary channel with one master and one slave*/
+ if (ap->port_no != 0)
+ return;
There should be no references to the secondary channel from libata since
you should explicitly specify that there is no secondary channel.
+
+ pci_read_config_dword(dev, port, &data);
+ /* see SCH datasheet page 351 */
+ /* enable PIO with PPE*/
No space before */...
+ data &= ~(PIO);
Unneeded parens.
+ data |= (PPE |pio);
You shouldn't enable prefetch for ATAPI deives I think -- look at what
ata_piix does...
Also, unneeded parens, no space after | operator. Be consistent please.
+ /* mask reserved bits */
Why?
+ data = data & (USD|PPE|UDM|MDM|PIO);
No spaces between | operator and operands.
+ pci_write_config_dword(dev, port, data);
+}
+
+/**
+ * sch_set_dmamode - Initialize host controller PATA DMA timings
+ * @ap: Port whose timings we are configuring
+ * @adev: um
+ *
+ * Set MW/UDMA mode for device, in host controller PCI config space.
+ *
+ * LOCKING:
+ * None (inherited from caller).
+ */
+
+static void sch_set_dmamode(struct ata_port *ap, struct ata_device *adev)
+{
+ unsigned int dma_mode = adev->dma_mode;
+ struct pci_dev *dev = to_pci_dev(ap->host->dev);
+ unsigned int port = adev->devno ? D1TIM : D0TIM;
+ unsigned int data;
+
+ /* SCH only support primary channel with one master and one slave*/
+ if (ap->port_no != 0)
+ return;
Same comment as in sch_set_piomode().
+
+ pci_read_config_dword(dev, port, &data);
+ /* see SCH datasheet page 351 */
+ if (dma_mode >= XFER_UDMA_0) {
+ /* enable Synchronous DMA mode */
+ data |= USD;
+ data &= ~(UDM);
Unneeded parens.
+ data |= (dma_mode - XFER_UDMA_0) << 16;
+ } else { /* must be MWDMA mode, since we masked SWDMA already */
+ data &= ~(USD|MDM);
No spaces between | and operands.
+ data |= (dma_mode - XFER_MW_DMA_0) << 8;
+ }
+ /* mask reserved bits */
Why again?
+ data = data & (USD|PPE|UDM|MDM|PIO);
No spaces between | and operands.
MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
