Messages in this thread | | | Date | Wed, 29 Apr 2015 17:15:28 +0100 | From | Lee Jones <> | Subject | Re: [RESEND PATCH V2 1/2] input: misc: da9063: OnKey driver |
| |
On Wed, 29 Apr 2015, Opensource [Steve Twiss] wrote:
> > On 28 April 2015 12:57 Lee Jones [mailto:lee.jones@linaro.org] 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; > }
Sorry, that's not what I meant.
The fall-through is perfectly fine. I was tutting because you added an unrelated 'clean-up'.
> Thanks for the review comments. > The next patch set for DA9063 will follow shortly. > > Regards, > Steve
-- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
| |