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 <awa...@md.metrocast.net>
> >>   *
> >> + * 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to