| Message ID | c5925ea6-c071-4216-91fb-c25213d6b753@CO9EHSMHS022.ehs.local |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
On Saturday, January 18, 2014 at 09:06:31 PM, Jagannadha Sutradharudu Teki wrote: > - Used readable names for read/write command macros > - Added comments for the same > > Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Simon Glass <sjg@chromium.org> Does this patch have any impact other than making the code harder to understand ? :-( What's the rationale for making the code more cryptic ? Best regards, Marek Vasut
On Sun, Jan 19, 2014 at 2:06 AM, Marek Vasut <marex@denx.de> wrote: > On Saturday, January 18, 2014 at 09:06:31 PM, Jagannadha Sutradharudu Teki > wrote: >> - Used readable names for read/write command macros >> - Added comments for the same >> >> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com> >> Cc: Marek Vasut <marex@denx.de> >> Cc: Simon Glass <sjg@chromium.org> > > Does this patch have any impact other than making the code harder to understand > ? :-( > > What's the rationale for making the code more cryptic ? No issues I guess with the readability as each macro we can easily understand. like CMD_RD_QUAD --> command_read_quad CMD_WR_PAGE --> command_write_page_program And this will minimize the macro length - good for in coding and more over description is added in drivers/mtd/spi/sf_internal.h anyway.
Hi Jagan, > On Sun, Jan 19, 2014 at 2:06 AM, Marek Vasut <marex@denx.de> wrote: >> On Saturday, January 18, 2014 at 09:06:31 PM, Jagannadha >> Sutradharudu Teki >> wrote: >>> - Used readable names for read/write command macros >>> - Added comments for the same >>> >>> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com> >>> Cc: Marek Vasut <marex@denx.de> >>> Cc: Simon Glass <sjg@chromium.org> >> >> Does this patch have any impact other than making the code harder to >> understand >> ? :-( >> >> What's the rationale for making the code more cryptic ? > > No issues I guess with the readability as each macro we can easily > understand. > like CMD_RD_QUAD --> command_read_quad > CMD_WR_PAGE --> command_write_page_program > > And this will minimize the macro length - good for in coding and more over > description is added in drivers/mtd/spi/sf_internal.h anyway. Again I agree with Marek that readability of code is more important than saving a few characters while coding. This is especially true as editors can support you in coding (Emacs has lots of packages to help here for example). Cheers Detlev
On Mon, Jan 20, 2014 at 4:43 PM, Detlev Zundel <dzu@denx.de> wrote: > Hi Jagan, > >> On Sun, Jan 19, 2014 at 2:06 AM, Marek Vasut <marex@denx.de> wrote: >>> On Saturday, January 18, 2014 at 09:06:31 PM, Jagannadha >>> Sutradharudu Teki >>> wrote: >>>> - Used readable names for read/write command macros >>>> - Added comments for the same >>>> >>>> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com> >>>> Cc: Marek Vasut <marex@denx.de> >>>> Cc: Simon Glass <sjg@chromium.org> >>> >>> Does this patch have any impact other than making the code harder to >>> understand >>> ? :-( >>> >>> What's the rationale for making the code more cryptic ? >> >> No issues I guess with the readability as each macro we can easily >> understand. >> like CMD_RD_QUAD --> command_read_quad >> CMD_WR_PAGE --> command_write_page_program >> >> And this will minimize the macro length - good for in coding and more over >> description is added in drivers/mtd/spi/sf_internal.h anyway. > > Again I agree with Marek that readability of code is more important than > saving a few characters while coding. This is especially true as > editors can support you in coding (Emacs has lots of packages to help > here for example). I don't think nothing much gone the readability with these updated: CMD_READ_ARRAY_FAST has updated CMD_RD_FAST and it seems like easy to understand. and anyway I have added comments for full name as well. Few of the flashes can be call this as array fast read and fewer call this as fast read and few more call this as high frequency read. CMD_RD_FAST will suits all these names. Comments please!
On Monday, January 20, 2014 at 12:46:07 PM, Jagan Teki wrote: > On Mon, Jan 20, 2014 at 4:43 PM, Detlev Zundel <dzu@denx.de> wrote: > > Hi Jagan, > > > >> On Sun, Jan 19, 2014 at 2:06 AM, Marek Vasut <marex@denx.de> wrote: > >>> On Saturday, January 18, 2014 at 09:06:31 PM, Jagannadha > >>> Sutradharudu Teki > >>> > >>> wrote: > >>>> - Used readable names for read/write command macros > >>>> - Added comments for the same > >>>> > >>>> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com> > >>>> Cc: Marek Vasut <marex@denx.de> > >>>> Cc: Simon Glass <sjg@chromium.org> > >>> > >>> Does this patch have any impact other than making the code harder to > >>> understand > >>> ? :-( > >>> > >>> What's the rationale for making the code more cryptic ? > >> > >> No issues I guess with the readability as each macro we can easily > >> understand. > >> like CMD_RD_QUAD --> command_read_quad > >> > >> CMD_WR_PAGE --> command_write_page_program > >> > >> And this will minimize the macro length - good for in coding and more > >> over description is added in drivers/mtd/spi/sf_internal.h anyway. > > > > Again I agree with Marek that readability of code is more important than > > saving a few characters while coding. This is especially true as > > editors can support you in coding (Emacs has lots of packages to help > > here for example). > > I don't think nothing much gone the readability with these updated: > CMD_READ_ARRAY_FAST has updated CMD_RD_FAST and it seems like > easy to understand. and anyway I have added comments for full name as well. CMD_READ_ARRAY_FAST contains all the necessary bits for me to understand what the macro means. CMD_RD_FAST does not. I fail to see the rationale behind changing the names. > Few of the flashes can be call this as array fast read and fewer call > this as fast read > and few more call this as high frequency read. CMD_RD_FAST will suits > all these names. > > Comments please! If you want to align the names with anything, align then with linux's m25p80.c driver . But I see this change as moot and confusing, sorry. Best regards, Marek Vasut
On Mon, Jan 20, 2014 at 6:36 PM, Marek Vasut <marex@denx.de> wrote: > On Monday, January 20, 2014 at 12:46:07 PM, Jagan Teki wrote: >> On Mon, Jan 20, 2014 at 4:43 PM, Detlev Zundel <dzu@denx.de> wrote: >> > Hi Jagan, >> > >> >> On Sun, Jan 19, 2014 at 2:06 AM, Marek Vasut <marex@denx.de> wrote: >> >>> On Saturday, January 18, 2014 at 09:06:31 PM, Jagannadha >> >>> Sutradharudu Teki >> >>> >> >>> wrote: >> >>>> - Used readable names for read/write command macros >> >>>> - Added comments for the same >> >>>> >> >>>> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com> >> >>>> Cc: Marek Vasut <marex@denx.de> >> >>>> Cc: Simon Glass <sjg@chromium.org> >> >>> >> >>> Does this patch have any impact other than making the code harder to >> >>> understand >> >>> ? :-( >> >>> >> >>> What's the rationale for making the code more cryptic ? >> >> >> >> No issues I guess with the readability as each macro we can easily >> >> understand. >> >> like CMD_RD_QUAD --> command_read_quad >> >> >> >> CMD_WR_PAGE --> command_write_page_program >> >> >> >> And this will minimize the macro length - good for in coding and more >> >> over description is added in drivers/mtd/spi/sf_internal.h anyway. >> > >> > Again I agree with Marek that readability of code is more important than >> > saving a few characters while coding. This is especially true as >> > editors can support you in coding (Emacs has lots of packages to help >> > here for example). >> >> I don't think nothing much gone the readability with these updated: >> CMD_READ_ARRAY_FAST has updated CMD_RD_FAST and it seems like >> easy to understand. and anyway I have added comments for full name as well. > > CMD_READ_ARRAY_FAST contains all the necessary bits for me to understand what > the macro means. CMD_RD_FAST does not. I fail to see the rationale behind > changing the names. > >> Few of the flashes can be call this as array fast read and fewer call >> this as fast read >> and few more call this as high frequency read. CMD_RD_FAST will suits >> all these names. >> >> Comments please! > > If you want to align the names with anything, align then with linux's m25p80.c > driver . But I see this change as moot and confusing, sorry. No issues, we can skip these as of now for this release.!
On Mon, Jan 20, 2014 at 6:40 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote: > On Mon, Jan 20, 2014 at 6:36 PM, Marek Vasut <marex@denx.de> wrote: >> On Monday, January 20, 2014 at 12:46:07 PM, Jagan Teki wrote: >>> On Mon, Jan 20, 2014 at 4:43 PM, Detlev Zundel <dzu@denx.de> wrote: >>> > Hi Jagan, >>> > >>> >> On Sun, Jan 19, 2014 at 2:06 AM, Marek Vasut <marex@denx.de> wrote: >>> >>> On Saturday, January 18, 2014 at 09:06:31 PM, Jagannadha >>> >>> Sutradharudu Teki >>> >>> >>> >>> wrote: >>> >>>> - Used readable names for read/write command macros >>> >>>> - Added comments for the same >>> >>>> >>> >>>> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com> >>> >>>> Cc: Marek Vasut <marex@denx.de> >>> >>>> Cc: Simon Glass <sjg@chromium.org> >>> >>> >>> >>> Does this patch have any impact other than making the code harder to >>> >>> understand >>> >>> ? :-( >>> >>> >>> >>> What's the rationale for making the code more cryptic ? >>> >> >>> >> No issues I guess with the readability as each macro we can easily >>> >> understand. >>> >> like CMD_RD_QUAD --> command_read_quad >>> >> >>> >> CMD_WR_PAGE --> command_write_page_program >>> >> >>> >> And this will minimize the macro length - good for in coding and more >>> >> over description is added in drivers/mtd/spi/sf_internal.h anyway. >>> > >>> > Again I agree with Marek that readability of code is more important than >>> > saving a few characters while coding. This is especially true as >>> > editors can support you in coding (Emacs has lots of packages to help >>> > here for example). >>> >>> I don't think nothing much gone the readability with these updated: >>> CMD_READ_ARRAY_FAST has updated CMD_RD_FAST and it seems like >>> easy to understand. and anyway I have added comments for full name as well. >> >> CMD_READ_ARRAY_FAST contains all the necessary bits for me to understand what >> the macro means. CMD_RD_FAST does not. I fail to see the rationale behind >> changing the names. >> >>> Few of the flashes can be call this as array fast read and fewer call >>> this as fast read >>> and few more call this as high frequency read. CMD_RD_FAST will suits >>> all these names. >>> >>> Comments please! >> >> If you want to align the names with anything, align then with linux's m25p80.c >> driver . But I see this change as moot and confusing, sorry. > > No issues, we can skip these as of now for this release.! Just fyi: if we need a reference of m25p80.c the name as OPCODE_NORM_READ which is similar to what i did :)
Hi Jagan, [...] > I don't think nothing much gone the readability with these updated: > CMD_READ_ARRAY_FAST has updated CMD_RD_FAST and it seems like easy to > understand. and anyway I have added comments for full name as well. Comments in a far away place cannot compensate for self-explaining constructs at the location where they are used. Stating that a constants needs comments to explain it is actually a good sign that its name is not chosen carefully enough. Really, naming constants and variables for readable and maintainable code is a much harder problem than it looks (cf. my signature) and unfortunately not easily measurable. But I assure you that good names can make a world of a difference. That's why Marek and me are so passionate about this seemingly "trivial" change. > Few of the flashes can be call this as array fast read and fewer call > this as fast read and few more call this as high frequency > read. CMD_RD_FAST will suits all these names. > > Comments please! When we change code, we don't do this for the sake of changing it, but in order to improve one aspect of it, i.e. the performance, the maintainability or the features. When everything "stays the same", we are even _opposed_ to a change because there is nothing to outweigh the effort to adjusting to the new things. To summarize - we need proof that a change _improves_ something. Showing that we "do not loose something" is clearly not enough. Now in this specific case, we have multiple people voicing the concern that the renaming looses vital information, thus effectively making reading and maintining the code harder. On the other hand even you agree that "something" although "not much" will be gone after the rename. So taking this into account we have only "saving of a few keystorkes" on the positive side and substantial degradation on readability and maintainability on the negative side. Best wishes Detlev
diff --git a/drivers/mtd/spi/ramtron.c b/drivers/mtd/spi/ramtron.c index d50da37..bdf69f7 100644 --- a/drivers/mtd/spi/ramtron.c +++ b/drivers/mtd/spi/ramtron.c @@ -167,7 +167,7 @@ static int ramtron_common(struct spi_flash *flash, return ret; } - if (command == CMD_PAGE_PROGRAM) { + if (command == CMD_WR_PAGE) { /* send WREN */ ret = spi_flash_cmd_write_enable(flash); if (ret < 0) { @@ -177,7 +177,7 @@ static int ramtron_common(struct spi_flash *flash, } /* do the transaction */ - if (command == CMD_PAGE_PROGRAM) + if (command == CMD_WR_PAGE) ret = spi_flash_cmd_write(flash->spi, cmd, cmd_len, buf, len); else ret = spi_flash_cmd_read(flash->spi, cmd, cmd_len, buf, len); @@ -193,15 +193,13 @@ releasebus: static int ramtron_read(struct spi_flash *flash, u32 offset, size_t len, void *buf) { - return ramtron_common(flash, offset, len, buf, - CMD_READ_ARRAY_SLOW); + return ramtron_common(flash, offset, len, buf, CMD_RD_SLOW); } static int ramtron_write(struct spi_flash *flash, u32 offset, size_t len, const void *buf) { - return ramtron_common(flash, offset, len, (void *)buf, - CMD_PAGE_PROGRAM); + return ramtron_common(flash, offset, len, (void *)buf, CMD_WR_PAGE); } static int ramtron_erase(struct spi_flash *flash, u32 offset, size_t len) diff --git a/drivers/mtd/spi/sandbox.c b/drivers/mtd/spi/sandbox.c index a62ef4c..bebfb32 100644 --- a/drivers/mtd/spi/sandbox.c +++ b/drivers/mtd/spi/sandbox.c @@ -219,10 +219,10 @@ static int sandbox_sf_process_cmd(struct sandbox_spi_flash *sbsf, const u8 *rx, sbsf->state = SF_ID; sbsf->cmd = SF_ID; break; - case CMD_READ_ARRAY_FAST: + case CMD_RD_FAST: sbsf->pad_addr_bytes = 1; - case CMD_READ_ARRAY_SLOW: - case CMD_PAGE_PROGRAM: + case CMD_RD_SLOW: + case CMD_WR_PAGE: state_addr: sbsf->state = SF_ADDR; break; @@ -339,11 +339,11 @@ static int sandbox_sf_xfer(void *priv, const u8 *rx, u8 *tx, return 1; } switch (sbsf->cmd) { - case CMD_READ_ARRAY_FAST: - case CMD_READ_ARRAY_SLOW: + case CMD_RD_FAST: + case CMD_RD_SLOW: sbsf->state = SF_READ; break; - case CMD_PAGE_PROGRAM: + case CMD_WR_PAGE: sbsf->state = SF_WRITE; break; default: diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h index 47d5ac2..6b6fa22 100644 --- a/drivers/mtd/spi/sf_internal.h +++ b/drivers/mtd/spi/sf_internal.h @@ -28,22 +28,22 @@ /* Write commands */ #define CMD_WRITE_STATUS 0x01 -#define CMD_PAGE_PROGRAM 0x02 +#define CMD_WR_PAGE 0x02 /* Page program */ #define CMD_WRITE_DISABLE 0x04 #define CMD_READ_STATUS 0x05 -#define CMD_QUAD_PAGE_PROGRAM 0x32 +#define CMD_WR_QUAD 0x32 /* Quad page program */ #define CMD_READ_STATUS1 0x35 #define CMD_WRITE_ENABLE 0x06 #define CMD_READ_CONFIG 0x35 #define CMD_FLAG_STATUS 0x70 /* Read commands */ -#define CMD_READ_ARRAY_SLOW 0x03 -#define CMD_READ_ARRAY_FAST 0x0b -#define CMD_READ_DUAL_OUTPUT_FAST 0x3b -#define CMD_READ_DUAL_IO_FAST 0xbb -#define CMD_READ_QUAD_OUTPUT_FAST 0x6b -#define CMD_READ_QUAD_IO_FAST 0xeb +#define CMD_RD_SLOW 0x03 /* Array slow */ +#define CMD_RD_FAST 0x0b /* Array fast */ +#define CMD_RD_DUAL 0x3b /* Dual output fast */ +#define CMD_RD_DUAL_IO 0xbb /* Dual IO fast */ +#define CMD_RD_QUAD 0x6b /* Quad output fast */ +#define CMD_RD_QUAD_IO 0xeb /* Quad IO fast */ #define CMD_READ_ID 0x9f /* Bank addr access commands */ diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index 79fbad7..7ba0605 100644 --- a/drivers/mtd/spi/sf_probe.c +++ b/drivers/mtd/spi/sf_probe.c @@ -161,30 +161,30 @@ static struct spi_flash *spi_flash_validate_params(struct spi_slave *spi, } /* Compute read command and dummy_byte */ - flash->read_cmd = CMD_READ_ARRAY_FAST; + flash->read_cmd = CMD_RD_FAST; flash->dummy_byte = 1; switch (SPI_RX_MODES & flash->spi->mode_bits) { case SPI_RX_SLOW: if (params->flags & RD_SLOW) { - flash->read_cmd = CMD_READ_ARRAY_FAST; + flash->read_cmd = CMD_RD_SLOW; flash->dummy_byte = 0; } break; case SPI_RX_DUAL: if (params->flags & RD_DUAL) - flash->read_cmd = CMD_READ_DUAL_OUTPUT_FAST; + flash->read_cmd = CMD_RD_DUAL; break; case SPI_RX_DUAL_IO: if (params->flags & RD_DUAL_IO) - flash->read_cmd = CMD_READ_DUAL_OUTPUT_IO; + flash->read_cmd = CMD_RD_DUAL_IO; break; case SPI_RX_QUAD: if (params->flags & RD_QUAD) - flash->read_cmd = CMD_READ_QUAD_OUTPUT_FAST; + flash->read_cmd = CMD_RD_QUAD; break; case SPI_RX_QUAD_IO: if (params->flags & RD_QUAD_IO) { - flash->read_cmd = CMD_READ_QUAD_OUTPUT_FAST_IO; + flash->read_cmd = CMD_RD_QUAD_IO; flash->dummy_byte = 2; } break; @@ -195,12 +195,12 @@ static struct spi_flash *spi_flash_validate_params(struct spi_slave *spi, flash->write_cmd = CMD_QUAD_PAGE_PROGRAM; else /* Go for default supported write cmd */ - flash->write_cmd = CMD_PAGE_PROGRAM; + flash->write_cmd = CMD_WR_PAGE; /* Set the quad enable bit - only for quad commands */ - if ((flash->read_cmd == CMD_READ_QUAD_OUTPUT_FAST) || - (flash->read_cmd == CMD_READ_QUAD_IO_FAST) || - (flash->write_cmd == CMD_QUAD_PAGE_PROGRAM)) { + if ((flash->read_cmd == CMD_RD_QUAD) || + (flash->read_cmd == CMD_RD_QUAD_IO) || + (flash->write_cmd == CMD_WR_QUAD)) { if (spi_flash_set_qeb(flash, idcode[0])) { debug("SF: Fail to set QEB for %02x\n", idcode[0]); return NULL;
- Used readable names for read/write command macros - Added comments for the same Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com> Cc: Marek Vasut <marex@denx.de> Cc: Simon Glass <sjg@chromium.org> --- drivers/mtd/spi/ramtron.c | 10 ++++------ drivers/mtd/spi/sandbox.c | 12 ++++++------ drivers/mtd/spi/sf_internal.h | 16 ++++++++-------- drivers/mtd/spi/sf_probe.c | 20 ++++++++++---------- 4 files changed, 28 insertions(+), 30 deletions(-)