Re: [PATCH v3 6/9] mtd: spi-nor: core: Add helpers to read/write any register

[Michael Walle <michael@walle.cc>]

Am 2022-04-19 14:32, schrieb Pratyush Yadav:
> On 19/04/22 12:08PM, Tudor.Ambarus@microchip.com wrote:
>> On 4/19/22 14:46, Michael Walle wrote:
>> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>> >
>> > Am 2022-04-19 13:19, schrieb Michael Walle:
>> >> Am 2022-04-11 11:10, schrieb Tudor Ambarus:
>> >>> There are manufacturers that use registers indexed by address. Some of
>> >>> them support "read/write any register" opcodes. Provide core methods
>> >>> that
>> >>> can be used by all manufacturers. SPI NOR controller ops are
>> >>> intentionally
>> >>> not supported as we intend to move all the SPI NOR controller drivers
>> >>> under the SPI subsystem.
>> >>>
>> >>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> >>> Tested-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> >>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>> >>
>> >> I still don't like it because the function doesn't do
>> >> anything what the function name might suggest. The read
>> >> just executes an op, the write executes an op with a
>> >> write enable before. All the behavior is determined by the
>> >> 'op' argument.
>> >>
>> >> Anyway,
>> >> Reviewed-by: Michael Walle <michael@walle.cc>
>> >>
>> >>> ---
>> >>> v3: no changes
>> >>>
>> >>>  drivers/mtd/spi-nor/core.c | 41
>> >>> ++++++++++++++++++++++++++++++++++++++
>> >>>  drivers/mtd/spi-nor/core.h |  4 ++++
>> >>>  2 files changed, 45 insertions(+)
>> >>>
>> >>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> >>> index 6165dc7bfd17..42794328d3b6 100644
>> >>> --- a/drivers/mtd/spi-nor/core.c
>> >>> +++ b/drivers/mtd/spi-nor/core.c
>> >>> @@ -307,6 +307,47 @@ ssize_t spi_nor_write_data(struct spi_nor *nor,
>> >>> loff_t to, size_t len,
>> >>>      return nor->controller_ops->write(nor, to, len, buf);
>> >>>  }
>> >>>
>> >>> +/**
>> >>> + * spi_nor_read_reg() - read register to flash memory
>> >>> + * @nor:        pointer to 'struct spi_nor'.
>> >>> + * @op:             SPI memory operation. op->data.buf must be DMA-able.
>> >>> + * @proto:  SPI protocol to use for the register operation.
>> >>> + *
>> >>> + * Return: zero on success, -errno otherwise
>> >>> + */
>> >>> +int spi_nor_read_reg(struct spi_nor *nor, struct spi_mem_op *op,
>> >>> +                 enum spi_nor_protocol proto)
>> >>> +{
>> >>> +    if (!nor->spimem)
>> >>> +            return -EOPNOTSUPP;
>> >>> +
>> >>> +    spi_nor_spimem_setup_op(nor, op, proto);
>> >>> +    return spi_nor_spimem_exec_op(nor, op);
>> >>> +}
>> >>> +
>> >>> +/**
>> >>> + * spi_nor_write_reg() - write register to flash memory
>> >>> + * @nor:        pointer to 'struct spi_nor'
>> >>> + * @op:             SPI memory operation. op->data.buf must be DMA-able.
>> >>> + * @proto:  SPI protocol to use for the register operation.
>> >>> + *
>> >>> + * Return: zero on success, -errno otherwise
>> >>> + */
>> >>> +int spi_nor_write_reg(struct spi_nor *nor, struct spi_mem_op *op,
>> >>> +                  enum spi_nor_protocol proto)
>> >>> +{
>> >>> +    int ret;
>> >>> +
>> >>> +    if (!nor->spimem)
>> >>> +            return -EOPNOTSUPP;
>> >>> +
>> >>> +    ret = spi_nor_write_enable(nor);
>> >>> +    if (ret)
>> >>> +            return ret;
>> >>> +    spi_nor_spimem_setup_op(nor, op, proto);
>> >>> +    return spi_nor_spimem_exec_op(nor, op);
>> >
>> > After seeing your next two patches. Shouldn't the
>> > spi_nor_wait_until_ready() call be here too?
>> >
>> 
>> I thought of this too, but seems that for a reason that I don't
>> remember, we don't call for spi_nor_wait_until_ready after we
>> write the octal DTR bit. Pratyush, do you remember why?
> 
> We are not sure the protocol changed correctly so we can't rely on
> spi_nor_wait_until_ready(). We read the ID instead to be sure.

So besides the fact that the write_reg only works with the 'correct'
op parameter, it is also tailored to the special use case. For real
write_reg(), the user would actually has to poll the status bit
afterwards? :(

-michael