>>> @@ -294,7 +427,7 @@ static int ocores_init(struct device *dev, struct >>> ocores_i2c *i2c) >> >>> >>> >>> /* Init the device */ >>> oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK); >>> >>> - oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN | OCI2C_CTRL_EN); >>> + oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_EN); >> >> >> You fix this up in patch 5 (in what is perhaps carelessly marketed as >> fixes for minor checkpatch issues), but for this patch you are actually >> no longer making sure that you clear the OCI2C_CTRL_IEN bit as the code >> used to before this patch. I think you intended to preserve that >> behavior, no? > > I think you got confused by the two patches because those lines look very > similar. > > In PATCH 5 what you see is the "style fix" to clear EN and IEN before clock > configuration > > in PATCH 3 (this one) what you see is when later (same function, after clock > configuration) the device is re-enabled (EN), but without enabling the > interrupt because on polling configuration we do not want to see interrupt > flowing. > > So, the behavior is preserved for what concern clearing the IEN bit before > clock configuration. > > am I wrong? No, I don't think I'm confused and I think you are wrong. Current code does this: u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL); /* make sure the device is disabled */ oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~(OCI2C_CTRL_IEN|OCI2C_CTRL_EN)); ... oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN | OCI2C_CTRL_EN)); Here, the final oc_setreg always sets OCI2C_CTRL_IEN. Patch 3 changes it to: u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL); /* make sure the device is disabled */ oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~(OCI2C_CTRL_IEN|OCI2C_CTRL_EN)); ... oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_EN)); Here, the final oc_setreg restores OCI2C_CTRL_IEN to whatever it was at function entry. And patch 5 changes it again to: u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL); /* make sure the device is disabled */ ctrl &= ~(OCI2C_CTRL_IEN | OCI2C_CTRL_EN); oc_setreg(i2c, OCI2C_CONTROL, ctrl); ... oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_EN)); Here, the final oc_setreg keeps OCI2C_CTRL_IEN cleared. I think you wanted this deterministic behavior for patch 3 as well. Cheers, Peter
