x86/sfi: Enable enumeration of SD devices

Hello Andy Shevchenko,

The patch 05f310e26fe9: "x86/sfi: Enable enumeration of SD devices" from Jul 12, 2016, leads to the following static checker warning:

	arch/x86/platform/intel-mid/sfi.c:427 sfi_handle_sd_dev()
	warn: 'pdata' isn't an ERR_PTR

arch/x86/platform/intel-mid/sfi.c
   416          memset(&sd_info, 0, sizeof(sd_info));
   417          strncpy(sd_info.name, pentry->name, SFI_NAME_LEN); 418          sd_info.bus_num = pentry->host_num;
   419          sd_info.max_clk = pentry->max_freq;
   420          sd_info.addr = pentry->addr;
   421          pr_debug("SD bus = %d, name = %16.16s, max_clk = %d, addr = 0x%x\n",
   422                   sd_info.bus_num,
   423                   sd_info.name,
   424                   sd_info.max_clk,
   425                   sd_info.addr);
   426          pdata = intel_mid_sfi_get_pdata(dev, &sd_info); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This is a macro calling a function pointer.  None of the functions return error pointers.  Some return NULL on error but some return NULL on success.

   427          if (IS_ERR(pdata))
   428                  return;
   429
   430          /* Nothing we can do with this for now */ 431          sd_info.platform_data = pdata; 432

Quoted 26 lines · Dan Carpenter

Hello Andy Shevchenko,

The patch 05f310e26fe9: "x86/sfi: Enable enumeration of SD devices" from Jul 12, 2016, leads to the following static checker warning:

	arch/x86/platform/intel-mid/sfi.c:427 sfi_handle_sd_dev()
	warn: 'pdata' isn't an ERR_PTR

arch/x86/platform/intel-mid/sfi.c
   416          memset(&sd_info, 0, sizeof(sd_info));
   417          strncpy(sd_info.name, pentry->name, SFI_NAME_LEN); 418          sd_info.bus_num = pentry->host_num;
   419          sd_info.max_clk = pentry->max_freq;
   420          sd_info.addr = pentry->addr;
   421          pr_debug("SD bus = %d, name = %16.16s, max_clk = %d, addr = 0x%x\n",
   422                   sd_info.bus_num,
   423                   sd_info.name,
   424                   sd_info.max_clk,
   425                   sd_info.addr);
   426          pdata = intel_mid_sfi_get_pdata(dev, &sd_info); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This is a macro calling a function pointer.  None of the functions return error pointers.  Some return NULL on error but some return NULL on success.

Quoted 6 lines

   427          if (IS_ERR(pdata))
   428                  return;
   429
   430          /* Nothing we can do with this for now */ 431          sd_info.platform_data = pdata; 432

Thanks for catching up this. At some point in the future I will re-check all those so called "device lib" files to be aligned to one standard. Of course you may propose a patch if you feel you can do it.

Quoted 38 lines · Andy Shevchenko

Quoted 35 lines · Dan Carpenter

Hello Andy Shevchenko,

The patch 05f310e26fe9: "x86/sfi: Enable enumeration of SD devices"
from Jul 12, 2016, leads to the following static checker warning:

	arch/x86/platform/intel-mid/sfi.c:427 sfi_handle_sd_dev()
	warn: 'pdata' isn't an ERR_PTR

arch/x86/platform/intel-mid/sfi.c
   416          memset(&sd_info, 0, sizeof(sd_info));
   417          strncpy(sd_info.name, pentry->name, SFI_NAME_LEN); 418          sd_info.bus_num = pentry->host_num;
   419          sd_info.max_clk = pentry->max_freq;
   420          sd_info.addr = pentry->addr;
   421          pr_debug("SD bus = %d, name = %16.16s, max_clk > > > %d, addr = 0x%x\n",
   422                   sd_info.bus_num,
   423                   sd_info.name,
   424                   sd_info.max_clk,
   425                   sd_info.addr);
   426          pdata = intel_mid_sfi_get_pdata(dev, &sd_info); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This is a macro calling a function pointer.  None of the functions return error pointers.  Some return NULL on error but some return NULL
on success.

   427          if (IS_ERR(pdata))
   428                  return;
   429
   430          /* Nothing we can do with this for now */ 431          sd_info.platform_data = pdata; 432

Quoted 5 lines

Thanks for catching up this. At some point in the future I will re- check
all those so called "device lib" files to be aligned to one standard. Of
course you may propose a patch if you feel you can do it.

I'm a temporary haitus from work but what's the standard supposed to be?

Quoted 38 lines · Andy Shevchenko

Quoted 35 lines · Dan Carpenter

Hello Andy Shevchenko,

The patch 05f310e26fe9: "x86/sfi: Enable enumeration of SD devices"
from Jul 12, 2016, leads to the following static checker warning:

arch/x86/platform/intel-mid/sfi.c:427 sfi_handle_sd_dev()
warn: 'pdata' isn't an ERR_PTR

arch/x86/platform/intel-mid/sfi.c

   416          memset(&sd_info, 0, sizeof(sd_info));
   417          strncpy(sd_info.name, pentry->name, SFI_NAME_LEN);
   418          sd_info.bus_num = pentry->host_num;
   419          sd_info.max_clk = pentry->max_freq;
   420          sd_info.addr = pentry->addr;
   421          pr_debug("SD bus = %d, name = %16.16s, max_clk > > > %d,
addr = 0x%x\n",
   422                   sd_info.bus_num,
   423                   sd_info.name,
   424                   sd_info.max_clk,
   425                   sd_info.addr);
   426          pdata = intel_mid_sfi_get_pdata(dev, &sd_info);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This is a macro calling a function pointer. None of the functions return error pointers. Some return NULL on error but some return NULL
on success.

   427          if (IS_ERR(pdata))
   428                  return;
   429
   430          /* Nothing we can do with this for now */
   431          sd_info.platform_data = pdata;
   432

Quoted 5 lines

Thanks for catching up this. At some point in the future I will re- check
all those so called "device lib" files to be aligned to one standard. Of
course you may propose a patch if you feel you can do it.

I'm a temporary haitus from work but what's the standard supposed to be?

Hi Andy/Dan,

Thanks for catching this bug. As Andy mentioned, this code is written in this manner to let the get_platform_data() function pointer to return the error value on initialization failure. But it has never been used properly in any of the existing code. So my suggestion is either change the platform_lib code to return ERR_PTR on failure or change the intel_mid_sfi_get_pdata to check for NULL as well. Since all the use case of intel_mid_sfi_get_pdata are void functions, I would prefer to go with second solution. Please let me know your comments.

arch/x86/platform/intel-mid/sfi.c +0 -0

index 051d264..a6bd275 100644

arch/x86/platform/intel-mid/sfi.c +0 -0

                 return;

        pdev = platform_device_alloc(pentry->name, 0);

arch/x86/platform/intel-mid/sfi.c +0 -0

@@ -371,7 +371,7 @@ 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_OR_NULL(pdata))
                return;

        spi_info.platform_data = pdata;

arch/x86/platform/intel-mid/sfi.c +1 -1

@@ -398,7 +398,7 @@ 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_OR_NULL(pdata))
                 return;

        if (dev->delay)

arch/x86/platform/intel-mid/sfi.c +1 -1

@@ -424,7 +424,7 @@ 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_OR_NULL(pdata))
                 return;

        /* Nothing we can do with this for now */

Thanks and regards,
Sathyanarayanan KN

________________________________________
From: Andy Shevchenko [[email protected]]
Sent: Sunday, August 28, 2016 6:31 AM
To: Dan Carpenter
Cc: [email protected]; David Cohen; Kuppuswamy, Sathyanarayanan
Subject: Re: [bug report] x86/sfi: Enable enumeration of SD devices

+ David, Sathya

Quoted 53 lines · Dan Carpenter

Quoted 44 lines · Andy Shevchenko

Quoted 34 lines · Dan Carpenter

Hello Andy Shevchenko,

The patch 05f310e26fe9: "x86/sfi: Enable enumeration of SD devices"
from Jul 12, 2016, leads to the following static checker warning:

  arch/x86/platform/intel-mid/sfi.c:427 sfi_handle_sd_dev()
  warn: 'pdata' isn't an ERR_PTR

arch/x86/platform/intel-mid/sfi.c
   416          memset(&sd_info, 0, sizeof(sd_info));
   417          strncpy(sd_info.name, pentry->name, SFI_NAME_LEN);
   418          sd_info.bus_num = pentry->host_num;
   419          sd_info.max_clk = pentry->max_freq;
   420          sd_info.addr = pentry->addr;
   421          pr_debug("SD bus = %d, name = %16.16s, max_clk > > > > %d, addr = 0x%x\n",
   422                   sd_info.bus_num,
   423                   sd_info.name,
   424                   sd_info.max_clk,
   425                   sd_info.addr);
   426          pdata = intel_mid_sfi_get_pdata(dev, &sd_info); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This is a macro calling a function pointer.  None of the functions
return error pointers.  Some return NULL on error but some return
NULL
on success.

Quoted 6 lines

   427          if (IS_ERR(pdata))
   428                  return;
   429
   430          /* Nothing we can do with this for now */ 431          sd_info.platform_data = pdata; 432

Quoted 6 lines

Thanks for catching up this. At some point in the future I will re-
check
all those so called "device lib" files to be aligned to one standard. Of
course you may propose a patch if you feel you can do it.

Quoted 2 lines

I'm a temporary haitus from work but what's the standard supposed to be?

I've checked all upstreamed platform modules (arch/x86/platform/intel- mid/device_libs/) and noticed that not a single one returns ERR_PTR.

Though I think the idea was to provide a way to fail initialization in some cases where hardware must be initialized properly. Maybe David or Sathya can shed a light on this.

If we decide to change that it should be done for all so called device handlers in sfi.c.

--
Andy Shevchenko <[email protected]> Intel Finland Oy

Quoted 12 lines

Hi Andy/Dan,

Thanks for catching this bug. As Andy mentioned, this code is written in this manner to let the get_platform_data() function pointer to return the error value on initialization failure. But it has never been used properly in any of the existing code. So my suggestion is either change the platform_lib code to return ERR_PTR on failure or change the intel_mid_sfi_get_pdata to check for NULL as well. Since all the use case of intel_mid_sfi_get_pdata are void functions, I would prefer to go with second solution. Please let me know your comments.

arch/x86/platform/intel-mid/sfi.c static void __init sfi_handle_ipc_dev(struct · line 340

Quoted 12 lines

arch/x86/platform/intel-mid/sfi.c +1 -1

index 051d264..a6bd275 100644
@@ -336,7 +336,7 @@ 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_OR_NULL(pdata))

But this looks wrong.
pdata = NULL is valid case for many devices! In other words pdata is an optional argument to the device drivers.

Quoted 118 lines

arch/x86/platform/intel-mid/sfi.c +0 -0

                return;
 

        pdev = platform_device_alloc(pentry->name, 0);

arch/x86/platform/intel-mid/sfi.c +1 -1

@@ -371,7 +371,7 @@ 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_OR_NULL(pdata))
                return;
 

        spi_info.platform_data = pdata;

arch/x86/platform/intel-mid/sfi.c +1 -1

@@ -398,7 +398,7 @@ 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_OR_NULL(pdata))
                return;
 

        if (dev->delay)

arch/x86/platform/intel-mid/sfi.c +1 -1

@@ -424,7 +424,7 @@ 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_OR_NULL(pdata))
                return;
 

        /* Nothing we can do with this for now */

Thanks and regards,
Sathyanarayanan KN

________________________________________
From: Andy Shevchenko [[email protected]]
Sent: Sunday, August 28, 2016 6:31 AM
To: Dan Carpenter
Cc: [email protected]; David Cohen; Kuppuswamy, Sathyanarayanan
Subject: Re: [bug report] x86/sfi: Enable enumeration of SD devices

+ David, Sathya

Quoted 53 lines · Dan Carpenter

Quoted 44 lines · Andy Shevchenko

Quoted 34 lines · Dan Carpenter

Hello Andy Shevchenko,

The patch 05f310e26fe9: "x86/sfi: Enable enumeration of SD devices"
from Jul 12, 2016, leads to the following static checker warning:

arch/x86/platform/intel-mid/sfi.c:427 sfi_handle_sd_dev()
warn: 'pdata' isn't an ERR_PTR

arch/x86/platform/intel-mid/sfi.c

   416          memset(&sd_info, 0, sizeof(sd_info));
   417          strncpy(sd_info.name, pentry->name,
SFI_NAME_LEN);
   418          sd_info.bus_num = pentry->host_num;
   419          sd_info.max_clk = pentry->max_freq;
   420          sd_info.addr = pentry->addr;
   421          pr_debug("SD bus = %d, name = %16.16s, max_clk >>>>> %d,
addr = 0x%x\n",
   422                   sd_info.bus_num,
   423                   sd_info.name,
   424                   sd_info.max_clk,
   425                   sd_info.addr);
   426          pdata = intel_mid_sfi_get_pdata(dev, &sd_info);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This is a macro calling a function pointer. None of the functions
return error pointers. Some return NULL on error but some return
NULL
on success.

Quoted 6 lines

   427          if (IS_ERR(pdata))
   428                  return;
   429
   430          /* Nothing we can do with this for now */
   431          sd_info.platform_data = pdata;
   432

Quoted 6 lines

Thanks for catching up this. At some point in the future I will re-
check
all those so called "device lib" files to be aligned to one standard. Of
course you may propose a patch if you feel you can do it.

Quoted 2 lines

I'm a temporary haitus from work but what's the standard supposed to be?

I've checked all upstreamed platform modules (arch/x86/platform/intel- mid/device_libs/) and noticed that not a single one returns ERR_PTR.

Though I think the idea was to provide a way to fail initialization in some cases where hardware must be initialized properly. Maybe David or Sathya can shed a light on this.

If we decide to change that it should be done for all so called device handlers in sfi.c.

--
Andy Shevchenko <[email protected]> Intel Finland Oy

Am 30.08.2016 11:46, schrieb Andy Shevchenko:

Quoted 29 lines

Quoted 26 lines · Sathyanarayanan

Hi Andy/Dan,

Thanks for catching this bug. As Andy mentioned, this code is written
in this manner to let the get_platform_data() function pointer to return the error value on initialization failure. But it has never been used properly in any of the existing code. So my suggestion is
either change the platform_lib code to return ERR_PTR on failure or
change the intel_mid_sfi_get_pdata to check for NULL as well. Since
all the use case of intel_mid_sfi_get_pdata are void functions, I would prefer to go with second solution. Please let me know your comments.

arch/x86/platform/intel-mid/sfi.c +0 -0

index 051d264..a6bd275 100644
@@ -336,7 +336,7 @@ 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);

Quoted 9 lines

arch/x86/platform/intel-mid/sfi.c static void __init sfi_handle_ipc_dev(struct · line 340

Quoted 3 lines

arch/x86/platform/intel-mid/sfi.c +1 -1

         pdata = intel_mid_sfi_get_pdata(dev, pentry);
-       if (IS_ERR(pdata))
+       if (IS_ERR_OR_NULL(pdata))

But this looks wrong.
pdata = NULL is valid case for many devices! In other words pdata is an
optional argument to the device drivers.

Yep, the way is wrong.
NULL can say: get_platform_data does not exists
or get_platform_data() returned NULL (what ever that means).

IMHO it feels better to drop the define and replace it with a proper function call and error.

#define intel_mid_sfi_get_pdata(dev, priv)      \
         ((dev)->get_platform_data ? (dev)->get_platform_data(priv) :
NULL)

void *fkt(struct devs_id *dev, void *info)
{
     if ( ! dev->get_platform_data)

	  return ERR_PTR(-ENOSYS);

	return dev->get_platform_data(info);
}

Quoted 19 lines

Quoted 18 lines

I've checked all upstreamed platform modules (arch/x86/platform/intel-
mid/device_libs/) and noticed that not a single one returns ERR_PTR.

Though I think the idea was to provide a way to fail initialization in
some cases where hardware must be initialized properly. Maybe David or
Sathya can shed a light on this.

If we decide to change that it should be done for all so called device
handlers in sfi.c.

--
Andy Shevchenko <[email protected]> Intel Finland Oy

Quoted 36 lines · walter harms

Am 30.08.2016 11:46, schrieb Andy Shevchenko:

Quoted 35 lines

Quoted 22 lines · Sathyanarayanan

Hi Andy/Dan,

Thanks for catching this bug. As Andy mentioned, this code is written
in this manner to let the get_platform_data() function pointer to return the error value on initialization failure. But it has never been used properly in any of the existing code. So my suggestion is
either change the platform_lib code to return ERR_PTR on failure or
change the intel_mid_sfi_get_pdata to check for NULL as well. Since
all the use case of intel_mid_sfi_get_pdata are void functions, I would prefer to go with second solution. Please let me know your comments.

arch/x86/platform/intel-mid/sfi.c +0 -0

index 051d264..a6bd275 100644

arch/x86/platform/intel-mid/sfi.c static void __init sfi_handle_ipc_dev(struct · line 340

Quoted 6 lines

arch/x86/platform/intel-mid/sfi.c +1 -1

@@ -336,7 +336,7 @@ 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_OR_NULL(pdata))

But this looks wrong.
pdata = NULL is valid case for many devices! In other words pdata is an
optional argument to the device drivers.

Quoted 6 lines

Yep, the way is wrong.
NULL can say: get_platform_data does not exists
or get_platform_data() returned NULL (what ever that means).

IMHO it feels better to drop the define and replace it with a proper function call and error.

Yeah, this direction looks better, thanks.

Quoted 32 lines

#define intel_mid_sfi_get_pdata(dev, priv)      \
          ((dev)->get_platform_data ? (dev)->get_platform_data(priv) :

NULL)

void *fkt(struct devs_id *dev, void *info)

{
      if ( ! dev->get_platform_data)
	  return ERR_PTR(-ENOSYS);

	return dev->get_platform_data(info);
}

Quoted 18 lines

Quoted 18 lines

I've checked all upstreamed platform modules (arch/x86/platform/intel-
mid/device_libs/) and noticed that not a single one returns ERR_PTR.

Though I think the idea was to provide a way to fail initialization in
some cases where hardware must be initialized properly. Maybe David or
Sathya can shed a light on this.

If we decide to change that it should be done for all so called device
handlers in sfi.c.

--
Andy Shevchenko <[email protected]> Intel Finland Oy

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(-)

arch/x86/platform/intel-mid/device_libs/platform_lis331.c +0 -0

index a35cf91..2fd200b 100644
@@ -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");

arch/x86/platform/intel-mid/device_libs/platform_lis331.c +2 -0

+		return ERR_PTR(intr2nd);
+	}

  i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET; intr2nd_pdata = intr2nd + INTEL_MID_IRQ_OFFSET;

(quoted diff) +0 -0

mid/device_libs/platform_max7315.c b/arch/x86/platform/intel- mid/device_libs/platform_max7315.c
index 6e075af..cc20dfc 100644

arch/x86/platform/intel-mid/device_libs/platform_max7315.c +0 -0

  	}
  	/* we have several max7315 on the board, we only need load

several
   * instances of the same pca953x driver to cover them

arch/x86/platform/intel-mid/device_libs/platform_max7315.c +0 -0

@@ -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);

arch/x86/platform/intel-mid/device_libs/platform_max7315.c +2 -0

+	}
+

  max7315->gpio_base = gpio_base;
  if (intr != -1) {
  i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;

(quoted diff) +0 -0

mid/device_libs/platform_mpu3050.c b/arch/x86/platform/intel- mid/device_libs/platform_mpu3050.c
index ee22864..2008824 100644

arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c +0 -0

@@ -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");

arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c +2 -0

+		return ERR_PTR(intr);
+	}

  i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET; +
  return NULL;
 }

(quoted diff) +0 -0

mid/device_libs/platform_pcal9555a.c b/arch/x86/platform/intel- mid/device_libs/platform_pcal9555a.c
index 429a941..97e92a2 100644

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +0 -0

@@ -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 */

  }

  pcal9555a = &pcal9555a_pdata[nr++];

(quoted diff) +0 -0

mid/device_libs/platform_tca6416.c b/arch/x86/platform/intel- mid/device_libs/platform_tca6416.c
index 4f41372..2796956 100644

arch/x86/platform/intel-mid/device_libs/platform_tca6416.c +0 -0

@@ -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);

arch/x86/platform/intel-mid/device_libs/platform_tca6416.c +2 -0

+	}
+

  tca6416.gpio_base = gpio_base;
  if (intr >= 0) {
  i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;

arch/x86/platform/intel-mid/sfi.c +0 -0

index 051d264..8e7361f 100644
@@ -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);

arch/x86/platform/intel-mid/sfi.c +2 -1

-	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) {

arch/x86/platform/intel-mid/sfi.c +0 -0

@@ -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);

arch/x86/platform/intel-mid/sfi.c +2 -1

-	if (IS_ERR(pdata))
+	if (IS_ERR(pdata)) {
 
 
+		pr_err("spi_device: %s: invalid platform data\n",

pentry->name);

  return;

arch/x86/platform/intel-mid/sfi.c +1 -0

+	}

  spi_info.platform_data = pdata;
  if (dev->delay)

arch/x86/platform/intel-mid/sfi.c +1 -0

+	}

  if (dev->delay) intel_scu_i2c_device_register(pentry->host_num, &i2c_info);

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

Quoted 6 lines · 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.

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

See also comments below.

Quoted 21 lines

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(-)

arch/x86/platform/intel-mid/device_libs/platform_lis331.c +0 -0

index a35cf91..2fd200b 100644

arch/x86/platform/intel-mid/device_libs/platform_lis331.c static void __init *lis331dl_platform_data(void · line 26

Quoted 10 lines

arch/x86/platform/intel-mid/device_libs/platform_lis331.c +2 -4

@@ -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);

arch/x86/platform/intel-mid/device_libs/platform_lis331.c static void __init *lis331dl_platform_data(void *info) · line 30

Quoted 5 lines

arch/x86/platform/intel-mid/device_libs/platform_lis331.c +5 -0

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

Ditto.

Quoted 13 lines

arch/x86/platform/intel-mid/device_libs/platform_lis331.c +2 -0

+		return ERR_PTR(intr2nd);
+	}
 
  	i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;

intr2nd_pdata = intr2nd + INTEL_MID_IRQ_OFFSET;

(quoted diff) +0 -0

mid/device_libs/platform_max7315.c b/arch/x86/platform/intel- mid/device_libs/platform_max7315.c
index 6e075af..cc20dfc 100644

arch/x86/platform/intel-mid/device_libs/platform_max7315.c static void __init *max7315_platform_data(void · line 34

Quoted 4 lines

arch/x86/platform/intel-mid/device_libs/platform_max7315.c +0 -0

@@ -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

arch/x86/platform/intel-mid/device_libs/platform_max7315.c static void __init *max7315_platform_data(void · line 35

Quoted 2 lines

arch/x86/platform/intel-mid/device_libs/platform_max7315.c +1 -1

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

-ENOMEM

Quoted 6 lines

arch/x86/platform/intel-mid/device_libs/platform_max7315.c +0 -0

  	}
  	/* we have several max7315 on the board, we only need load

several
* instances of the same pca953x driver to cover them

arch/x86/platform/intel-mid/device_libs/platform_max7315.c static void __init *max7315_platform_data(void · line 54

Quoted 9 lines

arch/x86/platform/intel-mid/device_libs/platform_max7315.c +3 -2

@@ -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)

Quoted 16 lines

arch/x86/platform/intel-mid/device_libs/platform_max7315.c +2 -0

+	}
+
  	max7315->gpio_base = gpio_base;
  	if (intr != -1) {

i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;

(quoted diff) +0 -0

mid/device_libs/platform_mpu3050.c b/arch/x86/platform/intel- mid/device_libs/platform_mpu3050.c
index ee22864..2008824 100644

arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c +0 -0

@@ -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");
 

arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c static void *mpu3050_platform_data(void *info) · line 23

Quoted 4 lines

arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c +2 -2

-	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);

Quoted 19 lines

arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c +3 -0

+		return ERR_PTR(intr);
+	}
 
  	i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
+
  	return NULL;
  }
 

(quoted diff) +0 -0

mid/device_libs/platform_pcal9555a.c b/arch/x86/platform/intel- mid/device_libs/platform_pcal9555a.c
index 429a941..97e92a2 100644

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +0 -0

@@ -41,13 +41,15 @@ static void __init *pcal9555a_platform_data(void
 *info)
  	intr = get_gpio_by_name(intr_pin_name);
 

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c static void __init *pcal9555a_platform_data(void *info) · line 46

Quoted 6 lines

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +3 -2

  	/* 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.

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c static void __init *pcal9555a_platform_data(void *info) · line 52

Quoted 8 lines

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +2 -1

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

-ENOMEM

Quoted 14 lines

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +0 -0

  	}

pcal9555a = &pcal9555a_pdata[nr++];

(quoted diff) +0 -0

mid/device_libs/platform_tca6416.c b/arch/x86/platform/intel- mid/device_libs/platform_tca6416.c
index 4f41372..2796956 100644

arch/x86/platform/intel-mid/device_libs/platform_tca6416.c +0 -0

@@ -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);
 

arch/x86/platform/intel-mid/device_libs/platform_tca6416.c static void *tca6416_platform_data(void *info) · line 39

Quoted 5 lines

arch/x86/platform/intel-mid/device_libs/platform_tca6416.c +3 -2

-	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.

Quoted 14 lines

arch/x86/platform/intel-mid/device_libs/platform_tca6416.c +2 -0

+	}
+
  	tca6416.gpio_base = gpio_base;
  	if (intr >= 0) {
  		i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;

arch/x86/platform/intel-mid/sfi.c +0 -0

index 051d264..8e7361f 100644
@@ -335,9 +335,12 @@ static void __init sfi_handle_ipc_dev(struct
 sfi_device_table_entry *pentry,
 

arch/x86/platform/intel-mid/sfi.c static void __init sfi_handle_ipc_dev(struct sfi_device_table_entry *pentry, · line 343

Quoted 10 lines

arch/x86/platform/intel-mid/sfi.c +4 -1

  	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.

Quoted 12 lines

arch/x86/platform/intel-mid/sfi.c +0 -0

  	pdev = platform_device_alloc(pentry->name, 0);

  	if (pdev = NULL) {

arch/x86/platform/intel-mid/sfi.c +1 -1

@@ -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)) {
 

arch/x86/platform/intel-mid/sfi.c static void __init sfi_handle_spi_dev(struct · line 381

Quoted 2 lines

arch/x86/platform/intel-mid/sfi.c +1 -0

+		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);

Quoted 4 lines

arch/x86/platform/intel-mid/sfi.c +1 -0

  		return;
+	}

arch/x86/platform/intel-mid/sfi.c static void __init sfi_handle_i2c_dev(struct · line 410

Quoted 12 lines

  	spi_info.platform_data = pdata;
  	if (dev->delay)

arch/x86/platform/intel-mid/sfi.c +2 -1

@@ -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.

Quoted 7 lines

arch/x86/platform/intel-mid/sfi.c +1 -0

+	}
 

if (dev->delay)
intel_scu_i2c_device_register(pentry->host_num,
&i2c_info);

arch/x86/platform/intel-mid/sfi.c static void __init sfi_handle_sd_dev(struct · line 439

Quoted 10 lines

arch/x86/platform/intel-mid/sfi.c +3 -1

@@ -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.

Quoted 4 lines

arch/x86/platform/intel-mid/sfi.c +0 -0

  	/* Nothing we can do with this for now */

  	sd_info.platform_data = pdata;

--- a/arch/x86/platform/intel-mid/sfi.c

Quoted 8 lines

Quoted 8 lines

+++ b/arch/x86/platform/intel-mid/sfi.c

(quoted diff) +0 -0

@@ -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);

Quoted 8 lines

Review

Quoted 6 lines

arch/x86/platform/intel-mid/sfi.c +2 -1

-	if (IS_ERR(pdata))
+	if (IS_ERR(pdata)) {
+		pr_err("ipc_device: %s: invalid platform data\n",

pentry->name);
   return;

(quoted diff) +1 -0

+	}

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.

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.

index ee22864..4b33aab 100644

arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c +0 -0

@@ -14,15 +14,21 @@

  i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET; +
  return NULL;

 }

(quoted diff) +0 -0

mid/device_libs/platform_pcal9555a.c b/arch/x86/platform/intel- mid/device_libs/platform_pcal9555a.c
index 429a941..190b2d2 100644

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +0 -0

@@ -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 */

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +2 -1

-	if (gpio_base = -1)
+	if (gpio_base = -1) {
+		pr_err("%s: Unknown GPIO base number, falling back to

dynamic"

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +1 -0

+		       "allocation\n", __func__);

  return NULL;

Quoted 1 line · Kuppuswamy Sathyanarayanan

According to the intel_mid_sfi_get_pdata() function definition,

"function" is implied, remove the word.

Quoted 2 lines

get_platform_data() function

Ditto.

Quoted 2 lines

should returns NULL on no platform

returns -> return

Quoted 5 lines

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

arch/x86/platform/intel-mid/device_libs/platform_max7315.c static void __init *max7315_platform_data(void · line 34

Quoted 13 lines

arch/x86/platform/intel-mid/device_libs/platform_max7315.c +2 -3

@@ -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",

arch/x86/platform/intel-mid/device_libs/platform_max7315.c static void __init *max7315_platform_data(void *info) · line 53

Quoted 11 lines

arch/x86/platform/intel-mid/device_libs/platform_max7315.c +2 -1

@@ -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"

arch/x86/platform/intel-mid/device_libs/platform_max7315.c +1 -0

+		       "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.

arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c line 17

Quoted 9 lines

index ee22864..4b33aab 100644

arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c +1 -0

@@ -14,15 +14,21 @@
 
  	i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
+
  	return NULL;

This change doesn't belong to the series.

Quoted 13 lines

arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c +0 -0

  }
 

(quoted diff) +0 -0

mid/device_libs/platform_pcal9555a.c b/arch/x86/platform/intel- mid/device_libs/platform_pcal9555a.c
index 429a941..190b2d2 100644

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +0 -0

@@ -41,13 +41,16 @@ static void __init *pcal9555a_platform_data(void
 *info)
  	intr = get_gpio_by_name(intr_pin_name);
 

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c

Quoted 7 lines

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +2 -1

  	/* 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"

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +1 -0

+		       "allocation\n", __func__);
  		return NULL;

Same comment as above for gpio_base.

arch/x86/platform/intel-mid/device_libs/platform_tca6416.c

Quoted 12 lines

arch/x86/platform/intel-mid/device_libs/platform_tca6416.c +2 -1

@@ -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"

arch/x86/platform/intel-mid/device_libs/platform_tca6416.c +1 -0

+		       "allocation\n", __func__);

Ditto.

1. Rewrite GPIO expander logic to cover dynamic allocation. You have

Quoted 2 lines

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         }

Quoted 11 lines

arch/x86/platform/intel-mid/device_libs/platform_max7315.c static void __init *max7315_platform_data(void *info) · line 53

Quoted 6 lines

arch/x86/platform/intel-mid/device_libs/platform_max7315.c +2 -1

-	if (gpio_base < 0)
+	if (gpio_base < 0) {
+		pr_err("%s: Unknown GPIO base number, falling

back
to"

arch/x86/platform/intel-mid/device_libs/platform_max7315.c +1 -0

+		       "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__);

--- a/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c

Quoted 7 lines

Quoted 6 lines

+++ b/arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c

(quoted diff) +0 -0

@@ -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 ?

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

+
 static void __init *emc1403_platform_data(void *info) {
  static short intr2nd_pdata;
  struct i2c_board_info *i2c_info = info;

arch/x86/platform/intel-mid/device_libs/platform_emc1403.c +2 -2

-	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);

arch/x86/platform/intel-mid/device_libs/platform_emc1403.c +2 -1

-	if (intr < 0)
+	if (intr < 0) {
+		pr_err("%s: Can't find %s GPIO interrupt\n",

__func__,

arch/x86/platform/intel-mid/device_libs/platform_emc1403.c +1 -0

+		       EMC1403_THERMAL_INT);

  return NULL;

arch/x86/platform/intel-mid/device_libs/platform_emc1403.c +4 -1

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

__func__,

arch/x86/platform/intel-mid/device_libs/platform_emc1403.c +1 -0

+		       EMC1403_THERMAL_ALERT_INT);

  return NULL;

arch/x86/platform/intel-mid/device_libs/platform_emc1403.c +1 -0

+	}

  i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET; intr2nd_pdata = intr2nd + INTEL_MID_IRQ_OFFSET;

arch/x86/platform/intel-mid/device_libs/platform_ipc.c +0 -0

index a84b73d..6704694 100644
@@ -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. */

arch/x86/platform/intel-mid/device_libs/platform_ipc.c +2 -1

-	if (intel_mid_has_msic())
+	if (intel_mid_has_msic()) {
+		pr_err("%s: device %s will be handled by MSIC mfd

driver\n",

arch/x86/platform/intel-mid/device_libs/platform_ipc.c +1 -0

+		       __func__, pentry->name);

  return;

arch/x86/platform/intel-mid/device_libs/platform_ipc.c +1 -0

+	}

  pdev = platform_device_alloc(pentry->name, 0); if (pdev = NULL) {

arch/x86/platform/intel-mid/device_libs/platform_lis331.c +0 -0

index 8be5d40..393c23e 100644
@@ -14,17 +14,27 @@
  #include <linux/gpio.h>
  #include <asm/intel-mid.h>

arch/x86/platform/intel-mid/device_libs/platform_lis331.c +0 -0

@@ -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);

arch/x86/platform/intel-mid/device_libs/platform_lis331.c +2 -1

-	if (gpio_base < 0)
+	if (gpio_base < 0) {
+		pr_warn("%s: falling back to dynamic gpio

allocation\n",

arch/x86/platform/intel-mid/device_libs/platform_lis331.c +1 -0

+			__func__);

  return NULL;

arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c +0 -0

@@ -14,13 +14,18 @@
  #include <linux/i2c.h>
  #include <asm/intel-mid.h>

+#define MPU3050_INT "mpu3050_int"

mid/device_libs/platform_pcal9555a.c b/arch/x86/platform/intel- mid/device_libs/platform_pcal9555a.c
index 563f77f..cde764e 100644

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +0 -0

@@ -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 */

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +2 -1

-	if (gpio_base = -1)
+	if (gpio_base = -1) {
+		pr_warn("%s: falling back to dynamic gpio

allocation\n",

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +1 -0

+			__func__);

  return NULL;

mid/device_libs/platform_tca6416.c b/arch/x86/platform/intel- mid/device_libs/platform_tca6416.c
index 4f41372..4d4393e 100644

arch/x86/platform/intel-mid/device_libs/platform_tca6416.c +0 -0

@@ -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);

arch/x86/platform/intel-mid/device_libs/platform_tca6416.c +2 -1

-	if (gpio_base < 0)
+	if (gpio_base < 0) {
+		pr_warn("%s: falling back to dynamic gpio

allocation\n",

arch/x86/platform/intel-mid/device_libs/platform_tca6416.c +1 -0

+			__func__);

  return NULL;

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(-)

(quoted diff) +0 -0

mid/device_libs/platform_pcal9555a.c b/arch/x86/platform/intel- mid/device_libs/platform_pcal9555a.c
index cde764e..4e5dd95 100644

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +0 -0

@@ -34,6 +34,12 @@ static void __init *pcal9555a_platform_data(void
 *info)
  	char intr_pin_name[SFI_NAME_LEN + 1];
  	int gpio_base, intr;

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +2 -0

+	if (nr >= PCAL9555A_NUM) {
+		pr_err("%s: Too many instances, only %d supported\n",

__func__,

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +4 -0

+		       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);

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +0 -0

@@ -47,12 +53,6 @@ static void __init *pcal9555a_platform_data(void
 *info)
  		return NULL;
  	}

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +0 -2

-	if (nr >= PCAL9555A_NUM) {
-		pr_err("%s: Too many instances, only %d supported\n",

__func__,

arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c +0 -4

-		       PCAL9555A_NUM);
-		return ERR_PTR(-ENOMEM);
-	}
-

  pcal9555a = &pcal9555a_pdata[nr++];
  pcal9555a->gpio_base = gpio_base;

IMO, Main problem here is, In some scenarios get_platform_data returns
NULL on error case and in some cases it return NULL for no platform data
case.

For example, in following function, returning NULL on gpio_base = -1 and when nr >= PCAL9555A_NUM is incorrect. I think in both these scenarios valid behavior is to stop the device creation process.

Creating an i2c device without validi irq number or gpio number is incorrect.

So after investigating it bit more, I think its more appropriate to fix
these error cases in device_libs dir. Let me know your comments.

Quoted 5 lines · Sathyanarayanan Kuppuswamy

IMO, Main problem here is, In some scenarios get_platform_data returns
NULL on error case and in some cases it return NULL for no platform data
case.

We have possibility of the following scenarios:
1) fatal error (should return error code and fail booting)
2) non-fatal error (prevents certain device to be enumerated, pdata = NULL)
3) no pdata for the device (not an error! pdata is optional to the certain driver)
4) pdata != NULL (fully armed device driver)

According to above I doubt we will have many 1) cases. Otherwise pdata = NULL is okay.

Quoted 4 lines

For example, in following function, returning NULL on gpio_base == -1 and when nr >= PCAL9555A_NUM is incorrect. I think in both these scenarios valid behavior is to stop the device creation process.

Yes, but do _not_ stop processing and booting!

Quoted 6 lines

Creating an i2c device without validi irq number or gpio number is incorrect.

So after investigating it bit more, I think its more appropriate to fix
these error cases in device_libs dir. Let me know your comments.

I doubt that is a proper fix. It might be part of the fix in sfi.c where you somehow distinguish fatal errors from non-fatal.

Quoted 13 lines · Andy Shevchenko

On Tue, 2016-08-30 at 11:18 -0700, Sathyanarayanan Kuppuswamy wrote:
IMO, Main problem here is, In some scenarios get_platform_data

Quoted 2 lines

returns NULL on error case and in some cases it return NULL for no platform data case.

We have possibility of the following scenarios:
1) fatal error (should return error code and fail booting)
2) non-fatal error (prevents certain device to be enumerated, pdata = NULL)
3) no pdata for the device (not an error! pdata is optional to the certain driver)
4) pdata != NULL (fully armed device driver)

Quoted 2 lines

According to above I doubt we will have many 1) cases. Otherwise pdata = NULL is okay.

I agree with 2,3 and 4 scenarios. But I am not sure about the first case. Since these are peripheral devices, any failure in them should not stop the device boot. Do you have any examples for this case ?

Attached patch fixes the return value issues in get_platform_data code
in device_libs directory. Please check and let me know your comments.