linux-kernel - Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 14 Nov 2013 09:32:40 +0100
From:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
To:	Mark Rutland <mark.rutland@....com>
Cc:	MyungJoo Ham <myungjoo.ham@...sung.com>,
	Chanwoo Choi <cw00.choi@...sung.com>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Lee Jones <lee.jones@...aro.org>,
	Anton Vorontsov <anton@...msg.org>,
	David Woodhouse <dwmw2@...radead.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Kyungmin Park <kyungmin.park@...sung.com>
Subject: Re: [PATCH 1/4] mfd: max14577: Add max14577 MFD driver core

Hi,


On Wed, 2013-11-13 at 11:50 +0000, Mark Rutland wrote:
> I see some of_* calls in the code, but no documentation of the binding.
> As this has more than just a compatible, interrupts, and reg, it needs
> its own binding document.
> 
> [...]
> 
> > +static struct mfd_cell max14577_devs[] = {
> > +       { .name = "max14577-muic",
> > +               .of_compatible = "maxim,max14577-muic", },
> 
> The compatible string looks fine to me, but should be documented.

Sure, I will add documentation.

> [...]
> 
> > +#ifdef CONFIG_OF
> > +static struct max14577_platform_data *max14577_i2c_parse_dt(struct device *dev)
> > +{
> > +       struct max14577_platform_data *pdata;
> > +       struct device_node *np = dev->of_node;
> > +
> > +       pdata = devm_kzalloc(dev, sizeof(struct max14577_platform_data),
> > +                            GFP_KERNEL);
> > +       if (!pdata)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       pdata->irq_gpio = of_get_named_gpio(np, "irq_gpio", 0);
> 
> In general, '-' is preferred to '_' in property names. This should be
> irq-gpio.

OK.


> What exactly is this used for? I know that others have strong opinions
> about how gpio/irq interaction should be handled.

Later also Mark Brown raised this. I'll leave this to discussion with
Kyungmin Park as I'm not entirely the author of the code.


> 
> > +       if (!gpio_is_valid(pdata->irq_gpio)) {
> > +               dev_err(dev, "Invalid irq_gpio in DT\n");
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       if (of_property_read_bool(np, "wakeup"))
> > +               pdata->wakeup = true;
> 
> What does this mean? It seems like a remarkably generic name for
> something that I'd guess is _very_ specific to this particular binding.
> 
> It might be worth prefixing it, and is certainly worth having a more
> precise name.

It is used for marking device as wake up source. This is same code as in
other max drivers used on Exynos boards (max77693 and max77686).


Thanks for review,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/