Hello,
Generally looks good. Some questions and suggestions follow.
Felix Domke wrote:
> Index: linux-2.6.20/drivers/ata/libata-core.c
> ===================================================================
> --- linux-2.6.20.orig/drivers/ata/libata-core.c 2007-03-07
> 19:01:12.000000000 +0100
> +++ linux-2.6.20/drivers/ata/libata-core.c 2007-03-07 19:01:22.000000000
> +0100
> @@ -1478,7 +1478,7 @@
> }
>
> tf.protocol = ATA_PROT_PIO;
> - tf.flags |= ATA_TFLAG_POLLING; /* for polling presence detection */
> +// tf.flags |= ATA_TFLAG_POLLING; /* for polling presence detection */
Polling IDENTIFY doesn't work for you? Care to explain how it's broken?
> Index: linux-2.6.20/drivers/ata/sata_xenon.c
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.20/drivers/ata/sata_xenon.c 2007-03-07 19:01:22.000000000
> +0100
> + Note on the DVD-ROM part:
> +
> + The drives usually require some tweaks to be usable under linux.
> +
> + You either need to hack the scsi layer, or, in case of the GDR3120L,
> + set 'modeB' in the bootloader.
Care to explain it further? Or is it some cryptic embedded stuff?
> +extern struct ata_probe_ent *ata_probe_ent_alloc(struct device *dev,
> + const struct ata_port_info *port);
This is an internal function and if you do this, your driver won't build
as a kernel module. Please do as other drivers do. probe_ent is in the
process of being removed, so looking ugly is okay for the time being.
> +static u32 xenon_scr_cfg_read (struct ata_port *ap, unsigned int sc_reg)
> +{
> + struct pci_dev *pdev = to_pci_dev(ap->host->dev);
> + unsigned int cfg_addr = get_scr_cfg_addr(ap->port_no, sc_reg,
> pdev->device);
> + u32 val;
> +
> + if (sc_reg == SCR_ERROR) /* doesn't exist in PCI cfg space */
> + return 0; /* assume no error */
Interesting, SError isn't there while other regs are?
> +static u32 xenon_scr_read (struct ata_port *ap, unsigned int sc_reg)
> +{
> + if (sc_reg > SCR_CONTROL)
> + return 0xffffffffU;
> +
> + return xenon_scr_cfg_read(ap, sc_reg);
> +}
Just collapse xenon_scr_cfg_read() into xenon_scr_read().
> +static void xenon_scr_write (struct ata_port *ap, unsigned int sc_reg,
> u32 val)
> +{
> + if (sc_reg > SCR_CONTROL)
> + return;
> +
> + xenon_scr_cfg_write(ap, sc_reg, val);
> +}
Ditto for write.
> +static int xenon_init_one (struct pci_dev *pdev, const struct
> pci_device_id *ent)
> +{
> + static int printed_version;
> + struct ata_probe_ent *probe_ent = NULL;
> + int rc;
> + int pci_dev_busy = 0;
> +
> + if (!printed_version++)
> + dev_printk(KERN_INFO, &pdev->dev, "version " DRV_VERSION "\n");
> +
> + rc = pci_enable_device(pdev);
> + if (rc)
> + return rc;
New drivers are generally accepted into libata-dev#upstream branch, and
it looks a bit different in the init path. If you're a gitter, please
generate patch against libata-dev#upstream, if not the latest -mm should
be good enough.
Thanks.
--
tejun
-
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
