(Resending to fix reply-all)
Hi Mauro,
Thanks for taking the time to look into this. I'll let Patrick respond
to the first part regards the pull request - I'll just respond to the
points you've made about the driver itself.
On 27/04/15 21:16, Mauro Carvalho Chehab wrote:
> +
> +
> +/* Write multiple registers */
> +static int cx24120_writeregN(struct cx24120_state *state,
> + u8 reg, const u8 *values, u16 len, u8 incr)
> +{
> + int ret;
> + u8 buf[5]; /* maximum 4 data bytes at once - flexcop limitation
> + (very limited i2c-interface this one) */
> Hmm... if the limit is at flexcop, then the best is to not be add such
> restriction here, but at the flexcop code, and passing the max limit that
> used for the I2C transfer as a parameter at the attach structure, just
> like other frontend and tuner drivers do.
I had considered doing that - the cx24116 driver does have i2c_wr_max as
part of it's config struct. The only reason I didn't however was that I
did consider that it's now quite unlikely this demod would be used in
anything else so it could probably safely stay fixed for a while yet.
As you say though it would be nicer as a config item, and it shouldn't
be too much to add in so I'll look into doing it that way.
>
> Also, this limit is hardcoded here. Please use a define instead.
So, if I do have this as a config item then this and the following
hardcoded values should all disappear...
>
>> +
>> + struct i2c_msg msg = {
>> + .addr = state->config->i2c_addr,
>> + .flags = 0,
>> + .buf = buf,
>> + .len = len };
>> +
>> + while (len) {
>> + buf[0] = reg;
>> + msg.len = len > 4 ? 4 : len;
> Again, don't hardcode the max buf size here.
>
> +
> + /* Wait for tuning */
> + while (delay_cnt >= 0) {
> + cx24120_read_status(fe, &status);
> + if (status & FE_HAS_LOCK)
> + goto tuned;
> + msleep(20);
> + delay_cnt -= 20;
> + }
> I don't see any need for waiting for tune here. This is generally done in
> userspace and at the kthread inside dvb_frontend.c.
>
> Any reason why this has to be done here?
Some point after tuning the cx24120_set_clock_ratios function needs to
be called and the firmware command CMD_CLOCK_SET sent. The ratios sent
to this command depend on delivery system, fec & pilot - the latter two
only available to read after tuning. If this isn't done then the mpeg
stream doesn't appear.
Is there a better point to set the ratios? Or for another way of asking
that, is there some other function that will always get hit after tuning?
> +
> +/* Calculate vco from config */
> +static u64 cx24120_calculate_vco(struct cx24120_state *state)
> +{
> + u32 vco;
> + u64 inv_vco, res, xxyyzz;
> + u32 xtal_khz = state->config->xtal_khz;
> +
> + xxyyzz = 0x400000000ULL;
> xxyyzz? Weird name for a var.
Yes... one of the oddities left from the dissasembled driver I hadn't
ironed out yet! I'll look into what it's supposed to be doing and rename
that to something far more sensible.
As time allows I'll get this fixed up as required - would it be best if
I send patches to here made against Patrick's tree? I presume also one
patch for each of the coding styles, and then one patch for each of the
other issues?
Jemma.
--
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
