lkml.org 
[lkml]   [2019]   [Feb]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v4 3/5] i2c:ocores: add polling interface
    Date
    On Monday, February 11, 2019 2:35:15 PM CET Peter Rosin wrote:
    > >>> @@ -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.

    Now I see what you mean and I agree. I will move the fix from PATCH 5 to PATCH
    3, so that bit IEN is clearly cleared.


    thanks

    >
    > Cheers,
    > Peter




    \
     
     \ /
      Last update: 2019-02-11 14:47    [W:2.634 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site