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 <awalls@xxxxxxxxxxxxxxxx>
>> ---
>> 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
>> @@ -1111,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.
>> + *
>> + * 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
> drivers/misc/eeprom/at24.c for an example.
>
> That being said, I doubt this is what we want here. i2c_new_dummy() is
> only meant for cases where the board code (or bridge driver in your
> case) declares a single I2C device by its main address. Here, you have
> declared both the TX and RX (sub-)devices in your bridge driver, so
> your I2C device driver's probe() function _will_ be called twice. It
> makes no sense to ask for this in the bridge driver and then to look
> for a way to work around it in the I2C device driver.
>
> Looking at ir_probe(), it seems rather clear to me that it needs to be
> redesigned seriously. This function is still doing old-style device
> detection which does not belong there in the standard device driver
> binding model. The bridge driver did take care of this already so there
> is no point in doing it again.
> 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.
> So we need more changes done to the lirc_zilog driver than your patch
> currently does. In particular:
> * struct IR should _not_ include private i2c_client structures. i2c
> clients are provided by the i2c-core, either as a parameter to the
> probe() function or as the result of i2c_new_dummy(). Private copies
> are never needed if you use the proper binding model.
> * struct IR should probably be split into IR_rx and IR_tx. Having a
> single data structure for both TX and RX doesn't make sense when
> probe() is called once for each.
> * ir_probe() should be cleaned up to do only what we expect from a
> probe function, i.e. initialize and bind. No device detection, no
> hard-coded client address. Note that it may make sense to move the
> disable_rx and disable_tx module parameters to the bridge driver, as
> in the standard device driver binding model, disabling must be done
> earlier (you want to prevent the I2C device from being instantiated.)
>
> I see comments in the code about RX and TX interfering and IR.ir_lock
> being used to prevent that. I presume this is the reason for the
> current driver design. However, I can see that the driver uses
> i2c_master_send() followed by i2c_master_send() or i2c_master_recv()
> when it should use i2c_transfer() to guarantee that nobody uses the
> adapter in-between. Assuming that the Z8 understands the repeated-start
> I2C condition, this should hopefully let you get rid of IR.ir_lock and
> solve the driver design issue.
Yeah, using i2c_transfer() will probably do the trick.
Cheers,
Mauro
--
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
