[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 at stwcx.xyz> wrote:
>> From: "Milton D. Miller II" <miltonm at 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