On Mon, 2011-11-14 at 07:59 -0500, Jean Delvare wrote: > On Mon, 14 Nov 2011 08:48:38 +0000, Tang Yuantian-B29983 wrote: > > > > > > > -----Original Message----- > > > From: Jean Delvare [mailto:khali@xxxxxxxxxxxx] > > > Sent: 2011年11月14日 16:02 > > > To: Tang Yuantian-B29983 > > > Cc: lm-sensors@xxxxxxxxxxxxxx; Tang Yuantian-B29983; Huang Changming- > > > R66093; Tabi Timur-B04825 > > > Subject: Re: [PATCH] hwmon: (pmbus) Fix two issues > > > > > > Hi Tang, > > > > > > On Mon, 14 Nov 2011 13:41:06 +0800, Yuantian.Tang@xxxxxxxxxxxxx wrote: > > > > From: Tang Yuantian <B29983@xxxxxxxxxxxxx> > > > > > > > > 1. Not all platforms support i2c_smbus_read_block_data function. > > > > Use i2c_smbus_read_i2c_block_data instead. > > > > > > You can't do that. These are two different transfer formats, they are not > > > interchangeable. If the device wants the SMBus block format, then you > > > have to use this in the driver. If the bus driver doesn't support that, > > > it has to be fixed. In general it is only a matter of adding support for > > > I2C_M_RECV_LEN to the bus driver. You can check in > > > i2c-core:i2c_smbus_xfer_emulated() for a reference implementation. > > > > > [Yuantian:] almost all platform's i2c driver don't support i2c_smbus_read_block_data. > > Why can't we just use i2c_smbus_read_i2c_block_data instead. > > Beside, according my test, i2c_smbus_read_block_data can't return correct value, > > But i2c_smbus_read_i2c_block_data. > > I think i2c_smbus_read_i2c_block_data is only right choice. > > Again, no. PMBus goes on top of SMBus which includes SMBus block reads > (i2c_smbus_read_block_data) but not I2C block reads > (i2c_smbus_read_i2c_block_data). As a result, many hardware SMBus > controllers do not support I2C block reads, and it can't be added (only > the set of transfers supported by the hardware controller can be > implemented by the driver) which means that your proposed change would > break support on existing systems. This is not acceptable. > > On the other hand, an I2C controller which supports I2C block reads can > almost always get SMBus block reads added, it's only a matter of adding > a few lines of code to the bus driver. > > Which i2c bus driver are you using? From which kernel version? > > > > > 2. Not all zlxx chips's id start with zlxx. zl6100's id starts with > > > > 0x10. Take this situation into account. > > > > > > No, this is incorrect. 0x10 is the first byte returned because SMBus > > > block reads receive the block length as the first byte. 0x10 == 16, which > > > is the length of the ID for these chips, this is no coincidence. > > > This is a bug introduced by your above change of SMBus block read for I2C > > > block read. > > > > > > So, nack. > > > > > [Yuantian:] I misunderstand the 0x10. But the existing zl6100 driver didn't consider it too. > > And still need to be fixed. > > I didn't write the driver, and don't have the device, but I think the > driver is OK as is. The block length (0x10) is handled by the > underlying bus driver, the device driver never gets to see it, if you > properly call i2c_smbus_read_block_data() as you are supposed to. > I have eval boards for all chips supported by the driver (lots of kudos to Intersil), so the driver was tested and is known to work for all chips it currently supports. Intersil told me that they don't update the firmware after they released a chip, so we can be sure that the firmware version on all chips of the same type is the same, and we don't have to look for version specific behavior. I agree that we should not replace the SMBus block read with SMBus I2C block read. The I2C bus driver should really implement SMBus block read command support. There are some I2C bus drivers which can not support SMBus block reads due to chip limitations, such as the Sibyte driver, but if the driver can support SMBus I2C block reads it should possible to support SMBus block reads as well. As far as I can see, the only driver supporting SMBUS I2C block reads but not SMBUS block reads is scx200_acb. That seems to be an oversight (or maybe laziness ;), and should be easy to fix. Thanks, Guenter _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
