Re: [PATCH v3 1/4] mxl301rf: add driver for MaxLinear MxL301RF OFDM tuner

[Akihiro TSUKADA <tskd08@gmail.com>]

Moi!,
thanks for the review.

>> +static int reg_read(struct mxl301rf_state *state, u8 reg, u8 *val)
.......
>> +    ret = i2c_transfer(state->i2c->adapter, msgs, ARRAY_SIZE(msgs));
>> +    if (ret >= 0 && ret < ARRAY_SIZE(msgs))
>> +        ret = -EIO;
>> +    return (ret == ARRAY_SIZE(msgs)) ? 0 : ret;
>> +}
> 
> Could you really implement that as a I2C write with STOP and I2C read
> with STOP. I don't like you abuse, without any good reason,
> i2c_transfer() with REPEATED START even we know chip does not do it.
> 
> You should use
> i2c_master_send()
> i2c_master_recv()

Though it's woking with this REPEATED_START style reads,
(and my reference win driver also uses REPEATED_START),
I'll incorporate this in the next version.

>> +static int mxl301rf_get_status(struct dvb_frontend *fe, u32 *status)
.....
> And whole function is quite useless in any case. It was aimed for analog
> radio driver originally, where was audio demod integrated. We usually
> just program tuner first, then demod, without waiting tuner lock, as
> tuner locks practically immediately to given freq. It is demod which
> locking then has any sense.
> 
> Tuner PLL lock bits could be interesting only when you want to test if
> you are in a frequency whole tuner is able to receive. Some corner case
> when tuner is driven over its limits to see if it locks or not.

I understand. I'll remove .get_status().

>> +static int mxl301rf_init(struct dvb_frontend *fe)
.......
>> +    /* tune to the initial freq */
.......
> This looks odd. Why it is tuned here to some freq? What happens if you
> don't do it and it will be tuned to requested freq. Sometimes that kind
> of things are used to initialize badly written driven...

In a PT3 board, mxl301rf is packaged into a canned tuner module
(Sharp	VA4M6JC2103) with another mxl301rf and two qm1d1c0042's.
A reference win driver says that it is to avoid "interference"
between mxl301rf and qm1d1c0042, so I added a config parameter
of initial freq.
I could have moved those initial tunings to the PCI driver,
but I don't know if it is a corner case that applys just to PT3.

I must admit that my code is written pretty badly,
but it is partly;) because the available/disclosed information is
very limited to the reference win driver kit, it hides lots of
register settings including those for init/config,
and is badly written not separating demod/tuner modules well.

>> +static const struct dvb_tuner_ops mxl301rf_ops = {
......
>> +    .init = mxl301rf_init,
>> +    .sleep = mxl301rf_sleep,
>> +
>> +    .set_params = mxl301rf_set_params,
>> +    .get_status = mxl301rf_get_status,
>> +    .get_rf_strength = mxl301rf_get_rf_strength,
> 
> get IF frequency is missing. That is tuner using IF so you will need to
> know IF in order to get demod working.

As the product guide of TC90522 says it can accept
3-6MHz low IF or 44/57MHz direct IF, so there must be
the register setting to select/set/get one of these configs.
But I don't have the data sheets of mxl301rf,
and cannot know which demod/tuner reigsters are set during
init/config phase (as I said above it's not disclosed), 
so I don't know the registers to set/get IF.

The tuner/demod drivers I wrote are certainly imcomplete ones
that lack init/config of the chips,
but currently they are used just by PT3 and
when someone gets to use them in other products,
I expect that [s]he would have more info and update my code.

regards,
akihiro