| Message ID | 1313562246-24627-1-git-send-email-Chunhe.Lan@freescale.com |
|---|---|
| State | Accepted |
| Commit | 8ddb10eae0c0e6936c69b19c21aaa2f6e0433717 |
| Headers | show |
Hi Chunhe Lan, On 08/17/2011 02:24 AM, Chunhe Lan wrote: [snip] > + > +static inline void do_fixup_by_path_string(void *fdt, const char *path, > + const char *prop, const char *status) > +{ > + do_fixup_by_path(fdt, path, prop, status, strlen(status) + 1, 1); > +} > + After all the good advice from Scott et al., the patch turns into a pretty trivial one-liner. I am questioning the advantage of calling do_fixup_by_path_string(fdt, path, prop, status); vs. simply calling do_fixup_by_path(fdt, path, prop, status, strlen(status) + 1, 1); The do_fixup_by_path_string() saves two parameters "strlen(status) + 1, 1" at the cost of Yet Another Function. Is it worth it? Thanks, gvb
On Thu, 18 Aug 2011 08:29:51 +0800, Jerry Van Baren <gvb.uboot@gmail.com> wrote: > Hi Chunhe Lan, > > On 08/17/2011 02:24 AM, Chunhe Lan wrote: > > [snip] > >> + >> +static inline void do_fixup_by_path_string(void *fdt, const char *path, >> + const char *prop, const char *status) >> +{ >> + do_fixup_by_path(fdt, path, prop, status, strlen(status) + 1, 1); >> +} >> + > > After all the good advice from Scott et al., the patch turns into a > pretty trivial one-liner. I am questioning the advantage of calling > do_fixup_by_path_string(fdt, path, prop, status); > vs. simply calling > do_fixup_by_path(fdt, path, prop, status, strlen(status) + 1, 1); > > The do_fixup_by_path_string() saves two parameters > "strlen(status) + 1, 1" > at the cost of Yet Another Function. Is it worth it? Yes, I think that it is worth. The encapsulation of function is used for that purpose. Please refer to do_fixup_by_path_u32(), and it only has two lines code. Thanks. -Jack Lan
On 08/17/2011 07:29 PM, Jerry Van Baren wrote: > Hi Chunhe Lan, > > On 08/17/2011 02:24 AM, Chunhe Lan wrote: > > [snip] > >> + >> +static inline void do_fixup_by_path_string(void *fdt, const char *path, >> + const char *prop, const char *status) >> +{ >> + do_fixup_by_path(fdt, path, prop, status, strlen(status) + 1, 1); >> +} >> + > > After all the good advice from Scott et al., the patch turns into a > pretty trivial one-liner. I am questioning the advantage of calling > do_fixup_by_path_string(fdt, path, prop, status); > vs. simply calling > do_fixup_by_path(fdt, path, prop, status, strlen(status) + 1, 1); > > The do_fixup_by_path_string() saves two parameters > "strlen(status) + 1, 1" > at the cost of Yet Another Function. Is it worth it? I think it's a nice convenience, with no runtime cost. It avoids any chance of a mismatch between the string passed to do_fixup_by_path and the string passed to strlen. More functions versus open-coding is not generally a bad thing. -Scott
On Aug 17, 2011, at 1:24 AM, Chunhe Lan wrote: > The do_fixup_by_path_string() will set the specified > node's property to the value contained in "status". > It would just be a wrapper for do_fixup_by_path() > that calls strlen on the argument. > Fix where you break the line, should like more like (use upto 75 chars per line): The do_fixup_by_path_string() will set the specified node's property to the value contained in "status". It would just be a wrapper for do_fixup_by_path() that calls strlen on the argument. > Signed-off-by: Chunhe Lan <Chunhe.Lan@freescale.com> > --- > include/fdt_support.h | 7 +++++++ > 1 files changed, 7 insertions(+), 0 deletions(-) > > diff --git a/include/fdt_support.h b/include/fdt_support.h > index 863024f..1de4a1d 100644 > --- a/include/fdt_support.h > +++ b/include/fdt_support.h > @@ -36,6 +36,13 @@ void do_fixup_by_path(void *fdt, const char *path, const char *prop, > const void *val, int len, int create); > void do_fixup_by_path_u32(void *fdt, const char *path, const char *prop, > u32 val, int create); > + > +static inline void do_fixup_by_path_string(void *fdt, const char *path, > + const char *prop, const char *status) > +{ > + do_fixup_by_path(fdt, path, prop, status, strlen(status) + 1, 1); > +} > + > void do_fixup_by_prop(void *fdt, > const char *pname, const void *pval, int plen, > const char *prop, const void *val, int len, > -- > 1.5.6.5
On Mon, 29 Aug 2011 02:15:15 +0800, Kumar Gala <kumar.gala@freescale.com> wrote: > > On Aug 17, 2011, at 1:24 AM, Chunhe Lan wrote: > >> The do_fixup_by_path_string() will set the specified >> node's property to the value contained in "status". >> It would just be a wrapper for do_fixup_by_path() >> that calls strlen on the argument. >> > > Fix where you break the line, should like more like (use upto 75 chars > per line): OK. Thanks. -Jack Lan > The do_fixup_by_path_string() will set the specified node's property > to the > value contained in "status". It would just be a wrapper for > do_fixup_by_path() that calls strlen on the argument. > > >> Signed-off-by: Chunhe Lan <Chunhe.Lan@freescale.com> >> --- >> include/fdt_support.h | 7 +++++++ >> 1 files changed, 7 insertions(+), 0 deletions(-) >> >> diff --git a/include/fdt_support.h b/include/fdt_support.h >> index 863024f..1de4a1d 100644 >> --- a/include/fdt_support.h >> +++ b/include/fdt_support.h >> @@ -36,6 +36,13 @@ void do_fixup_by_path(void *fdt, const char *path, >> const char *prop, >> const void *val, int len, int create); >> void do_fixup_by_path_u32(void *fdt, const char *path, const char *prop, >> u32 val, int create); >> + >> +static inline void do_fixup_by_path_string(void *fdt, const char *path, >> + const char *prop, const char *status) >> +{ >> + do_fixup_by_path(fdt, path, prop, status, strlen(status) + 1, 1); >> +} >> + >> void do_fixup_by_prop(void *fdt, >> const char *pname, const void *pval, int plen, >> const char *prop, const void *val, int len, >> -- >> 1.5.6.5 >
diff --git a/include/fdt_support.h b/include/fdt_support.h index 863024f..1de4a1d 100644 --- a/include/fdt_support.h +++ b/include/fdt_support.h @@ -36,6 +36,13 @@ void do_fixup_by_path(void *fdt, const char *path, const char *prop, const void *val, int len, int create); void do_fixup_by_path_u32(void *fdt, const char *path, const char *prop, u32 val, int create); + +static inline void do_fixup_by_path_string(void *fdt, const char *path, + const char *prop, const char *status) +{ + do_fixup_by_path(fdt, path, prop, status, strlen(status) + 1, 1); +} + void do_fixup_by_prop(void *fdt, const char *pname, const void *pval, int plen, const char *prop, const void *val, int len,
The do_fixup_by_path_string() will set the specified node's property to the value contained in "status". It would just be a wrapper for do_fixup_by_path() that calls strlen on the argument. Signed-off-by: Chunhe Lan <Chunhe.Lan@freescale.com> --- include/fdt_support.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)