Jeff Garzik wrote:
Tejun Heo wrote:
Add four irq manipulation callbacks to nv_host_desc and implement
those methods for nf2/3 and ck804. These methods will be used by new
irq_handler, EH and hotplug support.
Signed-off-by: Tejun Heo <htejun@xxxxxxxxx>
---
drivers/scsi/sata_nv.c | 87
++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 87 insertions(+), 0 deletions(-)
d47f98a5ada7fb993f659f4ba1eb496457122455
diff --git a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
index f370044..93e74aa 100644
--- a/drivers/scsi/sata_nv.c
+++ b/drivers/scsi/sata_nv.c
@@ -77,6 +77,16 @@ enum {
NV_MCP_SATA_CFG_20_SATA_SPACE_EN = 0x04,
};
+static u8 nf2_get_irq_mask(struct ata_host_set *host_set);
+static void nf2_set_irq_mask(struct ata_host_set *host_set, u8 mask);
+static u8 nf2_get_irq_status(struct ata_host_set *host_set);
+static void nf2_clr_irq_status(struct ata_host_set *host_set);
+
+static u8 ck804_get_irq_mask(struct ata_host_set *host_set);
+static void ck804_set_irq_mask(struct ata_host_set *host_set, u8 mask);
+static u8 ck804_get_irq_status(struct ata_host_set *host_set);
+static void ck804_clr_irq_status(struct ata_host_set *host_set);
+
static int nv_init_one (struct pci_dev *pdev, const struct
pci_device_id *ent);
static irqreturn_t nv_interrupt (int irq, void *dev_instance,
struct pt_regs *regs);
@@ -133,6 +143,10 @@ static const struct pci_device_id nv_pci
struct nv_host_desc
{
enum nv_host_type host_type;
+ u8 (*get_irq_mask) (struct ata_host_set *host_set);
+ void (*set_irq_mask) (struct ata_host_set *host_set, u8 mask);
+ u8 (*get_irq_status) (struct ata_host_set *host_set);
+ void (*clr_irq_status) (struct ata_host_set *host_set);
NAK this train of thought. We don't need new mini-API hooks like this,
when you could just implement nf2_xxx() and ck804_xxx() hooks at a
higher level. Rather than growing "stealth nf2 checks" like this:
+static void nv_freeze (struct ata_port *ap)
+{
+ struct ata_host_set *host_set = ap->host_set;
+ struct nv_host_desc *host_desc = host_set->private_data;
+ int shift = ap->port_no * NV_INT_PORT_SHIFT;
+ u8 mask;
+
+ if (!host_desc->get_irq_mask) {
+ ata_bmdma_freeze(ap);
+ return;
+ }
you should implement nf2_freeze() and nv_freeze() separately (or
nf2_freeze and ck804_freeze, whatever).
Another example of adding a test, where adding a hook would be better:
static void nv_host_stop (struct ata_host_set *host_set)
{
+ struct nv_host_desc *host_desc = host_set->private_data;
+
+ /* disable SATA space for CK804 */
+ if (host_desc->host_type == CK804) {
+ struct pci_dev *pdev = to_pci_dev(host_set->dev);
+ u8 regval;
+
+ pci_read_config_byte(pdev, NV_MCP_SATA_CFG_20, ®val);
+ regval &= ~NV_MCP_SATA_CFG_20_SATA_SPACE_EN;
+ pci_write_config_byte(pdev, NV_MCP_SATA_CFG_20, regval);
+ }
Thoughout _every_ single patch, one sees old-vs-ck804 differences. Your
patches highlight these differences with every single irq_stat_valid
test, for example.
I think it is better for testing a long term maintainability if your
patches create separate hooks for old and ck804 hardware. I think
you'll find that it makes implementing EH and hotplug stuff much easier,
and it will aid the integration of ADMA support down the road. A first
step is probably splitting the interrupt handler into nv_xxx and
ck804_xxx versions, then proceeding your 7-patch set, keeping in mind
the "hook preferred over 'if'" model.
Yeap, implemented that way at first but code was much more compact this
way, so... I'll revert to original implementation.
Thanks.
--
tejun
-
: 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
