Re: [PATCH 12/13] input: max77650: add onkey support

[Lee Jones <lee.jones@xxxxxxxxxx>]

> > From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> > 
> > Add support for the push- and slide-button events for max77650.
> > 
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> > ---
> >  drivers/input/misc/Kconfig          |   9 ++
> >  drivers/input/misc/Makefile         |   1 +
> >  drivers/input/misc/max77650-onkey.c | 135 ++++++++++++++++++++++++++++
> >  3 files changed, 145 insertions(+)
> >  create mode 100644 drivers/input/misc/max77650-onkey.c

[...]

Moving things around a bit:

> > +static int max77650_onkey_probe(struct platform_device *pdev)
> > +{

> > +	irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F);
> > +	if (irq_f <= 0)
> > +		return -EINVAL;
> > +
> > +	irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R);
> > +	if (irq_r <= 0)
> > +		return -EINVAL;
> 
> Ugh, it would be better if you handled IRQ mapping in the MFD piece and
> passed it as resources of platform device. Then you'd simply call
> platform_get_irq() here and did not have to reach into parent device for
> "irq_dara".

These device IRQs were defined and registered with the Regmap *set*
(actually init()) APIs and thus should be pulled out using the
appropriate reverse Regmap *get* APIs here in the device.

Registering them with Regmap *and* pulling them back out again in the
same (MFD in this case) driver, only to register them as platform data
is certainly not how I see the API being designed/used.

> > +	struct regmap_irq_chip_data *irq_data;
> > +	struct device *dev, *parent;

> > +	dev = &pdev->dev;
> > +	parent = dev->parent;
> > +	i2c = to_i2c_client(parent);
> > +	irq_data = i2c_get_clientdata(i2c);
> > +
> > +	map = dev_get_regmap(parent, NULL);
> > +	if (!map)
> > +		return -ENODEV;

However, this hoop jumping is a bit crazy and for the most part
superfluous.  Instead, create a struct of attributes you wish to share
with the child devices (containing both regmap (which you should call
regmap and not map by the way) and irq_data) and pass it as either
platform data (preferred) or as device data.

If you choose the latter, you do not need to convert from 'device' to
'i2c' to do the look-up.  Since this function (probe()) is provided
with a platform_device, you can just use platform_get_drvdata() to
achieve the same as above.

If you go the preferred (platform data) route, then you should use
dev_get_platdata() instead.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog