[PATCH 0/5] Last batch of W=1 warning fixes in MFD

[PATCH 0/5] Last batch of W=1 warning fixes in MFD

[Lee Jones]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/5] mfd: si476x-cmd: Add missing documentation for si476x_cmd_fm_rds_status()'s arg 'report'

[Lee Jones]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/5] mfd: lm3533-ctrlbank: Cap BRIGHTNESS_MAX to 127 since API uses u8 as carrier

[Lee Jones]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] mfd: lm3533-ctrlbank: Cap BRIGHTNESS_MAX to 127 since API uses u8 as carrier

[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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] mfd: lm3533-ctrlbank: Cap BRIGHTNESS_MAX to 127 since API uses u8 as carrier

[Lee Jones]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] mfd: lm3533-ctrlbank: Cap BRIGHTNESS_MAX to 127 since API uses u8 as carrier

[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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/5] mfd: rave-sp: Fix mistake in 'struct rave_sp_deframer's kerneldoc

[Lee Jones]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask

[Lee Jones]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask

[Johan Hovold]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask

[Lee Jones]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask

[Baolin Wang]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask

[Johan Hovold]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask

[Baolin Wang]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] mfd: sprd-sc27xx-spi: Fix divide by zero when allocating register offset/mask

[Lee Jones]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] mfd: sprd-sc27xx-spi: Fix-up bogus IRQ register offset and mask setting

[Lee Jones]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] mfd: sprd-sc27xx-spi: Fix-up bogus IRQ register offset and mask setting

[Baolin Wang]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/5] mfd: axp20x-i2c: Do not define 'struct acpi_device_id' when !CONFIG_ACPI

[Lee Jones]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] mfd: axp20x-i2c: Do not define 'struct acpi_device_id' when !CONFIG_ACPI

[Chen-Yu Tsai]

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
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] mfd: axp20x-i2c: Do not define 'struct acpi_device_id' when !CONFIG_ACPI

[Lee Jones]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2] mfd: axp20x-i2c: Tell the compiler that ACPI functions may not be used

[Lee Jones]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] mfd: axp20x-i2c: Tell the compiler that ACPI functions may not be used

[Chen-Yu Tsai]

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,

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel