Hi Andy,
On 08.06.2018 22:27, Andy Shevchenko wrote:
On Fri, Jun 8, 2018 at 2:54 PM, Yogesh Narayan Gaur
<yogeshnarayan.gaur@xxxxxxx> wrote:
Hi Frieder,
+#define QUADSPI_MCR_RESERVED_MASK (0xF << 16)
GENMASK()
Ok.
+#define QUADSPI_MCR_END_CFG_MASK (0x3 << 2)
+#define QUADSPI_BUF3CR_ADATSZ_MASK (0xFF << QUADSPI_BUF3CR_ADATSZ_SHIFT)
+#define QUADSPI_SMPR_DDRSMP_MASK (7 << 16)
+#define QUADSPI_RBCT_WMRK_MASK 0x1F
Ditto.
Ok.
+#define QUADSPI_LUT_OFFSET (SEQID_LUT * 4 * 4)
+#define QUADSPI_LUT_REG(idx) (QUADSPI_LUT_BASE + \
+ QUADSPI_LUT_OFFSET + (idx) * 4)
It looks slightly better when
#define FOO \
(BAR1 + BAR2 ...)
Ok.
+/*
+ * An IC bug makes it necessary to rearrange the 32-bit data.
+ * Later chips, such as IMX6SLX, have fixed this bug.
+ */
+static inline u32 fsl_qspi_endian_xchg(struct fsl_qspi *q, u32 a) {
+ return needs_swap_endian(q) ? __swab32(a) : a; }
func()
{
...
}
Fix this everywhere.
I will fix this.
+static void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
+*addr) {
+ if (q->big_endian)
+ iowrite32be(val, addr);
+ else
+ iowrite32(val, addr);
+}
+
+static u32 qspi_readl(struct fsl_qspi *q, void __iomem *addr) {
+ if (q->big_endian)
+ return ioread32be(addr);
+ else
+ return ioread32(addr);
+}
Better to define ->read() and ->write() callbacks and call them unconditionally.
Ok.
+static int fsl_qspi_check_buswidth(struct fsl_qspi *q, u8 width) {
+ switch (width) {
+ case 1:
+ case 2:
+ case 4:
+ return 0;
+ }
if (!is_power_of_2(width) || width >= 8)
return -E...;
return 0;
?
Your proposition is a bit shorter, but I think it's slightly harder to read.
+
+ return -ENOTSUPP;
+}
+static int fsl_qspi_clk_prep_enable(struct fsl_qspi *q) {
+ int ret;
+
+ ret = clk_prepare_enable(q->clk_en);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(q->clk);
+ if (ret) {
+ clk_disable_unprepare(q->clk_en);
Is it needed here?
No, this is probably superfluous. I will remove it.
+ return ret;
+ }
+
+ if (needs_wakeup_wait_mode(q))
+ pm_qos_add_request(&q->pm_qos_req, PM_QOS_CPU_DMA_LATENCY, 0);
+
+ return 0;
+}
+ qspi_writel(q, map_addr, q->iobase + QUADSPI_SFA1AD + (i * 4));
Redundant parens.
Ok, I thought this is better for readability.
+ seq = seq ? 0 : 1;
seq = (seq + 1) % 2;
?
Ok.
+}
+ for (i = 0; i < op->data.nbytes; i += 4) {
+ u32 val = 0;
+
+ memcpy(&val, op->data.buf.out + i,
+ min_t(unsigned int, op->data.nbytes - i, 4));
You may easily avoid this conditional on each iteration.
Do you mean something like this, or are there better ways?
u32 val = 0;
for (i = 0; i < ALIGN_DOWN(op->data.nbytes, 4); i += 4)
{
memcpy(&val, op->data.buf.out + i, 4);
val = fsl_qspi_endian_xchg(q, val);
qspi_writel(q, val, base + QUADSPI_TBDR);
}
memcpy(&val, op->data.buf.out + i, op->data.nbytes);
val = fsl_qspi_endian_xchg(q, val);
qspi_writel(q, val, base + QUADSPI_TBDR);
+
+ val = fsl_qspi_endian_xchg(q, val);
+ qspi_writel(q, val, base + QUADSPI_TBDR);
+ }
+ /* wait for the controller being ready */
FOREVER! See below.
+ do {
+ u32 status;
+
+ status = qspi_readl(q, base + QUADSPI_SR);
+ if (status &
+ (QUADSPI_SR_IP_ACC_MASK | QUADSPI_SR_AHB_ACC_MASK)) {
+ udelay(1);
+ dev_dbg(q->dev, "The controller is busy, 0x%x\n",
+ status);
+ continue;
+ }
+ break;
+ } while (1);
Please, avoid infinite loops.
unsigned int count = 100;
...
do {
...
} while (--count);
Ok, will change that.
+ if (of_get_available_child_count(q->dev->of_node) == 1)
+ name = dev_name(q->dev);
+ else
+ name = devm_kasprintf(dev, GFP_KERNEL,
+ "%s-%d", dev_name(q->dev),
+ mem->spi->chip_select);
+
+ if (!name) {
+ dev_err(dev, "failed to get memory for custom flash name\n");
+ return dev_name(q->dev);
Might it be racy if in between device gets a name assigned?
Ok, I will change that to make sure that dev_name() is only called once.
+ }
+ if (q->ahb_addr)
+ iounmap(q->ahb_addr);
Double unmap?
Right, this is redundant. I will remove it.
+static struct platform_driver fsl_qspi_driver = {
+ .driver = {
+ .name = "fsl-quadspi",
+ .of_match_table = fsl_qspi_dt_ids,
+ },
+ .probe = fsl_qspi_probe,
+ .remove = fsl_qspi_remove,
+ .suspend = fsl_qspi_suspend,
+ .resume = fsl_qspi_resume,
Why not in struct dev_pm_ops?
This was taken from the original driver. I will add a struct dev_pm_ops
to hold fsl_qspi_suspend() and fsl_qspi_resume().
+};
+MODULE_AUTHOR("Freescale Semiconductor Inc."); MODULE_AUTHOR("Boris
+Brezillion <boris.brezillon@xxxxxxxxxxx>"); MODULE_AUTHOR("Frieder
+Schrempf <frieder.schrempf@xxxxxxxxx>"); MODULE_LICENSE("GPL v2");
Wrong indentation.
What is wrong? Some newlines are missing here between the MODULE_
macros, but in my original patch it seems correct.
Thank you for your review,
Frieder
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
