On Thu, 2016-09-08 at 15:41 -0700, sathyanarayanan kuppuswamy wrote:
> 1. Rewrite GPIO expander logic to cover dynamic allocation. You have
> > to
> > check how it supposed to be in GPIO framework. IIRC gpio_base = -1
> I checked the expander driver logic. As long as we return platform
> data
> as NULL, it by default falls back to dynamic allocation. So I think
> returning NULL on gpio_base == -1 is itself enough to support the
> dynamic allocation.
>
> file: a/drivers/gpio/gpio-pca953x.c
>
> 755 pdata = dev_get_platdata(&client->dev);
> 756 if (pdata) {
> 757 irq_base = pdata->irq_base;
> 758 chip->gpio_start = pdata->gpio_base;
> 759 invert = pdata->invert;
> 760 chip->names = pdata->names;
> 761 } else {
> 762 chip->gpio_start = -1;
> 763 irq_base = 0;
> 764 }
Yes, but we get 2 parameters: IRQ line (if used) and gpio_base.
And I dunno how to proceed if we have gpio_base not set and IRQ line is
set.
So, it needs a comment what we will do. Currently it says that "Check if
the SFI record valid". So, if it's indeed so, than we can't use even
dynamic allocation.
>>> - if (gpio_base < 0)
> > > + if (gpio_base < 0) {
> > > + pr_err("%s: Unknown GPIO base number, falling
> > > back
> > > to"
> > > + "dynamic allocation\n", __func__);
> >
> > No. This not just the message you show and abort initialization, in
> > case
> > of dynamic allocation you have to proceed initialization.
> How about we go with following warning message. Since using dynamic
> gpio
> allocation is not an error, I think a warning message is more than
> enough here. Also , I don't see any value in adding "Unknown gpio
> base
> number" to the error message. So we can remove it to fit the log
> message
> into one line.
>
> + if (gpio_base == -1) {
> + pr_warn("%s: falling back to dynamic gpio
> allocation\n",
> + __func__);
See above. Perhaps we need to prevent the device driver initialization.
> --- a/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
> > > +++ b/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
> > > @@ -14,15 +14,21 @@
> > >
> > > i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
> > > +
> > > return NULL;
> > This change doesn't belong to the series.
> Submitting a separate patch to fix this this single style issue seems
> to
> be over kill. Will it be ok if I add this to my debug message patch ?
For me sounds okay, but what I know most of the maintainers doesn't
approve such changes ("white space" type of patches are most hateful).
So, I would not do this at all.
--
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
