Re: [PATCH v2 1/1] intel-mid: Fix sfi get_platform_data() return value issues

[Andy Shevchenko]

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