On 12/12/2011 02:55 PM, Manu Abraham wrote:
On Mon, Dec 12, 2011 at 6:13 PM, Andreas Oberritter<obi@xxxxxxxxxxx> wrote:
On 12.12.2011 05:28, Manu Abraham wrote:
On Sat, Dec 10, 2011 at 5:17 PM, Antti Palosaari<crope@xxxxxx> wrote:
On 12/10/2011 06:44 AM, Manu Abraham wrote:
static int cxd2820r_set_frontend(struct dvb_frontend *fe,
[...]
+ switch (c->delivery_system) {
+ case SYS_DVBT:
+ ret = cxd2820r_init_t(fe);
+ ret = cxd2820r_set_frontend_t(fe, p);
Anyhow, I don't now like idea you have put .init() calls to .set_frontend().
Could you move .init() happen in .init() callback as it was earlier?
This was there in the earlier patch as well. Maybe you have a
new issue now ? ;-)
You mean I didn't mentioned it when you send first version? Sorry, I
didn't looked it very carefully since I first meet stuff that was not
related whole thing, I mean there was that code changing from
.set_params() to .set_state(). And I stopped reading rest of the patch.
ok.
The argument what you make doesn't hold well, Why ?
int cxd2820r_init_t(struct dvb_frontend *fe)
{
ret = cxd2820r_wr_reg(priv, 0x00085, 0x07);
}
int cxd2820r_init_c(struct dvb_frontend *fe)
{
ret = cxd2820r_wr_reg(priv, 0x00085, 0x07);
}
Now, you might like to point that, the Base I2C address location
is different comparing DVB-T/DVBT2 to DVB-C
So, If you have the init as in earlier with a common init, then you
will likely init the wrong device at .init(), as init is called open().
So, this might result in an additional register write, which could
be avoided altogether. One register access is not definitely
something to brag about, but is definitely a small incremental
difference. Other than that this register write doesn't do anything
more than an ADC_START. So starting the ADC at init doesn't
make sense. But does so when you want to select the right ADC.
So definitely, this change is an improvement. Also, you can
compare the time taken for the device to tune now. It is quite
a lot faster compared to without this patch. So you or any other
user should be happy. :-)
I don't think that in any way, the init should be used at init as
you say, which sounds pretty much incorrect.
Maybe the function names should be modified to avoid confusion with the
init driver callback.
On another tangential thought, Is it really worth to wrap that single
register write with another function name ?
instead of the current usage; ie,
ret = cxd2820r_wr_reg(priv, 0x00085, 0x07); /* Start ADC */
within set_frontend()
in set_frontend(), another thing that's wrapped up similarly is
the set_frontend() within the search() callback, which causes
another set of confusions within the driver.
Actually there was was a lot more code first but because I ran problems
selsys needed for T/T2 init was not known at the time .init() was called
I moved those set_frontend. I left that in a hope I can later fix
properly adding more stuff back to init.
That is not functionality issue, it is issue about naming callbacks and
what is functionality of each callback.
As for these days it have been in my understanding initialization stuff
are done in .init() and leave as less as possible code to
.set_frontend(). Leaving set_frontend() handle only tuning requests and
reconfigure IF control etc. And if you look most demod drivers there is
rather similar logic used.
So I would like to ask what is meaning of:
.attach()
* create FE
* no HW init
* as less as possible HW I/O, mainly reading chip ID and nothing more
.init()
* do nothing here?
* download firmware?
.set_frontend()
* program tuner
* init demod?
* tune demod
* download firmware?
.sleep()
* put device sleep mode
* powersave
After all it is just fine for me apply that patch, but I would like to
get clear idea what is meaning of every single callback we have. And if
we really end up .init() is not needed and all should be put to
.set_frontend() when possible it means I have to change all my demod
drivers and maybe tuner drivers too.
regards
Antti
--
http://palosaari.fi/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
