Oliver Endriss wrote: > Mauro Carvalho Chehab wrote: >> Oliver Endriss wrote: >>> Manu Abraham wrote: >>>> On Mon, Jan 11, 2010 at 12:17 AM, Manu Abraham <abraham.manu@xxxxxxxxx> wrote: >>>>> Mauro, >>>>> >>>>> >>>>> Following the changesets 13830 - 13894 from my earlier pull request, >>>> Correction, 13880 - 13894 inclusive of both. >>>> >>>>> Please pull the following changeset as well >>>>> >>>>> >>>>> http://jusst.de/hg/stv090x/rev/80c74966d955 >>>>> >>>>> >>>>> It fixes a nasty bug as described at >>>>> http://osdir.com/ml/linux-media/2009-11/msg00643.html >>>>> >>>>> >>>>> Regards, >>>>> Manu >>> Mauro, >>> >>> when will you pull-in these changesets? >>> You are blocking the submission of the ngene driver. >> Hi Oliver, >> >> Since I returned from vacations this week, I had a pile of tasks to >> handle, both at the subsystem and at the work. >> >> Unfortunately, having to deal with both -hg and -git eats lot of my >> time, to be sure that nothing is missed. This time, I had to re-generate >> all my local -git trees that somehow had loose some patches in the process. >> Due to that, the patch merge is taking longer than I've expected. >> >> I still have a few issues pending at the subsystem, including this stv090x >> pull request, the omap pull (that it is very complex, as it requires both >> -hg and -git patch insertions), lnbh24 fix, my own proposals to dvb-apps, >> and two upstream submissions. >> >> My intention is to finish those tasks during this weekend if time >> allows me to do everything. >> >> In the specific case of stv090x, I intend to review again the locks, >> in the light of your comments. > > I just wondered why you did not pull-in these changesets. I finished reviewing the locking schema and it seemed OK with your fixes. Still, I think that the i2c_gate_control function is poorly documented. Somehow, it should be commented that the lock is taken when enable=1 and is dropped when enable=0, warning anyone that may be analyzing the code about that, especially since the lock function is exported to dvb core. It is not common to use a locking schema like that, where the information that the code is locked is hidden. IMO, the better way would be to split the function in two, like: i2c_gate_enable_lock(struct dvb_frontend *fe); i2c_gate_disable_unlock(struct dvb_frontend *fe); And use those two functions internally. You'll still need to have an i2c_gate_control() function that has the locks inside, due to the calls done at dvb_frontend.c, but the code will be better documented. In order to not add more delay to ngene, I'll apply the stv090x patches, but please better document the i2c_gate_control on your next patch series. Btw, with respect to dvb-core, we'll need to do soon a lock review, since dvbdev.c uses the BKL, and it will be removed on 2.6.34 or 2.6.35. > > The first lnbh24 patch was pulled so fast, that I had no chance to > respond. This patch was submitted much later than the stv090x stuff. > Btw, it breaks the ngene driver, so it is important to apply the > lnbh24 fix... Yes, it were submitted latter, but I've reviewed stv090x before lnbh24. Anyway, it is the next on my review list. > > CU > Oliver > -- 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
