| Message ID | 1382693145-15750-1-git-send-email-sourav.poddar@ti.com |
|---|---|
| State | New, archived |
| Headers | show |
于 2013年10月25日 17:25, Sourav Poddar 写道: > I have rebased this patches on top of l2-mtd + > http://permalink.gmane.org/gmane.linux.drivers.mtd/49129 > Why i can not git-am this patch with the latest l2-mtd? Huang Shijie
On Friday 25 October 2013 03:48 PM, Huang Shijie wrote: > 于 2013年10月25日 17:25, Sourav Poddar 写道: >> I have rebased this patches on top of l2-mtd + >> http://permalink.gmane.org/gmane.linux.drivers.mtd/49129 >> > Why i can not git-am this patch with the latest l2-mtd? > May be some format issue, better to save it from mail and apply. > Huang Shijie >
Dear Sourav Poddar, [...] > +static int macronix_quad_enable(struct m25p *flash) > +{ > + int ret, val; > + u8 cmd[2]; > + cmd[0] = OPCODE_WRSR; > + > + val = read_sr(flash); > + cmd[1] = val | SR_QUAD_EN_MX; > + write_enable(flash); > + > + spi_write(flash->spi, &cmd, 2); > + > + if (wait_till_ready(flash)) > + return 1; > + > + ret = read_sr(flash); Maybe read_sr() and read_cr() shall be fixed to return retval only and the val shall be passed to them as an argument pointer? Aka. ret = read_sr(flash, &val); That way, this dangerous construct below could become: if (!(val & SR_....)) { dev_err(); ret = -EINVAL; } return ret; It will also let us prevent mixing of error codes and register values, which is pretty ugly practice. > + if (!(ret > 0 && (ret & SR_QUAD_EN_MX))) { > + dev_err(&flash->spi->dev, > + "Macronix Quad bit not set"); > + return -EINVAL; > + } > + > + return 0; > +} > + [...] > +static inline int set_quad_mode(struct m25p *flash, u32 jedec_id) > +{ > + int status; > + > + switch (JEDEC_MFR(jedec_id)) { > + case CFI_MFR_MACRONIX: > + status = macronix_quad_enable(flash); > + if (status) { > + dev_err(&flash->spi->dev, > + "Macronix quad not enable"); "Macronix quad-read not enabled" would be more sensible, DTTO below. > + return -EINVAL; > + } > + return status; > + default: > + status = spansion_quad_enable(flash); > + if (status) { > + dev_err(&flash->spi->dev, > + "Spansion quad not enable"); > + return -EINVAL; > + } > + return status; > + } > +} [...] > @@ -774,7 +906,7 @@ static const struct spi_device_id m25p_ids[] = { > { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) }, > { "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) }, > { "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) }, > - { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, 0) }, > + { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, M25P80_QUAD_READ) }, I'm not convinced enabling 4-bit mode should be hard-coded in the MTD driver. There might be a board which uses this chip in 1-bit mode. We have a setup with Spansion chip here which uses 1-bit addressing in U-Boot, but uses 4-bit addressing in Linux. We use a DT property to configure the SPI bus width for that and I think that's a way to go. Note that there also are chips which use 2-bit wide SPI communication. [...]
Dear Marek, On Sunday 27 October 2013 10:15 PM, Marek Vasut wrote: > Dear Sourav Poddar, > > [...] > >> +static int macronix_quad_enable(struct m25p *flash) >> +{ >> + int ret, val; >> + u8 cmd[2]; >> + cmd[0] = OPCODE_WRSR; >> + >> + val = read_sr(flash); >> + cmd[1] = val | SR_QUAD_EN_MX; >> + write_enable(flash); >> + >> + spi_write(flash->spi,&cmd, 2); >> + >> + if (wait_till_ready(flash)) >> + return 1; >> + >> + ret = read_sr(flash); > Maybe read_sr() and read_cr() shall be fixed to return retval only and the val > shall be passed to them as an argument pointer? Aka. ret = read_sr(flash,&val); > > That way, this dangerous construct below could become: > > if (!(val& SR_....)) { > dev_err(); > ret = -EINVAL; > } > > return ret; > > It will also let us prevent mixing of error codes and register values, which is > pretty ugly practice. > ok. Will change. >> + if (!(ret> 0&& (ret& SR_QUAD_EN_MX))) { >> + dev_err(&flash->spi->dev, >> + "Macronix Quad bit not set"); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + > [...] > >> +static inline int set_quad_mode(struct m25p *flash, u32 jedec_id) >> +{ >> + int status; >> + >> + switch (JEDEC_MFR(jedec_id)) { >> + case CFI_MFR_MACRONIX: >> + status = macronix_quad_enable(flash); >> + if (status) { >> + dev_err(&flash->spi->dev, >> + "Macronix quad not enable"); > "Macronix quad-read not enabled" would be more sensible, DTTO below. > Ok. Will change. >> + return -EINVAL; >> + } >> + return status; >> + default: >> + status = spansion_quad_enable(flash); >> + if (status) { >> + dev_err(&flash->spi->dev, >> + "Spansion quad not enable"); >> + return -EINVAL; >> + } >> + return status; >> + } >> +} > [...] > >> @@ -774,7 +906,7 @@ static const struct spi_device_id m25p_ids[] = { >> { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) }, >> { "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) }, >> { "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) }, >> - { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, 0) }, >> + { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, M25P80_QUAD_READ) }, > I'm not convinced enabling 4-bit mode should be hard-coded in the MTD driver. > There might be a board which uses this chip in 1-bit mode. > > We have a setup with Spansion chip here which uses 1-bit addressing in U-Boot, > but uses 4-bit addressing in Linux. We use a DT property to configure the SPI > bus width for that and I think that's a way to go. Note that there also are > chips which use 2-bit wide SPI communication. > Yes, but if you trace down the patch below, you will see its not hard coded. Enabling quad read mode depends on the following: 1. Whether flash chip supports it. This information comes from the flash which we are setting above. 2. Whether SPI controller supports it. This information is the check down below in the patch(spi->mode & SPI_RX_QUAD). spi->mode is set to "SINGLE/DUAL/QUAD" in the SPI framework based on the dt property "spi-tx/rx-bus-width". So, unless you set spi->mode to SPI_RX_QUAD by setting "spi-rx-bus-width" to 4 in dt, default will be 1 bit mode only. > [...] >
Dear Sourav Poddar, > Dear Marek, > > On Sunday 27 October 2013 10:15 PM, Marek Vasut wrote: > > Dear Sourav Poddar, > > > > [...] [...] > >> @@ -774,7 +906,7 @@ static const struct spi_device_id m25p_ids[] = { > >> > >> { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) }, > >> { "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) }, > >> { "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) }, > >> > >> - { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, 0) }, > >> + { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, M25P80_QUAD_READ) > >> }, > > > > I'm not convinced enabling 4-bit mode should be hard-coded in the MTD > > driver. There might be a board which uses this chip in 1-bit mode. > > > > We have a setup with Spansion chip here which uses 1-bit addressing in > > U-Boot, but uses 4-bit addressing in Linux. We use a DT property to > > configure the SPI bus width for that and I think that's a way to go. > > Note that there also are chips which use 2-bit wide SPI communication. > > Yes, but if you trace down the patch below, you will see its not hard > coded. Enabling quad read mode depends on the following: > > 1. Whether flash chip supports it. > This information comes from the flash which we are setting above. > > 2. Whether SPI controller supports it. > This information is the check down below in the > patch(spi->mode & SPI_RX_QUAD). > spi->mode is set to "SINGLE/DUAL/QUAD" in the SPI framework > based on the > dt property "spi-tx/rx-bus-width". > So, unless you set spi->mode to SPI_RX_QUAD by setting > "spi-rx-bus-width" to 4 in dt, default > will be 1 bit mode only. So this M25P80_QUAD_READ flag tells us "this chip supports quad read", not "force enable quad read" . All right, thanks for clearing this up. I hope it's documented somewhere elsewhere than in this email ;-) Best regards, Marek Vasut
Dear Marek, On Monday 28 October 2013 12:00 AM, Marek Vasut wrote: > Dear Sourav Poddar, > >> Dear Marek, >> >> On Sunday 27 October 2013 10:15 PM, Marek Vasut wrote: >>> Dear Sourav Poddar, >>> >>> [...] > [...] > >>>> @@ -774,7 +906,7 @@ static const struct spi_device_id m25p_ids[] = { >>>> >>>> { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) }, >>>> { "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) }, >>>> { "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) }, >>>> >>>> - { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, 0) }, >>>> + { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, M25P80_QUAD_READ) >>>> }, >>> I'm not convinced enabling 4-bit mode should be hard-coded in the MTD >>> driver. There might be a board which uses this chip in 1-bit mode. >>> >>> We have a setup with Spansion chip here which uses 1-bit addressing in >>> U-Boot, but uses 4-bit addressing in Linux. We use a DT property to >>> configure the SPI bus width for that and I think that's a way to go. >>> Note that there also are chips which use 2-bit wide SPI communication. >> Yes, but if you trace down the patch below, you will see its not hard >> coded. Enabling quad read mode depends on the following: >> >> 1. Whether flash chip supports it. >> This information comes from the flash which we are setting above. >> >> 2. Whether SPI controller supports it. >> This information is the check down below in the >> patch(spi->mode& SPI_RX_QUAD). >> spi->mode is set to "SINGLE/DUAL/QUAD" in the SPI framework >> based on the >> dt property "spi-tx/rx-bus-width". >> So, unless you set spi->mode to SPI_RX_QUAD by setting >> "spi-rx-bus-width" to 4 in dt, default >> will be 1 bit mode only. > So this M25P80_QUAD_READ flag tells us "this chip supports quad read", not > "force enable quad read" . Yes, correct. > All right, thanks for clearing this up. I hope it's > documented somewhere elsewhere than in this email ;-) Its added as a comment under struct flash_info where this flag is defined. > Best regards, > Marek Vasut
Dear Sourav Poddar, > Dear Marek, > > On Monday 28 October 2013 12:00 AM, Marek Vasut wrote: > > Dear Sourav Poddar, > > > >> Dear Marek, > >> > >> On Sunday 27 October 2013 10:15 PM, Marek Vasut wrote: > >>> Dear Sourav Poddar, > >>> > >>> [...] > > > > [...] > > > >>>> @@ -774,7 +906,7 @@ static const struct spi_device_id m25p_ids[] = { > >>>> > >>>> { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) }, > >>>> { "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) }, > >>>> { "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) }, > >>>> > >>>> - { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, 0) }, > >>>> + { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, > >>>> M25P80_QUAD_READ) }, > >>> > >>> I'm not convinced enabling 4-bit mode should be hard-coded in the MTD > >>> driver. There might be a board which uses this chip in 1-bit mode. > >>> > >>> We have a setup with Spansion chip here which uses 1-bit addressing in > >>> U-Boot, but uses 4-bit addressing in Linux. We use a DT property to > >>> configure the SPI bus width for that and I think that's a way to go. > >>> Note that there also are chips which use 2-bit wide SPI communication. > >> > >> Yes, but if you trace down the patch below, you will see its not hard > >> coded. Enabling quad read mode depends on the following: > >> > >> 1. Whether flash chip supports it. > >> > >> This information comes from the flash which we are setting > >> above. > >> > >> 2. Whether SPI controller supports it. > >> > >> This information is the check down below in the > >> > >> patch(spi->mode& SPI_RX_QUAD). > >> > >> spi->mode is set to "SINGLE/DUAL/QUAD" in the SPI > >> framework > >> > >> based on the > >> > >> dt property "spi-tx/rx-bus-width". > >> > >> So, unless you set spi->mode to SPI_RX_QUAD by setting > >> "spi-rx-bus-width" to 4 in dt, default > >> will be 1 bit mode only. > > > > So this M25P80_QUAD_READ flag tells us "this chip supports quad read", > > not "force enable quad read" . > > Yes, correct. > > > All right, thanks for clearing this up. I hope it's > > documented somewhere elsewhere than in this email ;-) > > Its added as a comment under struct flash_info where > this flag is defined. Thank you! Best regards, Marek Vasut
Hi, On Sunday 27 October 2013 10:15 PM, Marek Vasut wrote: > Dear Sourav Poddar, > > [...] > >> +static int macronix_quad_enable(struct m25p *flash) >> +{ >> + int ret, val; >> + u8 cmd[2]; >> + cmd[0] = OPCODE_WRSR; >> + >> + val = read_sr(flash); >> + cmd[1] = val | SR_QUAD_EN_MX; >> + write_enable(flash); >> + >> + spi_write(flash->spi,&cmd, 2); >> + >> + if (wait_till_ready(flash)) >> + return 1; >> + >> + ret = read_sr(flash); > Maybe read_sr() and read_cr() shall be fixed to return retval only and the val > shall be passed to them as an argument pointer? Aka. ret = read_sr(flash,&val); > > That way, this dangerous construct below could become: > > if (!(val& SR_....)) { > dev_err(); > ret = -EINVAL; > } > > return ret; I was trying to work on it and realise, we dont need to pass val directly. We can continue returning the val and can still cleanup the below code as u suggetsed above. if (!(ret & SR_....)) { dev_err(); ret = -EINVAL; } > It will also let us prevent mixing of error codes and register values, which is > pretty ugly practice. > >> + if (!(ret> 0&& (ret& SR_QUAD_EN_MX))) { >> + dev_err(&flash->spi->dev, >> + "Macronix Quad bit not set"); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + > [...] > >> +static inline int set_quad_mode(struct m25p *flash, u32 jedec_id) >> +{ >> + int status; >> + >> + switch (JEDEC_MFR(jedec_id)) { >> + case CFI_MFR_MACRONIX: >> + status = macronix_quad_enable(flash); >> + if (status) { >> + dev_err(&flash->spi->dev, >> + "Macronix quad not enable"); > "Macronix quad-read not enabled" would be more sensible, DTTO below. > >> + return -EINVAL; >> + } >> + return status; >> + default: >> + status = spansion_quad_enable(flash); >> + if (status) { >> + dev_err(&flash->spi->dev, >> + "Spansion quad not enable"); >> + return -EINVAL; >> + } >> + return status; >> + } >> +} > [...] > >> @@ -774,7 +906,7 @@ static const struct spi_device_id m25p_ids[] = { >> { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) }, >> { "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) }, >> { "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) }, >> - { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, 0) }, >> + { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, M25P80_QUAD_READ) }, > I'm not convinced enabling 4-bit mode should be hard-coded in the MTD driver. > There might be a board which uses this chip in 1-bit mode. > > We have a setup with Spansion chip here which uses 1-bit addressing in U-Boot, > but uses 4-bit addressing in Linux. We use a DT property to configure the SPI > bus width for that and I think that's a way to go. Note that there also are > chips which use 2-bit wide SPI communication. > > [...] >
Dear Sourav Poddar, > Hi, > > On Sunday 27 October 2013 10:15 PM, Marek Vasut wrote: > > Dear Sourav Poddar, > > > > [...] > > > >> +static int macronix_quad_enable(struct m25p *flash) > >> +{ > >> + int ret, val; > >> + u8 cmd[2]; > >> + cmd[0] = OPCODE_WRSR; > >> + > >> + val = read_sr(flash); > >> + cmd[1] = val | SR_QUAD_EN_MX; > >> + write_enable(flash); > >> + > >> + spi_write(flash->spi,&cmd, 2); > >> + > >> + if (wait_till_ready(flash)) > >> + return 1; > >> + > >> + ret = read_sr(flash); > > > > Maybe read_sr() and read_cr() shall be fixed to return retval only and > > the val shall be passed to them as an argument pointer? Aka. ret = > > read_sr(flash,&val); > > > > That way, this dangerous construct below could become: > > > > if (!(val& SR_....)) { > > > > dev_err(); > > ret = -EINVAL; > > > > } > > > > return ret; > > I was trying to work on it and realise, we dont need to pass val directly. > We can continue returning the val and can still cleanup the below code as > u suggetsed above. > if (!(ret & SR_....)) { > dev_err(); > ret = -EINVAL; > } Uh oh, no. This doesn't seem right. I'd like to be able to clearly check if the function failed to read the register altogether OR if not, check the returned value of the register. Mixing these two together won't do us good. But maybe I just fail to understand your proposal, if so, then I appologize. [...] Best regards, Marek Vasut
Dear Marek Vasut, On Tuesday 29 October 2013 07:31 PM, Marek Vasut wrote: > Dear Sourav Poddar, > >> Hi, >> >> On Sunday 27 October 2013 10:15 PM, Marek Vasut wrote: >>> Dear Sourav Poddar, >>> >>> [...] >>> >>>> +static int macronix_quad_enable(struct m25p *flash) >>>> +{ >>>> + int ret, val; >>>> + u8 cmd[2]; >>>> + cmd[0] = OPCODE_WRSR; >>>> + >>>> + val = read_sr(flash); >>>> + cmd[1] = val | SR_QUAD_EN_MX; >>>> + write_enable(flash); >>>> + >>>> + spi_write(flash->spi,&cmd, 2); >>>> + >>>> + if (wait_till_ready(flash)) >>>> + return 1; >>>> + >>>> + ret = read_sr(flash); >>> Maybe read_sr() and read_cr() shall be fixed to return retval only and >>> the val shall be passed to them as an argument pointer? Aka. ret = >>> read_sr(flash,&val); >>> >>> That way, this dangerous construct below could become: >>> >>> if (!(val& SR_....)) { >>> >>> dev_err(); >>> ret = -EINVAL; >>> >>> } >>> >>> return ret; >> I was trying to work on it and realise, we dont need to pass val directly. >> We can continue returning the val and can still cleanup the below code as >> u suggetsed above. >> if (!(ret& SR_....)) { >> dev_err(); >> ret = -EINVAL; >> } > Uh oh, no. This doesn't seem right. I'd like to be able to clearly check if the > function failed to read the register altogether OR if not, check the returned > value of the register. Mixing these two together won't do us good. But maybe I > just fail to understand your proposal, if so, then I appologize. > Yes, what I am trying to propose is to eliminate the return error check. The check whether register read has happened correctly is embedded in read_sr/read_cr function itself. if (retval < 0) { dev_err(&flash->spi->dev, "error %d reading SR\n", (int) retval); return retval; } Same goes for read_cr. So, if the above condition is not hit, we simply return the read value and check it with the respective bits. > [...] > > Best regards, > Marek Vasut
Dear Sourav Poddar, > Dear Marek Vasut, > > On Tuesday 29 October 2013 07:31 PM, Marek Vasut wrote: > > Dear Sourav Poddar, > > > >> Hi, > >> > >> On Sunday 27 October 2013 10:15 PM, Marek Vasut wrote: > >>> Dear Sourav Poddar, > >>> > >>> [...] > >>> > >>>> +static int macronix_quad_enable(struct m25p *flash) > >>>> +{ > >>>> + int ret, val; > >>>> + u8 cmd[2]; > >>>> + cmd[0] = OPCODE_WRSR; > >>>> + > >>>> + val = read_sr(flash); > >>>> + cmd[1] = val | SR_QUAD_EN_MX; > >>>> + write_enable(flash); > >>>> + > >>>> + spi_write(flash->spi,&cmd, 2); > >>>> + > >>>> + if (wait_till_ready(flash)) > >>>> + return 1; > >>>> + > >>>> + ret = read_sr(flash); > >>> > >>> Maybe read_sr() and read_cr() shall be fixed to return retval only and > >>> the val shall be passed to them as an argument pointer? Aka. ret = > >>> read_sr(flash,&val); > >>> > >>> That way, this dangerous construct below could become: > >>> > >>> if (!(val& SR_....)) { > >>> > >>> dev_err(); > >>> ret = -EINVAL; > >>> > >>> } > >>> > >>> return ret; > >> > >> I was trying to work on it and realise, we dont need to pass val > >> directly. We can continue returning the val and can still cleanup the > >> below code as u suggetsed above. > >> if (!(ret& SR_....)) { > >> > >> dev_err(); > >> ret = -EINVAL; > >> > >> } > > > > Uh oh, no. This doesn't seem right. I'd like to be able to clearly check > > if the function failed to read the register altogether OR if not, check > > the returned value of the register. Mixing these two together won't do > > us good. But maybe I just fail to understand your proposal, if so, then > > I appologize. > > Yes, what I am trying to propose is to eliminate the return error check. But we want to be able to check if there is a failure :) > The check whether register read has happened correctly is embedded in > read_sr/read_cr function itself. > if (retval < 0) { > dev_err(&flash->spi->dev, "error %d reading SR\n", > (int) retval); > return retval; > } > Same goes for read_cr. > So, if the above condition is not hit, we simply return the read value and > check it with the respective bits. Look here: 107 static int read_sr(struct m25p *flash) 108 { 109 ssize_t retval; 110 u8 code = OPCODE_RDSR; 111 u8 val; 112 113 retval = spi_write_then_read(flash->spi, &code, 1, &val, 1); 114 115 if (retval < 0) { 116 dev_err(&flash->spi->dev, "error %d reading SR\n", 117 (int) retval); 118 return retval; here you return error value IFF spi_write_then_read() fails for some reason. 119 } 120 121 return val; here you return actual value of the register. 122 } This is how I'd change the function to make it less error-prone: *107 static int read_sr(struct m25p *flash, u8 *rval) 108 { 109 ssize_t retval; 110 u8 code = OPCODE_RDSR; 111 u8 val; 112 113 retval = spi_write_then_read(flash->spi, &code, 1, &val, 1); 114 115 if (retval < 0) { 116 dev_err(&flash->spi->dev, "error %d reading SR\n", 117 (int) retval); 118 return retval; 119 } *120 *rval = val; *121 return 0; 122 } This way, you can check if the SPI read failed and if so, handle it in some way. The return value would only be valid if this function returned 0. Best regards, Marek Vasut
On Tuesday 29 October 2013 08:57 PM, Marek Vasut wrote: > Dear Sourav Poddar, > >> Dear Marek Vasut, >> >> On Tuesday 29 October 2013 07:31 PM, Marek Vasut wrote: >>> Dear Sourav Poddar, >>> >>>> Hi, >>>> >>>> On Sunday 27 October 2013 10:15 PM, Marek Vasut wrote: >>>>> Dear Sourav Poddar, >>>>> >>>>> [...] >>>>> >>>>>> +static int macronix_quad_enable(struct m25p *flash) >>>>>> +{ >>>>>> + int ret, val; >>>>>> + u8 cmd[2]; >>>>>> + cmd[0] = OPCODE_WRSR; >>>>>> + >>>>>> + val = read_sr(flash); >>>>>> + cmd[1] = val | SR_QUAD_EN_MX; >>>>>> + write_enable(flash); >>>>>> + >>>>>> + spi_write(flash->spi,&cmd, 2); >>>>>> + >>>>>> + if (wait_till_ready(flash)) >>>>>> + return 1; >>>>>> + >>>>>> + ret = read_sr(flash); >>>>> Maybe read_sr() and read_cr() shall be fixed to return retval only and >>>>> the val shall be passed to them as an argument pointer? Aka. ret = >>>>> read_sr(flash,&val); >>>>> >>>>> That way, this dangerous construct below could become: >>>>> >>>>> if (!(val& SR_....)) { >>>>> >>>>> dev_err(); >>>>> ret = -EINVAL; >>>>> >>>>> } >>>>> >>>>> return ret; >>>> I was trying to work on it and realise, we dont need to pass val >>>> directly. We can continue returning the val and can still cleanup the >>>> below code as u suggetsed above. >>>> if (!(ret& SR_....)) { >>>> >>>> dev_err(); >>>> ret = -EINVAL; >>>> >>>> } >>> Uh oh, no. This doesn't seem right. I'd like to be able to clearly check >>> if the function failed to read the register altogether OR if not, check >>> the returned value of the register. Mixing these two together won't do >>> us good. But maybe I just fail to understand your proposal, if so, then >>> I appologize. >> Yes, what I am trying to propose is to eliminate the return error check. > But we want to be able to check if there is a failure :) > >> The check whether register read has happened correctly is embedded in >> read_sr/read_cr function itself. >> if (retval< 0) { >> dev_err(&flash->spi->dev, "error %d reading SR\n", >> (int) retval); >> return retval; >> } >> Same goes for read_cr. >> So, if the above condition is not hit, we simply return the read value and >> check it with the respective bits. > Look here: > > 107 static int read_sr(struct m25p *flash) > 108 { > 109 ssize_t retval; > 110 u8 code = OPCODE_RDSR; > 111 u8 val; > 112 > 113 retval = spi_write_then_read(flash->spi,&code, 1,&val, 1); > 114 > 115 if (retval< 0) { > 116 dev_err(&flash->spi->dev, "error %d reading SR\n", > 117 (int) retval); > 118 return retval; > > here you return error value IFF spi_write_then_read() fails for some reason. > > 119 } > 120 > 121 return val; > > here you return actual value of the register. > > 122 } > > This is how I'd change the function to make it less error-prone: > > *107 static int read_sr(struct m25p *flash, u8 *rval) > 108 { > 109 ssize_t retval; > 110 u8 code = OPCODE_RDSR; > 111 u8 val; > 112 > 113 retval = spi_write_then_read(flash->spi,&code, 1,&val, 1); > 114 > 115 if (retval< 0) { > 116 dev_err(&flash->spi->dev, "error %d reading SR\n", > 117 (int) retval); > 118 return retval; > 119 } > *120 *rval = val; > *121 return 0; > 122 } > > This way, you can check if the SPI read failed and if so, handle it in some way. > The return value would only be valid if this function returned 0. > I got this, but do you think its necessary to have two checks for verifying whether read passed. ? If I go by your code above, after returning from above, check for return value for successful read and then check the respective bit set(SR_*). ? > Best regards, > Marek Vasut
Dear Sourav Poddar, > On Tuesday 29 October 2013 08:57 PM, Marek Vasut wrote: > > Dear Sourav Poddar, > > > >> Dear Marek Vasut, > >> > >> On Tuesday 29 October 2013 07:31 PM, Marek Vasut wrote: > >>> Dear Sourav Poddar, > >>> > >>>> Hi, > >>>> > >>>> On Sunday 27 October 2013 10:15 PM, Marek Vasut wrote: > >>>>> Dear Sourav Poddar, > >>>>> > >>>>> [...] > >>>>> > >>>>>> +static int macronix_quad_enable(struct m25p *flash) > >>>>>> +{ > >>>>>> + int ret, val; > >>>>>> + u8 cmd[2]; > >>>>>> + cmd[0] = OPCODE_WRSR; > >>>>>> + > >>>>>> + val = read_sr(flash); > >>>>>> + cmd[1] = val | SR_QUAD_EN_MX; > >>>>>> + write_enable(flash); > >>>>>> + > >>>>>> + spi_write(flash->spi,&cmd, 2); > >>>>>> + > >>>>>> + if (wait_till_ready(flash)) > >>>>>> + return 1; > >>>>>> + > >>>>>> + ret = read_sr(flash); > >>>>> > >>>>> Maybe read_sr() and read_cr() shall be fixed to return retval only > >>>>> and the val shall be passed to them as an argument pointer? Aka. ret > >>>>> = read_sr(flash,&val); > >>>>> > >>>>> That way, this dangerous construct below could become: > >>>>> > >>>>> if (!(val& SR_....)) { > >>>>> > >>>>> dev_err(); > >>>>> ret = -EINVAL; > >>>>> > >>>>> } > >>>>> > >>>>> return ret; > >>>> > >>>> I was trying to work on it and realise, we dont need to pass val > >>>> directly. We can continue returning the val and can still cleanup the > >>>> below code as u suggetsed above. > >>>> if (!(ret& SR_....)) { > >>>> > >>>> dev_err(); > >>>> ret = -EINVAL; > >>>> > >>>> } > >>> > >>> Uh oh, no. This doesn't seem right. I'd like to be able to clearly > >>> check if the function failed to read the register altogether OR if > >>> not, check the returned value of the register. Mixing these two > >>> together won't do us good. But maybe I just fail to understand your > >>> proposal, if so, then I appologize. > >> > >> Yes, what I am trying to propose is to eliminate the return error check. > > > > But we want to be able to check if there is a failure :) > > > >> The check whether register read has happened correctly is embedded in > >> read_sr/read_cr function itself. > >> > >> if (retval< 0) { > >> > >> dev_err(&flash->spi->dev, "error %d reading SR\n", > >> > >> (int) retval); > >> > >> return retval; > >> > >> } > >> > >> Same goes for read_cr. > >> So, if the above condition is not hit, we simply return the read value > >> and check it with the respective bits. > > > > Look here: > > 107 static int read_sr(struct m25p *flash) > > 108 { > > 109 ssize_t retval; > > 110 u8 code = OPCODE_RDSR; > > 111 u8 val; > > 112 > > 113 retval = spi_write_then_read(flash->spi,&code, 1,&val, 1); > > 114 > > 115 if (retval< 0) { > > 116 dev_err(&flash->spi->dev, "error %d reading SR\n", > > 117 (int) retval); > > 118 return retval; > > > > here you return error value IFF spi_write_then_read() fails for some > > reason. > > > > 119 } > > 120 > > 121 return val; > > > > here you return actual value of the register. > > > > 122 } > > > > This is how I'd change the function to make it less error-prone: > > > > *107 static int read_sr(struct m25p *flash, u8 *rval) > > > > 108 { > > 109 ssize_t retval; > > 110 u8 code = OPCODE_RDSR; > > 111 u8 val; > > 112 > > 113 retval = spi_write_then_read(flash->spi,&code, 1,&val, 1); > > 114 > > 115 if (retval< 0) { > > 116 dev_err(&flash->spi->dev, "error %d reading SR\n", > > 117 (int) retval); > > 118 return retval; > > 119 } > > > > *120 *rval = val; > > *121 return 0; > > > > 122 } > > > > This way, you can check if the SPI read failed and if so, handle it in > > some way. The return value would only be valid if this function returned > > 0. > > I got this, but do you think its necessary to have two checks for verifying > whether read passed. ? Yes of course it is necessary, how else would you be able to tell if the value is valid ? Sure, you can depend on negative integer here and on the fact that the u8 will never be 32-bits wide (to produce a negative integer when the return value is valid), but personally I think this is error-prone as hell. > If I go by your code above, after returning from above, > check for return value for successful read > and then check the respective bit set(SR_*). ? Yes, you will be checking the bit in SR only if you are sure the value is valid.
On Tuesday 29 October 2013 10:38 PM, Marek Vasut wrote: > Dear Sourav Poddar, > >> On Tuesday 29 October 2013 08:57 PM, Marek Vasut wrote: >>> Dear Sourav Poddar, >>> >>>> Dear Marek Vasut, >>>> >>>> On Tuesday 29 October 2013 07:31 PM, Marek Vasut wrote: >>>>> Dear Sourav Poddar, >>>>> >>>>>> Hi, >>>>>> >>>>>> On Sunday 27 October 2013 10:15 PM, Marek Vasut wrote: >>>>>>> Dear Sourav Poddar, >>>>>>> >>>>>>> [...] >>>>>>> >>>>>>>> +static int macronix_quad_enable(struct m25p *flash) >>>>>>>> +{ >>>>>>>> + int ret, val; >>>>>>>> + u8 cmd[2]; >>>>>>>> + cmd[0] = OPCODE_WRSR; >>>>>>>> + >>>>>>>> + val = read_sr(flash); >>>>>>>> + cmd[1] = val | SR_QUAD_EN_MX; >>>>>>>> + write_enable(flash); >>>>>>>> + >>>>>>>> + spi_write(flash->spi,&cmd, 2); >>>>>>>> + >>>>>>>> + if (wait_till_ready(flash)) >>>>>>>> + return 1; >>>>>>>> + >>>>>>>> + ret = read_sr(flash); >>>>>>> Maybe read_sr() and read_cr() shall be fixed to return retval only >>>>>>> and the val shall be passed to them as an argument pointer? Aka. ret >>>>>>> = read_sr(flash,&val); >>>>>>> >>>>>>> That way, this dangerous construct below could become: >>>>>>> >>>>>>> if (!(val& SR_....)) { >>>>>>> >>>>>>> dev_err(); >>>>>>> ret = -EINVAL; >>>>>>> >>>>>>> } >>>>>>> >>>>>>> return ret; >>>>>> I was trying to work on it and realise, we dont need to pass val >>>>>> directly. We can continue returning the val and can still cleanup the >>>>>> below code as u suggetsed above. >>>>>> if (!(ret& SR_....)) { >>>>>> >>>>>> dev_err(); >>>>>> ret = -EINVAL; >>>>>> >>>>>> } >>>>> Uh oh, no. This doesn't seem right. I'd like to be able to clearly >>>>> check if the function failed to read the register altogether OR if >>>>> not, check the returned value of the register. Mixing these two >>>>> together won't do us good. But maybe I just fail to understand your >>>>> proposal, if so, then I appologize. >>>> Yes, what I am trying to propose is to eliminate the return error check. >>> But we want to be able to check if there is a failure :) >>> >>>> The check whether register read has happened correctly is embedded in >>>> read_sr/read_cr function itself. >>>> >>>> if (retval< 0) { >>>> >>>> dev_err(&flash->spi->dev, "error %d reading SR\n", >>>> >>>> (int) retval); >>>> >>>> return retval; >>>> >>>> } >>>> >>>> Same goes for read_cr. >>>> So, if the above condition is not hit, we simply return the read value >>>> and check it with the respective bits. >>> Look here: >>> 107 static int read_sr(struct m25p *flash) >>> 108 { >>> 109 ssize_t retval; >>> 110 u8 code = OPCODE_RDSR; >>> 111 u8 val; >>> 112 >>> 113 retval = spi_write_then_read(flash->spi,&code, 1,&val, 1); >>> 114 >>> 115 if (retval< 0) { >>> 116 dev_err(&flash->spi->dev, "error %d reading SR\n", >>> 117 (int) retval); >>> 118 return retval; >>> >>> here you return error value IFF spi_write_then_read() fails for some >>> reason. >>> >>> 119 } >>> 120 >>> 121 return val; >>> >>> here you return actual value of the register. >>> >>> 122 } >>> >>> This is how I'd change the function to make it less error-prone: >>> >>> *107 static int read_sr(struct m25p *flash, u8 *rval) >>> >>> 108 { >>> 109 ssize_t retval; >>> 110 u8 code = OPCODE_RDSR; >>> 111 u8 val; >>> 112 >>> 113 retval = spi_write_then_read(flash->spi,&code, 1,&val, 1); >>> 114 >>> 115 if (retval< 0) { >>> 116 dev_err(&flash->spi->dev, "error %d reading SR\n", >>> 117 (int) retval); >>> 118 return retval; >>> 119 } >>> >>> *120 *rval = val; >>> *121 return 0; >>> >>> 122 } >>> >>> This way, you can check if the SPI read failed and if so, handle it in >>> some way. The return value would only be valid if this function returned >>> 0. >> I got this, but do you think its necessary to have two checks for verifying >> whether read passed. ? > Yes of course it is necessary, how else would you be able to tell if the value > is valid ? Sure, you can depend on negative integer here and on the fact that > the u8 will never be 32-bits wide (to produce a negative integer when the return > value is valid), but personally I think this is error-prone as hell. > >> If I go by your code above, after returning from above, >> check for return value for successful read >> and then check the respective bit set(SR_*). ? > Yes, you will be checking the bit in SR only if you are sure the value is valid. hmm..alrite I will do the cleanup and send v2.
Dear Sourav Poddar, [...] > >>> This way, you can check if the SPI read failed and if so, handle it in > >>> some way. The return value would only be valid if this function > >>> returned 0. > >> > >> I got this, but do you think its necessary to have two checks for > >> verifying whether read passed. ? > > > > Yes of course it is necessary, how else would you be able to tell if the > > value is valid ? Sure, you can depend on negative integer here and on > > the fact that the u8 will never be 32-bits wide (to produce a negative > > integer when the return value is valid), but personally I think this is > > error-prone as hell. > > > >> If I go by your code above, after returning from above, > >> check for return value for successful read > >> and then check the respective bit set(SR_*). ? > > > > Yes, you will be checking the bit in SR only if you are sure the value is > > valid. > > hmm..alrite I will do the cleanup and send v2. Brian, does this make sense? I'd hate to misguide someone. Best regards, Marek Vasut
On Tuesday 29 October 2013 10:42 PM, Sourav Poddar wrote: > On Tuesday 29 October 2013 10:38 PM, Marek Vasut wrote: >> Dear Sourav Poddar, >> >>> On Tuesday 29 October 2013 08:57 PM, Marek Vasut wrote: >>>> Dear Sourav Poddar, >>>> >>>>> Dear Marek Vasut, >>>>> >>>>> On Tuesday 29 October 2013 07:31 PM, Marek Vasut wrote: >>>>>> Dear Sourav Poddar, >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> On Sunday 27 October 2013 10:15 PM, Marek Vasut wrote: >>>>>>>> Dear Sourav Poddar, >>>>>>>> >>>>>>>> [...] >>>>>>>> >>>>>>>>> +static int macronix_quad_enable(struct m25p *flash) >>>>>>>>> +{ >>>>>>>>> + int ret, val; >>>>>>>>> + u8 cmd[2]; >>>>>>>>> + cmd[0] = OPCODE_WRSR; >>>>>>>>> + >>>>>>>>> + val = read_sr(flash); >>>>>>>>> + cmd[1] = val | SR_QUAD_EN_MX; >>>>>>>>> + write_enable(flash); >>>>>>>>> + >>>>>>>>> + spi_write(flash->spi,&cmd, 2); >>>>>>>>> + >>>>>>>>> + if (wait_till_ready(flash)) >>>>>>>>> + return 1; >>>>>>>>> + >>>>>>>>> + ret = read_sr(flash); >>>>>>>> Maybe read_sr() and read_cr() shall be fixed to return retval only >>>>>>>> and the val shall be passed to them as an argument pointer? >>>>>>>> Aka. ret >>>>>>>> = read_sr(flash,&val); >>>>>>>> >>>>>>>> That way, this dangerous construct below could become: >>>>>>>> >>>>>>>> if (!(val& SR_....)) { >>>>>>>> >>>>>>>> dev_err(); >>>>>>>> ret = -EINVAL; >>>>>>>> >>>>>>>> } >>>>>>>> >>>>>>>> return ret; >>>>>>> I was trying to work on it and realise, we dont need to pass val >>>>>>> directly. We can continue returning the val and can still >>>>>>> cleanup the >>>>>>> below code as u suggetsed above. >>>>>>> if (!(ret& SR_....)) { >>>>>>> >>>>>>> dev_err(); >>>>>>> ret = -EINVAL; >>>>>>> >>>>>>> } >>>>>> Uh oh, no. This doesn't seem right. I'd like to be able to clearly >>>>>> check if the function failed to read the register altogether OR if >>>>>> not, check the returned value of the register. Mixing these two >>>>>> together won't do us good. But maybe I just fail to understand your >>>>>> proposal, if so, then I appologize. >>>>> Yes, what I am trying to propose is to eliminate the return error >>>>> check. >>>> But we want to be able to check if there is a failure :) >>>> >>>>> The check whether register read has happened correctly is embedded in >>>>> read_sr/read_cr function itself. >>>>> >>>>> if (retval< 0) { >>>>> >>>>> dev_err(&flash->spi->dev, "error %d reading SR\n", >>>>> >>>>> (int) retval); >>>>> >>>>> return retval; >>>>> >>>>> } >>>>> >>>>> Same goes for read_cr. >>>>> So, if the above condition is not hit, we simply return the read >>>>> value >>>>> and check it with the respective bits. >>>> Look here: >>>> 107 static int read_sr(struct m25p *flash) >>>> 108 { >>>> 109 ssize_t retval; >>>> 110 u8 code = OPCODE_RDSR; >>>> 111 u8 val; >>>> 112 >>>> 113 retval = spi_write_then_read(flash->spi,&code, >>>> 1,&val, 1); >>>> 114 >>>> 115 if (retval< 0) { >>>> 116 dev_err(&flash->spi->dev, "error %d reading >>>> SR\n", >>>> 117 (int) retval); >>>> 118 return retval; >>>> >>>> here you return error value IFF spi_write_then_read() fails for some >>>> reason. >>>> >>>> 119 } >>>> 120 >>>> 121 return val; >>>> >>>> here you return actual value of the register. >>>> >>>> 122 } >>>> >>>> This is how I'd change the function to make it less error-prone: >>>> >>>> *107 static int read_sr(struct m25p *flash, u8 *rval) >>>> >>>> 108 { >>>> 109 ssize_t retval; >>>> 110 u8 code = OPCODE_RDSR; >>>> 111 u8 val; >>>> 112 >>>> 113 retval = spi_write_then_read(flash->spi,&code, >>>> 1,&val, 1); >>>> 114 >>>> 115 if (retval< 0) { >>>> 116 dev_err(&flash->spi->dev, "error %d reading >>>> SR\n", >>>> 117 (int) retval); >>>> 118 return retval; >>>> 119 } >>>> >>>> *120 *rval = val; >>>> *121 return 0; >>>> >>>> 122 } >>>> >>>> This way, you can check if the SPI read failed and if so, handle it in >>>> some way. The return value would only be valid if this function >>>> returned >>>> 0. >>> I got this, but do you think its necessary to have two checks for >>> verifying >>> whether read passed. ? >> Yes of course it is necessary, how else would you be able to tell if >> the value >> is valid ? Sure, you can depend on negative integer here and on the >> fact that >> the u8 will never be 32-bits wide (to produce a negative integer when >> the return >> value is valid), but personally I think this is error-prone as hell. >> >>> If I go by your code above, after returning from above, >>> check for return value for successful read >>> and then check the respective bit set(SR_*). ? >> Yes, you will be checking the bit in SR only if you are sure the >> value is valid. > hmm..alrite I will do the cleanup and send v2. I think it will be better to take the above recommended cleanup as a seperate patch on top of $subject patch? Since, read_sr is not only used by the pieces added in this patch. There are other already available apis in the driver which make use of this function. > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
于 2013年10月30日 02:34, Sourav Poddar 写道: > I think it will be better to take the above recommended cleanup as a > seperate patch > on top of $subject patch? Since, read_sr is not only used by the > pieces added in this patch. > There are other already available apis in the driver which make use of > this function. yes, I also think use a seperate patch to fix the read_sr() (If you want to change it.) Btw, please add the version info, such "PATCH V3", in the title of your new patch. thanks Huang Shijie
Hi Huang, On Wednesday 30 October 2013 11:57 AM, Huang Shijie wrote: > 于 2013年10月30日 02:34, Sourav Poddar 写道: >> I think it will be better to take the above recommended cleanup as a >> seperate patch >> on top of $subject patch? Since, read_sr is not only used by the >> pieces added in this patch. >> There are other already available apis in the driver which make use >> of this function. > yes, I also think use a seperate patch to fix the read_sr() (If you > want to change it.) > > Btw, please add the version info, such "PATCH V3", in the title of > your new patch. > So done, I will try to clean up Marek's suggestion as a seperate patch once the $subject patch is done and dusted. There was a small comment by Marek on chaning the error print info, If there are no further comments on $subject patch, I will go ahead and fix that small comments and resend the series. I did prefer to name it as V2 to have consistency with this thread. ? > thanks > Huang Shijie >
于 2013年10月30日 14:46, Sourav Poddar 写道:
> I did prefer to name it as V2 to have consistency with this thread. ?
it's okay. :)
thanks
Huang Shijie
Dear Sourav Poddar, > On Tuesday 29 October 2013 10:42 PM, Sourav Poddar wrote: > > On Tuesday 29 October 2013 10:38 PM, Marek Vasut wrote: > >> Dear Sourav Poddar, > >> > >>> On Tuesday 29 October 2013 08:57 PM, Marek Vasut wrote: > >>>> Dear Sourav Poddar, > >>>> > >>>>> Dear Marek Vasut, > >>>>> > >>>>> On Tuesday 29 October 2013 07:31 PM, Marek Vasut wrote: > >>>>>> Dear Sourav Poddar, > >>>>>> > >>>>>>> Hi, > >>>>>>> > >>>>>>> On Sunday 27 October 2013 10:15 PM, Marek Vasut wrote: > >>>>>>>> Dear Sourav Poddar, > >>>>>>>> > >>>>>>>> [...] > >>>>>>>> > >>>>>>>>> +static int macronix_quad_enable(struct m25p *flash) > >>>>>>>>> +{ > >>>>>>>>> + int ret, val; > >>>>>>>>> + u8 cmd[2]; > >>>>>>>>> + cmd[0] = OPCODE_WRSR; > >>>>>>>>> + > >>>>>>>>> + val = read_sr(flash); > >>>>>>>>> + cmd[1] = val | SR_QUAD_EN_MX; > >>>>>>>>> + write_enable(flash); > >>>>>>>>> + > >>>>>>>>> + spi_write(flash->spi,&cmd, 2); > >>>>>>>>> + > >>>>>>>>> + if (wait_till_ready(flash)) > >>>>>>>>> + return 1; > >>>>>>>>> + > >>>>>>>>> + ret = read_sr(flash); > >>>>>>>> > >>>>>>>> Maybe read_sr() and read_cr() shall be fixed to return retval only > >>>>>>>> and the val shall be passed to them as an argument pointer? > >>>>>>>> Aka. ret > >>>>>>>> = read_sr(flash,&val); > >>>>>>>> > >>>>>>>> That way, this dangerous construct below could become: > >>>>>>>> > >>>>>>>> if (!(val& SR_....)) { > >>>>>>>> > >>>>>>>> dev_err(); > >>>>>>>> ret = -EINVAL; > >>>>>>>> > >>>>>>>> } > >>>>>>>> > >>>>>>>> return ret; > >>>>>>> > >>>>>>> I was trying to work on it and realise, we dont need to pass val > >>>>>>> directly. We can continue returning the val and can still > >>>>>>> cleanup the > >>>>>>> below code as u suggetsed above. > >>>>>>> if (!(ret& SR_....)) { > >>>>>>> > >>>>>>> dev_err(); > >>>>>>> ret = -EINVAL; > >>>>>>> > >>>>>>> } > >>>>>> > >>>>>> Uh oh, no. This doesn't seem right. I'd like to be able to clearly > >>>>>> check if the function failed to read the register altogether OR if > >>>>>> not, check the returned value of the register. Mixing these two > >>>>>> together won't do us good. But maybe I just fail to understand your > >>>>>> proposal, if so, then I appologize. > >>>>> > >>>>> Yes, what I am trying to propose is to eliminate the return error > >>>>> check. > >>>> > >>>> But we want to be able to check if there is a failure :) > >>>> > >>>>> The check whether register read has happened correctly is embedded in > >>>>> read_sr/read_cr function itself. > >>>>> > >>>>> if (retval< 0) { > >>>>> > >>>>> dev_err(&flash->spi->dev, "error %d reading SR\n", > >>>>> > >>>>> (int) retval); > >>>>> > >>>>> return retval; > >>>>> > >>>>> } > >>>>> > >>>>> Same goes for read_cr. > >>>>> So, if the above condition is not hit, we simply return the read > >>>>> value > >>>>> and check it with the respective bits. > >>>> > >>>> Look here: > >>>> 107 static int read_sr(struct m25p *flash) > >>>> 108 { > >>>> 109 ssize_t retval; > >>>> 110 u8 code = OPCODE_RDSR; > >>>> 111 u8 val; > >>>> 112 > >>>> 113 retval = spi_write_then_read(flash->spi,&code, > >>>> > >>>> 1,&val, 1); > >>>> > >>>> 114 > >>>> 115 if (retval< 0) { > >>>> 116 dev_err(&flash->spi->dev, "error %d reading > >>>> > >>>> SR\n", > >>>> > >>>> 117 (int) retval); > >>>> 118 return retval; > >>>> > >>>> here you return error value IFF spi_write_then_read() fails for some > >>>> reason. > >>>> > >>>> 119 } > >>>> 120 > >>>> 121 return val; > >>>> > >>>> here you return actual value of the register. > >>>> > >>>> 122 } > >>>> > >>>> This is how I'd change the function to make it less error-prone: > >>>> > >>>> *107 static int read_sr(struct m25p *flash, u8 *rval) > >>>> > >>>> 108 { > >>>> 109 ssize_t retval; > >>>> 110 u8 code = OPCODE_RDSR; > >>>> 111 u8 val; > >>>> 112 > >>>> 113 retval = spi_write_then_read(flash->spi,&code, > >>>> > >>>> 1,&val, 1); > >>>> > >>>> 114 > >>>> 115 if (retval< 0) { > >>>> 116 dev_err(&flash->spi->dev, "error %d reading > >>>> > >>>> SR\n", > >>>> > >>>> 117 (int) retval); > >>>> 118 return retval; > >>>> 119 } > >>>> > >>>> *120 *rval = val; > >>>> *121 return 0; > >>>> > >>>> 122 } > >>>> > >>>> This way, you can check if the SPI read failed and if so, handle it in > >>>> some way. The return value would only be valid if this function > >>>> returned > >>>> 0. > >>> > >>> I got this, but do you think its necessary to have two checks for > >>> verifying > >>> whether read passed. ? > >> > >> Yes of course it is necessary, how else would you be able to tell if > >> the value > >> is valid ? Sure, you can depend on negative integer here and on the > >> fact that > >> the u8 will never be 32-bits wide (to produce a negative integer when > >> the return > >> value is valid), but personally I think this is error-prone as hell. > >> > >>> If I go by your code above, after returning from above, > >>> check for return value for successful read > >>> and then check the respective bit set(SR_*). ? > >> > >> Yes, you will be checking the bit in SR only if you are sure the > >> value is valid. > > > > hmm..alrite I will do the cleanup and send v2. > > I think it will be better to take the above recommended cleanup as a > seperate patch > on top of $subject patch? Separate patch is OK, but I think it's better to put it before this series to not spread this bad practice further. Again, I will wave at Brian to stop my possible misguidance ASAP here.
On Wed, Oct 30, 2013 at 11:11:54AM +0100, Marek Vasut wrote: > Dear Sourav Poddar, > > > On Tuesday 29 October 2013 10:42 PM, Sourav Poddar wrote: > > > On Tuesday 29 October 2013 10:38 PM, Marek Vasut wrote: > > >> Dear Sourav Poddar, > > >> > > >>> On Tuesday 29 October 2013 08:57 PM, Marek Vasut wrote: > > >>>> Dear Sourav Poddar, > > >>>> > > >>>>> Dear Marek Vasut, > > >>>>> > > >>>>> On Tuesday 29 October 2013 07:31 PM, Marek Vasut wrote: > > >>>>>> Dear Sourav Poddar, > > >>>>>> > > >>>>>>> Hi, > > >>>>>>> > > >>>>>>> On Sunday 27 October 2013 10:15 PM, Marek Vasut wrote: > > >>>>>>>> Dear Sourav Poddar, > > >>>>>>>> > > >>>>>>>> [...] > > >>>>>>>> > > >>>>>>>>> +static int macronix_quad_enable(struct m25p *flash) > > >>>>>>>>> +{ > > >>>>>>>>> + int ret, val; > > >>>>>>>>> + u8 cmd[2]; > > >>>>>>>>> + cmd[0] = OPCODE_WRSR; > > >>>>>>>>> + > > >>>>>>>>> + val = read_sr(flash); > > >>>>>>>>> + cmd[1] = val | SR_QUAD_EN_MX; > > >>>>>>>>> + write_enable(flash); > > >>>>>>>>> + > > >>>>>>>>> + spi_write(flash->spi,&cmd, 2); > > >>>>>>>>> + > > >>>>>>>>> + if (wait_till_ready(flash)) > > >>>>>>>>> + return 1; > > >>>>>>>>> + > > >>>>>>>>> + ret = read_sr(flash); > > >>>>>>>> > > >>>>>>>> Maybe read_sr() and read_cr() shall be fixed to return retval only > > >>>>>>>> and the val shall be passed to them as an argument pointer? > > >>>>>>>> Aka. ret > > >>>>>>>> = read_sr(flash,&val); > > >>>>>>>> > > >>>>>>>> That way, this dangerous construct below could become: > > >>>>>>>> > > >>>>>>>> if (!(val& SR_....)) { > > >>>>>>>> > > >>>>>>>> dev_err(); > > >>>>>>>> ret = -EINVAL; > > >>>>>>>> > > >>>>>>>> } > > >>>>>>>> > > >>>>>>>> return ret; > > >>>>>>> > > >>>>>>> I was trying to work on it and realise, we dont need to pass val > > >>>>>>> directly. We can continue returning the val and can still > > >>>>>>> cleanup the > > >>>>>>> below code as u suggetsed above. > > >>>>>>> if (!(ret& SR_....)) { > > >>>>>>> > > >>>>>>> dev_err(); > > >>>>>>> ret = -EINVAL; > > >>>>>>> > > >>>>>>> } > > >>>>>> > > >>>>>> Uh oh, no. This doesn't seem right. I'd like to be able to clearly > > >>>>>> check if the function failed to read the register altogether OR if > > >>>>>> not, check the returned value of the register. Mixing these two > > >>>>>> together won't do us good. But maybe I just fail to understand your > > >>>>>> proposal, if so, then I appologize. > > >>>>> > > >>>>> Yes, what I am trying to propose is to eliminate the return error > > >>>>> check. > > >>>> > > >>>> But we want to be able to check if there is a failure :) > > >>>> > > >>>>> The check whether register read has happened correctly is embedded in > > >>>>> read_sr/read_cr function itself. > > >>>>> > > >>>>> if (retval< 0) { > > >>>>> > > >>>>> dev_err(&flash->spi->dev, "error %d reading SR\n", > > >>>>> > > >>>>> (int) retval); > > >>>>> > > >>>>> return retval; > > >>>>> > > >>>>> } > > >>>>> > > >>>>> Same goes for read_cr. > > >>>>> So, if the above condition is not hit, we simply return the read > > >>>>> value > > >>>>> and check it with the respective bits. > > >>>> > > >>>> Look here: > > >>>> 107 static int read_sr(struct m25p *flash) > > >>>> 108 { > > >>>> 109 ssize_t retval; > > >>>> 110 u8 code = OPCODE_RDSR; > > >>>> 111 u8 val; > > >>>> 112 > > >>>> 113 retval = spi_write_then_read(flash->spi,&code, > > >>>> > > >>>> 1,&val, 1); > > >>>> > > >>>> 114 > > >>>> 115 if (retval< 0) { > > >>>> 116 dev_err(&flash->spi->dev, "error %d reading > > >>>> > > >>>> SR\n", > > >>>> > > >>>> 117 (int) retval); > > >>>> 118 return retval; > > >>>> > > >>>> here you return error value IFF spi_write_then_read() fails for some > > >>>> reason. > > >>>> > > >>>> 119 } > > >>>> 120 > > >>>> 121 return val; > > >>>> > > >>>> here you return actual value of the register. > > >>>> > > >>>> 122 } > > >>>> > > >>>> This is how I'd change the function to make it less error-prone: > > >>>> > > >>>> *107 static int read_sr(struct m25p *flash, u8 *rval) > > >>>> > > >>>> 108 { > > >>>> 109 ssize_t retval; > > >>>> 110 u8 code = OPCODE_RDSR; > > >>>> 111 u8 val; > > >>>> 112 > > >>>> 113 retval = spi_write_then_read(flash->spi,&code, > > >>>> > > >>>> 1,&val, 1); > > >>>> > > >>>> 114 > > >>>> 115 if (retval< 0) { > > >>>> 116 dev_err(&flash->spi->dev, "error %d reading > > >>>> > > >>>> SR\n", > > >>>> > > >>>> 117 (int) retval); > > >>>> 118 return retval; > > >>>> 119 } > > >>>> > > >>>> *120 *rval = val; > > >>>> *121 return 0; > > >>>> > > >>>> 122 } > > >>>> > > >>>> This way, you can check if the SPI read failed and if so, handle it in > > >>>> some way. The return value would only be valid if this function > > >>>> returned > > >>>> 0. > > >>> > > >>> I got this, but do you think its necessary to have two checks for > > >>> verifying > > >>> whether read passed. ? > > >> > > >> Yes of course it is necessary, how else would you be able to tell if > > >> the value > > >> is valid ? Sure, you can depend on negative integer here and on the > > >> fact that > > >> the u8 will never be 32-bits wide (to produce a negative integer when > > >> the return > > >> value is valid), but personally I think this is error-prone as hell. > > >> > > >>> If I go by your code above, after returning from above, > > >>> check for return value for successful read > > >>> and then check the respective bit set(SR_*). ? > > >> > > >> Yes, you will be checking the bit in SR only if you are sure the > > >> value is valid. > > > > > > hmm..alrite I will do the cleanup and send v2. > > > > I think it will be better to take the above recommended cleanup as a > > seperate patch > > on top of $subject patch? > > Separate patch is OK, but I think it's better to put it before this series to > not spread this bad practice further. > > Again, I will wave at Brian to stop my possible misguidance ASAP here. Wow, I have to apologize for my lack of attention span (and more importantly, lack of clear organizational skills for managing a large inbox). I missed Marek's pleas for help here, and only noticed after I came to clean out the old versions of Sourav's patch. I'm not entirely convinced that the "negative means error; positive means return value" construct needs changed. I've seen that in a few other APIs, and it's worked OK (e.g., the mtd->_read() API, where a driver can return 'max bitflips' as a non-negative integer, or negative for an error). Of course, this case is slightly different, as we return a full status register. But I think the guarantee that the status is 8 bits gives us headroom to make sure the positive and negative cases cannot clash. But now that I'm noticing your request here... I think the existing style of checks separates the error and success case clearly (in wait_till_ready()): if ((sr = read_sr(flash)) < 0) break; else if (!(sr & SR_WIP)) return 0; Where Sourav's code chooses brevity while sacrificing clarity: ret = read_sr(flash); if (!(ret > 0 && (ret & SR_QUAD_EN_MX))) { dev_err(&flash->spi->dev, "Macronix Quad bit not set"); return -EINVAL; } It'd be clearer to say: ret = read_sr(flash); if (ret < 0) return ret; else if (!(ret & SR_QUAD_EN_MX)) { dev_err(&flash->spi->dev, "Macronix Quad bit not set\n"); return -EINVAL; /* Probably -EIO? */ } Feel free to follow up with a patch if you'd like. Brian
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index d6c5c57..88f9421 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -41,6 +41,7 @@ #define OPCODE_WRSR 0x01 /* Write status register 1 byte */ #define OPCODE_NORM_READ 0x03 /* Read data bytes (low frequency) */ #define OPCODE_FAST_READ 0x0b /* Read data bytes (high frequency) */ +#define OPCODE_QUAD_READ 0x6b /* Read data bytes */ #define OPCODE_PP 0x02 /* Page program (up to 256 bytes) */ #define OPCODE_BE_4K 0x20 /* Erase 4KiB block */ #define OPCODE_BE_4K_PMC 0xd7 /* Erase 4KiB block on PMC chips */ @@ -48,10 +49,12 @@ #define OPCODE_CHIP_ERASE 0xc7 /* Erase whole flash chip */ #define OPCODE_SE 0xd8 /* Sector erase (usually 64KiB) */ #define OPCODE_RDID 0x9f /* Read JEDEC ID */ +#define OPCODE_RDCR 0x35 /* Read configuration register */ /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */ #define OPCODE_NORM_READ_4B 0x13 /* Read data bytes (low frequency) */ #define OPCODE_FAST_READ_4B 0x0c /* Read data bytes (high frequency) */ +#define OPCODE_QUAD_READ_4B 0x6c /* Read data bytes */ #define OPCODE_PP_4B 0x12 /* Page program (up to 256 bytes) */ #define OPCODE_SE_4B 0xdc /* Sector erase (usually 64KiB) */ @@ -76,6 +79,10 @@ #define SR_BP2 0x10 /* Block protect 2 */ #define SR_SRWD 0x80 /* SR write protect */ +/* Configuration Register bits. */ +#define CR_QUAD_EN_SPAN 0x2 /* Spansion Quad I/O */ +#define SR_QUAD_EN_MX 0x40 /* Macronix Quad I/O */ + /* Define max times to check status register before we give up. */ #define MAX_READY_WAIT_JIFFIES (40 * HZ) /* M25P16 specs 40s max chip erase */ #define MAX_CMD_SIZE 6 @@ -95,6 +102,7 @@ struct m25p { u8 program_opcode; u8 *command; bool fast_read; + bool quad_read; }; static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd) @@ -131,6 +139,26 @@ static int read_sr(struct m25p *flash) } /* + * Read configuration register, returning its value in the + * location. Return the configuration register value. + * Returns negative if error occured. + */ +static int read_cr(struct m25p *flash) +{ + u8 code = OPCODE_RDCR; + int ret; + u8 val; + + ret = spi_write_then_read(flash->spi, &code, 1, &val, 1); + if (ret < 0) { + dev_err(&flash->spi->dev, "error %d reading CR\n", ret); + return ret; + } + + return val; +} + +/* * Write status register 1 byte * Returns negative if error occurred. */ @@ -220,6 +248,95 @@ static int wait_till_ready(struct m25p *flash) } /* + * Write status Register and configuration register with 2 bytes + * The first byte will be written to the status register, while the + * second byte will be written to the configuration register. + * Return negative if error occured. + */ +static int write_sr_cr(struct m25p *flash, u16 val) +{ + flash->command[0] = OPCODE_WRSR; + flash->command[1] = val & 0xff; + flash->command[2] = (val >> 8); + + return spi_write(flash->spi, flash->command, 3); +} + +static int macronix_quad_enable(struct m25p *flash) +{ + int ret, val; + u8 cmd[2]; + cmd[0] = OPCODE_WRSR; + + val = read_sr(flash); + cmd[1] = val | SR_QUAD_EN_MX; + write_enable(flash); + + spi_write(flash->spi, &cmd, 2); + + if (wait_till_ready(flash)) + return 1; + + ret = read_sr(flash); + if (!(ret > 0 && (ret & SR_QUAD_EN_MX))) { + dev_err(&flash->spi->dev, + "Macronix Quad bit not set"); + return -EINVAL; + } + + return 0; +} + +static int spansion_quad_enable(struct m25p *flash) +{ + int ret; + int quad_en = CR_QUAD_EN_SPAN << 8; + + write_enable(flash); + + ret = write_sr_cr(flash, quad_en); + if (ret < 0) { + dev_err(&flash->spi->dev, + "error while writing configuration register"); + return -EINVAL; + } + + /* read back and check it */ + ret = read_cr(flash); + if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) { + dev_err(&flash->spi->dev, + "Spansion Quad bit not set"); + return -EINVAL; + } + + return 0; +} + +static inline int set_quad_mode(struct m25p *flash, u32 jedec_id) +{ + int status; + + switch (JEDEC_MFR(jedec_id)) { + case CFI_MFR_MACRONIX: + status = macronix_quad_enable(flash); + if (status) { + dev_err(&flash->spi->dev, + "Macronix quad not enable"); + return -EINVAL; + } + return status; + default: + status = spansion_quad_enable(flash); + if (status) { + dev_err(&flash->spi->dev, + "Spansion quad not enable"); + return -EINVAL; + } + return status; + } +} + +/* * Erase the whole flash memory * * Returns 0 if successful, non-zero otherwise. @@ -253,11 +370,24 @@ static void m25p_addr2cmd(struct m25p *flash, unsigned int addr, u8 *cmd) cmd[4] = addr >> (flash->addr_width * 8 - 32); } +/* + * Dummy Cycle calculation for fast and quad read. + * It can be used to support more commands with + * different dummy cycle requirement. + */ static int m25p_cmdsz(struct m25p *flash) { return 1 + flash->addr_width; } +static inline int m25p80_dummy_cycles_read(struct m25p *flash) +{ + if (flash->quad_read || flash->fast_read) + return 1; + + return 0; +} + /* * Erase one sector of flash memory at offset ``offset'' which is any * address within the sector which should be erased. @@ -368,9 +498,10 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len, memset(t, 0, (sizeof t)); t[0].tx_buf = flash->command; - t[0].len = m25p_cmdsz(flash) + (flash->fast_read ? 1 : 0); + t[0].len = m25p_cmdsz(flash) + m25p80_dummy_cycles_read(flash); spi_message_add_tail(&t[0], &m); + t[1].rx_nbits = flash->quad_read ? SPI_NBITS_QUAD : 1; t[1].rx_buf = buf; t[1].len = len; spi_message_add_tail(&t[1], &m); @@ -392,7 +523,7 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len, spi_sync(flash->spi, &m); *retlen = m.actual_length - m25p_cmdsz(flash) - - (flash->fast_read ? 1 : 0); + m25p80_dummy_cycles_read(flash); mutex_unlock(&flash->lock); @@ -698,6 +829,7 @@ struct flash_info { #define SST_WRITE 0x04 /* use SST byte programming */ #define M25P_NO_FR 0x08 /* Can't do fastread */ #define SECT_4K_PMC 0x10 /* OPCODE_BE_4K_PMC works uniformly */ +#define M25P80_QUAD_READ 0x20 /* Flash supports Quad Read */ }; #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \ @@ -774,7 +906,7 @@ static const struct spi_device_id m25p_ids[] = { { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) }, { "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) }, { "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) }, - { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, 0) }, + { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, M25P80_QUAD_READ) }, /* Micron */ { "n25q064", INFO(0x20ba17, 0, 64 * 1024, 128, 0) }, @@ -794,7 +926,7 @@ static const struct spi_device_id m25p_ids[] = { { "s25sl032p", INFO(0x010215, 0x4d00, 64 * 1024, 64, 0) }, { "s25sl064p", INFO(0x010216, 0x4d00, 64 * 1024, 128, 0) }, { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, 0) }, - { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, 0) }, + { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, M25P80_QUAD_READ) }, { "s25fl512s", INFO(0x010220, 0x4d00, 256 * 1024, 256, 0) }, { "s70fl01gs", INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) }, { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024, 64, 0) }, @@ -936,6 +1068,7 @@ static int m25p_probe(struct spi_device *spi) unsigned i; struct mtd_part_parser_data ppdata; struct device_node __maybe_unused *np = spi->dev.of_node; + int ret; #ifdef CONFIG_MTD_OF_PARTS if (!of_device_is_available(np)) @@ -1055,6 +1188,15 @@ static int m25p_probe(struct spi_device *spi) flash->page_size = info->page_size; flash->mtd.writebufsize = flash->page_size; + if (spi->mode & SPI_RX_QUAD && info->flags & M25P80_QUAD_READ) { + ret = set_quad_mode(flash, info->jedec_id); + if (ret) { + dev_err(&flash->spi->dev, "quad mode not supported\n"); + return ret; + } + flash->quad_read = true; + } + if (np) /* If we were instantiated by DT, use it */ flash->fast_read = of_property_read_bool(np, "m25p,fast-read"); @@ -1067,7 +1209,9 @@ static int m25p_probe(struct spi_device *spi) flash->fast_read = false; /* Default commands */ - if (flash->fast_read) + if (flash->quad_read) + flash->read_opcode = OPCODE_QUAD_READ; + else if (flash->fast_read) flash->read_opcode = OPCODE_FAST_READ; else flash->read_opcode = OPCODE_NORM_READ; @@ -1081,6 +1225,12 @@ static int m25p_probe(struct spi_device *spi) flash->addr_width = 4; if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) { /* Dedicated 4-byte command set */ + if (flash->quad_read) + flash->read_opcode = OPCODE_QUAD_READ_4B; + else + flash->read_opcode = flash->fast_read ? + OPCODE_FAST_READ_4B : + OPCODE_NORM_READ_4B; flash->read_opcode = flash->fast_read ? OPCODE_FAST_READ_4B : OPCODE_NORM_READ_4B;
Some flash also support quad read mode. Adding support for adding quad mode in m25p80 for spansion and macronix flash. Signed-off-by: Sourav Poddar <sourav.poddar@ti.com> --- This posted was initially posted as a series[1], posting this as a seperate feature patch, while other patch in that series will go as a seperate feature patch. Brian, I have rebased this patches on top of l2-mtd + http://permalink.gmane.org/gmane.linux.drivers.mtd/49129 Changes from the previous version [1] - Added deault 4 byte command. - Make the code more modular by using different apis "set_quad_mode" etc. - Remove few bug fixes which went as a seperate patch from Brian's series. - Miscellaneous cleanups. - Change the subject to reflect m25p80 changes. [1]: http://permalink.gmane.org/gmane.linux.drivers.mtd/48755 drivers/mtd/devices/m25p80.c | 160 ++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 155 insertions(+), 5 deletions(-)