Manu Abraham wrote: > 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. Yes, but that's not a function is expect to behave. In general, functions handle the lock/unlock inside it, returning the mutex unlocked. In this case, the function returns with a mutex on a different state. This should be documented somehow. The better is to document at the name of the function, as on other cases along the kernel. > > >> 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. Some examples of similar functions on kernel: spin_lock_irqsave/spin_unlock_irqrestore test_and_set_bit_lock/clear_bit_unlock ttm_read_lock/ttm_read_unlock queue_flag_test_and_clear ... > I don't agree with your comment on this one. If you didn't like the name, feel free to use another one, but this needs to be documented somehow. Maybe you could use, for example: tuner_lock/tuner_unlock tuner_opengate_lock/tuner_closegate_unlock ... Cheers, Mauro. -- 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
