From: Mauro Carvalho Chehab <m.chehab@samsung.com>
To: Antti Palosaari <crope@iki.fi>
Cc: linux-media@vger.kernel.org
Subject: Re: [PATCH 01/17] e4000: add manual gain controls
Date: Sat, 01 Feb 2014 17:43:10 -0200 [thread overview]
Message-ID: <20140201174310.42d070e2@samsung.com> (raw)
In-Reply-To: <1391264674-4395-2-git-send-email-crope@iki.fi>
Em Sat, 1 Feb 2014 16:24:18 +0200
Antti Palosaari <crope@iki.fi> escreveu:
> Add gain control for LNA, Mixer and IF. Expose controls via DVB
> frontend .set_config callback.
This is not a full review of this patch (or this series), but please
see below.
> Signed-off-by: Antti Palosaari <crope@iki.fi>
> ---
> drivers/media/tuners/e4000.c | 68 +++++++++++++++++++++++++++++++++++++++
> drivers/media/tuners/e4000.h | 6 ++++
> drivers/media/tuners/e4000_priv.h | 63 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 137 insertions(+)
>
> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
> index 0153169..651de11 100644
> --- a/drivers/media/tuners/e4000.c
> +++ b/drivers/media/tuners/e4000.c
> @@ -385,6 +385,73 @@ static int e4000_get_if_frequency(struct dvb_frontend *fe, u32 *frequency)
> return 0;
> }
>
> +static int e4000_set_config(struct dvb_frontend *fe, void *priv_cfg)
> +{
Hmm... that looks weird to me... the set_config() callback should
be used only be those parameters that never change and are required for
the device initialization. It is similar to the parameters passed during
a DVB attach.
So, it is for those things that you won't be exposing to userspace.
Normal control parameters are, instead, implemented using something
like:
static int tvp5150_s_ctrl(struct v4l2_ctrl *ctrl)
{
struct v4l2_subdev *sd = to_sd(ctrl);
switch (ctrl->id) {
case V4L2_CID_BRIGHTNESS:
tvp5150_write(sd, TVP5150_BRIGHT_CTL, ctrl->val);
return 0;
case V4L2_CID_CONTRAST:
tvp5150_write(sd, TVP5150_CONTRAST_CTL, ctrl->val);
return 0;
case V4L2_CID_SATURATION:
tvp5150_write(sd, TVP5150_SATURATION_CTL, ctrl->val);
return 0;
case V4L2_CID_HUE:
tvp5150_write(sd, TVP5150_HUE_CTL, ctrl->val);
return 0;
}
return -EINVAL;
}
static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
.s_ctrl = tvp5150_s_ctrl,
};
That allows them to be independently set, with reduces the I2C
traffic and makes their updates faster.
It also benefits from V4L controls core implementation.
Are there any reason why not using it here?
> + struct e4000_priv *priv = fe->tuner_priv;
> + struct e4000_ctrl *ctrl = priv_cfg;
> + int ret;
> + u8 buf[2];
> + u8 u8tmp;
> + dev_dbg(&priv->client->dev, "%s: lna=%d mixer=%d if=%d\n", __func__,
> + ctrl->lna_gain, ctrl->mixer_gain, ctrl->if_gain);
> +
> + if (fe->ops.i2c_gate_ctrl)
> + fe->ops.i2c_gate_ctrl(fe, 1);
> +
> + if (ctrl->lna_gain == INT_MIN && ctrl->if_gain == INT_MIN)
> + u8tmp = 0x17;
> + else if (ctrl->lna_gain == INT_MIN)
> + u8tmp = 0x19;
> + else if (ctrl->if_gain == INT_MIN)
> + u8tmp = 0x16;
> + else
> + u8tmp = 0x10;
> +
> + ret = e4000_wr_reg(priv, 0x1a, u8tmp);
> + if (ret)
> + goto err;
> +
> + if (ctrl->mixer_gain == INT_MIN)
> + u8tmp = 0x15;
> + else
> + u8tmp = 0x14;
> +
> + ret = e4000_wr_reg(priv, 0x20, u8tmp);
> + if (ret)
> + goto err;
> +
> + if (ctrl->lna_gain != INT_MIN) {
> + ret = e4000_wr_reg(priv, 0x14, ctrl->lna_gain);
> + if (ret)
> + goto err;
> + }
> +
> + if (ctrl->mixer_gain != INT_MIN) {
> + ret = e4000_wr_reg(priv, 0x15, ctrl->mixer_gain);
> + if (ret)
> + goto err;
> + }
> +
> + if (ctrl->if_gain != INT_MIN) {
> + buf[0] = e4000_if_gain_lut[ctrl->if_gain].reg16_val;
> + buf[1] = e4000_if_gain_lut[ctrl->if_gain].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 const struct dvb_tuner_ops e4000_tuner_ops = {
> .info = {
> .name = "Elonics E4000",
> @@ -395,6 +462,7 @@ static const struct dvb_tuner_ops e4000_tuner_ops = {
> .init = e4000_init,
> .sleep = e4000_sleep,
> .set_params = e4000_set_params,
> + .set_config = e4000_set_config,
>
> .get_if_frequency = e4000_get_if_frequency,
> };
> diff --git a/drivers/media/tuners/e4000.h b/drivers/media/tuners/e4000.h
> index e74b8b2..d95c472 100644
> --- a/drivers/media/tuners/e4000.h
> +++ b/drivers/media/tuners/e4000.h
> @@ -40,4 +40,10 @@ struct e4000_config {
> u32 clock;
> };
>
> +struct e4000_ctrl {
> + int lna_gain;
> + int mixer_gain;
> + int if_gain;
> +};
> +
> #endif
> diff --git a/drivers/media/tuners/e4000_priv.h b/drivers/media/tuners/e4000_priv.h
> index 8f45a30..a75a383 100644
> --- a/drivers/media/tuners/e4000_priv.h
> +++ b/drivers/media/tuners/e4000_priv.h
> @@ -145,4 +145,67 @@ static const struct e4000_if_filter e4000_if_filter_lut[] = {
> { 0xffffffff, 0x00, 0x20 },
> };
>
> +struct e4000_if_gain {
> + u8 reg16_val;
> + u8 reg17_val;
> +};
> +
> +static const struct e4000_if_gain e4000_if_gain_lut[] = {
> + {0x00, 0x00},
> + {0x20, 0x00},
> + {0x40, 0x00},
> + {0x02, 0x00},
> + {0x22, 0x00},
> + {0x42, 0x00},
> + {0x04, 0x00},
> + {0x24, 0x00},
> + {0x44, 0x00},
> + {0x01, 0x00},
> + {0x21, 0x00},
> + {0x41, 0x00},
> + {0x03, 0x00},
> + {0x23, 0x00},
> + {0x43, 0x00},
> + {0x05, 0x00},
> + {0x25, 0x00},
> + {0x45, 0x00},
> + {0x07, 0x00},
> + {0x27, 0x00},
> + {0x47, 0x00},
> + {0x0f, 0x00},
> + {0x2f, 0x00},
> + {0x4f, 0x00},
> + {0x17, 0x00},
> + {0x37, 0x00},
> + {0x57, 0x00},
> + {0x1f, 0x00},
> + {0x3f, 0x00},
> + {0x5f, 0x00},
> + {0x1f, 0x01},
> + {0x3f, 0x01},
> + {0x5f, 0x01},
> + {0x1f, 0x02},
> + {0x3f, 0x02},
> + {0x5f, 0x02},
> + {0x1f, 0x03},
> + {0x3f, 0x03},
> + {0x5f, 0x03},
> + {0x1f, 0x04},
> + {0x3f, 0x04},
> + {0x5f, 0x04},
> + {0x1f, 0x0c},
> + {0x3f, 0x0c},
> + {0x5f, 0x0c},
> + {0x1f, 0x14},
> + {0x3f, 0x14},
> + {0x5f, 0x14},
> + {0x1f, 0x1c},
> + {0x3f, 0x1c},
> + {0x5f, 0x1c},
> + {0x1f, 0x24},
> + {0x3f, 0x24},
> + {0x5f, 0x24},
> + {0x7f, 0x24},
> +};
> +
> #endif
--
Cheers,
Mauro
next prev parent reply other threads:[~2014-02-01 19:43 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-02-01 14:24 [PATCH 00/17] SDR API - controls, stream formats Antti Palosaari
2014-02-01 14:24 ` [PATCH 01/17] e4000: add manual gain controls Antti Palosaari
2014-02-01 19:43 ` Mauro Carvalho Chehab [this message]
2014-02-01 20:15 ` Antti Palosaari
2014-02-01 14:24 ` [PATCH 02/17] rtl2832_sdr: expose E4000 gain controls to user space Antti Palosaari
2014-02-01 14:24 ` [PATCH 03/17] r820t: add manual gain controls Antti Palosaari
2014-02-01 14:24 ` [PATCH 04/17] rtl2832_sdr: expose R820 gain controls to user space Antti Palosaari
2014-02-01 14:24 ` [PATCH 05/17] e4000: fix PLL calc to allow higher frequencies Antti Palosaari
2014-02-01 14:24 ` [PATCH 06/17] msi3101: fix device caps to advertise SDR receiver Antti Palosaari
2014-02-01 14:24 ` [PATCH 07/17] rtl2832_sdr: " Antti Palosaari
2014-02-01 14:24 ` [PATCH 08/17] msi3101: add default FMT and ADC frequency Antti Palosaari
2014-02-01 14:24 ` [PATCH 09/17] msi3101: sleep USB ADC and tuner when streaming is stopped Antti Palosaari
2014-02-01 14:24 ` [PATCH 10/17] DocBook: document RF tuner gain controls Antti Palosaari
2014-02-01 14:24 ` [PATCH 11/17] DocBook: V4L: add V4L2_SDR_FMT_CU8 - 'CU08' Antti Palosaari
2014-02-01 14:24 ` [PATCH 12/17] DocBook: V4L: add V4L2_SDR_FMT_CU16LE - 'CU16' Antti Palosaari
2014-02-01 14:24 ` [PATCH 13/17] DocBook: media: document V4L2_CTRL_CLASS_RF_TUNER Antti Palosaari
2014-02-01 14:24 ` [PATCH 14/17] xc2028: silence compiler warnings Antti Palosaari
2014-02-01 14:24 ` [PATCH 15/17] v4l: add RF tuner channel bandwidth control Antti Palosaari
2014-02-03 9:08 ` Hans Verkuil
2014-02-03 9:11 ` Hans Verkuil
2014-02-03 10:17 ` Antti Palosaari
2014-02-03 10:13 ` Antti Palosaari
2014-02-03 10:24 ` Hans Verkuil
2014-02-01 14:24 ` [PATCH 16/17] msi3101: implement tuner " Antti Palosaari
2014-02-01 14:24 ` [PATCH 17/17] rtl2832_sdr: " Antti Palosaari
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140201174310.42d070e2@samsung.com \
--to=m.chehab@samsung.com \
--cc=crope@iki.fi \
--cc=linux-media@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox