[PATCH linux 0/2] I2c : do not rename added devices, and a minor dtsi fix

[PATCH linux 0/2] I2c : do not rename added devices, and a minor dtsi fix

[OpenBMC Patches]

The documentation says you can't rename devices once they are added.

The commit log is a bit wide because I didn't wrap the cut & pasted comments from the code.

The code is a bit ugly looking for properties in the child to determine the name.

But there are rules!

Comments and alternatives welcome.

Also is a separate patch for the dtsi which I noticed while looking at the tree structure.

milton


https://github.com/openbmc/linux/pull/49

Milton D. Miller II (2):
  i2c: aspeed: Don't rename added devices
  arm: dts: unit address should not have 0x prefix

 arch/arm/boot/dts/ast2400.dtsi  |  2 +-
 drivers/i2c/busses/i2c-aspeed.c | 28 ++++++++++++++++++++--------
 2 files changed, 21 insertions(+), 9 deletions(-)

-- 
2.7.1

[PATCH linux 1/2] i2c: aspeed: Don't rename added devices

[OpenBMC Patches]

From: "Milton D. Miller II" <miltonm@us.ibm.com>

Changing the name of a device with dev_set_name after device_add
is not allowed.

However, of_create_device takes a bus_id parameter that specifies
the name of the platform device being added.

This does mean the bus property has to be retrieved multiple times.
Creation is skiped if there is trouble retrieving or formatting
the name.

The documentation for kobject_set_name (non-vargs) says:
 * This sets the name of the kobject.  If you have already added the
 * kobject to the system, you must call kobject_rename() in order to
 * change the name of the kobject.

As the kernel doc for device_rename says:
 * Make up a "real" name in the driver before you register anything, or add
 * some other attributes for userspace to find the device, or use udev to add
 * symlinks -- but never rename kernel devices later, it's a complete mess. We
 * don't even want to get into that and try to implement the missing pieces in
 * the core. We really have other pieces to fix in the driver core mess. :)

before:
1e78a000.i2c/1e78a1c0.i2c-bus/i2c-6/i2c-dev/i2c-6
1e78a000.i2c/1e78a180.i2c-bus/i2c-5/i2c-dev/i2c-5
1e78a000.i2c/1e78a140.i2c-bus/i2c-4/i2c-dev/i2c-4
1e78a000.i2c/1e78a100.i2c-bus/i2c-3/i2c-dev/i2c-3
1e78a000.i2c/1e78a100.i2c-bus/i2c-3/3-0050
1e78a000.i2c/1e78a0c0.i2c-bus/i2c-2/i2c-dev/i2c-2
1e78a000.i2c/1e78a0c0.i2c-bus/i2c-2/2-004c/hwmon/hwmon0
1e78a000.i2c/1e78a080.i2c-bus/i2c-1/i2c-dev/i2c-1
1e78a000.i2c/1e78a040.i2c-bus/i2c-0/i2c-dev/i2c-0
1e78a000.i2c/1e78a040.i2c-bus/i2c-0/0-0050
1e78a000.i2c/1e78a040.i2c-bus/i2c-0/0-0068/rtc/rtc0
1e78a000.i2c/1e78a340.i2c-bus/i2c-8/i2c-dev/i2c-8

after:
1e78a000.i2c/i2c-0/i2c-0/i2c-dev/i2c-0
1e78a000.i2c/i2c-0/i2c-0/0-0050
1e78a000.i2c/i2c-0/i2c-0/0-0068/rtc/rtc0
1e78a000.i2c/i2c-1/i2c-1/i2c-dev/i2c-1
1e78a000.i2c/i2c-2/i2c-2/i2c-dev/i2c-2
1e78a000.i2c/i2c-2/i2c-2/2-004c/hwmon/hwmon0
1e78a000.i2c/i2c-3/i2c-3/i2c-dev/i2c-3
1e78a000.i2c/i2c-3/i2c-3/3-0050
1e78a000.i2c/i2c-4/i2c-4/i2c-dev/i2c-4
1e78a000.i2c/i2c-5/i2c-5/i2c-dev/i2c-5
1e78a000.i2c/i2c-6/i2c-6/i2c-dev/i2c-6
1e78a000.i2c/i2c-7/i2c-7/i2c-dev/i2c-7

1e620000.fmc                   1e78e000.serial
1e620000.fmc:flash@0           1e78f000.serial
1e630000.spi                   ahb
1e630000.spi:flash             ahb:apb
1e660000.ethernet              alarmtimer
1e6c0080.interrupt-controller  i2c-0
1e6e2000.syscon                i2c-1
1e720000.sram                  i2c-2
1e780000.gpio                  i2c-3
1e782000.timer                 i2c-4
1e783000.serial                i2c-5
1e784000.serial                i2c-6
1e785000.wdt                   i2c-7
1e787000.vuart                 i2c-8
1e789140.ibt                   leds
1e78a000.i2c                   serial8250
1e78d000.serial

Signed-off-by: Milton Miller <miltonm@us.ibm.com>
---
 drivers/i2c/busses/i2c-aspeed.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 5488233..ccb438f 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -693,13 +693,6 @@ static int ast_i2c_probe_bus(struct platform_device *pdev)
 	if (ret)
 		return -ENXIO;
 
-	/*
-	 * Set a useful name derived from the bus number; the device tree
-	 * should provide us with one that corresponds to the hardware
-	 * numbering
-	 */
-	dev_set_name(&pdev->dev, "i2c-%d", bus_num);
-
 	bus->pclk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(bus->pclk)) {
 		dev_dbg(&pdev->dev, "clk_get failed\n");
@@ -828,7 +821,26 @@ static int ast_i2c_probe_controller(struct platform_device *pdev)
 			controller->irq);
 
 	for_each_child_of_node(pdev->dev.of_node, np) {
-		of_platform_device_create(np, NULL, &pdev->dev);
+		int ret;
+		u32 bus_num;
+		char bus_id[sizeof("i2c-12345")];
+
+		/*
+		 * Set a useful name derived from the bus number; the device
+		 * tree should provide us with one that corresponds to the
+		 * hardware numbering.  If the property is missing the
+		 * probe would fail so just skip it here.
+		 */
+
+		ret = of_property_read_u32(np, "bus", &bus_num);
+		if (ret)
+			continue;
+
+		ret = snprintf(bus_id, sizeof(bus_id), "i2c-%u", bus_num);
+		if (ret >= sizeof(bus_id))
+			continue;
+
+		of_platform_device_create(np, bus_id, &pdev->dev);
 		of_node_put(np);
 	}
 
-- 
2.7.1

[PATCH linux 2/2] arm: dts: unit address should not have 0x prefix

[OpenBMC Patches]

From: "Milton D. Miller II" <miltonm@us.ibm.com>

The implied format for most bus bindings implies the uname-address
part of the unit-name should be in hex without a leading 0x.

Fix the one i2c bus that did not follow this convention.

Signed-off-by: Milton Miller <miltonm@us.ibm.com>
---
 arch/arm/boot/dts/ast2400.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/ast2400.dtsi b/arch/arm/boot/dts/ast2400.dtsi
index de816a9..49516fd 100644
--- a/arch/arm/boot/dts/ast2400.dtsi
+++ b/arch/arm/boot/dts/ast2400.dtsi
@@ -159,7 +159,7 @@
 					interrupts = <4>;
 				};
 
-				i2c5: i2c-bus@0x180 {
+				i2c5: i2c-bus@180 {
 					#address-cells = <1>;
 					#size-cells = <0>;
 					reg = <0x180 0x40>;
-- 
2.7.1

Re: [PATCH linux 2/2] arm: dts: unit address should not have 0x prefix

[Joel Stanley]

On Thu, Feb 11, 2016 at 12:00 PM, OpenBMC Patches
<openbmc-patches@stwcx.xyz> wrote:
> From: "Milton D. Miller II" <miltonm@us.ibm.com>
>
> The implied format for most bus bindings implies the uname-address
> part of the unit-name should be in hex without a leading 0x.
>
> Fix the one i2c bus that did not follow this convention.
>
> Signed-off-by: Milton Miller <miltonm@us.ibm.com>

Thanks, applied.

> ---
>  arch/arm/boot/dts/ast2400.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/ast2400.dtsi b/arch/arm/boot/dts/ast2400.dtsi
> index de816a9..49516fd 100644
> --- a/arch/arm/boot/dts/ast2400.dtsi
> +++ b/arch/arm/boot/dts/ast2400.dtsi
> @@ -159,7 +159,7 @@
>                                         interrupts = <4>;
>                                 };
>
> -                               i2c5: i2c-bus@0x180 {
> +                               i2c5: i2c-bus@180 {
>                                         #address-cells = <1>;
>                                         #size-cells = <0>;
>                                         reg = <0x180 0x40>;
> --
> 2.7.1
>
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc

Re: [PATCH linux 1/2] i2c: aspeed: Don't rename added devices

[Joel Stanley]

On Thu, Feb 11, 2016 at 12:00 PM, OpenBMC Patches
<openbmc-patches@stwcx.xyz> wrote:
> From: "Milton D. Miller II" <miltonm@us.ibm.com>
>
> Changing the name of a device with dev_set_name after device_add
> is not allowed.
>
> However, of_create_device takes a bus_id parameter that specifies
> the name of the platform device being added.
>
> This does mean the bus property has to be retrieved multiple times.
> Creation is skiped if there is trouble retrieving or formatting
> the name.

Ok, this approach looks alright.

> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 5488233..ccb438f 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -693,13 +693,6 @@ static int ast_i2c_probe_bus(struct platform_device *pdev)
>         if (ret)
>                 return -ENXIO;
>
> -       /*
> -        * Set a useful name derived from the bus number; the device tree
> -        * should provide us with one that corresponds to the hardware
> -        * numbering
> -        */
> -       dev_set_name(&pdev->dev, "i2c-%d", bus_num);
> -
>         bus->pclk = devm_clk_get(&pdev->dev, NULL);
>         if (IS_ERR(bus->pclk)) {
>                 dev_dbg(&pdev->dev, "clk_get failed\n");
> @@ -828,7 +821,26 @@ static int ast_i2c_probe_controller(struct platform_device *pdev)
>                         controller->irq);
>
>         for_each_child_of_node(pdev->dev.of_node, np) {
> -               of_platform_device_create(np, NULL, &pdev->dev);
> +               int ret;
> +               u32 bus_num;
> +               char bus_id[sizeof("i2c-12345")];

Clever, but I don't think it's kernel style. A constant will do.

> +
> +               /*
> +                * Set a useful name derived from the bus number; the device
> +                * tree should provide us with one that corresponds to the
> +                * hardware numbering.  If the property is missing the
> +                * probe would fail so just skip it here.
> +                */
> +
> +               ret = of_property_read_u32(np, "bus", &bus_num);
> +               if (ret)
> +                       continue;
> +
> +               ret = snprintf(bus_id, sizeof(bus_id), "i2c-%u", bus_num);
> +               if (ret >= sizeof(bus_id))
> +                       continue;

You should check for a negative value as well.

> +
> +               of_platform_device_create(np, bus_id, &pdev->dev);
>                 of_node_put(np);
>         }
>
> --
> 2.7.1
>
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc

Re: [PATCH linux 1/2] i2c: aspeed: Don't rename added devices

[Milton Miller II]

On 02/11/2016 10:23PM, Joel Stanley wrote:
>Subject: Re: [PATCH linux 1/2] i2c: aspeed: Don't rename added devices
>
>On Thu, Feb 11, 2016 at 12:00 PM, OpenBMC Patches
<openbmc-patches@stwcx.xyz> wrote:
>> From: "Milton D. Miller II" <miltonm@us.ibm.com>
>>
>> Changing the name of a device with dev_set_name after device_add
>> is not allowed.
>>
>> However, of_create_device takes a bus_id parameter that specifies
>> the name of the platform device being added.
>>
>> This does mean the bus property has to be retrieved multiple times.
>> Creation is skiped if there is trouble retrieving or formatting
>> the name.

Spell checking this pointed out skipped above is missing a p in the
commit message.

>
> Ok, this approach looks alright.
>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index 5488233..ccb438f 100644
>>         for_each_child_of_node(pdev->dev.of_node, np) {
>> -               of_platform_device_create(np, NULL, &pdev->dev);
>> +               int ret;
>> +               u32 bus_num;
>> +               char bus_id[sizeof("i2c-12345")];
>
> Clever, but I don't think it's kernel style. A constant will do.

git grep  'sizeof("'  | wc -l

yields 235 occurrences, 94 with an additional \[ in front.  Including 
well maintained sections of the kernel like vsnprintf for uuid length 
and network code for formatting ip addresses.

I admit the 5 digit bus number is a bit arbitrary, but I think the origin of 
a constant will be less clear than this code.

>> +
>> +               ret = snprintf(bus_id, sizeof(bus_id), "i2c-%u", bus_num);
>> +               if (ret >= sizeof(bus_id))
>> +                       continue;
>
> You should check for a negative value as well.

It's defined to return "the number of characters which would be written to the buffer", 
not sure how that can ever be < 0 other than the undefined overflow of the int type. 
vscnprintf doesn't consider a need to check for < 0.  The size_t input of the underlying 
vsnprintf is checked vs INT_MAX and returns 0 in that case.

Can you point to other kernel code that checks printf family for returns < 0 ?

Thanks,
milton

Re: [PATCH linux 1/2] i2c: aspeed: Don't rename added devices

[Joel Stanley]

On Sun, Feb 14, 2016 at 11:05 AM, Milton Miller II <miltonm@us.ibm.com> wrote:
>
>
>
> On 02/11/2016 10:23PM, Joel Stanley wrote:
>>Subject: Re: [PATCH linux 1/2] i2c: aspeed: Don't rename added devices
>>
>>On Thu, Feb 11, 2016 at 12:00 PM, OpenBMC Patches
> <openbmc-patches@stwcx.xyz> wrote:
>>> From: "Milton D. Miller II" <miltonm@us.ibm.com>
>>>
>>> Changing the name of a device with dev_set_name after device_add
>>> is not allowed.
>>>
>>> However, of_create_device takes a bus_id parameter that specifies
>>> the name of the platform device being added.
>>>
>>> This does mean the bus property has to be retrieved multiple times.
>>> Creation is skiped if there is trouble retrieving or formatting
>>> the name.
>
> Spell checking this pointed out skipped above is missing a p in the
> commit message.
>
>>
>> Ok, this approach looks alright.
>>
>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>> index 5488233..ccb438f 100644
>>>         for_each_child_of_node(pdev->dev.of_node, np) {
>>> -               of_platform_device_create(np, NULL, &pdev->dev);
>>> +               int ret;
>>> +               u32 bus_num;
>>> +               char bus_id[sizeof("i2c-12345")];
>>
>> Clever, but I don't think it's kernel style. A constant will do.
>
> git grep  'sizeof("'  | wc -l
>
> yields 235 occurrences, 94 with an additional \[ in front.  Including
> well maintained sections of the kernel like vsnprintf for uuid length
> and network code for formatting ip addresses.
>
> I admit the 5 digit bus number is a bit arbitrary, but I think the origin of
> a constant will be less clear than this code.

I was referring to doing sizeof("constant string").

>
>>> +
>>> +               ret = snprintf(bus_id, sizeof(bus_id), "i2c-%u", bus_num);
>>> +               if (ret >= sizeof(bus_id))
>>> +                       continue;
>>
>> You should check for a negative value as well.
>
> It's defined to return "the number of characters which would be written to the buffer",
> not sure how that can ever be < 0 other than the undefined overflow of the int type.
> vscnprintf doesn't consider a need to check for < 0.  The size_t input of the underlying
> vsnprintf is checked vs INT_MAX and returns 0 in that case.
>
> Can you point to other kernel code that checks printf family for returns < 0 ?

I was thinking of the userspace man page which suggests snprintf
returns a negative value on error. You're right; the kernel
implementation doesn't ever do this.