On Sat, Jan 16, 2010 at 5:14 PM, Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxx> wrote: > Hi Andreas, > > Andreas Regel wrote: > >> you don't have to look at stv090x_i2c_gate_ctrl alone. stv090x driver >> contains code like this: >> >> if (stv090x_i2c_gate_ctrl(fe, 1) < 0) >> goto err; >> >> tuner access >> >> if (stv090x_i2c_gate_ctrl(fe, 0) < 0) >> goto err; > > Ok. It is very unusual to have a lock internally like the above, since > the code becomes poorly documented. That's how a tuner is accessed for "any" dvb device. > In the cases that we want to do things like that, it is documented like: > > if (stv090x_gate_enable_lock(fe) < 0) > goto err; > > tuner access > > if (stv090x_gate_disable_unlock(fe, 0) < 0) > goto err; To me, that looks horrible and do non-intuitive. I would have to read those functions additionally to understand the implications of the same. What you suggest, will look to a user at initial glance it would seem that you are trying to lock register access, while it is not. I don't agree with your comment on this one. > This way, it is clear to anyone that is reviewing the code that the functions > are returning with the lock on a different state and prevent the kernel janitors > to send patches that will try to "fix" the lock. > > I bet that their coccinelle rules will generate some false alarms with the current code. We will wait and see, since there is nothing wrong in the semantic construct in there. It is just that the gate control for that device is slightly different from other devices, due to the difference in the device type and the mode of operation. Regards, Manu -- 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
