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

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

[Kuppuswamy Sathyanarayanan]

According to the intel_mid_sfi_get_pdata() function definition,
get_platform_data() function should returns NULL on no platform
data scenario and return ERR_PTR on platform data initialization
failures. But current device platform initialization code does not
follow this requirement. This patch fixes the return values issues
in various sfi device libs code.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 .../platform/intel-mid/device_libs/platform_lis331.c    | 13 +++++++++----
 .../platform/intel-mid/device_libs/platform_max7315.c   |  9 ++++++---
 .../platform/intel-mid/device_libs/platform_mpu3050.c   |  7 +++++--
 .../platform/intel-mid/device_libs/platform_pcal9555a.c |  8 +++++---
 .../platform/intel-mid/device_libs/platform_tca6416.c   |  7 +++++--
 arch/x86/platform/intel-mid/sfi.c                       | 17 +++++++++++++----
 6 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/arch/x86/platform/intel-mid/device_libs/platform_lis331.c b/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
index a35cf91..2fd200b 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
@@ -21,10 +21,15 @@ static void __init *lis331dl_platform_data(void *info)
 	int intr = get_gpio_by_name("accel_int");
 	int intr2nd = get_gpio_by_name("accel_2");
 
-	if (intr < 0)
-		return NULL;
-	if (intr2nd < 0)
-		return NULL;
+	if (intr < 0) {
+		pr_err("%s: invalid interrupt1 error\n", __func__);
+		return ERR_PTR(intr);
+	}
+
+	if (intr2nd < 0) {
+		pr_err("%s: invalid interrupt2 error\n", __func__);
+		return ERR_PTR(intr2nd);
+	}
 
 	i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
 	intr2nd_pdata = intr2nd + INTEL_MID_IRQ_OFFSET;
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_max7315.c b/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
index 6e075af..cc20dfc 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
@@ -31,7 +31,7 @@ static void __init *max7315_platform_data(void *info)
 	if (nr == MAX7315_NUM) {
 		pr_err("too many max7315s, we only support %d\n",
 				MAX7315_NUM);
-		return NULL;
+		return ERR_PTR(-ENODEV);
 	}
 	/* we have several max7315 on the board, we only need load several
 	 * instances of the same pca953x driver to cover them
@@ -48,8 +48,11 @@ static void __init *max7315_platform_data(void *info)
 	gpio_base = get_gpio_by_name(base_pin_name);
 	intr = get_gpio_by_name(intr_pin_name);
 
-	if (gpio_base < 0)
-		return NULL;
+	if (gpio_base < 0) {
+		pr_err("%s: invalid gpio base error\n", __func__);
+		return ERR_PTR(gpio_base);
+	}
+
 	max7315->gpio_base = gpio_base;
 	if (intr != -1) {
 		i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c b/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
index ee22864..2008824 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
@@ -19,10 +19,13 @@ static void *mpu3050_platform_data(void *info)
 	struct i2c_board_info *i2c_info = info;
 	int intr = get_gpio_by_name("mpu3050_int");
 
-	if (intr < 0)
-		return NULL;
+	if (intr < 0) {
+		pr_err("%s: invalid interrupt error\n", __func__);
+		return ERR_PTR(intr);
+	}
 
 	i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
+
 	return NULL;
 }
 
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c b/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
index 429a941..97e92a2 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
@@ -41,13 +41,15 @@ static void __init *pcal9555a_platform_data(void *info)
 	intr = get_gpio_by_name(intr_pin_name);
 
 	/* Check if the SFI record valid */
-	if (gpio_base == -1)
-		return NULL;
+	if (gpio_base == -1) {
+		pr_err("%s: invalid gpio base error\n", __func__);
+		return ERR_PTR(gpio_base);
+	}
 
 	if (nr >= PCAL9555A_NUM) {
 		pr_err("%s: Too many instances, only %d supported\n", __func__,
 		       PCAL9555A_NUM);
-		return NULL;
+		return ERR_PTR(-ENODEV);
 	}
 
 	pcal9555a = &pcal9555a_pdata[nr++];
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c b/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
index 4f41372..2796956 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
@@ -34,8 +34,11 @@ static void *tca6416_platform_data(void *info)
 	gpio_base = get_gpio_by_name(base_pin_name);
 	intr = get_gpio_by_name(intr_pin_name);
 
-	if (gpio_base < 0)
-		return NULL;
+	if (gpio_base < 0) {
+		pr_err("%s: invalid gpio base error\n", __func__);
+		return ERR_PTR(gpio_base);
+	}
+
 	tca6416.gpio_base = gpio_base;
 	if (intr >= 0) {
 		i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
diff --git a/arch/x86/platform/intel-mid/sfi.c b/arch/x86/platform/intel-mid/sfi.c
index 051d264..8e7361f 100644
--- a/arch/x86/platform/intel-mid/sfi.c
+++ b/arch/x86/platform/intel-mid/sfi.c
@@ -335,9 +335,12 @@ static void __init sfi_handle_ipc_dev(struct sfi_device_table_entry *pentry,
 
 	pr_debug("IPC bus, name = %16.16s, irq = 0x%2x\n",
 		pentry->name, pentry->irq);
+
 	pdata = intel_mid_sfi_get_pdata(dev, pentry);
-	if (IS_ERR(pdata))
+	if (IS_ERR(pdata)) {
+		pr_err("ipc_device: %s: invalid platform data\n", pentry->name);
 		return;
+	}
 
 	pdev = platform_device_alloc(pentry->name, 0);
 	if (pdev == NULL) {
@@ -371,8 +374,10 @@ static void __init sfi_handle_spi_dev(struct sfi_device_table_entry *pentry,
 		spi_info.chip_select);
 
 	pdata = intel_mid_sfi_get_pdata(dev, &spi_info);
-	if (IS_ERR(pdata))
+	if (IS_ERR(pdata)) {
+		pr_err("spi_device: %s: invalid platform data\n", pentry->name);
 		return;
+	}
 
 	spi_info.platform_data = pdata;
 	if (dev->delay)
@@ -398,8 +403,10 @@ static void __init sfi_handle_i2c_dev(struct sfi_device_table_entry *pentry,
 		i2c_info.addr);
 	pdata = intel_mid_sfi_get_pdata(dev, &i2c_info);
 	i2c_info.platform_data = pdata;
-	if (IS_ERR(pdata))
+	if (IS_ERR(pdata)) {
+		pr_err("i2c_device: %s: invalid platform data\n", pentry->name);
 		return;
+	}
 
 	if (dev->delay)
 		intel_scu_i2c_device_register(pentry->host_num, &i2c_info);
@@ -424,8 +431,10 @@ static void __init sfi_handle_sd_dev(struct sfi_device_table_entry *pentry,
 		 sd_info.max_clk,
 		 sd_info.addr);
 	pdata = intel_mid_sfi_get_pdata(dev, &sd_info);
-	if (IS_ERR(pdata))
+	if (IS_ERR(pdata)) {
+		pr_err("sd_device: %s: invalid platform data\n", pentry->name);
 		return;
+	}
 
 	/* Nothing we can do with this for now */
 	sd_info.platform_data = pdata;
-- 
2.7.4

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

[Andy Shevchenko]

On Tue, 2016-09-06 at 18:04 -0700, Kuppuswamy Sathyanarayanan wrote:
> According to the intel_mid_sfi_get_pdata() function definition,
> get_platform_data() function should returns NULL on no platform
> data scenario and return ERR_PTR on platform data initialization
> failures. But current device platform initialization code does not
> follow this requirement. This patch fixes the return values issues
> in various sfi device libs code.

I'm fine with this as long as it doesn't prevent booting.

See also comments below.

> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@
> linux.intel.com>
> ---
>  .../platform/intel-mid/device_libs/platform_lis331.c    | 13
> +++++++++----
>  .../platform/intel-mid/device_libs/platform_max7315.c   |  9 ++++++
> ---
>  .../platform/intel-mid/device_libs/platform_mpu3050.c   |  7 +++++--
>  .../platform/intel-mid/device_libs/platform_pcal9555a.c |  8 +++++---
>  .../platform/intel-mid/device_libs/platform_tca6416.c   |  7 +++++--
>  arch/x86/platform/intel-mid/sfi.c                       | 17
> +++++++++++++----
>  6 files changed, 43 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/platform/intel-mid/device_libs/platform_lis331.c 
> b/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
> index a35cf91..2fd200b 100644
> --- a/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
> @@ -21,10 +21,15 @@ static void __init *lis331dl_platform_data(void
> *info)
>  	int intr = get_gpio_by_name("accel_int");
>  	int intr2nd = get_gpio_by_name("accel_2");
>  
> -	if (intr < 0)
> -		return NULL;
> -	if (intr2nd < 0)
> -		return NULL;
> +	if (intr < 0) {
> +		pr_err("%s: invalid interrupt1 error\n", __func__);

I would rephrase to something like

#define LIS331DL_ACCEL_INT	"accel_int"

...

pr_err("%s: Can't find %s GPIO interrupt\n", __func__,
LIS331DL_ACCEL_INT);


> +		return ERR_PTR(intr);
> +	}
> +
> +	if (intr2nd < 0) {
> +		pr_err("%s: invalid interrupt2 error\n", __func__);

Ditto.

> +		return ERR_PTR(intr2nd);
> +	}
>  
>  	i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
>  	intr2nd_pdata = intr2nd + INTEL_MID_IRQ_OFFSET;
> diff --git a/arch/x86/platform/intel-
> mid/device_libs/platform_max7315.c b/arch/x86/platform/intel-
> mid/device_libs/platform_max7315.c
> index 6e075af..cc20dfc 100644
> --- a/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
> @@ -31,7 +31,7 @@ static void __init *max7315_platform_data(void
> *info)
>  	if (nr == MAX7315_NUM) {
>  		pr_err("too many max7315s, we only support %d\n",
>  				MAX7315_NUM);

"%s: too many instances, we only support %d\n", __func__, MAX7315_NUM

> -		return NULL;
> +		return ERR_PTR(-ENODEV);

-ENOMEM

>  	}
>  	/* we have several max7315 on the board, we only need load
> several
>  	 * instances of the same pca953x driver to cover them
> @@ -48,8 +48,11 @@ static void __init *max7315_platform_data(void
> *info)
>  	gpio_base = get_gpio_by_name(base_pin_name);
>  	intr = get_gpio_by_name(intr_pin_name);
>  
> -	if (gpio_base < 0)
> -		return NULL;
> +	if (gpio_base < 0) {
> +		pr_err("%s: invalid gpio base error\n", __func__);
> +		return ERR_PTR(gpio_base);

"Unknown GPIO base, falling back to dynamic allocation"

Would it work like that? (Needs more work on patch, perhaps separate
patch)

> +	}
> +
>  	max7315->gpio_base = gpio_base;
>  	if (intr != -1) {
>  		i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
> diff --git a/arch/x86/platform/intel-
> mid/device_libs/platform_mpu3050.c b/arch/x86/platform/intel-
> mid/device_libs/platform_mpu3050.c
> index ee22864..2008824 100644
> --- a/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
> @@ -19,10 +19,13 @@ static void *mpu3050_platform_data(void *info)
>  	struct i2c_board_info *i2c_info = info;
>  	int intr = get_gpio_by_name("mpu3050_int");
>  
> -	if (intr < 0)
> -		return NULL;
> +	if (intr < 0) {
> +		pr_err("%s: invalid interrupt error\n", __func__);

pr_err("%s: Can't find %s GPIO interrupt\n", __func__, MPU3050_INT);


> +		return ERR_PTR(intr);
> +	}
>  
>  	i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
> +
>  	return NULL;
>  }
>  
> diff --git a/arch/x86/platform/intel-
> mid/device_libs/platform_pcal9555a.c b/arch/x86/platform/intel-
> mid/device_libs/platform_pcal9555a.c
> index 429a941..97e92a2 100644
> --- a/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
> @@ -41,13 +41,15 @@ static void __init *pcal9555a_platform_data(void
> *info)
>  	intr = get_gpio_by_name(intr_pin_name);
>  
>  	/* Check if the SFI record valid */
> -	if (gpio_base == -1)
> -		return NULL;
> +	if (gpio_base == -1) {
> +		pr_err("%s: invalid gpio base error\n", __func__);
> +		return ERR_PTR(gpio_base);

Same as above for gpio_base.

> +	}
>  
>  	if (nr >= PCAL9555A_NUM) {
>  		pr_err("%s: Too many instances, only %d supported\n",
> __func__,
>  		       PCAL9555A_NUM);
> -		return NULL;
> +		return ERR_PTR(-ENODEV);

-ENOMEM

>  	}
>  
>  	pcal9555a = &pcal9555a_pdata[nr++];
> diff --git a/arch/x86/platform/intel-
> mid/device_libs/platform_tca6416.c b/arch/x86/platform/intel-
> mid/device_libs/platform_tca6416.c
> index 4f41372..2796956 100644
> --- a/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
> @@ -34,8 +34,11 @@ static void *tca6416_platform_data(void *info)
>  	gpio_base = get_gpio_by_name(base_pin_name);
>  	intr = get_gpio_by_name(intr_pin_name);
>  
> -	if (gpio_base < 0)
> -		return NULL;
> +	if (gpio_base < 0) {
> +		pr_err("%s: invalid gpio base error\n", __func__);
> +		return ERR_PTR(gpio_base);

Same as above for gpio_base.

> +	}
> +
>  	tca6416.gpio_base = gpio_base;
>  	if (intr >= 0) {
>  		i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
> diff --git a/arch/x86/platform/intel-mid/sfi.c
> b/arch/x86/platform/intel-mid/sfi.c
> index 051d264..8e7361f 100644
> --- a/arch/x86/platform/intel-mid/sfi.c
> +++ b/arch/x86/platform/intel-mid/sfi.c
> @@ -335,9 +335,12 @@ static void __init sfi_handle_ipc_dev(struct
> sfi_device_table_entry *pentry,
>  
>  	pr_debug("IPC bus, name = %16.16s, irq = 0x%2x\n",
>  		pentry->name, pentry->irq);
> +
>  	pdata = intel_mid_sfi_get_pdata(dev, pentry);
> -	if (IS_ERR(pdata))
> +	if (IS_ERR(pdata)) {
> +		pr_err("ipc_device: %s: invalid platform data\n",
> pentry->name);
>  		return;
> +	}

This is actually needs more work. We have duplication in sfi.c and
platform_ipc.c.

>  
>  	pdev = platform_device_alloc(pentry->name, 0);
>  	if (pdev == NULL) {
> @@ -371,8 +374,10 @@ static void __init sfi_handle_spi_dev(struct
> sfi_device_table_entry *pentry,
>  		spi_info.chip_select);
>  
>  	pdata = intel_mid_sfi_get_pdata(dev, &spi_info);
> -	if (IS_ERR(pdata))
> +	if (IS_ERR(pdata)) {
> 

> +		pr_err("spi_device: %s: invalid platform data\n",
> pentry->name);

Since you print messages in device_libs files I would drop this one
because it has no value. OTOH you can move it to debug level and
rephrase:

pr_debug("%s: Can't get platform data for %s\n", __func__ [or "SPI
..."], pentry->name);

>  		return;
> +	}
>  
>  	spi_info.platform_data = pdata;
>  	if (dev->delay)
> @@ -398,8 +403,10 @@ static void __init sfi_handle_i2c_dev(struct
> sfi_device_table_entry *pentry,
>  		i2c_info.addr);
>  	pdata = intel_mid_sfi_get_pdata(dev, &i2c_info);
>  	i2c_info.platform_data = pdata;
> -	if (IS_ERR(pdata))
> +	if (IS_ERR(pdata)) {
> +		pr_err("i2c_device: %s: invalid platform data\n",
> pentry->name);
>  		return;

Ditto.

> +	}
>  
>  	if (dev->delay)
>  		intel_scu_i2c_device_register(pentry->host_num,
> &i2c_info);
> @@ -424,8 +431,10 @@ static void __init sfi_handle_sd_dev(struct
> sfi_device_table_entry *pentry,
>  		 sd_info.max_clk,
>  		 sd_info.addr);
>  	pdata = intel_mid_sfi_get_pdata(dev, &sd_info);
> -	if (IS_ERR(pdata))
> +	if (IS_ERR(pdata)) {
> +		pr_err("sd_device: %s: invalid platform data\n",
> pentry->name);
>  		return;
> +	}

Ditto.

>  
>  	/* Nothing we can do with this for now */
>  	sd_info.platform_data = pdata;

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[sathyanarayanan kuppuswamy]

On 09/07/2016 05:15 AM, Andy Shevchenko wrote:
> On Tue, 2016-09-06 at 18:04 -0700, Kuppuswamy Sathyanarayanan wrote:
>> According to the intel_mid_sfi_get_pdata() function definition,
>> get_platform_data() function should returns NULL on no platform
>> data scenario and return ERR_PTR on platform data initialization
>> failures. But current device platform initialization code does not
>> follow this requirement. This patch fixes the return values issues
>> in various sfi device libs code.
> I'm fine with this as long as it doesn't prevent booting.
>
> See also comments below.
Thanks for the review. Please find my comments inline.
>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@
>> linux.intel.com>
>> ---
>>   .../platform/intel-mid/device_libs/platform_lis331.c    | 13
>> +++++++++----
>>   .../platform/intel-mid/device_libs/platform_max7315.c   |  9 ++++++
>> ---
>>   .../platform/intel-mid/device_libs/platform_mpu3050.c   |  7 +++++--
>>   .../platform/intel-mid/device_libs/platform_pcal9555a.c |  8 +++++---
>>   .../platform/intel-mid/device_libs/platform_tca6416.c   |  7 +++++--
>>   arch/x86/platform/intel-mid/sfi.c                       | 17
>> +++++++++++++----
>>   6 files changed, 43 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
>> b/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
>> index a35cf91..2fd200b 100644
>> --- a/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
>> +++ b/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
>> @@ -21,10 +21,15 @@ static void __init *lis331dl_platform_data(void
>> *info)
>>   	int intr = get_gpio_by_name("accel_int");
>>   	int intr2nd = get_gpio_by_name("accel_2");
>>   
>> -	if (intr < 0)
>> -		return NULL;
>> -	if (intr2nd < 0)
>> -		return NULL;
>> +	if (intr < 0) {
>> +		pr_err("%s: invalid interrupt1 error\n", __func__);
> I would rephrase to something like
>
> #define LIS331DL_ACCEL_INT	"accel_int"
>
> ...
>
> pr_err("%s: Can't find %s GPIO interrupt\n", __func__,
> LIS331DL_ACCEL_INT);
>
Agreed. Will be fixed in next patch version.
>> +		return ERR_PTR(intr);
>> +	}
>> +
>> +	if (intr2nd < 0) {
>> +		pr_err("%s: invalid interrupt2 error\n", __func__);
> Ditto.
Agreed. Will be fixed in next patch version.
>
>> +		return ERR_PTR(intr2nd);
>> +	}
>>   
>>   	i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
>>   	intr2nd_pdata = intr2nd + INTEL_MID_IRQ_OFFSET;
>> diff --git a/arch/x86/platform/intel-
>> mid/device_libs/platform_max7315.c b/arch/x86/platform/intel-
>> mid/device_libs/platform_max7315.c
>> index 6e075af..cc20dfc 100644
>> --- a/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
>> +++ b/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
>> @@ -31,7 +31,7 @@ static void __init *max7315_platform_data(void
>> *info)
>>   	if (nr == MAX7315_NUM) {
>>   		pr_err("too many max7315s, we only support %d\n",
>>   				MAX7315_NUM);
> "%s: too many instances, we only support %d\n", __func__, MAX7315_NUM
>
>> -		return NULL;
>> +		return ERR_PTR(-ENODEV);
> -ENOMEM
Agreed. Will be fixed in next patch version.
>
>>   	}
>>   	/* we have several max7315 on the board, we only need load
>> several
>>   	 * instances of the same pca953x driver to cover them
>> @@ -48,8 +48,11 @@ static void __init *max7315_platform_data(void
>> *info)
>>   	gpio_base = get_gpio_by_name(base_pin_name);
>>   	intr = get_gpio_by_name(intr_pin_name);
>>   
>> -	if (gpio_base < 0)
>> -		return NULL;
>> +	if (gpio_base < 0) {
>> +		pr_err("%s: invalid gpio base error\n", __func__);
>> +		return ERR_PTR(gpio_base);
> "Unknown GPIO base, falling back to dynamic allocation"
>
> Would it work like that? (Needs more work on patch, perhaps separate
> patch)
Agreed. Will be fixed in next patch version.
>
>> +	}
>> +
>>   	max7315->gpio_base = gpio_base;
>>   	if (intr != -1) {
>>   		i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
>> diff --git a/arch/x86/platform/intel-
>> mid/device_libs/platform_mpu3050.c b/arch/x86/platform/intel-
>> mid/device_libs/platform_mpu3050.c
>> index ee22864..2008824 100644
>> --- a/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
>> +++ b/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
>> @@ -19,10 +19,13 @@ static void *mpu3050_platform_data(void *info)
>>   	struct i2c_board_info *i2c_info = info;
>>   	int intr = get_gpio_by_name("mpu3050_int");
>>   
>> -	if (intr < 0)
>> -		return NULL;
>> +	if (intr < 0) {
>> +		pr_err("%s: invalid interrupt error\n", __func__);
> pr_err("%s: Can't find %s GPIO interrupt\n", __func__, MPU3050_INT);
Agreed. Will be fixed in next patch version.
>
>> +		return ERR_PTR(intr);
>> +	}
>>   
>>   	i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
>> +
>>   	return NULL;
>>   }
>>   
>> diff --git a/arch/x86/platform/intel-
>> mid/device_libs/platform_pcal9555a.c b/arch/x86/platform/intel-
>> mid/device_libs/platform_pcal9555a.c
>> index 429a941..97e92a2 100644
>> --- a/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
>> +++ b/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
>> @@ -41,13 +41,15 @@ static void __init *pcal9555a_platform_data(void
>> *info)
>>   	intr = get_gpio_by_name(intr_pin_name);
>>   
>>   	/* Check if the SFI record valid */
>> -	if (gpio_base == -1)
>> -		return NULL;
>> +	if (gpio_base == -1) {
>> +		pr_err("%s: invalid gpio base error\n", __func__);
>> +		return ERR_PTR(gpio_base);
> Same as above for gpio_base.
Same as above
>> +	}
>>   
>>   	if (nr >= PCAL9555A_NUM) {
>>   		pr_err("%s: Too many instances, only %d supported\n",
>> __func__,
>>   		       PCAL9555A_NUM);
>> -		return NULL;
>> +		return ERR_PTR(-ENODEV);
> -ENOMEM
Agreed. Will be fixed in next patch version.
>
>>   	}
>>   
>>   	pcal9555a = &pcal9555a_pdata[nr++];
>> diff --git a/arch/x86/platform/intel-
>> mid/device_libs/platform_tca6416.c b/arch/x86/platform/intel-
>> mid/device_libs/platform_tca6416.c
>> index 4f41372..2796956 100644
>> --- a/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
>> +++ b/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
>> @@ -34,8 +34,11 @@ static void *tca6416_platform_data(void *info)
>>   	gpio_base = get_gpio_by_name(base_pin_name);
>>   	intr = get_gpio_by_name(intr_pin_name);
>>   
>> -	if (gpio_base < 0)
>> -		return NULL;
>> +	if (gpio_base < 0) {
>> +		pr_err("%s: invalid gpio base error\n", __func__);
>> +		return ERR_PTR(gpio_base);
> Same as above for gpio_base.
Same as above
>
>> +	}
>> +
>>   	tca6416.gpio_base = gpio_base;
>>   	if (intr >= 0) {
>>   		i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
>> diff --git a/arch/x86/platform/intel-mid/sfi.c
>> b/arch/x86/platform/intel-mid/sfi.c
>> index 051d264..8e7361f 100644
>> --- a/arch/x86/platform/intel-mid/sfi.c
>> +++ b/arch/x86/platform/intel-mid/sfi.c
>> @@ -335,9 +335,12 @@ static void __init sfi_handle_ipc_dev(struct
>> sfi_device_table_entry *pentry,
>>   
>>   	pr_debug("IPC bus, name = %16.16s, irq = 0x%2x\n",
>>   		pentry->name, pentry->irq);
>> +
>>   	pdata = intel_mid_sfi_get_pdata(dev, pentry);
>> -	if (IS_ERR(pdata))
>> +	if (IS_ERR(pdata)) {
>> +		pr_err("ipc_device: %s: invalid platform data\n",
>> pentry->name);
>>   		return;
>> +	}
> This is actually needs more work. We have duplication in sfi.c and
> platform_ipc.c.
Yes. But platform_ipc.c implements custom ipc handler for audio ipc 
device. Even though there are duplications between custom handler and 
generic handler in sfi.c, I think its bit early to optimize this. I 
think we should revisit this once we have one more implementation of 
custom ipc handler.
>
>>   
>>   	pdev = platform_device_alloc(pentry->name, 0);
>>   	if (pdev == NULL) {
>> @@ -371,8 +374,10 @@ static void __init sfi_handle_spi_dev(struct
>> sfi_device_table_entry *pentry,
>>   		spi_info.chip_select);
>>   
>>   	pdata = intel_mid_sfi_get_pdata(dev, &spi_info);
>> -	if (IS_ERR(pdata))
>> +	if (IS_ERR(pdata)) {
>>
>> +		pr_err("spi_device: %s: invalid platform data\n",
>> pentry->name);
> Since you print messages in device_libs files I would drop this one
> because it has no value. OTOH you can move it to debug level and
> rephrase:
>
> pr_debug("%s: Can't get platform data for %s\n", __func__ [or "SPI
> ..."], pentry->name);
Agreed. Will be fixed in next version.
>
>>   		return;
>> +	}
>>   
>>   	spi_info.platform_data = pdata;
>>   	if (dev->delay)
>> @@ -398,8 +403,10 @@ static void __init sfi_handle_i2c_dev(struct
>> sfi_device_table_entry *pentry,
>>   		i2c_info.addr);
>>   	pdata = intel_mid_sfi_get_pdata(dev, &i2c_info);
>>   	i2c_info.platform_data = pdata;
>> -	if (IS_ERR(pdata))
>> +	if (IS_ERR(pdata)) {
>> +		pr_err("i2c_device: %s: invalid platform data\n",
>> pentry->name);
>>   		return;
> Ditto.
Same as above.
>
>> +	}
>>   
>>   	if (dev->delay)
>>   		intel_scu_i2c_device_register(pentry->host_num,
>> &i2c_info);
>> @@ -424,8 +431,10 @@ static void __init sfi_handle_sd_dev(struct
>> sfi_device_table_entry *pentry,
>>   		 sd_info.max_clk,
>>   		 sd_info.addr);
>>   	pdata = intel_mid_sfi_get_pdata(dev, &sd_info);
>> -	if (IS_ERR(pdata))
>> +	if (IS_ERR(pdata)) {
>> +		pr_err("sd_device: %s: invalid platform data\n",
>> pentry->name);
>>   		return;
>> +	}
> Ditto.
Same as above.
>
>>   
>>   	/* Nothing we can do with this for now */
>>   	sd_info.platform_data = pdata;

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

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

[Kuppuswamy Sathyanarayanan]

According to the intel_mid_sfi_get_pdata() function definition,
get_platform_data() function should returns NULL on no platform
data scenario and return ERR_PTR on platform data initialization
failures. But current device platform initialization code does not
follow this requirement. This patch fixes the return values issues
in various sfi device libs code.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 .../intel-mid/device_libs/platform_lis331.c        | 22 ++++++++++++++++------
 .../intel-mid/device_libs/platform_max7315.c       | 12 ++++++++----
 .../intel-mid/device_libs/platform_mpu3050.c       | 12 +++++++++---
 .../intel-mid/device_libs/platform_pcal9555a.c     |  7 +++++--
 .../intel-mid/device_libs/platform_tca6416.c       |  6 +++++-
 arch/x86/platform/intel-mid/sfi.c                  | 21 +++++++++++++++++----
 6 files changed, 60 insertions(+), 20 deletions(-)

diff --git a/arch/x86/platform/intel-mid/device_libs/platform_lis331.c b/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
index a35cf91..393c23e 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
@@ -14,17 +14,27 @@
 #include <linux/gpio.h>
 #include <asm/intel-mid.h>
 
+#define LIS331DL_ACCEL_INT1	"accel_int"
+#define LIS331DL_ACCEL_INT2	"accel_2"
+
 static void __init *lis331dl_platform_data(void *info)
 {
 	static short intr2nd_pdata;
 	struct i2c_board_info *i2c_info = info;
-	int intr = get_gpio_by_name("accel_int");
-	int intr2nd = get_gpio_by_name("accel_2");
+	int intr = get_gpio_by_name(LIS331DL_ACCEL_INT1);
+	int intr2nd = get_gpio_by_name(LIS331DL_ACCEL_INT2);
+
+	if (intr < 0) {
+		pr_err("%s: Can't find %s GPIO interrupt\n", __func__,
+		       LIS331DL_ACCEL_INT1);
+		return ERR_PTR(intr);
+	}
 
-	if (intr < 0)
-		return NULL;
-	if (intr2nd < 0)
-		return NULL;
+	if (intr2nd < 0) {
+		pr_err("%s: Can't find %s GPIO interrupt\n", __func__,
+		       LIS331DL_ACCEL_INT2);
+		return ERR_PTR(intr2nd);
+	}
 
 	i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
 	intr2nd_pdata = intr2nd + INTEL_MID_IRQ_OFFSET;
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_max7315.c b/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
index 6e075af..2cd3c7f 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
@@ -29,9 +29,9 @@ static void __init *max7315_platform_data(void *info)
 	char intr_pin_name[SFI_NAME_LEN + 1];
 
 	if (nr == MAX7315_NUM) {
-		pr_err("too many max7315s, we only support %d\n",
-				MAX7315_NUM);
-		return NULL;
+		pr_err("%s: too many max7315s, we only support %d\n",
+		       __func__, MAX7315_NUM);
+		return ERR_PTR(-ENOMEM);
 	}
 	/* we have several max7315 on the board, we only need load several
 	 * instances of the same pca953x driver to cover them
@@ -48,8 +48,12 @@ static void __init *max7315_platform_data(void *info)
 	gpio_base = get_gpio_by_name(base_pin_name);
 	intr = get_gpio_by_name(intr_pin_name);
 
-	if (gpio_base < 0)
+	if (gpio_base < 0) {
+		pr_err("%s: Unknown GPIO base number, falling back to"
+		       "dynamic allocation\n", __func__);
 		return NULL;
+	}
+
 	max7315->gpio_base = gpio_base;
 	if (intr != -1) {
 		i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c b/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
index ee22864..4b33aab 100644
--- 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 @@
 #include <linux/i2c.h>
 #include <asm/intel-mid.h>
 
+#define MPU3050_INT		"mpu3050_int"
+
 static void *mpu3050_platform_data(void *info)
 {
 	struct i2c_board_info *i2c_info = info;
-	int intr = get_gpio_by_name("mpu3050_int");
+	int intr = get_gpio_by_name(MPU3050_INT);
 
-	if (intr < 0)
-		return NULL;
+	if (intr < 0) {
+		pr_err("%s: Can't find %s GPIO interrupt\n", __func__,
+		       MPU3050_INT);
+		return ERR_PTR(intr);
+	}
 
 	i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
+
 	return NULL;
 }
 
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c b/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
index 429a941..190b2d2 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
@@ -41,13 +41,16 @@ static void __init *pcal9555a_platform_data(void *info)
 	intr = get_gpio_by_name(intr_pin_name);
 
 	/* Check if the SFI record valid */
-	if (gpio_base == -1)
+	if (gpio_base == -1) {
+		pr_err("%s: Unknown GPIO base number, falling back to dynamic"
+		       "allocation\n", __func__);
 		return NULL;
+	}
 
 	if (nr >= PCAL9555A_NUM) {
 		pr_err("%s: Too many instances, only %d supported\n", __func__,
 		       PCAL9555A_NUM);
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 	}
 
 	pcal9555a = &pcal9555a_pdata[nr++];
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c b/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
index 4f41372..145b03d 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
@@ -34,8 +34,12 @@ static void *tca6416_platform_data(void *info)
 	gpio_base = get_gpio_by_name(base_pin_name);
 	intr = get_gpio_by_name(intr_pin_name);
 
-	if (gpio_base < 0)
+	if (gpio_base < 0) {
+		pr_err("%s: Unknown GPIO base number, falling back to dynamic"
+		       "allocation\n", __func__);
 		return NULL;
+	}
+
 	tca6416.gpio_base = gpio_base;
 	if (intr >= 0) {
 		i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
diff --git a/arch/x86/platform/intel-mid/sfi.c b/arch/x86/platform/intel-mid/sfi.c
index 051d264..78ee7eb 100644
--- a/arch/x86/platform/intel-mid/sfi.c
+++ b/arch/x86/platform/intel-mid/sfi.c
@@ -335,9 +335,13 @@ static void __init sfi_handle_ipc_dev(struct sfi_device_table_entry *pentry,
 
 	pr_debug("IPC bus, name = %16.16s, irq = 0x%2x\n",
 		pentry->name, pentry->irq);
+
 	pdata = intel_mid_sfi_get_pdata(dev, pentry);
-	if (IS_ERR(pdata))
+	if (IS_ERR(pdata)) {
+		pr_debug("%s:  Can't get platform data for %s\n", __func__,
+			 pentry->name);
 		return;
+	}
 
 	pdev = platform_device_alloc(pentry->name, 0);
 	if (pdev == NULL) {
@@ -371,8 +375,11 @@ static void __init sfi_handle_spi_dev(struct sfi_device_table_entry *pentry,
 		spi_info.chip_select);
 
 	pdata = intel_mid_sfi_get_pdata(dev, &spi_info);
-	if (IS_ERR(pdata))
+	if (IS_ERR(pdata)) {
+		pr_debug("%s:  Can't get platform data for %s\n", __func__,
+			 pentry->name);
 		return;
+	}
 
 	spi_info.platform_data = pdata;
 	if (dev->delay)
@@ -398,8 +405,11 @@ static void __init sfi_handle_i2c_dev(struct sfi_device_table_entry *pentry,
 		i2c_info.addr);
 	pdata = intel_mid_sfi_get_pdata(dev, &i2c_info);
 	i2c_info.platform_data = pdata;
-	if (IS_ERR(pdata))
+	if (IS_ERR(pdata)) {
+		pr_debug("%s:  Can't get platform data for %s\n", __func__,
+			 pentry->name);
 		return;
+	}
 
 	if (dev->delay)
 		intel_scu_i2c_device_register(pentry->host_num, &i2c_info);
@@ -424,8 +434,11 @@ static void __init sfi_handle_sd_dev(struct sfi_device_table_entry *pentry,
 		 sd_info.max_clk,
 		 sd_info.addr);
 	pdata = intel_mid_sfi_get_pdata(dev, &sd_info);
-	if (IS_ERR(pdata))
+	if (IS_ERR(pdata)) {
+		pr_debug("%s:  Can't get platform data for %s\n", __func__,
+			 pentry->name);
 		return;
+	}
 
 	/* Nothing we can do with this for now */
 	sd_info.platform_data = pdata;
-- 
2.7.4

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

[Andy Shevchenko]

On Wed, 2016-09-07 at 17:04 -0700, sathyanarayanan kuppuswamy wrote:

> --- a/arch/x86/platform/intel-mid/sfi.c
> > > +++ b/arch/x86/platform/intel-mid/sfi.c
> > > @@ -335,9 +335,12 @@ static void __init sfi_handle_ipc_dev(struct
> > > sfi_device_table_entry *pentry,
> > >   
> > >   	pr_debug("IPC bus, name = %16.16s, irq = 0x%2x\n",
> > >   		pentry->name, pentry->irq);
> > > +
> > >   	pdata = intel_mid_sfi_get_pdata(dev, pentry);
> > > -	if (IS_ERR(pdata))
> > > +	if (IS_ERR(pdata)) {
> > > +		pr_err("ipc_device: %s: invalid platform data\n",
> > > pentry->name);
> > >   		return;
> > > +	}
> > This is actually needs more work. We have duplication in sfi.c and
> > platform_ipc.c.
> Yes. But platform_ipc.c implements custom ipc handler for audio ipc 
> device. Even though there are duplications between custom handler and 
> generic handler in sfi.c, I think its bit early to optimize this. I 
> think we should revisit this once we have one more implementation of 
> custom ipc handler.

Definitely it's out of scope for now.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Andy Shevchenko]

On Wed, 2016-09-07 at 17:05 -0700, Kuppuswamy Sathyanarayanan wrote:
> According to the intel_mid_sfi_get_pdata() function definition,

"function" is implied, remove the word.

> get_platform_data() function 

Ditto.

> should returns NULL on no platform

returns -> return

> data scenario and return ERR_PTR on platform data initialization
> failures. But current device platform initialization code does not
> follow this requirement. This patch fixes the return values issues
> in various sfi device libs code.

sfi -> SFI


Looking into patch I would consider to split it to series:

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
(perhaps a defined constant) will do the trick.
2. Fix the actual return codes (maybe with changes to sfi.c).
3. Fix and add error messages.
4+ (in the future) Address code duplication

> --- a/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
> @@ -29,9 +29,9 @@ static void __init *max7315_platform_data(void
> *info)
>  	char intr_pin_name[SFI_NAME_LEN + 1];
>  
>  	if (nr == MAX7315_NUM) {
> -		pr_err("too many max7315s, we only support %d\n",
> -				MAX7315_NUM);
> -		return NULL;
> +		pr_err("%s: too many max7315s, we only support %d\n",
> +		       __func__, MAX7315_NUM);

Use the same as for PCAL9555A:

pr_err("%s: Too many instances, only %d supported\n",

> @@ -48,8 +48,12 @@ static void __init *max7315_platform_data(void
> *info)
>  	gpio_base = get_gpio_by_name(base_pin_name);
>  	intr = get_gpio_by_name(intr_pin_name);
>  
> -	if (gpio_base < 0)
> +	if (gpio_base < 0) {
> +		pr_err("%s: Unknown GPIO base number, falling back
> to"
> +		       "dynamic allocation\n", __func__);

Don't split literals.

pr_err("...long literal...\n",
       args...);

No. This not just the message you show and abort initialization, in case
of dynamic allocation you have to proceed initialization.

> index ee22864..4b33aab 100644
> --- 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.

>  }
>  
> diff --git a/arch/x86/platform/intel-
> mid/device_libs/platform_pcal9555a.c b/arch/x86/platform/intel-
> mid/device_libs/platform_pcal9555a.c
> index 429a941..190b2d2 100644
> --- a/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
> @@ -41,13 +41,16 @@ static void __init *pcal9555a_platform_data(void
> *info)
>  	intr = get_gpio_by_name(intr_pin_name);
>  
>  	/* Check if the SFI record valid */
> -	if (gpio_base == -1)
> +	if (gpio_base == -1) {
> +		pr_err("%s: Unknown GPIO base number, falling back to
> dynamic"
> +		       "allocation\n", __func__);
>  		return NULL;

Same comment as above for gpio_base.

> 
> --- a/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
> @@ -34,8 +34,12 @@ static void *tca6416_platform_data(void *info)
>  	gpio_base = get_gpio_by_name(base_pin_name);
>  	intr = get_gpio_by_name(intr_pin_name);
>  
> -	if (gpio_base < 0)
> +	if (gpio_base < 0) {
> +		pr_err("%s: Unknown GPIO base number, falling back to
> dynamic"
> +		       "allocation\n", __func__);

Ditto.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[sathyanarayanan kuppuswamy]

Thanks for the review Andy. Please check my comments inline.


On 09/08/2016 05:51 AM, Andy Shevchenko wrote:
> On Wed, 2016-09-07 at 17:05 -0700, Kuppuswamy Sathyanarayanan wrote:
>> According to the intel_mid_sfi_get_pdata() function definition,
> "function" is implied, remove the word.
Will be fixed in next version.
>
>> get_platform_data() function
> Ditto.
Same as above.
>
>> should returns NULL on no platform
> returns -> return
Same as above.
>
>> data scenario and return ERR_PTR on platform data initialization
>> failures. But current device platform initialization code does not
>> follow this requirement. This patch fixes the return values issues
>> in various sfi device libs code.
> sfi -> SFI
Same as above.
>
>
> Looking into patch I would consider to split it to series:
>
> 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         }

> (perhaps a defined constant) will do the trick.
> 2. Fix the actual return codes (maybe with changes to sfi.c).
> 3. Fix and add error messages.
I can split the patch into two. One for return code fix and another for 
adding error messages.
> 4+ (in the future) Address code duplication
Agreed.
>
>> --- a/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
>> +++ b/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
>> @@ -29,9 +29,9 @@ static void __init *max7315_platform_data(void
>> *info)
>>   	char intr_pin_name[SFI_NAME_LEN + 1];
>>   
>>   	if (nr == MAX7315_NUM) {
>> -		pr_err("too many max7315s, we only support %d\n",
>> -				MAX7315_NUM);
>> -		return NULL;
>> +		pr_err("%s: too many max7315s, we only support %d\n",
>> +		       __func__, MAX7315_NUM);
> Use the same as for PCAL9555A:
>
> pr_err("%s: Too many instances, only %d supported\n",
Will be fixed in next version.
>
>> @@ -48,8 +48,12 @@ static void __init *max7315_platform_data(void
>> *info)
>>   	gpio_base = get_gpio_by_name(base_pin_name);
>>   	intr = get_gpio_by_name(intr_pin_name);
>>   
>> -	if (gpio_base < 0)
>> +	if (gpio_base < 0) {
>> +		pr_err("%s: Unknown GPIO base number, falling back
>> to"
>> +		       "dynamic allocation\n", __func__);
> Don't split literals.
>
> pr_err("...long literal...\n",
>         args...);
>
> 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__);

>
>> index ee22864..4b33aab 100644
>> --- 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 ?
>
>>   }
>>   
>> diff --git a/arch/x86/platform/intel-
>> mid/device_libs/platform_pcal9555a.c b/arch/x86/platform/intel-
>> mid/device_libs/platform_pcal9555a.c
>> index 429a941..190b2d2 100644
>> --- a/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
>> +++ b/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
>> @@ -41,13 +41,16 @@ static void __init *pcal9555a_platform_data(void
>> *info)
>>   	intr = get_gpio_by_name(intr_pin_name);
>>   
>>   	/* Check if the SFI record valid */
>> -	if (gpio_base == -1)
>> +	if (gpio_base == -1) {
>> +		pr_err("%s: Unknown GPIO base number, falling back to
>> dynamic"
>> +		       "allocation\n", __func__);
>>   		return NULL;
> Same comment as above for gpio_base.
Will be fixed in next version.
>
>> --- a/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
>> +++ b/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
>> @@ -34,8 +34,12 @@ static void *tca6416_platform_data(void *info)
>>   	gpio_base = get_gpio_by_name(base_pin_name);
>>   	intr = get_gpio_by_name(intr_pin_name);
>>   
>> -	if (gpio_base < 0)
>> +	if (gpio_base < 0) {
>> +		pr_err("%s: Unknown GPIO base number, falling back to
>> dynamic"
>> +		       "allocation\n", __func__);
> Ditto.
>
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

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

[Kuppuswamy Sathyanarayanan]

According to the intel_mid_sfi_get_pdata() definition,
get_platform_data() should return NULL on no platform
data scenario and return ERR_PTR on platform data
initialization failures. But current device platform
initialization code does not follow this requirement.
This patch fixes the return values issues in various SFI
device libs code.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/platform/intel-mid/device_libs/platform_lis331.c    | 4 ++--
 arch/x86/platform/intel-mid/device_libs/platform_max7315.c   | 2 +-
 arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c   | 2 +-
 arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/platform/intel-mid/device_libs/platform_lis331.c b/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
index a35cf91..8be5d40 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
@@ -22,9 +22,9 @@ static void __init *lis331dl_platform_data(void *info)
 	int intr2nd = get_gpio_by_name("accel_2");
 
 	if (intr < 0)
-		return NULL;
+		return ERR_PTR(intr);
 	if (intr2nd < 0)
-		return NULL;
+		ERR_PTR(intr2nd);
 
 	i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
 	intr2nd_pdata = intr2nd + INTEL_MID_IRQ_OFFSET;
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_max7315.c b/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
index 6e075af..34dc59d 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
@@ -31,7 +31,7 @@ static void __init *max7315_platform_data(void *info)
 	if (nr == MAX7315_NUM) {
 		pr_err("too many max7315s, we only support %d\n",
 				MAX7315_NUM);
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 	}
 	/* we have several max7315 on the board, we only need load several
 	 * instances of the same pca953x driver to cover them
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c b/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
index ee22864..f434f88 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
@@ -20,7 +20,7 @@ static void *mpu3050_platform_data(void *info)
 	int intr = get_gpio_by_name("mpu3050_int");
 
 	if (intr < 0)
-		return NULL;
+		return ERR_PTR(intr);
 
 	i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
 	return NULL;
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c b/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
index 429a941..563f77f 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
@@ -47,7 +47,7 @@ static void __init *pcal9555a_platform_data(void *info)
 	if (nr >= PCAL9555A_NUM) {
 		pr_err("%s: Too many instances, only %d supported\n", __func__,
 		       PCAL9555A_NUM);
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 	}
 
 	pcal9555a = &pcal9555a_pdata[nr++];
-- 
2.7.4

[PATCH v3 2/3] intel-mid: Add valid error messages on init failure

[Kuppuswamy Sathyanarayanan]

Added valid error/warning messages to platform data
initalization failures in SFI device libs code.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 .../intel-mid/device_libs/platform_emc1403.c        | 18 ++++++++++++++----
 .../platform/intel-mid/device_libs/platform_ipc.c   |  5 ++++-
 .../intel-mid/device_libs/platform_lis331.c         | 20 +++++++++++++++-----
 .../intel-mid/device_libs/platform_max7315.c        | 10 +++++++---
 .../intel-mid/device_libs/platform_mpu3050.c        |  9 +++++++--
 .../intel-mid/device_libs/platform_pcal9555a.c      |  5 ++++-
 .../intel-mid/device_libs/platform_tca6416.c        |  6 +++++-
 arch/x86/platform/intel-mid/sfi.c                   | 21 +++++++++++++++++----
 8 files changed, 73 insertions(+), 21 deletions(-)

diff --git a/arch/x86/platform/intel-mid/device_libs/platform_emc1403.c b/arch/x86/platform/intel-mid/device_libs/platform_emc1403.c
index c259fb6..bd776b04 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_emc1403.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_emc1403.c
@@ -15,17 +15,27 @@
 #include <linux/i2c.h>
 #include <asm/intel-mid.h>
 
+#define EMC1403_THERMAL_INT		"thermal_int"
+#define EMC1403_THERMAL_ALERT_INT	"thermal_alert"
+
 static void __init *emc1403_platform_data(void *info)
 {
 	static short intr2nd_pdata;
 	struct i2c_board_info *i2c_info = info;
-	int intr = get_gpio_by_name("thermal_int");
-	int intr2nd = get_gpio_by_name("thermal_alert");
+	int intr = get_gpio_by_name(EMC1403_THERMAL_INT);
+	int intr2nd = get_gpio_by_name(EMC1403_THERMAL_ALERT_INT);
 
-	if (intr < 0)
+	if (intr < 0) {
+		pr_err("%s: Can't find %s GPIO interrupt\n", __func__,
+		       EMC1403_THERMAL_INT);
 		return NULL;
-	if (intr2nd < 0)
+	}
+
+	if (intr2nd < 0) {
+		pr_err("%s: Can't find %s GPIO interrupt\n", __func__,
+		       EMC1403_THERMAL_ALERT_INT);
 		return NULL;
+	}
 
 	i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
 	intr2nd_pdata = intr2nd + INTEL_MID_IRQ_OFFSET;
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_ipc.c b/arch/x86/platform/intel-mid/device_libs/platform_ipc.c
index a84b73d..6704694 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_ipc.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_ipc.c
@@ -42,8 +42,11 @@ void __init ipc_device_handler(struct sfi_device_table_entry *pentry,
 	 * On Medfield the platform device creation is handled by the MSIC
 	 * MFD driver so we don't need to do it here.
 	 */
-	if (intel_mid_has_msic())
+	if (intel_mid_has_msic()) {
+		pr_err("%s: device %s will be handled by MSIC mfd driver\n",
+		       __func__, pentry->name);
 		return;
+	}
 
 	pdev = platform_device_alloc(pentry->name, 0);
 	if (pdev == NULL) {
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_lis331.c b/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
index 8be5d40..393c23e 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
@@ -14,17 +14,27 @@
 #include <linux/gpio.h>
 #include <asm/intel-mid.h>
 
+#define LIS331DL_ACCEL_INT1	"accel_int"
+#define LIS331DL_ACCEL_INT2	"accel_2"
+
 static void __init *lis331dl_platform_data(void *info)
 {
 	static short intr2nd_pdata;
 	struct i2c_board_info *i2c_info = info;
-	int intr = get_gpio_by_name("accel_int");
-	int intr2nd = get_gpio_by_name("accel_2");
+	int intr = get_gpio_by_name(LIS331DL_ACCEL_INT1);
+	int intr2nd = get_gpio_by_name(LIS331DL_ACCEL_INT2);
 
-	if (intr < 0)
+	if (intr < 0) {
+		pr_err("%s: Can't find %s GPIO interrupt\n", __func__,
+		       LIS331DL_ACCEL_INT1);
 		return ERR_PTR(intr);
-	if (intr2nd < 0)
-		ERR_PTR(intr2nd);
+	}
+
+	if (intr2nd < 0) {
+		pr_err("%s: Can't find %s GPIO interrupt\n", __func__,
+		       LIS331DL_ACCEL_INT2);
+		return ERR_PTR(intr2nd);
+	}
 
 	i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
 	intr2nd_pdata = intr2nd + INTEL_MID_IRQ_OFFSET;
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_max7315.c b/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
index 34dc59d..8989f81 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_max7315.c
@@ -29,8 +29,8 @@ static void __init *max7315_platform_data(void *info)
 	char intr_pin_name[SFI_NAME_LEN + 1];
 
 	if (nr == MAX7315_NUM) {
-		pr_err("too many max7315s, we only support %d\n",
-				MAX7315_NUM);
+		pr_err("%s: Too many instances, only %d supported\n", __func__,
+		       MAX7315_NUM);
 		return ERR_PTR(-ENOMEM);
 	}
 	/* we have several max7315 on the board, we only need load several
@@ -48,8 +48,12 @@ static void __init *max7315_platform_data(void *info)
 	gpio_base = get_gpio_by_name(base_pin_name);
 	intr = get_gpio_by_name(intr_pin_name);
 
-	if (gpio_base < 0)
+	if (gpio_base < 0) {
+		pr_warn("%s: falling back to dynamic gpio allocation\n",
+			__func__);
 		return NULL;
+	}
+
 	max7315->gpio_base = gpio_base;
 	if (intr != -1) {
 		i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c b/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
index f434f88..c79c87f 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
@@ -14,13 +14,18 @@
 #include <linux/i2c.h>
 #include <asm/intel-mid.h>
 
+#define MPU3050_INT		"mpu3050_int"
+
 static void *mpu3050_platform_data(void *info)
 {
 	struct i2c_board_info *i2c_info = info;
-	int intr = get_gpio_by_name("mpu3050_int");
+	int intr = get_gpio_by_name(MPU3050_INT);
 
-	if (intr < 0)
+	if (intr < 0) {
+		pr_err("%s: Can't find %s GPIO interrupt\n", __func__,
+		       MPU3050_INT);
 		return ERR_PTR(intr);
+	}
 
 	i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
 	return NULL;
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c b/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
index 563f77f..cde764e 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
@@ -41,8 +41,11 @@ static void __init *pcal9555a_platform_data(void *info)
 	intr = get_gpio_by_name(intr_pin_name);
 
 	/* Check if the SFI record valid */
-	if (gpio_base == -1)
+	if (gpio_base == -1) {
+		pr_warn("%s: falling back to dynamic gpio allocation\n",
+			__func__);
 		return NULL;
+	}
 
 	if (nr >= PCAL9555A_NUM) {
 		pr_err("%s: Too many instances, only %d supported\n", __func__,
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c b/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
index 4f41372..4d4393e 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
@@ -34,8 +34,12 @@ static void *tca6416_platform_data(void *info)
 	gpio_base = get_gpio_by_name(base_pin_name);
 	intr = get_gpio_by_name(intr_pin_name);
 
-	if (gpio_base < 0)
+	if (gpio_base < 0) {
+		pr_warn("%s: falling back to dynamic gpio allocation\n",
+			__func__);
 		return NULL;
+	}
+
 	tca6416.gpio_base = gpio_base;
 	if (intr >= 0) {
 		i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
diff --git a/arch/x86/platform/intel-mid/sfi.c b/arch/x86/platform/intel-mid/sfi.c
index 051d264..78ee7eb 100644
--- a/arch/x86/platform/intel-mid/sfi.c
+++ b/arch/x86/platform/intel-mid/sfi.c
@@ -335,9 +335,13 @@ static void __init sfi_handle_ipc_dev(struct sfi_device_table_entry *pentry,
 
 	pr_debug("IPC bus, name = %16.16s, irq = 0x%2x\n",
 		pentry->name, pentry->irq);
+
 	pdata = intel_mid_sfi_get_pdata(dev, pentry);
-	if (IS_ERR(pdata))
+	if (IS_ERR(pdata)) {
+		pr_debug("%s:  Can't get platform data for %s\n", __func__,
+			 pentry->name);
 		return;
+	}
 
 	pdev = platform_device_alloc(pentry->name, 0);
 	if (pdev == NULL) {
@@ -371,8 +375,11 @@ static void __init sfi_handle_spi_dev(struct sfi_device_table_entry *pentry,
 		spi_info.chip_select);
 
 	pdata = intel_mid_sfi_get_pdata(dev, &spi_info);
-	if (IS_ERR(pdata))
+	if (IS_ERR(pdata)) {
+		pr_debug("%s:  Can't get platform data for %s\n", __func__,
+			 pentry->name);
 		return;
+	}
 
 	spi_info.platform_data = pdata;
 	if (dev->delay)
@@ -398,8 +405,11 @@ static void __init sfi_handle_i2c_dev(struct sfi_device_table_entry *pentry,
 		i2c_info.addr);
 	pdata = intel_mid_sfi_get_pdata(dev, &i2c_info);
 	i2c_info.platform_data = pdata;
-	if (IS_ERR(pdata))
+	if (IS_ERR(pdata)) {
+		pr_debug("%s:  Can't get platform data for %s\n", __func__,
+			 pentry->name);
 		return;
+	}
 
 	if (dev->delay)
 		intel_scu_i2c_device_register(pentry->host_num, &i2c_info);
@@ -424,8 +434,11 @@ static void __init sfi_handle_sd_dev(struct sfi_device_table_entry *pentry,
 		 sd_info.max_clk,
 		 sd_info.addr);
 	pdata = intel_mid_sfi_get_pdata(dev, &sd_info);
-	if (IS_ERR(pdata))
+	if (IS_ERR(pdata)) {
+		pr_debug("%s:  Can't get platform data for %s\n", __func__,
+			 pentry->name);
 		return;
+	}
 
 	/* Nothing we can do with this for now */
 	sd_info.platform_data = pdata;
-- 
2.7.4

[PATCH v3 3/3] intel-mid: Move boundry check to the start of init code

[Kuppuswamy Sathyanarayanan]

Moved the instance boundary check to the start of the pcal9555a
platform init code. This will prevent unnecessary initialization
on instance boundary error.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c b/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
index cde764e..4e5dd95 100644
--- a/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
+++ b/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
@@ -34,6 +34,12 @@ static void __init *pcal9555a_platform_data(void *info)
 	char intr_pin_name[SFI_NAME_LEN + 1];
 	int gpio_base, intr;
 
+	if (nr >= PCAL9555A_NUM) {
+		pr_err("%s: Too many instances, only %d supported\n", __func__,
+		       PCAL9555A_NUM);
+		return ERR_PTR(-ENOMEM);
+	}
+
 	snprintf(base_pin_name, sizeof(base_pin_name), "%s_base", type);
 	snprintf(intr_pin_name, sizeof(intr_pin_name), "%s_int", type);
 
@@ -47,12 +53,6 @@ static void __init *pcal9555a_platform_data(void *info)
 		return NULL;
 	}
 
-	if (nr >= PCAL9555A_NUM) {
-		pr_err("%s: Too many instances, only %d supported\n", __func__,
-		       PCAL9555A_NUM);
-		return ERR_PTR(-ENOMEM);
-	}
-
 	pcal9555a = &pcal9555a_pdata[nr++];
 	pcal9555a->gpio_base = gpio_base;
 
-- 
2.7.4

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@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 2/3] intel-mid: Add valid error messages on init failure

[Andy Shevchenko]

On Thu, 2016-09-08 at 19:07 -0700, Kuppuswamy Sathyanarayanan wrote:
> Added valid error/warning messages to platform data
> initalization failures in SFI device libs code.

Looks good to me after addressing the following comments.

> 
> diff --git a/arch/x86/platform/intel-
> mid/device_libs/platform_emc1403.c b/arch/x86/platform/intel-
> mid/device_libs/platform_emc1403.c
> index c259fb6..bd776b04 100644
> --- a/arch/x86/platform/intel-mid/device_libs/platform_emc1403.c
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_emc1403.c
> @@ -15,17 +15,27 @@
>  #include <linux/i2c.h>
>  #include <asm/intel-mid.h>
>  
> +#define EMC1403_THERMAL_INT		"thermal_int"
> +#define EMC1403_THERMAL_ALERT_INT	"thermal_alert"

I would be cosistent, i.e.
EMC1403_INT1  "..."
EMC1403_INT2  "..."

> +
>  static void __init *emc1403_platform_data(void *info)
>  {
>  	static short intr2nd_pdata;
>  	struct i2c_board_info *i2c_info = info;
> -	int intr = get_gpio_by_name("thermal_int");
> -	int intr2nd = get_gpio_by_name("thermal_alert");
> +	int intr = get_gpio_by_name(EMC1403_THERMAL_INT);
> +	int intr2nd = get_gpio_by_name(EMC1403_THERMAL_ALERT_INT);
>  
> -	if (intr < 0)
> +	if (intr < 0) {
> +		pr_err("%s: Can't find %s GPIO interrupt\n",
> __func__,
> +		       EMC1403_THERMAL_INT);
>  		return NULL;

Souldn't we return an error here?

> -	if (intr2nd < 0)
> +	}
> +
> +	if (intr2nd < 0) {
> +		pr_err("%s: Can't find %s GPIO interrupt\n",
> __func__,
> +		       EMC1403_THERMAL_ALERT_INT);
>  		return NULL;

Ditto.

Would you check _all_ files under device libs?

> +	}
>  
>  	i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
>  	intr2nd_pdata = intr2nd + INTEL_MID_IRQ_OFFSET;
> diff --git a/arch/x86/platform/intel-mid/device_libs/platform_ipc.c
> b/arch/x86/platform/intel-mid/device_libs/platform_ipc.c
> index a84b73d..6704694 100644
> --- a/arch/x86/platform/intel-mid/device_libs/platform_ipc.c
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_ipc.c
> @@ -42,8 +42,11 @@ void __init ipc_device_handler(struct
> sfi_device_table_entry *pentry,
>  	 * On Medfield the platform device creation is handled by the
> MSIC
>  	 * MFD driver so we don't need to do it here.
>  	 */
> -	if (intel_mid_has_msic())
> +	if (intel_mid_has_msic()) {
> +		pr_err("%s: device %s will be handled by MSIC mfd
> driver\n",

Remove "mfd" word.

> +		       __func__, pentry->name);
>  		return;
> +	}
>  
>  	pdev = platform_device_alloc(pentry->name, 0);
>  	if (pdev == NULL) {
> diff --git a/arch/x86/platform/intel-mid/device_libs/platform_lis331.c 
> b/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
> index 8be5d40..393c23e 100644
> --- a/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_lis331.c
> @@ -14,17 +14,27 @@
>  #include <linux/gpio.h>
>  #include <asm/intel-mid.h>
>  
> +#define LIS331DL_ACCEL_INT1	"accel_int"
> +#define LIS331DL_ACCEL_INT2	"accel_2"

LIS331DL_INT1
LIS331DL_INT2

> @@ -48,8 +48,12 @@ static void __init *max7315_platform_data(void
> *info)
>  	gpio_base = get_gpio_by_name(base_pin_name);
>  	intr = get_gpio_by_name(intr_pin_name);
>  
> -	if (gpio_base < 0)
> +	if (gpio_base < 0) {
> +		pr_warn("%s: falling back to dynamic gpio
> allocation\n",
> +			__func__);
>  		return NULL;

Depends on my comment to the previous series.

> --- a/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
> @@ -14,13 +14,18 @@
>  #include <linux/i2c.h>
>  #include <asm/intel-mid.h>
>  
> +#define MPU3050_INT		"mpu3050_int"

MPU3050_INT1

> mid/device_libs/platform_pcal9555a.c b/arch/x86/platform/intel-
> mid/device_libs/platform_pcal9555a.c
> index 563f77f..cde764e 100644
> --- a/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
> @@ -41,8 +41,11 @@ static void __init *pcal9555a_platform_data(void
> *info)
>  	intr = get_gpio_by_name(intr_pin_name);
>  
>  	/* Check if the SFI record valid */
> -	if (gpio_base == -1)
> +	if (gpio_base == -1) {
> +		pr_warn("%s: falling back to dynamic gpio
> allocation\n",
> +			__func__);
>  		return NULL;

Same as above

> mid/device_libs/platform_tca6416.c b/arch/x86/platform/intel-
> mid/device_libs/platform_tca6416.c
> index 4f41372..4d4393e 100644
> --- a/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
> @@ -34,8 +34,12 @@ static void *tca6416_platform_data(void *info)
>  	gpio_base = get_gpio_by_name(base_pin_name);
>  	intr = get_gpio_by_name(intr_pin_name);
>  
> -	if (gpio_base < 0)
> +	if (gpio_base < 0) {
> +		pr_warn("%s: falling back to dynamic gpio
> allocation\n",
> +			__func__);
>  		return NULL;

Same as above

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 3/3] intel-mid: Move boundry check to the start of init code

[Andy Shevchenko]

On Thu, 2016-09-08 at 19:07 -0700, Kuppuswamy Sathyanarayanan wrote:
> Moved the instance boundary check to the start of the pcal9555a
> platform init code. This will prevent unnecessary initialization
> on instance boundary error.

I don't see how it makes any better.

I would drop this patch.

> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@
> linux.intel.com>
> ---
>  arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c | 12
> ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/platform/intel-
> mid/device_libs/platform_pcal9555a.c b/arch/x86/platform/intel-
> mid/device_libs/platform_pcal9555a.c
> index cde764e..4e5dd95 100644
> --- a/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
> @@ -34,6 +34,12 @@ static void __init *pcal9555a_platform_data(void
> *info)
>  	char intr_pin_name[SFI_NAME_LEN + 1];
>  	int gpio_base, intr;
>  
> +	if (nr >= PCAL9555A_NUM) {
> +		pr_err("%s: Too many instances, only %d supported\n",
> __func__,
> +		       PCAL9555A_NUM);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
>  	snprintf(base_pin_name, sizeof(base_pin_name), "%s_base",
> type);
>  	snprintf(intr_pin_name, sizeof(intr_pin_name), "%s_int",
> type);
>  
> @@ -47,12 +53,6 @@ static void __init *pcal9555a_platform_data(void
> *info)
>  		return NULL;
>  	}
>  
> -	if (nr >= PCAL9555A_NUM) {
> -		pr_err("%s: Too many instances, only %d supported\n",
> __func__,
> -		       PCAL9555A_NUM);
> -		return ERR_PTR(-ENOMEM);
> -	}
> -
>  	pcal9555a = &pcal9555a_pdata[nr++];
>  	pcal9555a->gpio_base = gpio_base;
>  

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy