Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1270490 > unrolled thread

[PATCH v3 2/5] mfd: syscon: add a DT property to set value width

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 2/5] mfd: syscon: add a DT property to set value width Damien Riegel <damien.riegel@savoirfairelinux.com> - 2015-11-16 20:20 +0100
    Re: [PATCH v3 2/5] mfd: syscon: add a DT property to set value width Lee Jones <lee.jones@linaro.org> - 2015-11-17 10:30 +0100
      Re: [PATCH v3 2/5] mfd: syscon: add a DT property to set value width Lee Jones <lee.jones@linaro.org> - 2015-11-17 10:30 +0100
      Re: [PATCH v3 2/5] mfd: syscon: add a DT property to set value width Guenter Roeck <linux@roeck-us.net> - 2015-11-17 18:30 +0100
        Re: [PATCH v3 2/5] mfd: syscon: add a DT property to set value width Lee Jones <lee.jones@linaro.org> - 2015-11-18 09:30 +0100
          Re: [PATCH v3 2/5] mfd: syscon: add a DT property to set value width Guenter Roeck <linux@roeck-us.net> - 2015-11-18 16:20 +0100
            Re: [PATCH v3 2/5] mfd: syscon: add a DT property to set value width Lee Jones <lee.jones@linaro.org> - 2015-11-18 16:30 +0100
        Re: [PATCH v3 2/5] mfd: syscon: add a DT property to set value width Arnd Bergmann <arnd@arndb.de> - 2015-11-21 00:20 +0100
          Re: [PATCH v3 2/5] mfd: syscon: add a DT property to set value width Lee Jones <lee.jones@linaro.org> - 2015-11-23 09:30 +0100

#1270490 — [PATCH v3 2/5] mfd: syscon: add a DT property to set value width

Currently syscon has a fixed configuration of 32 bits for register and
values widths. In some cases, it would be desirable to be able to
customize the value width.

For example, certain boards (like the ones manufactured by Technologic
Systems) have a FPGA that is memory-mapped, but its registers are only
16-bit wide.

This patch adds an optional "bus-width" DT binding for syscon that
allows to change the width for the data bus (i.e. val_bits). If this
property is provided, it will also adjust the register stride to
bus-width / 8. If not provided, the default configuration is used.

Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
---
 Documentation/devicetree/bindings/mfd/syscon.txt |  3 +++
 drivers/mfd/syscon.c                             | 12 ++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
index fe8150b..4c9d187 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.txt
+++ b/Documentation/devicetree/bindings/mfd/syscon.txt
@@ -13,6 +13,9 @@ Required properties:
 - compatible: Should contain "syscon".
 - reg: the register region can be accessed from syscon
 
+Optional property:
+- bus-width: width of the data bus. Can be <8>, <16>, <32>, or <64>.
+
 Examples:
 gpr: iomuxc-gpr@020e0000 {
 	compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 176bf0f..00b0995 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -47,6 +47,7 @@ static struct syscon *of_syscon_register(struct device_node *np)
 	struct syscon *syscon;
 	struct regmap *regmap;
 	void __iomem *base;
+	u32 bus_width;
 	int ret;
 	struct regmap_config syscon_config = syscon_regmap_config;
 
@@ -69,6 +70,17 @@ static struct syscon *of_syscon_register(struct device_node *np)
 	 else if (of_property_read_bool(np, "little-endian"))
 		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
 
+	 ret = of_property_read_u32(np, "bus-width", &bus_width);
+	 if (!ret) {
+		 /*
+		  * the property has been provided, regmap_init_mmio
+		  * will return an error if these values are invalid
+		  * so there is no need to check them here.
+		  */
+		 syscon_config.val_bits = bus_width;
+		 syscon_config.reg_stride = syscon_config.val_bits / 8;
+	 }
+
 	regmap = regmap_init_mmio(NULL, base, &syscon_config);
 	if (IS_ERR(regmap)) {
 		pr_err("regmap init failed\n");
-- 
2.5.0

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

[toc] | [next] | [standalone]

#1270996

On Mon, 16 Nov 2015, Damien Riegel wrote:

> Currently syscon has a fixed configuration of 32 bits for register and
> values widths. In some cases, it would be desirable to be able to
> customize the value width.
> 
> For example, certain boards (like the ones manufactured by Technologic
> Systems) have a FPGA that is memory-mapped, but its registers are only
> 16-bit wide.
> 
> This patch adds an optional "bus-width" DT binding for syscon that
> allows to change the width for the data bus (i.e. val_bits). If this
> property is provided, it will also adjust the register stride to
> bus-width / 8. If not provided, the default configuration is used.
> 
> Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> ---
>  Documentation/devicetree/bindings/mfd/syscon.txt |  3 +++
>  drivers/mfd/syscon.c                             | 12 ++++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
> index fe8150b..4c9d187 100644
> --- a/Documentation/devicetree/bindings/mfd/syscon.txt
> +++ b/Documentation/devicetree/bindings/mfd/syscon.txt
> @@ -13,6 +13,9 @@ Required properties:
>  - compatible: Should contain "syscon".
>  - reg: the register region can be accessed from syscon
>  
> +Optional property:
> +- bus-width: width of the data bus. Can be <8>, <16>, <32>, or <64>.
> +
>  Examples:
>  gpr: iomuxc-gpr@020e0000 {
>  	compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index 176bf0f..00b0995 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -47,6 +47,7 @@ static struct syscon *of_syscon_register(struct device_node *np)
>  	struct syscon *syscon;
>  	struct regmap *regmap;
>  	void __iomem *base;
> +	u32 bus_width;
>  	int ret;
>  	struct regmap_config syscon_config = syscon_regmap_config;
>  
> @@ -69,6 +70,17 @@ static struct syscon *of_syscon_register(struct device_node *np)
>  	 else if (of_property_read_bool(np, "little-endian"))
>  		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
>  
> +	 ret = of_property_read_u32(np, "bus-width", &bus_width);
> +	 if (!ret) {

This syntax is confusing, as we normally associate it with an error
condition.  Instead, I'd use:

  if (of_property_read_u32(np, "bus-width", &bus_width) == 0)

Or, for more clarity:

  of_property_read_u32(np, "bus-width", &bus_width);
  if (bus_width)

If you choose this version (which I think is my preferred method, don't
forget to initialise 'bus_width' to zero.

> +		 /*
> +		  * the property has been provided, regmap_init_mmio
> +		  * will return an error if these values are invalid
> +		  * so there is no need to check them here.
> +		  */
> +		 syscon_config.val_bits = bus_width;
> +		 syscon_config.reg_stride = syscon_config.val_bits / 8;
> +	 }
> +
>  	regmap = regmap_init_mmio(NULL, base, &syscon_config);
>  	if (IS_ERR(regmap)) {
>  		pr_err("regmap init failed\n");

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1271001

Looks like you forgot to Cc Arnd too.

Doing that for you now.

On Tue, 17 Nov 2015, Lee Jones wrote:
> On Mon, 16 Nov 2015, Damien Riegel wrote:
> 
> > Currently syscon has a fixed configuration of 32 bits for register and
> > values widths. In some cases, it would be desirable to be able to
> > customize the value width.
> > 
> > For example, certain boards (like the ones manufactured by Technologic
> > Systems) have a FPGA that is memory-mapped, but its registers are only
> > 16-bit wide.
> > 
> > This patch adds an optional "bus-width" DT binding for syscon that
> > allows to change the width for the data bus (i.e. val_bits). If this
> > property is provided, it will also adjust the register stride to
> > bus-width / 8. If not provided, the default configuration is used.
> > 
> > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> > ---
> >  Documentation/devicetree/bindings/mfd/syscon.txt |  3 +++
> >  drivers/mfd/syscon.c                             | 12 ++++++++++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
> > index fe8150b..4c9d187 100644
> > --- a/Documentation/devicetree/bindings/mfd/syscon.txt
> > +++ b/Documentation/devicetree/bindings/mfd/syscon.txt
> > @@ -13,6 +13,9 @@ Required properties:
> >  - compatible: Should contain "syscon".
> >  - reg: the register region can be accessed from syscon
> >  
> > +Optional property:
> > +- bus-width: width of the data bus. Can be <8>, <16>, <32>, or <64>.
> > +
> >  Examples:
> >  gpr: iomuxc-gpr@020e0000 {
> >  	compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
> > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > index 176bf0f..00b0995 100644
> > --- a/drivers/mfd/syscon.c
> > +++ b/drivers/mfd/syscon.c
> > @@ -47,6 +47,7 @@ static struct syscon *of_syscon_register(struct device_node *np)
> >  	struct syscon *syscon;
> >  	struct regmap *regmap;
> >  	void __iomem *base;
> > +	u32 bus_width;
> >  	int ret;
> >  	struct regmap_config syscon_config = syscon_regmap_config;
> >  
> > @@ -69,6 +70,17 @@ static struct syscon *of_syscon_register(struct device_node *np)
> >  	 else if (of_property_read_bool(np, "little-endian"))
> >  		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
> >  
> > +	 ret = of_property_read_u32(np, "bus-width", &bus_width);
> > +	 if (!ret) {
> 
> This syntax is confusing, as we normally associate it with an error
> condition.  Instead, I'd use:
> 
>   if (of_property_read_u32(np, "bus-width", &bus_width) == 0)
> 
> Or, for more clarity:
> 
>   of_property_read_u32(np, "bus-width", &bus_width);
>   if (bus_width)
> 
> If you choose this version (which I think is my preferred method, don't
> forget to initialise 'bus_width' to zero.
> 
> > +		 /*
> > +		  * the property has been provided, regmap_init_mmio
> > +		  * will return an error if these values are invalid
> > +		  * so there is no need to check them here.
> > +		  */
> > +		 syscon_config.val_bits = bus_width;
> > +		 syscon_config.reg_stride = syscon_config.val_bits / 8;
> > +	 }
> > +
> >  	regmap = regmap_init_mmio(NULL, base, &syscon_config);
> >  	if (IS_ERR(regmap)) {
> >  		pr_err("regmap init failed\n");
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1271473

On Tue, Nov 17, 2015 at 09:19:46AM +0000, Lee Jones wrote:
> On Mon, 16 Nov 2015, Damien Riegel wrote:
> 
> > Currently syscon has a fixed configuration of 32 bits for register and
> > values widths. In some cases, it would be desirable to be able to
> > customize the value width.
> > 
> > For example, certain boards (like the ones manufactured by Technologic
> > Systems) have a FPGA that is memory-mapped, but its registers are only
> > 16-bit wide.
> > 
> > This patch adds an optional "bus-width" DT binding for syscon that
> > allows to change the width for the data bus (i.e. val_bits). If this
> > property is provided, it will also adjust the register stride to
> > bus-width / 8. If not provided, the default configuration is used.
> > 
> > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> > ---
> >  Documentation/devicetree/bindings/mfd/syscon.txt |  3 +++
> >  drivers/mfd/syscon.c                             | 12 ++++++++++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
> > index fe8150b..4c9d187 100644
> > --- a/Documentation/devicetree/bindings/mfd/syscon.txt
> > +++ b/Documentation/devicetree/bindings/mfd/syscon.txt
> > @@ -13,6 +13,9 @@ Required properties:
> >  - compatible: Should contain "syscon".
> >  - reg: the register region can be accessed from syscon
> >  
> > +Optional property:
> > +- bus-width: width of the data bus. Can be <8>, <16>, <32>, or <64>.
> > +
> >  Examples:
> >  gpr: iomuxc-gpr@020e0000 {
> >  	compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
> > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > index 176bf0f..00b0995 100644
> > --- a/drivers/mfd/syscon.c
> > +++ b/drivers/mfd/syscon.c
> > @@ -47,6 +47,7 @@ static struct syscon *of_syscon_register(struct device_node *np)
> >  	struct syscon *syscon;
> >  	struct regmap *regmap;
> >  	void __iomem *base;
> > +	u32 bus_width;
> >  	int ret;
> >  	struct regmap_config syscon_config = syscon_regmap_config;
> >  
> > @@ -69,6 +70,17 @@ static struct syscon *of_syscon_register(struct device_node *np)
> >  	 else if (of_property_read_bool(np, "little-endian"))
> >  		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
> >  
> > +	 ret = of_property_read_u32(np, "bus-width", &bus_width);
> > +	 if (!ret) {
> 
> This syntax is confusing, as we normally associate it with an error
> condition.  Instead, I'd use:
> 
>   if (of_property_read_u32(np, "bus-width", &bus_width) == 0)

Or maybe better

	if (!of_property_read_u32(np, "bus-width", &bus_width))

> 
> Or, for more clarity:
> 
>   of_property_read_u32(np, "bus-width", &bus_width);
>   if (bus_width)
> 
> If you choose this version (which I think is my preferred method, don't
> forget to initialise 'bus_width' to zero.
> 
Ignoring an error and depending on bus_width==0 to determine if the property
was provided seems odd, especially since it would "hide" if the bus-width
property is set to 0. In the original code, this would be detected as error.

Guenter

> > +		 /*
> > +		  * the property has been provided, regmap_init_mmio
> > +		  * will return an error if these values are invalid
> > +		  * so there is no need to check them here.
> > +		  */
> > +		 syscon_config.val_bits = bus_width;
> > +		 syscon_config.reg_stride = syscon_config.val_bits / 8;
> > +	 }
> > +
> >  	regmap = regmap_init_mmio(NULL, base, &syscon_config);
> >  	if (IS_ERR(regmap)) {
> >  		pr_err("regmap init failed\n");
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1271926

On Tue, 17 Nov 2015, Guenter Roeck wrote:

> On Tue, Nov 17, 2015 at 09:19:46AM +0000, Lee Jones wrote:
> > On Mon, 16 Nov 2015, Damien Riegel wrote:
> > 
> > > Currently syscon has a fixed configuration of 32 bits for register and
> > > values widths. In some cases, it would be desirable to be able to
> > > customize the value width.
> > > 
> > > For example, certain boards (like the ones manufactured by Technologic
> > > Systems) have a FPGA that is memory-mapped, but its registers are only
> > > 16-bit wide.
> > > 
> > > This patch adds an optional "bus-width" DT binding for syscon that
> > > allows to change the width for the data bus (i.e. val_bits). If this
> > > property is provided, it will also adjust the register stride to
> > > bus-width / 8. If not provided, the default configuration is used.
> > > 
> > > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> > > ---
> > >  Documentation/devicetree/bindings/mfd/syscon.txt |  3 +++
> > >  drivers/mfd/syscon.c                             | 12 ++++++++++++
> > >  2 files changed, 15 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
> > > index fe8150b..4c9d187 100644
> > > --- a/Documentation/devicetree/bindings/mfd/syscon.txt
> > > +++ b/Documentation/devicetree/bindings/mfd/syscon.txt
> > > @@ -13,6 +13,9 @@ Required properties:
> > >  - compatible: Should contain "syscon".
> > >  - reg: the register region can be accessed from syscon
> > >  
> > > +Optional property:
> > > +- bus-width: width of the data bus. Can be <8>, <16>, <32>, or <64>.
> > > +
> > >  Examples:
> > >  gpr: iomuxc-gpr@020e0000 {
> > >  	compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
> > > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > > index 176bf0f..00b0995 100644
> > > --- a/drivers/mfd/syscon.c
> > > +++ b/drivers/mfd/syscon.c
> > > @@ -47,6 +47,7 @@ static struct syscon *of_syscon_register(struct device_node *np)
> > >  	struct syscon *syscon;
> > >  	struct regmap *regmap;
> > >  	void __iomem *base;
> > > +	u32 bus_width;
> > >  	int ret;
> > >  	struct regmap_config syscon_config = syscon_regmap_config;
> > >  
> > > @@ -69,6 +70,17 @@ static struct syscon *of_syscon_register(struct device_node *np)
> > >  	 else if (of_property_read_bool(np, "little-endian"))
> > >  		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
> > >  
> > > +	 ret = of_property_read_u32(np, "bus-width", &bus_width);
> > > +	 if (!ret) {
> > 
> > This syntax is confusing, as we normally associate it with an error
> > condition.  Instead, I'd use:
> > 
> >   if (of_property_read_u32(np, "bus-width", &bus_width) == 0)
> 
> Or maybe better
> 
> 	if (!of_property_read_u32(np, "bus-width", &bus_width))

Not sure if it's better, but it's also suitable.

> > Or, for more clarity:
> > 
> >   of_property_read_u32(np, "bus-width", &bus_width);
> >   if (bus_width)
> > 
> > If you choose this version (which I think is my preferred method, don't
> > forget to initialise 'bus_width' to zero.
> > 
> Ignoring an error and depending on bus_width==0 to determine if the property
> was provided seems odd, especially since it would "hide" if the bus-width
> property is set to 0. In the original code, this would be detected as error.

I'm not sure what you mean.  If bus_width==0, then a problem has
occurred and we will not use the value.  If bus_width!=0 then we can
assume that it's been set and (as the comment describes) the value
will be checked for errors in regmap_init_mmio().

> > > +		 /*
> > > +		  * the property has been provided, regmap_init_mmio
> > > +		  * will return an error if these values are invalid
> > > +		  * so there is no need to check them here.
> > > +		  */
> > > +		 syscon_config.val_bits = bus_width;
> > > +		 syscon_config.reg_stride = syscon_config.val_bits / 8;
> > > +	 }
> > > +
> > >  	regmap = regmap_init_mmio(NULL, base, &syscon_config);
> > >  	if (IS_ERR(regmap)) {
> > >  		pr_err("regmap init failed\n");
> > 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272286

On 11/18/2015 12:21 AM, Lee Jones wrote:
[ ... ]
>>> Or, for more clarity:
>>>
>>>    of_property_read_u32(np, "bus-width", &bus_width);
>>>    if (bus_width)
>>>
>>> If you choose this version (which I think is my preferred method, don't
>>> forget to initialise 'bus_width' to zero.
>>>
>> Ignoring an error and depending on bus_width==0 to determine if the property
>> was provided seems odd, especially since it would "hide" if the bus-width
>> property is set to 0. In the original code, this would be detected as error.
>
> I'm not sure what you mean.  If bus_width==0, then a problem has
> occurred and we will not use the value.  If bus_width!=0 then we can
> assume that it's been set and (as the comment describes) the value
> will be checked for errors in regmap_init_mmio().
>

Your proposed code does not distinguish a missing property from "bus-width = <0>;".
It will silently ignore this case.

Guenter

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

[toc] | [prev] | [next] | [standalone]

#1272295

On Wed, 18 Nov 2015, Guenter Roeck wrote:

> On 11/18/2015 12:21 AM, Lee Jones wrote:
> [ ... ]
> >>>Or, for more clarity:
> >>>
> >>>   of_property_read_u32(np, "bus-width", &bus_width);
> >>>   if (bus_width)
> >>>
> >>>If you choose this version (which I think is my preferred method, don't
> >>>forget to initialise 'bus_width' to zero.
> >>>
> >>Ignoring an error and depending on bus_width==0 to determine if the property
> >>was provided seems odd, especially since it would "hide" if the bus-width
> >>property is set to 0. In the original code, this would be detected as error.
> >
> >I'm not sure what you mean.  If bus_width==0, then a problem has
> >occurred and we will not use the value.  If bus_width!=0 then we can
> >assume that it's been set and (as the comment describes) the value
> >will be checked for errors in regmap_init_mmio().
> >
> 
> Your proposed code does not distinguish a missing property from "bus-width = <0>;".
> It will silently ignore this case.

Okay, so what you're saying is, you'd prefer to throw and error
instead of using the default.  Fair point.  If this is an issue then
feel free to use either of the other two suggestions.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1274478

On Tuesday 17 November 2015 09:26:49 Guenter Roeck wrote:
> > 
> > This syntax is confusing, as we normally associate it with an error
> > condition.  Instead, I'd use:
> > 
> >   if (of_property_read_u32(np, "bus-width", &bus_width) == 0)
> 
> Or maybe better
> 
>         if (!of_property_read_u32(np, "bus-width", &bus_width))

I would also prefer the latter, but it doesn't matter much either way.

> > 
> > Or, for more clarity:
> > 
> >   of_property_read_u32(np, "bus-width", &bus_width);
> >   if (bus_width)
> > 
> > If you choose this version (which I think is my preferred method, don't
> > forget to initialise 'bus_width' to zero.
> > 
> Ignoring an error and depending on bus_width==0 to determine if the property
> was provided seems odd, especially since it would "hide" if the bus-width
> property is set to 0. In the original code, this would be detected as error.

Right.

Another option would be

	ret = of_property_read_u32(np, "bus-width", &bus_width);
	/* no bus width provided, default to 32-bit */
	if (ret)
		bus_width = 32;

	syscon_config.val_bits = bus_width;
	syscon_config.reg_stride = syscon_config.val_bits / 8;

which has the same effect but seems a little clearer to me.

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

[toc] | [prev] | [next] | [standalone]

#1275118

On Sat, 21 Nov 2015, Arnd Bergmann wrote:

> On Tuesday 17 November 2015 09:26:49 Guenter Roeck wrote:
> > > 
> > > This syntax is confusing, as we normally associate it with an error
> > > condition.  Instead, I'd use:
> > > 
> > >   if (of_property_read_u32(np, "bus-width", &bus_width) == 0)
> > 
> > Or maybe better
> > 
> >         if (!of_property_read_u32(np, "bus-width", &bus_width))
> 
> I would also prefer the latter, but it doesn't matter much either way.
> 
> > > 
> > > Or, for more clarity:
> > > 
> > >   of_property_read_u32(np, "bus-width", &bus_width);
> > >   if (bus_width)
> > > 
> > > If you choose this version (which I think is my preferred method, don't
> > > forget to initialise 'bus_width' to zero.
> > > 
> > Ignoring an error and depending on bus_width==0 to determine if the property
> > was provided seems odd, especially since it would "hide" if the bus-width
> > property is set to 0. In the original code, this would be detected as error.
> 
> Right.
> 
> Another option would be
> 
> 	ret = of_property_read_u32(np, "bus-width", &bus_width);
> 	/* no bus width provided, default to 32-bit */
> 	if (ret)
> 		bus_width = 32;
> 
> 	syscon_config.val_bits = bus_width;
> 	syscon_config.reg_stride = syscon_config.val_bits / 8;
> 
> which has the same effect but seems a little clearer to me.

Works for me.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web