[PATCH v4 1/5] drivers: memory: Introduce Marvell EBU Device Bus driver

[Ezequiel Garcia]

Hi Jason

On Mon, Apr 08, 2013 at 11:19:40AM -0600, Jason Gunthorpe wrote:
> Looks OK to me, though with mbus dt bindings we'd see some more
> changes.
> 

Thanks for the review!

> Just to note on them for future:
> 
> > +	child = of_get_next_child(node, NULL);
> > +	if (!child) {
> > +		dev_err(dev, "%s has no childs\n", node->full_name);
> > +		return -EINVAL;
> > +	}
> 
> This isn't really necessary anymore, the ranges of the node itself
> should be parsed to get the window start/end. The driver should never
> need to look at the children directly..
> 

You mean the above will not be necessary in the *future*,
once we add mbus DT bindings, right?

AFAICT, it's needed for the current workaround to work.
FWIW, the whole code below the *FIXME* is part of the temporary
to-be-removed thing.

Maybe I'll add a special comment on each chunk that's meant
to be removed to avoid confusions.

> > +	/* Get the CS to choose the window string */
> > +	err = of_property_read_u32(node, "ranges", &cs);
> > +	if (err < 0)
> > +		return err;
> 
> .. and this is why you've kept the 2 cells address - the top cell is
> still encoding the target id. This would go away eventually too..
> 
> Maybe it would be better in the interm to compute the CS offset from
> the control register offset?
> 
> (reg.start % 0x400)/8 should do the trick?
> 

Yes, I'll do this and drop the 2-cell address.

> > +	devbus->child = of_platform_device_create(child, NULL, &pdev->dev);
> > +	if (!devbus->child) {
> > +		dev_warn(dev, "cannot create child device %s\n", child->name);
> > +		/* Remove the allocated window */
> > +		mvebu_mbus_del_window(devbus->child_mem.start,
> > +				      resource_size(&devbus->child_mem));
> > +	}
> 
> This can probably just be of_platform_populate or similar to do all
> children? For instance, I often use many DT nodes to represent a FPGA,
> since the my FPGA's tend to have many functionally orthogonal units
> inside.
> 

In the past I've found it's not possible to use of_platform_populate,
altough I can be wrong.

The problem seems to be that through of_platform_populate() it's
possible that the child device (cfi-flash, for instance) gets probed
before the devbus controller. In other words, the actual parent-child
relationship gets lost and it's necesarry to use some child probe deferal
mechanism.

This has been recently discussed [1] when implementing GPMC where for simplicity
the of_platform_device_create() solution was chosen.
Anyway, I'll try of_platform_populate early tomorrow.

Thanks,

[1]  http://www.mail-archive.com/linux-omap@vger.kernel.org/msg85897.html
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com