Re: [PULL] For 4.2 (or even 4.1?) add support for cx24120/Technisat SkyStar S2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



(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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux