Re: [PATCH 04/12] libata: kill ata_id_to_dma_mode()
Tejun Heo wrote: ata_id_to_dma_mode() isn't quite generic. The function is basically privately implemented ata_id_xfermask() combined with hardcoded mode printing and configuration which are specific to ata_generic. Kill the function and open code it in generic_set_mode() using generic xfermode handling functions. Signed-off-by: Tejun Heo <[EMAIL PROTECTED]> [responding to Alan's objection, and subsequent thread] Yeah, it's a matter of taste. I applied, figuring "in for a dime in for a dozen" :) I'm not thrilled with all these fine-grained exports of timing tables and mode mask functions. I applied the patches, but I would really like to hide more of these details away from LLD [if possible, the obvious disclaimed...] Jeff - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/12] libata: kill ata_id_to_dma_mode()
Alan Cox wrote: > On Tue, 27 Nov 2007 19:43:41 +0900 > Tejun Heo <[EMAIL PROTECTED]> wrote: > >> ata_id_to_dma_mode() isn't quite generic. The function is basically >> privately implemented ata_id_xfermask() combined with hardcoded mode >> printing and configuration which are specific to ata_generic. >> >> Kill the function and open code it in generic_set_mode() using generic >> xfermode handling functions. > > That one I still think is a bad idea. This sort of internal knowledge > belongs in library routines. It is a pretty generic function for the case > where the device is using existing modes and wants to display them. The thing that bothers me is that the code assumes SWDMA and reports "DMA" and sets xfer_mask for MWDMA0. This is specific to ata_generic and with that part removed, what can be factored out is... ata_dev_printk(dev, KERN_INFO, "configured for %s\n", ata_mode_string(xfer_mask)); dev->xfer_mode = ata_xfer_mask2mode(xfer_mask); dev->xfer_shift = ata_xfer_mode2shift(dev->xfer_mode); Unfortunately, ata_generic won't be able to use such generic helper because it uses unspecified DMA and PIO modes which generic helper wouldn't support. Unless we are gonna have other drivers which would blindly trust device setting regarding PIO/DMA modes, I don't think ata_id_to_dma_mode() would be useful, and if we are going to have more such drivers, we at least need to rename ata_id_to_dma_mode() to note that the function blindly trusts device reported configuration. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/12] libata: kill ata_id_to_dma_mode()
On Tue, 27 Nov 2007 19:43:41 +0900 Tejun Heo <[EMAIL PROTECTED]> wrote: > ata_id_to_dma_mode() isn't quite generic. The function is basically > privately implemented ata_id_xfermask() combined with hardcoded mode > printing and configuration which are specific to ata_generic. > > Kill the function and open code it in generic_set_mode() using generic > xfermode handling functions. That one I still think is a bad idea. This sort of internal knowledge belongs in library routines. It is a pretty generic function for the case where the device is using existing modes and wants to display them. - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 04/12] libata: kill ata_id_to_dma_mode()
ata_id_to_dma_mode() isn't quite generic. The function is basically
privately implemented ata_id_xfermask() combined with hardcoded mode
printing and configuration which are specific to ata_generic.
Kill the function and open code it in generic_set_mode() using generic
xfermode handling functions.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
drivers/ata/ata_generic.c | 17 -
drivers/ata/libata-core.c | 43 ---
include/linux/libata.h|1 -
3 files changed, 16 insertions(+), 45 deletions(-)
diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
index fb5434c..d11a4f1 100644
--- a/drivers/ata/ata_generic.c
+++ b/drivers/ata/ata_generic.c
@@ -63,7 +63,22 @@ static int generic_set_mode(struct ata_link *link, struct
ata_device **unused)
/* We do need the right mode information for DMA or PIO
and this comes from the current configuration flags */
if (dma_enabled & (1 << (5 + dev->devno))) {
- ata_id_to_dma_mode(dev, XFER_MW_DMA_0);
+ unsigned int xfer_mask = ata_id_xfermask(dev->id);
+ const char *name;
+
+ if (xfer_mask & (ATA_MASK_MWDMA | ATA_MASK_UDMA))
+ name = ata_mode_string(xfer_mask);
+ else {
+ /* SWDMA perhaps? */
+ name = "DMA";
+ xfer_mask |= ata_xfer_mode2mask(XFER_MW_DMA_0);
+ }
+
+ ata_dev_printk(dev, KERN_INFO, "configured for %s\n",
+ name);
+
+ dev->xfer_mode = ata_xfer_mask2mode(xfer_mask);
+ dev->xfer_shift = ata_xfer_mode2shift(dev->xfer_mode);
dev->flags &= ~ATA_DFLAG_PIO;
} else {
ata_dev_printk(dev, KERN_INFO, "configured for PIO\n");
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 4f06ba7..88ed491 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1286,48 +1286,6 @@ static int ata_hpa_resize(struct ata_device *dev)
}
/**
- * ata_id_to_dma_mode - Identify DMA mode from id block
- * @dev: device to identify
- * @unknown: mode to assume if we cannot tell
- *
- * Set up the timing values for the device based upon the identify
- * reported values for the DMA mode. This function is used by drivers
- * which rely upon firmware configured modes, but wish to report the
- * mode correctly when possible.
- *
- * In addition we emit similarly formatted messages to the default
- * ata_dev_set_mode handler, in order to provide consistency of
- * presentation.
- */
-
-void ata_id_to_dma_mode(struct ata_device *dev, u8 unknown)
-{
- unsigned int mask;
- u8 mode;
-
- /* Pack the DMA modes */
- mask = ((dev->id[63] >> 8) << ATA_SHIFT_MWDMA) & ATA_MASK_MWDMA;
- if (dev->id[53] & 0x04)
- mask |= ((dev->id[88] >> 8) << ATA_SHIFT_UDMA) & ATA_MASK_UDMA;
-
- /* Select the mode in use */
- mode = ata_xfer_mask2mode(mask);
-
- if (mode != 0xff) {
- ata_dev_printk(dev, KERN_INFO, "configured for %s\n",
- ata_mode_string(mask));
- } else {
- /* SWDMA perhaps ? */
- mode = unknown;
- ata_dev_printk(dev, KERN_INFO, "configured for DMA\n");
- }
-
- /* Configure the device reporting */
- dev->xfer_mode = mode;
- dev->xfer_shift = ata_xfer_mode2shift(mode);
-}
-
-/**
* ata_noop_dev_select - Select device 0/1 on ATA bus
* @ap: ATA channel to manipulate
* @device: ATA device (numbered from zero) to select
@@ -7620,7 +7578,6 @@ EXPORT_SYMBOL_GPL(ata_host_resume);
#endif /* CONFIG_PM */
EXPORT_SYMBOL_GPL(ata_id_string);
EXPORT_SYMBOL_GPL(ata_id_c_string);
-EXPORT_SYMBOL_GPL(ata_id_to_dma_mode);
EXPORT_SYMBOL_GPL(ata_scsi_simulate);
EXPORT_SYMBOL_GPL(ata_pio_need_iordy);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 2a8c925..006183e 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -887,7 +887,6 @@ extern void ata_id_string(const u16 *id, unsigned char *s,
unsigned int ofs, unsigned int len);
extern void ata_id_c_string(const u16 *id, unsigned char *s,
unsigned int ofs, unsigned int len);
-extern void ata_id_to_dma_mode(struct ata_device *dev, u8 unknown);
extern void ata_bmdma_setup(struct ata_queued_cmd *qc);
extern void ata_bmdma_start(struct ata_queued_cmd *qc);
extern void ata_bmdma_stop(struct ata_queued_cmd *qc);
--
1.5.2.4
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.
Re: [PATCH 04/12] libata: kill ata_id_to_dma_mode()
Tejun Heo wrote: ata_id_to_dma_mode() isn't quite generic. The function is basically privately implemented ata_id_xfermask() combined with hardcoded mode printing and configuration which are specific to ata_generic. Kill the function and open code it in generic_set_mode() using generic xfermode handling functions. Signed-off-by: Tejun Heo <[EMAIL PROTECTED]> ACK patches 1-4 - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/12] libata: kill ata_id_to_dma_mode()
Alan Cox wrote: > On Tue, 6 Nov 2007 14:39:02 +0900 > Tejun Heo <[EMAIL PROTECTED]> wrote: > >> ata_id_to_dma_mode() isn't quite generic. The function is basically >> privately implemented ata_id_xfermask() combined with hardcoded mode >> printing and configuration which are specific to ata_generic. > > I anticpiated other users. This seems a backward move - we end up > exporting lots of low level detail into the drivers which should be none > of their business. Those functions need to be exported anyway for pata_amd and I think it's natural to export them as xfer mode and mask handling is pretty fundamental and having those helpers around is handy when implementing LLD-specific mode selection. ata_id_to_dma_mode() just seemed a bit too specific to me. Sans ata_id_xfermask() part, printing configured mode and using "DMA" when there's no applicable DMA mode don't seem too useful as generic xfer mode/mask helper. I agree that the latter part of the function can be factored tho (setting xfer_mode, shift and clearing PIO flag according to determined xfer_mask). But since ata_generic() is currently the only user, factoring that part out seems an overkill. Well, I guess this is a matter of taste after all. How about letting Jeff determine it? I'm okay as long as generic ata_id_xfermask() is used there. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/12] libata: kill ata_id_to_dma_mode()
On Tue, 6 Nov 2007 14:39:02 +0900 Tejun Heo <[EMAIL PROTECTED]> wrote: > ata_id_to_dma_mode() isn't quite generic. The function is basically > privately implemented ata_id_xfermask() combined with hardcoded mode > printing and configuration which are specific to ata_generic. I anticpiated other users. This seems a backward move - we end up exporting lots of low level detail into the drivers which should be none of their business. - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 04/12] libata: kill ata_id_to_dma_mode()
ata_id_to_dma_mode() isn't quite generic. The function is basically
privately implemented ata_id_xfermask() combined with hardcoded mode
printing and configuration which are specific to ata_generic.
Kill the function and open code it in generic_set_mode() using generic
xfermode handling functions.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
drivers/ata/ata_generic.c | 17 -
drivers/ata/libata-core.c | 43 ---
include/linux/libata.h|1 -
3 files changed, 16 insertions(+), 45 deletions(-)
diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
index fb5434c..d11a4f1 100644
--- a/drivers/ata/ata_generic.c
+++ b/drivers/ata/ata_generic.c
@@ -63,7 +63,22 @@ static int generic_set_mode(struct ata_link *link, struct
ata_device **unused)
/* We do need the right mode information for DMA or PIO
and this comes from the current configuration flags */
if (dma_enabled & (1 << (5 + dev->devno))) {
- ata_id_to_dma_mode(dev, XFER_MW_DMA_0);
+ unsigned int xfer_mask = ata_id_xfermask(dev->id);
+ const char *name;
+
+ if (xfer_mask & (ATA_MASK_MWDMA | ATA_MASK_UDMA))
+ name = ata_mode_string(xfer_mask);
+ else {
+ /* SWDMA perhaps? */
+ name = "DMA";
+ xfer_mask |= ata_xfer_mode2mask(XFER_MW_DMA_0);
+ }
+
+ ata_dev_printk(dev, KERN_INFO, "configured for %s\n",
+ name);
+
+ dev->xfer_mode = ata_xfer_mask2mode(xfer_mask);
+ dev->xfer_shift = ata_xfer_mode2shift(dev->xfer_mode);
dev->flags &= ~ATA_DFLAG_PIO;
} else {
ata_dev_printk(dev, KERN_INFO, "configured for PIO\n");
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index dc7e2f1..0eae1c6 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1274,48 +1274,6 @@ static int ata_hpa_resize(struct ata_device *dev)
}
/**
- * ata_id_to_dma_mode - Identify DMA mode from id block
- * @dev: device to identify
- * @unknown: mode to assume if we cannot tell
- *
- * Set up the timing values for the device based upon the identify
- * reported values for the DMA mode. This function is used by drivers
- * which rely upon firmware configured modes, but wish to report the
- * mode correctly when possible.
- *
- * In addition we emit similarly formatted messages to the default
- * ata_dev_set_mode handler, in order to provide consistency of
- * presentation.
- */
-
-void ata_id_to_dma_mode(struct ata_device *dev, u8 unknown)
-{
- unsigned int mask;
- u8 mode;
-
- /* Pack the DMA modes */
- mask = ((dev->id[63] >> 8) << ATA_SHIFT_MWDMA) & ATA_MASK_MWDMA;
- if (dev->id[53] & 0x04)
- mask |= ((dev->id[88] >> 8) << ATA_SHIFT_UDMA) & ATA_MASK_UDMA;
-
- /* Select the mode in use */
- mode = ata_xfer_mask2mode(mask);
-
- if (mode != 0xff) {
- ata_dev_printk(dev, KERN_INFO, "configured for %s\n",
- ata_mode_string(mask));
- } else {
- /* SWDMA perhaps ? */
- mode = unknown;
- ata_dev_printk(dev, KERN_INFO, "configured for DMA\n");
- }
-
- /* Configure the device reporting */
- dev->xfer_mode = mode;
- dev->xfer_shift = ata_xfer_mode2shift(mode);
-}
-
-/**
* ata_noop_dev_select - Select device 0/1 on ATA bus
* @ap: ATA channel to manipulate
* @device: ATA device (numbered from zero) to select
@@ -7643,7 +7601,6 @@ EXPORT_SYMBOL_GPL(ata_host_resume);
#endif /* CONFIG_PM */
EXPORT_SYMBOL_GPL(ata_id_string);
EXPORT_SYMBOL_GPL(ata_id_c_string);
-EXPORT_SYMBOL_GPL(ata_id_to_dma_mode);
EXPORT_SYMBOL_GPL(ata_scsi_simulate);
EXPORT_SYMBOL_GPL(ata_pio_need_iordy);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 4deb8a7..f84106a 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -888,7 +888,6 @@ extern void ata_id_string(const u16 *id, unsigned char *s,
unsigned int ofs, unsigned int len);
extern void ata_id_c_string(const u16 *id, unsigned char *s,
unsigned int ofs, unsigned int len);
-extern void ata_id_to_dma_mode(struct ata_device *dev, u8 unknown);
extern void ata_bmdma_setup(struct ata_queued_cmd *qc);
extern void ata_bmdma_start(struct ata_queued_cmd *qc);
extern void ata_bmdma_stop(struct ata_queued_cmd *qc);
--
1.5.2.4
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.
