[PULL] http://udev.netup.ru/hg/v4l-dvb-netup

On Sat, 28 Feb 2009 16:32:47 +0200
"Igor M. Liplianin" <liplianin@tut.by> 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;
<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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PULL] http://udev.netup.ru/hg/v4l-dvb-netup

Commit Message

Comments

Patch