| Message ID | 1449807000-6457-4-git-send-email-vigneshr@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 11, 2015 at 09:39:58AM +0530, Vignesh R wrote: > + if (spi_flash_read_supported(spi)) { > + struct spi_flash_read_message msg; > + int ret; > + > + msg.buf = buf; > + msg.from = from; > + msg.len = len; > + msg.read_opcode = nor->read_opcode; > + msg.addr_width = nor->addr_width; > + msg.dummy_bytes = dummy; > + /* TODO: Support other combinations */ > + msg.opcode_nbits = SPI_NBITS_SINGLE; > + msg.addr_nbits = SPI_NBITS_SINGLE; > + msg.data_nbits = m25p80_rx_nbits(nor); > + > + ret = spi_flash_read(spi, &msg); > + *retlen = msg.retlen; > + return ret; Looking at this I can't help but think that spi_flash_read() ought to have the stub in rather than the caller. But given that we're pretty much only ever expecting one user I'm not 100% sure it actually matters. Anyway, I applied the first two patches.
On 02/10/2016 01:06 AM, Mark Brown wrote: > On Fri, Dec 11, 2015 at 09:39:58AM +0530, Vignesh R wrote: > >> + if (spi_flash_read_supported(spi)) { >> + struct spi_flash_read_message msg; >> + int ret; >> + >> + msg.buf = buf; >> + msg.from = from; >> + msg.len = len; >> + msg.read_opcode = nor->read_opcode; >> + msg.addr_width = nor->addr_width; >> + msg.dummy_bytes = dummy; >> + /* TODO: Support other combinations */ >> + msg.opcode_nbits = SPI_NBITS_SINGLE; >> + msg.addr_nbits = SPI_NBITS_SINGLE; >> + msg.data_nbits = m25p80_rx_nbits(nor); >> + >> + ret = spi_flash_read(spi, &msg); >> + *retlen = msg.retlen; >> + return ret; > > Looking at this I can't help but think that spi_flash_read() ought to > have the stub in rather than the caller. But given that we're pretty > much only ever expecting one user I'm not 100% sure it actually matters. Well, my initial patch set passed long list of arguments to spi_flash_read(), but Brian suggested to use struct[1] in order to avoid unnecessary churn when things need changed in the API. [1] https://lkml.org/lkml/2015/11/11/454
On Thu, Feb 11, 2016 at 11:03:50AM +0530, Vignesh R wrote: > On 02/10/2016 01:06 AM, Mark Brown wrote: > > On Fri, Dec 11, 2015 at 09:39:58AM +0530, Vignesh R wrote: > >> + if (spi_flash_read_supported(spi)) { > >> + struct spi_flash_read_message msg; > >> + int ret; > > Looking at this I can't help but think that spi_flash_read() ought to > > have the stub in rather than the caller. But given that we're pretty > > much only ever expecting one user I'm not 100% sure it actually matters. > Well, my initial patch set passed long list of arguments to > spi_flash_read(), but Brian suggested to use struct[1] in order to avoid > unnecessary churn when things need changed in the API. I don't see what that has to do with my point?
On 02/13/2016 04:07 AM, Mark Brown wrote: > On Thu, Feb 11, 2016 at 11:03:50AM +0530, Vignesh R wrote: >> On 02/10/2016 01:06 AM, Mark Brown wrote: >>> On Fri, Dec 11, 2015 at 09:39:58AM +0530, Vignesh R wrote: > >>>> + if (spi_flash_read_supported(spi)) { >>>> + struct spi_flash_read_message msg; >>>> + int ret; > >>> Looking at this I can't help but think that spi_flash_read() ought to >>> have the stub in rather than the caller. But given that we're pretty >>> much only ever expecting one user I'm not 100% sure it actually matters. > >> Well, my initial patch set passed long list of arguments to >> spi_flash_read(), but Brian suggested to use struct[1] in order to avoid >> unnecessary churn when things need changed in the API. > > I don't see what that has to do with my point? > AFAIU, your previous comment was to move initialization of spi_flash_read_message struct to spi_flash_read(). This would mean sending long list of arguments to spi_flash_read() which needs to be updated whenever an argument needs to be added/deleted (in future). Instead passing around a struct would be much easier in case of adding/removing parameters. Please correct me if I misunderstood your comment?
On Tue, Feb 16, 2016 at 01:30:49PM +0530, Vignesh R wrote: > On 02/13/2016 04:07 AM, Mark Brown wrote: > > On Thu, Feb 11, 2016 at 11:03:50AM +0530, Vignesh R wrote: > >> On 02/10/2016 01:06 AM, Mark Brown wrote: > >>> Looking at this I can't help but think that spi_flash_read() ought to > >>> have the stub in rather than the caller. But given that we're pretty > >>> much only ever expecting one user I'm not 100% sure it actually matters. > >> Well, my initial patch set passed long list of arguments to > >> spi_flash_read(), but Brian suggested to use struct[1] in order to avoid > >> unnecessary churn when things need changed in the API. > > I don't see what that has to do with my point? > AFAIU, your previous comment was to move initialization of > spi_flash_read_message struct to spi_flash_read(). This would mean No, not at all. I'm talking about how we handle the case where we don't have hardware support for this and need to implement it in software - currently that's in a separate place to the place where we call the driver.
On 02/16/2016 06:08 PM, Mark Brown wrote: > On Tue, Feb 16, 2016 at 01:30:49PM +0530, Vignesh R wrote: >> On 02/13/2016 04:07 AM, Mark Brown wrote: >>> On Thu, Feb 11, 2016 at 11:03:50AM +0530, Vignesh R wrote: >>>> On 02/10/2016 01:06 AM, Mark Brown wrote: > >>>>> Looking at this I can't help but think that spi_flash_read() ought to >>>>> have the stub in rather than the caller. But given that we're pretty >>>>> much only ever expecting one user I'm not 100% sure it actually matters. > >>>> Well, my initial patch set passed long list of arguments to >>>> spi_flash_read(), but Brian suggested to use struct[1] in order to avoid >>>> unnecessary churn when things need changed in the API. > >>> I don't see what that has to do with my point? > >> AFAIU, your previous comment was to move initialization of >> spi_flash_read_message struct to spi_flash_read(). This would mean > > No, not at all. I'm talking about how we handle the case where we don't > have hardware support for this and need to implement it in software - > currently that's in a separate place to the place where we call the > driver. > Yeah, but AFAIK, hardware accelerated read support is applicable for m25p80 flashes only, I doubt whether spi_flash_read() will be used by other types. I felt keeping the software implementation in m25p80_read() will be consistent with m25p80_write().
On 02/17/2016 09:41 PM, R, Vignesh wrote: > > > On 02/16/2016 06:08 PM, Mark Brown wrote: >> On Tue, Feb 16, 2016 at 01:30:49PM +0530, Vignesh R wrote: >>> On 02/13/2016 04:07 AM, Mark Brown wrote: >>>> On Thu, Feb 11, 2016 at 11:03:50AM +0530, Vignesh R wrote: >>>>> On 02/10/2016 01:06 AM, Mark Brown wrote: >> >>>>>> Looking at this I can't help but think that spi_flash_read() ought to >>>>>> have the stub in rather than the caller. But given that we're pretty >>>>>> much only ever expecting one user I'm not 100% sure it actually matters. >> >>>>> Well, my initial patch set passed long list of arguments to >>>>> spi_flash_read(), but Brian suggested to use struct[1] in order to avoid >>>>> unnecessary churn when things need changed in the API. >> >>>> I don't see what that has to do with my point? >> >>> AFAIU, your previous comment was to move initialization of >>> spi_flash_read_message struct to spi_flash_read(). This would mean >> >> No, not at all. I'm talking about how we handle the case where we don't >> have hardware support for this and need to implement it in software - >> currently that's in a separate place to the place where we call the >> driver. >> > > Yeah, but AFAIK, hardware accelerated read support is applicable for > m25p80 flashes only, I doubt whether spi_flash_read() will be used by > other types. I felt keeping the software implementation in m25p80_read() > will be consistent with m25p80_write(). Is there any further work required on the patch? If not, what's the plan to merge this patch?
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index fe9ceb7b5405..00094a668c62 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -131,6 +131,26 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len, /* convert the dummy cycles to the number of bytes */ dummy /= 8; + if (spi_flash_read_supported(spi)) { + struct spi_flash_read_message msg; + int ret; + + msg.buf = buf; + msg.from = from; + msg.len = len; + msg.read_opcode = nor->read_opcode; + msg.addr_width = nor->addr_width; + msg.dummy_bytes = dummy; + /* TODO: Support other combinations */ + msg.opcode_nbits = SPI_NBITS_SINGLE; + msg.addr_nbits = SPI_NBITS_SINGLE; + msg.data_nbits = m25p80_rx_nbits(nor); + + ret = spi_flash_read(spi, &msg); + *retlen = msg.retlen; + return ret; + } + spi_message_init(&m); memset(t, 0, (sizeof t));
Certain spi controllers may provide accelerated interface to read from m25p80 type flash devices. This interface provides better read performance than regular SPI interface. Call spi_flash_read(), if supported, to make use of such interface. Signed-off-by: Vignesh R <vigneshr@ti.com> --- v5: No changes drivers/mtd/devices/m25p80.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)