RE: [RESEND PATCH V2 1/2] input: misc: da9063: OnKey driver

[Opensource [Steve Twiss]]

On 28 April 2015 12:57 Lee Jones [mailto:lee.jones@xxxxxxxxxx] wrote:

> On Fri, 17 Apr 2015, S Twiss wrote:
> 
> ++++++++++++++++++++++++++++++++++++++
> >  drivers/mfd/da9063-core.c         |  55 +++++++++
> >  include/linux/mfd/da9063/pdata.h  |   1 +
> 
> This should be a seperate patch.
> 

Okay, done this now. Added a new PATCH 3/3

> >  static struct resource da9063_onkey_resources[] = {
> >  	{
> > +		.name	= "ONKEY",
> >  		.start	= DA9063_IRQ_ONKEY,
> >  		.end	= DA9063_IRQ_ONKEY,
> >  		.flags	= IORESOURCE_IRQ,
> > @@ -97,6 +98,7 @@ static const struct mfd_cell da9063_devs[] = {
> >  		.name		= DA9063_DRVNAME_ONKEY,
> >  		.num_resources	=
> ARRAY_SIZE(da9063_onkey_resources),
> >  		.resources	= da9063_onkey_resources,
> > +		.of_compatible = "dlg,da9063-onkey",
> >  	},
> >  	{
> >  		.name		= DA9063_DRVNAME_RTC,
> 
> This is lowercase, so why does "ONKEY" have to be uppercase?
> 

No real reason why this is uppercase in favour of lowercase except it
is following the convention of the existing DA9063 driver code.
Currently the DA9063 uses uppercase for its naming, there are several
others components that use the same uppercase convention, e.g. the
RTC alarm and tick interrupt and the hardware LDO limit:

> cat /proc/interrupts | grep 9063
384:          0          0          0          0  da9063-irq   0  ONKEY
385:          0          2          0          0  da9063-irq   1  ALARM
387:          0         30          0          0  da9063-irq   3  HWMON
392:          0          0          0          0  da9063-irq   8  LDO_LIM

I was going to leave this uppercase, but I can easily change it if
this is necessary.

> >  	if (ret)
> >  		dev_err(da9063->dev, "Cannot add MFD cells\n");
> >
> > +
> 
> Tut tut!
> 
> >  	return ret;
> >  }

I've changed that to remove the lazy fall-through on the error path.
It now has the following form:

@@ -229,9 +229,10 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
        ret = mfd_add_devices(da9063->dev, -1, da9063_devs,
                              ARRAY_SIZE(da9063_devs), NULL, da9063->irq_base,
                              NULL);
-       if (ret)
+       if (ret) {
                dev_err(da9063->dev, "Cannot add MFD cells\n");
-
+               return ret;
+       }
 
        return ret;
 }

Thanks for the review comments.
The next patch set for DA9063 will follow shortly.

Regards,
Steve
N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå