Re: [PATCH 4/7] sata_nv: implement irq manipulation methods

[Jeff Garzik <jgarzik@pobox.com>]

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@gmail.com>
> 
> ---
> 
>  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, &regval);
+		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.

	Jeff