| Message ID | 20220613043002.28152-1-shubhrajyoti.datta@xilinx.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] i2c-xiic: Fix the type check for xiic_wakeup | expand |
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.
[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.
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
> 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.
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
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
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
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(-)