Em 01-10-2012 08:31, Antti Palosaari escreveu:
> On 10/01/2012 01:42 PM, Mauro Carvalho Chehab wrote:
>> Em Fri, 28 Sep 2012 21:31:07 +0300
>> Antti Palosaari <cr...@iki.fi> escreveu:
>>
>>> Hello,
>>> Did not fix the issue. Problem remains same.
>>
>> Ok, that's what I was afraid: there's likely something at drxk firmware that
>> it is needed for tda18271 to be visible - maybe gpio settings.
>>
>>> With the sleep + that patch
>>> it works still.
>>
>> Good, no regressions added.
>
> Currently there is regression as you haven't committed that sleep patch.
Yes, I won't apply it before we finish those discussions.
>> IMO, we should add a defer job at dvb_attach, that will postpone the
>> tuner attach to happen after drxk firmware is loaded, or add there a
>> wait queue. Still, I think that this patch is needed anyway, in order
>> to avoid race conditions with CI and Remote Controller polls that may
>> affect devices with tda18271.
>>
>> It should be easy to check if the firmware is loaded: all it is needed
>> is to, for example, call:
>> drxk_ops.get_tune_settings()
>>
>> This function returns -ENODEV if firmware load fails; -EAGAIN if
>> firmware was not loaded yet and 0 or -EINVAL if firmware is OK.
>>
>> So, instead of using sleep, you could do:
>>
>> static bool is_drxk_firmware_loaded(struct dvb_frontend *fe) {
>> struct dvb_frontend_tune_settings sets;
>> int ret = fe->ops.get_tune_settings(fe, &sets);
>>
>> if (ret == -ENODEV || ret == -EAGAIN)
>> return false;
>> else
>> return true;
>> };
>>
>> and, at the place you coded the sleep(), replace it by:
>>
>> ret = wait_event_interruptible(waitq,
>> is_drxk_firmware_loaded(dev->dvb->fe[0]));
>> if (ret < 0) {
>> dvb_frontend_detach(dev->dvb->fe[0]);
>> dev->dvb->fe[0] = NULL;
>> return -EINVAL;
>> }
>>
>> It might have sense to add an special callback to return the tuner
>> state (firmware not loaded, firmware loaded, firmware load failed).
>
> This is stupid approach. It does not change the original behavior which was
> we are not allowed to block module init path.
> It blocks module init just as long as earlier, even longer, with increased
> code complexity!
Not really. em28xx-dvb is loaded/attached asynchronously, already:
static void request_module_async(struct work_struct *work)
{
struct em28xx *dev = container_of(work,
struct em28xx, request_module_wk);
if (dev->has_audio_class)
request_module("snd-usb-audio");
else if (dev->has_alsa_audio)
request_module("em28xx-alsa");
if (dev->board.has_dvb)
request_module("em28xx-dvb");
if (dev->board.ir_codes && !disable_ir)
request_module("em28xx-rc");
}
static void request_modules(struct em28xx *dev)
{
INIT_WORK(&dev->request_module_wk, request_module_async);
schedule_work(&dev->request_module_wk);
}
(one small change there is actually needed, for the case where the
driver is built-in)
> Why the hell you want add this kind of hacks every single chip driver that
> downloads firmware? Instead, put it to the bridge and leave demod, tuner,
> sec, etc, drivers free.
A sleep() hack is worse. Firmware load can take up to 60 seconds.
Btw, I know one atom-based hardware where firmware load can actually
take a lot more than 2 seconds to happen, when the root fs is mounted
via nfs.
>
> If you put that asyncronous stuff to em28xx (with possible dev unregister if
> you wish to be elegant) then all the rest sub-drivers could be hack free.
>
> regards
> Antti
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
