* [PATCH 0/5] Last batch of W=1 warning fixes in MFD @ 2020-06-29 12:32 Lee Jones 2020-06-29 12:32 ` [PATCH 1/5] mfd: si476x-cmd: Add missing documentation for si476x_cmd_fm_rds_status()'s arg 'report' Lee Jones ` (4 more replies) 0 siblings, 5 replies; 21+ messages in thread From: Lee Jones @ 2020-06-29 12:32 UTC (permalink / raw) To: lee.jones; +Cc: linux-arm-kernel, linux-kernel Attempting to clean-up W=1 kernel builds, which are currently overwhelmingly riddled with niggly little warnings. Lee Jones (5): mfd: si476x-cmd: Add missing documentation for si476x_cmd_fm_rds_status()'s arg 'report' mfd: lm3533-ctrlbank: Cap BRIGHTNESS_MAX to 127 since API uses u8 as carrier mfd: rave-sp: Fix mistake in 'struct rave_sp_deframer's kerneldoc mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask mfd: axp20x-i2c: Do not define 'struct acpi_device_id' when !CONFIG_ACPI drivers/mfd/axp20x-i2c.c | 2 ++ drivers/mfd/lm3533-ctrlbank.c | 2 +- drivers/mfd/rave-sp.c | 2 +- drivers/mfd/si476x-cmd.c | 2 ++ drivers/mfd/sprd-sc27xx-spi.c | 2 +- 5 files changed, 7 insertions(+), 3 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/5] mfd: si476x-cmd: Add missing documentation for si476x_cmd_fm_rds_status()'s arg 'report' 2020-06-29 12:32 [PATCH 0/5] Last batch of W=1 warning fixes in MFD Lee Jones @ 2020-06-29 12:32 ` Lee Jones 2020-06-29 12:32 ` [PATCH 2/5] mfd: lm3533-ctrlbank: Cap BRIGHTNESS_MAX to 127 since API uses u8 as carrier Lee Jones ` (3 subsequent siblings) 4 siblings, 0 replies; 21+ messages in thread From: Lee Jones @ 2020-06-29 12:32 UTC (permalink / raw) To: lee.jones; +Cc: linux-arm-kernel, linux-kernel, Andrey Smirnov Kerneldoc syntax is used, but not complete. Descriptions are required for all arguments. Fixes the following W=1 build warning: drivers/mfd/si476x-cmd.c:907: warning: Function parameter or member 'report' not described in 'si476x_core_cmd_fm_rds_status' Cc: Andrey Smirnov <andrew.smirnov@gmail.com> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/mfd/si476x-cmd.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/mfd/si476x-cmd.c b/drivers/mfd/si476x-cmd.c index 0a40c5cb751db..d15b3e7833692 100644 --- a/drivers/mfd/si476x-cmd.c +++ b/drivers/mfd/si476x-cmd.c @@ -892,6 +892,8 @@ EXPORT_SYMBOL_GPL(si476x_core_cmd_fm_seek_start); * rest RDS data contains the last valid info received * @mtfifo: if set the command clears RDS receive FIFO * @intack: if set the command clards the RDSINT bit. + * @report: - all signal quality information retured by the command + * (if NULL then the output of the command is ignored) * * Function returns 0 on success and negative error code on failure */ -- 2.25.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/5] mfd: lm3533-ctrlbank: Cap BRIGHTNESS_MAX to 127 since API uses u8 as carrier 2020-06-29 12:32 [PATCH 0/5] Last batch of W=1 warning fixes in MFD Lee Jones 2020-06-29 12:32 ` [PATCH 1/5] mfd: si476x-cmd: Add missing documentation for si476x_cmd_fm_rds_status()'s arg 'report' Lee Jones @ 2020-06-29 12:32 ` Lee Jones 2020-06-29 12:51 ` Johan Hovold 2020-06-29 12:32 ` [PATCH 3/5] mfd: rave-sp: Fix mistake in 'struct rave_sp_deframer's kerneldoc Lee Jones ` (2 subsequent siblings) 4 siblings, 1 reply; 21+ messages in thread From: Lee Jones @ 2020-06-29 12:32 UTC (permalink / raw) To: lee.jones; +Cc: linux-arm-kernel, linux-kernel, Johan Hovold Since its conception in 2012 brightness has been artificially capped at 127 since the variable carrying the value is u8. We could go to the trouble of changing the whole API (crossing 3 different subsystems), but clearly this hasn't bothered anyone in the best part of a decade. Simply, cap BRIGHTNESS_MAX to 127 instead (for now at least). Fixes the following W=1 warning(s): drivers/mfd/lm3533-ctrlbank.c: In function ‘lm3533_ctrlbank_set_brightness’: drivers/mfd/lm3533-ctrlbank.c:98:10: warning: comparison is always false due to limited range of data type [-Wtype-limits] 98 | if (val > LM3533_##_NAME##_MAX) | ^ drivers/mfd/lm3533-ctrlbank.c:125:1: note: in expansion of macro ‘lm3533_ctrlbank_set’ 125 | lm3533_ctrlbank_set(brightness, BRIGHTNESS); | ^~~~~~~~~~~~~~~~~~~ Cc: Johan Hovold <jhovold@gmail.com> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/mfd/lm3533-ctrlbank.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mfd/lm3533-ctrlbank.c b/drivers/mfd/lm3533-ctrlbank.c index 34fba06ec7057..348ce67453092 100644 --- a/drivers/mfd/lm3533-ctrlbank.c +++ b/drivers/mfd/lm3533-ctrlbank.c @@ -17,7 +17,7 @@ #define LM3533_MAX_CURRENT_MAX 29800 #define LM3533_MAX_CURRENT_STEP 800 -#define LM3533_BRIGHTNESS_MAX 255 +#define LM3533_BRIGHTNESS_MAX 127 /* Capped by API - could be up to 255 */ #define LM3533_PWM_MAX 0x3f #define LM3533_REG_PWM_BASE 0x14 -- 2.25.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/5] mfd: lm3533-ctrlbank: Cap BRIGHTNESS_MAX to 127 since API uses u8 as carrier 2020-06-29 12:32 ` [PATCH 2/5] mfd: lm3533-ctrlbank: Cap BRIGHTNESS_MAX to 127 since API uses u8 as carrier Lee Jones @ 2020-06-29 12:51 ` Johan Hovold 2020-06-29 13:25 ` Lee Jones 0 siblings, 1 reply; 21+ messages in thread From: Johan Hovold @ 2020-06-29 12:51 UTC (permalink / raw) To: Lee Jones; +Cc: linux-arm-kernel, linux-kernel, Johan Hovold On Mon, Jun 29, 2020 at 01:32:12PM +0100, Lee Jones wrote: > Since its conception in 2012 brightness has been artificially capped > at 127 since the variable carrying the value is u8. We could go to > the trouble of changing the whole API (crossing 3 different subsystems), > but clearly this hasn't bothered anyone in the best part of a decade. > > Simply, cap BRIGHTNESS_MAX to 127 instead (for now at least). Hmm. This patch is clearly broken and would contrary to the claim be introducing an artificial cap at half brightness. u8 can hold the max brightness value 255 just fine. > Fixes the following W=1 warning(s): > > drivers/mfd/lm3533-ctrlbank.c: In function ‘lm3533_ctrlbank_set_brightness’: > drivers/mfd/lm3533-ctrlbank.c:98:10: warning: comparison is always false due to limited range of data type [-Wtype-limits] > 98 | if (val > LM3533_##_NAME##_MAX) | ^ > drivers/mfd/lm3533-ctrlbank.c:125:1: note: in expansion of macro ‘lm3533_ctrlbank_set’ > 125 | lm3533_ctrlbank_set(brightness, BRIGHTNESS); > | ^~~~~~~~~~~~~~~~~~~ This warning is benign. The same macro is used to defined two function where in one case the max value coincides with U8_MAX so that the sanity check becomes redundant. > Cc: Johan Hovold <jhovold@gmail.com> > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/mfd/lm3533-ctrlbank.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mfd/lm3533-ctrlbank.c b/drivers/mfd/lm3533-ctrlbank.c > index 34fba06ec7057..348ce67453092 100644 > --- a/drivers/mfd/lm3533-ctrlbank.c > +++ b/drivers/mfd/lm3533-ctrlbank.c > @@ -17,7 +17,7 @@ > #define LM3533_MAX_CURRENT_MAX 29800 > #define LM3533_MAX_CURRENT_STEP 800 > > -#define LM3533_BRIGHTNESS_MAX 255 > +#define LM3533_BRIGHTNESS_MAX 127 /* Capped by API - could be up to 255 */ > #define LM3533_PWM_MAX 0x3f > > #define LM3533_REG_PWM_BASE 0x14 Johan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/5] mfd: lm3533-ctrlbank: Cap BRIGHTNESS_MAX to 127 since API uses u8 as carrier 2020-06-29 12:51 ` Johan Hovold @ 2020-06-29 13:25 ` Lee Jones 2020-06-30 8:38 ` Johan Hovold 0 siblings, 1 reply; 21+ messages in thread From: Lee Jones @ 2020-06-29 13:25 UTC (permalink / raw) To: Johan Hovold; +Cc: linux-arm-kernel, linux-kernel, Johan Hovold On Mon, 29 Jun 2020, Johan Hovold wrote: > On Mon, Jun 29, 2020 at 01:32:12PM +0100, Lee Jones wrote: > > Since its conception in 2012 brightness has been artificially capped > > at 127 since the variable carrying the value is u8. We could go to > > the trouble of changing the whole API (crossing 3 different subsystems), > > but clearly this hasn't bothered anyone in the best part of a decade. > > > > Simply, cap BRIGHTNESS_MAX to 127 instead (for now at least). > > Hmm. This patch is clearly broken and would contrary to the claim be > introducing an artificial cap at half brightness. u8 can hold the max > brightness value 255 just fine. Yes, of course it can. Senior moment on my account. > > Fixes the following W=1 warning(s): > > > > drivers/mfd/lm3533-ctrlbank.c: In function ‘lm3533_ctrlbank_set_brightness’: > > drivers/mfd/lm3533-ctrlbank.c:98:10: warning: comparison is always false due to limited range of data type [-Wtype-limits] > > 98 | if (val > LM3533_##_NAME##_MAX) | ^ > > drivers/mfd/lm3533-ctrlbank.c:125:1: note: in expansion of macro ‘lm3533_ctrlbank_set’ > > 125 | lm3533_ctrlbank_set(brightness, BRIGHTNESS); > > | ^~~~~~~~~~~~~~~~~~~ > > This warning is benign. The same macro is used to defined two function > where in one case the max value coincides with U8_MAX so that the sanity > check becomes redundant. A benign warning, as most W=1 warnings are, is still a warning. So how do you propose we fix it? Is 255 a valid and used brightness level? If so, how do you feel about: /* Avoid 'always false' check '(u8) > 255' */ if (LM3533_##_NAME##_MAX != 0xff && val > LM3533_##_NAME##_MAX) return -EINVAL; > > Cc: Johan Hovold <jhovold@gmail.com> > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > --- > > drivers/mfd/lm3533-ctrlbank.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/mfd/lm3533-ctrlbank.c b/drivers/mfd/lm3533-ctrlbank.c > > index 34fba06ec7057..348ce67453092 100644 > > --- a/drivers/mfd/lm3533-ctrlbank.c > > +++ b/drivers/mfd/lm3533-ctrlbank.c > > @@ -17,7 +17,7 @@ > > #define LM3533_MAX_CURRENT_MAX 29800 > > #define LM3533_MAX_CURRENT_STEP 800 > > > > -#define LM3533_BRIGHTNESS_MAX 255 > > +#define LM3533_BRIGHTNESS_MAX 127 /* Capped by API - could be up to 255 */ > > #define LM3533_PWM_MAX 0x3f > > > > #define LM3533_REG_PWM_BASE 0x14 > > Johan -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/5] mfd: lm3533-ctrlbank: Cap BRIGHTNESS_MAX to 127 since API uses u8 as carrier 2020-06-29 13:25 ` Lee Jones @ 2020-06-30 8:38 ` Johan Hovold 0 siblings, 0 replies; 21+ messages in thread From: Johan Hovold @ 2020-06-30 8:38 UTC (permalink / raw) To: Lee Jones; +Cc: Johan Hovold, linux-arm-kernel, linux-kernel, Johan Hovold On Mon, Jun 29, 2020 at 02:25:06PM +0100, Lee Jones wrote: > On Mon, 29 Jun 2020, Johan Hovold wrote: > > > On Mon, Jun 29, 2020 at 01:32:12PM +0100, Lee Jones wrote: > > > Since its conception in 2012 brightness has been artificially capped > > > at 127 since the variable carrying the value is u8. We could go to > > > the trouble of changing the whole API (crossing 3 different subsystems), > > > but clearly this hasn't bothered anyone in the best part of a decade. > > > > > > Simply, cap BRIGHTNESS_MAX to 127 instead (for now at least). > > > > Hmm. This patch is clearly broken and would contrary to the claim be > > introducing an artificial cap at half brightness. u8 can hold the max > > brightness value 255 just fine. > > Yes, of course it can. Senior moment on my account. > > > > Fixes the following W=1 warning(s): > > > > > > drivers/mfd/lm3533-ctrlbank.c: In function ‘lm3533_ctrlbank_set_brightness’: > > > drivers/mfd/lm3533-ctrlbank.c:98:10: warning: comparison is always false due to limited range of data type [-Wtype-limits] > > > 98 | if (val > LM3533_##_NAME##_MAX) | ^ > > > drivers/mfd/lm3533-ctrlbank.c:125:1: note: in expansion of macro ‘lm3533_ctrlbank_set’ > > > 125 | lm3533_ctrlbank_set(brightness, BRIGHTNESS); > > > | ^~~~~~~~~~~~~~~~~~~ > > > > This warning is benign. The same macro is used to defined two function > > where in one case the max value coincides with U8_MAX so that the sanity > > check becomes redundant. > > A benign warning, as most W=1 warnings are, is still a warning. Not every warning needs to be addressed, there's a reason some of these are hidden behind W=1 or higher. > So how do you propose we fix it? > > Is 255 a valid and used brightness level? Yes. > If so, how do you feel about: > > /* Avoid 'always false' check '(u8) > 255' */ > if (LM3533_##_NAME##_MAX != 0xff && val > LM3533_##_NAME##_MAX) > return -EINVAL; I'm afraid that's not sufficient to shut the compiler up. I'll send you patch expanding these accessors instead. Having exported functions implemented by macros is particularly nice (hard to grep for etc). There are a couple of more sets of control-bank registers that could potentially have shared the implementation and which motivated the use of macros, but it does not seem very likely that we'll be adding those anytime soon anyway. Johan ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/5] mfd: rave-sp: Fix mistake in 'struct rave_sp_deframer's kerneldoc 2020-06-29 12:32 [PATCH 0/5] Last batch of W=1 warning fixes in MFD Lee Jones 2020-06-29 12:32 ` [PATCH 1/5] mfd: si476x-cmd: Add missing documentation for si476x_cmd_fm_rds_status()'s arg 'report' Lee Jones 2020-06-29 12:32 ` [PATCH 2/5] mfd: lm3533-ctrlbank: Cap BRIGHTNESS_MAX to 127 since API uses u8 as carrier Lee Jones @ 2020-06-29 12:32 ` Lee Jones 2020-06-29 12:32 ` [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask Lee Jones 2020-06-29 12:32 ` [PATCH 5/5] mfd: axp20x-i2c: Do not define 'struct acpi_device_id' when !CONFIG_ACPI Lee Jones 4 siblings, 0 replies; 21+ messages in thread From: Lee Jones @ 2020-06-29 12:32 UTC (permalink / raw) To: lee.jones Cc: linux-arm-kernel, linux-kernel, Andrey Vostrikov, Nikita Yushchenko, Andrey Smirnov Argument 'received' was incorrectly named by its struct type 'completion' instead of its name. Fixes the following W=1 warning: drivers/mfd/rave-sp.c:107: warning: Function parameter or member 'received' not described in 'rave_sp_reply' Cc: Andrey Vostrikov <andrey.vostrikov@cogentembedded.com> Cc: Nikita Yushchenko <nikita.yoush@cogentembedded.com> Cc: Andrey Smirnov <andrew.smirnov@gmail.com> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/mfd/rave-sp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mfd/rave-sp.c b/drivers/mfd/rave-sp.c index 26c7b63e008a8..abaab541df19d 100644 --- a/drivers/mfd/rave-sp.c +++ b/drivers/mfd/rave-sp.c @@ -96,7 +96,7 @@ struct rave_sp_deframer { * @data: Buffer to store reply payload in * @code: Expected reply code * @ackid: Expected reply ACK ID - * @completion: Successful reply reception completion + * @received: Successful reply reception completion */ struct rave_sp_reply { size_t length; -- 2.25.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask 2020-06-29 12:32 [PATCH 0/5] Last batch of W=1 warning fixes in MFD Lee Jones ` (2 preceding siblings ...) 2020-06-29 12:32 ` [PATCH 3/5] mfd: rave-sp: Fix mistake in 'struct rave_sp_deframer's kerneldoc Lee Jones @ 2020-06-29 12:32 ` Lee Jones 2020-06-29 13:06 ` Johan Hovold 2020-07-01 9:15 ` [PATCH] mfd: sprd-sc27xx-spi: Fix-up bogus IRQ register offset and mask setting Lee Jones 2020-06-29 12:32 ` [PATCH 5/5] mfd: axp20x-i2c: Do not define 'struct acpi_device_id' when !CONFIG_ACPI Lee Jones 4 siblings, 2 replies; 21+ messages in thread From: Lee Jones @ 2020-06-29 12:32 UTC (permalink / raw) To: lee.jones Cc: linux-arm-kernel, linux-kernel, Orson Zhai, Baolin Wang, Chunyan Zhang Since ddata->irqs[] is already zeroed when allocated by devm_kcalloc() and dividing 0 by anything is still 0, there is no need to re-assign ddata->irqs[i].* values. Instead, it should be safe to begin at 1. This fixes the following W=1 warning: drivers/mfd/sprd-sc27xx-spi.c:255 sprd_pmic_probe() debug: sval_binop_unsigned: divide by zero Cc: Orson Zhai <orsonzhai@gmail.com> Cc: Baolin Wang <baolin.wang7@gmail.com> Cc: Chunyan Zhang <zhang.lyra@gmail.com> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/mfd/sprd-sc27xx-spi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c index c305e941e435c..694a7d429ccff 100644 --- a/drivers/mfd/sprd-sc27xx-spi.c +++ b/drivers/mfd/sprd-sc27xx-spi.c @@ -251,7 +251,7 @@ static int sprd_pmic_probe(struct spi_device *spi) return -ENOMEM; ddata->irq_chip.irqs = ddata->irqs; - for (i = 0; i < pdata->num_irqs; i++) { + for (i = 1; i < pdata->num_irqs; i++) { ddata->irqs[i].reg_offset = i / pdata->num_irqs; ddata->irqs[i].mask = BIT(i % pdata->num_irqs); } -- 2.25.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask 2020-06-29 12:32 ` [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask Lee Jones @ 2020-06-29 13:06 ` Johan Hovold 2020-06-29 14:01 ` Lee Jones 2020-07-01 9:15 ` [PATCH] mfd: sprd-sc27xx-spi: Fix-up bogus IRQ register offset and mask setting Lee Jones 1 sibling, 1 reply; 21+ messages in thread From: Johan Hovold @ 2020-06-29 13:06 UTC (permalink / raw) To: Lee Jones Cc: Baolin Wang, Orson Zhai, linux-kernel, linux-arm-kernel, Chunyan Zhang On Mon, Jun 29, 2020 at 01:32:14PM +0100, Lee Jones wrote: > Since ddata->irqs[] is already zeroed when allocated by devm_kcalloc() and > dividing 0 by anything is still 0, there is no need to re-assign > ddata->irqs[i].* values. Instead, it should be safe to begin at 1. > > This fixes the following W=1 warning: > > drivers/mfd/sprd-sc27xx-spi.c:255 sprd_pmic_probe() debug: sval_binop_unsigned: divide by zero > > Cc: Orson Zhai <orsonzhai@gmail.com> > Cc: Baolin Wang <baolin.wang7@gmail.com> > Cc: Chunyan Zhang <zhang.lyra@gmail.com> > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/mfd/sprd-sc27xx-spi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c > index c305e941e435c..694a7d429ccff 100644 > --- a/drivers/mfd/sprd-sc27xx-spi.c > +++ b/drivers/mfd/sprd-sc27xx-spi.c > @@ -251,7 +251,7 @@ static int sprd_pmic_probe(struct spi_device *spi) > return -ENOMEM; > > ddata->irq_chip.irqs = ddata->irqs; > - for (i = 0; i < pdata->num_irqs; i++) { > + for (i = 1; i < pdata->num_irqs; i++) { > ddata->irqs[i].reg_offset = i / pdata->num_irqs; > ddata->irqs[i].mask = BIT(i % pdata->num_irqs); > } This doesn't look right either. First, the loop is never executed if num_irqs is zero. Second, the current code looks bogus too as reg_offset is always set to zero and mask to BIT(i)... Johan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask 2020-06-29 13:06 ` Johan Hovold @ 2020-06-29 14:01 ` Lee Jones 2020-06-29 14:35 ` Baolin Wang 0 siblings, 1 reply; 21+ messages in thread From: Lee Jones @ 2020-06-29 14:01 UTC (permalink / raw) To: Johan Hovold Cc: Baolin Wang, Orson Zhai, linux-kernel, linux-arm-kernel, Chunyan Zhang On Mon, 29 Jun 2020, Johan Hovold wrote: > On Mon, Jun 29, 2020 at 01:32:14PM +0100, Lee Jones wrote: > > Since ddata->irqs[] is already zeroed when allocated by devm_kcalloc() and > > dividing 0 by anything is still 0, there is no need to re-assign > > ddata->irqs[i].* values. Instead, it should be safe to begin at 1. > > > > This fixes the following W=1 warning: > > > > drivers/mfd/sprd-sc27xx-spi.c:255 sprd_pmic_probe() debug: sval_binop_unsigned: divide by zero > > > > Cc: Orson Zhai <orsonzhai@gmail.com> > > Cc: Baolin Wang <baolin.wang7@gmail.com> > > Cc: Chunyan Zhang <zhang.lyra@gmail.com> > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > --- > > drivers/mfd/sprd-sc27xx-spi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c > > index c305e941e435c..694a7d429ccff 100644 > > --- a/drivers/mfd/sprd-sc27xx-spi.c > > +++ b/drivers/mfd/sprd-sc27xx-spi.c > > @@ -251,7 +251,7 @@ static int sprd_pmic_probe(struct spi_device *spi) > > return -ENOMEM; > > > > ddata->irq_chip.irqs = ddata->irqs; > > - for (i = 0; i < pdata->num_irqs; i++) { > > + for (i = 1; i < pdata->num_irqs; i++) { > > ddata->irqs[i].reg_offset = i / pdata->num_irqs; > > ddata->irqs[i].mask = BIT(i % pdata->num_irqs); > > } > > This doesn't look right either. > > First, the loop is never executed if num_irqs is zero. The point of the patch is that 0 entries are never processed. However, what I appear to have overlooked is that BIT(0 % x) is not 0, it's 1. > Second, the current code looks bogus too as reg_offset is always set to > zero and mask to BIT(i)... Heh. I wonder if/how this was tested. I'm going to wait to hear from the authors before attempting to fix this again. Baolin, Could you please clarify this for us please? -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask 2020-06-29 14:01 ` Lee Jones @ 2020-06-29 14:35 ` Baolin Wang 2020-06-29 14:43 ` Johan Hovold 0 siblings, 1 reply; 21+ messages in thread From: Baolin Wang @ 2020-06-29 14:35 UTC (permalink / raw) To: Lee Jones; +Cc: Johan Hovold, Orson Zhai, LKML, linux-arm-kernel, Chunyan Zhang On Mon, Jun 29, 2020 at 10:01 PM Lee Jones <lee.jones@linaro.org> wrote: > > On Mon, 29 Jun 2020, Johan Hovold wrote: > > > On Mon, Jun 29, 2020 at 01:32:14PM +0100, Lee Jones wrote: > > > Since ddata->irqs[] is already zeroed when allocated by devm_kcalloc() and > > > dividing 0 by anything is still 0, there is no need to re-assign > > > ddata->irqs[i].* values. Instead, it should be safe to begin at 1. > > > > > > This fixes the following W=1 warning: > > > > > > drivers/mfd/sprd-sc27xx-spi.c:255 sprd_pmic_probe() debug: sval_binop_unsigned: divide by zero > > > > > > Cc: Orson Zhai <orsonzhai@gmail.com> > > > Cc: Baolin Wang <baolin.wang7@gmail.com> > > > Cc: Chunyan Zhang <zhang.lyra@gmail.com> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > > --- > > > drivers/mfd/sprd-sc27xx-spi.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c > > > index c305e941e435c..694a7d429ccff 100644 > > > --- a/drivers/mfd/sprd-sc27xx-spi.c > > > +++ b/drivers/mfd/sprd-sc27xx-spi.c > > > @@ -251,7 +251,7 @@ static int sprd_pmic_probe(struct spi_device *spi) > > > return -ENOMEM; > > > > > > ddata->irq_chip.irqs = ddata->irqs; > > > - for (i = 0; i < pdata->num_irqs; i++) { > > > + for (i = 1; i < pdata->num_irqs; i++) { > > > ddata->irqs[i].reg_offset = i / pdata->num_irqs; > > > ddata->irqs[i].mask = BIT(i % pdata->num_irqs); > > > } > > > > This doesn't look right either. > > > > First, the loop is never executed if num_irqs is zero. > > The point of the patch is that 0 entries are never processed. > > However, what I appear to have overlooked is that BIT(0 % x) is not 0, > it's 1. Yes. > > > Second, the current code looks bogus too as reg_offset is always set to > > zero and mask to BIT(i)... Now the result is correct, since all PMIC irq mask bits are in one register now, which means the reg_offset is always 0 can work well. But I think the logics still can be improved if our PMIC irq numbers are larger than 32 in future. > > Heh. I wonder if/how this was tested. > > I'm going to wait to hear from the authors before attempting to fix > this again. > > Baolin, Could you please clarify this for us please? Yes, see above comments. -- Baolin Wang ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask 2020-06-29 14:35 ` Baolin Wang @ 2020-06-29 14:43 ` Johan Hovold 2020-06-29 15:08 ` Baolin Wang 0 siblings, 1 reply; 21+ messages in thread From: Johan Hovold @ 2020-06-29 14:43 UTC (permalink / raw) To: Baolin Wang Cc: Lee Jones, Johan Hovold, Orson Zhai, LKML, linux-arm-kernel, Chunyan Zhang On Mon, Jun 29, 2020 at 10:35:06PM +0800, Baolin Wang wrote: > On Mon, Jun 29, 2020 at 10:01 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > On Mon, 29 Jun 2020, Johan Hovold wrote: > > > > > On Mon, Jun 29, 2020 at 01:32:14PM +0100, Lee Jones wrote: > > > > Since ddata->irqs[] is already zeroed when allocated by devm_kcalloc() and > > > > dividing 0 by anything is still 0, there is no need to re-assign > > > > ddata->irqs[i].* values. Instead, it should be safe to begin at 1. > > > > > > > > This fixes the following W=1 warning: > > > > > > > > drivers/mfd/sprd-sc27xx-spi.c:255 sprd_pmic_probe() debug: sval_binop_unsigned: divide by zero > > > > > > > > Cc: Orson Zhai <orsonzhai@gmail.com> > > > > Cc: Baolin Wang <baolin.wang7@gmail.com> > > > > Cc: Chunyan Zhang <zhang.lyra@gmail.com> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > > > --- > > > > drivers/mfd/sprd-sc27xx-spi.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c > > > > index c305e941e435c..694a7d429ccff 100644 > > > > --- a/drivers/mfd/sprd-sc27xx-spi.c > > > > +++ b/drivers/mfd/sprd-sc27xx-spi.c > > > > @@ -251,7 +251,7 @@ static int sprd_pmic_probe(struct spi_device *spi) > > > > return -ENOMEM; > > > > > > > > ddata->irq_chip.irqs = ddata->irqs; > > > > - for (i = 0; i < pdata->num_irqs; i++) { > > > > + for (i = 1; i < pdata->num_irqs; i++) { > > > > ddata->irqs[i].reg_offset = i / pdata->num_irqs; > > > > ddata->irqs[i].mask = BIT(i % pdata->num_irqs); > > > > } > > > > > > This doesn't look right either. > > > > > > First, the loop is never executed if num_irqs is zero. > > > > The point of the patch is that 0 entries are never processed. So what's the problem? There's no division by zero here. And what compiler are you using, Lee? Seems broken. > > > Second, the current code looks bogus too as reg_offset is always set to > > > zero and mask to BIT(i)... > > Now the result is correct, since all PMIC irq mask bits are in one > register now, which means the reg_offset is always 0 can work well. > But I think the logics still can be improved if our PMIC irq numbers > are larger than 32 in future. The code is still bogus as pointed out above. Why do you bother to divide by num_irqs at all? And what have you guys been smoking? ;) Johan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask 2020-06-29 14:43 ` Johan Hovold @ 2020-06-29 15:08 ` Baolin Wang 2020-06-29 15:45 ` Lee Jones 0 siblings, 1 reply; 21+ messages in thread From: Baolin Wang @ 2020-06-29 15:08 UTC (permalink / raw) To: Johan Hovold; +Cc: Lee Jones, Orson Zhai, LKML, linux-arm-kernel, Chunyan Zhang On Mon, Jun 29, 2020 at 10:43 PM Johan Hovold <johan@kernel.org> wrote: > > On Mon, Jun 29, 2020 at 10:35:06PM +0800, Baolin Wang wrote: > > On Mon, Jun 29, 2020 at 10:01 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > > > On Mon, 29 Jun 2020, Johan Hovold wrote: > > > > > > > On Mon, Jun 29, 2020 at 01:32:14PM +0100, Lee Jones wrote: > > > > > Since ddata->irqs[] is already zeroed when allocated by devm_kcalloc() and > > > > > dividing 0 by anything is still 0, there is no need to re-assign > > > > > ddata->irqs[i].* values. Instead, it should be safe to begin at 1. > > > > > > > > > > This fixes the following W=1 warning: > > > > > > > > > > drivers/mfd/sprd-sc27xx-spi.c:255 sprd_pmic_probe() debug: sval_binop_unsigned: divide by zero > > > > > > > > > > Cc: Orson Zhai <orsonzhai@gmail.com> > > > > > Cc: Baolin Wang <baolin.wang7@gmail.com> > > > > > Cc: Chunyan Zhang <zhang.lyra@gmail.com> > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > > > > --- > > > > > drivers/mfd/sprd-sc27xx-spi.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c > > > > > index c305e941e435c..694a7d429ccff 100644 > > > > > --- a/drivers/mfd/sprd-sc27xx-spi.c > > > > > +++ b/drivers/mfd/sprd-sc27xx-spi.c > > > > > @@ -251,7 +251,7 @@ static int sprd_pmic_probe(struct spi_device *spi) > > > > > return -ENOMEM; > > > > > > > > > > ddata->irq_chip.irqs = ddata->irqs; > > > > > - for (i = 0; i < pdata->num_irqs; i++) { > > > > > + for (i = 1; i < pdata->num_irqs; i++) { > > > > > ddata->irqs[i].reg_offset = i / pdata->num_irqs; > > > > > ddata->irqs[i].mask = BIT(i % pdata->num_irqs); > > > > > } > > > > > > > > This doesn't look right either. > > > > > > > > First, the loop is never executed if num_irqs is zero. > > > > > > The point of the patch is that 0 entries are never processed. > > So what's the problem? There's no division by zero here. > > And what compiler are you using, Lee? Seems broken. > > > > > Second, the current code looks bogus too as reg_offset is always set to > > > > zero and mask to BIT(i)... > > > > Now the result is correct, since all PMIC irq mask bits are in one > > register now, which means the reg_offset is always 0 can work well. > > But I think the logics still can be improved if our PMIC irq numbers > > are larger than 32 in future. > > The code is still bogus as pointed out above. Why do you bother to > divide by num_irqs at all? Right, no need to divide by num_irqs, can be simplified as below. Lee, care to resend your patch with simplifying the code? Or you want me to send a patch? diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c index 33336cde4724..2ed5f3a4e79c 100644 --- a/drivers/mfd/sprd-sc27xx-spi.c +++ b/drivers/mfd/sprd-sc27xx-spi.c @@ -250,10 +250,8 @@ static int sprd_pmic_probe(struct spi_device *spi) return -ENOMEM; ddata->irq_chip.irqs = ddata->irqs; - for (i = 0; i < pdata->num_irqs; i++) { - ddata->irqs[i].reg_offset = i / pdata->num_irqs; - ddata->irqs[i].mask = BIT(i % pdata->num_irqs); - } + for (i = 0; i < pdata->num_irqs; i++) + ddata->irqs[i].mask = BIT(i); -- Baolin Wang ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask 2020-06-29 15:08 ` Baolin Wang @ 2020-06-29 15:45 ` Lee Jones 0 siblings, 0 replies; 21+ messages in thread From: Lee Jones @ 2020-06-29 15:45 UTC (permalink / raw) To: Baolin Wang Cc: Johan Hovold, Orson Zhai, LKML, linux-arm-kernel, Chunyan Zhang On Mon, 29 Jun 2020, Baolin Wang wrote: > On Mon, Jun 29, 2020 at 10:43 PM Johan Hovold <johan@kernel.org> wrote: > > > > On Mon, Jun 29, 2020 at 10:35:06PM +0800, Baolin Wang wrote: > > > On Mon, Jun 29, 2020 at 10:01 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > > > > > On Mon, 29 Jun 2020, Johan Hovold wrote: > > > > > > > > > On Mon, Jun 29, 2020 at 01:32:14PM +0100, Lee Jones wrote: > > > > > > Since ddata->irqs[] is already zeroed when allocated by devm_kcalloc() and > > > > > > dividing 0 by anything is still 0, there is no need to re-assign > > > > > > ddata->irqs[i].* values. Instead, it should be safe to begin at 1. > > > > > > > > > > > > This fixes the following W=1 warning: > > > > > > > > > > > > drivers/mfd/sprd-sc27xx-spi.c:255 sprd_pmic_probe() debug: sval_binop_unsigned: divide by zero > > > > > > > > > > > > Cc: Orson Zhai <orsonzhai@gmail.com> > > > > > > Cc: Baolin Wang <baolin.wang7@gmail.com> > > > > > > Cc: Chunyan Zhang <zhang.lyra@gmail.com> > > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > > > > > --- > > > > > > drivers/mfd/sprd-sc27xx-spi.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c > > > > > > index c305e941e435c..694a7d429ccff 100644 > > > > > > --- a/drivers/mfd/sprd-sc27xx-spi.c > > > > > > +++ b/drivers/mfd/sprd-sc27xx-spi.c > > > > > > @@ -251,7 +251,7 @@ static int sprd_pmic_probe(struct spi_device *spi) > > > > > > return -ENOMEM; > > > > > > > > > > > > ddata->irq_chip.irqs = ddata->irqs; > > > > > > - for (i = 0; i < pdata->num_irqs; i++) { > > > > > > + for (i = 1; i < pdata->num_irqs; i++) { > > > > > > ddata->irqs[i].reg_offset = i / pdata->num_irqs; > > > > > > ddata->irqs[i].mask = BIT(i % pdata->num_irqs); > > > > > > } > > > > > > > > > > This doesn't look right either. > > > > > > > > > > First, the loop is never executed if num_irqs is zero. > > > > > > > > The point of the patch is that 0 entries are never processed. > > > > So what's the problem? There's no division by zero here. > > > > And what compiler are you using, Lee? Seems broken. > > > > > > > Second, the current code looks bogus too as reg_offset is always set to > > > > > zero and mask to BIT(i)... > > > > > > Now the result is correct, since all PMIC irq mask bits are in one > > > register now, which means the reg_offset is always 0 can work well. > > > But I think the logics still can be improved if our PMIC irq numbers > > > are larger than 32 in future. > > > > The code is still bogus as pointed out above. Why do you bother to > > divide by num_irqs at all? > > Right, no need to divide by num_irqs, can be simplified as below. Lee, > care to resend your patch with simplifying the code? Or you want me to > send a patch? > diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c > index 33336cde4724..2ed5f3a4e79c 100644 > --- a/drivers/mfd/sprd-sc27xx-spi.c > +++ b/drivers/mfd/sprd-sc27xx-spi.c > @@ -250,10 +250,8 @@ static int sprd_pmic_probe(struct spi_device *spi) > return -ENOMEM; > > ddata->irq_chip.irqs = ddata->irqs; > - for (i = 0; i < pdata->num_irqs; i++) { > - ddata->irqs[i].reg_offset = i / pdata->num_irqs; > - ddata->irqs[i].mask = BIT(i % pdata->num_irqs); > - } > + for (i = 0; i < pdata->num_irqs; i++) > + ddata->irqs[i].mask = BIT(i); I'm happy to resend with your Suggested-by. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] mfd: sprd-sc27xx-spi: Fix-up bogus IRQ register offset and mask setting 2020-06-29 12:32 ` [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask Lee Jones 2020-06-29 13:06 ` Johan Hovold @ 2020-07-01 9:15 ` Lee Jones 2020-07-01 14:10 ` Baolin Wang 1 sibling, 1 reply; 21+ messages in thread From: Lee Jones @ 2020-07-01 9:15 UTC (permalink / raw) To: linux-arm-kernel, linux-kernel, Orson Zhai, Baolin Wang, Chunyan Zhang Cc: johan 'i / pdata->num_irqs' always equates to 0 and 'BIT(i % pdata->num_irqs)' always ends up being BIT(i) here, so make that clearer in the code. If the code base needs to support more than 32 IRQs in the future, this will have to be reworked, but lets just keep it simple for as long as we can. This fixes the following W=1 warning: drivers/mfd/sprd-sc27xx-spi.c:255 sprd_pmic_probe() debug: sval_binop_unsigned: divide by zero Cc: Orson Zhai <orsonzhai@gmail.com> Cc: Chunyan Zhang <zhang.lyra@gmail.com> Cc: Johan Hovold <johan@kernel.org> Suggested-by: Baolin Wang <baolin.wang7@gmail.com> Signed-off-by: Lee Jones <lee.jones@linaro.org> diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c index c305e941e435c..4a1a61e1a86ea 100644 --- a/drivers/mfd/sprd-sc27xx-spi.c +++ b/drivers/mfd/sprd-sc27xx-spi.c @@ -251,10 +251,8 @@ static int sprd_pmic_probe(struct spi_device *spi) return -ENOMEM; ddata->irq_chip.irqs = ddata->irqs; - for (i = 0; i < pdata->num_irqs; i++) { - ddata->irqs[i].reg_offset = i / pdata->num_irqs; - ddata->irqs[i].mask = BIT(i % pdata->num_irqs); - } + for (i = 0; i < pdata->num_irqs; i++) + ddata->irqs[i].mask = BIT(i); ret = devm_regmap_add_irq_chip(&spi->dev, ddata->regmap, ddata->irq, IRQF_ONESHOT | IRQF_NO_SUSPEND, 0, ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] mfd: sprd-sc27xx-spi: Fix-up bogus IRQ register offset and mask setting 2020-07-01 9:15 ` [PATCH] mfd: sprd-sc27xx-spi: Fix-up bogus IRQ register offset and mask setting Lee Jones @ 2020-07-01 14:10 ` Baolin Wang 0 siblings, 0 replies; 21+ messages in thread From: Baolin Wang @ 2020-07-01 14:10 UTC (permalink / raw) To: Lee Jones; +Cc: linux-arm-kernel, LKML, Orson Zhai, Chunyan Zhang, Johan Hovold On Wed, Jul 1, 2020 at 5:15 PM Lee Jones <lee.jones@linaro.org> wrote: > > 'i / pdata->num_irqs' always equates to 0 and 'BIT(i % pdata->num_irqs)' > always ends up being BIT(i) here, so make that clearer in the code. If > the code base needs to support more than 32 IRQs in the future, this will > have to be reworked, but lets just keep it simple for as long as we can. > > This fixes the following W=1 warning: > > drivers/mfd/sprd-sc27xx-spi.c:255 sprd_pmic_probe() debug: sval_binop_unsigned: divide by zero > > Cc: Orson Zhai <orsonzhai@gmail.com> > Cc: Chunyan Zhang <zhang.lyra@gmail.com> > Cc: Johan Hovold <johan@kernel.org> > Suggested-by: Baolin Wang <baolin.wang7@gmail.com> > Signed-off-by: Lee Jones <lee.jones@linaro.org> Thanks. Reviewed-by: Baolin Wang <baolin.wang7@gmail.com> > > diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c > index c305e941e435c..4a1a61e1a86ea 100644 > --- a/drivers/mfd/sprd-sc27xx-spi.c > +++ b/drivers/mfd/sprd-sc27xx-spi.c > @@ -251,10 +251,8 @@ static int sprd_pmic_probe(struct spi_device *spi) > return -ENOMEM; > > ddata->irq_chip.irqs = ddata->irqs; > - for (i = 0; i < pdata->num_irqs; i++) { > - ddata->irqs[i].reg_offset = i / pdata->num_irqs; > - ddata->irqs[i].mask = BIT(i % pdata->num_irqs); > - } > + for (i = 0; i < pdata->num_irqs; i++) > + ddata->irqs[i].mask = BIT(i); > > ret = devm_regmap_add_irq_chip(&spi->dev, ddata->regmap, ddata->irq, > IRQF_ONESHOT | IRQF_NO_SUSPEND, 0, -- Baolin Wang ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 5/5] mfd: axp20x-i2c: Do not define 'struct acpi_device_id' when !CONFIG_ACPI 2020-06-29 12:32 [PATCH 0/5] Last batch of W=1 warning fixes in MFD Lee Jones ` (3 preceding siblings ...) 2020-06-29 12:32 ` [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask Lee Jones @ 2020-06-29 12:32 ` Lee Jones 2020-06-29 15:38 ` Chen-Yu Tsai 2020-07-01 6:59 ` [PATCH v2] mfd: axp20x-i2c: Tell the compiler that ACPI functions may not be used Lee Jones 4 siblings, 2 replies; 21+ messages in thread From: Lee Jones @ 2020-06-29 12:32 UTC (permalink / raw) To: lee.jones; +Cc: linux-arm-kernel, linux-kernel, Chen-Yu Tsai, Carlo Caione Since ACPI_PTR() is used to NULLify the value when !CONFIG_ACPI, struct axp20x_i2c_acpi_match becomes defined by unused. This squashes the current W=1 kernel builds warning: drivers/mfd/axp20x-i2c.c:82:36: warning: ‘axp20x_i2c_acpi_match’ defined but not used [-Wunused-const-variable=] Cc: Chen-Yu Tsai <wens@csie.org> Cc: Carlo Caione <carlo@caione.org> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/mfd/axp20x-i2c.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c index 14f9df74f855c..3dd650125c239 100644 --- a/drivers/mfd/axp20x-i2c.c +++ b/drivers/mfd/axp20x-i2c.c @@ -79,6 +79,7 @@ static const struct i2c_device_id axp20x_i2c_id[] = { }; MODULE_DEVICE_TABLE(i2c, axp20x_i2c_id); +#if IS_ENABLED(CONFIG_ACPI) static const struct acpi_device_id axp20x_i2c_acpi_match[] = { { .id = "INT33F4", @@ -87,6 +88,7 @@ static const struct acpi_device_id axp20x_i2c_acpi_match[] = { { }, }; MODULE_DEVICE_TABLE(acpi, axp20x_i2c_acpi_match); +#endif static struct i2c_driver axp20x_i2c_driver = { .driver = { -- 2.25.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 5/5] mfd: axp20x-i2c: Do not define 'struct acpi_device_id' when !CONFIG_ACPI 2020-06-29 12:32 ` [PATCH 5/5] mfd: axp20x-i2c: Do not define 'struct acpi_device_id' when !CONFIG_ACPI Lee Jones @ 2020-06-29 15:38 ` Chen-Yu Tsai 2020-07-06 7:31 ` Lee Jones 2020-07-01 6:59 ` [PATCH v2] mfd: axp20x-i2c: Tell the compiler that ACPI functions may not be used Lee Jones 1 sibling, 1 reply; 21+ messages in thread From: Chen-Yu Tsai @ 2020-06-29 15:38 UTC (permalink / raw) To: Lee Jones; +Cc: linux-arm-kernel, linux-kernel, Carlo Caione Adding Hans to the list as he's the one that deals with all the x86 platforms that use this series of chips. On Mon, Jun 29, 2020 at 8:32 PM Lee Jones <lee.jones@linaro.org> wrote: > > Since ACPI_PTR() is used to NULLify the value when !CONFIG_ACPI, > struct axp20x_i2c_acpi_match becomes defined by unused. > > This squashes the current W=1 kernel builds warning: > > drivers/mfd/axp20x-i2c.c:82:36: warning: ‘axp20x_i2c_acpi_match’ defined but not used [-Wunused-const-variable=] > > Cc: Chen-Yu Tsai <wens@csie.org> > Cc: Carlo Caione <carlo@caione.org> > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/mfd/axp20x-i2c.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c > index 14f9df74f855c..3dd650125c239 100644 > --- a/drivers/mfd/axp20x-i2c.c > +++ b/drivers/mfd/axp20x-i2c.c > @@ -79,6 +79,7 @@ static const struct i2c_device_id axp20x_i2c_id[] = { > }; > MODULE_DEVICE_TABLE(i2c, axp20x_i2c_id); > > +#if IS_ENABLED(CONFIG_ACPI) > static const struct acpi_device_id axp20x_i2c_acpi_match[] = { I'd rather use "__maybe_unused" if possible to at least get a compile check, and also because "ACPI_PTR NULLifies the value" might not be well known to people not working on ACPI-based platforms. Either way, Acked-by: Chen-Yu Tsai <wens@csie.org> > { > .id = "INT33F4", > @@ -87,6 +88,7 @@ static const struct acpi_device_id axp20x_i2c_acpi_match[] = { > { }, > }; > MODULE_DEVICE_TABLE(acpi, axp20x_i2c_acpi_match); > +#endif > > static struct i2c_driver axp20x_i2c_driver = { > .driver = { > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/5] mfd: axp20x-i2c: Do not define 'struct acpi_device_id' when !CONFIG_ACPI 2020-06-29 15:38 ` Chen-Yu Tsai @ 2020-07-06 7:31 ` Lee Jones 0 siblings, 0 replies; 21+ messages in thread From: Lee Jones @ 2020-07-06 7:31 UTC (permalink / raw) To: Chen-Yu Tsai; +Cc: linux-arm-kernel, linux-kernel, Carlo Caione On Mon, 29 Jun 2020, Chen-Yu Tsai wrote: > Adding Hans to the list as he's the one that deals with all the x86 > platforms that use this series of chips. > > On Mon, Jun 29, 2020 at 8:32 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > Since ACPI_PTR() is used to NULLify the value when !CONFIG_ACPI, > > struct axp20x_i2c_acpi_match becomes defined by unused. > > > > This squashes the current W=1 kernel builds warning: > > > > drivers/mfd/axp20x-i2c.c:82:36: warning: ‘axp20x_i2c_acpi_match’ defined but not used [-Wunused-const-variable=] > > > > Cc: Chen-Yu Tsai <wens@csie.org> > > Cc: Carlo Caione <carlo@caione.org> > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > --- > > drivers/mfd/axp20x-i2c.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c > > index 14f9df74f855c..3dd650125c239 100644 > > --- a/drivers/mfd/axp20x-i2c.c > > +++ b/drivers/mfd/axp20x-i2c.c > > @@ -79,6 +79,7 @@ static const struct i2c_device_id axp20x_i2c_id[] = { > > }; > > MODULE_DEVICE_TABLE(i2c, axp20x_i2c_id); > > > > +#if IS_ENABLED(CONFIG_ACPI) > > static const struct acpi_device_id axp20x_i2c_acpi_match[] = { > > I'd rather use "__maybe_unused" if possible to at least get a compile > check, and also because "ACPI_PTR NULLifies the value" might not be > well known to people not working on ACPI-based platforms. FYI, I've gone back to this patch, as it seems to be the common census across the rest of the kernel, due to the extra space saving. The code will get compile checks when compiled with different architectures, and I'm not sure I understand the last point you made. > Either way, > > Acked-by: Chen-Yu Tsai <wens@csie.org> I've applied this to the patch. Thanks. > > { > > .id = "INT33F4", > > @@ -87,6 +88,7 @@ static const struct acpi_device_id axp20x_i2c_acpi_match[] = { > > { }, > > }; > > MODULE_DEVICE_TABLE(acpi, axp20x_i2c_acpi_match); > > +#endif > > > > static struct i2c_driver axp20x_i2c_driver = { > > .driver = { > > -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] mfd: axp20x-i2c: Tell the compiler that ACPI functions may not be used 2020-06-29 12:32 ` [PATCH 5/5] mfd: axp20x-i2c: Do not define 'struct acpi_device_id' when !CONFIG_ACPI Lee Jones 2020-06-29 15:38 ` Chen-Yu Tsai @ 2020-07-01 6:59 ` Lee Jones 2020-07-01 8:38 ` Chen-Yu Tsai 1 sibling, 1 reply; 21+ messages in thread From: Lee Jones @ 2020-07-01 6:59 UTC (permalink / raw) To: linux-arm-kernel, linux-kernel, Chen-Yu Tsai, Carlo Caione ... as is the case when !CONFIG_ACPI. This squashes the current W=1 kernel builds warning: drivers/mfd/axp20x-i2c.c:82:36: warning: ‘axp20x_i2c_acpi_match’ defined but not used [-Wunused-const-variable=] Cc: Chen-Yu Tsai <wens@csie.org> Cc: Carlo Caione <carlo@caione.org> Signed-off-by: Lee Jones <lee.jones@linaro.org> diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c index 14f9df74f855c..7d08c06de7afc 100644 --- a/drivers/mfd/axp20x-i2c.c +++ b/drivers/mfd/axp20x-i2c.c @@ -79,7 +79,7 @@ static const struct i2c_device_id axp20x_i2c_id[] = { }; MODULE_DEVICE_TABLE(i2c, axp20x_i2c_id); -static const struct acpi_device_id axp20x_i2c_acpi_match[] = { +static const struct acpi_device_id __maybe_unused axp20x_i2c_acpi_match[] = { { .id = "INT33F4", .driver_data = AXP288_ID, ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2] mfd: axp20x-i2c: Tell the compiler that ACPI functions may not be used 2020-07-01 6:59 ` [PATCH v2] mfd: axp20x-i2c: Tell the compiler that ACPI functions may not be used Lee Jones @ 2020-07-01 8:38 ` Chen-Yu Tsai 0 siblings, 0 replies; 21+ messages in thread From: Chen-Yu Tsai @ 2020-07-01 8:38 UTC (permalink / raw) To: Lee Jones; +Cc: linux-arm-kernel, linux-kernel, Carlo Caione On Wed, Jul 1, 2020 at 3:00 PM Lee Jones <lee.jones@linaro.org> wrote: > > ... as is the case when !CONFIG_ACPI. > > This squashes the current W=1 kernel builds warning: > > drivers/mfd/axp20x-i2c.c:82:36: warning: ‘axp20x_i2c_acpi_match’ defined but not used [-Wunused-const-variable=] > > Cc: Chen-Yu Tsai <wens@csie.org> > Cc: Carlo Caione <carlo@caione.org> > Signed-off-by: Lee Jones <lee.jones@linaro.org> Acked-by: Chen-Yu Tsai <wens@csie.org> > > diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c > index 14f9df74f855c..7d08c06de7afc 100644 > --- a/drivers/mfd/axp20x-i2c.c > +++ b/drivers/mfd/axp20x-i2c.c > @@ -79,7 +79,7 @@ static const struct i2c_device_id axp20x_i2c_id[] = { > }; > MODULE_DEVICE_TABLE(i2c, axp20x_i2c_id); > > -static const struct acpi_device_id axp20x_i2c_acpi_match[] = { > +static const struct acpi_device_id __maybe_unused axp20x_i2c_acpi_match[] = { > { > .id = "INT33F4", > .driver_data = AXP288_ID, ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2020-07-06 7:31 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-29 12:32 [PATCH 0/5] Last batch of W=1 warning fixes in MFD Lee Jones 2020-06-29 12:32 ` [PATCH 1/5] mfd: si476x-cmd: Add missing documentation for si476x_cmd_fm_rds_status()'s arg 'report' Lee Jones 2020-06-29 12:32 ` [PATCH 2/5] mfd: lm3533-ctrlbank: Cap BRIGHTNESS_MAX to 127 since API uses u8 as carrier Lee Jones 2020-06-29 12:51 ` Johan Hovold 2020-06-29 13:25 ` Lee Jones 2020-06-30 8:38 ` Johan Hovold 2020-06-29 12:32 ` [PATCH 3/5] mfd: rave-sp: Fix mistake in 'struct rave_sp_deframer's kerneldoc Lee Jones 2020-06-29 12:32 ` [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask Lee Jones 2020-06-29 13:06 ` Johan Hovold 2020-06-29 14:01 ` Lee Jones 2020-06-29 14:35 ` Baolin Wang 2020-06-29 14:43 ` Johan Hovold 2020-06-29 15:08 ` Baolin Wang 2020-06-29 15:45 ` Lee Jones 2020-07-01 9:15 ` [PATCH] mfd: sprd-sc27xx-spi: Fix-up bogus IRQ register offset and mask setting Lee Jones 2020-07-01 14:10 ` Baolin Wang 2020-06-29 12:32 ` [PATCH 5/5] mfd: axp20x-i2c: Do not define 'struct acpi_device_id' when !CONFIG_ACPI Lee Jones 2020-06-29 15:38 ` Chen-Yu Tsai 2020-07-06 7:31 ` Lee Jones 2020-07-01 6:59 ` [PATCH v2] mfd: axp20x-i2c: Tell the compiler that ACPI functions may not be used Lee Jones 2020-07-01 8:38 ` Chen-Yu Tsai
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).