I am tackling on this driver to use it for my SoCs.
The difficulty is a bunch of platform specific stuff
(more specifically, Intel MRST specific) is hard-coded in this driver.
I need lots of rework to utilize the driver for generic cases,
but at the same time, I found the driver code is really dirty,
lots of unused code, odd comments, etc.
The first thing I needed to do was to clean up the code.
My work is still under the way, but I decided to drop this series
for now. I hope this series is easy to review, so I guess
splitting into a small chunks is better than a one-shot patch bomb.
Masahiro Yamada (11):
mtd: nand: denali: remove unneeded <linux/slab.h> includes
mtd: nand: denali: remove unused struct member denali_nand_info::idx
mtd: nand: denali: remove bogus comment about interrupt handler setup
mtd: nand: denali: remove detect_partition_feature()
mtd: nand: denali: remove "Spectra:" prefix from printk strings
mtd: nand: denali: remove unused struct member totalblks, blksperchip
mtd: nand: denali: use managed devm_irq_request()
mtd: nand: denali: return error code from devm_request_irq() on error
mtd: nand: denali: return error code from nand_scan_ident/tail on
error
mtd: nand: denali: remove unneeded parentheses
mtd: nand: denali: remove debug lines of __FILE__, __LINE__, __func__
drivers/mtd/nand/denali.c | 101 +++++++++---------------------------------
drivers/mtd/nand/denali.h | 12 -----
drivers/mtd/nand/denali_dt.c | 1 -
drivers/mtd/nand/denali_pci.c | 1 -
4 files changed, 21 insertions(+), 94 deletions(-)
--
1.9.1
Use the managed variant instead of request_irq() and free_irq().
Signed-off-by: Masahiro Yamada <[email protected]>
---
drivers/mtd/nand/denali.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 548278b..44e075a 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -575,7 +575,6 @@ static void denali_irq_init(struct denali_nand_info *denali)
static void denali_irq_cleanup(int irqnum, struct denali_nand_info *denali)
{
denali_set_intr_modes(denali, false);
- free_irq(irqnum, denali);
}
static void denali_irq_enable(struct denali_nand_info *denali,
@@ -1456,8 +1455,8 @@ int denali_init(struct denali_nand_info *denali)
* denali_isr register is done after all the hardware
* initilization is finished
*/
- if (request_irq(denali->irq, denali_isr, IRQF_SHARED,
- DENALI_NAND_NAME, denali)) {
+ if (devm_request_irq(denali->dev, denali->irq, denali_isr, IRQF_SHARED,
+ DENALI_NAND_NAME, denali)) {
dev_err(denali->dev, "Unable to request IRQ\n");
return -ENODEV;
}
--
1.9.1
The nand_scan_ident/tail() returns an appropriate error value when
it fails. Use it instead of the fixed -ENXIO.
Signed-off-by: Masahiro Yamada <[email protected]>
---
drivers/mtd/nand/denali.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index f188a48..d482d8d 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1474,10 +1474,9 @@ int denali_init(struct denali_nand_info *denali)
* this is the first stage in a two step process to register
* with the nand subsystem
*/
- if (nand_scan_ident(mtd, denali->max_banks, NULL)) {
- ret = -ENXIO;
+ ret = nand_scan_ident(mtd, denali->max_banks, NULL);
+ if (ret)
goto failed_req_irq;
- }
/* allocate the right size buffer now */
devm_kfree(denali->dev, denali->buf.buf);
@@ -1580,10 +1579,9 @@ int denali_init(struct denali_nand_info *denali)
denali->nand.ecc.write_oob = denali_write_oob;
denali->nand.erase = denali_erase;
- if (nand_scan_tail(mtd)) {
- ret = -ENXIO;
+ ret = nand_scan_tail(mtd);
+ if (ret)
goto failed_req_irq;
- }
ret = mtd_device_register(mtd, NULL, 0);
if (ret) {
--
1.9.1
Such debug lines might be useful when debugging the driver first,
but should be deleted from the upstream code.
Signed-off-by: Masahiro Yamada <[email protected]>
---
drivers/mtd/nand/denali.c | 12 ------------
1 file changed, 12 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 14e66ab..e32e13c 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -181,9 +181,6 @@ static uint16_t denali_nand_reset(struct denali_nand_info *denali)
{
int i;
- dev_dbg(denali->dev, "%s, Line %d, Function: %s\n",
- __FILE__, __LINE__, __func__);
-
for (i = 0; i < denali->max_banks; i++)
iowrite32(INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT,
denali->flash_reg + INTR_STATUS(i));
@@ -233,9 +230,6 @@ static void nand_onfi_timing_set(struct denali_nand_info *denali,
uint16_t acc_clks;
uint16_t addr_2_data, re_2_we, re_2_re, we_2_re, cs_cnt;
- dev_dbg(denali->dev, "%s, Line %d, Function: %s\n",
- __FILE__, __LINE__, __func__);
-
en_lo = CEIL_DIV(Trp[mode], CLK_X);
en_hi = CEIL_DIV(Treh[mode], CLK_X);
#if ONFI_BLOOM_TIME
@@ -480,9 +474,6 @@ static uint16_t denali_nand_timing_set(struct denali_nand_info *denali)
uint8_t maf_id, device_id;
int i;
- dev_dbg(denali->dev, "%s, Line %d, Function: %s\n",
- __FILE__, __LINE__, __func__);
-
/*
* Use read id method to get device ID and other params.
* For some NAND chips, controller can't report the correct
@@ -537,9 +528,6 @@ static uint16_t denali_nand_timing_set(struct denali_nand_info *denali)
static void denali_set_intr_modes(struct denali_nand_info *denali,
uint16_t INT_ENABLE)
{
- dev_dbg(denali->dev, "%s, Line %d, Function: %s\n",
- __FILE__, __LINE__, __func__);
-
if (INT_ENABLE)
iowrite32(1, denali->flash_reg + GLOBAL_INT_ENABLE);
else
--
1.9.1
Remove parentheses surrounding the whole right side of an assignment.
Signed-off-by: Masahiro Yamada <[email protected]>
---
drivers/mtd/nand/denali.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index d482d8d..14e66ab 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1510,16 +1510,16 @@ int denali_init(struct denali_nand_info *denali)
* the real pagesize and anything necessery
*/
denali->devnum = ioread32(denali->flash_reg + DEVICES_CONNECTED);
- denali->nand.chipsize <<= (denali->devnum - 1);
- denali->nand.page_shift += (denali->devnum - 1);
+ denali->nand.chipsize <<= denali->devnum - 1;
+ denali->nand.page_shift += denali->devnum - 1;
denali->nand.pagemask = (denali->nand.chipsize >>
denali->nand.page_shift) - 1;
- denali->nand.bbt_erase_shift += (denali->devnum - 1);
+ denali->nand.bbt_erase_shift += denali->devnum - 1;
denali->nand.phys_erase_shift = denali->nand.bbt_erase_shift;
- denali->nand.chip_shift += (denali->devnum - 1);
- mtd->writesize <<= (denali->devnum - 1);
- mtd->oobsize <<= (denali->devnum - 1);
- mtd->erasesize <<= (denali->devnum - 1);
+ denali->nand.chip_shift += denali->devnum - 1;
+ mtd->writesize <<= denali->devnum - 1;
+ mtd->oobsize <<= denali->devnum - 1;
+ mtd->erasesize <<= denali->devnum - 1;
mtd->size = denali->nand.numchips * denali->nand.chipsize;
denali->bbtskipbytes *= denali->devnum;
--
1.9.1
The devm_request_irq() returns an appropriate error value when it
fails. Use it instead of the fixed -ENODEV.
While we are here, reword the comment to make it fit in a single
line, fixing the misspelling of "initialization".
Signed-off-by: Masahiro Yamada <[email protected]>
---
drivers/mtd/nand/denali.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 44e075a..f188a48 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1451,14 +1451,12 @@ int denali_init(struct denali_nand_info *denali)
denali_hw_init(denali);
denali_drv_init(denali);
- /*
- * denali_isr register is done after all the hardware
- * initilization is finished
- */
- if (devm_request_irq(denali->dev, denali->irq, denali_isr, IRQF_SHARED,
- DENALI_NAND_NAME, denali)) {
+ /* request IRQ after all the hardware initialization is finished */
+ ret = devm_request_irq(denali->dev, denali->irq, denali_isr,
+ IRQF_SHARED, DENALI_NAND_NAME, denali);
+ if (ret) {
dev_err(denali->dev, "Unable to request IRQ\n");
- return -ENODEV;
+ return ret;
}
/* now that our ISR is registered, we can enable interrupts */
--
1.9.1
The interrupt handler is setup in denali_init(), not in
denali_drv_init(). This comment is false.
Such a comment adds no value, so just delete it instead of move.
Signed-off-by: Masahiro Yamada <[email protected]>
---
drivers/mtd/nand/denali.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 51ddb84..d6f1b29 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1436,7 +1436,6 @@ static int denali_ooblayout_free(struct mtd_info *mtd, int section,
/* initialize driver data structures */
static void denali_drv_init(struct denali_nand_info *denali)
{
- /* setup interrupt handler */
/*
* the completion object will be used to notify
* the callee that the interrupt is done
--
1.9.1
The denali->blksperchip is set, but not referenced any more. The
denali->totalblks is used only for calculating denali->blksperchip.
Both of them are unneeded.
Signed-off-by: Masahiro Yamada <[email protected]>
---
drivers/mtd/nand/denali.c | 8 --------
drivers/mtd/nand/denali.h | 2 --
2 files changed, 10 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 78d795b..548278b 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1573,14 +1573,6 @@ int denali_init(struct denali_nand_info *denali)
denali->nand.ecc.bytes *= denali->devnum;
denali->nand.ecc.strength *= denali->devnum;
- /*
- * Let driver know the total blocks number and how many blocks
- * contained by each nand chip. blksperchip will help driver to
- * know how many blocks is taken by FW.
- */
- denali->totalblks = mtd->size >> denali->nand.phys_erase_shift;
- denali->blksperchip = denali->totalblks / denali->nand.numchips;
-
/* override the default read operations */
denali->nand.ecc.size = ECC_SECTOR_SIZE * denali->devnum;
denali->nand.ecc.read_page = denali_read_page;
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index 7c0800d..ea22191 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -462,8 +462,6 @@ struct denali_nand_info {
int irq;
uint32_t devnum; /* represent how many nands connected */
- uint32_t totalblks;
- uint32_t blksperchip;
uint32_t bbtskipbytes;
uint32_t max_banks;
};
--
1.9.1
As far as I understood from the Kconfig menu deleted by commit
be7f39c5ecf5 ("Staging: delete spectra driver"), the "Spectra" is
specific to Intel Moorestown Platform.
The Denali NAND controller IP is used for various SoCs such as
Altera SOCFPGA, Socionext UniPhier, etc. The platform specific
strings are not preferred in this driver.
Signed-off-by: Masahiro Yamada <[email protected]>
---
As an ARM-SoC developer, I only need denali.c and denali_dt.c.
I see some "Spectra:" in drivers/mtd/nand/denali_pci.c as well.
I was not quite sure if they are needed or not.
If desired, I can update this patch to remove them too.
drivers/mtd/nand/denali.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 80d3e26..78d795b 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -402,7 +402,7 @@ static void get_hynix_nand_para(struct denali_nand_info *denali,
break;
default:
dev_warn(denali->dev,
- "Spectra: Unknown Hynix NAND (Device ID: 0x%x).\n"
+ "Unknown Hynix NAND (Device ID: 0x%x).\n"
"Will use default parameter values instead.\n",
device_id);
}
@@ -1458,7 +1458,7 @@ int denali_init(struct denali_nand_info *denali)
*/
if (request_irq(denali->irq, denali_isr, IRQF_SHARED,
DENALI_NAND_NAME, denali)) {
- pr_err("Spectra: Unable to allocate IRQ\n");
+ dev_err(denali->dev, "Unable to request IRQ\n");
return -ENODEV;
}
@@ -1495,7 +1495,7 @@ int denali_init(struct denali_nand_info *denali)
/* Is 32-bit DMA supported? */
ret = dma_set_mask(denali->dev, DMA_BIT_MASK(32));
if (ret) {
- pr_err("Spectra: no usable DMA configuration\n");
+ dev_err(denali->dev, "no usable DMA configuration\n");
goto failed_req_irq;
}
@@ -1503,7 +1503,7 @@ int denali_init(struct denali_nand_info *denali)
mtd->writesize + mtd->oobsize,
DMA_BIDIRECTIONAL);
if (dma_mapping_error(denali->dev, denali->buf.dma_buf)) {
- dev_err(denali->dev, "Spectra: failed to map DMA buffer\n");
+ dev_err(denali->dev, "failed to map DMA buffer\n");
ret = -EIO;
goto failed_req_irq;
}
@@ -1598,8 +1598,7 @@ int denali_init(struct denali_nand_info *denali)
ret = mtd_device_register(mtd, NULL, 0);
if (ret) {
- dev_err(denali->dev, "Spectra: Failed to register MTD: %d\n",
- ret);
+ dev_err(denali->dev, "Failed to register MTD: %d\n", ret);
goto failed_req_irq;
}
return 0;
--
1.9.1
The driver calls devm_kzalloc()/devm_kfree() to allocate/free memory.
They are declared in <linux/device.h>, not in <linux/slab.h>.
Signed-off-by: Masahiro Yamada <[email protected]>
---
drivers/mtd/nand/denali.c | 1 -
drivers/mtd/nand/denali_dt.c | 1 -
drivers/mtd/nand/denali_pci.c | 1 -
3 files changed, 3 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 7e2c650..062d5b5 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -21,7 +21,6 @@
#include <linux/dma-mapping.h>
#include <linux/wait.h>
#include <linux/mutex.h>
-#include <linux/slab.h>
#include <linux/mtd/mtd.h>
#include <linux/module.h>
diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
index f821dc1..5607fcd 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -21,7 +21,6 @@
#include <linux/platform_device.h>
#include <linux/of.h>
#include <linux/of_device.h>
-#include <linux/slab.h>
#include "denali.h"
diff --git a/drivers/mtd/nand/denali_pci.c b/drivers/mtd/nand/denali_pci.c
index de31514..ac84323 100644
--- a/drivers/mtd/nand/denali_pci.c
+++ b/drivers/mtd/nand/denali_pci.c
@@ -14,7 +14,6 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/pci.h>
-#include <linux/slab.h>
#include "denali.h"
--
1.9.1
The struct member "idx" was used as an index for debug_array long
ago, but the DEBUG_DENALI feature was removed by commit 7cfffac06ca0
("nand/denali: use dev_xx debug function to replace nand_dbg_print
and some printk"). Since then, this has been only initialized, but
never referenced.
Signed-off-by: Masahiro Yamada <[email protected]>
---
drivers/mtd/nand/denali.c | 2 --
drivers/mtd/nand/denali.h | 1 -
2 files changed, 3 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 062d5b5..51ddb84 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1436,8 +1436,6 @@ static int denali_ooblayout_free(struct mtd_info *mtd, int section,
/* initialize driver data structures */
static void denali_drv_init(struct denali_nand_info *denali)
{
- denali->idx = 0;
-
/* setup interrupt handler */
/*
* the completion object will be used to notify
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index e7ab486..0ce7344 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -467,7 +467,6 @@ struct denali_nand_info {
spinlock_t irq_lock;
uint32_t irq_status;
int irq_debug_array[32];
- int idx;
int irq;
uint32_t devnum; /* represent how many nands connected */
--
1.9.1
The denali->fwblks is set by detect_partition_feature(), but it is
not referenced from anywhere. That means the struct member fwblks
and the whole of detect_partition_feature() are unneeded.
The comment block implies this function is only for Intel platforms.
I found drivers/staging/spectra used to exist, but it was deleted by
commit be7f39c5ecf5 ("Staging: delete spectra driver") 5 years ago.
So, I guess nobody would need this function any more.
Signed-off-by: Masahiro Yamada <[email protected]>
---
drivers/mtd/nand/denali.c | 29 -----------------------------
drivers/mtd/nand/denali.h | 9 ---------
2 files changed, 38 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index d6f1b29..80d3e26 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -473,33 +473,6 @@ static void detect_max_banks(struct denali_nand_info *denali)
denali->max_banks = 1 << (features & FEATURES__N_BANKS);
}
-static void detect_partition_feature(struct denali_nand_info *denali)
-{
- /*
- * For MRST platform, denali->fwblks represent the
- * number of blocks firmware is taken,
- * FW is in protect partition and MTD driver has no
- * permission to access it. So let driver know how many
- * blocks it can't touch.
- */
- if (ioread32(denali->flash_reg + FEATURES) & FEATURES__PARTITION) {
- if ((ioread32(denali->flash_reg + PERM_SRC_ID(1)) &
- PERM_SRC_ID__SRCID) == SPECTRA_PARTITION_ID) {
- denali->fwblks =
- ((ioread32(denali->flash_reg + MIN_MAX_BANK(1)) &
- MIN_MAX_BANK__MIN_VALUE) *
- denali->blksperchip)
- +
- (ioread32(denali->flash_reg + MIN_BLK_ADDR(1)) &
- MIN_BLK_ADDR__VALUE);
- } else {
- denali->fwblks = SPECTRA_START_BLOCK;
- }
- } else {
- denali->fwblks = SPECTRA_START_BLOCK;
- }
-}
-
static uint16_t denali_nand_timing_set(struct denali_nand_info *denali)
{
uint16_t status = PASS;
@@ -551,8 +524,6 @@ static uint16_t denali_nand_timing_set(struct denali_nand_info *denali)
find_valid_banks(denali);
- detect_partition_feature(denali);
-
/*
* If the user specified to override the default timings
* with a specific ONFI mode, we apply those changes here.
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index 0ce7344..7c0800d 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -383,14 +383,6 @@
#define CLK_X 5
#define CLK_MULTI 4
-/* spectraswconfig.h */
-#define CMD_DMA 0
-
-#define SPECTRA_PARTITION_ID 0
-/**** Block Table and Reserved Block Parameters *****/
-#define SPECTRA_START_BLOCK 3
-#define NUM_FREE_BLOCKS_GATE 30
-
/* KBV - Updated to LNW scratch register address */
#define SCRATCH_REG_ADDR CONFIG_MTD_NAND_DENALI_SCRATCH_REG_ADDR
#define SCRATCH_REG_SIZE 64
@@ -470,7 +462,6 @@ struct denali_nand_info {
int irq;
uint32_t devnum; /* represent how many nands connected */
- uint32_t fwblks; /* represent how many blocks FW used */
uint32_t totalblks;
uint32_t blksperchip;
uint32_t bbtskipbytes;
--
1.9.1
On 11/09/2016 05:35 AM, Masahiro Yamada wrote:
> The struct member "idx" was used as an index for debug_array long
> ago, but the DEBUG_DENALI feature was removed by commit 7cfffac06ca0
> ("nand/denali: use dev_xx debug function to replace nand_dbg_print
> and some printk"). Since then, this has been only initialized, but
> never referenced.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Marek Vasut <[email protected]>
> ---
>
> drivers/mtd/nand/denali.c | 2 --
> drivers/mtd/nand/denali.h | 1 -
> 2 files changed, 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 062d5b5..51ddb84 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -1436,8 +1436,6 @@ static int denali_ooblayout_free(struct mtd_info *mtd, int section,
> /* initialize driver data structures */
> static void denali_drv_init(struct denali_nand_info *denali)
> {
> - denali->idx = 0;
> -
> /* setup interrupt handler */
> /*
> * the completion object will be used to notify
> diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
> index e7ab486..0ce7344 100644
> --- a/drivers/mtd/nand/denali.h
> +++ b/drivers/mtd/nand/denali.h
> @@ -467,7 +467,6 @@ struct denali_nand_info {
> spinlock_t irq_lock;
> uint32_t irq_status;
> int irq_debug_array[32];
> - int idx;
> int irq;
>
> uint32_t devnum; /* represent how many nands connected */
>
--
Best regards,
Marek Vasut
On 11/09/2016 05:35 AM, Masahiro Yamada wrote:
> Use the managed variant instead of request_irq() and free_irq().
>
> Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Marek Vasut <[email protected]>
> ---
>
> drivers/mtd/nand/denali.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 548278b..44e075a 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -575,7 +575,6 @@ static void denali_irq_init(struct denali_nand_info *denali)
> static void denali_irq_cleanup(int irqnum, struct denali_nand_info *denali)
> {
> denali_set_intr_modes(denali, false);
> - free_irq(irqnum, denali);
> }
>
> static void denali_irq_enable(struct denali_nand_info *denali,
> @@ -1456,8 +1455,8 @@ int denali_init(struct denali_nand_info *denali)
> * denali_isr register is done after all the hardware
> * initilization is finished
> */
> - if (request_irq(denali->irq, denali_isr, IRQF_SHARED,
> - DENALI_NAND_NAME, denali)) {
> + if (devm_request_irq(denali->dev, denali->irq, denali_isr, IRQF_SHARED,
> + DENALI_NAND_NAME, denali)) {
> dev_err(denali->dev, "Unable to request IRQ\n");
> return -ENODEV;
> }
>
--
Best regards,
Marek Vasut
On 11/09/2016 05:35 AM, Masahiro Yamada wrote:
> Such debug lines might be useful when debugging the driver first,
> but should be deleted from the upstream code.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Marek Vasut <[email protected]>
> ---
>
> drivers/mtd/nand/denali.c | 12 ------------
> 1 file changed, 12 deletions(-)
>
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 14e66ab..e32e13c 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -181,9 +181,6 @@ static uint16_t denali_nand_reset(struct denali_nand_info *denali)
> {
> int i;
>
> - dev_dbg(denali->dev, "%s, Line %d, Function: %s\n",
> - __FILE__, __LINE__, __func__);
> -
> for (i = 0; i < denali->max_banks; i++)
> iowrite32(INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT,
> denali->flash_reg + INTR_STATUS(i));
> @@ -233,9 +230,6 @@ static void nand_onfi_timing_set(struct denali_nand_info *denali,
> uint16_t acc_clks;
> uint16_t addr_2_data, re_2_we, re_2_re, we_2_re, cs_cnt;
>
> - dev_dbg(denali->dev, "%s, Line %d, Function: %s\n",
> - __FILE__, __LINE__, __func__);
> -
> en_lo = CEIL_DIV(Trp[mode], CLK_X);
> en_hi = CEIL_DIV(Treh[mode], CLK_X);
> #if ONFI_BLOOM_TIME
> @@ -480,9 +474,6 @@ static uint16_t denali_nand_timing_set(struct denali_nand_info *denali)
> uint8_t maf_id, device_id;
> int i;
>
> - dev_dbg(denali->dev, "%s, Line %d, Function: %s\n",
> - __FILE__, __LINE__, __func__);
> -
> /*
> * Use read id method to get device ID and other params.
> * For some NAND chips, controller can't report the correct
> @@ -537,9 +528,6 @@ static uint16_t denali_nand_timing_set(struct denali_nand_info *denali)
> static void denali_set_intr_modes(struct denali_nand_info *denali,
> uint16_t INT_ENABLE)
> {
> - dev_dbg(denali->dev, "%s, Line %d, Function: %s\n",
> - __FILE__, __LINE__, __func__);
> -
> if (INT_ENABLE)
> iowrite32(1, denali->flash_reg + GLOBAL_INT_ENABLE);
> else
>
--
Best regards,
Marek Vasut
On 11/09/2016 05:35 AM, Masahiro Yamada wrote:
> The nand_scan_ident/tail() returns an appropriate error value when
> it fails. Use it instead of the fixed -ENXIO.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Marek Vasut <[email protected]>
> ---
>
> drivers/mtd/nand/denali.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index f188a48..d482d8d 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -1474,10 +1474,9 @@ int denali_init(struct denali_nand_info *denali)
> * this is the first stage in a two step process to register
> * with the nand subsystem
> */
> - if (nand_scan_ident(mtd, denali->max_banks, NULL)) {
> - ret = -ENXIO;
> + ret = nand_scan_ident(mtd, denali->max_banks, NULL);
> + if (ret)
> goto failed_req_irq;
> - }
>
> /* allocate the right size buffer now */
> devm_kfree(denali->dev, denali->buf.buf);
> @@ -1580,10 +1579,9 @@ int denali_init(struct denali_nand_info *denali)
> denali->nand.ecc.write_oob = denali_write_oob;
> denali->nand.erase = denali_erase;
>
> - if (nand_scan_tail(mtd)) {
> - ret = -ENXIO;
> + ret = nand_scan_tail(mtd);
> + if (ret)
> goto failed_req_irq;
> - }
>
> ret = mtd_device_register(mtd, NULL, 0);
> if (ret) {
>
--
Best regards,
Marek Vasut
On 11/09/2016 05:35 AM, Masahiro Yamada wrote:
> I am tackling on this driver to use it for my SoCs.
> The difficulty is a bunch of platform specific stuff
> (more specifically, Intel MRST specific) is hard-coded in this driver.
>
> I need lots of rework to utilize the driver for generic cases,
> but at the same time, I found the driver code is really dirty,
> lots of unused code, odd comments, etc.
>
> The first thing I needed to do was to clean up the code.
> My work is still under the way, but I decided to drop this series
> for now. I hope this series is easy to review, so I guess
> splitting into a small chunks is better than a one-shot patch bomb.
Thanks ! except for minor nits, the series looks great!
--
Best regards,
Marek Vasut
On 11/09/2016 05:35 AM, Masahiro Yamada wrote:
> Remove parentheses surrounding the whole right side of an assignment.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Marek Vasut <[email protected]>
> ---
>
> drivers/mtd/nand/denali.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index d482d8d..14e66ab 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -1510,16 +1510,16 @@ int denali_init(struct denali_nand_info *denali)
> * the real pagesize and anything necessery
> */
> denali->devnum = ioread32(denali->flash_reg + DEVICES_CONNECTED);
> - denali->nand.chipsize <<= (denali->devnum - 1);
> - denali->nand.page_shift += (denali->devnum - 1);
> + denali->nand.chipsize <<= denali->devnum - 1;
> + denali->nand.page_shift += denali->devnum - 1;
> denali->nand.pagemask = (denali->nand.chipsize >>
> denali->nand.page_shift) - 1;
> - denali->nand.bbt_erase_shift += (denali->devnum - 1);
> + denali->nand.bbt_erase_shift += denali->devnum - 1;
> denali->nand.phys_erase_shift = denali->nand.bbt_erase_shift;
> - denali->nand.chip_shift += (denali->devnum - 1);
> - mtd->writesize <<= (denali->devnum - 1);
> - mtd->oobsize <<= (denali->devnum - 1);
> - mtd->erasesize <<= (denali->devnum - 1);
> + denali->nand.chip_shift += denali->devnum - 1;
> + mtd->writesize <<= denali->devnum - 1;
> + mtd->oobsize <<= denali->devnum - 1;
> + mtd->erasesize <<= denali->devnum - 1;
I won't claim I completely understand what this code does, but it
certainly does raise some eyebrows.
> mtd->size = denali->nand.numchips * denali->nand.chipsize;
> denali->bbtskipbytes *= denali->devnum;
>
>
--
Best regards,
Marek Vasut
On 11/09/2016 05:35 AM, Masahiro Yamada wrote:
> The devm_request_irq() returns an appropriate error value when it
> fails. Use it instead of the fixed -ENODEV.
>
> While we are here, reword the comment to make it fit in a single
> line, fixing the misspelling of "initialization".
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---
>
> drivers/mtd/nand/denali.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 44e075a..f188a48 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -1451,14 +1451,12 @@ int denali_init(struct denali_nand_info *denali)
> denali_hw_init(denali);
> denali_drv_init(denali);
>
> - /*
> - * denali_isr register is done after all the hardware
> - * initilization is finished
> - */
> - if (devm_request_irq(denali->dev, denali->irq, denali_isr, IRQF_SHARED,
> - DENALI_NAND_NAME, denali)) {
> + /* request IRQ after all the hardware initialization is finished */
You can use capital letter to start the sentence -- Request ....
Otherwise:
Reviewed-by: Marek Vasut <[email protected]>
> + ret = devm_request_irq(denali->dev, denali->irq, denali_isr,
> + IRQF_SHARED, DENALI_NAND_NAME, denali);
> + if (ret) {
> dev_err(denali->dev, "Unable to request IRQ\n");
> - return -ENODEV;
> + return ret;
> }
>
> /* now that our ISR is registered, we can enable interrupts */
>
--
Best regards,
Marek Vasut
On 11/09/2016 05:35 AM, Masahiro Yamada wrote:
> As far as I understood from the Kconfig menu deleted by commit
> be7f39c5ecf5 ("Staging: delete spectra driver"), the "Spectra" is
> specific to Intel Moorestown Platform.
>
> The Denali NAND controller IP is used for various SoCs such as
> Altera SOCFPGA, Socionext UniPhier, etc. The platform specific
> strings are not preferred in this driver.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Marek Vasut <[email protected]>
> ---
>
> As an ARM-SoC developer, I only need denali.c and denali_dt.c.
>
> I see some "Spectra:" in drivers/mtd/nand/denali_pci.c as well.
> I was not quite sure if they are needed or not.
> If desired, I can update this patch to remove them too.
Is anyone even using Denali on Intel now ?
> drivers/mtd/nand/denali.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 80d3e26..78d795b 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -402,7 +402,7 @@ static void get_hynix_nand_para(struct denali_nand_info *denali,
> break;
> default:
> dev_warn(denali->dev,
> - "Spectra: Unknown Hynix NAND (Device ID: 0x%x).\n"
> + "Unknown Hynix NAND (Device ID: 0x%x).\n"
> "Will use default parameter values instead.\n",
> device_id);
> }
> @@ -1458,7 +1458,7 @@ int denali_init(struct denali_nand_info *denali)
> */
> if (request_irq(denali->irq, denali_isr, IRQF_SHARED,
> DENALI_NAND_NAME, denali)) {
> - pr_err("Spectra: Unable to allocate IRQ\n");
> + dev_err(denali->dev, "Unable to request IRQ\n");
> return -ENODEV;
> }
>
> @@ -1495,7 +1495,7 @@ int denali_init(struct denali_nand_info *denali)
> /* Is 32-bit DMA supported? */
> ret = dma_set_mask(denali->dev, DMA_BIT_MASK(32));
> if (ret) {
> - pr_err("Spectra: no usable DMA configuration\n");
> + dev_err(denali->dev, "no usable DMA configuration\n");
> goto failed_req_irq;
> }
>
> @@ -1503,7 +1503,7 @@ int denali_init(struct denali_nand_info *denali)
> mtd->writesize + mtd->oobsize,
> DMA_BIDIRECTIONAL);
> if (dma_mapping_error(denali->dev, denali->buf.dma_buf)) {
> - dev_err(denali->dev, "Spectra: failed to map DMA buffer\n");
> + dev_err(denali->dev, "failed to map DMA buffer\n");
Nit: For consistency's sake, use Failed with capital F . Fix the "No
usable DMA ..." too.
> ret = -EIO;
> goto failed_req_irq;
> }
> @@ -1598,8 +1598,7 @@ int denali_init(struct denali_nand_info *denali)
>
> ret = mtd_device_register(mtd, NULL, 0);
> if (ret) {
> - dev_err(denali->dev, "Spectra: Failed to register MTD: %d\n",
> - ret);
> + dev_err(denali->dev, "Failed to register MTD: %d\n", ret);
> goto failed_req_irq;
> }
> return 0;
>
--
Best regards,
Marek Vasut
On 11/09/2016 05:35 AM, Masahiro Yamada wrote:
> The denali->fwblks is set by detect_partition_feature(), but it is
> not referenced from anywhere. That means the struct member fwblks
> and the whole of detect_partition_feature() are unneeded.
>
> The comment block implies this function is only for Intel platforms.
> I found drivers/staging/spectra used to exist, but it was deleted by
> commit be7f39c5ecf5 ("Staging: delete spectra driver") 5 years ago.
>
> So, I guess nobody would need this function any more.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Marek Vasut <[email protected]>
> ---
>
> drivers/mtd/nand/denali.c | 29 -----------------------------
> drivers/mtd/nand/denali.h | 9 ---------
> 2 files changed, 38 deletions(-)
>
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index d6f1b29..80d3e26 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -473,33 +473,6 @@ static void detect_max_banks(struct denali_nand_info *denali)
> denali->max_banks = 1 << (features & FEATURES__N_BANKS);
> }
>
> -static void detect_partition_feature(struct denali_nand_info *denali)
> -{
> - /*
> - * For MRST platform, denali->fwblks represent the
> - * number of blocks firmware is taken,
> - * FW is in protect partition and MTD driver has no
> - * permission to access it. So let driver know how many
> - * blocks it can't touch.
> - */
> - if (ioread32(denali->flash_reg + FEATURES) & FEATURES__PARTITION) {
> - if ((ioread32(denali->flash_reg + PERM_SRC_ID(1)) &
> - PERM_SRC_ID__SRCID) == SPECTRA_PARTITION_ID) {
> - denali->fwblks =
> - ((ioread32(denali->flash_reg + MIN_MAX_BANK(1)) &
> - MIN_MAX_BANK__MIN_VALUE) *
> - denali->blksperchip)
> - +
> - (ioread32(denali->flash_reg + MIN_BLK_ADDR(1)) &
> - MIN_BLK_ADDR__VALUE);
> - } else {
> - denali->fwblks = SPECTRA_START_BLOCK;
> - }
> - } else {
> - denali->fwblks = SPECTRA_START_BLOCK;
> - }
> -}
> -
> static uint16_t denali_nand_timing_set(struct denali_nand_info *denali)
> {
> uint16_t status = PASS;
> @@ -551,8 +524,6 @@ static uint16_t denali_nand_timing_set(struct denali_nand_info *denali)
>
> find_valid_banks(denali);
>
> - detect_partition_feature(denali);
> -
> /*
> * If the user specified to override the default timings
> * with a specific ONFI mode, we apply those changes here.
> diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
> index 0ce7344..7c0800d 100644
> --- a/drivers/mtd/nand/denali.h
> +++ b/drivers/mtd/nand/denali.h
> @@ -383,14 +383,6 @@
> #define CLK_X 5
> #define CLK_MULTI 4
>
> -/* spectraswconfig.h */
> -#define CMD_DMA 0
> -
> -#define SPECTRA_PARTITION_ID 0
> -/**** Block Table and Reserved Block Parameters *****/
> -#define SPECTRA_START_BLOCK 3
> -#define NUM_FREE_BLOCKS_GATE 30
> -
> /* KBV - Updated to LNW scratch register address */
> #define SCRATCH_REG_ADDR CONFIG_MTD_NAND_DENALI_SCRATCH_REG_ADDR
> #define SCRATCH_REG_SIZE 64
> @@ -470,7 +462,6 @@ struct denali_nand_info {
> int irq;
>
> uint32_t devnum; /* represent how many nands connected */
> - uint32_t fwblks; /* represent how many blocks FW used */
> uint32_t totalblks;
> uint32_t blksperchip;
> uint32_t bbtskipbytes;
>
--
Best regards,
Marek Vasut
On 11/09/2016 05:35 AM, Masahiro Yamada wrote:
> The interrupt handler is setup in denali_init(), not in
> denali_drv_init(). This comment is false.
>
> Such a comment adds no value, so just delete it instead of move.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Marek Vasut <[email protected]>
> ---
>
> drivers/mtd/nand/denali.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 51ddb84..d6f1b29 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -1436,7 +1436,6 @@ static int denali_ooblayout_free(struct mtd_info *mtd, int section,
> /* initialize driver data structures */
> static void denali_drv_init(struct denali_nand_info *denali)
> {
> - /* setup interrupt handler */
> /*
> * the completion object will be used to notify
> * the callee that the interrupt is done
>
--
Best regards,
Marek Vasut
On 11/09/2016 05:35 AM, Masahiro Yamada wrote:
> The denali->blksperchip is set, but not referenced any more. The
> denali->totalblks is used only for calculating denali->blksperchip.
> Both of them are unneeded.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Marek Vasut <[email protected]>
> ---
>
> drivers/mtd/nand/denali.c | 8 --------
> drivers/mtd/nand/denali.h | 2 --
> 2 files changed, 10 deletions(-)
>
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 78d795b..548278b 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -1573,14 +1573,6 @@ int denali_init(struct denali_nand_info *denali)
> denali->nand.ecc.bytes *= denali->devnum;
> denali->nand.ecc.strength *= denali->devnum;
>
> - /*
> - * Let driver know the total blocks number and how many blocks
> - * contained by each nand chip. blksperchip will help driver to
> - * know how many blocks is taken by FW.
> - */
> - denali->totalblks = mtd->size >> denali->nand.phys_erase_shift;
> - denali->blksperchip = denali->totalblks / denali->nand.numchips;
> -
> /* override the default read operations */
> denali->nand.ecc.size = ECC_SECTOR_SIZE * denali->devnum;
> denali->nand.ecc.read_page = denali_read_page;
> diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
> index 7c0800d..ea22191 100644
> --- a/drivers/mtd/nand/denali.h
> +++ b/drivers/mtd/nand/denali.h
> @@ -462,8 +462,6 @@ struct denali_nand_info {
> int irq;
>
> uint32_t devnum; /* represent how many nands connected */
> - uint32_t totalblks;
> - uint32_t blksperchip;
> uint32_t bbtskipbytes;
> uint32_t max_banks;
> };
>
--
Best regards,
Marek Vasut
On 11/09/2016 05:35 AM, Masahiro Yamada wrote:
> The driver calls devm_kzalloc()/devm_kfree() to allocate/free memory.
> They are declared in <linux/device.h>, not in <linux/slab.h>.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Marek Vasut <[email protected]>
> ---
>
> drivers/mtd/nand/denali.c | 1 -
> drivers/mtd/nand/denali_dt.c | 1 -
> drivers/mtd/nand/denali_pci.c | 1 -
> 3 files changed, 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 7e2c650..062d5b5 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -21,7 +21,6 @@
> #include <linux/dma-mapping.h>
> #include <linux/wait.h>
> #include <linux/mutex.h>
> -#include <linux/slab.h>
> #include <linux/mtd/mtd.h>
> #include <linux/module.h>
>
> diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
> index f821dc1..5607fcd 100644
> --- a/drivers/mtd/nand/denali_dt.c
> +++ b/drivers/mtd/nand/denali_dt.c
> @@ -21,7 +21,6 @@
> #include <linux/platform_device.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> -#include <linux/slab.h>
>
> #include "denali.h"
>
> diff --git a/drivers/mtd/nand/denali_pci.c b/drivers/mtd/nand/denali_pci.c
> index de31514..ac84323 100644
> --- a/drivers/mtd/nand/denali_pci.c
> +++ b/drivers/mtd/nand/denali_pci.c
> @@ -14,7 +14,6 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/pci.h>
> -#include <linux/slab.h>
>
> #include "denali.h"
>
>
--
Best regards,
Marek Vasut
2016-11-13 6:35 GMT+09:00 Marek Vasut <[email protected]>:
> On 11/09/2016 05:35 AM, Masahiro Yamada wrote:
>> As far as I understood from the Kconfig menu deleted by commit
>> be7f39c5ecf5 ("Staging: delete spectra driver"), the "Spectra" is
>> specific to Intel Moorestown Platform.
>>
>> The Denali NAND controller IP is used for various SoCs such as
>> Altera SOCFPGA, Socionext UniPhier, etc. The platform specific
>> strings are not preferred in this driver.
>>
>> Signed-off-by: Masahiro Yamada <[email protected]>
>
> Reviewed-by: Marek Vasut <[email protected]>
>
>> ---
>>
>> As an ARM-SoC developer, I only need denali.c and denali_dt.c.
>>
>> I see some "Spectra:" in drivers/mtd/nand/denali_pci.c as well.
>> I was not quite sure if they are needed or not.
>> If desired, I can update this patch to remove them too.
>
> Is anyone even using Denali on Intel now ?
I assume this question is address to Intel guys, not me.
May I drop the "Spectra:" from printk strings entirely?
--
Best Regards
Masahiro Yamada
Hi Marek,
2016-11-13 6:35 GMT+09:00 Marek Vasut <[email protected]>:
> On 11/09/2016 05:35 AM, Masahiro Yamada wrote:
>> As far as I understood from the Kconfig menu deleted by commit
>> be7f39c5ecf5 ("Staging: delete spectra driver"), the "Spectra" is
>> specific to Intel Moorestown Platform.
>>
>> The Denali NAND controller IP is used for various SoCs such as
>> Altera SOCFPGA, Socionext UniPhier, etc. The platform specific
>> strings are not preferred in this driver.
>>
>> Signed-off-by: Masahiro Yamada <[email protected]>
>
> Reviewed-by: Marek Vasut <[email protected]>
>
>> ---
>>
>> As an ARM-SoC developer, I only need denali.c and denali_dt.c.
>>
>> I see some "Spectra:" in drivers/mtd/nand/denali_pci.c as well.
>> I was not quite sure if they are needed or not.
>> If desired, I can update this patch to remove them too.
>
> Is anyone even using Denali on Intel now ?
>
>> drivers/mtd/nand/denali.c | 11 +++++------
>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>> index 80d3e26..78d795b 100644
>> --- a/drivers/mtd/nand/denali.c
>> +++ b/drivers/mtd/nand/denali.c
>> @@ -402,7 +402,7 @@ static void get_hynix_nand_para(struct denali_nand_info *denali,
>> break;
>> default:
>> dev_warn(denali->dev,
>> - "Spectra: Unknown Hynix NAND (Device ID: 0x%x).\n"
>> + "Unknown Hynix NAND (Device ID: 0x%x).\n"
>> "Will use default parameter values instead.\n",
>> device_id);
>> }
>> @@ -1458,7 +1458,7 @@ int denali_init(struct denali_nand_info *denali)
>> */
>> if (request_irq(denali->irq, denali_isr, IRQF_SHARED,
>> DENALI_NAND_NAME, denali)) {
>> - pr_err("Spectra: Unable to allocate IRQ\n");
>> + dev_err(denali->dev, "Unable to request IRQ\n");
>> return -ENODEV;
>> }
>>
>> @@ -1495,7 +1495,7 @@ int denali_init(struct denali_nand_info *denali)
>> /* Is 32-bit DMA supported? */
>> ret = dma_set_mask(denali->dev, DMA_BIT_MASK(32));
>> if (ret) {
>> - pr_err("Spectra: no usable DMA configuration\n");
>> + dev_err(denali->dev, "no usable DMA configuration\n");
>> goto failed_req_irq;
>> }
>>
>> @@ -1503,7 +1503,7 @@ int denali_init(struct denali_nand_info *denali)
>> mtd->writesize + mtd->oobsize,
>> DMA_BIDIRECTIONAL);
>> if (dma_mapping_error(denali->dev, denali->buf.dma_buf)) {
>> - dev_err(denali->dev, "Spectra: failed to map DMA buffer\n");
>> + dev_err(denali->dev, "failed to map DMA buffer\n");
>
> Nit: For consistency's sake, use Failed with capital F . Fix the "No
> usable DMA ..." too.
Even if we fix those two, we still have some more printk strings
that start with a lower case.
line 177: dev_err(denali->dev, "reset bank failed.\n");
line 699: pr_err("timeout occurred, status = 0x%x, mask = 0x%x\n",
line 863: dev_err(denali->dev, "unable to send pipeline command\n");
line 1074: dev_err(denali->dev, "timeout on write_page (type = %d)\n",
line 1309: pr_err(": unsupported command received 0x%x\n", cmd);
If you say "consistency's sake" and
you are a big fan of capital letters instead of lower cases,
will you send a patch that touches those globally?
Your comments against this series are just about
"upper cases vs lower cases".
If I get more useful comments, I am happy to send v2.
But, at this moment, I see no good reason for v2
because changing those two lines does not give us any consistency.
--
Best Regards
Masahiro Yamada
On 11/18/2016 09:42 AM, Masahiro Yamada wrote:
> Hi Marek,
>
>
> 2016-11-13 6:35 GMT+09:00 Marek Vasut <[email protected]>:
>> On 11/09/2016 05:35 AM, Masahiro Yamada wrote:
>>> As far as I understood from the Kconfig menu deleted by commit
>>> be7f39c5ecf5 ("Staging: delete spectra driver"), the "Spectra" is
>>> specific to Intel Moorestown Platform.
>>>
>>> The Denali NAND controller IP is used for various SoCs such as
>>> Altera SOCFPGA, Socionext UniPhier, etc. The platform specific
>>> strings are not preferred in this driver.
>>>
>>> Signed-off-by: Masahiro Yamada <[email protected]>
>>
>> Reviewed-by: Marek Vasut <[email protected]>
>>
>>> ---
>>>
>>> As an ARM-SoC developer, I only need denali.c and denali_dt.c.
>>>
>>> I see some "Spectra:" in drivers/mtd/nand/denali_pci.c as well.
>>> I was not quite sure if they are needed or not.
>>> If desired, I can update this patch to remove them too.
>>
>> Is anyone even using Denali on Intel now ?
>>
>>> drivers/mtd/nand/denali.c | 11 +++++------
>>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>> index 80d3e26..78d795b 100644
>>> --- a/drivers/mtd/nand/denali.c
>>> +++ b/drivers/mtd/nand/denali.c
>>> @@ -402,7 +402,7 @@ static void get_hynix_nand_para(struct denali_nand_info *denali,
>>> break;
>>> default:
>>> dev_warn(denali->dev,
>>> - "Spectra: Unknown Hynix NAND (Device ID: 0x%x).\n"
>>> + "Unknown Hynix NAND (Device ID: 0x%x).\n"
>>> "Will use default parameter values instead.\n",
>>> device_id);
>>> }
>>> @@ -1458,7 +1458,7 @@ int denali_init(struct denali_nand_info *denali)
>>> */
>>> if (request_irq(denali->irq, denali_isr, IRQF_SHARED,
>>> DENALI_NAND_NAME, denali)) {
>>> - pr_err("Spectra: Unable to allocate IRQ\n");
>>> + dev_err(denali->dev, "Unable to request IRQ\n");
>>> return -ENODEV;
>>> }
>>>
>>> @@ -1495,7 +1495,7 @@ int denali_init(struct denali_nand_info *denali)
>>> /* Is 32-bit DMA supported? */
>>> ret = dma_set_mask(denali->dev, DMA_BIT_MASK(32));
>>> if (ret) {
>>> - pr_err("Spectra: no usable DMA configuration\n");
>>> + dev_err(denali->dev, "no usable DMA configuration\n");
>>> goto failed_req_irq;
>>> }
>>>
>>> @@ -1503,7 +1503,7 @@ int denali_init(struct denali_nand_info *denali)
>>> mtd->writesize + mtd->oobsize,
>>> DMA_BIDIRECTIONAL);
>>> if (dma_mapping_error(denali->dev, denali->buf.dma_buf)) {
>>> - dev_err(denali->dev, "Spectra: failed to map DMA buffer\n");
>>> + dev_err(denali->dev, "failed to map DMA buffer\n");
>>
>> Nit: For consistency's sake, use Failed with capital F . Fix the "No
>> usable DMA ..." too.
>
>
> Even if we fix those two, we still have some more printk strings
> that start with a lower case.
>
>
> line 177: dev_err(denali->dev, "reset bank failed.\n");
>
> line 699: pr_err("timeout occurred, status = 0x%x, mask = 0x%x\n",
>
> line 863: dev_err(denali->dev, "unable to send pipeline command\n");
>
> line 1074: dev_err(denali->dev, "timeout on write_page (type = %d)\n",
>
> line 1309: pr_err(": unsupported command received 0x%x\n", cmd);
>
>
>
> If you say "consistency's sake" and
> you are a big fan of capital letters instead of lower cases,
> will you send a patch that touches those globally?
It's not really _that_ important.
> Your comments against this series are just about
> "upper cases vs lower cases".
>
> If I get more useful comments, I am happy to send v2.
>
> But, at this moment, I see no good reason for v2
> because changing those two lines does not give us any consistency.
Well we have to start somewhere, but I can fix those two when applying.
--
Best regards,
Marek Vasut
On Fri, 18 Nov 2016 17:42:55 +0900
Masahiro Yamada <[email protected]> wrote:
> Hi Marek,
>
>
> 2016-11-13 6:35 GMT+09:00 Marek Vasut <[email protected]>:
> > On 11/09/2016 05:35 AM, Masahiro Yamada wrote:
> >> As far as I understood from the Kconfig menu deleted by commit
> >> be7f39c5ecf5 ("Staging: delete spectra driver"), the "Spectra" is
> >> specific to Intel Moorestown Platform.
> >>
> >> The Denali NAND controller IP is used for various SoCs such as
> >> Altera SOCFPGA, Socionext UniPhier, etc. The platform specific
> >> strings are not preferred in this driver.
> >>
> >> Signed-off-by: Masahiro Yamada <[email protected]>
> >
> > Reviewed-by: Marek Vasut <[email protected]>
> >
> >> ---
> >>
> >> As an ARM-SoC developer, I only need denali.c and denali_dt.c.
> >>
> >> I see some "Spectra:" in drivers/mtd/nand/denali_pci.c as well.
> >> I was not quite sure if they are needed or not.
> >> If desired, I can update this patch to remove them too.
> >
> > Is anyone even using Denali on Intel now ?
> >
> >> drivers/mtd/nand/denali.c | 11 +++++------
> >> 1 file changed, 5 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> >> index 80d3e26..78d795b 100644
> >> --- a/drivers/mtd/nand/denali.c
> >> +++ b/drivers/mtd/nand/denali.c
> >> @@ -402,7 +402,7 @@ static void get_hynix_nand_para(struct denali_nand_info *denali,
> >> break;
> >> default:
> >> dev_warn(denali->dev,
> >> - "Spectra: Unknown Hynix NAND (Device ID: 0x%x).\n"
> >> + "Unknown Hynix NAND (Device ID: 0x%x).\n"
> >> "Will use default parameter values instead.\n",
> >> device_id);
> >> }
> >> @@ -1458,7 +1458,7 @@ int denali_init(struct denali_nand_info *denali)
> >> */
> >> if (request_irq(denali->irq, denali_isr, IRQF_SHARED,
> >> DENALI_NAND_NAME, denali)) {
> >> - pr_err("Spectra: Unable to allocate IRQ\n");
> >> + dev_err(denali->dev, "Unable to request IRQ\n");
> >> return -ENODEV;
> >> }
> >>
> >> @@ -1495,7 +1495,7 @@ int denali_init(struct denali_nand_info *denali)
> >> /* Is 32-bit DMA supported? */
> >> ret = dma_set_mask(denali->dev, DMA_BIT_MASK(32));
> >> if (ret) {
> >> - pr_err("Spectra: no usable DMA configuration\n");
> >> + dev_err(denali->dev, "no usable DMA configuration\n");
> >> goto failed_req_irq;
> >> }
> >>
> >> @@ -1503,7 +1503,7 @@ int denali_init(struct denali_nand_info *denali)
> >> mtd->writesize + mtd->oobsize,
> >> DMA_BIDIRECTIONAL);
> >> if (dma_mapping_error(denali->dev, denali->buf.dma_buf)) {
> >> - dev_err(denali->dev, "Spectra: failed to map DMA buffer\n");
> >> + dev_err(denali->dev, "failed to map DMA buffer\n");
> >
> > Nit: For consistency's sake, use Failed with capital F . Fix the "No
> > usable DMA ..." too.
>
>
> Even if we fix those two, we still have some more printk strings
> that start with a lower case.
>
>
> line 177: dev_err(denali->dev, "reset bank failed.\n");
>
> line 699: pr_err("timeout occurred, status = 0x%x, mask = 0x%x\n",
>
> line 863: dev_err(denali->dev, "unable to send pipeline command\n");
>
> line 1074: dev_err(denali->dev, "timeout on write_page (type = %d)\n",
>
> line 1309: pr_err(": unsupported command received 0x%x\n", cmd);
>
>
>
> If you say "consistency's sake" and
> you are a big fan of capital letters instead of lower cases,
> will you send a patch that touches those globally?
>
>
> Your comments against this series are just about
> "upper cases vs lower cases".
It's not like your series was doing useful things either ;-), all I see
is a bunch of cleanup and cosmetic changes. I'm perfectly fine taking
the series, but Marek comments about 'Upper case vs Lower case' is
perfectly valid in regards to this kind of changes.
>
> If I get more useful comments, I am happy to send v2.
>
> But, at this moment, I see no good reason for v2
> because changing those two lines does not give us any consistency.
>
>
Hi Masahiro,
On Wed, 9 Nov 2016 13:35:19 +0900
Masahiro Yamada <[email protected]> wrote:
> I am tackling on this driver to use it for my SoCs.
> The difficulty is a bunch of platform specific stuff
> (more specifically, Intel MRST specific) is hard-coded in this driver.
>
> I need lots of rework to utilize the driver for generic cases,
> but at the same time, I found the driver code is really dirty,
> lots of unused code, odd comments, etc.
>
> The first thing I needed to do was to clean up the code.
> My work is still under the way, but I decided to drop this series
> for now. I hope this series is easy to review, so I guess
> splitting into a small chunks is better than a one-shot patch bomb.
Well, all I've seen coming from you so far are minor cleanups and
cosmetic changes (including one introducing a regression).
I'll apply this series, but please, next time, try to send the whole
series, so that we can see the big picture, and not just random
cleanups.
Thanks,
Boris
>
>
>
> Masahiro Yamada (11):
> mtd: nand: denali: remove unneeded <linux/slab.h> includes
> mtd: nand: denali: remove unused struct member denali_nand_info::idx
> mtd: nand: denali: remove bogus comment about interrupt handler setup
> mtd: nand: denali: remove detect_partition_feature()
> mtd: nand: denali: remove "Spectra:" prefix from printk strings
> mtd: nand: denali: remove unused struct member totalblks, blksperchip
> mtd: nand: denali: use managed devm_irq_request()
> mtd: nand: denali: return error code from devm_request_irq() on error
> mtd: nand: denali: return error code from nand_scan_ident/tail on
> error
> mtd: nand: denali: remove unneeded parentheses
> mtd: nand: denali: remove debug lines of __FILE__, __LINE__, __func__
>
> drivers/mtd/nand/denali.c | 101 +++++++++---------------------------------
> drivers/mtd/nand/denali.h | 12 -----
> drivers/mtd/nand/denali_dt.c | 1 -
> drivers/mtd/nand/denali_pci.c | 1 -
> 4 files changed, 21 insertions(+), 94 deletions(-)
>
Hi Boris,
2016-11-19 17:30 GMT+09:00 Boris Brezillon <[email protected]>:
> Hi Masahiro,
>
> On Wed, 9 Nov 2016 13:35:19 +0900
> Masahiro Yamada <[email protected]> wrote:
>
>> I am tackling on this driver to use it for my SoCs.
>> The difficulty is a bunch of platform specific stuff
>> (more specifically, Intel MRST specific) is hard-coded in this driver.
>>
>> I need lots of rework to utilize the driver for generic cases,
>> but at the same time, I found the driver code is really dirty,
>> lots of unused code, odd comments, etc.
>>
>> The first thing I needed to do was to clean up the code.
>> My work is still under the way, but I decided to drop this series
>> for now. I hope this series is easy to review, so I guess
>> splitting into a small chunks is better than a one-shot patch bomb.
>
> Well, all I've seen coming from you so far are minor cleanups and
> cosmetic changes (including one introducing a regression).
> I'll apply this series, but please, next time, try to send the whole
> series, so that we can see the big picture, and not just random
> cleanups.
>
> Thanks,
>
> Boris
Right, this series alone is not useful at all.
I've been mostly stuck in some local works this week,
but hopefully I will find some time to complete the series next week.
If you want to see the big picture,
you can postpone this series until then.
--
Best Regards
Masahiro Yamada
Hi Masahiro,
On Sun, 20 Nov 2016 01:15:05 +0900
Masahiro Yamada <[email protected]> wrote:
> Hi Boris,
>
>
> 2016-11-19 17:30 GMT+09:00 Boris Brezillon <[email protected]>:
> > Hi Masahiro,
> >
> > On Wed, 9 Nov 2016 13:35:19 +0900
> > Masahiro Yamada <[email protected]> wrote:
> >
> >> I am tackling on this driver to use it for my SoCs.
> >> The difficulty is a bunch of platform specific stuff
> >> (more specifically, Intel MRST specific) is hard-coded in this driver.
> >>
> >> I need lots of rework to utilize the driver for generic cases,
> >> but at the same time, I found the driver code is really dirty,
> >> lots of unused code, odd comments, etc.
> >>
> >> The first thing I needed to do was to clean up the code.
> >> My work is still under the way, but I decided to drop this series
> >> for now. I hope this series is easy to review, so I guess
> >> splitting into a small chunks is better than a one-shot patch bomb.
> >
> > Well, all I've seen coming from you so far are minor cleanups and
> > cosmetic changes (including one introducing a regression).
> > I'll apply this series, but please, next time, try to send the whole
> > series, so that we can see the big picture, and not just random
> > cleanups.
> >
> > Thanks,
> >
> > Boris
>
> Right, this series alone is not useful at all.
>
> I've been mostly stuck in some local works this week,
> but hopefully I will find some time to complete the series next week.
>
> If you want to see the big picture,
> you can postpone this series until then.
>
I already applied the series. Looking forward to the next steps ;-).
Sorry if I've been harsh to you when saying that your contributions
were 'useless'. It's just that we regularly receive random 'cleanups'
(generated with coccinelle, or similar tools), and, while these kind of
things help getting consistent code across the kernel, or
simplifying/clarifying existing code, it's not the kind of rework that
really improve the framework or address drivers flaws.
It's also sometime used by developers who just want to have patches in
mainline, and never reach the next step (contribute things to really
improve a framework or add support for new features/hardware).
Another reason I'm reluctant to apply such cleanup changes is that,
sometime a simple change might actually break things (see the 'mtd:
nand: denali_pci: add missing pci_release_regions() calls' patch).
And the last aspect is that cosmetic changes pollutes other
contributions by delaying their review.
Don't mistake me, if all these cleanups are serving a higher purpose (in
your case, adding support for a new IP revision in a clean way), then
that's all fine. But otherwise, I tend to dequeue those patches last.
Regards,
Boris