Hi Ollivier On Wed, Apr 20, 2016 at 9:21 AM, Olliver Schinagl <oliver@xxxxxxxxxxx> wrote: What I am propossing is at probe(): replace: if (pdata) { /* Configure output: open-drain or totem pole (push-pull) */ if (pdata->outdrv == PCA963X_OPEN_DRAIN) i2c_smbus_write_byte_data(client, PCA963X_MODE2, 0x01); else i2c_smbus_write_byte_data(client, PCA963X_MODE2, 0x05); } with something like (forgive the spacing): u8 mode2 = PCA963X_MODE2_DMBLNK | 0x1; if (pdata && pdata->outdrv == PCA963X_OPEN_DRAIN) mode2 |= 0x4; i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2, mode2); and then remove from pca963x_blink() these lines: u8 mode2 = i2c_smbus_read_byte_data(pca963x->chip->client, PCA963X_MODE2); if (!(mode2 & PCA963X_MODE2_DMBLNK)) i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2, mode2 | PCA963X_MODE2_DMBLNK); As I said before, the reason for this proposal is that the code NEVER clears PCA963X_MODE2_DMBLNK, only sets it. Unfortunately I do not have the HW to test this change. > > Now I understand your concern, the i2c operations are slow and time > consuming making the mutex very expensive. > > The thing is, to be able to write the blink bit, we need to read the whole > mode2 register, to do a proper read-modify-write. We don't know what's in > the mode2 register and we only want to write the bit if it is actually not > set to begin with, to save a i2c write operation. As I said earlier, nowhere in the code clears that bit. The bit is only set. so no reason to read/modify/write. We can set that bit at probe time and assume that it will not be changed. > > We start this function already however with with two write calls of > sequential registers, the grp and pwm enable registers. There is even a call > to automatically update these registers, which I think we'd use > i2c_master_send() to set the address via the auto-increment register and > enable auto increment of these two registers. Now we reduced the 2 seperate > calls into one bigger 'faster' call. So 1 win there. But! it will require us > however to change the other calls to disable auto increment via de mode1 > register. Since this is an extra i2c_write operation, it makes the other i2c > writes more expensive, which may happen much more often (enable blink only > happens occasionally, changing the brightness may change a lot (fade in fade > out). Be careful with that. Sometimes this chips are connected to the smbus, wich has a limited number of operations/lengths. We should keep the i2c_smbus_write_byte_data() call, because that makes the driver compatible with more hardware. > > So unless i'm totally misunderstanding something, I don't think we can safe > much here at all. To be clear: My proposal is that (after being tested), move the set of DMBLINK to probe, remove the read/modify/write from blink() and keep the locking as it is now, only protecting ledout. Also you need to fix the patch that breaks bisect, but I believe that you are already working on that. I have reviewed a bit Documentation/device-tree and it seems that there is already a binding for active-low. Instead of nxp,active-low you should call it just active-low. But I am not a device-tree expert. Finally, you mention that you are fixing some checkpatch errors, but I cannot replicate those in my side :S ricardo@pilix:~/curro/linux$ scripts/checkpatch.pl -f drivers/leds/leds-pca963x.c total: 0 errors, 0 warnings, 439 lines checked drivers/leds/leds-pca963x.c has no obvious style problems and is ready for submission. So maybe the errors you are fixing are introduced by your patches. About the other style patches, I do not know what is the policy of the Maintainer in that matter, especially when the driver did not break originally checkpatch. Regards! --- Ricardo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html