Re: [PATCH 5/9] cx231xx: Switch to using new dvb i2c helpers

[Brad Love]

Hi Matthias,


On 2018-04-19 12:09, Matthias Schwarzott wrote:
> Am 17.04.2018 um 18:39 schrieb Brad Love:
>> Mostly very straight forward replace of blocks with equivalent code.
>>
>> Cleanup added at end of dvb_init in case of failure.
>>
> Hi Brad,
>
> I have some suggestions. See below.
>
> Matthias


Thanks for the review, I'll work your suggestions into a v2 set soon.




>
>> Signed-off-by: Brad Love 
>> ---
>>  drivers/media/usb/cx231xx/cx231xx-dvb.c | 331 
>> 
>>  1 file changed, 82 insertions(+), 249 deletions(-)
>>
>> diff --git a/drivers/media/usb/cx231xx/cx231xx-dvb.c 
>> b/drivers/media/usb/cx231xx/cx231xx-dvb.c
>> index 681610f..318a6cd 100644
>> --- a/drivers/media/usb/cx231xx/cx231xx-dvb.c
>> +++ b/drivers/media/usb/cx231xx/cx231xx-dvb.c
>> @@ -613,23 +613,18 @@ static void unregister_dvb(struct cx231xx_dvb *dvb)
>>  dvb_frontend_detach(dvb->frontend[1]);
>>  dvb_frontend_detach(dvb->frontend[0]);
>>  dvb_unregister_adapter(>adapter);
>> +
>>  /* remove I2C tuner */
>>  client = dvb->i2c_client_tuner;
>> -if (client) {
>> -module_put(client->dev.driver->owner);
>> -i2c_unregister_device(client);
>> -}
>> -/* remove I2C demod */
>> +if (client)
>> +dvb_module_release(client);
> The pointer check is not needed, dvb_module_release does check itself
> for NULL.
>
> Other drivers added code to set the client-pointers to NULL after
> releasing it.
>
> I suggest to do it like this:
> /* remove I2C tuner */
> dvb_module_release(dvb->i2c_client_tuner);
> dvb->i2c_client_tuner = NULL;
>
>
>> +/* remove I2C demod(s) */
>>  client = dvb->i2c_client_demod[1];
>> -if (client) {
>> -module_put(client->dev.driver->owner);
>> -i2c_unregister_device(client);
>> -}
>> +if (client)
>> +dvb_module_release(client);
>>  client = dvb->i2c_client_demod[0];
>> -if (client) {
>> -module_put(client->dev.driver->owner);
>> -i2c_unregister_device(client);
>> -}
>> +if (client)
>> +dvb_module_release(client);
>>  }
>>  
>>  static int dvb_init(struct cx231xx *dev)
>> @@ -638,6 +633,8 @@ static int dvb_init(struct cx231xx *dev)
>>  struct cx231xx_dvb *dvb;
>>  struct i2c_adapter *tuner_i2c;
>>  struct i2c_adapter *demod_i2c;
>> +struct i2c_client *client;
>> +struct i2c_adapter *adapter;
>>  
>>  if (!dev->board.has_dvb) {
>>  /* This device does not support the extension */
>> @@ -785,8 +782,6 @@ static int dvb_init(struct cx231xx *dev)
>>  
>>  case CX231XX_BOARD_HAUPPAUGE_930C_HD_1113xx:
>>  {
>> -struct i2c_client *client;
>> -struct i2c_board_info info;
>>  struct si2165_platform_data si2165_pdata = {};
>>  
>>  /* attach demod */
>> @@ -794,25 +789,14 @@ static int dvb_init(struct cx231xx *dev)
>>  si2165_pdata.chip_mode = SI2165_MODE_PLL_XTAL;
>>  si2165_pdata.ref_freq_hz = 1600;
>>  
>> -memset(, 0, sizeof(struct i2c_board_info));
>> -strlcpy(info.type, "si2165", I2C_NAME_SIZE);
>> -info.addr = dev->board.demod_addr;
>> -info.platform_data = _pdata;
>> -request_module(info.type);
>> -client = i2c_new_device(demod_i2c, );
>> -if (!client || !client->dev.driver || !dev->dvb->frontend[0]) {
>> -dev_err(dev->dev,
>> -"Failed to attach SI2165 front end\n");
>> -result = -EINVAL;
>> -goto out_free;
>> -}
>> -
>> -if (!try_module_get(client->dev.driver->owner)) {
>> -i2c_unregister_device(client);
>> +/* perform tuner probe/init/attach */
>> +client = dvb_module_probe("si2165", NULL, demod_i2c,
>> +dev->board.demod_addr,
>> +_pdata);
>> +if (!client) {
>>  result = -ENODEV;
>>  goto out_free;
>>  }
>> -
>>  dvb->i2c_client_demod[0] = client;
>>  
>>  dev->dvb->frontend[0]->ops.i2c_gate_ctrl = NULL;
>> @@ -829,8 +813,6 @@ static int dvb_init(struct cx231xx *dev)
>>  }
>>  case CX231XX_BOARD_HAUPPAUGE_930C_HD_1114xx:
>>  {
>> -struct i2c_client *client;
>> -struct i2c_board_info info;
>>  struct si2165_platform_data si2165_pdata = {};
>>  struct si2157_config si2157_config = {};
>>  
>> @@ -839,29 +821,16 @@ static int dvb_init(struct cx231xx *dev)
>>  si2165_pdata.chip_mode = SI2165_MODE_PLL_EXT;
>>  si2165_pdata.ref_freq_hz = 2400;
>>  
>> -memset(, 0, sizeof(struct i2c_board_info));
>> -strlcpy(info.type, "si2165", I2C_NAME_SIZE);
>> -  

Re: [PATCH 5/9] cx231xx: Switch to using new dvb i2c helpers

[Matthias Schwarzott]

Am 17.04.2018 um 18:39 schrieb Brad Love:
> Mostly very straight forward replace of blocks with equivalent code.
> 
> Cleanup added at end of dvb_init in case of failure.
> 
Hi Brad,

I have some suggestions. See below.

Matthias

> Signed-off-by: Brad Love 
> ---
>  drivers/media/usb/cx231xx/cx231xx-dvb.c | 331 
> 
>  1 file changed, 82 insertions(+), 249 deletions(-)
> 
> diff --git a/drivers/media/usb/cx231xx/cx231xx-dvb.c 
> b/drivers/media/usb/cx231xx/cx231xx-dvb.c
> index 681610f..318a6cd 100644
> --- a/drivers/media/usb/cx231xx/cx231xx-dvb.c
> +++ b/drivers/media/usb/cx231xx/cx231xx-dvb.c
> @@ -613,23 +613,18 @@ static void unregister_dvb(struct cx231xx_dvb *dvb)
>   dvb_frontend_detach(dvb->frontend[1]);
>   dvb_frontend_detach(dvb->frontend[0]);
>   dvb_unregister_adapter(>adapter);
> +
>   /* remove I2C tuner */
>   client = dvb->i2c_client_tuner;
> - if (client) {
> - module_put(client->dev.driver->owner);
> - i2c_unregister_device(client);
> - }
> - /* remove I2C demod */
> + if (client)
> + dvb_module_release(client);
The pointer check is not needed, dvb_module_release does check itself
for NULL.

Other drivers added code to set the client-pointers to NULL after
releasing it.

I suggest to do it like this:
/* remove I2C tuner */
dvb_module_release(dvb->i2c_client_tuner);
dvb->i2c_client_tuner = NULL;


> + /* remove I2C demod(s) */
>   client = dvb->i2c_client_demod[1];
> - if (client) {
> - module_put(client->dev.driver->owner);
> - i2c_unregister_device(client);
> - }
> + if (client)
> + dvb_module_release(client);
>   client = dvb->i2c_client_demod[0];
> - if (client) {
> - module_put(client->dev.driver->owner);
> - i2c_unregister_device(client);
> - }
> + if (client)
> + dvb_module_release(client);

>  }
>  
>  static int dvb_init(struct cx231xx *dev)
> @@ -638,6 +633,8 @@ static int dvb_init(struct cx231xx *dev)
>   struct cx231xx_dvb *dvb;
>   struct i2c_adapter *tuner_i2c;
>   struct i2c_adapter *demod_i2c;
> + struct i2c_client *client;
> + struct i2c_adapter *adapter;
>  
>   if (!dev->board.has_dvb) {
>   /* This device does not support the extension */
> @@ -785,8 +782,6 @@ static int dvb_init(struct cx231xx *dev)
>  
>   case CX231XX_BOARD_HAUPPAUGE_930C_HD_1113xx:
>   {
> - struct i2c_client *client;
> - struct i2c_board_info info;
>   struct si2165_platform_data si2165_pdata = {};
>  
>   /* attach demod */
> @@ -794,25 +789,14 @@ static int dvb_init(struct cx231xx *dev)
>   si2165_pdata.chip_mode = SI2165_MODE_PLL_XTAL;
>   si2165_pdata.ref_freq_hz = 1600;
>  
> - memset(, 0, sizeof(struct i2c_board_info));
> - strlcpy(info.type, "si2165", I2C_NAME_SIZE);
> - info.addr = dev->board.demod_addr;
> - info.platform_data = _pdata;
> - request_module(info.type);
> - client = i2c_new_device(demod_i2c, );
> - if (!client || !client->dev.driver || !dev->dvb->frontend[0]) {
> - dev_err(dev->dev,
> - "Failed to attach SI2165 front end\n");
> - result = -EINVAL;
> - goto out_free;
> - }
> -
> - if (!try_module_get(client->dev.driver->owner)) {
> - i2c_unregister_device(client);
> + /* perform tuner probe/init/attach */
> + client = dvb_module_probe("si2165", NULL, demod_i2c,
> + dev->board.demod_addr,
> + _pdata);
> + if (!client) {
>   result = -ENODEV;
>   goto out_free;
>   }
> -
>   dvb->i2c_client_demod[0] = client;
>  
>   dev->dvb->frontend[0]->ops.i2c_gate_ctrl = NULL;
> @@ -829,8 +813,6 @@ static int dvb_init(struct cx231xx *dev)
>   }
>   case CX231XX_BOARD_HAUPPAUGE_930C_HD_1114xx:
>   {
> - struct i2c_client *client;
> - struct i2c_board_info info;
>   struct si2165_platform_data si2165_pdata = {};
>   struct si2157_config si2157_config = {};
>  
> @@ -839,29 +821,16 @@ static int dvb_init(struct cx231xx *dev)
>   si2165_pdata.chip_mode = SI2165_MODE_PLL_EXT;
>   si2165_pdata.ref_freq_hz = 2400;
>  
> - memset(, 0, sizeof(struct i2c_board_info));
> - strlcpy(info.type, "si2165", I2C_NAME_SIZE);
> - info.addr = dev->board.demod_addr;
> - info.platform_data = _pdata;
> - request_module(info.type);
> - client = i2c_new_device(demod_i2c, );
> - if 

[PATCH 5/9] cx231xx: Switch to using new dvb i2c helpers

[Brad Love]

Mostly very straight forward replace of blocks with equivalent code.

Cleanup added at end of dvb_init in case of failure.

Signed-off-by: Brad Love 
---
 drivers/media/usb/cx231xx/cx231xx-dvb.c | 331 
 1 file changed, 82 insertions(+), 249 deletions(-)

diff --git a/drivers/media/usb/cx231xx/cx231xx-dvb.c 
b/drivers/media/usb/cx231xx/cx231xx-dvb.c
index 681610f..318a6cd 100644
--- a/drivers/media/usb/cx231xx/cx231xx-dvb.c
+++ b/drivers/media/usb/cx231xx/cx231xx-dvb.c
@@ -613,23 +613,18 @@ static void unregister_dvb(struct cx231xx_dvb *dvb)
dvb_frontend_detach(dvb->frontend[1]);
dvb_frontend_detach(dvb->frontend[0]);
dvb_unregister_adapter(>adapter);
+
/* remove I2C tuner */
client = dvb->i2c_client_tuner;
-   if (client) {
-   module_put(client->dev.driver->owner);
-   i2c_unregister_device(client);
-   }
-   /* remove I2C demod */
+   if (client)
+   dvb_module_release(client);
+   /* remove I2C demod(s) */
client = dvb->i2c_client_demod[1];
-   if (client) {
-   module_put(client->dev.driver->owner);
-   i2c_unregister_device(client);
-   }
+   if (client)
+   dvb_module_release(client);
client = dvb->i2c_client_demod[0];
-   if (client) {
-   module_put(client->dev.driver->owner);
-   i2c_unregister_device(client);
-   }
+   if (client)
+   dvb_module_release(client);
 }
 
 static int dvb_init(struct cx231xx *dev)
@@ -638,6 +633,8 @@ static int dvb_init(struct cx231xx *dev)
struct cx231xx_dvb *dvb;
struct i2c_adapter *tuner_i2c;
struct i2c_adapter *demod_i2c;
+   struct i2c_client *client;
+   struct i2c_adapter *adapter;
 
if (!dev->board.has_dvb) {
/* This device does not support the extension */
@@ -785,8 +782,6 @@ static int dvb_init(struct cx231xx *dev)
 
case CX231XX_BOARD_HAUPPAUGE_930C_HD_1113xx:
{
-   struct i2c_client *client;
-   struct i2c_board_info info;
struct si2165_platform_data si2165_pdata = {};
 
/* attach demod */
@@ -794,25 +789,14 @@ static int dvb_init(struct cx231xx *dev)
si2165_pdata.chip_mode = SI2165_MODE_PLL_XTAL;
si2165_pdata.ref_freq_hz = 1600;
 
-   memset(, 0, sizeof(struct i2c_board_info));
-   strlcpy(info.type, "si2165", I2C_NAME_SIZE);
-   info.addr = dev->board.demod_addr;
-   info.platform_data = _pdata;
-   request_module(info.type);
-   client = i2c_new_device(demod_i2c, );
-   if (!client || !client->dev.driver || !dev->dvb->frontend[0]) {
-   dev_err(dev->dev,
-   "Failed to attach SI2165 front end\n");
-   result = -EINVAL;
-   goto out_free;
-   }
-
-   if (!try_module_get(client->dev.driver->owner)) {
-   i2c_unregister_device(client);
+   /* perform tuner probe/init/attach */
+   client = dvb_module_probe("si2165", NULL, demod_i2c,
+   dev->board.demod_addr,
+   _pdata);
+   if (!client) {
result = -ENODEV;
goto out_free;
}
-
dvb->i2c_client_demod[0] = client;
 
dev->dvb->frontend[0]->ops.i2c_gate_ctrl = NULL;
@@ -829,8 +813,6 @@ static int dvb_init(struct cx231xx *dev)
}
case CX231XX_BOARD_HAUPPAUGE_930C_HD_1114xx:
{
-   struct i2c_client *client;
-   struct i2c_board_info info;
struct si2165_platform_data si2165_pdata = {};
struct si2157_config si2157_config = {};
 
@@ -839,29 +821,16 @@ static int dvb_init(struct cx231xx *dev)
si2165_pdata.chip_mode = SI2165_MODE_PLL_EXT;
si2165_pdata.ref_freq_hz = 2400;
 
-   memset(, 0, sizeof(struct i2c_board_info));
-   strlcpy(info.type, "si2165", I2C_NAME_SIZE);
-   info.addr = dev->board.demod_addr;
-   info.platform_data = _pdata;
-   request_module(info.type);
-   client = i2c_new_device(demod_i2c, );
-   if (!client || !client->dev.driver || !dev->dvb->frontend[0]) {
-   dev_err(dev->dev,
-   "Failed to attach SI2165 front end\n");
-   result = -EINVAL;
-   goto out_free;
-   }
-
-   if (!try_module_get(client->dev.driver->owner)) {
-   i2c_unregister_device(client);
+   /* perform tuner probe/init/attach */
+