Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Lee Jones]

On Wed, 18 Feb 2015, Antoine Tenart wrote:

> On Tue, Feb 17, 2015 at 11:54:48AM +0000, Lee Jones wrote:
> > On Tue, 17 Feb 2015, Antoine Tenart wrote:
> > > On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote:
> > > > On Wed, 11 Feb 2015, Antoine Tenart wrote:
> > 
> > > > > +static int berlin_ctrl_probe(struct platform_device *pdev)
> > > > > +{
> > > > > +	struct device *dev = &pdev->dev;
> > > > > +	const struct of_device_id *match;
> > > > > +	const struct berlin_ctrl_priv *priv;
> > > > > +	int ret;
> > > > > +
> > > > > +	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
> > > > > +	if (!match)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	priv = match->data;
> > > > > +
> > > > > +	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
> > > > > +	if (ret) {
> > > > > +		dev_err(dev, "Failed to add devices: %d\n", ret);
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > 
> > > > I'm not sure I see the point in this driver.  Why can't you just
> > > > register these devices directly from DT?
> > > 
> > > All these devices share the same bank of registers and we previously
> > > used a single node. But with many devices sharing a single node, this is
> > > problematic to register all the devices from DT. Using this MFD driver
> > > to do it is a proper solution in this case.
> > 
> > Tell me more.  What are the problems you encountered?
> 
> So we had a single node, chip-controller, accessed by multiple
> devices -and drivers-. We ended up with:
> 
> chip: chip-control@ea0000 {
> 	compatible = "marvell,berlin2q-chip-ctrl";
> 	reg = <0xea0000 0x400>, <0xdd0170 0x10>;
> 	#clock-cells = <1>;
> 	#reset-cells = <2>;
> 	clocks = <&refclk>;
> 	clock-names = "refclk";
> 
> 	[pinmux nodes]
> };
> 
> In addition to being a mess, how can you probe various drivers with this
> single node? We had to probe a clock driver in addition to the
> pin-controller and reset drivers. We ended up using arch_initcall() in
> the reset driver, which was *not* acceptable.
> 
> These chip and system controllers are not an IP, but helps not spreading
> this bank of registers all over the DT.
> 
> The solution to this problem is to introduce an mtd driver which
> registers all the sub-devices described by these chip and system
> controller nodes.

I'm still not convinced that your problem can't be solved in DT, but
creating a single psudo-hardware node is not correct either.  What
does the h/w _really_ look like?  Is all of this stuff on a single
chip?  If so, I would expect to see something like:

control@ea0000 {
	compatible = "marvel,control";

	pinctrl@xxxxx {
		compatible = "marvel,pinctrl";
	};

	reset@xxxxx {
		compatible = "marvel,reset";
	};
};

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/