Re: [PATCH 1/9] reset: add the Berlin reset controller driver

[Sebastian Hesselbarth]

On 06/05/2014 06:36 PM, Philipp Zabel wrote:
> Am Donnerstag, den 05.06.2014, 17:48 +0200 schrieb Antoine TÃnart:
> > Add a reset controller for Marvell Berlin SoCs which is used by the
> > USB PHYs drivers (for now).
> >
> > Signed-off-by: Antoine TÃnart <antoine.tenart@xxxxxxxxxxxxxxxxxx>
> > ---
> >   drivers/reset/Makefile       |   1 +
> >   drivers/reset/reset-berlin.c | 113 +++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 114 insertions(+)
> >   create mode 100644 drivers/reset/reset-berlin.c
[...]
> >   obj-$(CONFIG_ARCH_STI) += sti/
> > diff --git a/drivers/reset/reset-berlin.c b/drivers/reset/reset-berlin.c
> > new file mode 100644
> > index 000000000000..78b42c882cb2
> > --- /dev/null
> > +++ b/drivers/reset/reset-berlin.c
> > @@ -0,0 +1,113 @@
> > +/*
> > + * Copyright (C) 2014 Marvell Technology Group Ltd.
> > + *
> > + * Antoine TÃnart <antoine.tenart@xxxxxxxxxxxxxxxxxx>
> > + *
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2. This program is licensed "as is" without any
> > + * warranty of any kind, whether express or implied.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
>
> Is there a reason this is not actually implemented as platform device?
[...]
> > +static int __berlin_reset_init(struct device_node *np)
> > +{
> > +	struct berlin_reset_priv *priv;
> > +	struct resource res;
> > +	resource_size_t size;
> > +	int ret;
> > +
> > +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	ret = of_address_to_resource(np, 0, &res);
> > +	if (ret)
> > +		goto err;
> > +
> > +	size = resource_size(&res);
> > +
> > +	priv->base = ioremap(res.start, size);
> > +	if (!priv->base) {
> > +		ret = -ENOMEM;
> > +		goto err;
> > +	}
>
> A platform driver could use devm_kzalloc, platform_get_resource,
> and devm_ioremap_resource here.
>
> > +	priv->base += BERLIN_RESET_REGISTER;
> > +
> > +	priv->rcdev.owner = THIS_MODULE;
> > +	priv->rcdev.nr_resets = size * 32;
>
> This doesn't seem right. The device tree patch shows that
> size = 0x400.

Actually, not using a platform_device now is the outcome of
some late DT node rework we had for the clock driver. The reason
we only have one node for the whole register set providing
pinctrl, reset, clock, ... is that it would require tiny separate
register ranges spread over the whole register set.

Instead, the idea is to have a single DT node, register a
driver providing a mmio regmap, and registering individual
platform_devices for the non-early drivers using the regmap.
We also evaluated syscon, but it will require dummy nodes for
each proper platform_device and that is something we really
want to avoid here.

Of course, writing that driver is delayed on my side because
of other non-Linux stuff that has to be taken care of first.
As I cannot tell how much time I can spend on it now, I prefer
to take this as is and provide update patches as soon as I have
worked out the regset driver.

Sebastian
--
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/