Re: [PATCH 10/13] hackrf: add support for transmitter
On 10/16/2015 10:59 AM, Antti Palosaari wrote:
>
>
> On 10/16/2015 11:53 AM, Hans Verkuil wrote:
>> On 09/04/2015 12:06 PM, Hans Verkuil wrote:
>>> 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
---
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);
>>>
>>>
>>>
+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;
>>
>> Oops, I was wrong. You need this regardless for the simple reason that the
>> spec
>> mandates it. And indeed without s_modulator v4l2-compliance will fail.
>>
>> I've put back this function, but replacing the index check with the
>> one-liner I
>> suggested above.
>>
>> I'm merging this hackrf patch series with that change and a small fix for the
>> krobot 'unused intf' warning, so you don't need to do anything.
>>
>> Thanks for doing this work!
>
> OK, good! Update also documentation changelog / history kernel version
> number to one which is correct (~4.0 => 4.4).
Done!
Hans
--
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 10/13] hackrf: add support for transmitter
On 10/16/2015 11:53 AM, Hans Verkuil wrote:
On 09/04/2015 12:06 PM, Hans Verkuil wrote:
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
---
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);
+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;
Oops, I was wrong. You need this regardless for the simple reason that the spec
mandates it. And indeed without s_modulator v4l2-compliance will fail.
I've put back this function, but replacing the index check with the one-liner I
suggested above.
I'm merging this hackrf patch series with that change and a small fix for the
krobot 'unused intf' warning, so you don't need to do anything.
Thanks for doing this work!
OK, good! Update also documentation changelog / history kernel version
number to one which is correct (~4.0 => 4.4).
regards
Antti
--
http://palosaari.fi/
--
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 10/13] hackrf: add support for transmitter
On 09/04/2015 12:06 PM, Hans Verkuil wrote:
> 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
>> ---
>> 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);
>>
>
>
>
>> +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;
Oops, I was wrong. You need this regardless for the simple reason that the spec
mandates it. And indeed without s_modulator v4l2-compliance will fail.
I've put back this function, but replacing the index check with the one-liner I
suggested above.
I'm merging this hackrf patch series with that change and a small fix for the
krobot 'unused intf' warning, so you don't need to do anything.
Thanks for doing this work!
Regards,
Hans
--
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 10/13] hackrf: add support for transmitter
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
> ---
> 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);
>
> +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
--
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
