Re: [PATCH] tda18271-common: hold the I2C adapter during write transfers

[Antti Palosaari <crope@iki.fi>]

On 10/01/2012 01:42 PM, Mauro Carvalho Chehab wrote:
> Em Fri, 28 Sep 2012 21:31:07 +0300
> Antti Palosaari <crope@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.

> 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!

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.

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
-- 
http://palosaari.fi/