From: Mauro Carvalho Chehab <mchehab@redhat.com>
To: HoP <jpetrous@gmail.com>
Cc: o.endriss@gmx.de, patrick.boettcher@desy.de, kraxel@bytesex.org,
ajurik@quick.cz, hermann pitton <hermann-pitton@arcor.de>,
linux-media@vger.kernel.org
Subject: Re: [PATCH v2] isl6421.c - added tone control and temporary diseqc overcurrent
Date: Tue, 15 Dec 2009 16:03:35 -0200 [thread overview]
Message-ID: <4B27CF77.1050008@redhat.com> (raw)
In-Reply-To: <846899810912150749q38d8a1ffy96b135cf355fe8eb@mail.gmail.com>
HoP wrote:
> Hi Mauro,
>
> I have finally found some time for reworking our patch
> with regards of notes I got in disscussion.
>
> BTW, I learnt that sending patch for review to original
> authors is right thing (tm),
Yes, it is, but sending the emails to linux-media also works, since
the maintainers are all there.
> so I have added Oliver,
> as isl6421.c author, Patrick as flexcop author, Gerd
> as cx88/saa7134 author (I hope I found correct persons,
> if no please ignore posting).
Gerd is not maintaining cx88/saa7134 anymore. I took his place on
maintaining those drivers some years ago.
I'm still missing a driver or a board entry that requires those
changes. Could you please send it together with this patch series?
Also, you forgot to send your Signed-off-By. This is required for
patch submission.
>
> Regards
>
> /Honza
>
> ---
>
> isl6421.c - added tone control and temporary diseqc overcurrent
Please, always send patches in-lined. makes easier for commenting.
> diff -r 79fc32bba0a0 linux/drivers/media/dvb/b2c2/flexcop-fe-tuner.c
> --- a/linux/drivers/media/dvb/b2c2/flexcop-fe-tuner.c Mon Dec 14 17:43:13 2009 -0200
> +++ b/linux/drivers/media/dvb/b2c2/flexcop-fe-tuner.c Tue Dec 15 16:36:14 2009 +0100
> @@ -302,6 +302,12 @@ static struct itd1000_config skystar2_re
> .i2c_address = 0x61,
> };
>
> +static struct isl6421_config skystar2_rev2_7_isl6421_config = {
> + .i2c_address = 0x08,
> + .override_set = 0x01,
> + .override_clear = 0x01,
> +};
> +
> static int skystar2_rev27_attach(struct flexcop_device *fc,
> struct i2c_adapter *i2c)
> {
> @@ -325,7 +331,7 @@ static int skystar2_rev27_attach(struct
> /* enable no_base_addr - no repeated start when reading */
> fc->fc_i2c_adap[2].no_base_addr = 1;
> if (!dvb_attach(isl6421_attach, fc->fe, &fc->fc_i2c_adap[2].i2c_adap,
> - 0x08, 1, 1)) {
> + &skystar2_rev2_7_isl6421_config)) {
> err("ISL6421 could NOT be attached");
> goto fail_isl;
> }
> @@ -368,6 +374,12 @@ static const struct cx24113_config skyst
> .xtal_khz = 10111,
> };
>
> +static struct isl6421_config skystar2_rev2_8_isl6421_config = {
> + .i2c_address = 0x08,
> + .override_set = 0x00,
> + .override_clear = 0x00,
Please, do not set any static value to zero. Kernel module support already
does that, and this will just add uneeded stuff into BSS.
> +};
> +
> static int skystar2_rev28_attach(struct flexcop_device *fc,
> struct i2c_adapter *i2c)
> {
> @@ -391,7 +403,7 @@ static int skystar2_rev28_attach(struct
>
> fc->fc_i2c_adap[2].no_base_addr = 1;
> if (!dvb_attach(isl6421_attach, fc->fe, &fc->fc_i2c_adap[2].i2c_adap,
> - 0x08, 0, 0)) {
> + &skystar2_rev2_8_isl6421_config)) {
> err("ISL6421 could NOT be attached");
> fc->fc_i2c_adap[2].no_base_addr = 0;
> return 0;
> diff -r 79fc32bba0a0 linux/drivers/media/dvb/frontends/isl6421.c
> --- a/linux/drivers/media/dvb/frontends/isl6421.c Mon Dec 14 17:43:13 2009 -0200
> +++ b/linux/drivers/media/dvb/frontends/isl6421.c Tue Dec 15 16:36:14 2009 +0100
> @@ -3,6 +3,9 @@
> *
> * Copyright (C) 2006 Andrew de Quincey
> * Copyright (C) 2006 Oliver Endriss
> + * Copyright (C) 2009 Ales Jurik and Jan Petrous (added optional 22k tone
> + * support and temporary diseqc overcurrent enable until
> + * next command - set voltage or tone)
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License
> @@ -36,37 +39,88 @@
> #include "isl6421.h"
>
> struct isl6421 {
> - u8 config;
> - u8 override_or;
> - u8 override_and;
> - struct i2c_adapter *i2c;
> - u8 i2c_addr;
> + const struct isl6421_config *config;
> + u8 reg1;
reg1 seems a very bad name. Based on the datasheet, maybe
you could call it as sys_config or sys_reg_config.
> +
> + struct i2c_adapter *i2c;
> +
> + int (*diseqc_send_master_cmd_orig)(struct dvb_frontend *fe,
> + struct dvb_diseqc_master_cmd *cmd);
> };
>
> static int isl6421_set_voltage(struct dvb_frontend *fe, fe_sec_voltage_t voltage)
> {
> struct isl6421 *isl6421 = (struct isl6421 *) fe->sec_priv;
> - struct i2c_msg msg = { .addr = isl6421->i2c_addr, .flags = 0,
> - .buf = &isl6421->config,
> - .len = sizeof(isl6421->config) };
> + struct i2c_msg msg = { .addr = isl6421->config->i2c_addr, .flags = 0,
> + .buf = &isl6421->reg1,
> + .len = sizeof(isl6421->reg1) };
>
> - isl6421->config &= ~(ISL6421_VSEL1 | ISL6421_EN1);
> + isl6421->reg1 &= ~(ISL6421_VSEL1 | ISL6421_EN1);
>
> switch(voltage) {
> case SEC_VOLTAGE_OFF:
> break;
> case SEC_VOLTAGE_13:
> - isl6421->config |= ISL6421_EN1;
> + isl6421->reg1 |= ISL6421_EN1;
> break;
> case SEC_VOLTAGE_18:
> - isl6421->config |= (ISL6421_EN1 | ISL6421_VSEL1);
> + isl6421->reg1 |= (ISL6421_EN1 | ISL6421_VSEL1);
> break;
> default:
> return -EINVAL;
> };
>
> - isl6421->config |= isl6421->override_or;
> - isl6421->config &= isl6421->override_and;
> + isl6421->reg1 |= isl6421->config->override_set;
> + isl6421->reg1 &= ~isl6421->config->override_clear;
> +
> + return (i2c_transfer(isl6421->i2c, &msg, 1) == 1) ? 0 : -EIO;
> +}
> +
> +static int isl6421_send_diseqc(struct dvb_frontend *fe,
> + struct dvb_diseqc_master_cmd *cmd)
Please add a comment explaining that this function is only called
when diseqc_send_master_cmd_orig() is defined. On a first look, it seemed
to me that you would cause a crash by not checking if diseqc_send_master_cmd_orig()
is not null.
> +{
> + struct isl6421 *isl6421 = (struct isl6421 *) fe->sec_priv;
> + struct i2c_msg msg = { .addr = isl6421->config->i2c_addr, .flags = 0,
> + .buf = &isl6421->reg1,
> + .len = sizeof(isl6421->reg1) };
> +
> + isl6421->reg1 |= ISL6421_DCL;
> +
> + isl6421->reg1 |= isl6421->config->override_set;
> + isl6421->reg1 &= ~isl6421->config->override_clear;
> +
> + if (i2c_transfer(isl6421->i2c, &msg, 1) != 1)
> + return -EIO;
> +
> + isl6421->reg1 &= ~ISL6421_DCL;
> +
> + return isl6421->diseqc_send_master_cmd_orig(fe, cmd);
> +}
> +
> +static int isl6421_set_tone(struct dvb_frontend *fe, fe_sec_tone_mode_t tone)
> +{
> + struct isl6421 *isl6421 = (struct isl6421 *) fe->sec_priv;
> + struct i2c_msg msg = { .addr = isl6421->config->i2c_addr, .flags = 0,
> + .buf = &isl6421->reg1,
> + .len = sizeof(isl6421->reg1) };
> +
> + isl6421->reg1 &= ~(ISL6421_ENT1);
> +
> + printk(KERN_INFO "%s: %s\n", __func__, ((tone == SEC_TONE_OFF) ?
> + "Off" : "On"));
> +
> + switch (tone) {
> + case SEC_TONE_ON:
> + isl6421->reg1 |= ISL6421_ENT1;
> + break;
> + case SEC_TONE_OFF:
> + break;
> + default:
> + return -EINVAL;
> + };
> +
> + isl6421->reg1 |= isl6421->config->override_set;
> + isl6421->reg1 &= ~isl6421->config->override_clear;
>
> return (i2c_transfer(isl6421->i2c, &msg, 1) == 1) ? 0 : -EIO;
> }
> @@ -74,49 +128,52 @@ static int isl6421_enable_high_lnb_volta
> static int isl6421_enable_high_lnb_voltage(struct dvb_frontend *fe, long arg)
> {
> struct isl6421 *isl6421 = (struct isl6421 *) fe->sec_priv;
> - struct i2c_msg msg = { .addr = isl6421->i2c_addr, .flags = 0,
> - .buf = &isl6421->config,
> - .len = sizeof(isl6421->config) };
> + struct i2c_msg msg = { .addr = isl6421->config->i2c_addr, .flags = 0,
> + .buf = &isl6421->reg1,
> + .len = sizeof(isl6421->reg1) };
>
> if (arg)
> - isl6421->config |= ISL6421_LLC1;
> + isl6421->reg1 |= ISL6421_LLC1;
> else
> - isl6421->config &= ~ISL6421_LLC1;
> + isl6421->reg1 &= ~ISL6421_LLC1;
>
> - isl6421->config |= isl6421->override_or;
> - isl6421->config &= isl6421->override_and;
> + isl6421->reg1 |= isl6421->config->override_set;
> + isl6421->reg1 &= ~isl6421->config->override_clear;
>
> return (i2c_transfer(isl6421->i2c, &msg, 1) == 1) ? 0 : -EIO;
> }
>
> static void isl6421_release(struct dvb_frontend *fe)
> {
> + struct isl6421 *isl6421 = (struct isl6421 *) fe->sec_priv;
> +
> /* power off */
> isl6421_set_voltage(fe, SEC_VOLTAGE_OFF);
> +
> + if (isl6421->config->disable_overcurrent_protection)
> + fe->ops.diseqc_send_master_cmd =
> + isl6421->diseqc_send_master_cmd_orig;
You need to test if this function pointer were defined or not at the config struct.
>
> /* free */
> kfree(fe->sec_priv);
> fe->sec_priv = NULL;
> }
>
> -struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter *i2c, u8 i2c_addr,
> - u8 override_set, u8 override_clear)
> +struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe,
> + struct i2c_adapter *i2c,
> + const struct isl6421_config *config)
> {
> struct isl6421 *isl6421 = kmalloc(sizeof(struct isl6421), GFP_KERNEL);
> +
> if (!isl6421)
> return NULL;
>
> - /* default configuration */
> - isl6421->config = ISL6421_ISEL1;
> + isl6421->config = config;
> isl6421->i2c = i2c;
> - isl6421->i2c_addr = i2c_addr;
> fe->sec_priv = isl6421;
>
> - /* bits which should be forced to '1' */
> - isl6421->override_or = override_set;
> -
> - /* bits which should be forced to '0' */
> - isl6421->override_and = ~override_clear;
> + /* default configuration */
> + isl6421->reg1 = ISL6421_ISEL1;
>
> /* detect if it is present or not */
> if (isl6421_set_voltage(fe, SEC_VOLTAGE_OFF)) {
> @@ -131,11 +188,38 @@ struct dvb_frontend *isl6421_attach(stru
> /* override frontend ops */
> fe->ops.set_voltage = isl6421_set_voltage;
> fe->ops.enable_high_lnb_voltage = isl6421_enable_high_lnb_voltage;
> + if (config->tone_control)
> + fe->ops.set_tone = isl6421_set_tone;
> +
> + printk(KERN_INFO "ISL6421 attached on addr=%x\n", config->i2c_addr);
> +
> + if (config->disable_overcurrent_protection) {
> + if ((config->override_set & ISL6421_DCL) ||
> + (config->override_clear & ISL6421_DCL)) {
> + /* there is no sense to use overcurrent_enable
> + * with DCL bit set in any override byte */
> + if (config->override_set & ISL6421_DCL)
> + printk(KERN_WARNING "ISL6421 overcurrent_enable"
> + " with DCL bit in override_set,"
> + " overcurrent_enable ignored\n");
> + if (config->override_clear & ISL6421_DCL)
> + printk(KERN_WARNING "ISL6421 overcurrent_enable"
> + " with DCL bit in override_clear,"
> + " overcurrent_enable ignored\n");
> + } else {
> + printk(KERN_WARNING "ISL6421 overcurrent_enable "
> + " activated. WARNING: it can be "
> + " dangerous for your hardware!");
> + isl6421->diseqc_send_master_cmd_orig =
> + fe->ops.diseqc_send_master_cmd;
> + fe->ops.diseqc_send_master_cmd = isl6421_send_diseqc;
> + }
> + }
>
> return fe;
> }
> EXPORT_SYMBOL(isl6421_attach);
>
> MODULE_DESCRIPTION("Driver for lnb supply and control ic isl6421");
> -MODULE_AUTHOR("Andrew de Quincey & Oliver Endriss");
> +MODULE_AUTHOR("Andrew de Quincey,Oliver Endriss,Ales Jurik,Jan Petrous");
> MODULE_LICENSE("GPL");
> diff -r 79fc32bba0a0 linux/drivers/media/dvb/frontends/isl6421.h
> --- a/linux/drivers/media/dvb/frontends/isl6421.h Mon Dec 14 17:43:13 2009 -0200
> +++ b/linux/drivers/media/dvb/frontends/isl6421.h Tue Dec 15 16:36:14 2009 +0100
> @@ -39,14 +39,40 @@
> #define ISL6421_ISEL1 0x20
> #define ISL6421_DCL 0x40
>
> +struct isl6421_config {
> + /* I2C address */
> + u8 i2c_addr;
> +
> + /* Enable DISEqC tone control mode */
> + bool tone_control;
> +
> + /*
> + * Disable isl6421 overcurrent protection.
> + *
> + * WARNING: Don't disable the protection unless you are 100% sure about
> + * what you're doing, otherwise you may damage your board.
> + * Only a few designs require to disable the protection, since
> + * the hardware designer opted to use a hardware protection instead
> + */
> + bool disable_overcurrent_protection;
> +
> + /* bits which should be forced to '1' */
> + u8 override_set;
> +
> + /* bits which should be forced to '0' */
> + u8 override_clear;
> +};
> +
> +
> #if defined(CONFIG_DVB_ISL6421) || (defined(CONFIG_DVB_ISL6421_MODULE) && defined(MODULE))
> /* override_set and override_clear control which system register bits (above) to always set & clear */
> -extern struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter *i2c, u8 i2c_addr,
> - u8 override_set, u8 override_clear);
> +extern struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe,
> + struct i2c_adapter *i2c,
> + const struct isl6421_config *config);
> #else
> -static inline struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter *i2c, u8 i2c_addr,
> - u8 override_set, u8 override_clear)
> -{
> +static struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe,
> + struct i2c_adapter *i2c,
> + const struct isl6421_config *config);
> printk(KERN_WARNING "%s: driver disabled by Kconfig\n", __func__);
> return NULL;
> }
> diff -r 79fc32bba0a0 linux/drivers/media/video/cx88/cx88-dvb.c
> --- a/linux/drivers/media/video/cx88/cx88-dvb.c Mon Dec 14 17:43:13 2009 -0200
> +++ b/linux/drivers/media/video/cx88/cx88-dvb.c Tue Dec 15 16:36:14 2009 +0100
> @@ -456,6 +456,12 @@ static struct cx24123_config hauppauge_n
> .set_ts_params = cx24123_set_ts_param,
> };
>
> +static struct isl6421_config hauppauge_novas_isl6421_config = {
> + .i2c_address = 0x08,
> + .override_set = ISL6421_DCL,
> + .override_clear = 0x00,
Don't initialize a value with zero.
> +};
> +
> static struct cx24123_config kworld_dvbs_100_config = {
> .demod_address = 0x15,
> .set_ts_params = cx24123_set_ts_param,
> @@ -614,6 +620,12 @@ static struct cx24116_config hauppauge_h
> .reset_device = cx24116_reset_device,
> };
>
> +static struct isl6421_config hauppauge_hvr4000_isl6421_config = {
> + .i2c_address = 0x08,
> + .override_set = ISL6421_DCL,
> + .override_clear = 0x00,
Don't initialize a value with zero.
> +};
> +
> static struct cx24116_config tevii_s460_config = {
> .demod_address = 0x55,
> .set_ts_params = cx24116_set_ts_param,
> @@ -757,7 +769,7 @@ static int dvb_register(struct cx8802_de
> if (!dvb_attach(isl6421_attach,
> fe0->dvb.frontend,
> &dev->core->i2c_adap,
> - 0x08, ISL6421_DCL, 0x00))
> + &hauppauge_novas_isl6421_config))
> goto frontend_detach;
> }
> /* MFE frontend 2 */
> @@ -995,7 +1007,8 @@ static int dvb_register(struct cx8802_de
> &core->i2c_adap);
> if (fe0->dvb.frontend) {
> if (!dvb_attach(isl6421_attach, fe0->dvb.frontend,
> - &core->i2c_adap, 0x08, ISL6421_DCL, 0x00))
> + &core->i2c_adap,
> + &hauppauge_novas_isl6421_config))
> goto frontend_detach;
> }
> break;
> @@ -1100,7 +1113,7 @@ static int dvb_register(struct cx8802_de
> if (!dvb_attach(isl6421_attach,
> fe0->dvb.frontend,
> &dev->core->i2c_adap,
> - 0x08, ISL6421_DCL, 0x00))
> + &hauppauge_hvr4000_isl6421_config))
> goto frontend_detach;
> }
> /* MFE frontend 2 */
> @@ -1128,7 +1141,7 @@ static int dvb_register(struct cx8802_de
> if (!dvb_attach(isl6421_attach,
> fe0->dvb.frontend,
> &dev->core->i2c_adap,
> - 0x08, ISL6421_DCL, 0x00))
> + &hauppauge_hvr4000_isl6421_config))
> goto frontend_detach;
> }
> break;
> diff -r 79fc32bba0a0 linux/drivers/media/video/saa7134/saa7134-dvb.c
> --- a/linux/drivers/media/video/saa7134/saa7134-dvb.c Mon Dec 14 17:43:13 2009 -0200
> +++ b/linux/drivers/media/video/saa7134/saa7134-dvb.c Tue Dec 15 16:36:14 2009 +0100
> @@ -716,6 +716,12 @@ static struct tda1004x_config lifeview_t
> .request_firmware = philips_tda1004x_request_firmware
> };
>
> +static struct isl6421_config lifeview_trio_isl6421_config = {
> + .i2c_address = 0x08,
> + .override_set = 0x00,
> + .override_clear = 0x00,
Don't initialize a value with zero.
> +};
> +
> static struct tda1004x_config tevion_dvbt220rf_config = {
> .demod_address = 0x08,
> .invert = 1,
> @@ -895,6 +901,12 @@ static struct tda10086_config flydvbs =
> .invert = 0,
> .diseqc_tone = 0,
> .xtal_freq = TDA10086_XTAL_16M,
> +};
> +
> +static struct isl6421_config flydvbs_isl6421_config = {
> + .i2c_address = 0x08,
> + .override_set = 0x00,
> + .override_clear = 0x00,
Don't initialize a value with zero.
> };
>
> static struct tda10086_config sd1878_4m = {
> @@ -1248,7 +1260,7 @@ static int dvb_init(struct saa7134_dev *
> goto dettach_frontend;
> }
> if (dvb_attach(isl6421_attach, fe0->dvb.frontend, &dev->i2c_adap,
> - 0x08, 0, 0) == NULL) {
> + &lifeview_trio_isl6421_config) == NULL) {
> wprintk("%s: Lifeview Trio, No ISL6421 found!\n", __func__);
> goto dettach_frontend;
> }
> @@ -1349,7 +1361,8 @@ static int dvb_init(struct saa7134_dev *
> goto dettach_frontend;
> }
> if (dvb_attach(isl6421_attach, fe0->dvb.frontend,
> - &dev->i2c_adap, 0x08, 0, 0) == NULL) {
> + &dev->i2c_adap,
> + &flydvbs_isl6421_config) == NULL) {
> wprintk("%s: No ISL6421 found!\n", __func__);
> goto dettach_frontend;
> }
next prev parent reply other threads:[~2009-12-15 18:04 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-12-15 15:49 [PATCH v2] isl6421.c - added tone control and temporary diseqc overcurrent HoP
2009-12-15 18:03 ` Mauro Carvalho Chehab [this message]
2009-12-16 0:20 ` [PATCH v3] " HoP
2010-01-20 13:58 ` HoP
2010-01-20 18:31 ` Mauro Carvalho Chehab
2010-01-20 22:43 ` HoP
2010-01-20 23:26 ` Mauro Carvalho Chehab
2010-01-20 23:59 ` HoP
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=4B27CF77.1050008@redhat.com \
--to=mchehab@redhat.com \
--cc=ajurik@quick.cz \
--cc=hermann-pitton@arcor.de \
--cc=jpetrous@gmail.com \
--cc=kraxel@bytesex.org \
--cc=linux-media@vger.kernel.org \
--cc=o.endriss@gmx.de \
--cc=patrick.boettcher@desy.de \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.