On Sat, 28 Feb 2009 16:32:47 +0200 "Igor M. Liplianin" <liplianin@xxxxxx> wrote: > Mauro, > It is driver for NetUP Dual DVB-S2 CI PCI-e card. > You can find short description of it on linuxtv wiki. > > Please pull from http://udev.netup.ru/hg/v4l-dvb-netup > > for the following 11 changesets: Hi Igor, Your changesets look clean, except for a few points, as noticed bellow. The first is just a cleanup to improve code readability. However, the others are more critical, since one of them changes the defaults for lnbp21. Please correct and ask me to pull it after your fixes. Cheers, Mauro. > 03/11: Add CIMax(R) SP2 Common Interface code for NetUP Dual DVB-S2 CI card > http://udev.netup.ru/hg/v4l-dvb-netup?cmd=changeset;node=7af6715bacec > Hmm... by looking on this macro: +#define CHOOSE_CI_CS \ + switch (state->ci_i2c_addr) {\ + case 0x40:\ + cx_clear(MC417_RWD, NETUP_CS0); \ + break;\ + case 0x41:\ + cx_clear(MC417_RWD, NETUP_CS1); \ + break;\ + } It assumes that a given function have a parameter, called "state". This kind of code should really be avoided, since it hides the macro dependency of such parameter. While looking on all places it occurred, it seems that, on all places, you're using about the same struct: First occurrence: + mutex_lock(&gpio_mutex); + + WRITE_CI_ADDR; + cx_write(MC417_OEN, NETUP_EN_ALL | NETUP_DATA); + CHOOSE_CI_CS; + cx_clear(MC417_RWD, NETUP_RD); + mem = netup_ci_get_mem(dev); + + mutex_unlock(&gpio_mutex); Second occurrence: + mutex_lock(&gpio_mutex); + + WRITE_CI_ADDR; + cx_write(MC417_RWD, NETUP_CTRL_OFF | data); + CHOOSE_CI_CS; + cx_clear(MC417_RWD, NETUP_WR); + mem = netup_ci_get_mem(dev); + + mutex_unlock(&gpio_mutex); Third occurrence: + mutex_lock(&gpio_mutex); + + WRITE_CI_ADDR; + cx_write(MC417_OEN, NETUP_EN_ALL | NETUP_DATA); + CHOOSE_CI_CS; + cx_clear(MC417_RWD, NETUP_RD); + mem = netup_ci_get_mem(dev); + + mutex_unlock(&gpio_mutex); Fourth occurrence: + mutex_lock(&gpio_mutex); + + WRITE_CI_ADDR; + cx_write(MC417_RWD, NETUP_CTRL_OFF | data); + CHOOSE_CI_CS; + cx_clear(MC417_RWD, NETUP_WR); + mem = netup_ci_get_mem(dev); + + mutex_unlock(&gpio_mutex); The only difference between those occurrences is the register that you're changing and the data value. IMO, you should, instead, use a function, and get rid of both CHOOSE_CI_CS and WRITE_CI_ADDR. Something like (code not tested): static void netup_ci_write_selecting_ci(netup_ci_state *state, int reg, int value) { + mutex_lock(&gpio_mutex); + + /* Choose CI CS */ + do { \ + cx_write(MC417_OEN, NETUP_EN_ALL);\ + cx_write(MC417_RWD, NETUP_CTRL_OFF | \ + NETUP_ADLO | (0xff & addr));\ + cx_clear(MC417_RWD, NETUP_ADLO);\ + cx_write(MC417_RWD, NETUP_CTRL_OFF | \ + NETUP_ADHI | (0xff & (addr >> 8)));\ + cx_clear(MC417_RWD, NETUP_ADHI); \ + } while (0) + + cx_write(reg, val); + + /* Select CI Address */ + switch (state->ci_i2c_addr) {\ + case 0x40:\ + cx_clear(MC417_RWD, NETUP_CS0); \ + break;\ + case 0x41:\ + cx_clear(MC417_RWD, NETUP_CS1); \ + break;\ + } + cx_clear(MC417_RWD, NETUP_WR); + mem = netup_ci_get_mem(dev); + + mutex_unlock(&gpio_mutex); } > 05/11: Add support for ST LNBH24 LNB power controller. > http://udev.netup.ru/hg/v4l-dvb-netup?cmd=changeset;node=3b65476f39a9 +config DVB_LNBH24 + tristate "LNBH24 SEC controller" + depends on DVB_CORE && I2C + default m if DVB_FE_CUSTOMISE + help + An SEC control chip. + --- a/linux/drivers/media/dvb/frontends/Makefile Thu Jan 22 00:22:36 2009 +0200 +++ b/linux/drivers/media/dvb/frontends/Makefile Tue Feb 10 00:58:37 2009 +0200 @@ -43,6 +43,7 @@ obj-$(CONFIG_DVB_LGDT3304) += lgdt3304.o obj-$(CONFIG_DVB_CX24123) += cx24123.o obj-$(CONFIG_DVB_LNBP21) += lnbp21.o +obj-$(CONFIG_DVB_LNBH24) += lnbp21.o No. Instead, just edit the Kconfig entry for LNBH24 to state that it is used by both LNBH21 and LNBH24. Something like: config DVB_LNBP21 tristate "LNBP21/LNBH21 SEC controller" depends on DVB_CORE && I2C default m if DVB_FE_CUSTOMISE help This driver provides support for both lnbp21/lnbp24 SEC control chips. -struct dvb_frontend *lnbp21_attach(struct dvb_frontend *fe, struct i2c_adapter *i2c, u8 override_set, u8 override_clear) +struct dvb_frontend *lnbh24_attach(struct dvb_frontend *fe, + struct i2c_adapter *i2c, u8 override_set, + u8 override_clear, u8 i2c_addr) { struct lnbp21 *lnbp21 = kmalloc(sizeof(struct lnbp21), GFP_KERNEL); if (!lnbp21) return NULL; /* default configuration */ - lnbp21->config = LNBP21_ISEL; + lnbp21->config = LNBH24_TTX; <snip/> +struct dvb_frontend *lnbp21_attach(struct dvb_frontend *fe, + struct i2c_adapter *i2c, u8 override_set, + u8 override_clear) +{ + + return lnbh24_attach(fe, i2c, override_set, override_clear, 0x08); +} I don't like the idea of changing the default here. You should preserve the default for lnbp21, when this function is called via lnbp21_attach. Maybe you can create a generic static lnbp2x_attach function, called by both lnbp21_attach and lnbh24_attach, passing the config as a parameter. Cheers, Mauro -- 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
