Re: [PATCH 1/3] MFD: WL1273 FM Radio: MFD driver for the FM radio.
Hello.
Thanks for the comments.
On Tue, 2010-04-20 at 21:33 +0200, ext Jonathan Corbet wrote:
On Tue, 20 Apr 2010 18:20:05 +0300
Matti J. Aaltonen matti.j.aalto...@nokia.com wrote:
This is a parent driver for two child drivers: the V4L2 driver and
the ALSA codec driver. The MFD part provides the I2C communication
to the device and the state handling.
So I was taking a quick look at this; it mostly looks OK (though I wonder
about all those symbol exports - does all that stuff need to be in the
Some functions get called from both child drivers/modules, but some
stuff could probably be moved from the core to either of the children.
Should I actually do that?
core?). One thing caught my eye, though:
+int wl1273_fm_rds_off(struct wl1273_core *core)
+{
+ struct device *dev = core-i2c_dev-dev;
+ int r;
+
+ if (core-mode == WL1273_MODE_TX)
+ return 0;
+
+ wait_for_completion_timeout(core-busy, msecs_to_jiffies(1000));
The use of a completion here looks like a mistake to me. This isn't a
normal pattern, and I'm not quite sure what you're trying to do. Here,
also, you're ignoring the return value, so you don't know if completion
was signaled or not.
Yes that wait_for_completion is a mistake.
If you look in functions like:
+int wl1273_fm_set_rx_freq(struct wl1273_core *core, unsigned int freq)
+{
[...]
+ INIT_COMPLETION(core-busy);
+ r = wl1273_fm_write_cmd(core, WL1273_TUNER_MODE_SET,
+ TUNER_MODE_PRESET);
+ if (r) {
+ complete(core-busy);
+ goto err;
+ }
What will prevent multiple threads from doing INIT_COMPLETION()
simultaneously? It looks racy to me. What happens if multiple
threads try to wait simultaneously on the completion?
OK, looking further, you're not using it for mutual exclusion. In fact,
you're not using anything for mutual exclusion; how do you prevent
concurrent access to the hardware registers?
I have mutexes everywhere that function is called from. My aim was to
have the mutexes in the interface functions. So that the radio use is
serialized on as high a level as possible.
What I would suggest you do is remove the completion in favor of a wait
queue which the interrupt handler can use to signal that something has
completed. You can then use wait_event() to wait for a wakeup and test
that the specific condition you are waiting for has come to pass.
Do you agree with my explanation? Or should I switch to using wait
queue?
Cheers,
Matti
jon
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] MFD: WL1273 FM Radio: MFD driver for the FM radio.
On Wed, 21 Apr 2010 12:29:00 +0300 m7aalton matti.j.aalto...@nokia.com wrote: So I was taking a quick look at this; it mostly looks OK (though I wonder about all those symbol exports - does all that stuff need to be in the Some functions get called from both child drivers/modules, but some stuff could probably be moved from the core to either of the children. Should I actually do that? Depends. If it's truly only useful to a single child device, the code probably belongs there, without an export. If it's truly a core function, in that (1) it's applicable to multiple devices, or (2) it can't be implemented without exposing stuff you want to keep private to the core, then it should stay in the core. What I would suggest you do is remove the completion in favor of a wait queue which the interrupt handler can use to signal that something has completed. You can then use wait_event() to wait for a wakeup and test that the specific condition you are waiting for has come to pass. Do you agree with my explanation? Or should I switch to using wait queue? My belief is that the code would be cleaner with a wait queue; that's the normal pattern for implementing this kind of logic. I'll stop here, though; if others want to take it upstream with the completion, I'll not scream about it. Thanks, jon -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] MFD: WL1273 FM Radio: MFD driver for the FM radio.
On Tue, 20 Apr 2010 18:20:05 +0300
Matti J. Aaltonen matti.j.aalto...@nokia.com wrote:
This is a parent driver for two child drivers: the V4L2 driver and
the ALSA codec driver. The MFD part provides the I2C communication
to the device and the state handling.
So I was taking a quick look at this; it mostly looks OK (though I wonder
about all those symbol exports - does all that stuff need to be in the
core?). One thing caught my eye, though:
+int wl1273_fm_rds_off(struct wl1273_core *core)
+{
+ struct device *dev = core-i2c_dev-dev;
+ int r;
+
+ if (core-mode == WL1273_MODE_TX)
+ return 0;
+
+ wait_for_completion_timeout(core-busy, msecs_to_jiffies(1000));
The use of a completion here looks like a mistake to me. This isn't a
normal pattern, and I'm not quite sure what you're trying to do. Here,
also, you're ignoring the return value, so you don't know if completion
was signaled or not.
If you look in functions like:
+int wl1273_fm_set_rx_freq(struct wl1273_core *core, unsigned int freq)
+{
[...]
+ INIT_COMPLETION(core-busy);
+ r = wl1273_fm_write_cmd(core, WL1273_TUNER_MODE_SET,
+ TUNER_MODE_PRESET);
+ if (r) {
+ complete(core-busy);
+ goto err;
+ }
What will prevent multiple threads from doing INIT_COMPLETION()
simultaneously? It looks racy to me. What happens if multiple
threads try to wait simultaneously on the completion?
OK, looking further, you're not using it for mutual exclusion. In fact,
you're not using anything for mutual exclusion; how do you prevent
concurrent access to the hardware registers?
What I would suggest you do is remove the completion in favor of a wait
queue which the interrupt handler can use to signal that something has
completed. You can then use wait_event() to wait for a wakeup and test
that the specific condition you are waiting for has come to pass.
jon
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
