Message ID | 20210401195012.4009166-1-yaliang.wang@windriver.com |
---|---|
State | Changes Requested |
Delegated to: | Ambarus Tudor |
Headers | show |
Series | [1/3] mtd: spi-nor: core: Add manuafacturer/chip specific operations | expand |
On 02/04/21 03:50AM, yaliang.wang@windriver.com wrote: > From: Yaliang Wang <Yaliang.Wang@windriver.com> > > This is quite similar to the patch made by Takahiro Kuwano[1], except > replacing the bare ->ready() hook with a structure spi_nor_ops. I don't think this belongs in the commit message. The commit message should describe the change in isolation. All the "metadata" like "depends on patch X", or "rework of patch Y", etc. should go below the 3 dashes [2]. > > The purpose of this introduction is to provide a more flexible method, > so that we can expand it when needed. Also Basing on this, the > manufacturer specific checking ready codes can be shifted into their own > file. > > [1]http://lists.infradead.org/pipermail/linux-mtd/2021-March/085741.html > > Signed-off-by: Yaliang Wang <Yaliang.Wang@windriver.com> > --- [2] Here. > drivers/mtd/spi-nor/core.c | 8 +++++--- > drivers/mtd/spi-nor/core.h | 9 +++++++++ > 2 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 0522304f52fa..6dc8bd0a6bd4 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -785,12 +785,13 @@ static int spi_nor_fsr_ready(struct spi_nor *nor) > } > > /** > - * spi_nor_ready() - Query the flash to see if it is ready for new commands. > + * spi_nor_default_ready() - Query the flash to see if it is ready for new > + * commands. > * @nor: pointer to 'struct spi_nor'. > * > * Return: 1 if ready, 0 if not ready, -errno on errors. > */ > -static int spi_nor_ready(struct spi_nor *nor) > +static int spi_nor_default_ready(struct spi_nor *nor) > { > int sr, fsr; > > @@ -826,7 +827,7 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor, > if (time_after_eq(jiffies, deadline)) > timeout = 1; > > - ret = spi_nor_ready(nor); > + ret = nor->params->ops.ready(nor); > if (ret < 0) > return ret; > if (ret) > @@ -2920,6 +2921,7 @@ static void spi_nor_info_init_params(struct spi_nor *nor) > params->quad_enable = spi_nor_sr2_bit1_quad_enable; > params->set_4byte_addr_mode = spansion_set_4byte_addr_mode; > params->setup = spi_nor_default_setup; > + params->ops.ready = spi_nor_default_ready; > /* Default to 16-bit Write Status (01h) Command */ > nor->flags |= SNOR_F_HAS_16BIT_SR; > > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index 4a3f7f150b5d..bc042a0ef94e 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -187,6 +187,14 @@ struct spi_nor_locking_ops { > int (*is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len); > }; > > +/** > + * struct spi_nor_ops - SPI manuafacturer/chip specific operations > + * @ready: query if a chip is ready. > + */ > +struct spi_nor_ops { > + int (*ready)(struct spi_nor *nor); I don't understand how this is more flexible than just having the ready() hook in spi_nor_flash_parameter like Takahiro's patch did. I'm not completely opposed to the idea but I'd like to understand your thought process first. Also... > +}; > + > /** > * struct spi_nor_flash_parameter - SPI NOR flash parameters and settings. > * Includes legacy flash parameters and settings that can be overwritten > @@ -232,6 +240,7 @@ struct spi_nor_flash_parameter { > struct spi_nor_pp_command page_programs[SNOR_CMD_PP_MAX]; > > struct spi_nor_erase_map erase_map; > + struct spi_nor_ops ops; > > int (*octal_dtr_enable)(struct spi_nor *nor, bool enable); > int (*quad_enable)(struct spi_nor *nor); ... shouldn't octal_dtr_enable(), quad_enable(), set_4byte_addr_mode(), convert_addr(), and setup() hooks also be moved into spi_nor_ops? Why is the ready() hook different from these hooks?
On 4/5/21 11:54 AM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On 02/04/21 03:50AM, yaliang.wang@windriver.com wrote: >> From: Yaliang Wang <Yaliang.Wang@windriver.com> >> >> This is quite similar to the patch made by Takahiro Kuwano[1], >> except replacing the bare ->ready() hook with a structure >> spi_nor_ops. > > I don't think this belongs in the commit message. The commit message > should describe the change in isolation. All the "metadata" like > "depends on patch X", or "rework of patch Y", etc. should go below > the 3 dashes [2]. > >> >> The purpose of this introduction is to provide a more flexible >> method, so that we can expand it when needed. Also Basing on this, >> the manufacturer specific checking ready codes can be shifted into >> their own file. >> >> [1]http://lists.infradead.org/pipermail/linux-mtd/2021-March/085741.html >> >> >> >> Signed-off-by: Yaliang Wang <Yaliang.Wang@windriver.com> >> --- > > [2] Here. > >> drivers/mtd/spi-nor/core.c | 8 +++++--- drivers/mtd/spi-nor/core.h >> | 9 +++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/core.c >> b/drivers/mtd/spi-nor/core.c index 0522304f52fa..6dc8bd0a6bd4 >> 100644 --- a/drivers/mtd/spi-nor/core.c +++ >> b/drivers/mtd/spi-nor/core.c @@ -785,12 +785,13 @@ static int >> spi_nor_fsr_ready(struct spi_nor *nor) } >> >> /** - * spi_nor_ready() - Query the flash to see if it is ready >> for new commands. + * spi_nor_default_ready() - Query the flash to >> see if it is ready for new + * commands. * @nor: pointer to >> 'struct spi_nor'. * * Return: 1 if ready, 0 if not ready, -errno >> on errors. */ -static int spi_nor_ready(struct spi_nor *nor) >> +static int spi_nor_default_ready(struct spi_nor *nor) { int sr, >> fsr; >> >> @@ -826,7 +827,7 @@ static int >> spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor, if >> (time_after_eq(jiffies, deadline)) timeout = 1; >> >> - ret = spi_nor_ready(nor); + ret = >> nor->params->ops.ready(nor); if (ret < 0) return ret; if (ret) @@ >> -2920,6 +2921,7 @@ static void spi_nor_info_init_params(struct >> spi_nor *nor) params->quad_enable = spi_nor_sr2_bit1_quad_enable; >> params->set_4byte_addr_mode = spansion_set_4byte_addr_mode; >> params->setup = spi_nor_default_setup; + params->ops.ready = >> spi_nor_default_ready; /* Default to 16-bit Write Status (01h) >> Command */ nor->flags |= SNOR_F_HAS_16BIT_SR; >> >> diff --git a/drivers/mtd/spi-nor/core.h >> b/drivers/mtd/spi-nor/core.h index 4a3f7f150b5d..bc042a0ef94e >> 100644 --- a/drivers/mtd/spi-nor/core.h +++ >> b/drivers/mtd/spi-nor/core.h @@ -187,6 +187,14 @@ struct >> spi_nor_locking_ops { int (*is_locked)(struct spi_nor *nor, loff_t >> ofs, uint64_t len); }; >> >> +/** + * struct spi_nor_ops - SPI manuafacturer/chip specific >> operations + * @ready: query if a chip is ready. + */ +struct >> spi_nor_ops { + int (*ready)(struct spi_nor *nor); > > I don't understand how this is more flexible than just having the > ready() hook in spi_nor_flash_parameter like Takahiro's patch did. > I'm not completely opposed to the idea but I'd like to understand > your thought process first. > When I proposed the introduction of spi_nor_ops I thought about having one place for storing different register operations or command opcodes. For example, macronix uses for SPINOR_OP_RDCR a 0x15 value instead of 0x35. But maybe we'll find a better place for these when parsing SCCR SFDP table. I'm ok with dropping spi_nor_ops. > Also... > >> +}; + /** * struct spi_nor_flash_parameter - SPI NOR flash >> parameters and settings. * Includes legacy flash parameters and >> settings that can be overwritten @@ -232,6 +240,7 @@ struct >> spi_nor_flash_parameter { struct spi_nor_pp_command >> page_programs[SNOR_CMD_PP_MAX]; >> >> struct spi_nor_erase_map erase_map; + struct >> spi_nor_ops ops; >> >> int (*octal_dtr_enable)(struct spi_nor *nor, bool enable); int >> (*quad_enable)(struct spi_nor *nor); > > ... shouldn't octal_dtr_enable(), quad_enable(), > set_4byte_addr_mode(), convert_addr(), and setup() hooks also be > moved into spi_nor_ops? Why is the ready() hook different from these > hooks? > Right, maybe ready() resembles more the ones that you indicated. As the spi_nor_flash_parameter is not yet big, we can keep all scattered for now.
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 0522304f52fa..6dc8bd0a6bd4 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -785,12 +785,13 @@ static int spi_nor_fsr_ready(struct spi_nor *nor) } /** - * spi_nor_ready() - Query the flash to see if it is ready for new commands. + * spi_nor_default_ready() - Query the flash to see if it is ready for new + * commands. * @nor: pointer to 'struct spi_nor'. * * Return: 1 if ready, 0 if not ready, -errno on errors. */ -static int spi_nor_ready(struct spi_nor *nor) +static int spi_nor_default_ready(struct spi_nor *nor) { int sr, fsr; @@ -826,7 +827,7 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor, if (time_after_eq(jiffies, deadline)) timeout = 1; - ret = spi_nor_ready(nor); + ret = nor->params->ops.ready(nor); if (ret < 0) return ret; if (ret) @@ -2920,6 +2921,7 @@ static void spi_nor_info_init_params(struct spi_nor *nor) params->quad_enable = spi_nor_sr2_bit1_quad_enable; params->set_4byte_addr_mode = spansion_set_4byte_addr_mode; params->setup = spi_nor_default_setup; + params->ops.ready = spi_nor_default_ready; /* Default to 16-bit Write Status (01h) Command */ nor->flags |= SNOR_F_HAS_16BIT_SR; diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h index 4a3f7f150b5d..bc042a0ef94e 100644 --- a/drivers/mtd/spi-nor/core.h +++ b/drivers/mtd/spi-nor/core.h @@ -187,6 +187,14 @@ struct spi_nor_locking_ops { int (*is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len); }; +/** + * struct spi_nor_ops - SPI manuafacturer/chip specific operations + * @ready: query if a chip is ready. + */ +struct spi_nor_ops { + int (*ready)(struct spi_nor *nor); +}; + /** * struct spi_nor_flash_parameter - SPI NOR flash parameters and settings. * Includes legacy flash parameters and settings that can be overwritten @@ -232,6 +240,7 @@ struct spi_nor_flash_parameter { struct spi_nor_pp_command page_programs[SNOR_CMD_PP_MAX]; struct spi_nor_erase_map erase_map; + struct spi_nor_ops ops; int (*octal_dtr_enable)(struct spi_nor *nor, bool enable); int (*quad_enable)(struct spi_nor *nor);