linux-kernel - Re: [PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <54EE0138.7020300@cit-ec.uni-bielefeld.de>
Date:	Wed, 25 Feb 2015 18:07:04 +0100
From:	Robert Abel <rabel@...-ec.uni-bielefeld.de>
To:	Roger Quadros <rogerq@...com>, linux-omap@...r.kernel.org
CC:	linux-kernel@...r.kernel.org, tony@...mide.com,
	linux@....linux.org.uk
Subject: Re: [PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider
 bug

Hi Roger,

On 25 Feb 2015 17:58, Roger Quadros wrote:
>> static unsigned int gpmc_ticks_to_ps(unsigned int ticks)
>> @@ -346,16 +395,22 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
>>    * @st_bit  Start Bit
>>    * @end_bit End Bit. Must be >= @st_bit.
>>    * @name    DTS node name, w/o "gpmc,"
>> + * @cd      Clock Domain of timing parameter.
>> + * @shift   Parameter value left shifts @shift, which is then printed instead of value.
>>    * @raw     Raw Format Option.
>>    *          raw format:  gpmc,name = <value>
>>    *          tick format: gpmc,name = <value> /&zwj;* x ticks *&zwj;/
>>    * @noval   Parameter values equal to 0 are not printed.
>> - * @shift   Parameter value left shifts @shift, which is then printed instead of value.
>>    *
>>    */
>> -static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>> -			       bool raw, bool noval, int shift,
>> -			       const char *name)
>> +static int get_gpmc_timing_reg(
>> +	/* timing specifiers */
>> +	int cs, int reg, int st_bit, int end_bit,
>> +	const char *name, const enum gpmc_clk_domain cd,
>> +	/* value transform */
>> +	int shift,
>> +	/* format specifiers */
>> +	bool raw, bool noval)
> now that you are rearranging the parameters, "name" parameter should probably be
> at the same position (or last) in get_gpmc_timing_reg() and set_gpmc_timing_reg()?
> Also clock domain (cd) position could be matched if possible.
I rearranged them primarily, because I wanted to group the specifiers 
according to function, because I found it unnatural to add clock domain 
to the end, when it's "more important" than the format specifiers.
set_gpmc_timing_reg are fine in that regard as it doesn't have format 
specifiers.
>> +/**
>> + * set_gpmc_timing_reg - set a single timing parameter for Chip Select Region.
>> + * @cs      Chip Select Region.
>> + * @reg     GPMC_CS_CONFIGn register offset.
>> + * @st_bit  Start Bit
>> + * @end_bit End Bit. Must be >= @st_bit.
>> + * @time    Timing parameter in ns.
>> + * @cd      Timing parameter clock domain.
>> + * @name    Timing parameter name.
>> + * @note    Caller is expected to have initialized CONFIG1 GPMCFCLKDIVIDER
> @note is not a parameter.
Well no, note's a note. This is a doxygen-style comment, so tools should 
put a note in the created documentation. Doxygen will put a box with 
yellow background, for instance.
>> -		pr_err("%s: GPMC error! CS%d: %s: %d ns, %d ticks > %d\n",
>> +		pr_err("%s: GPMC CS%d: %s %d ns, %d ticks > %d ticks\n",
> any reason for removing the "error!" string?
It's already pr_err, the "error!" in-between "GPMC CS%d" made it hard to 
read and there's a WARN after that statement in all cases, because a 
child _must_ fail if a timing parameter constraint is broken.

Regards,

Robert

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/