From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com>,
wharms@bfs.de
Cc: dan.carpenter@oracle.com, linux-kernel@vger.kernel.org,
kernel-janitors@vger.kernel.org, david.a.cohen@linux.intel.com
Subject: Re: [PATCH 1/1] intel-mid: Fix sfi get_platform_data() return value issues
Date: Wed, 07 Sep 2016 15:15:09 +0300 [thread overview]
Message-ID: <1473250509.11323.60.camel@linux.intel.com> (raw)
In-Reply-To: <1473210255-227672-1-git-send-email-sathyanarayanan.kuppuswamy@linux.intel.com>
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
next prev parent reply other threads:[~2016-09-07 12:15 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <a42b4af5-e2a5-d922-b5a5-ab177bb15140@intel.com>
2016-09-07 1:04 ` [PATCH 1/1] intel-mid: Fix sfi get_platform_data() return value issues Kuppuswamy Sathyanarayanan
2016-09-07 12:15 ` Andy Shevchenko [this message]
2016-09-08 0:04 ` sathyanarayanan kuppuswamy
2016-09-08 9:49 ` Andy Shevchenko
2016-09-08 0:05 ` [PATCH v2 " Kuppuswamy Sathyanarayanan
2016-09-08 12:51 ` Andy Shevchenko
2016-09-08 22:41 ` sathyanarayanan kuppuswamy
2016-09-09 11:20 ` Andy Shevchenko
2016-09-09 2:07 ` [PATCH v3 1/3] " Kuppuswamy Sathyanarayanan
2016-09-09 2:07 ` [PATCH v3 2/3] intel-mid: Add valid error messages on init failure Kuppuswamy Sathyanarayanan
2016-09-09 11:27 ` Andy Shevchenko
2016-09-09 2:07 ` [PATCH v3 3/3] intel-mid: Move boundry check to the start of init code Kuppuswamy Sathyanarayanan
2016-09-09 11:30 ` Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1473250509.11323.60.camel@linux.intel.com \
--to=andriy.shevchenko@linux.intel.com \
--cc=dan.carpenter@oracle.com \
--cc=david.a.cohen@linux.intel.com \
--cc=kernel-janitors@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=wharms@bfs.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).