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 <brad@xxxxxxxxxxxxxxxx> 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 <brad@xxxxxxxxxxxxxxxx>
>>>> ---
>>>> 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) {
>>>> - ret = -ENOMEM;
>>>> - dev_err(&client->dev, "kzalloc() failed\n");
>>>> - goto err;
>>>> - }
>>>> -
>>>> - i2c_set_clientdata(client, dev);
>>>> - dev->fe = cfg->fe;
>>>> - dev->inversion = cfg->inversion;
>>>> - dev->if_port = cfg->if_port;
>>>> - dev->chiptype = (u8)id->driver_data;
>>>> - dev->if_frequency = 5000000; /* default value of property 0x0706 */
>>>> - mutex_init(&dev->i2c_mutex);
>>>> - INIT_DELAYED_WORK(&dev->stat_work, si2157_stat_work);
>>>> + struct si2157_dev *dev = NULL;
>>>> + unsigned short addr = client->addr;
>>>> + int ret = 0;
>>>> - /* check if the tuner is there */
>>>> - cmd.wlen = 0;
>>>> - cmd.rlen = 1;
>>>> - ret = si2157_cmd_execute(client, &cmd);
>>>> - if (ret)
>>>> - goto err_kfree;
>>>> -
>>>> - memcpy(&fe->ops.tuner_ops, &si2157_ops, sizeof(struct
>>>> dvb_tuner_ops));
>>>> + dev_dbg(&client->dev, "Probing tuner\n");
>>>> fe->tuner_priv = client;
>>>> -#ifdef CONFIG_MEDIA_CONTROLLER
>>>> - if (cfg->mdev) {
>>>> - dev->mdev = cfg->mdev;
>>>> -
>>>> - dev->ent.name = KBUILD_MODNAME;
>>>> - dev->ent.function = MEDIA_ENT_F_TUNER;
>>>> -
>>>> - dev->pad[TUNER_PAD_RF_INPUT].flags = MEDIA_PAD_FL_SINK;
>>>> - dev->pad[TUNER_PAD_OUTPUT].flags = MEDIA_PAD_FL_SOURCE;
>>>> - dev->pad[TUNER_PAD_AUD_OUT].flags = MEDIA_PAD_FL_SOURCE;
>>>> -
>>>> - ret = media_entity_pads_init(&dev->ent, TUNER_NUM_PADS,
>>>> - &dev->pad[0]);
>>>> -
>>>> - if (ret)
>>>> - goto err_kfree;
>>>> -
>>>> - ret = media_device_register_entity(cfg->mdev, &dev->ent);
>>>> - if (ret) {
>>>> - media_entity_cleanup(&dev->ent);
>>>> - goto err_kfree;
>>>> - }
>>>> + if (si2157_attach(fe, (u8)addr, client->adapter, cfg) == NULL) {
>>>> + dev_err(&client->dev, "%s: attaching si2157 tuner failed\n",
>>>> + __func__);
>>>> + goto err;
>>>> }
>>>> -#endif
>>>> + fe->ops.tuner_ops.release = NULL;
>>>> + dev = i2c_get_clientdata(client);
>>>> + dev->chiptype = (u8)id->driver_data;
>>>> dev_info(&client->dev, "Silicon Labs %s successfully attached\n",
>>>> dev->chiptype == SI2157_CHIPTYPE_SI2141 ? "Si2141" :
>>>> dev->chiptype == SI2157_CHIPTYPE_SI2146 ?
>>>> "Si2146" : "Si2147/2148/2157/2158");
>>>> return 0;
>>>> -
>>>> -err_kfree:
>>>> - kfree(dev);
>>>> err:
>>>> - dev_dbg(&client->dev, "failed=%d\n", ret);
>>>> + dev_warn(&client->dev, "probe failed = %d\n", ret);
>>>> return ret;
>>>> }
>>>> @@ -505,19 +494,10 @@ static int si2157_remove(struct i2c_client
>>>> *client)
>>>> struct si2157_dev *dev = i2c_get_clientdata(client);
>>>> struct dvb_frontend *fe = dev->fe;
>>>> - dev_dbg(&client->dev, "\n");
>>>> -
>>>> - /* 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
>>>> + dev_dbg(&client->dev, "%s()\n", __func__);
>>>> memset(&fe->ops.tuner_ops, 0, sizeof(struct dvb_tuner_ops));
>>>> - fe->tuner_priv = NULL;
>>>> - kfree(dev);
>>>> + si2157_release(fe);
>>>> return 0;
>>>> }
>>>> @@ -542,7 +522,127 @@ static struct i2c_driver si2157_driver = {
>>>> module_i2c_driver(si2157_driver);
>>>> -MODULE_DESCRIPTION("Silicon Labs Si2141/Si2146/2147/2148/2157/2158
>>>> silicon tuner driver");
>>>> +struct dvb_frontend *si2157_attach(struct dvb_frontend *fe, u8 addr,
>>>> + struct i2c_adapter *i2c,
>>>> + struct si2157_config *cfg)
>>>> +{
>>>> + struct i2c_client *client = NULL;
>>>> + struct si2157_dev *dev = NULL;
>>>> + struct si2157_cmd cmd;
>>>> + int instance = 0, ret;
>>>> +
>>>> + pr_debug("%s (%d-%04x)\n", __func__,
>>>> + i2c ? i2c_adapter_id(i2c) : 0,
>>>> + addr);
>>>> +
>>>> + if (!cfg) {
>>>> + pr_warn("no configuration submitted\n");
>>>> + goto fail;
>>>> + }
>>>> +
>>>> + if (!fe) {
>>>> + pr_warn("fe is NULL\n");
>>>> + goto fail;
>>>> + }
>>>> +
>>>> + client = fe->tuner_priv;
>>>> + if (!client) {
>>>> + pr_warn("client is NULL\n");
>>>> + goto fail;
>>>> + }
>>>> +
>>>> + mutex_lock(&si2157_list_mutex);
>>>> +
>>>> + instance = hybrid_tuner_request_state(struct si2157_dev, dev,
>>>> + hybrid_tuner_instance_list,
>>>> + i2c, addr, "si2157");
>>>> +
>>>> + switch (instance) {
>>>> + case 0:
>>>> + goto fail;
>>>> + case 1:
>>>> + /* new tuner instance */
>>>> + dev_dbg(&client->dev, "%s(): new instance for tuner @0x%02x\n",
>>>> + __func__, addr);
>>>> + dev->addr = addr;
>>>> + i2c_set_clientdata(client, dev);
>>>> +
>>>> + dev->fe = fe;
>>>> + dev->chiptype = SI2157_CHIPTYPE_SI2157;
>>>> + dev->if_frequency = 0;
>>>> + dev->if_port = cfg->if_port;
>>>> + dev->inversion = cfg->inversion;
>>>> +
>>>> + mutex_init(&dev->i2c_mutex);
>>>> + INIT_DELAYED_WORK(&dev->stat_work, si2157_stat_work);
>>>> +
>>>> + break;
>>>> + default:
>>>> + /* existing tuner instance */
>>>> + dev_dbg(&client->dev,
>>>> + "%s(): using existing instance for tuner @0x%02x\n",
>>>> + __func__, addr);
>>>> + break;
>>>> + }
>>>> +
>>>> + /* check if the tuner is there */
>>>> + cmd.wlen = 0;
>>>> + cmd.rlen = 1;
>>>> + ret = si2157_cmd_execute(client, &cmd);
>>>> + /* verify no i2c error and CTS is set */
>>>> + if (ret) {
>>>> + dev_warn(&client->dev, "no HW found ret=%d\n", ret);
>>>> + goto fail_instance;
>>>> + }
>>>> +
>>>> + memcpy(&fe->ops.tuner_ops, &si2157_ops, sizeof(struct
>>>> dvb_tuner_ops));
>>>> +
>>>> +#ifdef CONFIG_MEDIA_CONTROLLER
>>>> + if (instance == 1 && cfg->mdev) {
>>>> + dev->mdev = cfg->mdev;
>>>> +
>>>> + dev->ent.name = KBUILD_MODNAME;
>>>> + dev->ent.function = MEDIA_ENT_F_TUNER;
>>>> +
>>>> + dev->pad[TUNER_PAD_RF_INPUT].flags = MEDIA_PAD_FL_SINK;
>>>> + dev->pad[TUNER_PAD_OUTPUT].flags = MEDIA_PAD_FL_SOURCE;
>>>> + dev->pad[TUNER_PAD_AUD_OUT].flags = MEDIA_PAD_FL_SOURCE;
>>>> +
>>>> + ret = media_entity_pads_init(&dev->ent, TUNER_NUM_PADS,
>>>> + &dev->pad[0]);
>>>> +
>>>> + if (ret)
>>>> + goto fail_instance;
>>>> +
>>>> + ret = media_device_register_entity(cfg->mdev, &dev->ent);
>>>> + if (ret) {
>>>> + dev_warn(&client->dev,
>>>> + "media_device_regiser_entity returns %d\n", ret);
>>>> + media_entity_cleanup(&dev->ent);
>>>> + goto fail_instance;
>>>> + }
>>>> + }
>>>> +#endif
>>>> + mutex_unlock(&si2157_list_mutex);
>>>> +
>>>> + if (instance != 1)
>>>> + dev_info(&client->dev, "Silicon Labs %s successfully
>>>> attached\n",
>>>> + dev->chiptype == SI2157_CHIPTYPE_SI2141 ? "Si2141" :
>>>> + dev->chiptype == SI2157_CHIPTYPE_SI2146 ?
>>>> + "Si2146" : "Si2147/2148/2157/2158");
>>>> +
>>>> + return fe;
>>>> +fail_instance:
>>>> + mutex_unlock(&si2157_list_mutex);
>>>> +
>>>> + si2157_release(fe);
>>>> +fail:
>>>> + dev_warn(&client->dev, "Attach failed\n");
>>>> + return NULL;
>>>> +}
>>>> +EXPORT_SYMBOL(si2157_attach);
>>>> +
>>>> +MODULE_DESCRIPTION("Silicon Labs Si2141/2146/2147/2148/2157/2158
>>>> silicon tuner driver");
>>>> MODULE_AUTHOR("Antti Palosaari <crope@xxxxxx>");
>>>> MODULE_LICENSE("GPL");
>>>> MODULE_FIRMWARE(SI2158_A20_FIRMWARE);
>>>> diff --git a/drivers/media/tuners/si2157.h
>>>> b/drivers/media/tuners/si2157.h
>>>> index de597fa..26b94ca 100644
>>>> --- a/drivers/media/tuners/si2157.h
>>>> +++ b/drivers/media/tuners/si2157.h
>>>> @@ -46,4 +46,18 @@ struct si2157_config {
>>>> u8 if_port;
>>>> };
>>>> +#if IS_REACHABLE(CONFIG_MEDIA_TUNER_SI2157)
>>>> +extern struct dvb_frontend *si2157_attach(struct dvb_frontend *fe,
>>>> u8 addr,
>>>> + struct i2c_adapter *i2c,
>>>> + struct si2157_config *cfg);
>>>> +#else
>>>> +static inline struct dvb_frontend *si2157_attach(struct dvb_frontend
>>>> *fe,
>>>> + u8 addr,
>>>> + struct i2c_adapter *i2c,
>>>> + struct si2157_config *cfg)
>>>> +{
>>>> + pr_err("%s: driver disabled by Kconfig\n", __func__);
>>>> + return NULL;
>>>> +}
>>>> +#endif
>>>> #endif
>>>> diff --git a/drivers/media/tuners/si2157_priv.h
>>>> b/drivers/media/tuners/si2157_priv.h
>>>> index e6436f7..2801aaa 100644
>>>> --- a/drivers/media/tuners/si2157_priv.h
>>>> +++ b/drivers/media/tuners/si2157_priv.h
>>>> @@ -19,15 +19,20 @@
>>>> #include <linux/firmware.h>
>>>> #include <media/v4l2-mc.h>
>>>> +#include "tuner-i2c.h"
>>>> #include "si2157.h"
>>>> /* state struct */
>>>> struct si2157_dev {
>>>> + struct list_head hybrid_tuner_instance_list;
>>>> + struct tuner_i2c_props i2c_props;
>>>> +
>>>> struct mutex i2c_mutex;
>>>> struct dvb_frontend *fe;
>>>> bool active;
>>>> bool inversion;
>>>> u8 chiptype;
>>>> + u8 addr;
>>>> u8 if_port;
>>>> u32 if_frequency;
>>>> struct delayed_work stat_work;
>>>>
>>>
>
>
> Thanks,
> Mauro
