lkml.org 
[lkml]   [2019]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface
    On Mon, Apr 29, 2019 at 11:31:14AM +0000, Naga Sureshkumar Relli wrote:
    > But just wanted to know, do you see issues with these __force and __iomem castings?

    I only see a minor issue: They're (deliberately) lengthy. Using many of
    them diverts attention of the reader. Therefore, my proposal attempted
    to reduce their frequency. The only issue I see here is readability.

    > >
    > > > + u8 addr_cycles;
    > > > + struct clk *mclk;
    > >
    > > All you need here is the memory clock frequency. Wouldn't it be easier to extract that
    > > frequency once during probe and store it here? That assumes a constant frequency, but if the
    > > frequency isn't constant, you have a race condition.
    > That is what we are doing in the probe.
    > In the probe, we are getting mclk using of_clk_get() and then we are getting the actual frequency
    > Using clk_get_rate().
    > And this is constant frequency only(getting from dts)

    Not quite. You're getting a clock reference in probe and then repeatedly
    access the frequency elswhere. I am suggesting that you get the clock
    frequency during probe and never save the clock reference to a struct.

    > > > + case NAND_OP_ADDR_INSTR:
    > > > + offset = nand_subop_get_addr_start_off(subop, op_id);
    > > > + naddrs = nand_subop_get_num_addr_cyc(subop, op_id);
    > > > + addrs = &instr->ctx.addr.addrs[offset];
    > > > + nfc_op->addrs = instr->ctx.addr.addrs[offset];
    > > > + for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) {
    > > > + nfc_op->addrs |= instr->ctx.addr.addrs[i] <<
    > >
    > > I don't quite understand what this code does, but it looks strange to me. I compared it to other
    > > drivers. The code here is quite similar to marvell_nand.c. It seems like we are copying a
    > > varying number (0 to 6) of addresses from the buffer instr->ctx.addr.addrs. However their
    > > indices are special: 0, 1, 2, 3, offset + 4, offset + 5. This is non-consecutive and different from
    > > marvell_nand.c in this regard. Could it be that you really meant index offset+i here?
    > I didn't get, what you are saying here.
    > It is about updating page and column addresses.
    > Are you asking me to remove nfc_op->addrs = instr->ctx.addr.addrs[offset]; before for loop?

    I compared this code to marvell_nand.c and noticed a subtle difference.
    Both snippets read 6 address bytes and consume them in a driver-specific
    way. Now which address bytes are consumed differs.

    marvell_nand.c consumes instr->ctx.addr.addrs at indices offset,
    offset+1, offset+2, offset+3, offset+4, offset+5. pl353_nand.c consumes
    instr->ctx.addr.addrs at indices 0, 1, 2, 3, offset, offset+4, offset+5.
    (In my previous mail, I didn't notice that it was also consuming the
    offset index.)

    I would have expected this behaviour to be consistent between different
    drivers. If I assume marvell_nand.c to do the right thing and
    pl353_nand.c to be wrong (which is not necessarily a correct
    assumption), then the code woule likely becom:

    addrs = &instr->ctx.addr.addrs[offset];
    for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) {
    nfc_op->addrs |= addrs[i] << (8 * i);
    // ^^^^^
    }

    Hope this helps.

    Helmut

    \
     
     \ /
      Last update: 2019-04-29 14:19    [W:4.145 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site