* Re: [bug report] x86/sfi: Enable enumeration of SD devices
2016-07-15 19:23 [bug report] x86/sfi: Enable enumeration of SD devices Dan Carpenter
@ 2016-08-09 15:32 ` Andy Shevchenko
2016-08-09 17:58 ` Dan Carpenter
` (9 subsequent siblings)
10 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-08-09 15:32 UTC (permalink / raw)
To: kernel-janitors
On Fri, 2016-07-15 at 22:23 +0300, Dan Carpenter wrote:
> 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
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.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [bug report] x86/sfi: Enable enumeration of SD devices
2016-07-15 19:23 [bug report] x86/sfi: Enable enumeration of SD devices Dan Carpenter
2016-08-09 15:32 ` Andy Shevchenko
@ 2016-08-09 17:58 ` Dan Carpenter
2016-08-28 13:31 ` Andy Shevchenko
` (8 subsequent siblings)
10 siblings, 0 replies; 38+ messages in thread
From: Dan Carpenter @ 2016-08-09 17:58 UTC (permalink / raw)
To: kernel-janitors
On Tue, Aug 09, 2016 at 06:32:55PM +0300, Andy Shevchenko wrote:
> On Fri, 2016-07-15 at 22:23 +0300, Dan Carpenter wrote:
> > 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
>
> 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?
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [bug report] x86/sfi: Enable enumeration of SD devices
2016-07-15 19:23 [bug report] x86/sfi: Enable enumeration of SD devices Dan Carpenter
2016-08-09 15:32 ` Andy Shevchenko
2016-08-09 17:58 ` Dan Carpenter
@ 2016-08-28 13:31 ` Andy Shevchenko
2016-08-29 20:59 ` Kuppuswamy, Sathyanarayanan
` (7 subsequent siblings)
10 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-08-28 13:31 UTC (permalink / raw)
To: kernel-janitors
+ David, Sathya
On Tue, 2016-08-09 at 20:58 +0300, Dan Carpenter wrote:
> On Tue, Aug 09, 2016 at 06:32:55PM +0300, Andy Shevchenko wrote:
> >
> > On Fri, 2016-07-15 at 22:23 +0300, Dan Carpenter wrote:
> > >
> > > 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
> >
> > 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?
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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 38+ messages in thread* RE: [bug report] x86/sfi: Enable enumeration of SD devices
2016-07-15 19:23 [bug report] x86/sfi: Enable enumeration of SD devices Dan Carpenter
` (2 preceding siblings ...)
2016-08-28 13:31 ` Andy Shevchenko
@ 2016-08-29 20:59 ` Kuppuswamy, Sathyanarayanan
2016-08-30 9:46 ` Andy Shevchenko
` (6 subsequent siblings)
10 siblings, 0 replies; 38+ messages in thread
From: Kuppuswamy, Sathyanarayanan @ 2016-08-29 20:59 UTC (permalink / raw)
To: kernel-janitors
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.
diff --git a/arch/x86/platform/intel-mid/sfi.c b/arch/x86/platform/intel-mid/sfi.c
index 051d264..a6bd275 100644
--- a/arch/x86/platform/intel-mid/sfi.c
+++ b/arch/x86/platform/intel-mid/sfi.c
@@ -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))
return;
pdev = platform_device_alloc(pentry->name, 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;
@@ -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)
@@ -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 [andriy.shevchenko@linux.intel.com]
Sent: Sunday, August 28, 2016 6:31 AM
To: Dan Carpenter
Cc: kernel-janitors@vger.kernel.org; David Cohen; Kuppuswamy, Sathyanarayanan
Subject: Re: [bug report] x86/sfi: Enable enumeration of SD devices
+ David, Sathya
On Tue, 2016-08-09 at 20:58 +0300, Dan Carpenter wrote:
> On Tue, Aug 09, 2016 at 06:32:55PM +0300, Andy Shevchenko wrote:
> >
> > On Fri, 2016-07-15 at 22:23 +0300, Dan Carpenter wrote:
> > >
> > > 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
> >
> > 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?
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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [bug report] x86/sfi: Enable enumeration of SD devices
2016-07-15 19:23 [bug report] x86/sfi: Enable enumeration of SD devices Dan Carpenter
` (3 preceding siblings ...)
2016-08-29 20:59 ` Kuppuswamy, Sathyanarayanan
@ 2016-08-30 9:46 ` Andy Shevchenko
2016-08-30 11:06 ` walter harms
` (5 subsequent siblings)
10 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-08-30 9:46 UTC (permalink / raw)
To: kernel-janitors
On Mon, 2016-08-29 at 20:59 +0000, Kuppuswamy, Sathyanarayanan wrote:
> 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.
>
> diff --git a/arch/x86/platform/intel-mid/sfi.c
> b/arch/x86/platform/intel-mid/sfi.c
> index 051d264..a6bd275 100644
> --- a/arch/x86/platform/intel-mid/sfi.c
> +++ b/arch/x86/platform/intel-mid/sfi.c
> @@ -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.
> return;
>
> pdev = platform_device_alloc(pentry->name, 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;
> @@ -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)
> @@ -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 [andriy.shevchenko@linux.intel.com]
> Sent: Sunday, August 28, 2016 6:31 AM
> To: Dan Carpenter
> Cc: kernel-janitors@vger.kernel.org; David Cohen; Kuppuswamy,
> Sathyanarayanan
> Subject: Re: [bug report] x86/sfi: Enable enumeration of SD devices
>
> + David, Sathya
>
> On Tue, 2016-08-09 at 20:58 +0300, Dan Carpenter wrote:
> >
> > On Tue, Aug 09, 2016 at 06:32:55PM +0300, Andy Shevchenko wrote:
> > >
> > >
> > > On Fri, 2016-07-15 at 22:23 +0300, Dan Carpenter wrote:
> > > >
> > > >
> > > > 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
> > >
> > > 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?
>
> 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 <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [bug report] x86/sfi: Enable enumeration of SD devices
2016-07-15 19:23 [bug report] x86/sfi: Enable enumeration of SD devices Dan Carpenter
` (4 preceding siblings ...)
2016-08-30 9:46 ` Andy Shevchenko
@ 2016-08-30 11:06 ` walter harms
2016-08-30 11:13 ` Andy Shevchenko
` (4 subsequent siblings)
10 siblings, 0 replies; 38+ messages in thread
From: walter harms @ 2016-08-30 11:06 UTC (permalink / raw)
To: kernel-janitors
Am 30.08.2016 11:46, schrieb Andy Shevchenko:
> On Mon, 2016-08-29 at 20:59 +0000, Kuppuswamy, Sathyanarayanan wrote:
>> 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.
>>
>> diff --git a/arch/x86/platform/intel-mid/sfi.c
>> b/arch/x86/platform/intel-mid/sfi.c
>> index 051d264..a6bd275 100644
>> --- a/arch/x86/platform/intel-mid/sfi.c
>> +++ b/arch/x86/platform/intel-mid/sfi.c
>> @@ -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.
>
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);
}
just my 2 cents,
re,
wh
>> return;
>>
>> pdev = platform_device_alloc(pentry->name, 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;
>> @@ -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)
>> @@ -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 [andriy.shevchenko@linux.intel.com]
>> Sent: Sunday, August 28, 2016 6:31 AM
>> To: Dan Carpenter
>> Cc: kernel-janitors@vger.kernel.org; David Cohen; Kuppuswamy,
>> Sathyanarayanan
>> Subject: Re: [bug report] x86/sfi: Enable enumeration of SD devices
>>
>> + David, Sathya
>>
>> On Tue, 2016-08-09 at 20:58 +0300, Dan Carpenter wrote:
>>>
>>> On Tue, Aug 09, 2016 at 06:32:55PM +0300, Andy Shevchenko wrote:
>>>>
>>>>
>>>> On Fri, 2016-07-15 at 22:23 +0300, Dan Carpenter wrote:
>>>>>
>>>>>
>>>>> 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
>>>>
>>>> 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?
>>
>> 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 <andriy.shevchenko@linux.intel.com>
>> Intel Finland Oy
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [bug report] x86/sfi: Enable enumeration of SD devices
2016-07-15 19:23 [bug report] x86/sfi: Enable enumeration of SD devices Dan Carpenter
` (5 preceding siblings ...)
2016-08-30 11:06 ` walter harms
@ 2016-08-30 11:13 ` Andy Shevchenko
2016-08-30 18:18 ` Sathyanarayanan Kuppuswamy
` (3 subsequent siblings)
10 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-08-30 11:13 UTC (permalink / raw)
To: kernel-janitors
On Tue, 2016-08-30 at 13:06 +0200, walter harms wrote:
>
> Am 30.08.2016 11:46, schrieb Andy Shevchenko:
> >
> > On Mon, 2016-08-29 at 20:59 +0000, Kuppuswamy, Sathyanarayanan
> > wrote:
> > >
> > > 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.
> > >
> > > diff --git a/arch/x86/platform/intel-mid/sfi.c
> > > b/arch/x86/platform/intel-mid/sfi.c
> > > index 051d264..a6bd275 100644
> > > --- a/arch/x86/platform/intel-mid/sfi.c
> > > +++ b/arch/x86/platform/intel-mid/sfi.c
> > > @@ -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.
> >
>
> 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.
>
> #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);
> }
>
> > > 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 <andriy.shevchenko@linux.intel.com>
> > > Intel Finland Oy
> >
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [bug report] x86/sfi: Enable enumeration of SD devices
2016-07-15 19:23 [bug report] x86/sfi: Enable enumeration of SD devices Dan Carpenter
` (6 preceding siblings ...)
2016-08-30 11:13 ` Andy Shevchenko
@ 2016-08-30 18:18 ` Sathyanarayanan Kuppuswamy
2016-09-07 1:04 ` Kuppuswamy Sathyanarayanan
2016-09-01 13:17 ` [bug report] x86/sfi: Enable enumeration of SD devices Andy Shevchenko
` (2 subsequent siblings)
10 siblings, 1 reply; 38+ messages in thread
From: Sathyanarayanan Kuppuswamy @ 2016-08-30 18:18 UTC (permalink / raw)
To: kernel-janitors
On 08/30/2016 04:13 AM, Andy Shevchenko wrote:
> On Tue, 2016-08-30 at 13:06 +0200, walter harms wrote:
>> Am 30.08.2016 11:46, schrieb Andy Shevchenko:
>>> On Mon, 2016-08-29 at 20:59 +0000, Kuppuswamy, Sathyanarayanan
>>> wrote:
>>>> 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.
>>>>
>>>> diff --git a/arch/x86/platform/intel-mid/sfi.c
>>>> b/arch/x86/platform/intel-mid/sfi.c
>>>> index 051d264..a6bd275 100644
>>>> --- a/arch/x86/platform/intel-mid/sfi.c
>>>> +++ b/arch/x86/platform/intel-mid/sfi.c
>>>> @@ -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.
>>>
>> 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.
I missed the generic null scenario. Thanks for pointing it out Andy.
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.
static void __init *pcal9555a_platform_data(void *info)
{
struct i2c_board_info *i2c_info = info;
char *type = i2c_info->type;
struct pca953x_platform_data *pcal9555a;
char base_pin_name[SFI_NAME_LEN + 1];
char intr_pin_name[SFI_NAME_LEN + 1];
int gpio_base, intr;
snprintf(base_pin_name, sizeof(base_pin_name), "%s_base", type);
snprintf(intr_pin_name, sizeof(intr_pin_name), "%s_int", type);
gpio_base = get_gpio_by_name(base_pin_name);
intr = get_gpio_by_name(intr_pin_name);
/* Check if the SFI record valid */
if (gpio_base = -1)
return NULL;
if (nr >= PCAL9555A_NUM) {
pr_err("%s: Too many instances, only %d supported\n",
__func__,
PCAL9555A_NUM);
return NULL;
}
pcal9555a = &pcal9555a_pdata[nr++];
pcal9555a->gpio_base = gpio_base;
if (intr >= 0) {
i2c_info->irq = intr + INTEL_MID_IRQ_OFFSET;
pcal9555a->irq_base = gpio_base + INTEL_MID_IRQ_OFFSET;
} else {
i2c_info->irq = -1;
pcal9555a->irq_base = -1;
}
strcpy(type, "pcal9555a");
return pcal9555a;
}
>
>> #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);
>> }
>>
>>>> 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 <andriy.shevchenko@linux.intel.com>
>>>> Intel Finland Oy
--
Sathyanarayanan Kuppuswamy
Android kernel developer
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH 1/1] intel-mid: Fix sfi get_platform_data() return value issues
2016-08-30 18:18 ` Sathyanarayanan Kuppuswamy
@ 2016-09-07 1:04 ` Kuppuswamy Sathyanarayanan
0 siblings, 0 replies; 38+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2016-09-07 1:04 UTC (permalink / raw)
To: andriy.shevchenko, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen,
sathyanarayanan.kuppuswamy
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH 1/1] intel-mid: Fix sfi get_platform_data() return value issues
@ 2016-09-07 1:04 ` Kuppuswamy Sathyanarayanan
0 siblings, 0 replies; 38+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2016-09-07 1:04 UTC (permalink / raw)
To: andriy.shevchenko, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen,
sathyanarayanan.kuppuswamy
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 1/1] intel-mid: Fix sfi get_platform_data() return value issues
2016-09-07 1:04 ` Kuppuswamy Sathyanarayanan
@ 2016-09-07 12:15 ` Andy Shevchenko
-1 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-09-07 12:15 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/1] intel-mid: Fix sfi get_platform_data() return value issues
@ 2016-09-07 12:15 ` Andy Shevchenko
0 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-09-07 12:15 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/1] intel-mid: Fix sfi get_platform_data() return value issues
2016-09-07 12:15 ` Andy Shevchenko
@ 2016-09-08 0:04 ` sathyanarayanan kuppuswamy
-1 siblings, 0 replies; 38+ messages in thread
From: sathyanarayanan kuppuswamy @ 2016-09-08 0:04 UTC (permalink / raw)
To: Andy Shevchenko, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/1] intel-mid: Fix sfi get_platform_data() return value issues
@ 2016-09-08 0:04 ` sathyanarayanan kuppuswamy
0 siblings, 0 replies; 38+ messages in thread
From: sathyanarayanan kuppuswamy @ 2016-09-08 0:04 UTC (permalink / raw)
To: Andy Shevchenko, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/1] intel-mid: Fix sfi get_platform_data() return value issues
2016-09-08 0:04 ` sathyanarayanan kuppuswamy
@ 2016-09-08 9:49 ` Andy Shevchenko
-1 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-09-08 9:49 UTC (permalink / raw)
To: sathyanarayanan.kuppuswamy, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/1] intel-mid: Fix sfi get_platform_data() return value issues
@ 2016-09-08 9:49 ` Andy Shevchenko
0 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-09-08 9:49 UTC (permalink / raw)
To: sathyanarayanan.kuppuswamy, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 1/1] intel-mid: Fix sfi get_platform_data() return value issues
2016-09-07 1:04 ` Kuppuswamy Sathyanarayanan
@ 2016-09-08 0:05 ` Kuppuswamy Sathyanarayanan
-1 siblings, 0 replies; 38+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2016-09-08 0:05 UTC (permalink / raw)
To: andriy.shevchenko, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen,
sathyanarayanan.kuppuswamy
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v2 1/1] intel-mid: Fix sfi get_platform_data() return value issues
@ 2016-09-08 0:05 ` Kuppuswamy Sathyanarayanan
0 siblings, 0 replies; 38+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2016-09-08 0:05 UTC (permalink / raw)
To: andriy.shevchenko, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen,
sathyanarayanan.kuppuswamy
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH v2 1/1] intel-mid: Fix sfi get_platform_data() return value issues
2016-09-08 0:05 ` Kuppuswamy Sathyanarayanan
@ 2016-09-08 12:51 ` Andy Shevchenko
-1 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-09-08 12:51 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v2 1/1] intel-mid: Fix sfi get_platform_data() return value issues
@ 2016-09-08 12:51 ` Andy Shevchenko
0 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-09-08 12:51 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v2 1/1] intel-mid: Fix sfi get_platform_data() return value issues
2016-09-08 12:51 ` Andy Shevchenko
@ 2016-09-08 22:41 ` sathyanarayanan kuppuswamy
-1 siblings, 0 replies; 38+ messages in thread
From: sathyanarayanan kuppuswamy @ 2016-09-08 22:41 UTC (permalink / raw)
To: Andy Shevchenko, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v2 1/1] intel-mid: Fix sfi get_platform_data() return value issues
@ 2016-09-08 22:41 ` sathyanarayanan kuppuswamy
0 siblings, 0 replies; 38+ messages in thread
From: sathyanarayanan kuppuswamy @ 2016-09-08 22:41 UTC (permalink / raw)
To: Andy Shevchenko, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v2 1/1] intel-mid: Fix sfi get_platform_data() return value issues
2016-09-08 22:41 ` sathyanarayanan kuppuswamy
@ 2016-09-09 11:20 ` Andy Shevchenko
-1 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-09-09 11:20 UTC (permalink / raw)
To: sathyanarayanan.kuppuswamy, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v2 1/1] intel-mid: Fix sfi get_platform_data() return value issues
@ 2016-09-09 11:20 ` Andy Shevchenko
0 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-09-09 11:20 UTC (permalink / raw)
To: sathyanarayanan.kuppuswamy, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 1/3] intel-mid: Fix sfi get_platform_data() return value issues
2016-09-07 1:04 ` Kuppuswamy Sathyanarayanan
@ 2016-09-09 2:07 ` Kuppuswamy Sathyanarayanan
-1 siblings, 0 replies; 38+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2016-09-09 2:07 UTC (permalink / raw)
To: andriy.shevchenko, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen,
sathyanarayanan.kuppuswamy
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v3 1/3] intel-mid: Fix sfi get_platform_data() return value issues
@ 2016-09-09 2:07 ` Kuppuswamy Sathyanarayanan
0 siblings, 0 replies; 38+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2016-09-09 2:07 UTC (permalink / raw)
To: andriy.shevchenko, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen,
sathyanarayanan.kuppuswamy
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v3 2/3] intel-mid: Add valid error messages on init failure
2016-09-07 1:04 ` Kuppuswamy Sathyanarayanan
@ 2016-09-09 2:07 ` Kuppuswamy Sathyanarayanan
-1 siblings, 0 replies; 38+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2016-09-09 2:07 UTC (permalink / raw)
To: andriy.shevchenko, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen,
sathyanarayanan.kuppuswamy
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v3 2/3] intel-mid: Add valid error messages on init failure
@ 2016-09-09 2:07 ` Kuppuswamy Sathyanarayanan
0 siblings, 0 replies; 38+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2016-09-09 2:07 UTC (permalink / raw)
To: andriy.shevchenko, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen,
sathyanarayanan.kuppuswamy
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH v3 2/3] intel-mid: Add valid error messages on init failure
2016-09-09 2:07 ` Kuppuswamy Sathyanarayanan
@ 2016-09-09 11:27 ` Andy Shevchenko
-1 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-09-09 11:27 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v3 2/3] intel-mid: Add valid error messages on init failure
@ 2016-09-09 11:27 ` Andy Shevchenko
0 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-09-09 11:27 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 3/3] intel-mid: Move boundry check to the start of init code
2016-09-07 1:04 ` Kuppuswamy Sathyanarayanan
@ 2016-09-09 2:07 ` Kuppuswamy Sathyanarayanan
-1 siblings, 0 replies; 38+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2016-09-09 2:07 UTC (permalink / raw)
To: andriy.shevchenko, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen,
sathyanarayanan.kuppuswamy
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v3 3/3] intel-mid: Move boundry check to the start of init code
@ 2016-09-09 2:07 ` Kuppuswamy Sathyanarayanan
0 siblings, 0 replies; 38+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2016-09-09 2:07 UTC (permalink / raw)
To: andriy.shevchenko, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen,
sathyanarayanan.kuppuswamy
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH v3 3/3] intel-mid: Move boundry check to the start of init code
2016-09-09 2:07 ` Kuppuswamy Sathyanarayanan
@ 2016-09-09 11:30 ` Andy Shevchenko
-1 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-09-09 11:30 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v3 3/3] intel-mid: Move boundry check to the start of init code
@ 2016-09-09 11:30 ` Andy Shevchenko
0 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-09-09 11:30 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan, wharms
Cc: dan.carpenter, linux-kernel, kernel-janitors, david.a.cohen
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [bug report] x86/sfi: Enable enumeration of SD devices
2016-07-15 19:23 [bug report] x86/sfi: Enable enumeration of SD devices Dan Carpenter
` (7 preceding siblings ...)
2016-08-30 18:18 ` Sathyanarayanan Kuppuswamy
@ 2016-09-01 13:17 ` Andy Shevchenko
2016-09-07 0:51 ` Sathyanarayanan Kuppuswamy
2016-09-07 12:00 ` Andy Shevchenko
10 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-09-01 13:17 UTC (permalink / raw)
To: kernel-janitors
On Tue, 2016-08-30 at 11:18 -0700, Sathyanarayanan Kuppuswamy wrote:
> 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.
>
> 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!
> 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.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [bug report] x86/sfi: Enable enumeration of SD devices
2016-07-15 19:23 [bug report] x86/sfi: Enable enumeration of SD devices Dan Carpenter
` (8 preceding siblings ...)
2016-09-01 13:17 ` [bug report] x86/sfi: Enable enumeration of SD devices Andy Shevchenko
@ 2016-09-07 0:51 ` Sathyanarayanan Kuppuswamy
2016-09-07 12:00 ` Andy Shevchenko
10 siblings, 0 replies; 38+ messages in thread
From: Sathyanarayanan Kuppuswamy @ 2016-09-07 0:51 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1751 bytes --]
Hi Andy,
On 09/01/2016 06:17 AM, Andy Shevchenko wrote:
> On Tue, 2016-08-30 at 11:18 -0700, Sathyanarayanan Kuppuswamy wrote:
>
>> 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.
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.
>
>> 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!
>
>> 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.
>
>
--
Sathyanarayanan Kuppuswamy
Android kernel developer
[-- Attachment #2: 0001-intel-mid-Fix-sfi-get_platform_data-return-value-iss.patch --]
[-- Type: text/x-patch, Size: 7075 bytes --]
From c534b068dbfe26abd0a730f2c61695fbccd6964a Mon Sep 17 00:00:00 2001
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Date: Tue, 6 Sep 2016 16:42:15 -0700
Subject: [PATCH 1/1] intel-mid: Fix sfi get_platform_data() return value
issues
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [bug report] x86/sfi: Enable enumeration of SD devices
2016-07-15 19:23 [bug report] x86/sfi: Enable enumeration of SD devices Dan Carpenter
` (9 preceding siblings ...)
2016-09-07 0:51 ` Sathyanarayanan Kuppuswamy
@ 2016-09-07 12:00 ` Andy Shevchenko
10 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2016-09-07 12:00 UTC (permalink / raw)
To: kernel-janitors
On Tue, 2016-09-06 at 17:51 -0700, Sathyanarayanan Kuppuswamy wrote:
> On 09/01/2016 06:17 AM, Andy Shevchenko wrote:
> > On Tue, 2016-08-30 at 11:18 -0700, Sathyanarayanan Kuppuswamy wrote:
> > 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.
> 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 ?
That's what I'm wondering of. I didn't investigate this deep.
In any case for 2) we have to print out a warning (?) message.
> Attached patch fixes the return value issues in get_platform_data
> code
> in device_libs directory. Please check and let me know your comments.
Will do.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 38+ messages in thread