On Mon, 2011-09-26 at 23:37 +0200, Maciej Szmigiero wrote:
> W dniu 24.09.2011 22:21, Andy Walls pisze:
> > Hi Maciej,
> >
> > I'll try and comment on the specific areas below, but overall the
> > problem is this:
> >
> > 1. The default setup and behavior of the cx25840 module was written
> > around hardware designs supported by the ivtv driver: i.e. interfacing
> > to a CX23416 MPEG encoder.
> >
> > 2. The ivtv and pvrusb2 drivers rely on that default setup and behavior
> > of the cx25840 module.
> >
> > 3. The PVR-150 and PVR-500 are very popular cards supported by ivtv that
> > use a CX25843 and CX23416. Many MythTV users still have these cards in
> > service.
> >
> > 4. The ivtv driver also supports other hardware designs that use
> > different encoders, so trying fix ivtv to match new changes in the
> > cx25840 will ripples along to other analog video decoder drivers. This
> > would result in a lot of time to perform regression testing with as many
> > different ivtv supported capture cards as possible.
> >
> >
> > What I recommend is that you rework your changes so that the cx25840
> > module is provided information by the bridge driver as to the board
> > model, and then have the cx25840 module behave appropriately based on
> > the board information passed in by the bridge driver.
> >
> > 1. Add whatever data fields you think you need to the "struct
> > cx25840_platform_data" structure in include/media/cx25840.h. Maybe
> > something as simple as "bool is_medion95700"
> >
> > 2. In cxusb-analog.c you instantiate the cx25840 sub-device with
> > v4l2_i2c_new_subdev_board() with the cx25840 platform data filled in as
> > needed for the Medion 95700. Look at
> > drivers/media/video/ivtv/ivtv-i2c.c:ivtv_i2c_register() for an example
> > of how this is done for the cx25840 module.
> >
> > 3. Modify the cx25840 module to behave as you need it if the platform
> > data indicates a Medion 95700; otherwise, leave the default cx25840
> > setup and behavior.
> >
>
> Hi Andy,
>
> Thanks for you detailed explanation, I did not know that ivtv boards are that
> quirky with regard to VBI capture.
> I will do as you wrote above, make my changes to cx25840 driver conditional,
> so ivtv won't be affected.
Thanks!
> > Any specific comments I have are in-line below:
> >
> >> @@ -18,6 +18,9 @@
> >> * CX2388[578] IRQ handling, IO Pin mux configuration and other small fixes are
> >> * Copyright (C) 2010 Andy Walls <awalls@xxxxxxxxxxxxxxxx>
> >> *
> >> + * CX2384x pin to pad mapping and output format configuration support are
> > ^^^^^^^
> > CX2584x?
> >> if ((fmt->width * 16 < Hsrc) || (Hsrc < fmt->width) ||
> >> (Vlines * 8 < Vsrc) || (Vsrc < Vlines)) {
> >> @@ -1403,6 +1695,112 @@ static void log_audio_status(struct i2c_client *client)
> >> }
> >> }
> >>
> >> +#define CX25480_VCONFIG_OPTION(option_mask) \
> > ^^^^^^
> > CX25840?
> >
> >> + if (config_in & option_mask) { \
> >> + state->vid_config &= ~(option_mask); \
> >> + state->vid_config |= config_in & option_mask; \
> >> + } \
> >> +
> >> +#define CX25480_VCONFIG_SET_BIT(optionmask, reg, bit, oneval) \
> > ^^^^^^
> > CX25840?
> >
>
> You mean here that it should be named consistently either as CX2584x or CX25840?
No. You simply appear to have transposed the 8 with the 4.
> >> return set_input(client, input, state->aud_input);
> >> }
> >>
> >> @@ -1877,7 +2278,7 @@ static int cx25840_probe(struct i2c_client *client,
> >> u16 device_id;
> >>
> >> /* Check if the adapter supports the needed features */
> >> - if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> >> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> >> return -EIO;
> >
> > On the surface, this change doesn't appear to adversely affect the ivtv,
> > pvrusb2, cx23885, and cx231xx bridge drivers.
> >
> > I would need to take a hard look at the CX2584[0123], CX2583[67],
> > CX2388[578], and CX2310[12] datasheets to see why, and if, all the Mako
> > core variants require I2C_FUNC_SMBUS_BYTE_DATA.
> >
> > However, if the cxusb bridge has a full I2C master, shouldn't the cxusb
> > driver be specifying (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL) as its
> > functionality? See Documentation/i2c/functionality.
>
> Adding I2C_FUNC_SMBUS_EMUL flag to cxusb i2c host seems to be a right thing to do for now,
> but I would be very surprised if any of Conexant video decoders actually used SMBus instead
> of plain I2C.
I'm confident they use plain I2C as well.
Hans added this particular i2c_check_functionality() call to the cx25840
module in 2007. It appears to be a cut and paste of what many I2C chip
drivers in drivers/media/video do.
The check might have its origin in some example code from Greg K-H in
2003 for a Tiny I2C chip driver:
http://www.linuxjournal.com/article/7252?page=0,0
ftp://ftp.linuxjournal.com/pub/lj/listings/issue118/7252.tgz
Regards,
Andy
--
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
