lkml.org 
[lkml]   [2021]   [Sep]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v3 8/8] memory: gpmc-omap: "gpmc,device-width" DT property is optional
    Date
    On 15/09/2021 11:11, Roger Quadros wrote:
    > Hi Krzysztof,
    >
    > On 07/09/2021 15:36, Krzysztof Kozlowski wrote:
    >> On 07/09/2021 13:32, Roger Quadros wrote:
    >>> Check for valid gpmc,device-width, nand-bus-width and bank-width
    >>> at one place. Default to 8-bit width if none present.
    >>
    >> I don't understand the message in the context of the patch. The title
    >> says one property is optional - that's it. The message says you
    >> consolidate checks. How is this related to the title?
    >>
    >> The patch itself moves around checking of properties and reads
    >> nand-bus-width *always*. It does not "check at one place" but rather
    >> "check always". In the same time, the patch does not remove
    >> gpmc,device-width check in other place.
    >>
    >> All three elements - the title, message and patch - do different things.
    >> What did you want to achieve here? Can you help in clarifying it?
    >>
    >
    > OK I will explain it better in commit log in next revision. Let me explain here a bit.
    >
    > Prior to this patch it was working like this
    >
    > /* in gpmc_read_settings_dt() */
    > s->device_width = 0; /* invalid width, should be 1 for 8-bit, 2 for 16-bit */
    > of_property_read_u32(np, "gpmc,device-width", s->device_width);
    >
    > /* in gpmc_probe_generic_child () */
    > if (of_device_is_compatible(child, "ti,omap2-nand")) {
    > /* check for nand-bus-width, if absent set s->device_width to 1 (i.e. 8-bit) */
    > } else {
    > /* check for bank-width, if absent and s->device_width not set, error out */
    > }
    >
    > So that means if all three, "gpmc,device-width". "nand-bus-width" and "bank-width" are missing then
    > it would create an error situation.
    >
    > The patch is doing 3 things.
    > 1) Make sure all DT checks related to bus width are being done at one place for better readability.

    Not entirely. The gpmc,device-width is still done in the other place
    because you did not remove it from the code. Unless you meant parsing of
    gpmc,device-width not reading from DT? But then another round of checks
    is in gpmc_cs_program_settings() so not in one place.

    If you consolidate the checks to one place, I would expect the code to
    be removed from other places, so from gpmc_cs_program_settings() and
    gpmc_read_settings_dt(). Since this is not happening, the message
    confuses me.

    > 2) even if all 3 width properties are absent, we will not treat it as error and default to 8-bit.

    This is not mentioned in commit msg.

    > 3) check for nand-bus-width regardless of whether compatible to "ti,omap2-nand" or not.

    Also not mentioned in commit msg.

    Your commit reorganizes parsing and validating the child DT properties
    but it does not change from "multiple place" to "one place".

    At least I don't see it.

    Best regards,
    Krzysztof

    \
     
     \ /
      Last update: 2021-09-16 12:49    [W:3.246 / U:0.576 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site