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;
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
