From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tejun Heo Subject: Re: [PATCH 4/7] sata_nv: implement irq manipulation methods Date: Wed, 14 Jun 2006 22:47:13 +0900 Message-ID: <44901361.1040301@gmail.com> References: <11502342644086-git-send-email-htejun@gmail.com> <448F5FC6.60807@pobox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from py-out-1112.google.com ([64.233.166.182]:23561 "EHLO py-out-1112.google.com") by vger.kernel.org with ESMTP id S964929AbWFNNrU (ORCPT ); Wed, 14 Jun 2006 09:47:20 -0400 Received: by py-out-1112.google.com with SMTP id i49so362445pye for ; Wed, 14 Jun 2006 06:47:20 -0700 (PDT) In-Reply-To: <448F5FC6.60807@pobox.com> Sender: linux-ide-owner@vger.kernel.org List-Id: linux-ide@vger.kernel.org To: Jeff Garzik Cc: linux-ide@vger.kernel.org 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 >> >> --- >> >> 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