On Tue, Feb 18, 2014 at 05:20:17PM -0600, Josh Cartwright wrote:
> On Tue, Feb 18, 2014 at 04:34:13PM +0100, Johannes Thumshirn wrote:
> > Add support for MCB over PCI devices. Both PCI attached on-board Chameleon FPGAs
> > as well as CompactPCI based MCB carrier cards are supported with this driver.
> >
> > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@xxxxxx>
> > ---
> > drivers/mcb/Kconfig | 13 ++++++
> > drivers/mcb/Makefile | 2 +
> > drivers/mcb/mcb-pci.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 123 insertions(+)
> > create mode 100644 drivers/mcb/mcb-pci.c
> >
> > diff --git a/drivers/mcb/Kconfig b/drivers/mcb/Kconfig
> > index 9e7d6f5..8b058bc 100644
> > --- a/drivers/mcb/Kconfig
> > +++ b/drivers/mcb/Kconfig
> > @@ -14,3 +14,16 @@ menuconfig MCB
> >
> > If build as a module, the module is called mcb.ko
> >
> > +if MCB
> > +config MCB_PCI
> > + tristate "PCI based MCB carrier"
> > + default m if MCB
>
> 'if MCB' is redundant (MCB has to be set for this option to even be
> available).
OK
>
> [..]
> > +++ b/drivers/mcb/mcb-pci.c
> > @@ -0,0 +1,108 @@
>
> Copyright/license blurb?
Args, forgotten, sorry.
>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/mcb.h>
> > +
> > +#include "mcb-internal.h"
> > +
> > +static struct mcb_bus *bus;
> > +struct priv {
> > + void __iomem *base;
> > +};
>
> This seems weird. Why is the bus not part of your private data? Seems
> like you're unnecessarily restricting yourself to supporting only one of
> these devices at a time...
You're right. Funny enough I didn't notice it, while testing as I used 2 identical
carrier cards. Card 2 must then have overwritten Card 1 :-(.
>
> > +
> > +static int mcb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > +{
> > + struct priv *priv;
> > + phys_addr_t mapbase;
> > + int ret = 0;
>
> No need to initialize.
OK
>
> > + int num_cells;
> > + unsigned long flags;
> > +
> > + priv = devm_kzalloc(&pdev->dev, sizeof(struct priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + ret = pci_enable_device(pdev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Failed to enable PCI device\n");
> > + return -ENODEV;
> > + }
> > +
> > + mapbase = pci_resource_start(pdev, 0);
> > + if (!mapbase) {
> > + dev_err(&pdev->dev, "No PCI resource\n");
> > + goto err_start;
> > + }
> > +
> > + ret = pci_request_region(pdev, 0, KBUILD_MODNAME);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Failed to request PCI BARs\n");
> > + goto err_start;
> > + }
> > +
> > + priv->base = pci_iomap(pdev, 0, 0);
>
> Hmm. There should really be a devm_* variant for PCI resources.
I don't think so, as I have to free the resources afterwards again so the mcb
devices can access these resources. Otherwise I would need to make them muxed,
but I don't see the reason to keep the resources after the parser code has
finished.
>
> [..]
> > +
> > +static void mcb_pci_remove(struct pci_dev *pdev)
> > +{
> > + struct priv *priv = pci_get_drvdata(pdev);
> > +
> > + mcb_release_bus(bus);
> > +
> > + pci_iounmap(pdev, priv->base);
> > + pci_release_region(pdev, 0);
> > + pci_disable_device(pdev);
> > + pci_set_drvdata(pdev, NULL);
> > +}
> > +
It's actually a "double free" of the resources, isn't it. Needs to be fixed.
> > +static struct pci_device_id mcb_pci_tbl[] = {
>
> const?
>
Yup.
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
As with the other patch, probably on monday. Sorry for the inconvenience.
Johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
