[PATCH v2] i2c-xiic: Fix the type check for xiic_wakeup

[PATCH v2] i2c-xiic: Fix the type check for xiic_wakeup

[Shubhrajyoti Datta]

Fix the coverity warning
mixed_enum_type: enumerated type mixed with another type

We are passing an enum in the xiic_wakeup lets change
the function parameters to reflect that.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
---
v2:
Update the wakeup_code to enum

 drivers/i2c/busses/i2c-xiic.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 9a1c3f8b7048..ec56b80653d3 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -34,9 +34,9 @@
 #define DRIVER_NAME "xiic-i2c"
 
 enum xilinx_i2c_state {
-	STATE_DONE,
-	STATE_ERROR,
-	STATE_START
+	STATE_DONE = 0,
+	STATE_ERROR = 1,
+	STATE_START = 2
 };
 
 enum xiic_endian {
@@ -367,7 +367,7 @@ static void xiic_fill_tx_fifo(struct xiic_i2c *i2c)
 	}
 }
 
-static void xiic_wakeup(struct xiic_i2c *i2c, int code)
+static void xiic_wakeup(struct xiic_i2c *i2c, enum xilinx_i2c_state code)
 {
 	i2c->tx_msg = NULL;
 	i2c->rx_msg = NULL;
@@ -383,7 +383,7 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 	u32 clr = 0;
 	int xfer_more = 0;
 	int wakeup_req = 0;
-	int wakeup_code = 0;
+	enum xilinx_i2c_state wakeup_code = STATE_DONE;
 	int ret;
 
 	/* Get the interrupt Status from the IPIF. There is no clearing of
-- 
2.17.1

Re: [PATCH v2] i2c-xiic: Fix the type check for xiic_wakeup

[Michal Simek]

On 6/9/22 17:37, Shubhrajyoti Datta wrote:
> Fix the coverity warning
> mixed_enum_type: enumerated type mixed with another type
> 
> We are passing an enum in the xiic_wakeup lets change
> the function parameters to reflect that.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> ---
> v2:
> Update the wakeup_code to enum
> 
>   drivers/i2c/busses/i2c-xiic.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> index 9a1c3f8b7048..ec56b80653d3 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -34,9 +34,9 @@
>   #define DRIVER_NAME "xiic-i2c"
>   
>   enum xilinx_i2c_state {
> -	STATE_DONE,
> -	STATE_ERROR,
> -	STATE_START
> +	STATE_DONE = 0,
> +	STATE_ERROR = 1,
> +	STATE_START = 2
>   };
>   
>   enum xiic_endian {
> @@ -367,7 +367,7 @@ static void xiic_fill_tx_fifo(struct xiic_i2c *i2c)
>   	}
>   }
>   
> -static void xiic_wakeup(struct xiic_i2c *i2c, int code)
> +static void xiic_wakeup(struct xiic_i2c *i2c, enum xilinx_i2c_state code)
>   {
>   	i2c->tx_msg = NULL;
>   	i2c->rx_msg = NULL;
> @@ -383,7 +383,7 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
>   	u32 clr = 0;
>   	int xfer_more = 0;
>   	int wakeup_req = 0;
> -	int wakeup_code = 0;
> +	enum xilinx_i2c_state wakeup_code = STATE_DONE;
>   	int ret;
>   
>   	/* Get the interrupt Status from the IPIF. There is no clearing of


Acked-by: Michal Simek <michal.simek@amd.com>

Thanks,
Michal

Re: [PATCH v2] i2c-xiic: Fix the type check for xiic_wakeup

[Michal Simek]

On 6/14/22 10:11, Wolfram Sang wrote:
> 
>> Actually this was recommended by Greg in one of our thread here.
>> https://lore.kernel.org/all/20200318125003.GA2727094@kroah.com/
> 
> It is in the C standard, so any compiler not adhering to it has a bug.
> It is especially not important here because we use the enums only
> locally and do not export.
> 
>> That's why we started to initialize all values in enums in our code.
>> It shouldn't be really any problem to do so.
> 
> It may be more readable if you have dozens of them, but it also adds the
> possibility of copy&paste errors.
> 
> I think I'll take V3 here.

Ok. Go for it.

Thanks,
Michal

Re: [PATCH v2] i2c-xiic: Fix the type check for xiic_wakeup

[Greg Kroah-Hartman]

On Tue, Jun 14, 2022 at 10:11:40AM +0200, Wolfram Sang wrote:
> 
> > Actually this was recommended by Greg in one of our thread here.
> > https://lore.kernel.org/all/20200318125003.GA2727094@kroah.com/
> 
> It is in the C standard, so any compiler not adhering to it has a bug.

Possibly, yes, but for enums that are part of the UAPI, we should
explicitly set them to be sure as we do not control what userspace
compilers are ever used.

Also when dealing with values that are reflected in hardware, it's good
to be explicit to help document the interface as well.

> It is especially not important here because we use the enums only
> locally and do not export.

Local stuff is fine, no need to set as there's no abi issues here.

thanks,

greg k-h

Re: [PATCH v2] i2c-xiic: Fix the type check for xiic_wakeup

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 555 bytes --]


> Actually this was recommended by Greg in one of our thread here.
> https://lore.kernel.org/all/20200318125003.GA2727094@kroah.com/

It is in the C standard, so any compiler not adhering to it has a bug.
It is especially not important here because we use the enums only
locally and do not export.

> That's why we started to initialize all values in enums in our code.
> It shouldn't be really any problem to do so.

It may be more readable if you have dozens of them, but it also adds the
possibility of copy&paste errors.

I think I'll take V3 here.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] i2c-xiic: Fix the type check for xiic_wakeup

[Michal Simek]

Hi Wolfram,

On 6/13/22 16:54, Wolfram Sang wrote:
> On Mon, Jun 13, 2022 at 10:00:02AM +0530, Shubhrajyoti Datta wrote:
>> Fix the coverity warning
>> mixed_enum_type: enumerated type mixed with another type
>>
>> We are passing an enum in the xiic_wakeup lets change
>> the function parameters to reflect that.
>>
>> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> 
> What is the difference to the V2 you sent 4 days earlier?
> 
>>   enum xilinx_i2c_state {
>> -	STATE_DONE,
>> -	STATE_ERROR,
>> -	STATE_START
>> +	STATE_DONE = 0,
>> +	STATE_ERROR = 1,
>> +	STATE_START = 2
> 
> No need for initializers.

Actually this was recommended by Greg in one of our thread here.
https://lore.kernel.org/all/20200318125003.GA2727094@kroah.com/

That's why we started to initialize all values in enums in our code.
It shouldn't be really any problem to do so.

Thanks,
Michal

RE: [PATCH v2] i2c-xiic: Fix the type check for xiic_wakeup

[Shubhrajyoti Datta]

[AMD Official Use Only - General]


Hi ,
Thanks for the review 

> -----Original Message-----
> From: Wolfram Sang <wsa@kernel.org>
> Sent: Monday, June 13, 2022 8:25 PM
> To: Shubhrajyoti Datta <shubhraj@xilinx.com>
> Cc: linux-i2c@vger.kernel.org; Michal Simek <michals@xilinx.com>; git 
> <git@xilinx.com>
> Subject: Re: [PATCH v2] i2c-xiic: Fix the type check for xiic_wakeup
> 
> On Mon, Jun 13, 2022 at 10:00:02AM +0530, Shubhrajyoti Datta wrote:
> > Fix the coverity warning
> > mixed_enum_type: enumerated type mixed with another type
> >
> > We are passing an enum in the xiic_wakeup lets change the function 
> > parameters to reflect that.
> >
> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> 
> What is the difference to the V2 you sent 4 days earlier?

Some issue with my mailer was not able to see that mail.
> 
> >  enum xilinx_i2c_state {
> > -	STATE_DONE,
> > -	STATE_ERROR,
> > -	STATE_START
> > +	STATE_DONE = 0,
> > +	STATE_ERROR = 1,
> > +	STATE_START = 2
> 
> No need for initializers.
Fixed it in v3.

Re: [PATCH v2] i2c-xiic: Fix the type check for xiic_wakeup

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

On Mon, Jun 13, 2022 at 10:00:02AM +0530, Shubhrajyoti Datta wrote:
> Fix the coverity warning
> mixed_enum_type: enumerated type mixed with another type
> 
> We are passing an enum in the xiic_wakeup lets change
> the function parameters to reflect that.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>

What is the difference to the V2 you sent 4 days earlier?

>  enum xilinx_i2c_state {
> -	STATE_DONE,
> -	STATE_ERROR,
> -	STATE_START
> +	STATE_DONE = 0,
> +	STATE_ERROR = 1,
> +	STATE_START = 2

No need for initializers.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2] i2c-xiic: Fix the type check for xiic_wakeup

[Shubhrajyoti Datta]

Fix the coverity warning
mixed_enum_type: enumerated type mixed with another type

We are passing an enum in the xiic_wakeup lets change
the function parameters to reflect that.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
---
v2:
Update the wakeup_code to enum

 drivers/i2c/busses/i2c-xiic.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 9a1c3f8b7048..ec56b80653d3 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -34,9 +34,9 @@
 #define DRIVER_NAME "xiic-i2c"
 
 enum xilinx_i2c_state {
-	STATE_DONE,
-	STATE_ERROR,
-	STATE_START
+	STATE_DONE = 0,
+	STATE_ERROR = 1,
+	STATE_START = 2
 };
 
 enum xiic_endian {
@@ -367,7 +367,7 @@ static void xiic_fill_tx_fifo(struct xiic_i2c *i2c)
 	}
 }
 
-static void xiic_wakeup(struct xiic_i2c *i2c, int code)
+static void xiic_wakeup(struct xiic_i2c *i2c, enum xilinx_i2c_state code)
 {
 	i2c->tx_msg = NULL;
 	i2c->rx_msg = NULL;
@@ -383,7 +383,7 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 	u32 clr = 0;
 	int xfer_more = 0;
 	int wakeup_req = 0;
-	int wakeup_code = 0;
+	enum xilinx_i2c_state wakeup_code = STATE_DONE;
 	int ret;
 
 	/* Get the interrupt Status from the IPIF. There is no clearing of
-- 
2.17.1