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

2015-10-16 Thread Antti Palosaari 



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

2015-10-16 Thread Hans Verkuil 
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

2015-10-16 Thread Hans Verkuil 
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

2015-09-04 Thread Hans Verkuil 
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

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

2015-09-01 Thread Antti Palosaari 
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
index 5bd291b..b6415b9 100644
--- a/drivers/media/usb/hackrf/hackrf.c
+++ b/drivers/media/usb/hackrf/hackrf.c
@@ -34,6 +34,7 @@ enum {
CMD_AMP_ENABLE = 0x11,
CMD_SET_LNA_GAIN   = 0x13,
CMD_SET_VGA_GAIN   = 0x14,
+   CMD_SET_TXVGA_GAIN = 0x15,
 };
 
 /*
@@ -44,10 +45,10 @@ enum {
 #define MAX_BULK_BUFS(6)
 #define BULK_BUFFER_SIZE (128 * 512)
 
-static const struct v4l2_frequency_band bands_adc[] = {
+static const struct v4l2_frequency_band bands_adc_dac[] = {
{
.tuner = 0,
-   .type = V4L2_TUNER_ADC,
+   .type = V4L2_TUNER_SDR,
.index = 0,
.capability = V4L2_TUNER_CAP_1HZ | V4L2_TUNER_CAP_FREQ_BANDS,
.rangelow   =   20,
@@ -55,7 +56,7 @@ static const struct v4l2_frequency_band bands_adc[] = {
},
 };
 
-static const struct v4l2_frequency_band bands_rf[] = {
+static const struct v4l2_frequency_band bands_rx_tx[] = {
{
.tuner = 1,
.type = V4L2_TUNER_RF,
@@ -85,34 +86,44 @@ static struct hackrf_format formats[] = {
 static const unsigned int NUM_FORMATS = ARRAY_SIZE(formats);
 
 /* intermediate buffers with raw data from the USB device */
-struct hackrf_frame_buf {
-   struct vb2_buffer vb;   /* common v4l buffer stuff -- must be first */
+struct hackrf_buffer {
+   struct vb2_buffer vb;
struct list_head list;
 };
 
 struct hackrf_dev {
-#define POWER_ON 1
-#define USB_STATE_URB_BUF2 /* XXX: set manually */
-#define SAMPLE_RATE_SET 10
-#define RX_BANDWIDTH11
-#define RX_RF_FREQUENCY 12
-#define RX_RF_GAIN  13
-#define RX_LNA_GAIN 14
-#define RX_IF_GAIN  15
+#define USB_STATE_URB_BUF1 /* XXX: set manually */
+#define RX_ON4
+#define TX_ON5
+#define RX_ADC_FREQUENCY11
+#define TX_DAC_FREQUENCY12
+#define RX_BANDWIDTH13
+#define TX_BANDWIDTH14
+#define RX_RF_FREQUENCY 15
+#define TX_RF_FREQUENCY 16
+#define RX_RF_GAIN  17
+#define TX_RF_GAIN  18
+#define RX_IF_GAIN  19
+#define RX_LNA_GAIN 20
+#define TX_LNA_GAIN 21
unsigned long flags;
 
struct usb_interface *intf;
struct device *dev;
struct usb_device *udev;
-   struct video_device vdev;
+   struct video_device rx_vdev;
+   struct video_device tx_vdev;
struct v4l2_device v4l2_dev;
 
/* videobuf2 queue and queued buffers list */
-   struct vb2_queue vb_queue;
-   struct list_head queued_bufs;
-   spinlock_t queued_bufs_lock; /* Protects queued_bufs */
+   struct vb2_queue rx_vb2_queue;
+   struct vb2_queue tx_vb2_queue;
+   struct list_head rx_buffer_list;
+   struct list_head tx_buffer_list;
+   spinlock_t buffer_list_lock; /* Protects buffer_list */
unsigned sequence;   /* Buffer sequence counter */
unsigned int vb_full;/* vb is full and packets dropped */
+   unsigned int vb_empty;   /* vb is empty and packets dropped */
 
/* Note if taking both locks v4l2_lock must always be locked first! */
struct mutex v4l2_lock;  /* Protects everything else */
@@ -132,17 +143,24 @@ struct hackrf_dev {
 
/* Current configuration */
unsigned int f_adc;
-   unsigned int f_rf;
+   unsigned int f_dac;
+   unsigned int f_rx;
+   unsigned int f_tx;
u32 pixelformat;
u32 buffersize;
 
/* Controls */
-   struct v4l2_ctrl_handler hdl;
-   struct v4l2_ctrl *bandwidth_auto;
-   struct v4l2_ctrl *bandwidth;
-   struct v4l2_ctrl *rf_gain;
-   struct v4l2_ctrl *lna_gain;
-   struct v4l2_ctrl *if_gain;
+   struct v4l2_ctrl_handler rx_ctrl_handler;
+   struct v4l2_ctrl *rx_bandwidth_auto;
+   struct v4l2_ctrl *rx_bandwidth;
+   struct v4l2_ctrl *rx_rf_gain;
+   struct v4l2_ctrl *rx_lna_gain;
+   struct v4l2_ctrl *rx_if_gain;
+   struct v4l2_ctrl_handler tx_ctrl_handler;
+   struct v4l2_ctrl *tx_bandwidth_auto;
+   struct v4l2_ctrl