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
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
