Hi Helmut, > -----Original Message----- > From: Helmut Grohne <[email protected]> > Sent: Monday, April 29, 2019 5:48 PM > To: Naga Sureshkumar Relli <[email protected]> > Cc: [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > linux- > [email protected]; [email protected]; Michal Simek > <[email protected]>; > [email protected] > Subject: Re: [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. Ok then, I will update it. > > > > > > > > + 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. Ok. got it. Will update. > > > > > + 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. Ok. let me re check this and I will update this accordingly.
Thanks, Naga Sureshkumar Relli > > Helmut
