Re: [PATCH 3/7] si2157: Add hybrid tuner support

[Brad Love]

Hi Mauro,


On 2018-03-06 06:24, Mauro Carvalho Chehab wrote:
> Hi Brad,
>
> As patches 1 and 2 are independent of this one, and should be backward
> compatible, I'm applying them, but I have issues with this one too :-)
>
> Em Tue, 16 Jan 2018 14:48:35 -0600
> Brad Love  escreveu:
>
>> On 2018-01-15 23:05, Antti Palosaari wrote:
>>> Hello
>>> So the use case is to share single tuner with multiple demodulators?
>>> Why don't just register single tuner and pass that info to multiple
>>> demods?
>>>
>>> Antti  
>> Hello Antti,
>>
>> It was done this way because of lack of knowledge of other ways. The
>> method I used mirrored that done by the three other drivers I found
>> which supported *and* included multiple front ends. We had this _attach
>> function sitting around as part of wip analog support to the si2157, and
>> it seemed like a nice fit here.
> The thing is that dvb_attach() is a very dirty and ugly hack,
> done when we needed to use I2C low level API calls inside DVB,
> while using high level bus-attach based I2C API for V4L.
>
> The hybrid_tuner_instance is yet-another-ugly dirty hack, with even
> causes lots of troubles for static analyzers.
>
> Nowadays, we should, instead, let the I2C bus bind the device
> at the bus and take care of lifetime issues.
>
> Btw, please take a look on this changeset:
>
>   8f569c0b4e6b ("media: dvb-core: add helper functions for I2C binding")
>
> and an example about how to simplify the binding code at:
>
>   ad32495b1513 ("media: em28xx-dvb: simplify DVB module probing logic")
>
> Em28xx is currently the only driver using the newer functions - My
> plan is to cleanup other drivers using the same logic as well,
> eventually improving the implementation of the new functions if needed.

I noticed the cleanup, I'll include it in my revised patch.



>
>> I just perused the tree again and noticed one spot I missed originally,
>> which does not use an _attach function. I didn't realize I could just
>> memcpy tuner_ops from fe[0] to fe[1] and call it a done deal, it does
>> appear to work the same though.
> I didn't write any hybrid tuner support using the new I2C binding
> schema, but, if both demods use the same tuner, I guess that's all
> it should be needed (and set adap->mfe_shared to 1).


This patch can be removed from the set, I have just memcpy'd
the tuner_ops to fe[1] now and everything works. I will resubmit a patch
with the mfe_shared property set though.



>
>> Is this really all that is required? If so, I'll modify patch 7/7 and
>> put this patch to the side for now.
>>
>> Cheers,
>>
>> Brad
>>
>>
>>> On 01/12/2018 06:19 PM, Brad Love wrote:  
 Add ability to share a tuner amongst demodulators. Addtional
 demods are attached using hybrid_tuner_instance_list.

 The changes are equivalent to moving all of probe to _attach.
 Results are backwards compatible with current usage.

 If the tuner is acquired via attach, then .release cleans state.
 if the tuner is an i2c driver, then .release is set to NULL, and
 .remove cleans remaining state.

 The following file contains a static si2157_attach:
 - drivers/media/pci/saa7164/saa7164-dvb.c
 The function name has been appended with _priv to appease
 the compiler.

 Signed-off-by: Brad Love 
 ---
   drivers/media/pci/saa7164/saa7164-dvb.c |  11 +-
   drivers/media/tuners/si2157.c   | 232
 +++-
   drivers/media/tuners/si2157.h   |  14 ++
   drivers/media/tuners/si2157_priv.h  |   5 +
   4 files changed, 192 insertions(+), 70 deletions(-)

 diff --git a/drivers/media/pci/saa7164/saa7164-dvb.c
 b/drivers/media/pci/saa7164/saa7164-dvb.c
 index e76d3ba..9522c6c 100644
 --- a/drivers/media/pci/saa7164/saa7164-dvb.c
 +++ b/drivers/media/pci/saa7164/saa7164-dvb.c
 @@ -110,8 +110,9 @@ static struct si2157_config
 hauppauge_hvr2255_tuner_config = {
   .if_port = 1,
   };
   -static int si2157_attach(struct saa7164_port *port, struct
 i2c_adapter *adapter,
 -    struct dvb_frontend *fe, u8 addr8bit, struct si2157_config *cfg)
 +static int si2157_attach_priv(struct saa7164_port *port,
 +    struct i2c_adapter *adapter, struct dvb_frontend *fe,
 +    u8 addr8bit, struct si2157_config *cfg)
   {
   struct i2c_board_info bi;
   struct i2c_client *tuner;
 @@ -624,11 +625,13 @@ int saa7164_dvb_register(struct saa7164_port
 *port)
   if (port->dvb.frontend != NULL) {
     if (port->nr == 0) {
 -    si2157_attach(port, >i2c_bus[0].i2c_adap,
 +    si2157_attach_priv(port,
 +  >i2c_bus[0].i2c_adap,
     port->dvb.frontend, 0xc0,
     _hvr2255_tuner_config);
   } else {
 -    

Re: [PATCH 3/7] si2157: Add hybrid tuner support

[Mauro Carvalho Chehab]

Hi Brad,

As patches 1 and 2 are independent of this one, and should be backward
compatible, I'm applying them, but I have issues with this one too :-)

Em Tue, 16 Jan 2018 14:48:35 -0600
Brad Love  escreveu:

> On 2018-01-15 23:05, Antti Palosaari wrote:
> > Hello
> > So the use case is to share single tuner with multiple demodulators?
> > Why don't just register single tuner and pass that info to multiple
> > demods?
> >
> > Antti  
> 
> Hello Antti,
> 
> It was done this way because of lack of knowledge of other ways. The
> method I used mirrored that done by the three other drivers I found
> which supported *and* included multiple front ends. We had this _attach
> function sitting around as part of wip analog support to the si2157, and
> it seemed like a nice fit here.

The thing is that dvb_attach() is a very dirty and ugly hack,
done when we needed to use I2C low level API calls inside DVB,
while using high level bus-attach based I2C API for V4L.

The hybrid_tuner_instance is yet-another-ugly dirty hack, with even
causes lots of troubles for static analyzers.

Nowadays, we should, instead, let the I2C bus bind the device
at the bus and take care of lifetime issues.

Btw, please take a look on this changeset:

8f569c0b4e6b ("media: dvb-core: add helper functions for I2C binding")

and an example about how to simplify the binding code at:

ad32495b1513 ("media: em28xx-dvb: simplify DVB module probing logic")

Em28xx is currently the only driver using the newer functions - My
plan is to cleanup other drivers using the same logic as well,
eventually improving the implementation of the new functions if needed.

> I just perused the tree again and noticed one spot I missed originally,
> which does not use an _attach function. I didn't realize I could just
> memcpy tuner_ops from fe[0] to fe[1] and call it a done deal, it does
> appear to work the same though.

I didn't write any hybrid tuner support using the new I2C binding
schema, but, if both demods use the same tuner, I guess that's all
it should be needed (and set adap->mfe_shared to 1).

> Is this really all that is required? If so, I'll modify patch 7/7 and
> put this patch to the side for now.
> 
> Cheers,
> 
> Brad
> 
> 
> >
> > On 01/12/2018 06:19 PM, Brad Love wrote:  
> >> Add ability to share a tuner amongst demodulators. Addtional
> >> demods are attached using hybrid_tuner_instance_list.
> >>
> >> The changes are equivalent to moving all of probe to _attach.
> >> Results are backwards compatible with current usage.
> >>
> >> If the tuner is acquired via attach, then .release cleans state.
> >> if the tuner is an i2c driver, then .release is set to NULL, and
> >> .remove cleans remaining state.
> >>
> >> The following file contains a static si2157_attach:
> >> - drivers/media/pci/saa7164/saa7164-dvb.c
> >> The function name has been appended with _priv to appease
> >> the compiler.
> >>
> >> Signed-off-by: Brad Love 
> >> ---
> >>   drivers/media/pci/saa7164/saa7164-dvb.c |  11 +-
> >>   drivers/media/tuners/si2157.c   | 232
> >> +++-
> >>   drivers/media/tuners/si2157.h   |  14 ++
> >>   drivers/media/tuners/si2157_priv.h  |   5 +
> >>   4 files changed, 192 insertions(+), 70 deletions(-)
> >>
> >> diff --git a/drivers/media/pci/saa7164/saa7164-dvb.c
> >> b/drivers/media/pci/saa7164/saa7164-dvb.c
> >> index e76d3ba..9522c6c 100644
> >> --- a/drivers/media/pci/saa7164/saa7164-dvb.c
> >> +++ b/drivers/media/pci/saa7164/saa7164-dvb.c
> >> @@ -110,8 +110,9 @@ static struct si2157_config
> >> hauppauge_hvr2255_tuner_config = {
> >>   .if_port = 1,
> >>   };
> >>   -static int si2157_attach(struct saa7164_port *port, struct
> >> i2c_adapter *adapter,
> >> -    struct dvb_frontend *fe, u8 addr8bit, struct si2157_config *cfg)
> >> +static int si2157_attach_priv(struct saa7164_port *port,
> >> +    struct i2c_adapter *adapter, struct dvb_frontend *fe,
> >> +    u8 addr8bit, struct si2157_config *cfg)
> >>   {
> >>   struct i2c_board_info bi;
> >>   struct i2c_client *tuner;
> >> @@ -624,11 +625,13 @@ int saa7164_dvb_register(struct saa7164_port
> >> *port)
> >>   if (port->dvb.frontend != NULL) {
> >>     if (port->nr == 0) {
> >> -    si2157_attach(port, >i2c_bus[0].i2c_adap,
> >> +    si2157_attach_priv(port,
> >> +  >i2c_bus[0].i2c_adap,
> >>     port->dvb.frontend, 0xc0,
> >>     _hvr2255_tuner_config);
> >>   } else {
> >> -    si2157_attach(port, >i2c_bus[1].i2c_adap,
> >> +    si2157_attach_priv(port,
> >> +  >i2c_bus[1].i2c_adap,
> >>     port->dvb.frontend, 0xc0,
> >>     _hvr2255_tuner_config);
> >>   }
> >> diff --git a/drivers/media/tuners/si2157.c
> >> b/drivers/media/tuners/si2157.c
> 

Re: [PATCH 3/7] si2157: Add hybrid tuner support

[Brad Love]

On 2018-01-15 23:05, Antti Palosaari wrote:
> Hello
> So the use case is to share single tuner with multiple demodulators?
> Why don't just register single tuner and pass that info to multiple
> demods?
>
> Antti

Hello Antti,

It was done this way because of lack of knowledge of other ways. The
method I used mirrored that done by the three other drivers I found
which supported *and* included multiple front ends. We had this _attach
function sitting around as part of wip analog support to the si2157, and
it seemed like a nice fit here.

I just perused the tree again and noticed one spot I missed originally,
which does not use an _attach function. I didn't realize I could just
memcpy tuner_ops from fe[0] to fe[1] and call it a done deal, it does
appear to work the same though.

Is this really all that is required? If so, I'll modify patch 7/7 and
put this patch to the side for now.

Cheers,

Brad


>
> On 01/12/2018 06:19 PM, Brad Love wrote:
>> Add ability to share a tuner amongst demodulators. Addtional
>> demods are attached using hybrid_tuner_instance_list.
>>
>> The changes are equivalent to moving all of probe to _attach.
>> Results are backwards compatible with current usage.
>>
>> If the tuner is acquired via attach, then .release cleans state.
>> if the tuner is an i2c driver, then .release is set to NULL, and
>> .remove cleans remaining state.
>>
>> The following file contains a static si2157_attach:
>> - drivers/media/pci/saa7164/saa7164-dvb.c
>> The function name has been appended with _priv to appease
>> the compiler.
>>
>> Signed-off-by: Brad Love 
>> ---
>>   drivers/media/pci/saa7164/saa7164-dvb.c |  11 +-
>>   drivers/media/tuners/si2157.c   | 232
>> +++-
>>   drivers/media/tuners/si2157.h   |  14 ++
>>   drivers/media/tuners/si2157_priv.h  |   5 +
>>   4 files changed, 192 insertions(+), 70 deletions(-)
>>
>> diff --git a/drivers/media/pci/saa7164/saa7164-dvb.c
>> b/drivers/media/pci/saa7164/saa7164-dvb.c
>> index e76d3ba..9522c6c 100644
>> --- a/drivers/media/pci/saa7164/saa7164-dvb.c
>> +++ b/drivers/media/pci/saa7164/saa7164-dvb.c
>> @@ -110,8 +110,9 @@ static struct si2157_config
>> hauppauge_hvr2255_tuner_config = {
>>   .if_port = 1,
>>   };
>>   -static int si2157_attach(struct saa7164_port *port, struct
>> i2c_adapter *adapter,
>> -    struct dvb_frontend *fe, u8 addr8bit, struct si2157_config *cfg)
>> +static int si2157_attach_priv(struct saa7164_port *port,
>> +    struct i2c_adapter *adapter, struct dvb_frontend *fe,
>> +    u8 addr8bit, struct si2157_config *cfg)
>>   {
>>   struct i2c_board_info bi;
>>   struct i2c_client *tuner;
>> @@ -624,11 +625,13 @@ int saa7164_dvb_register(struct saa7164_port
>> *port)
>>   if (port->dvb.frontend != NULL) {
>>     if (port->nr == 0) {
>> -    si2157_attach(port, >i2c_bus[0].i2c_adap,
>> +    si2157_attach_priv(port,
>> +  >i2c_bus[0].i2c_adap,
>>     port->dvb.frontend, 0xc0,
>>     _hvr2255_tuner_config);
>>   } else {
>> -    si2157_attach(port, >i2c_bus[1].i2c_adap,
>> +    si2157_attach_priv(port,
>> +  >i2c_bus[1].i2c_adap,
>>     port->dvb.frontend, 0xc0,
>>     _hvr2255_tuner_config);
>>   }
>> diff --git a/drivers/media/tuners/si2157.c
>> b/drivers/media/tuners/si2157.c
>> index e35b1fa..9121361 100644
>> --- a/drivers/media/tuners/si2157.c
>> +++ b/drivers/media/tuners/si2157.c
>> @@ -18,6 +18,11 @@
>>     static const struct dvb_tuner_ops si2157_ops;
>>   +static DEFINE_MUTEX(si2157_list_mutex);
>> +static LIST_HEAD(hybrid_tuner_instance_list);
>> +
>> +/*-*/
>>
>> +
>>   /* execute firmware command */
>>   static int si2157_cmd_execute(struct i2c_client *client, struct
>> si2157_cmd *cmd)
>>   {
>> @@ -385,6 +390,31 @@ static int si2157_get_if_frequency(struct
>> dvb_frontend *fe, u32 *frequency)
>>   return 0;
>>   }
>>   +static void si2157_release(struct dvb_frontend *fe)
>> +{
>> +    struct i2c_client *client = fe->tuner_priv;
>> +    struct si2157_dev *dev = i2c_get_clientdata(client);
>> +
>> +    dev_dbg(>dev, "%s()\n", __func__);
>> +
>> +    /* only do full cleanup on final instance */
>> +    if (hybrid_tuner_report_instance_count(dev) == 1) {
>> +    /* stop statistics polling */
>> +    cancel_delayed_work_sync(>stat_work);
>> +#ifdef CONFIG_MEDIA_CONTROLLER_DVB
>> +    if (dev->mdev)
>> +    media_device_unregister_entity(>ent);
>> +#endif
>> +    i2c_set_clientdata(client, NULL);
>> +    }
>> +
>> +    mutex_lock(_list_mutex);
>> +    hybrid_tuner_release_state(dev);
>> +    mutex_unlock(_list_mutex);
>> +
>> +    fe->tuner_priv = NULL;
>> +}
>> +
>>   static const struct dvb_tuner_ops si2157_ops = {
>> 

Re: [PATCH 3/7] si2157: Add hybrid tuner support

[Antti Palosaari]

Hello
So the use case is to share single tuner with multiple demodulators? Why 
don't just register single tuner and pass that info to multiple demods?


Antti

On 01/12/2018 06:19 PM, Brad Love wrote:

Add ability to share a tuner amongst demodulators. Addtional
demods are attached using hybrid_tuner_instance_list.

The changes are equivalent to moving all of probe to _attach.
Results are backwards compatible with current usage.

If the tuner is acquired via attach, then .release cleans state.
if the tuner is an i2c driver, then .release is set to NULL, and
.remove cleans remaining state.

The following file contains a static si2157_attach:
- drivers/media/pci/saa7164/saa7164-dvb.c
The function name has been appended with _priv to appease
the compiler.

Signed-off-by: Brad Love 
---
  drivers/media/pci/saa7164/saa7164-dvb.c |  11 +-
  drivers/media/tuners/si2157.c   | 232 +++-
  drivers/media/tuners/si2157.h   |  14 ++
  drivers/media/tuners/si2157_priv.h  |   5 +
  4 files changed, 192 insertions(+), 70 deletions(-)

diff --git a/drivers/media/pci/saa7164/saa7164-dvb.c 
b/drivers/media/pci/saa7164/saa7164-dvb.c
index e76d3ba..9522c6c 100644
--- a/drivers/media/pci/saa7164/saa7164-dvb.c
+++ b/drivers/media/pci/saa7164/saa7164-dvb.c
@@ -110,8 +110,9 @@ static struct si2157_config hauppauge_hvr2255_tuner_config 
= {
.if_port = 1,
  };
  
-static int si2157_attach(struct saa7164_port *port, struct i2c_adapter *adapter,

-   struct dvb_frontend *fe, u8 addr8bit, struct si2157_config *cfg)
+static int si2157_attach_priv(struct saa7164_port *port,
+   struct i2c_adapter *adapter, struct dvb_frontend *fe,
+   u8 addr8bit, struct si2157_config *cfg)
  {
struct i2c_board_info bi;
struct i2c_client *tuner;
@@ -624,11 +625,13 @@ int saa7164_dvb_register(struct saa7164_port *port)
if (port->dvb.frontend != NULL) {
  
  			if (port->nr == 0) {

-   si2157_attach(port, >i2c_bus[0].i2c_adap,
+   si2157_attach_priv(port,
+ >i2c_bus[0].i2c_adap,
  port->dvb.frontend, 0xc0,
  _hvr2255_tuner_config);
} else {
-   si2157_attach(port, >i2c_bus[1].i2c_adap,
+   si2157_attach_priv(port,
+ >i2c_bus[1].i2c_adap,
  port->dvb.frontend, 0xc0,
  _hvr2255_tuner_config);
}
diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index e35b1fa..9121361 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -18,6 +18,11 @@
  
  static const struct dvb_tuner_ops si2157_ops;
  
+static DEFINE_MUTEX(si2157_list_mutex);

+static LIST_HEAD(hybrid_tuner_instance_list);
+
+/*-*/
+
  /* execute firmware command */
  static int si2157_cmd_execute(struct i2c_client *client, struct si2157_cmd 
*cmd)
  {
@@ -385,6 +390,31 @@ static int si2157_get_if_frequency(struct dvb_frontend 
*fe, u32 *frequency)
return 0;
  }
  
+static void si2157_release(struct dvb_frontend *fe)

+{
+   struct i2c_client *client = fe->tuner_priv;
+   struct si2157_dev *dev = i2c_get_clientdata(client);
+
+   dev_dbg(>dev, "%s()\n", __func__);
+
+   /* only do full cleanup on final instance */
+   if (hybrid_tuner_report_instance_count(dev) == 1) {
+   /* stop statistics polling */
+   cancel_delayed_work_sync(>stat_work);
+#ifdef CONFIG_MEDIA_CONTROLLER_DVB
+   if (dev->mdev)
+   media_device_unregister_entity(>ent);
+#endif
+   i2c_set_clientdata(client, NULL);
+   }
+
+   mutex_lock(_list_mutex);
+   hybrid_tuner_release_state(dev);
+   mutex_unlock(_list_mutex);
+
+   fe->tuner_priv = NULL;
+}
+
  static const struct dvb_tuner_ops si2157_ops = {
.info = {
.name   = "Silicon Labs 
Si2141/Si2146/2147/2148/2157/2158",
@@ -396,6 +426,7 @@ static const struct dvb_tuner_ops si2157_ops = {
.sleep = si2157_sleep,
.set_params = si2157_set_params,
.get_if_frequency = si2157_get_if_frequency,
+   .release = si2157_release,
  };
  
  static void si2157_stat_work(struct work_struct *work)

@@ -431,72 +462,30 @@ static int si2157_probe(struct i2c_client *client,
  {
struct si2157_config *cfg = client->dev.platform_data;
struct dvb_frontend *fe = cfg->fe;
-   struct si2157_dev *dev;
-   struct si2157_cmd cmd;
-   int ret;
-
-   dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-   if (!dev) {
-   ret = -ENOMEM;
-