| Message ID | 20150526150407.10241.89123.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26/05/15 16:04, David Howells wrote: > Provide a DVBv5 API signal strength. This is in units of 0.001 dBm rather > than a percentage. > >>From Antti Palosaari's testing with a signal generator, it appears that the > gain calculated according to Montage's specification if negated is a > reasonable representation of the signal strength of the generator. > > To this end: > > (1) Polled statistic gathering needed to be implemented in the TS2020 driver. > This is done in the ts2020_stat_work() function. > > (2) The calculated gain is placed as the signal strength in the > dtv_property_cache associated with the front end with the scale set to > FE_SCALE_DECIBEL. > > (3) The DVBv3 format signal strength then needed to be calculated from the > signal strength stored in the dtv_property_cache rather than accessing > the value when ts2020_read_signal_strength() is called. > > Signed-off-by: David Howells <dhowells@redhat.com> > --- > > drivers/media/dvb-frontends/ts2020.c | 62 +++++++++++++++++++++++++++++----- > 1 file changed, 53 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/dvb-frontends/ts2020.c b/drivers/media/dvb-frontends/ts2020.c > index 277e1cf..80ae039 100644 > --- a/drivers/media/dvb-frontends/ts2020.c > +++ b/drivers/media/dvb-frontends/ts2020.c > @@ -32,10 +32,11 @@ struct ts2020_priv { > struct regmap_config regmap_config; > struct regmap *regmap; > struct dvb_frontend *fe; > + struct delayed_work stat_work; > int (*get_agc_pwm)(struct dvb_frontend *fe, u8 *_agc_pwm); > /* i2c details */ > - int i2c_address; > struct i2c_adapter *i2c; > + int i2c_address; > u8 clk_out:2; > u8 clk_out_div:5; > u32 frequency_div; /* LO output divider switch frequency */ > @@ -65,6 +66,7 @@ static int ts2020_release(struct dvb_frontend *fe) > static int ts2020_sleep(struct dvb_frontend *fe) > { > struct ts2020_priv *priv = fe->tuner_priv; > + int ret; > u8 u8tmp; > > if (priv->tuner == TS2020_M88TS2020) > @@ -72,11 +74,18 @@ static int ts2020_sleep(struct dvb_frontend *fe) > else > u8tmp = 0x00; > > - return regmap_write(priv->regmap, u8tmp, 0x00); > + ret = regmap_write(priv->regmap, u8tmp, 0x00); > + if (ret < 0) > + return ret; > + > + /* stop statistics polling */ > + cancel_delayed_work_sync(&priv->stat_work); > + return 0; > } > > static int ts2020_init(struct dvb_frontend *fe) > { > + struct dtv_frontend_properties *c = &fe->dtv_property_cache; > struct ts2020_priv *priv = fe->tuner_priv; > int i; > u8 u8tmp; > @@ -138,6 +147,13 @@ static int ts2020_init(struct dvb_frontend *fe) > reg_vals[i].val); > } > > + /* Initialise v5 stats here */ > + c->strength.len = 1; > + c->strength.stat[0].scale = FE_SCALE_DECIBEL; > + c->strength.stat[0].uvalue = 0; > + > + /* Start statistics polling */ > + schedule_delayed_work(&priv->stat_work, 0); > return 0; > } > Hi David Statistics polling can not be done by lmedm04 driver's implementation of M88RS2000/TS2020 because I2C messages stop the devices demuxer. So any polling must be a config option for this driver. Regards Malcolm -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Malcolm Priestley <tvboxspy@gmail.com> wrote: > Statistics polling can not be done by lmedm04 driver's implementation of > M88RS2000/TS2020 because I2C messages stop the devices demuxer. > > So any polling must be a config option for this driver. Ummm... I presume a runtime config option is okay. Also, does that mean that the lmedm04 driver can't be made compatible with the DVBv5 API? David -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28/05/15 11:08, David Howells wrote: > Malcolm Priestley <tvboxspy@gmail.com> wrote: > >> Statistics polling can not be done by lmedm04 driver's implementation of >> M88RS2000/TS2020 because I2C messages stop the devices demuxer. >> >> So any polling must be a config option for this driver. > > Ummm... I presume a runtime config option is okay. Yes, also, the workqueue appears not to be initialized when using the dvb attached method. > > Also, does that mean that the lmedm04 driver can't be made compatible with the > DVBv5 API? No, the driver will have to implement its own version. It doesn't need a polling thread it simply gets it directly from its interrupt urb buffer. Malcolm -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Malcolm Priestley <tvboxspy@gmail.com> wrote: > Yes, also, the workqueue appears not to be initialized when using the dvb > attached method. I'm not sure what you're referring to. It's initialised in ts2020_probe() just after the ts2020_priv struct is allocated - the only place it is allocated. David -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/28/2015 11:07 PM, Malcolm Priestley wrote: > On 28/05/15 11:08, David Howells wrote: >> Malcolm Priestley <tvboxspy@gmail.com> wrote: >> >>> Statistics polling can not be done by lmedm04 driver's implementation of >>> M88RS2000/TS2020 because I2C messages stop the devices demuxer. I did make tests (using that same lme2510 + rs2000 device) and didn't saw the issue TS was lost. Could test and and tell me how to reproduce it? Signal strength returned was quite boring though, about same value all the time, but it is different issue... >>> >>> So any polling must be a config option for this driver. >> >> Ummm... I presume a runtime config option is okay. > > Yes, also, the workqueue appears not to be initialized when using the > dvb attached method. > >> >> Also, does that mean that the lmedm04 driver can't be made compatible >> with the >> DVBv5 API? > > No, the driver will have to implement its own version. It doesn't need a > polling thread it simply gets it directly from its interrupt urb buffer. I assume lme2510 firmware will read signal strength from rs2000 and it is returned then directly by USB interface. regards Antti
On 03/06/15 11:17, David Howells wrote: > Malcolm Priestley <tvboxspy@gmail.com> wrote: > >> Yes, also, the workqueue appears not to be initialized when using the dvb >> attached method. > > I'm not sure what you're referring to. It's initialised in ts2020_probe() > just after the ts2020_priv struct is allocated - the only place it is > allocated. > ts2020_probe() isn't touched by devices not converted to I2C binding. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/06/15 12:13, Antti Palosaari wrote: > On 05/28/2015 11:07 PM, Malcolm Priestley wrote: >> On 28/05/15 11:08, David Howells wrote: >>> Malcolm Priestley <tvboxspy@gmail.com> wrote: >>> >>>> Statistics polling can not be done by lmedm04 driver's >>>> implementation of >>>> M88RS2000/TS2020 because I2C messages stop the devices demuxer. > > I did make tests (using that same lme2510 + rs2000 device) and didn't > saw the issue TS was lost. Could test and and tell me how to reproduce it? > Signal strength returned was quite boring though, about same value all > the time, but it is different issue... Hi Antti The workqueue is not working because ts2020_probe() isn't called. I am thinking that other drivers that still use dvb_attach may be broken. It will become an issue when the driver is converted to I2C binding. Regards Malcolm -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Malcolm Priestley <tvboxspy@gmail.com> wrote: > >> Yes, also, the workqueue appears not to be initialized when using the dvb > >> attached method. > > > > I'm not sure what you're referring to. It's initialised in ts2020_probe() > > just after the ts2020_priv struct is allocated - the only place it is > > allocated. > > > ts2020_probe() isn't touched by devices not converted to I2C binding. Hmmm... Doesn't that expose a larger problem? The only place the ts2020_priv struct is allocated is in ts2020_probe() within ts2020.c and the struct definition is private to that file and so it can't be allocated from outside. So if you don't pass through ts2020_probe(), fe->tuner_priv will remain NULL and the driver will crash. David -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/03/2015 07:37 PM, David Howells wrote: > Malcolm Priestley <tvboxspy@gmail.com> wrote: > >>>> Yes, also, the workqueue appears not to be initialized when using the dvb >>>> attached method. >>> >>> I'm not sure what you're referring to. It's initialised in ts2020_probe() >>> just after the ts2020_priv struct is allocated - the only place it is >>> allocated. >>> >> ts2020_probe() isn't touched by devices not converted to I2C binding. > > Hmmm... Doesn't that expose a larger problem? The only place the ts2020_priv > struct is allocated is in ts2020_probe() within ts2020.c and the struct > definition is private to that file and so it can't be allocated from outside. > So if you don't pass through ts2020_probe(), fe->tuner_priv will remain NULL > and the driver will crash. Malcolm misses some pending patches where attach() is wrapped to I2C model probe(). http://git.linuxtv.org/cgit.cgi/anttip/media_tree.git/log/?h=ts2020 regards Antti
On 03/06/15 17:37, David Howells wrote: > Malcolm Priestley <tvboxspy@gmail.com> wrote: > >>>> Yes, also, the workqueue appears not to be initialized when using the dvb >>>> attached method. >>> >>> I'm not sure what you're referring to. It's initialised in ts2020_probe() >>> just after the ts2020_priv struct is allocated - the only place it is >>> allocated. >>> >> ts2020_probe() isn't touched by devices not converted to I2C binding. > > Hmmm... Doesn't that expose a larger problem? The only place the ts2020_priv > struct is allocated is in ts2020_probe() within ts2020.c and the struct > definition is private to that file and so it can't be allocated from outside. > So if you don't pass through ts2020_probe(), fe->tuner_priv will remain NULL > and the driver will crash. > No, it is also allocated in ts2020_attach. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/06/15 17:43, Antti Palosaari wrote: > On 06/03/2015 07:37 PM, David Howells wrote: >> Malcolm Priestley <tvboxspy@gmail.com> wrote: >> >>>>> Yes, also, the workqueue appears not to be initialized when using >>>>> the dvb >>>>> attached method. >>>> >>>> I'm not sure what you're referring to. It's initialised in >>>> ts2020_probe() >>>> just after the ts2020_priv struct is allocated - the only place it is >>>> allocated. >>>> >>> ts2020_probe() isn't touched by devices not converted to I2C binding. >> >> Hmmm... Doesn't that expose a larger problem? The only place the >> ts2020_priv >> struct is allocated is in ts2020_probe() within ts2020.c and the struct >> definition is private to that file and so it can't be allocated from >> outside. >> So if you don't pass through ts2020_probe(), fe->tuner_priv will >> remain NULL >> and the driver will crash. > > Malcolm misses some pending patches where attach() is wrapped to I2C > model probe(). > http://git.linuxtv.org/cgit.cgi/anttip/media_tree.git/log/?h=ts2020 > Hmmm... Yes, I am indeed missing those patches. I will test. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Antti Palosaari <crope@iki.fi> wrote: > Malcolm misses some pending patches where attach() is wrapped to I2C model > probe(). > http://git.linuxtv.org/cgit.cgi/anttip/media_tree.git/log/?h=ts2020 Aha! That explains it. ts2020: register I2C driver from legacy media attach removes the allocation from attach() in the branch I'm working on top of. David -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/dvb-frontends/ts2020.c b/drivers/media/dvb-frontends/ts2020.c index 277e1cf..80ae039 100644 --- a/drivers/media/dvb-frontends/ts2020.c +++ b/drivers/media/dvb-frontends/ts2020.c @@ -32,10 +32,11 @@ struct ts2020_priv { struct regmap_config regmap_config; struct regmap *regmap; struct dvb_frontend *fe; + struct delayed_work stat_work; int (*get_agc_pwm)(struct dvb_frontend *fe, u8 *_agc_pwm); /* i2c details */ - int i2c_address; struct i2c_adapter *i2c; + int i2c_address; u8 clk_out:2; u8 clk_out_div:5; u32 frequency_div; /* LO output divider switch frequency */ @@ -65,6 +66,7 @@ static int ts2020_release(struct dvb_frontend *fe) static int ts2020_sleep(struct dvb_frontend *fe) { struct ts2020_priv *priv = fe->tuner_priv; + int ret; u8 u8tmp; if (priv->tuner == TS2020_M88TS2020) @@ -72,11 +74,18 @@ static int ts2020_sleep(struct dvb_frontend *fe) else u8tmp = 0x00; - return regmap_write(priv->regmap, u8tmp, 0x00); + ret = regmap_write(priv->regmap, u8tmp, 0x00); + if (ret < 0) + return ret; + + /* stop statistics polling */ + cancel_delayed_work_sync(&priv->stat_work); + return 0; } static int ts2020_init(struct dvb_frontend *fe) { + struct dtv_frontend_properties *c = &fe->dtv_property_cache; struct ts2020_priv *priv = fe->tuner_priv; int i; u8 u8tmp; @@ -138,6 +147,13 @@ static int ts2020_init(struct dvb_frontend *fe) reg_vals[i].val); } + /* Initialise v5 stats here */ + c->strength.len = 1; + c->strength.stat[0].scale = FE_SCALE_DECIBEL; + c->strength.stat[0].uvalue = 0; + + /* Start statistics polling */ + schedule_delayed_work(&priv->stat_work, 0); return 0; } @@ -411,19 +427,46 @@ static int ts2020_get_tuner_gain(struct dvb_frontend *fe, __s64 *_gain) } /* + * Gather statistics on a regular basis + */ +static void ts2020_stat_work(struct work_struct *work) +{ + struct ts2020_priv *priv = container_of(work, struct ts2020_priv, + stat_work.work); + struct i2c_client *client = priv->client; + struct dtv_frontend_properties *c = &priv->fe->dtv_property_cache; + int ret; + + dev_dbg(&client->dev, "\n"); + + ret = ts2020_get_tuner_gain(priv->fe, &c->strength.stat[0].svalue); + if (ret < 0) + goto err; + + c->strength.stat[0].scale = FE_SCALE_DECIBEL; + + schedule_delayed_work(&priv->stat_work, msecs_to_jiffies(2000)); + return; +err: + dev_dbg(&client->dev, "failed=%d\n", ret); +} + +/* * Read TS2020 signal strength in v3 format. */ static int ts2020_read_signal_strength(struct dvb_frontend *fe, - u16 *signal_strength) + u16 *_signal_strength) { + struct dtv_frontend_properties *c = &fe->dtv_property_cache; unsigned strength; __s64 gain; - int ret; - /* Determine the total gain of the tuner */ - ret = ts2020_get_tuner_gain(fe, &gain); - if (ret < 0) - return ret; + if (c->strength.stat[0].scale == FE_SCALE_NOT_AVAILABLE) { + *_signal_strength = 0; + return 0; + } + + gain = c->strength.stat[0].svalue; /* Calculate the signal strength based on the total gain of the tuner */ if (gain < -85000) @@ -439,7 +482,7 @@ static int ts2020_read_signal_strength(struct dvb_frontend *fe, /* 90% - 99%: strong signal */ strength = 90 + (45000 + gain) / 5000; - *signal_strength = strength * 65535 / 100; + *_signal_strength = strength * 65535 / 100; return 0; } @@ -546,6 +589,7 @@ static int ts2020_probe(struct i2c_client *client, dev->get_agc_pwm = pdata->get_agc_pwm; fe->tuner_priv = dev; dev->client = client; + INIT_DELAYED_WORK(&dev->stat_work, ts2020_stat_work); /* check if the tuner is there */ ret = regmap_read(dev->regmap, 0x00, &utmp);