Re: [PATCH v4 3/4] tc90522: add driver for Toshiba TC90522 quad demodulator

[Mauro Carvalho Chehab <mchehab@osg.samsung.com>]

Em Tue,  9 Sep 2014 02:20:42 +0900
tskd08@gmail.com escreveu:

> From: Akihiro Tsukada <tskd08@gmail.com>
> 
> This patch adds driver for tc90522 demodulator chips.
> The chip contains 4 demod modules that run in parallel and are independently
> controllable via separate I2C addresses.
> Two of the modules are for ISDB-T and the rest for ISDB-S.
> It is used in earthsoft pt3 cards.
> 
> Note that this driver does not init the chip,
> because the initilization sequence / register setting is not disclosed.
> Thus, the driver assumes that the chips are initilized externally
> by its parent board driver before fe->ops->init() are called.
> Earthsoft PT3 PCIe card, for example, contains the init sequence
> in its private memory and provides a command to trigger the sequence.

I applied this series, as we're discussing it already for a long time,
and it seems in a good shape...

Yet, I got some minor issues with FE status. See below.

PS.: could you also test (and send us patches as needed) for ISDB-S
support at libdvbv5 and dvbv5-utils[1]?

[1] http://linuxtv.org/wiki/index.php/DVBv5_Tools

Thanks!
Mauro


> +static int tc90522s_read_status(struct dvb_frontend *fe, fe_status_t *status)
> +{
> +	struct tc90522_state *state;
> +	int ret;
> +	u8 reg;
> +
> +	state = fe->demodulator_priv;
> +	ret = reg_read(state, 0xc3, &reg, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	*status = 0;
> +	if (reg & 0x80) /* input level under min ? */
> +		return 0;
> +	*status |= FE_HAS_SIGNAL;
> +
> +	if (reg & 0x60) /* carrier? */
> +		return 0;

Sure about that? Wouldn't it be, instead, reg & 0x60 == 0x60?

> +	*status |= FE_HAS_CARRIER | FE_HAS_VITERBI | FE_HAS_SYNC;
> +
> +	if (reg & 0x10)
> +		return 0;
> +	if (reg_read(state, 0xc5, &reg, 1) < 0 || !(reg & 0x03))
> +		return 0;
> +	*status |= FE_HAS_LOCK;
> +	return 0;
> +}
> +
> +static int tc90522t_read_status(struct dvb_frontend *fe, fe_status_t *status)
> +{
> +	struct tc90522_state *state;
> +	int ret;
> +	u8 reg;
> +
> +	state = fe->demodulator_priv;
> +	ret = reg_read(state, 0x96, &reg, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	*status = 0;
> +	if (reg & 0xe0) {
> +		*status = FE_HAS_SIGNAL | FE_HAS_CARRIER | FE_HAS_VITERBI
> +				| FE_HAS_SYNC | FE_HAS_LOCK;
> +		return 0;
> +	}
> +
> +	ret = reg_read(state, 0x80, &reg, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (reg & 0xf0)
> +		return 0;
> +	*status |= FE_HAS_SIGNAL | FE_HAS_CARRIER;
> +
> +	if (reg & 0x0c)
> +		return 0;
> +	*status |= FE_HAS_SYNC | FE_HAS_VITERBI;

The entire series of checks above seems wrong on my eyes too.

For example, if reg = 0x20 or 0x40 or 0x80 or ..., it will return
FE_HAS_LOCK.

Regards,
Mauro