Re: [PATCH 1/5] mxl5007t: fix buggy register read
On 04/10/2013 04:38 AM, Devin Heitmueller wrote: On Tue, Apr 9, 2013 at 8:45 PM, Antti Palosaari wrote: Yes, most devices do that, but not all! MxL5007t has a special register for setting register to read. Look the code and you could see it easily. It was over year ago I fixed it... That sounds kind of insane, but I haven't looked at the datasheets and if Mike Ack'd it, then so be it. Further, it *should* be done in a single call to i2c_transfer() or else you won't hold the lock and you will create a race condition. No. That's why I added new lock. Single i2c_transfer() means all messages are done using repeated START condition. No need to add a new lock to your bridge driver. You can just use __i2c_transfer() and i2c_lock_adapter(state->i2c). That's what they're doing in the drx-k driver for just such cases where they need the lock to span multiple calls to i2c_transfer(). Thanks for the tip. It could be cleaner way to do it like that and likely I will change it after quick check. I don't need to lock whole adapter, only unsure there is none changing "register to read" value (stored to special register) before I read it. So I have to ensure I could do normal writes without a unneeded waiting - even those happens just between that two phase read. This sounds more like it's a bug in the i2c master rather than the 5007 driver. No. Do you have i2c bus traces that clearly show that this was the cause of the issue? If we need to define something as "broken" behavior, at first glance it looks like the way *you're* proposing is the broken behavior - presumably to work around a bug in the i2c master not properly supporting repeated start. Yes and no. I made own Cypress FX2 firmware and saw initially that issue then. Also, as you could see looking the following patches I ensured / confirmed issue using two different I2C adapters (AF9015 and AF9035). So I have totally 3 working adapters to prove it (which are broken without that patch)! The whole think sounds screwed up, and without a real i2c trace of the bus we'll never know. That said, I'm not really in a position to dispute it given I don't have any devices with the chip in question. I would suggest renaming the configuration value to something less controversial, such as "use_repeated_start_for_reads" and avoid using terms like broken where it's not clear that's actually the case. It is 100% clearly broken :-] And MxL5007t will not reply correct value. It returns 0x3f for chip ID. I didn't tested it, but quite good theory is that there is some other than chip id reg to set special register when that happens. I have tested it with 3 devices and there is two devices to left which are using that new parameter. regards Antti -- http://palosaari.fi/ -- 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 1/5] mxl5007t: fix buggy register read
On Tue, Apr 9, 2013 at 8:45 PM, Antti Palosaari wrote: > Yes, most devices do that, but not all! > MxL5007t has a special register for setting register to read. Look the code > and you could see it easily. It was over year ago I fixed it... That sounds kind of insane, but I haven't looked at the datasheets and if Mike Ack'd it, then so be it. >> Further, it *should* be done in a single call to i2c_transfer() or >> else you won't hold the lock and you will create a race condition. > > > No. That's why I added new lock. Single i2c_transfer() means all messages > are done using repeated START condition. No need to add a new lock to your bridge driver. You can just use __i2c_transfer() and i2c_lock_adapter(state->i2c). That's what they're doing in the drx-k driver for just such cases where they need the lock to span multiple calls to i2c_transfer(). > >> This sounds more like it's a bug in the i2c master rather than the 5007 >> driver. > > > No. > > >> Do you have i2c bus traces that clearly show that this was the cause >> of the issue? If we need to define something as "broken" behavior, at >> first glance it looks like the way *you're* proposing is the broken >> behavior - presumably to work around a bug in the i2c master not >> properly supporting repeated start. > > > Yes and no. I made own Cypress FX2 firmware and saw initially that issue > then. Also, as you could see looking the following patches I ensured / > confirmed issue using two different I2C adapters (AF9015 and AF9035). So I > have totally 3 working adapters to prove it (which are broken without that > patch)! The whole think sounds screwed up, and without a real i2c trace of the bus we'll never know. That said, I'm not really in a position to dispute it given I don't have any devices with the chip in question. I would suggest renaming the configuration value to something less controversial, such as "use_repeated_start_for_reads" and avoid using terms like broken where it's not clear that's actually the case. 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 1/5] mxl5007t: fix buggy register read
On 04/10/2013 03:20 AM, Devin Heitmueller wrote: On Tue, Apr 9, 2013 at 7:53 PM, Antti Palosaari wrote: Chip uses WRITE + STOP + READ + STOP sequence for I2C register read. Driver was using REPEATED START condition which makes it failing if I2C adapter was implemented correctly. Add use_broken_read_reg_intentionally option to keep old buggy implantation as there is buggy I2C adapter implementation relying that bug... Signed-off-by: Antti Palosaari Hi Antti, The existing code actually looks fine. This is actually how most devices do register reads. Yes, most devices do that, but not all! MxL5007t has a special register for setting register to read. Look the code and you could see it easily. It was over year ago I fixed it... Further, it *should* be done in a single call to i2c_transfer() or else you won't hold the lock and you will create a race condition. No. That's why I added new lock. Single i2c_transfer() means all messages are done using repeated START condition. This sounds more like it's a bug in the i2c master rather than the 5007 driver. No. Do you have i2c bus traces that clearly show that this was the cause of the issue? If we need to define something as "broken" behavior, at first glance it looks like the way *you're* proposing is the broken behavior - presumably to work around a bug in the i2c master not properly supporting repeated start. Yes and no. I made own Cypress FX2 firmware and saw initially that issue then. Also, as you could see looking the following patches I ensured / confirmed issue using two different I2C adapters (AF9015 and AF9035). So I have totally 3 working adapters to prove it (which are broken without that patch)! Also, any reason you didn't put Mike into the cc: for this (since he owns the driver)? you added :) Devin regards Antti -- http://palosaari.fi/ -- 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 1/5] mxl5007t: fix buggy register read
On Tue, Apr 9, 2013 at 7:53 PM, Antti Palosaari wrote: > Chip uses WRITE + STOP + READ + STOP sequence for I2C register read. > Driver was using REPEATED START condition which makes it failing if > I2C adapter was implemented correctly. > > Add use_broken_read_reg_intentionally option to keep old buggy > implantation as there is buggy I2C adapter implementation relying > that bug... > > Signed-off-by: Antti Palosaari Hi Antti, The existing code actually looks fine. This is actually how most devices do register reads. Further, it *should* be done in a single call to i2c_transfer() or else you won't hold the lock and you will create a race condition. This sounds more like it's a bug in the i2c master rather than the 5007 driver. Do you have i2c bus traces that clearly show that this was the cause of the issue? If we need to define something as "broken" behavior, at first glance it looks like the way *you're* proposing is the broken behavior - presumably to work around a bug in the i2c master not properly supporting repeated start. Also, any reason you didn't put Mike into the cc: for this (since he owns the driver)? 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
