[review,3/6] radio-mr800: no need to pass curfreq value to amradio_setfreq()
Commit Message
Small cleanup of amradio_setfreq(). No need to pass radio->curfreq value
to this function.
Signed-off-by: Alexey Klimov <klimov.linux@gmail.com>
--
Comments
On Sat, Aug 8, 2009 at 1:46 PM, Alexey Klimov<klimov.linux@gmail.com> wrote:
> Small cleanup of amradio_setfreq(). No need to pass radio->curfreq value
> to this function.
>
> Signed-off-by: Alexey Klimov <klimov.linux@gmail.com>
>
> --
> diff -r 5f3329bebfe4 linux/drivers/media/radio/radio-mr800.c
> --- a/linux/drivers/media/radio/radio-mr800.c Wed Jul 29 12:36:46 2009 +0400
> +++ b/linux/drivers/media/radio/radio-mr800.c Wed Jul 29 12:41:51 2009 +0400
> @@ -202,11 +202,11 @@
> }
>
> /* set a frequency, freq is defined by v4l's TUNER_LOW, i.e. 1/16th kHz */
> -static int amradio_setfreq(struct amradio_device *radio, int freq)
> +static int amradio_setfreq(struct amradio_device *radio)
> {
> int retval;
> int size;
> - unsigned short freq_send = 0x10 + (freq >> 3) / 25;
> + unsigned short freq_send = 0x10 + (radio->curfreq >> 3) / 25;
I suspect there may be race conditions here. Once again you're reading
a value without first acquiring the lock. Since this is another
utility function, the lock should probably be held _before_ calling
this function and any locking in this function should be removed.
Adding a BUG_ON(!is_mutex_locked(&radio->lock)) should probably be
added as well.
>
> /* safety check */
> if (radio->removed)
> @@ -413,7 +413,7 @@
> radio->curfreq = f->frequency;
> mutex_unlock(&radio->lock);
>
> - retval = amradio_setfreq(radio, radio->curfreq);
> + retval = amradio_setfreq(radio);
> if (retval < 0)
> amradio_dev_warn(&radio->videodev->dev,
> "set frequency failed\n");
>
>
>
> --
> Best regards, Klimov Alexey
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Regards,
David Ellingsworth
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
@@ -202,11 +202,11 @@
}
/* set a frequency, freq is defined by v4l's TUNER_LOW, i.e. 1/16th kHz */
-static int amradio_setfreq(struct amradio_device *radio, int freq)
+static int amradio_setfreq(struct amradio_device *radio)
{
int retval;
int size;
- unsigned short freq_send = 0x10 + (freq >> 3) / 25;
+ unsigned short freq_send = 0x10 + (radio->curfreq >> 3) / 25;
/* safety check */
if (radio->removed)
@@ -413,7 +413,7 @@
radio->curfreq = f->frequency;
mutex_unlock(&radio->lock);
- retval = amradio_setfreq(radio, radio->curfreq);
+ retval = amradio_setfreq(radio);
if (retval < 0)
amradio_dev_warn(&radio->videodev->dev,
"set frequency failed\n");