Message ID | 20100218185922.16594.98925.sendpatchset@localhost |
---|---|
State | Not Applicable |
Delegated to: | David Miller |
Headers | show |
On Thu, 18 Feb 2010 19:59:22 +0100 Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote: > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > Subject: [PATCH] libata: pass host flags into __ata_pci_sff_init_one() helper > > This was orginally proposed by Alan Cox but as a change > for ata_pci_sff_init_one() helper function instead of > __ata_pci_sff_init_one() one which casues needless churn > to all host drivers and accidentally breakes few host > drivers which are still on their way upstream. > > Allows parallel scan and the like to be set without > having to stop using the existing full helper functions. NAK - __ is for internal symbol names. I was split 50/50 on adding ata_pci_sff_init_one_flags() or similar but the churn, given its a one off and we can then add all sorts of other future flags without pain, seemed worth it. I'm ambivalent about whether its best to go with a new function name as you have or take the hit now (which seems sensible to me). Either way the __ naming is wrong for an external interface. Anyway I'd *hope* we can get > 50% of interfaces parallel scanning at which point it ceases to be more noise anyway ! Jeff ? -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 18 February 2010 10:44:48 pm Alan Cox wrote: > On Thu, 18 Feb 2010 19:59:22 +0100 > Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote: > > > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > > Subject: [PATCH] libata: pass host flags into __ata_pci_sff_init_one() helper > > > > This was orginally proposed by Alan Cox but as a change > > for ata_pci_sff_init_one() helper function instead of > > __ata_pci_sff_init_one() one which casues needless churn > > to all host drivers and accidentally breakes few host > > drivers which are still on their way upstream. > > > > Allows parallel scan and the like to be set without > > having to stop using the existing full helper functions. > > NAK - __ is for internal symbol names. A NAK for an atang specific patch? :) - Please read DISCLAIMER in the first mail (it is there for a valid reasons). - The patch is for atang tree and said __ change happened _months_ ago as a part of (very much related) ->init_host work which is not 'upstream'. - __ has been also used for exactly such purpose in the kernel, i.e.: $ grep EXPORT_SYMBOL drivers/pci/*c|grep __ drivers/pci/htirq.c:EXPORT_SYMBOL(__ht_create_irq); drivers/pci/pci.c:EXPORT_SYMBOL_GPL(__pci_complete_power_transition); drivers/pci/pci.c:EXPORT_SYMBOL_GPL(__pci_reset_function); drivers/pci/pci-driver.c:EXPORT_SYMBOL(__pci_register_driver); Just see commit 863b18f introducing __pci_register_driver (please notice that the change was suggested by Al Viro!).. -- Bartlomiej Zolnierkiewicz -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> - __ has been also used for exactly such purpose in the kernel, i.e.:
As helpers for inlines and like the yes...
Matter of style anyway.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/18/2010 04:44 PM, Alan Cox wrote: > On Thu, 18 Feb 2010 19:59:22 +0100 > Bartlomiej Zolnierkiewicz<bzolnier@gmail.com> wrote: > >> From: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com> >> Subject: [PATCH] libata: pass host flags into __ata_pci_sff_init_one() helper >> >> This was orginally proposed by Alan Cox but as a change >> for ata_pci_sff_init_one() helper function instead of >> __ata_pci_sff_init_one() one which casues needless churn >> to all host drivers and accidentally breakes few host >> drivers which are still on their way upstream. >> >> Allows parallel scan and the like to be set without >> having to stop using the existing full helper functions. > > NAK - __ is for internal symbol names. > > I was split 50/50 on adding ata_pci_sff_init_one_flags() or similar but > the churn, given its a one off and we can then add all sorts of other > future flags without pain, seemed worth it. > > I'm ambivalent about whether its best to go with a new function name as > you have or take the hit now (which seems sensible to me). Either way the > __ naming is wrong for an external interface. Your proposed patch from yesterday adding flags to each is fine. All-driver patches are just fine, as long as the need is demonstrated [and it is, in this case]. > Anyway I'd *hope* we can get> 50% of interfaces parallel scanning at > which point it ceases to be more noise anyway ! > > Jeff ? Most hardware should support parallel scanning, sans caveats like chipsets that snoop SET FEATURES - XFER MODE (that command, and only that command, needs synchronization) Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: b/drivers/ata/libata-sff.c =================================================================== --- a/drivers/ata/libata-sff.c +++ b/drivers/ata/libata-sff.c @@ -3009,6 +3009,7 @@ EXPORT_SYMBOL_GPL(ata_pci_sff_activate_h * @sht: scsi_host_template to use when registering the host * @host_priv: host private_data * @init_host: host initialization method (optional) + * @hflag: host flags * * This is a helper function which can be called from a driver's * xxx_init_one() probe function if the hardware uses traditional @@ -3031,7 +3032,7 @@ EXPORT_SYMBOL_GPL(ata_pci_sff_activate_h int __ata_pci_sff_init_one(struct pci_dev *pdev, const struct ata_port_info * const *ppi, struct scsi_host_template *sht, void *host_priv, - int (*init_host)(struct device *dev)) + int (*init_host)(struct device *dev), int hflag) { struct device *dev = &pdev->dev; const struct ata_port_info *pi = NULL; @@ -3068,6 +3069,7 @@ int __ata_pci_sff_init_one(struct pci_de host->private_data = host_priv; host->init_host = init_host; + host->flags |= hflag; pci_set_master(pdev); rc = ata_pci_sff_activate_host(host, ata_sff_interrupt, sht); Index: b/drivers/ata/pata_ali.c =================================================================== --- a/drivers/ata/pata_ali.c +++ b/drivers/ata/pata_ali.c @@ -316,7 +316,7 @@ static int ali_init_one(struct pci_dev * } return __ata_pci_sff_init_one(pdev, ppi, &ali_sht, NULL, - ali_init_chipset); + ali_init_chipset, 0); } static const struct pci_device_id ali[] = { Index: b/drivers/ata/pata_amd.c =================================================================== --- a/drivers/ata/pata_amd.c +++ b/drivers/ata/pata_amd.c @@ -494,7 +494,7 @@ static int amd_init_one(struct pci_dev * } /* And fire it up */ - return __ata_pci_sff_init_one(pdev, ppi, &amd_sht, hpriv, amd_fixup); + return __ata_pci_sff_init_one(pdev, ppi, &amd_sht, hpriv, amd_fixup, 0); } static const struct pci_device_id amd[] = { Index: b/drivers/ata/pata_artop.c =================================================================== --- a/drivers/ata/pata_artop.c +++ b/drivers/ata/pata_artop.c @@ -177,7 +177,7 @@ static int artop_init_one (struct pci_de atp8xx_fixup(&pdev->dev); return __ata_pci_sff_init_one(pdev, ppi, &artop_sht, NULL, - atp8xx_fixup); + atp8xx_fixup, 0); } static const struct pci_device_id artop_pci_tbl[] = { Index: b/drivers/ata/pata_atiixp.c =================================================================== --- a/drivers/ata/pata_atiixp.c +++ b/drivers/ata/pata_atiixp.c @@ -77,23 +77,9 @@ static int atiixp_init_one(struct pci_de .port_ops = &atiixp_port_ops }; const struct ata_port_info *ppi[] = { &info, &info }; - struct ata_host *host; - int rc; - /* enable device and prepare host */ - rc = pcim_enable_device(pdev); - if (rc) - return rc; - - rc = ata_pci_sff_prepare_host(pdev, ppi, &host); - if (rc) - return rc; - - host->flags |= ATA_HOST_PARALLEL_SCAN; - - pci_set_master(pdev); - - return ata_pci_sff_activate_host(host, ata_sff_interrupt, &atiixp_sht); + return __ata_pci_sff_init_one(pdev, ppi, &atiixp_sht, NULL, NULL, + ATA_HOST_PARALLEL_SCAN); } static const struct pci_device_id atiixp[] = { Index: b/drivers/ata/pata_cmd640.c =================================================================== --- a/drivers/ata/pata_cmd640.c +++ b/drivers/ata/pata_cmd640.c @@ -226,7 +226,7 @@ static int cmd640_init_one(struct pci_de cmd640_hardware_init(&pdev->dev); return __ata_pci_sff_init_one(pdev, ppi, &cmd640_sht, NULL, - cmd640_hardware_init); + cmd640_hardware_init, 0); } static const struct pci_device_id cmd640[] = { Index: b/drivers/ata/pata_cs5530.c =================================================================== --- a/drivers/ata/pata_cs5530.c +++ b/drivers/ata/pata_cs5530.c @@ -150,7 +150,7 @@ static int cs5530_init_one(struct pci_de /* Now kick off ATA set up */ return __ata_pci_sff_init_one(pdev, ppi, &cs5530_sht, NULL, - cs5530_init_chip); + cs5530_init_chip, 0); } static const struct pci_device_id cs5530[] = { Index: b/drivers/ata/pata_efar.c =================================================================== --- a/drivers/ata/pata_efar.c +++ b/drivers/ata/pata_efar.c @@ -89,27 +89,13 @@ static int efar_init_one (struct pci_dev .port_ops = &efar_ops, }; const struct ata_port_info *ppi[] = { &info, &info }; - struct ata_host *host; - int rc; if (!printed_version++) dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n"); - /* enable device and prepare host */ - rc = pcim_enable_device(pdev); - if (rc) - return rc; - - rc = ata_pci_sff_prepare_host(pdev, ppi, &host); - if (rc) - return rc; - - host->flags |= ATA_HOST_PARALLEL_SCAN; - - pci_set_master(pdev); - - return ata_pci_sff_activate_host(host, ata_sff_interrupt, &efar_sht); + return __ata_pci_sff_init_one(pdev, ppi, &efar_sht, NULL, NULL, + ATA_HOST_PARALLEL_SCAN); } static const struct pci_device_id efar_pci_tbl[] = { Index: b/drivers/ata/pata_hpt366.c =================================================================== --- a/drivers/ata/pata_hpt366.c +++ b/drivers/ata/pata_hpt366.c @@ -413,7 +413,7 @@ static int hpt36x_init_one(struct pci_de } /* Now kick off ATA set up */ return __ata_pci_sff_init_one(dev, ppi, &hpt36x_sht, hpriv, - hpt36x_init_chipset); + hpt36x_init_chipset, 0); } static const struct pci_device_id hpt36x[] = { Index: b/drivers/ata/pata_it821x.c =================================================================== --- a/drivers/ata/pata_it821x.c +++ b/drivers/ata/pata_it821x.c @@ -625,7 +625,7 @@ static int it821x_init_one(struct pci_de ppi[0] = &info_smart; } return __ata_pci_sff_init_one(pdev, ppi, &it821x_sht, NULL, - it821x_fixup); + it821x_fixup, 0); } static const struct pci_device_id it821x[] = { Index: b/drivers/ata/pata_ns87415.c =================================================================== --- a/drivers/ata/pata_ns87415.c +++ b/drivers/ata/pata_ns87415.c @@ -385,7 +385,7 @@ static int ns87415_init_one (struct pci_ ns87415_fixup(&pdev->dev); return __ata_pci_sff_init_one(pdev, ppi, &ns87415_sht, NULL, - ns87415_fixup); + ns87415_fixup, 0); } static const struct pci_device_id ns87415_pci_tbl[] = { Index: b/drivers/ata/pata_sl82c105.c =================================================================== --- a/drivers/ata/pata_sl82c105.c +++ b/drivers/ata/pata_sl82c105.c @@ -222,7 +222,7 @@ static int sl82c105_init_one(struct pci_ sl82c105_fixup(&dev->dev); return __ata_pci_sff_init_one(dev, ppi, &sl82c105_sht, NULL, - sl82c105_fixup); + sl82c105_fixup, 0); } static const struct pci_device_id sl82c105[] = { Index: b/include/linux/libata.h =================================================================== --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -1605,12 +1605,12 @@ extern int ata_pci_sff_activate_host(str extern int __ata_pci_sff_init_one(struct pci_dev *pdev, const struct ata_port_info * const *ppi, struct scsi_host_template *sht, void *host_priv, - int (*init_host)(struct device *dev)); + int (*init_host)(struct device *dev), int hflag); static inline int ata_pci_sff_init_one(struct pci_dev *pdev, const struct ata_port_info * const *ppi, struct scsi_host_template *sht, void *host_priv) { - return __ata_pci_sff_init_one(pdev, ppi, sht, host_priv, NULL); + return __ata_pci_sff_init_one(pdev, ppi, sht, host_priv, NULL, 0); } #endif /* CONFIG_PCI */