Re: [PATCH 3/7] si2157: Add hybrid tuner support
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, &dev->i2c_bus[0].i2c_adap, + si2157_attach_priv(port, + &dev->i2c_bus[0].i2c_adap, port->dvb.frontend, 0xc0, &hauppauge_hvr2255_tuner_config); } else { - si2157_attach(port, &dev->i2c
Re: [PATCH 3/7] si2157: Add hybrid tuner support
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, &dev->i2c_bus[0].i2c_adap, > >> + si2157_attach_priv(port, > >> + &dev->i2c_bus[0].i2c_adap, > >> port->dvb.frontend, 0xc0, > >> &hauppauge_hvr2255_tuner_config); > >> } else { > >> - si2157_attach(port, &dev->i2c_bus[1].i2c_adap, > >> + si2157_attach_priv(port, > >> + &dev->i2c_bus[1].i2c_adap, > >> port->dvb.frontend, 0xc0, > >> &hauppauge_hvr2255_tuner_config); > >> } > >> diff --git a/drivers/media/tuners/si2157.c > >> b/drivers/media/tuners/si2157.c > >> ind
Re: [PATCH 3/7] si2157: Add hybrid tuner support
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, &dev->i2c_bus[0].i2c_adap, >> + si2157_attach_priv(port, >> + &dev->i2c_bus[0].i2c_adap, >> port->dvb.frontend, 0xc0, >> &hauppauge_hvr2255_tuner_config); >> } else { >> - si2157_attach(port, &dev->i2c_bus[1].i2c_adap, >> + si2157_attach_priv(port, >> + &dev->i2c_bus[1].i2c_adap, >> port->dvb.frontend, 0xc0, >> &hauppauge_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(&client->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(&dev->stat_work); >> +#ifdef CONFIG_MEDIA_CONTROLLER_DVB >> + if (dev->mdev) >> + media_device_unregister_entity(&dev->ent); >> +#endif >> + i2c_set_clientdata(client, NULL); >> + } >> + >> + mutex_lock(&si2157_list_mutex); >> + hybrid_tuner_release_state(dev); >> + mutex_unlock(&si2157_list_mutex); >> + >> + fe->tuner_priv = NULL; >> +} >> + >> sta
Re: [PATCH 3/7] si2157: Add hybrid tuner support
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, &dev->i2c_bus[0].i2c_adap, + si2157_attach_priv(port, + &dev->i2c_bus[0].i2c_adap, port->dvb.frontend, 0xc0, &hauppauge_hvr2255_tuner_config); } else { - si2157_attach(port, &dev->i2c_bus[1].i2c_adap, + si2157_attach_priv(port, + &dev->i2c_bus[1].i2c_adap, port->dvb.frontend, 0xc0, &hauppauge_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(&client->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(&dev->stat_work); +#ifdef CONFIG_MEDIA_CONTROLLER_DVB + if (dev->mdev) + media_device_unregister_entity(&dev->ent); +#endif + i2c_set_clientdata(client, NULL); + } + + mutex_lock(&si2157_list_mutex); + hybrid_tuner_release_state(dev); + mutex_unlock(&si2157_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) {