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

[Krzysztof Kozlowski <k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>]

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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html