subject:"\[REVIEW PATCH 02\/16\] e4000\: implement controls via v4l2 control framework"

Re: [REVIEW PATCH 02/16] e4000: implement controls via v4l2 control framework

2014-03-13 Thread Mauro Carvalho Chehab

Em Thu, 27 Feb 2014 02:30:11 +0200
Antti Palosaari cr...@iki.fi escreveu:

 Implement gain and bandwidth controls using v4l2 control framework.
 Pointer to control handler is provided by exported symbol.

 Cc: Mauro Carvalho Chehab m.che...@samsung.com
 Cc: Hans Verkuil hverk...@xs4all.nl
 Signed-off-by: Antti Palosaari cr...@iki.fi
 ---
  drivers/media/tuners/e4000.c  | 210 
 +-
  drivers/media/tuners/e4000.h  |  14 +++
  drivers/media/tuners/e4000_priv.h |  75 ++
  3 files changed, 298 insertions(+), 1 deletion(-)

...
 diff --git a/drivers/media/tuners/e4000.h b/drivers/media/tuners/e4000.h
 index e74b8b2..989f2ea 100644
 --- a/drivers/media/tuners/e4000.h
 +++ b/drivers/media/tuners/e4000.h
 @@ -40,4 +40,18 @@ struct e4000_config {
   u32 clock;
  };

 +#if IS_ENABLED(CONFIG_MEDIA_TUNER_E4000)
 +extern struct v4l2_ctrl_handler *e4000_get_ctrl_handler(
 + struct dvb_frontend *fe
 +);
 +#else
 +static inline struct v4l2_ctrl_handler *e4000_get_ctrl_handler(
 + struct dvb_frontend *fe
 +)
 +{
 + pr_warn(%s: driver disabled by Kconfig\n, __func__);
 + return NULL;
 +}
 +#endif
 +
  #endif

There are two things to be noticed here:

1) Please don't add any EXPORT_SYMBOL() on a pure I2C module. You
should, instead, use the subdev calls, in order to callback a
function provided by the module;

2) As you're now using request_module(), you don't need to use
#if IS_ENABLED() anymore. It is up to the module to register
itself as a V4L2 subdevice. The caller module should use the
subdevice interface to run the callbacks.

If you don't to that, you'll have several issues with the
building system.

Regards,
Mauro
--
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: [REVIEW PATCH 02/16] e4000: implement controls via v4l2 control framework

2014-03-13 Thread Antti Palosaari


On 13.03.2014 15:57, Mauro Carvalho Chehab wrote:

Em Thu, 27 Feb 2014 02:30:11 +0200
Antti Palosaari cr...@iki.fi escreveu:


Implement gain and bandwidth controls using v4l2 control framework.
Pointer to control handler is provided by exported symbol.

Cc: Mauro Carvalho Chehab m.che...@samsung.com
Cc: Hans Verkuil hverk...@xs4all.nl
Signed-off-by: Antti Palosaari cr...@iki.fi
---
  drivers/media/tuners/e4000.c  | 210 +-
  drivers/media/tuners/e4000.h  |  14 +++
  drivers/media/tuners/e4000_priv.h |  75 ++
  3 files changed, 298 insertions(+), 1 deletion(-)


...

diff --git a/drivers/media/tuners/e4000.h b/drivers/media/tuners/e4000.h
index e74b8b2..989f2ea 100644
--- a/drivers/media/tuners/e4000.h
+++ b/drivers/media/tuners/e4000.h
@@ -40,4 +40,18 @@ struct e4000_config {
u32 clock;
  };

+#if IS_ENABLED(CONFIG_MEDIA_TUNER_E4000)
+extern struct v4l2_ctrl_handler *e4000_get_ctrl_handler(
+   struct dvb_frontend *fe
+);
+#else
+static inline struct v4l2_ctrl_handler *e4000_get_ctrl_handler(
+   struct dvb_frontend *fe
+)
+{
+   pr_warn(%s: driver disabled by Kconfig\n, __func__);
+   return NULL;
+}
+#endif
+
  #endif


There are two things to be noticed here:

1) Please don't add any EXPORT_SYMBOL() on a pure I2C module. You
should, instead, use the subdev calls, in order to callback a
function provided by the module;


That means, I have to implement it as a V4L subdev driver then...

Is there any problem to leave as it is? It just only provides control 
handler using that export. If you look those existing dvb frontend or 
tuner drivers there is many kind of resources exported just similarly 
(example DibCom PID filters, af9033 pid filters), so I cannot see why 
that should be different.



2) As you're now using request_module(), you don't need to use
#if IS_ENABLED() anymore. It is up to the module to register
itself as a V4L2 subdevice. The caller module should use the
subdevice interface to run the callbacks.

If you don't to that, you'll have several issues with the
building system.


So basically you are saying I should implement that driver as a V4L 
subdev too?


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: [REVIEW PATCH 02/16] e4000: implement controls via v4l2 control framework

2014-03-13 Thread Antti Palosaari


On 13.03.2014 17:17, Antti Palosaari wrote:

On 13.03.2014 15:57, Mauro Carvalho Chehab wrote:

Em Thu, 27 Feb 2014 02:30:11 +0200
Antti Palosaari cr...@iki.fi escreveu:


Implement gain and bandwidth controls using v4l2 control framework.
Pointer to control handler is provided by exported symbol.



There are two things to be noticed here:

1) Please don't add any EXPORT_SYMBOL() on a pure I2C module. You
should, instead, use the subdev calls, in order to callback a
function provided by the module;


That means, I have to implement it as a V4L subdev driver then...

Is there any problem to leave as it is? It just only provides control
handler using that export. If you look those existing dvb frontend or
tuner drivers there is many kind of resources exported just similarly
(example DibCom PID filters, af9033 pid filters), so I cannot see why
that should be different.


2) As you're now using request_module(), you don't need to use
#if IS_ENABLED() anymore. It is up to the module to register
itself as a V4L2 subdevice. The caller module should use the
subdevice interface to run the callbacks.

If you don't to that, you'll have several issues with the
building system.


So basically you are saying I should implement that driver as a V4L
subdev too?



That is not so simple!
If you look how that device is build on driver level

rtl28xxu USB-interface driver is *master*
rtl28xxu loads rtl2832 demod driver
rtl28xxu loads e4000 tuner driver
rtl28xxu loads rtl2832_sdr driver

Whole palette is build upon DVB API, but rtl2832_sdr driver offers SDR 
API for userspace. So it is not that simple go to and implement V4L2 
subdevice to e4000 DVB driver. Only thing needed is to find out some 
way to pass control handler from e4000 to rtl2832_sdr. There is no 
likely any existing solution where DVB side uses V4L2 subdevice and 
implementing such at this tight schedule does not sound reasonable. Why 
you didn't mention you want subdevice earlier? At the very first I 
implemented that using DVB tuner .set_config() callback, but you asked 
to switch v4l2 control framework. And now I should switch to subdev.


There does not seems to be any suitable callback for getting that kind 
of pointer. There is only one existing quite similar solution what I 
know, it is stv6110x / stv090x and I don't like it :)


Any idea?

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: [REVIEW PATCH 02/16] e4000: implement controls via v4l2 control framework

2014-03-13 Thread Mauro Carvalho Chehab

Em Thu, 13 Mar 2014 17:17:50 +0200
Antti Palosaari cr...@iki.fi escreveu:

 On 13.03.2014 15:57, Mauro Carvalho Chehab wrote:
  Em Thu, 27 Feb 2014 02:30:11 +0200
  Antti Palosaari cr...@iki.fi escreveu:
 
  Implement gain and bandwidth controls using v4l2 control framework.
  Pointer to control handler is provided by exported symbol.
 
  Cc: Mauro Carvalho Chehab m.che...@samsung.com
  Cc: Hans Verkuil hverk...@xs4all.nl
  Signed-off-by: Antti Palosaari cr...@iki.fi
  ---
drivers/media/tuners/e4000.c  | 210 
  +-
drivers/media/tuners/e4000.h  |  14 +++
drivers/media/tuners/e4000_priv.h |  75 ++
3 files changed, 298 insertions(+), 1 deletion(-)
 
  ...
  diff --git a/drivers/media/tuners/e4000.h b/drivers/media/tuners/e4000.h
  index e74b8b2..989f2ea 100644
  --- a/drivers/media/tuners/e4000.h
  +++ b/drivers/media/tuners/e4000.h
  @@ -40,4 +40,18 @@ struct e4000_config {
 u32 clock;
};
 
  +#if IS_ENABLED(CONFIG_MEDIA_TUNER_E4000)
  +extern struct v4l2_ctrl_handler *e4000_get_ctrl_handler(
  +  struct dvb_frontend *fe
  +);
  +#else
  +static inline struct v4l2_ctrl_handler *e4000_get_ctrl_handler(
  +  struct dvb_frontend *fe
  +)
  +{
  +  pr_warn(%s: driver disabled by Kconfig\n, __func__);
  +  return NULL;
  +}
  +#endif
  +
#endif
 
  There are two things to be noticed here:
 
  1) Please don't add any EXPORT_SYMBOL() on a pure I2C module. You
  should, instead, use the subdev calls, in order to callback a
  function provided by the module;
 
 That means, I have to implement it as a V4L subdev driver then...

Yes, or to create some other type of binding. IMO, we should move
the subdev interface one level up, as we'll need to use it also
for some other DVB drivers, with a different set of callbacks.

 Is there any problem to leave as it is? It just only provides control 
 handler using that export. If you look those existing dvb frontend or 
 tuner drivers there is many kind of resources exported just similarly 
 (example DibCom PID filters, af9033 pid filters), so I cannot see why 
 that should be different.

Well, try googling for:
undefined reference to `dib3000mc_get_tuner_i2c_master'

And you'll see that having more than one export on a DVB frontend
driver doesn't work.

If you want to see what happens, try to compile rtl28xxu builtin
and e4000 as a module.

What happens is that dvb_attach() will request the tuner module, 
if the driver provides just the foo_attach. 

If you see what dvb_attach() does, it is actually a kind of
request_module that uses a symbol name instead of the module
name, plus a dynamically solved call to the attach function. 
It also increments the loaded module kref (but without assigning
the module caller ownership).

So, it is actually a dirty hack.

In your case, rtl28xxu builtin will have a hard reference for
e4000_get_ctrl_handler, with will cause a compilation breakage
if e4000 is compiled as module.

What the previous I2C model did, and v4l2 subdev does, is to provide
a way for the driver to register a set of callbacks. This way,
all that the caller driver needs to do is to check if the callback
is set. If so, calls it.

 
  2) As you're now using request_module(), you don't need to use
  #if IS_ENABLED() anymore. It is up to the module to register
  itself as a V4L2 subdevice. The caller module should use the
  subdevice interface to run the callbacks.
 
  If you don't to that, you'll have several issues with the
  building system.
 
 So basically you are saying I should implement that driver as a V4L 
 subdev too?

Basically, I'm saying that your patch will break compilation, and,
at a first glance, the subdev approach is the better way to solve it.
 
 regards
 Antti
 


-- 

Regards,
Mauro
--
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

[REVIEW PATCH 02/16] e4000: implement controls via v4l2 control framework

2014-02-26 Thread Antti Palosaari

Implement gain and bandwidth controls using v4l2 control framework.
Pointer to control handler is provided by exported symbol.

Cc: Mauro Carvalho Chehab m.che...@samsung.com
Cc: Hans Verkuil hverk...@xs4all.nl
Signed-off-by: Antti Palosaari cr...@iki.fi
---
 drivers/media/tuners/e4000.c  | 210 +-
 drivers/media/tuners/e4000.h  |  14 +++
 drivers/media/tuners/e4000_priv.h |  75 ++
 3 files changed, 298 insertions(+), 1 deletion(-)

diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
index 0153169..c97daa0 100644
--- a/drivers/media/tuners/e4000.c
+++ b/drivers/media/tuners/e4000.c
@@ -385,6 +385,178 @@ static int e4000_get_if_frequency(struct dvb_frontend 
*fe, u32 *frequency)
return 0;
 }
 
+static int e4000_set_lna_gain(struct dvb_frontend *fe)
+{
+   struct e4000_priv *priv = fe-tuner_priv;
+   int ret;
+   u8 u8tmp;
+   dev_dbg(priv-client-dev, %s: lna auto=%d-%d val=%d-%d\n,
+   __func__, priv-lna_gain_auto-cur.val,
+   priv-lna_gain_auto-val, priv-lna_gain-cur.val,
+   priv-lna_gain-val);
+
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 1);
+
+   if (priv-lna_gain_auto-val  priv-if_gain_auto-cur.val)
+   u8tmp = 0x17;
+   else if (priv-lna_gain_auto-val)
+   u8tmp = 0x19;
+   else if (priv-if_gain_auto-cur.val)
+   u8tmp = 0x16;
+   else
+   u8tmp = 0x10;
+
+   ret = e4000_wr_reg(priv, 0x1a, u8tmp);
+   if (ret)
+   goto err;
+
+   if (priv-lna_gain_auto-val == false) {
+   ret = e4000_wr_reg(priv, 0x14, priv-lna_gain-val);
+   if (ret)
+   goto err;
+   }
+
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 0);
+
+   return 0;
+err:
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 0);
+
+   dev_dbg(priv-client-dev, %s: failed=%d\n, __func__, ret);
+   return ret;
+}
+
+static int e4000_set_mixer_gain(struct dvb_frontend *fe)
+{
+   struct e4000_priv *priv = fe-tuner_priv;
+   int ret;
+   u8 u8tmp;
+   dev_dbg(priv-client-dev, %s: mixer auto=%d-%d val=%d-%d\n,
+   __func__, priv-mixer_gain_auto-cur.val,
+   priv-mixer_gain_auto-val, priv-mixer_gain-cur.val,
+   priv-mixer_gain-val);
+
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 1);
+
+   if (priv-mixer_gain_auto-val)
+   u8tmp = 0x15;
+   else
+   u8tmp = 0x14;
+
+   ret = e4000_wr_reg(priv, 0x20, u8tmp);
+   if (ret)
+   goto err;
+
+   if (priv-mixer_gain_auto-val == false) {
+   ret = e4000_wr_reg(priv, 0x15, priv-mixer_gain-val);
+   if (ret)
+   goto err;
+   }
+
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 0);
+
+   return 0;
+err:
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 0);
+
+   dev_dbg(priv-client-dev, %s: failed=%d\n, __func__, ret);
+   return ret;
+}
+
+static int e4000_set_if_gain(struct dvb_frontend *fe)
+{
+   struct e4000_priv *priv = fe-tuner_priv;
+   int ret;
+   u8 buf[2];
+   u8 u8tmp;
+   dev_dbg(priv-client-dev, %s: if auto=%d-%d val=%d-%d\n,
+   __func__, priv-if_gain_auto-cur.val,
+   priv-if_gain_auto-val, priv-if_gain-cur.val,
+   priv-if_gain-val);
+
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 1);
+
+   if (priv-if_gain_auto-val  priv-lna_gain_auto-cur.val)
+   u8tmp = 0x17;
+   else if (priv-lna_gain_auto-cur.val)
+   u8tmp = 0x19;
+   else if (priv-if_gain_auto-val)
+   u8tmp = 0x16;
+   else
+   u8tmp = 0x10;
+
+   ret = e4000_wr_reg(priv, 0x1a, u8tmp);
+   if (ret)
+   goto err;
+
+   if (priv-if_gain_auto-val == false) {
+   buf[0] = e4000_if_gain_lut[priv-if_gain-val].reg16_val;
+   buf[1] = e4000_if_gain_lut[priv-if_gain-val].reg17_val;
+   ret = e4000_wr_regs(priv, 0x16, buf, 2);
+   if (ret)
+   goto err;
+   }
+
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 0);
+
+   return 0;
+err:
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 0);
+
+   dev_dbg(priv-client-dev, %s: failed=%d\n, __func__, ret);
+   return ret;
+}
+
+static int e4000_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+   struct e4000_priv *priv =
+   container_of(ctrl-handler, struct e4000_priv, hdl);
+   struct dvb_frontend *fe = priv-fe;
+   struct dtv_frontend_properties *c = fe-dtv_property_cache;
+   int ret;
+

[REVIEW PATCH 02/16] e4000: implement controls via v4l2 control framework

2014-02-10 Thread Antti Palosaari

Implement gain and bandwidth controls using v4l2 control framework.
Pointer to control handler is provided by exported symbol.

Cc: Mauro Carvalho Chehab m.che...@samsung.com
Cc: Hans Verkuil hverk...@xs4all.nl
Signed-off-by: Antti Palosaari cr...@iki.fi
---
 drivers/media/tuners/e4000.c  | 210 +-
 drivers/media/tuners/e4000.h  |  14 +++
 drivers/media/tuners/e4000_priv.h |  75 ++
 3 files changed, 298 insertions(+), 1 deletion(-)

diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
index 0153169..ac44dd2 100644
--- a/drivers/media/tuners/e4000.c
+++ b/drivers/media/tuners/e4000.c
@@ -385,6 +385,178 @@ static int e4000_get_if_frequency(struct dvb_frontend 
*fe, u32 *frequency)
return 0;
 }
 
+static int e4000_set_lna_gain(struct dvb_frontend *fe)
+{
+   struct e4000_priv *priv = fe-tuner_priv;
+   int ret;
+   u8 u8tmp;
+   dev_dbg(priv-client-dev, %s: lna auto=%d-%d val=%d-%d\n,
+   __func__, priv-lna_gain_auto-cur.val,
+   priv-lna_gain_auto-val, priv-lna_gain-cur.val,
+   priv-lna_gain-val);
+
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 1);
+
+   if (priv-lna_gain_auto-val  priv-if_gain_auto-cur.val)
+   u8tmp = 0x17;
+   else if (priv-lna_gain_auto-val)
+   u8tmp = 0x19;
+   else if (priv-if_gain_auto-cur.val)
+   u8tmp = 0x16;
+   else
+   u8tmp = 0x10;
+
+   ret = e4000_wr_reg(priv, 0x1a, u8tmp);
+   if (ret)
+   goto err;
+
+   if (priv-lna_gain_auto-val == false) {
+   ret = e4000_wr_reg(priv, 0x14, priv-lna_gain-val);
+   if (ret)
+   goto err;
+   }
+
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 0);
+
+   return 0;
+err:
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 0);
+
+   dev_dbg(priv-client-dev, %s: failed=%d\n, __func__, ret);
+   return ret;
+}
+
+static int e4000_set_mixer_gain(struct dvb_frontend *fe)
+{
+   struct e4000_priv *priv = fe-tuner_priv;
+   int ret;
+   u8 u8tmp;
+   dev_dbg(priv-client-dev, %s: mixer auto=%d-%d val=%d-%d\n,
+   __func__, priv-mixer_gain_auto-cur.val,
+   priv-mixer_gain_auto-val, priv-mixer_gain-cur.val,
+   priv-mixer_gain-val);
+
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 1);
+
+   if (priv-mixer_gain_auto-val)
+   u8tmp = 0x15;
+   else
+   u8tmp = 0x14;
+
+   ret = e4000_wr_reg(priv, 0x20, u8tmp);
+   if (ret)
+   goto err;
+
+   if (priv-mixer_gain_auto-val == false) {
+   ret = e4000_wr_reg(priv, 0x15, priv-mixer_gain-val);
+   if (ret)
+   goto err;
+   }
+
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 0);
+
+   return 0;
+err:
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 0);
+
+   dev_dbg(priv-client-dev, %s: failed=%d\n, __func__, ret);
+   return ret;
+}
+
+static int e4000_set_if_gain(struct dvb_frontend *fe)
+{
+   struct e4000_priv *priv = fe-tuner_priv;
+   int ret;
+   u8 buf[2];
+   u8 u8tmp;
+   dev_dbg(priv-client-dev, %s: if auto=%d-%d val=%d-%d\n,
+   __func__, priv-if_gain_auto-cur.val,
+   priv-if_gain_auto-val, priv-if_gain-cur.val,
+   priv-if_gain-val);
+
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 1);
+
+   if (priv-if_gain_auto-val  priv-lna_gain_auto-cur.val)
+   u8tmp = 0x17;
+   else if (priv-lna_gain_auto-cur.val)
+   u8tmp = 0x19;
+   else if (priv-if_gain_auto-val)
+   u8tmp = 0x16;
+   else
+   u8tmp = 0x10;
+
+   ret = e4000_wr_reg(priv, 0x1a, u8tmp);
+   if (ret)
+   goto err;
+
+   if (priv-if_gain_auto-val == false) {
+   buf[0] = e4000_if_gain_lut[priv-if_gain-val].reg16_val;
+   buf[1] = e4000_if_gain_lut[priv-if_gain-val].reg17_val;
+   ret = e4000_wr_regs(priv, 0x16, buf, 2);
+   if (ret)
+   goto err;
+   }
+
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 0);
+
+   return 0;
+err:
+   if (fe-ops.i2c_gate_ctrl)
+   fe-ops.i2c_gate_ctrl(fe, 0);
+
+   dev_dbg(priv-client-dev, %s: failed=%d\n, __func__, ret);
+   return ret;
+}
+
+static int e4000_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+   struct e4000_priv *priv =
+   container_of(ctrl-handler, struct e4000_priv, hdl);
+   struct dvb_frontend *fe = priv-fe;
+   struct dtv_frontend_properties *c = fe-dtv_property_cache;
+   int ret;
+

Re: [REVIEW PATCH 02/16] e4000: implement controls via v4l2 control framework

Re: [REVIEW PATCH 02/16] e4000: implement controls via v4l2 control framework

Re: [REVIEW PATCH 02/16] e4000: implement controls via v4l2 control framework

Re: [REVIEW PATCH 02/16] e4000: implement controls via v4l2 control framework

[REVIEW PATCH 02/16] e4000: implement controls via v4l2 control framework

[REVIEW PATCH 02/16] e4000: implement controls via v4l2 control framework

6 matches

Site Navigation

Mail list logo

Footer information