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
--
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
