Hi Boris, Thanks for the review. > -----Original Message----- > From: Borislav Petkov [mailto:bp@xxxxxxxxx] > Sent: Friday, September 21, 2018 2:37 PM > To: Manish Narani <MNARANI@xxxxxxxxxx> > Cc: robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; mchehab@xxxxxxxxxx; > Michal Simek <michals@xxxxxxxxxx>; leoyang.li@xxxxxxx; > sudeep.holla@xxxxxxx; amit.kucheria@xxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux- > edac@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > 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
