Re: [PATCH v3 2/3] intel-mid: Add valid error messages on init failure

[Andy Shevchenko]

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