Hi Mark,

On 26 October 2016 at 23:46, Mark Rutland [off-list ref] wrote:

On Wed, Oct 26, 2016 at 11:24:32PM +0800, Fu Wei wrote:

quoted

On 21 October 2016 at 19:32, Mark Rutland [off-list ref] wrote:

quoted

On Thu, Sep 29, 2016 at 02:17:15AM +0800, fu.wei at linaro.org wrote:

quoted

+static int __init arch_timer_mem_register(struct device_node *np, void *frame)
 {
-     int ret;
-     irq_handler_t func;
+     struct device_node *frame_node = NULL;
      struct arch_timer *t;
+     void __iomem *base;
+     irq_handler_t func;
+     unsigned int irq;
+     int ret;
+
+     if (!frame)
+             return -EINVAL;

Why would we call this without a frame?

Sorry, I just verify it , make sure frame is not NULL,
Because it is a "static" function, so we do need this check?

I'd rather we simply don't call the function rather than passing a NULL
frame in.

OK,  NP, will do

quoted

quoted

quoted

+
+     if (np) {

... or without a node?

For "np", for now, we just  just verify it, but it is just paperation
for GTDT support,
Because in next patch, if np == NULL, that means we call this function
from GTDT, but not DT.

Please don't do that. More on that below.

quoted

quoted

Please as Marc requested several versions ago: split the FW parsing
(ACPI and DT) so that happens first, *then* once we have the data in a
common format, use that to drive poking the HW, requesting IRQs, etc,
completely independent of the source.

In patches, do this by:

(1) adding the data structures
(2) splitting the existing DT probing to use them
(3) Adding ACPI functionality atop

this patch is a preparation for GTDT support, I have splitted some
functions for reusing them in next patch(GTDT support)

if np == NULL, that means we call this function from GTDT, but
if np != NULL, that means we call this function from DT

As above, please structure the patches such that that never happens.

We currently have:

+--------------------------+
| DT Parsing + Common code |
+--------------------------+

Per (1 and 2) make this:

+------------+                       +-------------+
| DT parsing |--(common structure)-->| Common code |
+------------+                       +-------------+

Then per (3) make this:

+------------+
| DT parsing |--(common structure)------+
+------------+                          |
                                        v
                                    +-------------+
                                    | Common code |
                                    +-------------+
                                        ^
+--------------+                        |
| ACPI parsing |--(common structure)----+

Thanks for your suggestion , I will do this way in my next patchset

Thanks :-)

+--------------+

Thanks,
Mark.

-- 
Best regards,

Fu Wei
Software Engineer
Red Hat