Re: [PATCH 10/13] hackrf: add support for transmitter

[Hans Verkuil <hverkuil@xs4all.nl>]

Hi Antti,

Two comments, see below:

On 09/01/2015 11:59 PM, Antti Palosaari wrote:
> HackRF SDR device has both receiver and transmitter. There is limitation
> that receiver and transmitter cannot be used at the same time
> (half-duplex operation). That patch implements transmitter support to
> existing receiver only driver.
> 
> Signed-off-by: Antti Palosaari <crope@iki.fi>
> ---
>  drivers/media/usb/hackrf/hackrf.c | 923 ++++++++++++++++++++++++++------------
>  1 file changed, 648 insertions(+), 275 deletions(-)
> 
> diff --git a/drivers/media/usb/hackrf/hackrf.c b/drivers/media/usb/hackrf/hackrf.c
> -static unsigned int hackrf_convert_stream(struct hackrf_dev *dev,
> -		void *dst, void *src, unsigned int src_len)
> +void hackrf_copy_stream(struct hackrf_dev *dev, void *dst,

Is there any reason 'static' was removed here? It's not used externally as
far as I can tell.

> +			void *src, unsigned int src_len)
>  {
>  	memcpy(dst, src, src_len);
>  

<snip>

> +static int hackrf_s_modulator(struct file *file, void *fh,
> +		       const struct v4l2_modulator *a)
> +{
> +	struct hackrf_dev *dev = video_drvdata(file);
> +	int ret;
> +
> +	dev_dbg(dev->dev, "index=%d\n", a->index);
> +
> +	if (a->index == 0)
> +		ret = 0;
> +	else if (a->index == 1)
> +		ret = 0;
> +	else
> +		ret = -EINVAL;
> +
> +	return ret;
> +}

Why implement this at all? It's not doing anything. I'd just drop s_modulator
support.

If there is a reason why you do need it, then simplify it to:

	return a->index > 1 ? -EINVAL : 0;

Regards,

	Hans