| Message ID | 1392749474-12936-2-git-send-email-ezequiel.garcia@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Ezequiel, Le 18/02/2014 19:51, Ezequiel Garcia a écrit : > This commit adds simple helpers to obtain the devicetree properties > that specify the ECC strength and ECC step size to use on a given > NAND controller. As already discussed with you, IMHO these 2 properties should both be available to be meaningful, hence, we should provide an helper function that gets both properties or return an error if one of them is missing. Do you plan to provide a default value (within controller drivers) if one of these properties is missing ? In this case seperating the helper functions make sense. Best Regards, Boris > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > drivers/of/of_mtd.c | 34 ++++++++++++++++++++++++++++++++++ > include/linux/of_mtd.h | 12 ++++++++++++ > 2 files changed, 46 insertions(+) > > diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c > index a27ec94..b7361ed 100644 > --- a/drivers/of/of_mtd.c > +++ b/drivers/of/of_mtd.c > @@ -50,6 +50,40 @@ int of_get_nand_ecc_mode(struct device_node *np) > EXPORT_SYMBOL_GPL(of_get_nand_ecc_mode); > > /** > + * of_get_nand_ecc_step_size - Get ECC step size associated to > + * the required ECC strength (see below). > + * @np: Pointer to the given device_node > + * > + * return the ECC step size, or errno in error case. > + */ > +int of_get_nand_ecc_step_size(struct device_node *np) > +{ > + int ret; > + u32 val; > + > + ret = of_property_read_u32(np, "nand-ecc-step-size", &val); > + return ret ? ret : val; > +} > +EXPORT_SYMBOL_GPL(of_get_nand_ecc_step_size); > + > +/** > + * of_get_nand_ecc_strength - Get required ECC strength over the > + * correspnding step size as defined by 'nand-ecc-size' > + * @np: Pointer to the given device_node > + * > + * return the ECC strength, or errno in error case. > + */ > +int of_get_nand_ecc_strength(struct device_node *np) > +{ > + int ret; > + u32 val; > + > + ret = of_property_read_u32(np, "nand-ecc-strength", &val); > + return ret ? ret : val; > +} > +EXPORT_SYMBOL_GPL(of_get_nand_ecc_strength); > + > +/** > * of_get_nand_bus_width - Get nand bus witdh for given device_node > * @np: Pointer to the given device_node > * > diff --git a/include/linux/of_mtd.h b/include/linux/of_mtd.h > index cb32d9c..c078f99 100644 > --- a/include/linux/of_mtd.h > +++ b/include/linux/of_mtd.h > @@ -13,6 +13,8 @@ > > #include <linux/of.h> > int of_get_nand_ecc_mode(struct device_node *np); > +int of_get_nand_ecc_step_size(struct device_node *np) > +int of_get_nand_ecc_strength(struct device_node *np) > int of_get_nand_bus_width(struct device_node *np); > bool of_get_nand_on_flash_bbt(struct device_node *np); > > @@ -23,6 +25,16 @@ static inline int of_get_nand_ecc_mode(struct device_node *np) > return -ENOSYS; > } > > +static inline int of_get_nand_ecc_step_size(struct device_node *np) > +{ > + return -ENOSYS; > +} > + > +static inline int of_get_nand_ecc_strength(struct device_node *np) > +{ > + return -ENOSYS; > +} > + > static inline int of_get_nand_bus_width(struct device_node *np) > { > return -ENOSYS;
On Tue, Feb 18, 2014 at 09:01:11PM +0100, Boris BREZILLON wrote: > Le 18/02/2014 19:51, Ezequiel Garcia a écrit : > > This commit adds simple helpers to obtain the devicetree properties > > that specify the ECC strength and ECC step size to use on a given > > NAND controller. > > As already discussed with you, IMHO these 2 properties should both > be available to be meaningful, hence, we should provide an helper > function that gets both properties or return an error if one of them > is missing. > I think this way is both cleaner and simpler. I don't see the advantage of having a single function. > Do you plan to provide a default value (within controller drivers) if one > of these properties is missing ? In this case seperating the helper > functions make sense. > Being a general framework, we can't know what drivers will want to do, which is why I usually like helper function to say as simple as policy-free as possible.
On 18/02/2014 21:25, Ezequiel Garcia wrote: > On Tue, Feb 18, 2014 at 09:01:11PM +0100, Boris BREZILLON wrote: >> Le 18/02/2014 19:51, Ezequiel Garcia a écrit : >>> This commit adds simple helpers to obtain the devicetree properties >>> that specify the ECC strength and ECC step size to use on a given >>> NAND controller. >> As already discussed with you, IMHO these 2 properties should both >> be available to be meaningful, hence, we should provide an helper >> function that gets both properties or return an error if one of them >> is missing. >> > I think this way is both cleaner and simpler. I don't see the advantage > of having a single function. Okay, I'll ack this version and do the check in my driver then. Thanks. Best Regards, Boris > >> Do you plan to provide a default value (within controller drivers) if one >> of these properties is missing ? In this case seperating the helper >> functions make sense. >> > Being a general framework, we can't know what drivers will want to do, > which is why I usually like helper function to say as simple as > policy-free as possible.
On 18/02/2014 19:51, Ezequiel Garcia wrote: > This commit adds simple helpers to obtain the devicetree properties > that specify the ECC strength and ECC step size to use on a given > NAND controller. > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> Acked-by: Boris BREZILLOn <b.brezillon.dev@gmail.com> > --- > drivers/of/of_mtd.c | 34 ++++++++++++++++++++++++++++++++++ > include/linux/of_mtd.h | 12 ++++++++++++ > 2 files changed, 46 insertions(+) > > diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c > index a27ec94..b7361ed 100644 > --- a/drivers/of/of_mtd.c > +++ b/drivers/of/of_mtd.c > @@ -50,6 +50,40 @@ int of_get_nand_ecc_mode(struct device_node *np) > EXPORT_SYMBOL_GPL(of_get_nand_ecc_mode); > > /** > + * of_get_nand_ecc_step_size - Get ECC step size associated to > + * the required ECC strength (see below). > + * @np: Pointer to the given device_node > + * > + * return the ECC step size, or errno in error case. > + */ > +int of_get_nand_ecc_step_size(struct device_node *np) > +{ > + int ret; > + u32 val; > + > + ret = of_property_read_u32(np, "nand-ecc-step-size", &val); > + return ret ? ret : val; > +} > +EXPORT_SYMBOL_GPL(of_get_nand_ecc_step_size); > + > +/** > + * of_get_nand_ecc_strength - Get required ECC strength over the > + * correspnding step size as defined by 'nand-ecc-size' > + * @np: Pointer to the given device_node > + * > + * return the ECC strength, or errno in error case. > + */ > +int of_get_nand_ecc_strength(struct device_node *np) > +{ > + int ret; > + u32 val; > + > + ret = of_property_read_u32(np, "nand-ecc-strength", &val); > + return ret ? ret : val; > +} > +EXPORT_SYMBOL_GPL(of_get_nand_ecc_strength); > + > +/** > * of_get_nand_bus_width - Get nand bus witdh for given device_node > * @np: Pointer to the given device_node > * > diff --git a/include/linux/of_mtd.h b/include/linux/of_mtd.h > index cb32d9c..c078f99 100644 > --- a/include/linux/of_mtd.h > +++ b/include/linux/of_mtd.h > @@ -13,6 +13,8 @@ > > #include <linux/of.h> > int of_get_nand_ecc_mode(struct device_node *np); > +int of_get_nand_ecc_step_size(struct device_node *np) > +int of_get_nand_ecc_strength(struct device_node *np) > int of_get_nand_bus_width(struct device_node *np); > bool of_get_nand_on_flash_bbt(struct device_node *np); > > @@ -23,6 +25,16 @@ static inline int of_get_nand_ecc_mode(struct device_node *np) > return -ENOSYS; > } > > +static inline int of_get_nand_ecc_step_size(struct device_node *np) > +{ > + return -ENOSYS; > +} > + > +static inline int of_get_nand_ecc_strength(struct device_node *np) > +{ > + return -ENOSYS; > +} > + > static inline int of_get_nand_bus_width(struct device_node *np) > { > return -ENOSYS;
On 18/02/2014 19:51, Ezequiel Garcia wrote: > This commit adds simple helpers to obtain the devicetree properties > that specify the ECC strength and ECC step size to use on a given > NAND controller. > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > drivers/of/of_mtd.c | 34 ++++++++++++++++++++++++++++++++++ > include/linux/of_mtd.h | 12 ++++++++++++ > 2 files changed, 46 insertions(+) > > diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c > index a27ec94..b7361ed 100644 > --- a/drivers/of/of_mtd.c > +++ b/drivers/of/of_mtd.c > @@ -50,6 +50,40 @@ int of_get_nand_ecc_mode(struct device_node *np) > EXPORT_SYMBOL_GPL(of_get_nand_ecc_mode); > > /** > + * of_get_nand_ecc_step_size - Get ECC step size associated to > + * the required ECC strength (see below). > + * @np: Pointer to the given device_node > + * > + * return the ECC step size, or errno in error case. > + */ > +int of_get_nand_ecc_step_size(struct device_node *np) > +{ > + int ret; > + u32 val; > + > + ret = of_property_read_u32(np, "nand-ecc-step-size", &val); > + return ret ? ret : val; > +} > +EXPORT_SYMBOL_GPL(of_get_nand_ecc_step_size); > + > +/** > + * of_get_nand_ecc_strength - Get required ECC strength over the > + * correspnding step size as defined by 'nand-ecc-size' > + * @np: Pointer to the given device_node > + * > + * return the ECC strength, or errno in error case. > + */ > +int of_get_nand_ecc_strength(struct device_node *np) > +{ > + int ret; > + u32 val; > + > + ret = of_property_read_u32(np, "nand-ecc-strength", &val); > + return ret ? ret : val; > +} > +EXPORT_SYMBOL_GPL(of_get_nand_ecc_strength); > + > +/** > * of_get_nand_bus_width - Get nand bus witdh for given device_node > * @np: Pointer to the given device_node > * > diff --git a/include/linux/of_mtd.h b/include/linux/of_mtd.h > index cb32d9c..c078f99 100644 > --- a/include/linux/of_mtd.h > +++ b/include/linux/of_mtd.h > @@ -13,6 +13,8 @@ > > #include <linux/of.h> > int of_get_nand_ecc_mode(struct device_node *np); > +int of_get_nand_ecc_step_size(struct device_node *np) > +int of_get_nand_ecc_strength(struct device_node *np) You forgot the semicolon. > int of_get_nand_bus_width(struct device_node *np); > bool of_get_nand_on_flash_bbt(struct device_node *np); > > @@ -23,6 +25,16 @@ static inline int of_get_nand_ecc_mode(struct device_node *np) > return -ENOSYS; > } > > +static inline int of_get_nand_ecc_step_size(struct device_node *np) > +{ > + return -ENOSYS; > +} > + > +static inline int of_get_nand_ecc_strength(struct device_node *np) > +{ > + return -ENOSYS; > +} > + > static inline int of_get_nand_bus_width(struct device_node *np) > { > return -ENOSYS;
On Mon, Feb 24, 2014 at 4:13 PM, Boris BREZILLON <b.brezillon.dev@gmail.com> wrote: > On 18/02/2014 19:51, Ezequiel Garcia wrote: >> >> This commit adds simple helpers to obtain the devicetree properties >> that specify the ECC strength and ECC step size to use on a given >> NAND controller. >> >> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> >> --- >> diff --git a/include/linux/of_mtd.h b/include/linux/of_mtd.h >> index cb32d9c..c078f99 100644 >> --- a/include/linux/of_mtd.h >> +++ b/include/linux/of_mtd.h >> @@ -13,6 +13,8 @@ >> #include <linux/of.h> >> int of_get_nand_ecc_mode(struct device_node *np); >> +int of_get_nand_ecc_step_size(struct device_node *np) >> +int of_get_nand_ecc_strength(struct device_node *np) > > > You forgot the semicolon. Repeat after me: "I shall not post patches that I have not compiled" g.
On Mon, Feb 24, 2014 at 07:08:15PM +0000, Grant Likely wrote: > On Mon, Feb 24, 2014 at 4:13 PM, Boris BREZILLON > <b.brezillon.dev@gmail.com> wrote: > > On 18/02/2014 19:51, Ezequiel Garcia wrote: > >> > >> This commit adds simple helpers to obtain the devicetree properties > >> that specify the ECC strength and ECC step size to use on a given > >> NAND controller. > >> > >> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > >> --- > >> diff --git a/include/linux/of_mtd.h b/include/linux/of_mtd.h > >> index cb32d9c..c078f99 100644 > >> --- a/include/linux/of_mtd.h > >> +++ b/include/linux/of_mtd.h > >> @@ -13,6 +13,8 @@ > >> #include <linux/of.h> > >> int of_get_nand_ecc_mode(struct device_node *np); > >> +int of_get_nand_ecc_step_size(struct device_node *np) > >> +int of_get_nand_ecc_strength(struct device_node *np) > > > > > > You forgot the semicolon. > > Repeat after me: "I shall not post patches that I have not compiled" > Certainly: "I shall not post patches that I have not compiled". Hope this helps me learn the lesson.
diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c index a27ec94..b7361ed 100644 --- a/drivers/of/of_mtd.c +++ b/drivers/of/of_mtd.c @@ -50,6 +50,40 @@ int of_get_nand_ecc_mode(struct device_node *np) EXPORT_SYMBOL_GPL(of_get_nand_ecc_mode); /** + * of_get_nand_ecc_step_size - Get ECC step size associated to + * the required ECC strength (see below). + * @np: Pointer to the given device_node + * + * return the ECC step size, or errno in error case. + */ +int of_get_nand_ecc_step_size(struct device_node *np) +{ + int ret; + u32 val; + + ret = of_property_read_u32(np, "nand-ecc-step-size", &val); + return ret ? ret : val; +} +EXPORT_SYMBOL_GPL(of_get_nand_ecc_step_size); + +/** + * of_get_nand_ecc_strength - Get required ECC strength over the + * correspnding step size as defined by 'nand-ecc-size' + * @np: Pointer to the given device_node + * + * return the ECC strength, or errno in error case. + */ +int of_get_nand_ecc_strength(struct device_node *np) +{ + int ret; + u32 val; + + ret = of_property_read_u32(np, "nand-ecc-strength", &val); + return ret ? ret : val; +} +EXPORT_SYMBOL_GPL(of_get_nand_ecc_strength); + +/** * of_get_nand_bus_width - Get nand bus witdh for given device_node * @np: Pointer to the given device_node * diff --git a/include/linux/of_mtd.h b/include/linux/of_mtd.h index cb32d9c..c078f99 100644 --- a/include/linux/of_mtd.h +++ b/include/linux/of_mtd.h @@ -13,6 +13,8 @@ #include <linux/of.h> int of_get_nand_ecc_mode(struct device_node *np); +int of_get_nand_ecc_step_size(struct device_node *np) +int of_get_nand_ecc_strength(struct device_node *np) int of_get_nand_bus_width(struct device_node *np); bool of_get_nand_on_flash_bbt(struct device_node *np); @@ -23,6 +25,16 @@ static inline int of_get_nand_ecc_mode(struct device_node *np) return -ENOSYS; } +static inline int of_get_nand_ecc_step_size(struct device_node *np) +{ + return -ENOSYS; +} + +static inline int of_get_nand_ecc_strength(struct device_node *np) +{ + return -ENOSYS; +} + static inline int of_get_nand_bus_width(struct device_node *np) { return -ENOSYS;
This commit adds simple helpers to obtain the devicetree properties that specify the ECC strength and ECC step size to use on a given NAND controller. Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> --- drivers/of/of_mtd.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/of_mtd.h | 12 ++++++++++++ 2 files changed, 46 insertions(+)