>>> @@ -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
