lkml.org 
[lkml]   [2015]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 01/11] mfd: add the Berlin controller driver
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:
> >
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -840,6 +840,11 @@ config STMPE_SPI
> > > This is used to enable SPI interface of STMPE
> > > endmenu
> > >
> > > +config MFD_BERLIN_CTRL
> > > + bool
> >
> > Missing description.
> > Why can't this driver be built as a module?
>
> Well, this mfd driver registers various devices as the pinctrl and the
> reset driver. I think we want the pinctrl driver to always be
> registered.
>
> IMHO we want this driver to always be selected when using a Berlin SoC.

[...]

> > > +#include <linux/mfd/core.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> >
> > kernel.h?
>
> Is there a reason to add this header here?

I guess not if you're not using any of its macros. Although, I don't
recall every seeing s driver without it. I guess you learn something
every day.

[...]

> > > +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?

> To provide a regmap to the devices' drivers we also use syscon on the
> chip/system controller nodes.
>
> > > +MODULE_LICENSE("GPL");

I wonder if these are actually useful if you're always going to be
built-in?

> > > +MODULE_LICENSE("GPL");
> >
> > v2
>
> "GPL" is a valid choice, quoting include/linux.module.h:
>
> "GPL" [GNU Public License v2 or later]
> "GPL v2" [GNU Public License v2]
>
> Is there a reason you explicitly want to use GPLv2, and only GPLv2?

Yes, there is disparity between this and the comment in the file
header. I don't mind which one you amend, but a change is required.

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


\
 
 \ /
  Last update: 2015-02-17 13:21    [W:0.152 / U:2.304 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site