Re: [PATCH v3 1/3] powerpc/512x: add LocalPlus Bus FIFO device driver

Hello Timur, thanks again for your review.

On 25.09.2015 04:01, Timur Tabi wrote:

Alexander Popov wrote:

quoted

+
+    for (i = 0; i < lpbfifo.cs_n; i++) {
+        phys_addr_t cs_start;
+        phys_addr_t cs_end;
+
+        cs_start = lpbfifo.cs_ranges[i].addr;
+        cs_end = cs_start + lpbfifo.cs_ranges[i].size - 1;
+
+        if (lpbfifo.req->bus_phys >= cs_start &&
+            lpbfifo.req->bus_phys + lpbfifo.req->size - 1 <= cs_end) {
+            cs = lpbfifo.cs_ranges[i].csnum;
+            break;
+        }
+    }
+    if (i == lpbfifo.cs_n) {

Can you test for "!cs" here instead?

quoted

+        e = -EFAULT;
+        goto err_param;
+    }

Unfortunately no: 0 is a valid value for Chip Select.
Is it OK to leave it like that?

quoted

+
+    /* 2. Prepare DMA */
+    dma_dev = lpbfifo.chan->device;
+
+    sg_init_table(&sg, 1);
+    if (lpbfifo.req->dir == MPC512X_LPBFIFO_REQ_DIR_WRITE)
+        dir = DMA_TO_DEVICE;
+    else
+        dir = DMA_FROM_DEVICE;
+    sg_dma_address(&sg) = dma_map_single(dma_dev->dev,
+            lpbfifo.req->ram_virt, lpbfifo.req->size, dir);
+    if (dma_mapping_error(dma_dev->dev, sg_dma_address(&sg))) {
+        e = -EFAULT;
+        goto err_param;
+    }
+    lpbfifo.ram_bus_addr = sg_dma_address(&sg); /* For freeing later */
+    sg_dma_len(&sg) = lpbfifo.req->size;

I don't think sg_dma_len() is meant to be used as an lvalue.

I've double-checked and found many cases of such usage of this macro.
It seems that I can't avoid it too.

quoted

+    /*
+     * The node defined as compatible with 'fsl,mpc5121-localbus'
+     * should have 2 address cells and 1 size cell.
+     * One item of its ranges property should consist of:
+     * - the first address cell which is the chipselect number;
+     * - the second address cell which is the offset in the chipselect,
+     *    must be zero.
+     * - CPU address of the beginning of an access window;
+     * - the only size cell which is the size of an access window.
+     */
+    addr_cells_p = of_get_property(lb_node, "#address-cells", NULL);
+    size_cells_p = of_get_property(lb_node, "#size-cells", NULL);
+    if (addr_cells_p == NULL || *addr_cells_p != 2 ||
+                size_cells_p == NULL ||    *size_cells_p != 1) {
+        goto end1;
+    }

Is this really necessary?

I'm not sure.

I've found a device tree node compatible with "fsl,mpc5121-localbus"
and I just check the format of "ranges" property before parsing it
because devicetree/bindings/powerpc/fsl/lbc.txt doesn't have details
about it.

Should I blindly assume that this property has 2 address cells and 1 size
cell?

Can't you use the built-in OF functions for
parsing ranges? 

I don't see anything to use instead of of_property_count_u32_elems() and
of_property_read_u32_array().

Driver code that has to parse #address-cells or #size-cells
is usually wrong.

I would not call it "parsing", I just check whether the dts-file is good.
Anyway, could you give me a clue how to do better?

quoted

+
+    proplen = of_property_count_u32_elems(lb_node, "ranges");
+    if (proplen < 0 || proplen % 4 != 0)
+        goto end1;
+
+    lpbfifo.cs_n = proplen / 4;
+    lpbfifo.cs_ranges = kcalloc(lpbfifo.cs_n, sizeof(struct cs_range),
+                                GFP_KERNEL);
+    if (!lpbfifo.cs_ranges)
+        goto end1;
+
+    if (of_property_read_u32_array(lb_node, "ranges",
+                (u32 *)lpbfifo.cs_ranges, proplen) != 0) {
+        goto end2;
+    }
+
+    for (i = 0; i < lpbfifo.cs_n; i++) {
+        if (lpbfifo.cs_ranges[i].base != 0)
+            goto end2;
+    }
+
+    res = 0;
+ end2:
+    if (res != 0)
+        kfree(lpbfifo.cs_ranges);
+ end1:
+    of_node_put(lb_node);
+ end0:
+    return res;
+}

Best regards,
Alexander