[3/3] em28xx: add support for registering multiple i2c buses

Em Wed, 06 Mar 2013 17:53:19 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Hi Mauro,
> 
> I'm basically fine this patch, just a few comments/thoughts:
> 
> Am 05.03.2013 11:55, schrieb Mauro Carvalho Chehab:
> > Register both buses 0 and 1 via I2C API. For now, bus 0 is used
> > only by eeprom on all known devices. Later patches will be needed
> > if this changes in the future.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> > ---
> >  drivers/media/usb/em28xx/em28xx-cards.c | 29 +++++++---
> >  drivers/media/usb/em28xx/em28xx-i2c.c   | 93 ++++++++++++++++++++++-----------
> >  drivers/media/usb/em28xx/em28xx.h       | 19 +++++--
> >  3 files changed, 97 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> > index 16ab4d7..496b938 100644
> > --- a/drivers/media/usb/em28xx/em28xx-cards.c
> > +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> > @@ -2235,8 +2235,8 @@ static inline void em28xx_set_model(struct em28xx *dev)
> >  		dev->board.i2c_speed = EM28XX_I2C_CLK_WAIT_ENABLE |
> >  				       EM28XX_I2C_FREQ_100_KHZ;
> >  
> > -	if (dev->board.def_i2c_bus == 1)
> > -		dev->board.i2c_speed |= EM2874_I2C_SECONDARY_BUS_SELECT;
> > +	/* Should be initialized early, for I2C to work */
> > +	dev->def_i2c_bus = dev->board.def_i2c_bus;
> >  }
> >  
> >  
> > @@ -2642,7 +2642,7 @@ static int em28xx_hint_board(struct em28xx *dev)
> >  
> >  	/* user did not request i2c scanning => do it now */
> >  	if (!dev->i2c_hash)
> > -		em28xx_do_i2c_scan(dev);
> > +		em28xx_do_i2c_scan(dev, dev->def_i2c_bus);
> >  
> >  	for (i = 0; i < ARRAY_SIZE(em28xx_i2c_hash); i++) {
> >  		if (dev->i2c_hash == em28xx_i2c_hash[i].hash) {
> > @@ -2953,7 +2953,9 @@ void em28xx_release_resources(struct em28xx *dev)
> >  
> >  	em28xx_release_analog_resources(dev);
> >  
> > -	em28xx_i2c_unregister(dev);
> > +	if (dev->def_i2c_bus)
> > +		em28xx_i2c_unregister(dev, 1);
> > +	em28xx_i2c_unregister(dev, 0);
> >  
> >  	v4l2_ctrl_handler_free(&dev->ctrl_handler);
> >  
> > @@ -3109,14 +3111,23 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
> >  	v4l2_ctrl_handler_init(hdl, 8);
> >  	dev->v4l2_dev.ctrl_handler = hdl;
> >  
> > -	/* register i2c bus */
> > -	retval = em28xx_i2c_register(dev);
> > +	/* register i2c bus 0 */
> > +	retval = em28xx_i2c_register(dev, 0);
> >  	if (retval < 0) {
> > -		em28xx_errdev("%s: em28xx_i2c_register - error [%d]!\n",
> > +		em28xx_errdev("%s: em28xx_i2c_register bus 0 - error [%d]!\n",
> >  			__func__, retval);
> >  		goto unregister_dev;
> >  	}
> >  
> > +	if (dev->def_i2c_bus) {
> > +		retval = em28xx_i2c_register(dev, 1);
> 
> Hmm, ok, so you don't want to register unused busses ?

No, the reason here is to just not register it if the device has just
one bus. Ok, the driver could simply check for the em28xx chipset type,
but it sounded easier and cleaner to just check for this field.

> What about bus A when all subdevices are on bus B ?

Currently, there's no such case. If we ever find a device like that, then
we can change the logic. Doing it in advance sounds an overkill to me.

> You don't have to register the adapter for eeprom reading, the i2c
> transfer functions work with unregistered i2c_adapters, too.

Yes, I know it is possible to hack it. However, that makes the code
more complex to read and understand, as, on other drivers, each different
I2C bus is mapped as a different I2C adapter.

> 
> > +		if (retval < 0) {
> > +			em28xx_errdev("%s: em28xx_i2c_register bus 1 - error [%d]!\n",
> > +				__func__, retval);
> > +			goto unregister_dev;
> > +		}
> > +	}
> > +
> >  	/*
> >  	 * Default format, used for tvp5150 or saa711x output formats
> >  	 */
> > @@ -3186,7 +3197,9 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
> >  	return 0;
> >  
> >  fail:
> > -	em28xx_i2c_unregister(dev);
> > +	if (dev->def_i2c_bus)
> > +		em28xx_i2c_unregister(dev, 1);
> > +	em28xx_i2c_unregister(dev, 0);
> >  	v4l2_ctrl_handler_free(&dev->ctrl_handler);
> >  
> >  unregister_dev:
> > diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> > index 9086e57..ea63ac4 100644
> > --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> > +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> > @@ -280,9 +280,22 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr)
> >  static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
> >  			   struct i2c_msg msgs[], int num)
> >  {
> > -	struct em28xx *dev = i2c_adap->algo_data;
> > +	struct em28xx_i2c_bus *i2c_bus = i2c_adap->algo_data;
> > +	struct em28xx *dev = i2c_bus->dev;
> > +	unsigned bus = i2c_bus->bus, last_bus;
> >  	int addr, rc, i, byte;
> >  
> > +	/* Switch I2C bus if needed */
> > +	last_bus = (dev->board.i2c_speed & EM2874_I2C_SECONDARY_BUS_SELECT) ?
> > +		   1 : 0;
> > +	if (bus != last_bus) {
> > +		if (bus == 1)
> > +			dev->board.i2c_speed |= EM2874_I2C_SECONDARY_BUS_SELECT;
> > +		else
> > +			dev->board.i2c_speed &= ~EM2874_I2C_SECONDARY_BUS_SELECT;
> > +		em28xx_write_reg(dev, EM28XX_R06_I2C_CLK, dev->board.i2c_speed);
> > +	}
> > +
> 
> FYI: although it seems to be not necessary for the devices we've seen so
> far, the Windows driver clear/sets bit EM2874_I2C_SECONDARY_BUS_SELECT
> at the beginning of each i2c operation.

Yes, I'm aware of that, but we can do better than the alternative driver ;)

> Anyway, we shouldn't abuse the board data struct for saving the
> currently selected bus.
> This will also not work for briges which switch the bus by using a
> different algorithm instead of modifying register EM28XX_R06_I2C_CLK
> (like the em2765).
> So better save the currently used bus number to a variable in the device
> struct.

I actually started coding like that, but then I decided to just take the
simpler approach, as there's no big issue on modifying the value there.

Feel free to store it on a separate var at em28xx struct after adding support
for em2765.

> 
> >  	if (num <= 0)
> >  		return 0;
> >  	for (i = 0; i < num; i++) {
> > @@ -384,7 +397,7 @@ static int em28xx_i2c_read_block(struct em28xx *dev, u16 addr, bool addr_w16,
> >  	/* Select address */
> >  	buf[0] = addr >> 8;
> >  	buf[1] = addr & 0xff;
> > -	ret = i2c_master_send(&dev->i2c_client[dev->def_i2c_bus], buf + !addr_w16, 1 + addr_w16);
> > +	ret = i2c_master_send(&dev->i2c_client[0], buf + !addr_w16, 1 + addr_w16);
> 
> There is no need to restrict this function to bus 0.
> Just change dev->i2c_client[0] to dev->i2c_client[dev->current_i2c_bus]
> (or however you call this variable) or (even better) add a function
> parameter for bus selection (like you did it with em28xx_do_i2c_scan() ).
> It's currently used for eeprom reading only, but that may change in the
> future.

Again, I almost coded like that ;)
> 
> >  	if (ret < 0)
> >  		return ret;
> >  	/* Read data */
> > @@ -398,7 +411,7 @@ static int em28xx_i2c_read_block(struct em28xx *dev, u16 addr, bool addr_w16,
> >  		else
> >  			rsize = remain;
> >  
> > -		ret = i2c_master_recv(&dev->i2c_client[dev->def_i2c_bus], data, rsize);
> > +		ret = i2c_master_recv(&dev->i2c_client[0], data, rsize);
> 
> The same here.
> 
> The rest looks good and I think it's a step in the right direction. :)
> 
> Regards,
> Frank

Ok, addressed the pointed issues and re-tested with HVR-930C.

[PATCH v2 3/3] em28xx: add support for registering multiple i2c buses

Register both buses 0 and 1 via I2C API. For now, bus 0 is used
only by eeprom on all known devices. Later patches will be needed
if this changes in the future.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

-
v2:

- added a mutex to avoid using the two buses that depend on R06 at
  the same time (Devin's request);

- pass bus as a parameter for eeprom routines;

- use a separate field to store the currently used bus

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[3/3] em28xx: add support for registering multiple i2c buses

Commit Message

Patch