Hi Boris, Thanks for the review.
> -----Original Message----- > From: Borislav Petkov [mailto:b...@alien8.de] > Sent: Friday, September 21, 2018 2:37 PM > To: Manish Narani <mnar...@xilinx.com> > Cc: robh...@kernel.org; mark.rutl...@arm.com; mche...@kernel.org; > Michal Simek <mich...@xilinx.com>; leoyang...@nxp.com; > sudeep.ho...@arm.com; amit.kuche...@linaro.org; > devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > e...@vger.kernel.org; linux-arm-ker...@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: > > 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
