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

2015-02-25 Thread Roger Quadros 
On 25/02/15 19:22, Robert Abel wrote:
> Hi Roger,
> 
> On 25 Feb 2015 18:17, Roger Quadros wrote:
>> How will the user know by looking at the kernel log that it was really an 
>> error?
>> We don't fail probe if set_gpmc_timing_reg() fails so I feel it is necessary 
>> to
>> clearly show an Error message.
>>
>> You can probably reword it like "%s: Error!! GPMC CS %d..."
> 
> I'll put "error" in there. But just for the record, it's this messaged 
> followed by a WARN that explains "failed to add child %s".
> So I'd expect the user to put A and B together ;)

Sorry, my bad. We are in fact returning -1 in GPMC_SET_ONE().
So no need to add the "Error" string.

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

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

2015-02-25 Thread Robert Abel 

Hi Roger,

On 25 Feb 2015 18:17, Roger Quadros wrote:

How will the user know by looking at the kernel log that it was really an error?
We don't fail probe if set_gpmc_timing_reg() fails so I feel it is necessary to
clearly show an Error message.

You can probably reword it like "%s: Error!! GPMC CS %d..."


I'll put "error" in there. But just for the record, it's this messaged 
followed by a WARN that explains "failed to add child %s".

So I'd expect the user to put A and B together ;)

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

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

2015-02-25 Thread Roger Quadros 
Robert,

On 25/02/15 19:07, Robert Abel wrote:
> 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.
>>>* @nameDTS 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 = 
>>>*  tick format: gpmc,name =  /* x ticks */
>>>* @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.

OK.

>>> +/**
>>> + * 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.
>>> + * @timeTiming parameter in ns.
>>> + * @cd  Timing parameter clock domain.
>>> + * @nameTiming parameter name.
>>> + * @noteCaller 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.

Oh ok.

>>> -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.

How will the user know by looking at the kernel log that it was really an error?
We don't fail probe if set_gpmc_timing_reg() fails so I feel it is necessary to
clearly show an Error message.

You can probably reword it like "%s: Error!! GPMC CS %d..."

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

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

2015-02-25 Thread Robert Abel 

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.
   * @nameDTS 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 = 
   *  tick format: gpmc,name =  /* x ticks */
   * @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.
+ * @timeTiming parameter in ns.
+ * @cd  Timing parameter clock domain.
+ * @nameTiming parameter name.
+ * @noteCaller 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-02-25 Thread Roger Quadros 
Robert,

On 24/02/15 22:05, Robert ABEL wrote:
> The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
> even though the access is defined as asynchronous, and no GPMC_CLK clock
> is provided to the external device. Still, GPMCFCLKDIVIDER is used as a 
> divider
> for the GPMC clock, so it must be programmed to define the
> correct WAITMONITORINGTIME delay.
> 
> This patch correctly computes WAITMONITORINGTIME in GPMC_CLK cycles instead 
> of GPMC_FCLK cycles,
> both during programming (gpmc_cs_set_timings) and during retrieval 
> (gpmc_cs_show_timings).
> 
> Signed-off-by: Robert ABEL 
> ---
>  drivers/memory/omap-gpmc.c | 125 
> +++--
>  1 file changed, 99 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 9cf8d21..6591991 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -170,6 +170,11 @@
>   */
>  #define  GPMC_NR_IRQ 2
>  
> +enum gpmc_clk_domain {
> + GPMC_CD_FCLK,
> + GPMC_CD_CLK
> +};
> +
>  struct gpmc_cs_data {
>   const char *name;
>  
> @@ -268,16 +273,55 @@ static unsigned long gpmc_get_fclk_period(void)
>   return rate;
>  }
>  
> -static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
> +/**
> + * gpmc_get_clk_period - get period of selected clock domain in ps
> + * @cs Chip Select Region.
> + * @cd Clock Domain.
> + *
> + * GPMC_CS_CONFIG1 GPMCFCLKDIVIDER for cs has to be setup
> + * prior to calling this function with GPMC_CD_CLK.
> + * 
> + */
> +static unsigned long gpmc_get_clk_period(int cs, enum gpmc_clk_domain cd)
> +{
> +
> + unsigned long tick_ps = gpmc_get_fclk_period();
> + u32 l;
> + int div;
> +
> + switch (cd) {
> + case GPMC_CD_CLK:
> + /* get current clk divider */
> + l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> + div = (l & 0x03) + 1;
> + /* get GPMC_CLK period */
> + tick_ps *= div;
> + break;
> + case GPMC_CD_FCLK:
> + /* FALL-THROUGH */
> + default:
> + break;
> + }
> +
> + return tick_ps;
> +
> +}
> +
> +static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, enum 
> gpmc_clk_domain cd)
>  {
>   unsigned long tick_ps;
>  
>   /* Calculate in picosecs to yield more exact results */
> - tick_ps = gpmc_get_fclk_period();
> + tick_ps = gpmc_get_clk_period(cs, cd);
>  
>   return (time_ns * 1000 + tick_ps - 1) / tick_ps;
>  }
>  
> +static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
> +{
> + return gpmc_ns_to_clk_ticks(time_ns, /* any CS */ 0, GPMC_CD_FCLK);
> +}
> +
>  static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
>  {
>   unsigned long tick_ps;
> @@ -288,9 +332,14 @@ static unsigned int gpmc_ps_to_ticks(unsigned int 
> time_ps)
>   return (time_ps + tick_ps - 1) / tick_ps;
>  }
>  
> +unsigned int gpmc_clk_ticks_to_ns(unsigned ticks, int cs, enum 
> gpmc_clk_domain cd)
> +{
> + return ticks * gpmc_get_clk_period(cs, cd) / 1000;
> +}
> +
>  unsigned int gpmc_ticks_to_ns(unsigned int ticks)
>  {
> - return ticks * gpmc_get_fclk_period() / 1000;
> + return gpmc_clk_ticks_to_ns(ticks, /* any CS */ 0, GPMC_CD_FCLK);
>  }
>  
>  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.
>   * @nameDTS 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 = 
>   *  tick format: gpmc,name =  /* x ticks */
>   * @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.

>  {
>   u32 l;
>   int nr_bits;
> @@ -373,7 +428,7 @@ static int get_gpmc_timing_reg(int cs, int reg, int 
> st_bit, int end_bit,
>   /* DTS tick format for timings in ns */
>   unsigned int time_ns;
>  
> - time_ns =

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

2015-02-25 Thread Roger Quadros 
Robert,

On 24/02/15 22:05, Robert ABEL wrote:
 The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
 even though the access is defined as asynchronous, and no GPMC_CLK clock
 is provided to the external device. Still, GPMCFCLKDIVIDER is used as a 
 divider
 for the GPMC clock, so it must be programmed to define the
 correct WAITMONITORINGTIME delay.
 
 This patch correctly computes WAITMONITORINGTIME in GPMC_CLK cycles instead 
 of GPMC_FCLK cycles,
 both during programming (gpmc_cs_set_timings) and during retrieval 
 (gpmc_cs_show_timings).
 
 Signed-off-by: Robert ABEL ra...@cit-ec.uni-bielefeld.de
 ---
  drivers/memory/omap-gpmc.c | 125 
 +++--
  1 file changed, 99 insertions(+), 26 deletions(-)
 
 diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
 index 9cf8d21..6591991 100644
 --- a/drivers/memory/omap-gpmc.c
 +++ b/drivers/memory/omap-gpmc.c
 @@ -170,6 +170,11 @@
   */
  #define  GPMC_NR_IRQ 2
  
 +enum gpmc_clk_domain {
 + GPMC_CD_FCLK,
 + GPMC_CD_CLK
 +};
 +
  struct gpmc_cs_data {
   const char *name;
  
 @@ -268,16 +273,55 @@ static unsigned long gpmc_get_fclk_period(void)
   return rate;
  }
  
 -static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
 +/**
 + * gpmc_get_clk_period - get period of selected clock domain in ps
 + * @cs Chip Select Region.
 + * @cd Clock Domain.
 + *
 + * GPMC_CS_CONFIG1 GPMCFCLKDIVIDER for cs has to be setup
 + * prior to calling this function with GPMC_CD_CLK.
 + * 
 + */
 +static unsigned long gpmc_get_clk_period(int cs, enum gpmc_clk_domain cd)
 +{
 +
 + unsigned long tick_ps = gpmc_get_fclk_period();
 + u32 l;
 + int div;
 +
 + switch (cd) {
 + case GPMC_CD_CLK:
 + /* get current clk divider */
 + l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
 + div = (l  0x03) + 1;
 + /* get GPMC_CLK period */
 + tick_ps *= div;
 + break;
 + case GPMC_CD_FCLK:
 + /* FALL-THROUGH */
 + default:
 + break;
 + }
 +
 + return tick_ps;
 +
 +}
 +
 +static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, enum 
 gpmc_clk_domain cd)
  {
   unsigned long tick_ps;
  
   /* Calculate in picosecs to yield more exact results */
 - tick_ps = gpmc_get_fclk_period();
 + tick_ps = gpmc_get_clk_period(cs, cd);
  
   return (time_ns * 1000 + tick_ps - 1) / tick_ps;
  }
  
 +static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
 +{
 + return gpmc_ns_to_clk_ticks(time_ns, /* any CS */ 0, GPMC_CD_FCLK);
 +}
 +
  static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
  {
   unsigned long tick_ps;
 @@ -288,9 +332,14 @@ static unsigned int gpmc_ps_to_ticks(unsigned int 
 time_ps)
   return (time_ps + tick_ps - 1) / tick_ps;
  }
  
 +unsigned int gpmc_clk_ticks_to_ns(unsigned ticks, int cs, enum 
 gpmc_clk_domain cd)
 +{
 + return ticks * gpmc_get_clk_period(cs, cd) / 1000;
 +}
 +
  unsigned int gpmc_ticks_to_ns(unsigned int ticks)
  {
 - return ticks * gpmc_get_fclk_period() / 1000;
 + return gpmc_clk_ticks_to_ns(ticks, /* any CS */ 0, GPMC_CD_FCLK);
  }
  
  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.
   * @nameDTS 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.

  {
   u32 l;
   int nr_bits;
 @@ -373,7 +428,7 @@ static int get_gpmc_timing_reg(int cs, int reg, int 
 st_bit, int end_bit,
   /* DTS tick format for timings in ns */
   unsigned int time_ns;
  
 - time_ns = gpmc_ticks_to_ns(l);
 + time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
   pr_info(gpmc,%s = %u

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

2015-02-25 Thread Robert Abel 

Hi Roger,

On 25 Feb 2015 18:17, Roger Quadros wrote:

How will the user know by looking at the kernel log that it was really an error?
We don't fail probe if set_gpmc_timing_reg() fails so I feel it is necessary to
clearly show an Error message.

You can probably reword it like %s: Error!! GPMC CS %d...


I'll put error in there. But just for the record, it's this messaged 
followed by a WARN that explains failed to add child %s.

So I'd expect the user to put A and B together ;)

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

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

2015-02-25 Thread Robert Abel 

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.
   * @nameDTS 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.
+ * @timeTiming parameter in ns.
+ * @cd  Timing parameter clock domain.
+ * @nameTiming parameter name.
+ * @noteCaller 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-02-25 Thread Roger Quadros 
On 25/02/15 19:22, Robert Abel wrote:
 Hi Roger,
 
 On 25 Feb 2015 18:17, Roger Quadros wrote:
 How will the user know by looking at the kernel log that it was really an 
 error?
 We don't fail probe if set_gpmc_timing_reg() fails so I feel it is necessary 
 to
 clearly show an Error message.

 You can probably reword it like %s: Error!! GPMC CS %d...
 
 I'll put error in there. But just for the record, it's this messaged 
 followed by a WARN that explains failed to add child %s.
 So I'd expect the user to put A and B together ;)

Sorry, my bad. We are in fact returning -1 in GPMC_SET_ONE().
So no need to add the Error string.

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

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

2015-02-25 Thread Roger Quadros 
Robert,

On 25/02/15 19:07, Robert Abel wrote:
 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.
* @nameDTS 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.

OK.

 +/**
 + * 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.
 + * @timeTiming parameter in ns.
 + * @cd  Timing parameter clock domain.
 + * @nameTiming parameter name.
 + * @noteCaller 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.

Oh ok.

 -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.

How will the user know by looking at the kernel log that it was really an error?
We don't fail probe if set_gpmc_timing_reg() fails so I feel it is necessary to
clearly show an Error message.

You can probably reword it like %s: Error!! GPMC CS %d...

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