[U-Boot] [PATCH] ns16550: change reg-shift property default to zero

[U-Boot] [PATCH] ns16550: change reg-shift property default to zero

[Thomas Chou]

Change reg-shift property default to zero. When the integer property
is missing, it should be taken as zero. This is consistent to Linux
drivers/tty/serial/of_serial.c.

The x86 and most powerpc use reg-shift of 0. Most others use reg-shift
of 2. While reg-shift of 1 is rarely used.

Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
---
 drivers/serial/ns16550.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index d5bcbc3..5b64f7c 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -403,7 +403,7 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
 
 	plat->base = addr;
 	plat->reg_shift = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
-					 "reg-shift", 1);
+					 "reg-shift", 0);
 	plat->clock = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
 				     "clock-frequency",
 				     CONFIG_SYS_NS16550_CLK);
-- 
2.5.0

[U-Boot] [PATCH] ns16550: change reg-shift property default to zero

[Bin Meng]

On Sun, Nov 29, 2015 at 2:01 PM, Thomas Chou <thomas@wytron.com.tw> wrote:
> Change reg-shift property default to zero. When the integer property
> is missing, it should be taken as zero. This is consistent to Linux
> drivers/tty/serial/of_serial.c.
>
> The x86 and most powerpc use reg-shift of 0. Most others use reg-shift
> of 2. While reg-shift of 1 is rarely used.
>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
>  drivers/serial/ns16550.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index d5bcbc3..5b64f7c 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -403,7 +403,7 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
>
>         plat->base = addr;
>         plat->reg_shift = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
> -                                        "reg-shift", 1);
> +                                        "reg-shift", 0);
>         plat->clock = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
>                                      "clock-frequency",
>                                      CONFIG_SYS_NS16550_CLK);
> --

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH] ns16550: change reg-shift property default to zero

[Mugunthan V N]

On Sunday 29 November 2015 11:31 AM, Thomas Chou wrote:
> Change reg-shift property default to zero. When the integer property
> is missing, it should be taken as zero. This is consistent to Linux
> drivers/tty/serial/of_serial.c.
> 
> The x86 and most powerpc use reg-shift of 0. Most others use reg-shift
> of 2. While reg-shift of 1 is rarely used.
> 
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>

Reviewed-by: Mugunthan V N <mugunthanvnm@ti.com>

Regards
Mugunthan V N

[U-Boot] [PATCH] ns16550: change reg-shift property default to zero

[Simon Glass]

On 29 November 2015 at 23:07, Mugunthan V N <mugunthanvnm@ti.com> wrote:
> On Sunday 29 November 2015 11:31 AM, Thomas Chou wrote:
>> Change reg-shift property default to zero. When the integer property
>> is missing, it should be taken as zero. This is consistent to Linux
>> drivers/tty/serial/of_serial.c.
>>
>> The x86 and most powerpc use reg-shift of 0. Most others use reg-shift
>> of 2. While reg-shift of 1 is rarely used.
>>
>> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
>
> Reviewed-by: Mugunthan V N <mugunthanvnm@ti.com>

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] ns16550: change reg-shift property default to zero

[Tom Rini]

On Sun, Nov 29, 2015 at 02:01:03PM +0800, Thomas Chou wrote:

> Change reg-shift property default to zero. When the integer property
> is missing, it should be taken as zero. This is consistent to Linux
> drivers/tty/serial/of_serial.c.
> 
> The x86 and most powerpc use reg-shift of 0. Most others use reg-shift
> of 2. While reg-shift of 1 is rarely used.
> 
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> Reviewed-by: Mugunthan V N <mugunthanvnm@ti.com>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151206/c0746af8/attachment.sig>

[U-Boot] ns16550: change reg-shift property default to zero

[Stephen Warren]

On 12/06/2015 03:07 PM, Tom Rini wrote:
> On Sun, Nov 29, 2015 at 02:01:03PM +0800, Thomas Chou wrote:
>
>> Change reg-shift property default to zero. When the integer property
>> is missing, it should be taken as zero. This is consistent to Linux
>> drivers/tty/serial/of_serial.c.

It's not generally true that missing properties have value zero. Rather, 
whatever values was assumed by the semantics of the binding before that 
optional property was defined should be assumed.

What the Linux kernel driver does also isn't justification for this 
change, since DT bindings define how they work, rather than a particular 
OS implementation forcing the hand of the binding.

The DT binding documentation must state the default value/semantics for 
any optional property. Can you please make sure the DT binding 
documentation is updated to describe this case?

(Note that I have no objection to this patch; the actual change seems 
fine. It's simply that the justifications given in the patch description 
for it aren't entirely robust.)

[U-Boot] ns16550: change reg-shift property default to zero

[Thomas Chou]

Hi Stephen,

On 2015?12?08? 01:32, Stephen Warren wrote:
> On 12/06/2015 03:07 PM, Tom Rini wrote:
>> On Sun, Nov 29, 2015 at 02:01:03PM +0800, Thomas Chou wrote:
>>
>>> Change reg-shift property default to zero. When the integer property
>>> is missing, it should be taken as zero. This is consistent to Linux
>>> drivers/tty/serial/of_serial.c.
>
> It's not generally true that missing properties have value zero. Rather,
> whatever values was assumed by the semantics of the binding before that
> optional property was defined should be assumed.
>
> What the Linux kernel driver does also isn't justification for this
> change, since DT bindings define how they work, rather than a particular
> OS implementation forcing the hand of the binding.
>
> The DT binding documentation must state the default value/semantics for
> any optional property. Can you please make sure the DT binding
> documentation is updated to describe this case?
>
> (Note that I have no objection to this patch; the actual change seems
> fine. It's simply that the justifications given in the patch description
> for it aren't entirely robust.)
>

Thanks. Will send a follow-up patch for the DT binding.

Best regards,
Thomas