Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field
On Thu, 13 Jan 2011 12:07:34 -0500, Devin Heitmueller wrote: > On Thu, Jan 13, 2011 at 11:48 AM, Jean Delvare wrote: > > On Thu, 13 Jan 2011 11:34:42 -0500, Andy Walls wrote: > >> How should clock stretches by slaves be handled using i2c-algo-bit? > > > > It is already handled. But hdpvr-i2c doesn't use i2c-algo-bit. I2C > > support is done with USB commands instead. Maybe the hardware > > implementation doesn't support clock stretching by slaves. Apparently > > it doesn't support repeated start conditions either, so it wouldn't > > surprise me. > > The hardware implementation does support clock stretching, or else it > wouldn't be working under Windows. I think your conclusion is too fast and possibly incorrect. The traces Andy pointed us too earlier suggest that the windows driver is also "polling" the Zilog after the send operation to figure out when it is available again. If the Zilog was stretching the clock and the master was seeing that, it wouldn't return until the clock is released, so no polling would be necessary. So, either the Zilog isn't stretching the clock in the standard way, or the master doesn't notice. > That said, it's possible that the > driver for the i2c master isn't checking the proper bits to detect the > clock stretch. I haven't personally looked at the code for the i2c > master, so I cannot say one way or the other. If you're talking about hdpvr-i2c, it's sending USB commands, so it doesn't seem like we have much control over what happens behind the scene. If there is any way to improve its reliability, that will require external knowledge. -- Jean Delvare -- 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
Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field
On Thu, Jan 13, 2011 at 11:48 AM, Jean Delvare wrote: > On Thu, 13 Jan 2011 11:34:42 -0500, Andy Walls wrote: >> How should clock stretches by slaves be handled using i2c-algo-bit? > > It is already handled. But hdpvr-i2c doesn't use i2c-algo-bit. I2C > support is done with USB commands instead. Maybe the hardware > implementation doesn't support clock stretching by slaves. Apparently > it doesn't support repeated start conditions either, so it wouldn't > surprise me. The hardware implementation does support clock stretching, or else it wouldn't be working under Windows. That said, it's possible that the driver for the i2c master isn't checking the proper bits to detect the clock stretch. I haven't personally looked at the code for the i2c master, so I cannot say one way or the other. The Zilog is a pretty nasty beast, and for various reasons it is especially problematic on the HD-PVR due to some issues I cannot really get into on a public forum. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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
Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field
On Thu, Jan 13, 2011 at 11:34 AM, Andy Walls wrote: > Devin, > > You've seen the clock stretch with your I2C analyzer? The Beagle I was using when I did the captures doesn't show clock stretch. My "Logic" logic analyzer does, but I wasn't using that at the time. That said, I can say with some authority that the Zilog does put the bus into a busy state after certain operations (such as setting the send bit). Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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
Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field
Jean, Yes, however, I asked because ivtv and cx18 use i2c-algo-bit and also provide Zilog z8 IR I2C clients for lirc_zilog to use. So if those get clock stretch handling "for free", that's great. Regards, Andy Jean Delvare wrote: >On Thu, 13 Jan 2011 11:34:42 -0500, Andy Walls wrote: >> How should clock stretches by slaves be handled using i2c-algo-bit? > >It is already handled. But hdpvr-i2c doesn't use i2c-algo-bit. I2C >support is done with USB commands instead. Maybe the hardware >implementation doesn't support clock stretching by slaves. Apparently >it doesn't support repeated start conditions either, so it wouldn't >surprise me. > >-- >Jean Delvare >-- >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
Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field
On Thu, 13 Jan 2011 11:34:42 -0500, Andy Walls wrote: > How should clock stretches by slaves be handled using i2c-algo-bit? It is already handled. But hdpvr-i2c doesn't use i2c-algo-bit. I2C support is done with USB commands instead. Maybe the hardware implementation doesn't support clock stretching by slaves. Apparently it doesn't support repeated start conditions either, so it wouldn't surprise me. -- Jean Delvare -- 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
Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field
Devin,
You've seen the clock stretch with your I2C analyzer?
Jean,
How should clock stretches by slaves be handled using i2c-algo-bit?
Regards,
Andy
Devin Heitmueller wrote:
>On Thu, Jan 13, 2011 at 8:21 AM, Jean Delvare wrote:
>> My bet is that register at 0x00 is a control register, and writing bit
>> 7 (value 0x80) makes the chip busy enough that it can't process I2C
>> requests at the same time. The following naks would be until the
>> chip is operational again.
>
>Correct. Poking bit 7 of 0xE0:00 triggers the "send" for all the
>bytes that were previously loaded into the device. It puts the chip
>into a busy state, doing an i2c clock stretch until it is available
>again. During that time it cannot see any i2c traffic, which is why
>you are getting NAKs.
>
>Devin
>
>--
>Devin J. Heitmueller - Kernel Labs
>http://www.kernellabs.com
>--
>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
N�r��yb�X��ǧv�^�){.n�+{���bj)w* jg��� �ݢj/���z�ޖ��2�ޙ&�)ߡ�a���� �G���h��j:+v���w��٥
Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field
On Thu, Jan 13, 2011 at 8:21 AM, Jean Delvare wrote: > My bet is that register at 0x00 is a control register, and writing bit > 7 (value 0x80) makes the chip busy enough that it can't process I2C > requests at the same time. The following naks would be until the > chip is operational again. Correct. Poking bit 7 of 0xE0:00 triggers the "send" for all the bytes that were previously loaded into the device. It puts the chip into a busy state, doing an i2c clock stretch until it is available again. During that time it cannot see any i2c traffic, which is why you are getting NAKs. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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
Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field
On Wed, 05 Jan 2011 20:02:41 -0200, Mauro Carvalho Chehab wrote:
> Em 05-01-2011 19:51, Jean Delvare escreveu:
> > If you have specific cases you don't know how to solve, please point me
> > to them and I'll take a look.
>
> You can take a look at saa7134-cards.c, for example. saa7134_tuner_setup()
> has several examples. It starts with this one:
>
> switch (dev->board) {
> case SAA7134_BOARD_BMK_MPEX_NOTUNER:
> case SAA7134_BOARD_BMK_MPEX_TUNER:
> /* Checks if the device has a tuner at 0x60 addr
> If the device doesn't have a tuner, TUNER_ABSENT
> will be used at tuner_type, avoiding loading tuner
> without needing it
>*/
> dev->i2c_client.addr = 0x60;
> board = (i2c_master_recv(&dev->i2c_client, &buf, 0) < 0)
> ? SAA7134_BOARD_BMK_MPEX_NOTUNER
> : SAA7134_BOARD_BMK_MPEX_TUNER;
>
> In this specific case, it is simply a probe for a device at address 0x60, but
This call to i2c_master_recv() could be replaced easily with
i2c_transfer(), which doesn't require an i2c_client.
Alternatively, you could delay the probe until you are ready to
instantiate the tuner device, and use i2c_new_device() when you're sure
it's there, and i2c_new_probed_device() when you aren't. This would be
nicer, but this also requires non-trivial changes.
> there are more complex cases there, with eeprom reads and/or some random init
> that happens before actually attaching some driver at the i2c address.
> It is known to work, but it sounds like a hack.
For eeprom reads, I would definitely recommend getting a clean
i2c_client from i2c-core using i2c_new_dummy() or i2c_new_device().
For "random init", well, I guess each case is different, so I can't
make a general statement.
--
Jean Delvare
--
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
Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field
On Wed, 05 Jan 2011 19:46:20 -0500, Andy Walls wrote: > If you look at more of the dumps, it appears that accesses to I2C > addresses 0x70 and 0x71 can be interleaved, so it looks like the > IR.ir_lock might not be needed. Although looking further I see this: > > 2.035mS: 70 W 61 00 00 00 . . . > 10.887mS: 70 W 00 40 @ > 10.012mS: 70 W 00 > 681uS: 70 r A0 > 717uS: 70 W 00 80 . > 18.808mS: 70 W -nak- > 1.393mS: 70 W -nak- > 1.393mS: 70 W -nak- > 1.396mS: 70 W -nak- > 1.393mS: 70 W -nak- > 1.393mS: 70 W -nak- > [...] > 1.393mS: 70 W -nak- > 1.477mS: 71 W -nak- > 1.391mS: 71 W -nak- > 1.393mS: 71 W -nak- > 1.393mS: 71 W -nak- > 1.391mS: 71 W -nak- > 1.438mS: 71 W 00 > 681uS: 71 r 00 00 00 00 00 00 . . . . . > 51.079mS: 70 W 00 > 681uS: 70 r 80 > > Which seems to indicate that actions taken on the Transmit side of the > chip can cause it to go unresponsive for both Tx and Rx. The "goto > done;" statement that was in lirc_zilog skips the code that deals with > those -nak- for the HD PVR. My bet is that register at 0x00 is a control register, and writing bit 7 (value 0x80) makes the chip busy enough that it can't process I2C requests at the same time. The following naks would be until the chip is operational again. In fact, the "waiting" code in lirc_zilog.c:send_code() makes a lot of sense, and I wouldn't skip it: if the device is busy, you don't want to return immediately, otherwise a subsequent request will fail. The failure documented for the HD PVR simply suggests that the wait loop isn't long enough. It is 20 * 50 ms currently, i.e. 1 second total, maybe this isn't sufficient. Have you ever tried a longer delay? -- Jean Delvare -- 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
Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field
Hi Andy, On Wed, 05 Jan 2011 20:20:35 -0500, Andy Walls wrote: > The cx18 driver has a function scope i2c_client for reading the EEPROM, > and there's a good reason for it. We don't want to register the EEPROM > with the I2C system and make it visible to the rest of the system, > including i2c-dev and user-space tools. To avoid EEPROM corruption, > it's better keep communication with EEPROMs to a very limited scope. Note that it is possible to declare a read-only EEPROM, so that user-space has no chance to write to it. If you really don't want user-space to touch it, the best approach is i2c_new_dummy(), because it will mark the slave address as busy, preventing i2c-dev from accessing it (unless the user forces access - but then he/she is obviously on his/her own...) The i2c_client provided by i2c_new_dummy() can be unregistered at the end of the function which needs it, or kept for the lifetime of the driver, as you prefer. -- Jean Delvare -- 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
Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field
On Wed, 2011-01-05 at 20:02 -0200, Mauro Carvalho Chehab wrote:
> Em 05-01-2011 19:51, Jean Delvare escreveu:
> > Hi Mauro,
> >
> > On Wed, 05 Jan 2011 15:34:28 -0200, Mauro Carvalho Chehab wrote:
> >> Hi Jean,
> >>
> >> Thanks for your acks for patches 1 and 2. I've already applied the patches
> >> on my tree and at linux-next. I'll try to add the acks on it before sending
> >> upstream.
> >
> > If you can't, it's fine. I merely wanted to show my support to Andy's
> > work, I don't care if I'm not counted as a reviewer for these small
> > patches.
>
> Ok. So, it is probably better to keep it as-is, to avoid rebasing and having
> to wait for a couple days at linux-next before sending the git pull request.
>
> >
> >> Em 05-01-2011 12:45, Jean Delvare escreveu:
> >>> From a purely technical perspective, changing client->addr in the
> >>> probe() function is totally prohibited.
> >>
> >> Agreed. Btw, there are some other hacks with client->addr abuse on some
> >> other random places at drivers/media, mostly at the device bridge code,
> >> used to test if certain devices are present and/or to open some I2C gates
> >> before doing some init code. People use this approach as it provides a
> >> fast way to do some things. On several cases, the amount of code for
> >> doing such hack is very small, when compared to writing a new I2C driver
> >> just to do some static initialization code. Not sure what would be the
> >> better approach to fix them.
> >
> > Hard to tell without seeing the exact code. Ideally,
> > i2c_new_dummy() would cover these cases: you don't need to write an
> > actual driver for the device, it's perfectly OK to use the freshly
> > instantiated i2c_client from the bridge driver directly. Alternatively,
> > i2c_smbus_xfer() or i2c_transfer() can be used for one-time data
> > exchange with any slave on the bus as long as you know what you're
> > doing (i.e. you know that no i2c_client will ever be instantiated for
> > this slave.)
> >
> > If you have specific cases you don't know how to solve, please point me
> > to them and I'll take a look.
>
> You can take a look at saa7134-cards.c, for example. saa7134_tuner_setup()
> has several examples. It starts with this one:
>
> switch (dev->board) {
> case SAA7134_BOARD_BMK_MPEX_NOTUNER:
> case SAA7134_BOARD_BMK_MPEX_TUNER:
> /* Checks if the device has a tuner at 0x60 addr
> If the device doesn't have a tuner, TUNER_ABSENT
> will be used at tuner_type, avoiding loading tuner
> without needing it
>*/
> dev->i2c_client.addr = 0x60;
> board = (i2c_master_recv(&dev->i2c_client, &buf, 0) < 0)
> ? SAA7134_BOARD_BMK_MPEX_NOTUNER
> : SAA7134_BOARD_BMK_MPEX_TUNER;
>
> In this specific case, it is simply a probe for a device at address 0x60, but
> there are more complex cases there, with eeprom reads and/or some random init
> that happens before actually attaching some driver at the i2c address.
> It is known to work, but it sounds like a hack.
The cx18 driver has a function scope i2c_client for reading the EEPROM,
and there's a good reason for it. We don't want to register the EEPROM
with the I2C system and make it visible to the rest of the system,
including i2c-dev and user-space tools. To avoid EEPROM corruption,
it's better keep communication with EEPROMs to a very limited scope.
Regards,
Andy
> Cheers,
> Mauro
> --
> 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
--
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
Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field
On Wed, 2011-01-05 at 15:45 +0100, Jean Delvare wrote:
> Hi Andy,
>
> On Tue, 28 Dec 2010 20:49:50 -0500, Andy Walls wrote:
> > Remove use of deprecated struct i2c_adapter.id field. In the process,
> > perform different detection of the HD PVR's Z8 IR microcontroller versus
> > the other Hauppauge cards with the Z8 IR microcontroller.
>
> Thanks a lot for doing this. I'll be very happy when we can finally get
> rid of i2c_adapter.id.
You're welcome. If I hadn't done it, I was worried that lirc_zilog
would have been deleted. I want to fix lirc_zilog properly, but haven't
had the time yet.
Keep in mind, that in this patch I had one objective: remove use of
struct i2c_adapter.id .
I turned a blind-eye to other obvious problems, since fixing
lirc_zilog's problems looked like it was going to be like peeling an
onion. Once you peel back one layer of problems, you find another one
underneath. ;)
> > Also added a comment about probe() function behavior that needs to be
> > fixed.
>
> See my suggestion inline below.
>
> > Signed-off-by: Andy Walls
> > ---
> > drivers/staging/lirc/lirc_zilog.c | 47
> >
> > 1 files changed, 31 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/staging/lirc/lirc_zilog.c
> > b/drivers/staging/lirc/lirc_zilog.c
> > index 52be6de..ad29bb1 100644
> > --- a/drivers/staging/lirc/lirc_zilog.c
> > +++ b/drivers/staging/lirc/lirc_zilog.c
> > @@ -66,6 +66,7 @@ struct IR {
> > /* Device info */
> > struct mutex ir_lock;
> > int open;
> > + bool is_hdpvr;
> >
> > /* RX device */
> > struct i2c_client c_rx;
> > @@ -206,16 +207,12 @@ static int add_to_buf(struct IR *ir)
> > }
> >
> > /* key pressed ? */
> > -#ifdef I2C_HW_B_HDPVR
> > - if (ir->c_rx.adapter->id == I2C_HW_B_HDPVR) {
> > + if (ir->is_hdpvr) {
> > if (got_data && (keybuf[0] == 0x80))
> > return 0;
> > else if (got_data && (keybuf[0] == 0x00))
> > return -ENODATA;
> > } else if ((ir->b[0] & 0x80) == 0)
> > -#else
> > - if ((ir->b[0] & 0x80) == 0)
> > -#endif
> > return got_data ? 0 : -ENODATA;
> >
> > /* look what we have */
> > @@ -841,15 +838,15 @@ static int send_code(struct IR *ir, unsigned int
> > code, unsigned int key)
> > return ret < 0 ? ret : -EFAULT;
> > }
> >
> > -#ifdef I2C_HW_B_HDPVR
> > /*
> > * The sleep bits aren't necessary on the HD PVR, and in fact, the
> > * last i2c_master_recv always fails with a -5, so for now, we're
> > * going to skip this whole mess and say we're done on the HD PVR
> > */
> > - if (ir->c_rx.adapter->id == I2C_HW_B_HDPVR)
> > - goto done;
> > -#endif
> > + if (ir->is_hdpvr) {
> > + dprintk("sent code %u, key %u\n", code, key);
> > + return 0;
> > + }
>
> I don't get the change. What was wrong with the "goto done"? Now you
> duplicated the dprintk(), and as far as I can see the "done" label is
> left unused.
Mauro removed the "done:" label in a commit just prior to this one.
Otherwise, yes, I would have used a "goto done:".
So much needs cleaning up in lirc_zilog, that I didn't agonize over this
particular item.
> >
> > /*
> > * This bit NAKs until the device is ready, so we retry it
> > @@ -,12 +1108,14 @@ static int ir_remove(struct i2c_client *client);
> > static int ir_probe(struct i2c_client *client, const struct i2c_device_id
> > *id);
> > static int ir_command(struct i2c_client *client, unsigned int cmd, void
> > *arg);
> >
> > +#define ID_FLAG_TX 0x01
> > +#define ID_FLAG_HDPVR 0x02
> > +
> > static const struct i2c_device_id ir_transceiver_id[] = {
> > - /* Generic entry for any IR transceiver */
> > - { "ir_video", 0 },
> > - /* IR device specific entries should be added here */
> > - { "ir_tx_z8f0811_haup", 0 },
> > - { "ir_rx_z8f0811_haup", 0 },
> > + { "ir_tx_z8f0811_haup", ID_FLAG_TX },
> > + { "ir_rx_z8f0811_haup", 0 },
> > + { "ir_tx_z8f0811_hdpvr", ID_FLAG_HDPVR | ID_FLAG_TX },
> > + { "ir_rx_z8f0811_hdpvr", ID_FLAG_HDPVR },
> > { }
> > };
> >
> > @@ -1196,10 +1195,25 @@ static int ir_probe(struct i2c_client *client,
> > const struct i2c_device_id *id)
> > int ret;
> > int have_rx = 0, have_tx = 0;
> >
> > - dprintk("%s: adapter id=0x%x, client addr=0x%02x\n",
> > - __func__, adap->id, client->addr);
> > + dprintk("%s: adapter name (%s) nr %d, i2c_device_id name (%s), "
> > + "client addr=0x%02x\n",
> > + __func__, adap->name, adap->nr, id->name, client->addr);
>
> The debug message format is long and confusing. What about:
>
> dprintk("%s: %s on i2c-%d (%s), client addr=0x%02x\n",
> __func__, id->name, adap->nr, adap->name, client->addr);
Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field
Em 05-01-2011 19:51, Jean Delvare escreveu:
> Hi Mauro,
>
> On Wed, 05 Jan 2011 15:34:28 -0200, Mauro Carvalho Chehab wrote:
>> Hi Jean,
>>
>> Thanks for your acks for patches 1 and 2. I've already applied the patches
>> on my tree and at linux-next. I'll try to add the acks on it before sending
>> upstream.
>
> If you can't, it's fine. I merely wanted to show my support to Andy's
> work, I don't care if I'm not counted as a reviewer for these small
> patches.
Ok. So, it is probably better to keep it as-is, to avoid rebasing and having
to wait for a couple days at linux-next before sending the git pull request.
>
>> Em 05-01-2011 12:45, Jean Delvare escreveu:
>>> From a purely technical perspective, changing client->addr in the
>>> probe() function is totally prohibited.
>>
>> Agreed. Btw, there are some other hacks with client->addr abuse on some
>> other random places at drivers/media, mostly at the device bridge code,
>> used to test if certain devices are present and/or to open some I2C gates
>> before doing some init code. People use this approach as it provides a
>> fast way to do some things. On several cases, the amount of code for
>> doing such hack is very small, when compared to writing a new I2C driver
>> just to do some static initialization code. Not sure what would be the
>> better approach to fix them.
>
> Hard to tell without seeing the exact code. Ideally,
> i2c_new_dummy() would cover these cases: you don't need to write an
> actual driver for the device, it's perfectly OK to use the freshly
> instantiated i2c_client from the bridge driver directly. Alternatively,
> i2c_smbus_xfer() or i2c_transfer() can be used for one-time data
> exchange with any slave on the bus as long as you know what you're
> doing (i.e. you know that no i2c_client will ever be instantiated for
> this slave.)
>
> If you have specific cases you don't know how to solve, please point me
> to them and I'll take a look.
You can take a look at saa7134-cards.c, for example. saa7134_tuner_setup()
has several examples. It starts with this one:
switch (dev->board) {
case SAA7134_BOARD_BMK_MPEX_NOTUNER:
case SAA7134_BOARD_BMK_MPEX_TUNER:
/* Checks if the device has a tuner at 0x60 addr
If the device doesn't have a tuner, TUNER_ABSENT
will be used at tuner_type, avoiding loading tuner
without needing it
*/
dev->i2c_client.addr = 0x60;
board = (i2c_master_recv(&dev->i2c_client, &buf, 0) < 0)
? SAA7134_BOARD_BMK_MPEX_NOTUNER
: SAA7134_BOARD_BMK_MPEX_TUNER;
In this specific case, it is simply a probe for a device at address 0x60, but
there are more complex cases there, with eeprom reads and/or some random init
that happens before actually attaching some driver at the i2c address.
It is known to work, but it sounds like a hack.
Cheers,
Mauro
--
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
Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field
Hi Mauro, On Wed, 05 Jan 2011 15:34:28 -0200, Mauro Carvalho Chehab wrote: > Hi Jean, > > Thanks for your acks for patches 1 and 2. I've already applied the patches > on my tree and at linux-next. I'll try to add the acks on it before sending > upstream. If you can't, it's fine. I merely wanted to show my support to Andy's work, I don't care if I'm not counted as a reviewer for these small patches. > Em 05-01-2011 12:45, Jean Delvare escreveu: > > From a purely technical perspective, changing client->addr in the > > probe() function is totally prohibited. > > Agreed. Btw, there are some other hacks with client->addr abuse on some > other random places at drivers/media, mostly at the device bridge code, > used to test if certain devices are present and/or to open some I2C gates > before doing some init code. People use this approach as it provides a > fast way to do some things. On several cases, the amount of code for > doing such hack is very small, when compared to writing a new I2C driver > just to do some static initialization code. Not sure what would be the > better approach to fix them. Hard to tell without seeing the exact code. Ideally, i2c_new_dummy() would cover these cases: you don't need to write an actual driver for the device, it's perfectly OK to use the freshly instantiated i2c_client from the bridge driver directly. Alternatively, i2c_smbus_xfer() or i2c_transfer() can be used for one-time data exchange with any slave on the bus as long as you know what you're doing (i.e. you know that no i2c_client will ever be instantiated for this slave.) If you have specific cases you don't know how to solve, please point me to them and I'll take a look. -- Jean Delvare -- 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
Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field
Hi Jean,
Thanks for your acks for patches 1 and 2. I've already applied the patches
on my tree and at linux-next. I'll try to add the acks on it before sending
upstream.
Em 05-01-2011 12:45, Jean Delvare escreveu:
> Hi Andy,
>
> On Tue, 28 Dec 2010 20:49:50 -0500, Andy Walls wrote:
>> Remove use of deprecated struct i2c_adapter.id field. In the process,
>> perform different detection of the HD PVR's Z8 IR microcontroller versus
>> the other Hauppauge cards with the Z8 IR microcontroller.
>
> Thanks a lot for doing this. I'll be very happy when we can finally get
> rid of i2c_adapter.id.
>
>> Also added a comment about probe() function behavior that needs to be
>> fixed.
>
> See my suggestion inline below.
>
>> Signed-off-by: Andy Walls
>> ---
>> drivers/staging/lirc/lirc_zilog.c | 47
>>
>> 1 files changed, 31 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/staging/lirc/lirc_zilog.c
>> b/drivers/staging/lirc/lirc_zilog.c
>> index 52be6de..ad29bb1 100644
>> --- a/drivers/staging/lirc/lirc_zilog.c
>> +++ b/drivers/staging/lirc/lirc_zilog.c
>> @@ -66,6 +66,7 @@ struct IR {
>> /* Device info */
>> struct mutex ir_lock;
>> int open;
>> +bool is_hdpvr;
>>
>> /* RX device */
>> struct i2c_client c_rx;
>> @@ -206,16 +207,12 @@ static int add_to_buf(struct IR *ir)
>> }
>>
>> /* key pressed ? */
>> -#ifdef I2C_HW_B_HDPVR
>> -if (ir->c_rx.adapter->id == I2C_HW_B_HDPVR) {
>> +if (ir->is_hdpvr) {
>> if (got_data && (keybuf[0] == 0x80))
>> return 0;
>> else if (got_data && (keybuf[0] == 0x00))
>> return -ENODATA;
>> } else if ((ir->b[0] & 0x80) == 0)
>> -#else
>> -if ((ir->b[0] & 0x80) == 0)
>> -#endif
>> return got_data ? 0 : -ENODATA;
>>
>> /* look what we have */
>> @@ -841,15 +838,15 @@ static int send_code(struct IR *ir, unsigned int code,
>> unsigned int key)
>> return ret < 0 ? ret : -EFAULT;
>> }
>>
>> -#ifdef I2C_HW_B_HDPVR
>> /*
>> * The sleep bits aren't necessary on the HD PVR, and in fact, the
>> * last i2c_master_recv always fails with a -5, so for now, we're
>> * going to skip this whole mess and say we're done on the HD PVR
>> */
>> -if (ir->c_rx.adapter->id == I2C_HW_B_HDPVR)
>> -goto done;
>> -#endif
>> +if (ir->is_hdpvr) {
>> +dprintk("sent code %u, key %u\n", code, key);
>> +return 0;
>> +}
>
> I don't get the change. What was wrong with the "goto done"? Now you
> duplicated the dprintk(), and as far as I can see the "done" label is
> left unused.
You probably missed some other changes here. There's a patch fixing the warning
that happens due to the done: label that it was not used.
While this code is, in practice, not used, as IR support is disabled at hdpvr,
I don't see much sense on trying to optimize its code. I'd rather prefer
to see some patches re-enabling IR support on hdpvr and fixing the remaining
issues at lirc_zilog, converting it to use RC core.
>> /*
>> * This bit NAKs until the device is ready, so we retry it
>> @@ -,12 +1108,14 @@ static int ir_remove(struct i2c_client *client);
>> static int ir_probe(struct i2c_client *client, const struct i2c_device_id
>> *id);
>> static int ir_command(struct i2c_client *client, unsigned int cmd, void
>> *arg);
>>
>> +#define ID_FLAG_TX 0x01
>> +#define ID_FLAG_HDPVR 0x02
>> +
>> static const struct i2c_device_id ir_transceiver_id[] = {
>> -/* Generic entry for any IR transceiver */
>> -{ "ir_video", 0 },
>> -/* IR device specific entries should be added here */
>> -{ "ir_tx_z8f0811_haup", 0 },
>> -{ "ir_rx_z8f0811_haup", 0 },
>> +{ "ir_tx_z8f0811_haup", ID_FLAG_TX },
>> +{ "ir_rx_z8f0811_haup", 0 },
>> +{ "ir_tx_z8f0811_hdpvr", ID_FLAG_HDPVR | ID_FLAG_TX },
>> +{ "ir_rx_z8f0811_hdpvr", ID_FLAG_HDPVR },
>> { }
>> };
>>
>> @@ -1196,10 +1195,25 @@ static int ir_probe(struct i2c_client *client, const
>> struct i2c_device_id *id)
>> int ret;
>> int have_rx = 0, have_tx = 0;
>>
>> -dprintk("%s: adapter id=0x%x, client addr=0x%02x\n",
>> -__func__, adap->id, client->addr);
>> +dprintk("%s: adapter name (%s) nr %d, i2c_device_id name (%s), "
>> +"client addr=0x%02x\n",
>> +__func__, adap->name, adap->nr, id->name, client->addr);
>
> The debug message format is long and confusing. What about:
>
> dprintk("%s: %s on i2c-%d (%s), client addr=0x%02x\n",
> __func__, id->name, adap->nr, adap->name, client->addr);
Agreed.
>
>>
>> /*
>> + * FIXME - This probe function probes both the Tx and Rx
>> + * addresses of the IR microcontroller.
>> +
Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field
Hi Andy,
On Tue, 28 Dec 2010 20:49:50 -0500, Andy Walls wrote:
> Remove use of deprecated struct i2c_adapter.id field. In the process,
> perform different detection of the HD PVR's Z8 IR microcontroller versus
> the other Hauppauge cards with the Z8 IR microcontroller.
Thanks a lot for doing this. I'll be very happy when we can finally get
rid of i2c_adapter.id.
> Also added a comment about probe() function behavior that needs to be
> fixed.
See my suggestion inline below.
> Signed-off-by: Andy Walls
> ---
> drivers/staging/lirc/lirc_zilog.c | 47
> 1 files changed, 31 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/lirc/lirc_zilog.c
> b/drivers/staging/lirc/lirc_zilog.c
> index 52be6de..ad29bb1 100644
> --- a/drivers/staging/lirc/lirc_zilog.c
> +++ b/drivers/staging/lirc/lirc_zilog.c
> @@ -66,6 +66,7 @@ struct IR {
> /* Device info */
> struct mutex ir_lock;
> int open;
> + bool is_hdpvr;
>
> /* RX device */
> struct i2c_client c_rx;
> @@ -206,16 +207,12 @@ static int add_to_buf(struct IR *ir)
> }
>
> /* key pressed ? */
> -#ifdef I2C_HW_B_HDPVR
> - if (ir->c_rx.adapter->id == I2C_HW_B_HDPVR) {
> + if (ir->is_hdpvr) {
> if (got_data && (keybuf[0] == 0x80))
> return 0;
> else if (got_data && (keybuf[0] == 0x00))
> return -ENODATA;
> } else if ((ir->b[0] & 0x80) == 0)
> -#else
> - if ((ir->b[0] & 0x80) == 0)
> -#endif
> return got_data ? 0 : -ENODATA;
>
> /* look what we have */
> @@ -841,15 +838,15 @@ static int send_code(struct IR *ir, unsigned int code,
> unsigned int key)
> return ret < 0 ? ret : -EFAULT;
> }
>
> -#ifdef I2C_HW_B_HDPVR
> /*
>* The sleep bits aren't necessary on the HD PVR, and in fact, the
>* last i2c_master_recv always fails with a -5, so for now, we're
>* going to skip this whole mess and say we're done on the HD PVR
>*/
> - if (ir->c_rx.adapter->id == I2C_HW_B_HDPVR)
> - goto done;
> -#endif
> + if (ir->is_hdpvr) {
> + dprintk("sent code %u, key %u\n", code, key);
> + return 0;
> + }
I don't get the change. What was wrong with the "goto done"? Now you
duplicated the dprintk(), and as far as I can see the "done" label is
left unused.
>
> /*
>* This bit NAKs until the device is ready, so we retry it
> @@ -,12 +1108,14 @@ static int ir_remove(struct i2c_client *client);
> static int ir_probe(struct i2c_client *client, const struct i2c_device_id
> *id);
> static int ir_command(struct i2c_client *client, unsigned int cmd, void
> *arg);
>
> +#define ID_FLAG_TX 0x01
> +#define ID_FLAG_HDPVR0x02
> +
> static const struct i2c_device_id ir_transceiver_id[] = {
> - /* Generic entry for any IR transceiver */
> - { "ir_video", 0 },
> - /* IR device specific entries should be added here */
> - { "ir_tx_z8f0811_haup", 0 },
> - { "ir_rx_z8f0811_haup", 0 },
> + { "ir_tx_z8f0811_haup", ID_FLAG_TX },
> + { "ir_rx_z8f0811_haup", 0 },
> + { "ir_tx_z8f0811_hdpvr", ID_FLAG_HDPVR | ID_FLAG_TX },
> + { "ir_rx_z8f0811_hdpvr", ID_FLAG_HDPVR },
> { }
> };
>
> @@ -1196,10 +1195,25 @@ static int ir_probe(struct i2c_client *client, const
> struct i2c_device_id *id)
> int ret;
> int have_rx = 0, have_tx = 0;
>
> - dprintk("%s: adapter id=0x%x, client addr=0x%02x\n",
> - __func__, adap->id, client->addr);
> + dprintk("%s: adapter name (%s) nr %d, i2c_device_id name (%s), "
> + "client addr=0x%02x\n",
> + __func__, adap->name, adap->nr, id->name, client->addr);
The debug message format is long and confusing. What about:
dprintk("%s: %s on i2c-%d (%s), client addr=0x%02x\n",
__func__, id->name, adap->nr, adap->name, client->addr);
>
> /*
> + * FIXME - This probe function probes both the Tx and Rx
> + * addresses of the IR microcontroller.
> + *
> + * However, the I2C subsystem is passing along one I2C client at a
> + * time, based on matches to the ir_transceiver_id[] table above.
> + * The expectation is that each i2c_client address will be probed
> + * individually by drivers so the I2C subsystem can mark all client
> + * addresses as claimed or not.
> + *
> + * This probe routine causes only one of the client addresses, TX or RX,
> + * to be claimed. This will cause a problem if the I2C subsystem is
> + * subsequently triggered to probe unclaimed clients again.
> + */
This can be easily addressed. You can call i2c_new_dummy() from within
the probe function to register secondary addresses. See
dri
[PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field
Remove use of deprecated struct i2c_adapter.id field. In the process,
perform different detection of the HD PVR's Z8 IR microcontroller versus
the other Hauppauge cards with the Z8 IR microcontroller.
Also added a comment about probe() function behavior that needs to be
fixed.
Signed-off-by: Andy Walls
---
drivers/staging/lirc/lirc_zilog.c | 47
1 files changed, 31 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/lirc/lirc_zilog.c
b/drivers/staging/lirc/lirc_zilog.c
index 52be6de..ad29bb1 100644
--- a/drivers/staging/lirc/lirc_zilog.c
+++ b/drivers/staging/lirc/lirc_zilog.c
@@ -66,6 +66,7 @@ struct IR {
/* Device info */
struct mutex ir_lock;
int open;
+ bool is_hdpvr;
/* RX device */
struct i2c_client c_rx;
@@ -206,16 +207,12 @@ static int add_to_buf(struct IR *ir)
}
/* key pressed ? */
-#ifdef I2C_HW_B_HDPVR
- if (ir->c_rx.adapter->id == I2C_HW_B_HDPVR) {
+ if (ir->is_hdpvr) {
if (got_data && (keybuf[0] == 0x80))
return 0;
else if (got_data && (keybuf[0] == 0x00))
return -ENODATA;
} else if ((ir->b[0] & 0x80) == 0)
-#else
- if ((ir->b[0] & 0x80) == 0)
-#endif
return got_data ? 0 : -ENODATA;
/* look what we have */
@@ -841,15 +838,15 @@ static int send_code(struct IR *ir, unsigned int code,
unsigned int key)
return ret < 0 ? ret : -EFAULT;
}
-#ifdef I2C_HW_B_HDPVR
/*
* The sleep bits aren't necessary on the HD PVR, and in fact, the
* last i2c_master_recv always fails with a -5, so for now, we're
* going to skip this whole mess and say we're done on the HD PVR
*/
- if (ir->c_rx.adapter->id == I2C_HW_B_HDPVR)
- goto done;
-#endif
+ if (ir->is_hdpvr) {
+ dprintk("sent code %u, key %u\n", code, key);
+ return 0;
+ }
/*
* This bit NAKs until the device is ready, so we retry it
@@ -,12 +1108,14 @@ static int ir_remove(struct i2c_client *client);
static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id);
static int ir_command(struct i2c_client *client, unsigned int cmd, void *arg);
+#define ID_FLAG_TX 0x01
+#define ID_FLAG_HDPVR 0x02
+
static const struct i2c_device_id ir_transceiver_id[] = {
- /* Generic entry for any IR transceiver */
- { "ir_video", 0 },
- /* IR device specific entries should be added here */
- { "ir_tx_z8f0811_haup", 0 },
- { "ir_rx_z8f0811_haup", 0 },
+ { "ir_tx_z8f0811_haup", ID_FLAG_TX },
+ { "ir_rx_z8f0811_haup", 0 },
+ { "ir_tx_z8f0811_hdpvr", ID_FLAG_HDPVR | ID_FLAG_TX },
+ { "ir_rx_z8f0811_hdpvr", ID_FLAG_HDPVR },
{ }
};
@@ -1196,10 +1195,25 @@ static int ir_probe(struct i2c_client *client, const
struct i2c_device_id *id)
int ret;
int have_rx = 0, have_tx = 0;
- dprintk("%s: adapter id=0x%x, client addr=0x%02x\n",
- __func__, adap->id, client->addr);
+ dprintk("%s: adapter name (%s) nr %d, i2c_device_id name (%s), "
+ "client addr=0x%02x\n",
+ __func__, adap->name, adap->nr, id->name, client->addr);
/*
+* FIXME - This probe function probes both the Tx and Rx
+* addresses of the IR microcontroller.
+*
+* However, the I2C subsystem is passing along one I2C client at a
+* time, based on matches to the ir_transceiver_id[] table above.
+* The expectation is that each i2c_client address will be probed
+* individually by drivers so the I2C subsystem can mark all client
+* addresses as claimed or not.
+*
+* This probe routine causes only one of the client addresses, TX or RX,
+* to be claimed. This will cause a problem if the I2C subsystem is
+* subsequently triggered to probe unclaimed clients again.
+*/
+ /*
* The external IR receiver is at i2c address 0x71.
* The IR transmitter is at 0x70.
*/
@@ -1241,6 +1255,7 @@ static int ir_probe(struct i2c_client *client, const
struct i2c_device_id *id)
mutex_init(&ir->ir_lock);
mutex_init(&ir->buf_lock);
ir->need_boot = 1;
+ ir->is_hdpvr = (id->driver_data & ID_FLAG_HDPVR) ? true : false;
memcpy(&ir->l, &lirc_template, sizeof(struct lirc_driver));
ir->l.minor = -1;
--
1.7.2.1
--
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
