Messages in this thread | | | Date | Fri, 11 Jul 2014 19:07:31 -0700 | From | Brian Norris <> | Subject | Re: [PATCH V3] Add support for flag status register on Micron chips. |
| |
Hi guys,
Sorry to revisit this way late, and sorry for not paying as much attention initially. I'm prepped to merge v4, but some of the conversation matches what I was just thinking.
On Mon, Apr 28, 2014 at 07:06:17AM +0200, Marek Vasut wrote: > On Saturday, April 26, 2014 at 05:10:13 AM, Huang Shijie wrote: > > On Sat, Apr 26, 2014 at 12:12:24AM +0200, Marek Vasut wrote: > > > > > > the drivers may fills this hook itself, so the code should like this: > > > > > > -------------------------------------------------- > > > > > > > > > > > > if ((info->flags & USE_FSR) && > > > > > > > > > > > > nor->wait_till_ready == spi_nor_wait_till_fsr_ready) > > > > > > > > > > > > nor->wait_till_ready = spi_nor_wait_till_fsr_ready; > > > > > > > > > > > > -------------------------------------------------- > > > > > > > > > > I sense a misdesign of the SPI NOR subsystem here. The subsystem and > > > > > the driver compete for a function pointer here ? I guess one should > > > > > have precedence in some way then ... and also, they should be two > > > > > different pointers, where the subsystem decides which to use. > > > > > > > > the subsystem do not decides which one to use, the driver decides which > > > > one to use. > > > > > > > > If driver has its own @wait_till_ready , it means the driver knows the > > > > feature, and has implemented it in its own @wait_till_ready. > > > > > > > > If the driver does not fill any wait_till_ready, it means the driver > > > > will use the default @wait_till_ready. We can treat the > > > > spi_nor_wait_till_fsr_ready as a default hook too. > > > > > > I see the driver overwriting a hook previously set by the subsystem. This > > > > not sure ;) > > > > The driver set the hooks before the subsystem set these hooks. > > > > If the driver has already set the @wait_till_ready hook before it calls > > the spi_nor_scan, the subsystem will not set the hook anymore. > > > > Please see the spi_nor_check(). > > Two things competing over the same pointer looks misdesigned to me. I will need > to dig into this one more time ...
Yes, that is misdesigned. And looking at nand_base for examples is not foolproof; it has quite a bit of legacy and workarounds. It'd be best to get the design right for spi-nor.
The subsystem code should not require a function pointer for FSR vs. non-FSR -- all devices should be able to use the same function. We just need to stash some flash-ID-related data (i.e., a flags field) in the spi_nor struct. On the plus side, we can avoid the code duplication between spi_nor_wait_till_fsr_ready() and spi_nor_wait_till_ready().
I think the wait_till_ready pointer should be reserved for the driver, as a hardware-specific "wait" function.
This still leaves the question of whether the SPI NOR core should assume that any driver's 'wait_till_ready' function (if present) actually implements all necessary waits (FSR vs. non-FSR, for instance). I'd argue that's a maintenance burden, and that the subsystem should still do a sanity check that the status register is correct. After all, that's what the ->{read,write}_reg() functions are useful for. But perhaps there is some performance argument for avoiding the (potentially redundant) register checks?
Anyway, I've tested v4, and I plan to merge it soon. Patches can be sent on top. (I may even cook up my own.)
Regards, Brian
| |