Re: [PATCH 1/3] mfd: add support for Janz CMOD-IO PCI MODULbus Carrier Board

["Ira W. Snyder" <iws@ovro.caltech.edu>]

On Thu, Mar 25, 2010 at 11:59:30PM +0100, Samuel Ortiz wrote:
> Hi Ira,
> 
> First of all, sorry for the late reply. Then my answers:
> 
> On Fri, Mar 19, 2010 at 11:22:09AM -0700, Ira W. Snyder wrote:
> > 
> > > > +/*
> > > > + * Subdevice Support
> > > > + */
> > > Please use the mfd-core API for building and registering platform sub devices.
> > > The pieces of code below should shrink significantly.
> > > 
> > 
> > Using this framework, how is it possible to create  the devices that I
> > do down below. For each subdevice, I need three resources:
> > 
> > 1) MODULbus registers -- PCI BAR3 + (0x200 * module_num)
> > 2) PLX Control Registers -- PCI BAR4
> > 3) IRQ
> > 
> > Specifically, the way IORESOURCE_MEM resources are copied seems wrong.
> > They start at the base address of only one resource and use the offsets
> > provided in the struct mfd_cell. See the if-statement at
> > drivers/mfd/mfd-core.c line 48.
> > 
> > I need two use two different parent resources. The mfd_add_devices()
> > function doesn't support this.
> I would still like you to use the mfd-core API. Here is my proposal:
> 
> 1) I modify mfd_add_device() to support a NULL mem_base argument. When
> mem_base is NULL, we would have:
> 
> res[r].parent = NULL and res[r].start = cell->resources[r].start;
> 
> The platform code will use iomem_resource as the parent for this resource.
> 

I don't know the implications of using iomem_resource as the parent
resource. If you think it is ok, I have no objections.

If it helps, I can provide the PCI resource as a parent resource in my
resources. Then, when mem_base is NULL, the mfd-core could do this:

res[r].parent = cell->resources[r].parent

This is basically what I did in my patch. I used the PCI resource as the
parent of all child resources. I don't know if that is safe, but it
works. :)

The mfd_add_device() function does this for IORESOURCE_IO resources. It
only tries to be smart for IORESOURCE_MEM and IORESOURCE_IRQ resources.

> 2) Your mfd_cell cells would have 3 resources, and you just need to set the
> IORESOURCE_MEM ones at probe time, with pci->resource[n]->start + offset as
> the start field.
> 
> Would that make sense to you ?
> 

Yep, that makes sense.

> > > > +	/* Onboard configuration registers */
> > > > +	priv->ctrl = pci_ioremap_bar(dev, 4);
> > > Why 4 ?
> > > 
> > > 
> > 
> > Because that is how the device works ;) There is a comment up above that
> > describes them as the "PLX control registers". Are you suggesting that I
> > add a comment here too?
> No, that's ok, I missed the comment.
> 
> > > > +#define PCI_VENDOR_ID_JANZ		0x13c3
> > > That probably belongs to pci_ids.h
> > > 
> > 
> > Should I add a patch to the series for this?
> Either that or merge the pci_ids.h changes with this patch.
> 

I guess it is a trivial enough change to merge with this patch.

I'll wait for your patch to the mfd-core API before making changes and
sending out the next round of updates.

Thanks for replying,
Ira