Hi Boris,

Thanks for the review.

-----Original Message-----
From: Borislav Petkov [mailto:bp at alien8.de]
Sent: Friday, September 21, 2018 2:37 PM
To: Manish Narani <redacted>
Cc: robh+dt at kernel.org; mark.rutland at arm.com; mchehab at kernel.org;
Michal Simek [off-list ref]; leoyang.li at nxp.com;
sudeep.holla at arm.com; amit.kucheria at linaro.org;
devicetree at vger.kernel.org; linux-kernel at vger.kernel.org; linux-
edac at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH v7 2/7] edac: synps: Add platform specific structures for
ddrc controller

On Wed, Sep 19, 2018 at 01:33:58PM +0000, Manish Narani wrote:

quoted

Apart from this one, I have covered all the comments from the previous

review.

Are you sure?

Let's see. I said:

| Kill all that "function pointer" fluff. Here's how I've changed it:
|
| /**
|  * struct synps_platform_data -  synps platform data structure
|  * @edac_geterror_info: edac error info
|  * @edac_get_mtype:     get mtype
|  * @edac_get_dtype:     get dtype
|  * @edac_get_eccstate:  get ECC state
			  ^^^^^^^^^^^^^

This is supposed to denote that this function returns whether ECC checking is
enabled on the controller or not.

Your patch has:

+ * struct synps_platform_data -  synps platform data structure.
+ * @geterror_info:     Get error info.
+ * @get_mtype:         Get mtype.
+ * @get_dtype:         Get dtype.
+ * @get_eccstate:      Get eccstate.

So what is "eccstate"? Is it a struct or a variable or ...?

Do you see my point?

I know, it is a small thing but documenting our code properly is something
which people would be thanking us for. Even you will be thanking yourself when
you look at this months from now after having forgotten it all.

Please check the rest of your additions for similar discrepancies.

Okay sure. Will be rectified in v8.

Thanks,
Manish