Re: [PATCH] ata: add CONFIG_SATA_HOST config option

From: Sergei Shtylyov
Date: Wed Feb 09 2011 - 14:59:36 EST


Hello.

Bartlomiej Zolnierkiewicz wrote:

Add CONFIG_SATA_HOST config option (for selecting SATA Host
support) to make setup easier on PATA-only systems.

Additionally this allows us to save ~9.5k of the output code
size (x86-64) on such systems:

text data bss dec hex filename
49909 6576 57 56542 dcde drivers/ata/libata-core.o.before
31862 16 2 31880 7c88 drivers/ata/libata-eh.o.before
20256 0 19 20275 4f33 drivers/ata/libata-sff.o.before
44495 6576 57 51128 c7b8 drivers/ata/libata-core.o.after
27831 16 2 27849 6cc9 drivers/ata/libata-eh.o.after
20144 0 19 20163 4ec3 drivers/ata/libata-sff.o.after

Good job!

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
---
I also wonder if SATA code should be moved to libata-sata.c (to avoid ifdefs)?

drivers/ata/Kconfig | 25 ++++++++--
drivers/ata/libata-core.c | 107 +++++++++++++++++++++++++++++++++++++++-------
drivers/ata/libata-eh.c | 21 ++++++++-
drivers/ata/libata-sff.c | 8 +++
4 files changed, 141 insertions(+), 20 deletions(-)

Index: b/drivers/ata/Kconfig
===================================================================
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
[...]
@@ -122,7 +133,7 @@ comment "SFF controllers with custom DMA
config PDC_ADMA
tristate "Pacific Digital ADMA support"
- depends on PCI
+ depends on PCI && SATA_HOST

I thought ADMA spec. was designed for PATA... are you sure this should depend on CONFIG_SATA_HOST?

@@ -165,8 +176,6 @@ config ATA_BMDMA
if ATA_BMDMA
-comment "SATA SFF controllers with BMDMA"
-
config ATA_PIIX
tristate "Intel ESB, ICH, PIIX3, PIIX4 PATA/SATA support"
depends on PCI
@@ -177,6 +186,10 @@ config ATA_PIIX
If unsure, say N.
+if SATA_HOST
+
+comment "SATA/PATA SFF controllers with BMDMA"

How it can be "SATA/PATA" if it's only enabled when CONFIG_SATA_HOST=y?

Index: b/drivers/ata/libata-core.c
===================================================================
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
[...]
@@ -601,6 +609,9 @@ void ata_tf_from_fis(const u8 *fis, stru
tf->nsect = fis[12];
tf->hob_nsect = fis[13];
}
+#else
+void ata_tf_from_fis(const u8 *fis, struct ata_taskfile *tf) { }
+#endif

Is it really ever called by non-SATA code? Ah, it's exported at end of file and so should be defined... isn't it better to carry over the export from the bottom of the file (and thus silence one of checkpatch.pl's warnings :-).

@@ -2690,6 +2718,7 @@ struct ata_device *ata_dev_pair(struct a
return pair;
}
+#ifdef CONFIG_SATA_HOST
/**
* sata_down_spd_limit - adjust SATA spd limit downward
* @link: Link to adjust SATA spd limit for
@@ -2843,6 +2872,18 @@ int sata_set_spd(struct ata_link *link)
return 1;
}
+#else
[...]
+int sata_set_spd(struct ata_link *link)
+{
+ return 0;
+}

Again, I don't think that's called by non-SATA code...

+#endif
+EXPORT_SYMBOL_GPL(sata_set_spd);

Here you're carrying over the export...

@@ -3436,6 +3477,7 @@ int ata_wait_after_reset(struct ata_link
return ata_wait_ready(link, deadline, check_ready);
}
+#ifdef CONFIG_SATA_HOST
/**
* sata_link_debounce - debounce SATA phy status
* @link: ATA link to debounce SATA phy status for
@@ -3638,6 +3680,26 @@ int sata_link_scr_lpm(struct ata_link *l
ehc->i.serror &= ~SERR_PHYRDY_CHG;
return sata_scr_write(link, SCR_ERROR, SERR_PHYRDY_CHG);
}
+#else
+int sata_link_debounce(struct ata_link *link, const unsigned long *params,
+ unsigned long deadline)
+{
+ return -EOPNOTSUPP;
+}

Again, not called by non-SATA code...

+int sata_link_resume(struct ata_link *link, const unsigned long *params,
+ unsigned long deadline)
+{
+ return -EOPNOTSUPP;
+}

This one is not called by non-SATA code now either...

WBR, Sergei

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