From patchwork Mon Mar 2 12:33:54 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mauro Carvalho Chehab X-Patchwork-Id: 9531 Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n22CYOYd027199 for ; Mon, 2 Mar 2009 12:34:24 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751130AbZCBMeY (ORCPT ); Mon, 2 Mar 2009 07:34:24 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751179AbZCBMeY (ORCPT ); Mon, 2 Mar 2009 07:34:24 -0500 Received: from bombadil.infradead.org ([18.85.46.34]:51026 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751130AbZCBMeY (ORCPT ); Mon, 2 Mar 2009 07:34:24 -0500 Received: from 200.220.139.66.nipcable.com ([200.220.139.66] helo=pedra.chehab.org) by bombadil.infradead.org with esmtpsa (Exim 4.69 #1 (Red Hat Linux)) id 1Le7Lg-0006YR-Kx; Mon, 02 Mar 2009 12:34:21 +0000 Date: Mon, 2 Mar 2009 09:33:54 -0300 From: Mauro Carvalho Chehab To: "Igor M. Liplianin" Cc: linux-media@vger.kernel.org, Steven Toth Subject: Re: [PULL] http://udev.netup.ru/hg/v4l-dvb-netup Message-ID: <20090302093354.5b240099@pedra.chehab.org> In-Reply-To: <200902281632.47597.liplianin@tut.by> References: <200902281632.47597.liplianin@tut.by> X-Mailer: Claws Mail 3.3.1 (GTK+ 2.10.4; x86_64-redhat-linux-gnu) Mime-Version: 1.0 X-SRS-Rewrite: SMTP reverse-path rewritten from by bombadil.infradead.org See http://www.infradead.org/rpr.html Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On Sat, 28 Feb 2009 16:32:47 +0200 "Igor M. Liplianin" 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. + 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; +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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- 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: