Re: [PATCH] powerpc/512x: add function for CS parameter configuration

On Fri, 1 Feb 2013 22:31:51 -0600
Timur Tabi [off-list ref] wrote:

On Fri, Feb 1, 2013 at 7:28 AM, Anatolij Gustschin [off-list ref] wrote:

quoted

Add ability to configure CS parameters for devices that need
different CS parameters setup after their configuration. I.e.
an FPGA device on LP bus can require different CS parameters
for its bus interface after loading firmware into it. A driver
can easily reconfigure the LPC CS parameters using this function.

Could you define "CS" somewhere?

I'll define it in revised commit log in v2.

quoted

+struct mpc512x_lpc {
+       u32     cs_cfg[8];      /* CS config */
+       u32     cs_ctrl;        /* CS Control Register */
+       u32     cs_status;      /* CS Status Register */
+       u32     burst_ctrl;     /* CS Burst Control Register */
+       u32     deadcycle_ctrl; /* CS Deadcycle Control Register */
+       u32     holdcycle_ctrl; /* CS Holdcycle Control Register */
+       u32     alt;            /* Address Latch Timing Register */
+};

These should be __be32.

Why? To add a new bunch of sparse warnings?

...

quoted

+       if (cs < 0 || cs > 7)
+               return -EINVAL;

If you make cs an unsigned int, you won't need to test for < 0.

can do that, yes.

quoted

+
+       if (!lpc) {
+               np = of_find_compatible_node(NULL, NULL, "fsl,mpc5121-lpc");
+               lpc = of_iomap(np, 0);
+               of_node_put(np);
+               if (!lpc)
+                       return -ENOMEM;
+       }
+
+       out_be32(&lpc->cs_cfg[cs], val);

You forgot the iounmap() if lpc == NULL.

No, it is intentional, no need to map/unmap again and again for all
subsequent calls.

quoted

+       return 0;
+}
+EXPORT_SYMBOL_GPL(mpc512x_cs_config);

EXPORT_SYMBOL, please, to match the rest of the Freescale platforms.

I'll change it in v2. Thanks!

Anatolij