On Fri, Jan 22, 2010 at 9:48 PM, Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxx> wrote: > 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. It's in fact a Finite State Machine or FSM as popularly known ... http://en.wikipedia.org/wiki/Finite_State_Machine >> >> >>> 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. Converting to a function a FSM, will definitely cause a confusion. Maybe an explanation why a FSM is needed there would be a better thing. 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
