On Mon, Nov 21, 2011 at 4:28 PM, Manu Abraham <abraham.manu@xxxxxxxxx> wrote:
> On 11/22/11, Michael Krufky <mkrufky@xxxxxxxxxxx> wrote:
>> Thank you, Manu... After the Linux Kernel Summit in Prague, I had
>> intentions of solving this exact problem, but you did it first -- good
>> job!
>>
>> I have reviewed the patch to the tda18271 driver, and the changes make
>> good sense to me. I have one question, however:
>>
>> Perhaps my eyes have overlooked something -- I fail to see any code
>> that defines the new "set_state" callback or any code that calls this
>> new callback within dvb-core (assuming dvb_frontend.c) I also can't
>> find the structure declaration of the "tuner_state" struct... ... is
>> this patch missing from your series, or did I just overlook it?
>
> I guess more like that. The data structure existed for quite a long
> while in dvb_frontend.h and hence you don't find any new changes. Only
> delivery and modulation added to it.
>
>>
>> That missing patch is what interests me most. Once I can see that
>> missing code, I'd like to begin discussion on whether we actually need
>> the additional callback, or if it can simply be handled by the
>> set_params call. Likewise, I'm not exactly sure why we need this
>> affional "struct tuner_state" ... Perhaps the answer will be
>> self-explanatory once I see the code - maybe no discussion is
>> necessary :-P
>>
>> But this does look good to me so far. I'd be happy to provide my
>> "reviewed-by" tag once I can see the missing code mentioned above.
>
> The callback is used from within a demodulator context as usual and hence.
> eg:
>
> /* program tuner */
> - if (fe->ops.tuner_ops.set_params)
> - fe->ops.tuner_ops.set_params(fe, params);
> + tstate.delsys = SYS_DVBC_ANNEX_AC;
> + tstate.frequency = c->frequency;
> +
> + if (fe->ops.tuner_ops.set_state) {
> + fe->ops.tuner_ops.set_state(fe,
> + DVBFE_TUNER_DELSYS |
> + DVBFE_TUNER_FREQUENCY,
> + &tstate);
> + } else {
> + if (fe->ops.tuner_ops.set_params)
> + fe->ops.tuner_ops.set_params(fe, params);
> + }
>
>
> Best Regards,
> Manu
>
Manu,
Thank you for explaining -- I found that structure in dvb_frontend.h,
now that you've pointed that out.
I am on board with this change -- it is a positive move in the right
direction. I believe that after this is merged, we may be able to
obsolete and remove the set_params callback. In fact, we can even
obsolete the set_analog_params callback as well, using set_state as
the single entry point for setting the tuner. Of course, one step at
a time -- this is great for now. We should consider the other
optimizations after this has been merged and tested. :-)
Reviewed-by: Michael Krufky <mkrufky@xxxxxxxxxxx>
--
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
