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

From: Andy Shevchenko
Date: Wed Sep 07 2016 - 08:15:21 EST

Next message: Arnd Bergmann: "Re: [PATCH] qed: add missing header dependencies"
Previous message: BÃlint RÃczey: "Oops.kernel.org usage"
In reply to: Kuppuswamy Sathyanarayanan: "[PATCH 1/1] intel-mid: Fix sfi get_platform_data() return value issues"
Next in thread: sathyanarayanan kuppuswamy: "Re: [PATCH 1/1] intel-mid: Fix sfi get_platform_data() return value issues"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxxxx>
Intel Finland Oy

Next message: Arnd Bergmann: "Re: [PATCH] qed: add missing header dependencies"
Previous message: BÃlint RÃczey: "Oops.kernel.org usage"
In reply to: Kuppuswamy Sathyanarayanan: "[PATCH 1/1] intel-mid: Fix sfi get_platform_data() return value issues"
Next in thread: sathyanarayanan kuppuswamy: "Re: [PATCH 1/1] intel-mid: Fix sfi get_platform_data() return value issues"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]