Re: [PATCH 1/6] cx18: Fix the handling of i2c bus registration error

2009-04-07 Thread Andy Walls 
On Tue, 2009-04-07 at 11:31 +0200, Jean Delvare wrote:
 On Sat, 04 Apr 2009 08:46:00 -0400, Andy Walls wrote:
  On Sat, 2009-04-04 at 14:26 +0200, Jean Delvare wrote:
   * Return actual error values as returned by the i2c subsystem, rather
 than 0 or 1.
   * If the registration of the second bus fails, unregister the first one
 before exiting, otherwise we are leaking resources.
   
   Signed-off-by: Jean Delvare kh...@linux-fr.org
   Cc: Hans Verkuil hverk...@xs4all.nl
   Cc: Andy Walls awa...@radix.net
   ---
linux/drivers/media/video/cx18/cx18-i2c.c |   16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
   
   --- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c
   2009-03-01 16:09:09.0 +0100
   +++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c 2009-04-03 
   18:45:18.0 +0200
   @@ -214,7 +214,7 @@ static struct i2c_algo_bit_data cx18_i2c
/* init + register i2c algo-bit adapter */
int init_cx18_i2c(struct cx18 *cx)
{
   - int i;
   + int i, err;
 CX18_DEBUG_I2C(i2c init\n);

 for (i = 0; i  2; i++) {
   @@ -273,8 +273,18 @@ int init_cx18_i2c(struct cx18 *cx)
 cx18_call_hw(cx, CX18_HW_GPIO_RESET_CTRL,
  core, reset, (u32) CX18_GPIO_RESET_I2C);

   - return i2c_bit_add_bus(cx-i2c_adap[0]) ||
   - i2c_bit_add_bus(cx-i2c_adap[1]);
   + err = i2c_bit_add_bus(cx-i2c_adap[0]);
   + if (err)
   + goto err;
   + err = i2c_bit_add_bus(cx-i2c_adap[1]);
   + if (err)
   + goto err_del_bus_0;
   + return 0;
   +
   + err_del_bus_0:
   + i2c_del_adapter(cx-i2c_adap[0]);
   + err:
   + return err;
}

void exit_cx18_i2c(struct cx18 *cx)
  
  Reviewed-by: Andy Walls awa...@radix.net

If it matters:

Acked-by: Andy Walls awa...@radix.net

Regards,
Andy

 [Context edited for clarity.]
 
 Mauro, can you please apply this patch now? It is a bug fix not
 directly related to my ir-kbd-i2c work, it would be nice to have it
 applied quickly so that I don't have to carry the patch around.
 
 Thanks,

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] cx18: Fix the handling of i2c bus registration error

2009-04-07 Thread Jean Delvare 
On Sat, 04 Apr 2009 08:46:00 -0400, Andy Walls wrote:
 On Sat, 2009-04-04 at 14:26 +0200, Jean Delvare wrote:
  * Return actual error values as returned by the i2c subsystem, rather
than 0 or 1.
  * If the registration of the second bus fails, unregister the first one
before exiting, otherwise we are leaking resources.
  
  Signed-off-by: Jean Delvare kh...@linux-fr.org
  Cc: Hans Verkuil hverk...@xs4all.nl
  Cc: Andy Walls awa...@radix.net
  ---
   linux/drivers/media/video/cx18/cx18-i2c.c |   16 +---
   1 file changed, 13 insertions(+), 3 deletions(-)
  
  --- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c  2009-03-01 
  16:09:09.0 +0100
  +++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c   2009-04-03 
  18:45:18.0 +0200
  @@ -214,7 +214,7 @@ static struct i2c_algo_bit_data cx18_i2c
   /* init + register i2c algo-bit adapter */
   int init_cx18_i2c(struct cx18 *cx)
   {
  -   int i;
  +   int i, err;
  CX18_DEBUG_I2C(i2c init\n);
   
  for (i = 0; i  2; i++) {
  @@ -273,8 +273,18 @@ int init_cx18_i2c(struct cx18 *cx)
  cx18_call_hw(cx, CX18_HW_GPIO_RESET_CTRL,
   core, reset, (u32) CX18_GPIO_RESET_I2C);
   
  -   return i2c_bit_add_bus(cx-i2c_adap[0]) ||
  -   i2c_bit_add_bus(cx-i2c_adap[1]);
  +   err = i2c_bit_add_bus(cx-i2c_adap[0]);
  +   if (err)
  +   goto err;
  +   err = i2c_bit_add_bus(cx-i2c_adap[1]);
  +   if (err)
  +   goto err_del_bus_0;
  +   return 0;
  +
  + err_del_bus_0:
  +   i2c_del_adapter(cx-i2c_adap[0]);
  + err:
  +   return err;
   }
   
   void exit_cx18_i2c(struct cx18 *cx)
 
 Reviewed-by: Andy Walls awa...@radix.net

[Context edited for clarity.]

Mauro, can you please apply this patch now? It is a bug fix not
directly related to my ir-kbd-i2c work, it would be nice to have it
applied quickly so that I don't have to carry the patch around.

Thanks,
-- 
Jean Delvare
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/6] cx18: Fix the handling of i2c bus registration error

2009-04-04 Thread Jean Delvare 
* Return actual error values as returned by the i2c subsystem, rather
  than 0 or 1.
* If the registration of the second bus fails, unregister the first one
  before exiting, otherwise we are leaking resources.

Signed-off-by: Jean Delvare kh...@linux-fr.org
Cc: Hans Verkuil hverk...@xs4all.nl
Cc: Andy Walls awa...@radix.net
---
 linux/drivers/media/video/cx18/cx18-i2c.c |   16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

--- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c  2009-03-01 
16:09:09.0 +0100
+++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c   2009-04-03 
18:45:18.0 +0200
@@ -214,7 +214,7 @@ static struct i2c_algo_bit_data cx18_i2c
 /* init + register i2c algo-bit adapter */
 int init_cx18_i2c(struct cx18 *cx)
 {
-   int i;
+   int i, err;
CX18_DEBUG_I2C(i2c init\n);
 
for (i = 0; i  2; i++) {
@@ -273,8 +273,18 @@ int init_cx18_i2c(struct cx18 *cx)
cx18_call_hw(cx, CX18_HW_GPIO_RESET_CTRL,
 core, reset, (u32) CX18_GPIO_RESET_I2C);
 
-   return i2c_bit_add_bus(cx-i2c_adap[0]) ||
-   i2c_bit_add_bus(cx-i2c_adap[1]);
+   err = i2c_bit_add_bus(cx-i2c_adap[0]);
+   if (err)
+   goto err;
+   err = i2c_bit_add_bus(cx-i2c_adap[1]);
+   if (err)
+   goto err_del_bus_0;
+   return 0;
+
+ err_del_bus_0:
+   i2c_del_adapter(cx-i2c_adap[0]);
+ err:
+   return err;
 }
 
 void exit_cx18_i2c(struct cx18 *cx)

-- 
Jean Delvare
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] cx18: Fix the handling of i2c bus registration error

2009-04-04 Thread Andy Walls 
On Sat, 2009-04-04 at 14:26 +0200, Jean Delvare wrote:
 * Return actual error values as returned by the i2c subsystem, rather
   than 0 or 1.
 * If the registration of the second bus fails, unregister the first one
   before exiting, otherwise we are leaking resources.
 
 Signed-off-by: Jean Delvare kh...@linux-fr.org
 Cc: Hans Verkuil hverk...@xs4all.nl
 Cc: Andy Walls awa...@radix.net

Jean,

Thanks for noticing this one and providing a patch.  I have one comment
below...


 ---
  linux/drivers/media/video/cx18/cx18-i2c.c |   16 +---
  1 file changed, 13 insertions(+), 3 deletions(-)
 
 --- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c2009-03-01 
 16:09:09.0 +0100
 +++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c 2009-04-03 
 18:45:18.0 +0200
 @@ -214,7 +214,7 @@ static struct i2c_algo_bit_data cx18_i2c
  /* init + register i2c algo-bit adapter */
  int init_cx18_i2c(struct cx18 *cx)
  {
 - int i;
 + int i, err;
   CX18_DEBUG_I2C(i2c init\n);
  
   for (i = 0; i  2; i++) {
 @@ -273,8 +273,18 @@ int init_cx18_i2c(struct cx18 *cx)
   cx18_call_hw(cx, CX18_HW_GPIO_RESET_CTRL,
core, reset, (u32) CX18_GPIO_RESET_I2C);
  
 - return i2c_bit_add_bus(cx-i2c_adap[0]) ||
 - i2c_bit_add_bus(cx-i2c_adap[1]);
 + err = i2c_bit_add_bus(cx-i2c_adap[0]);

if (err)
return err;
err = i2c_bit_add_bus(cx-i2c_adap[1]);
if (err)
i2c_del_adapter(cx-i2c_adap[0]);
return err;

This sequence saves a few lines of code and gets rid of the goto's
compared to what you proposed below.


 + if (err)
 + goto err;
 + err = i2c_bit_add_bus(cx-i2c_adap[1]);
 + if (err)
 + goto err_del_bus_0;
 + return 0;
 +
 + err_del_bus_0:
 + i2c_del_adapter(cx-i2c_adap[0]);
 + err:
 + return err;
  }
  
  void exit_cx18_i2c(struct cx18 *cx)

Reviewed-by: Andy Walls awa...@radix.net

Regards,
Andy


--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] cx18: Fix the handling of i2c bus registration error

2009-04-04 Thread Jean Delvare 
Hi Andy,

Thanks for the fast review.

On Sat, 04 Apr 2009 08:46:00 -0400, Andy Walls wrote:
 On Sat, 2009-04-04 at 14:26 +0200, Jean Delvare wrote:
  * Return actual error values as returned by the i2c subsystem, rather
than 0 or 1.
  * If the registration of the second bus fails, unregister the first one
before exiting, otherwise we are leaking resources.
  
  Signed-off-by: Jean Delvare kh...@linux-fr.org
  Cc: Hans Verkuil hverk...@xs4all.nl
  Cc: Andy Walls awa...@radix.net
 
 Jean,
 
 Thanks for noticing this one and providing a patch.  I have one comment
 below...
 
 
  ---
   linux/drivers/media/video/cx18/cx18-i2c.c |   16 +---
   1 file changed, 13 insertions(+), 3 deletions(-)
  
  --- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c  2009-03-01 
  16:09:09.0 +0100
  +++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c   2009-04-03 
  18:45:18.0 +0200
  @@ -214,7 +214,7 @@ static struct i2c_algo_bit_data cx18_i2c
   /* init + register i2c algo-bit adapter */
   int init_cx18_i2c(struct cx18 *cx)
   {
  -   int i;
  +   int i, err;
  CX18_DEBUG_I2C(i2c init\n);
   
  for (i = 0; i  2; i++) {
  @@ -273,8 +273,18 @@ int init_cx18_i2c(struct cx18 *cx)
  cx18_call_hw(cx, CX18_HW_GPIO_RESET_CTRL,
   core, reset, (u32) CX18_GPIO_RESET_I2C);
   
  -   return i2c_bit_add_bus(cx-i2c_adap[0]) ||
  -   i2c_bit_add_bus(cx-i2c_adap[1]);
  +   err = i2c_bit_add_bus(cx-i2c_adap[0]);
 
   if (err)
   return err;
   err = i2c_bit_add_bus(cx-i2c_adap[1]);
   if (err)
   i2c_del_adapter(cx-i2c_adap[0]);
   return err;
 
 This sequence saves a few lines of code and gets rid of the goto's
 compared to what you proposed below.

Correct, actually my initial attempt looked like this. But then patch
3/6 adds code, which makes your solution 2 lines bigger, while my
solution stays as is, so the difference between both becomes very thin.

Some developers (including me) prefer to have a single error path,
others hate gotos more than (potential) code duplication. I didn't know
what you'd prefer as the driver maintainer. If you want me to use the
variant without gotos, I can do that, no problem.

  +   if (err)
  +   goto err;
  +   err = i2c_bit_add_bus(cx-i2c_adap[1]);
  +   if (err)
  +   goto err_del_bus_0;
  +   return 0;
  +
  + err_del_bus_0:
  +   i2c_del_adapter(cx-i2c_adap[0]);
  + err:
  +   return err;
   }
   
   void exit_cx18_i2c(struct cx18 *cx)
 
 Reviewed-by: Andy Walls awa...@radix.net

Thanks,
-- 
Jean Delvare
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] cx18: Fix the handling of i2c bus registration error

2009-04-04 Thread Andy Walls 
On Sat, 2009-04-04 at 16:23 +0200, Jean Delvare wrote:
 Hi Andy,
 
 Thanks for the fast review.
 
 On Sat, 04 Apr 2009 08:46:00 -0400, Andy Walls wrote:

 
 Correct, actually my initial attempt looked like this. But then patch
 3/6 adds code, which makes your solution 2 lines bigger, while my
 solution stays as is, so the difference between both becomes very thin.
 
 Some developers (including me) prefer to have a single error path,
 others hate gotos more than (potential) code duplication. I didn't know
 what you'd prefer as the driver maintainer. If you want me to use the
 variant without gotos, I can do that, no problem.

Meh, whichever way you like is fine for now.  If I really decide to care
about it, I'll muck with it when I get the hardware I2C masters working.
I'll have to touch that section of code at that time anyway.

Acked-by: Andy Walls awa...@radix.net

Regards,
Andy


--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html