On 11/14/16 03:04, Hans de Goede wrote:
> Hi,
>
> On 14-11-16 08:34, Frank Rowand wrote:
>> Hi Hans, Pantelis,
>>
>> On 11/12/16 18:15, Frank Rowand wrote:
>>> On 11/04/16 07:42, Hans de Goede wrote:
>>>> From: Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
>>>>
>>>> Changesets are very powerful, but the lack of a helper API
>>>> makes using them cumbersome. Introduce a simple copy based
>>>> API that makes things considerably easier.
>>>>
>>>> To wit, adding a property using the raw API.
>>>>
>>>> struct property *prop;
>>>> prop = kzalloc(sizeof(*prop)), GFP_KERNEL);
>>>> prop->name = kstrdup("compatible");
>>>> prop->value = kstrdup("foo,bar");
>>>> prop->length = strlen(prop->value) + 1;
>>>> of_changeset_add_property(ocs, np, prop);
>>>>
>>>> while using the helper API
>>>>
>>>> of_changeset_add_property_string(ocs, np, "compatible",
>>>> "foo,bar");
>>>>
>>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
>>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>>>> ---
>>>> Changes in v2 (hdegoede@xxxxxxxxxx):
>>>> -Address review comments from:
>>>> https://www.spinics.net/lists/kernel/msg2252845.html
>>>
>>> That points to the May 9 version 1 patches from Pantelis (as expected),
>>> but containing 4, not 2, patches. Patch 1/4 was applied. Patch 4/4
>>> seems to have disappeared?
>>>
>>> Pantelis then sent a version 2 set of the patches on May 16.
>>>
>>> Your version is a modification of the May 9 patches (as would be expected
>>> of a version 2). It is confusing to have two different version 2 patch
>>> sets. I don't have any brilliant ideas on how this patch set could have
>>> been named differently to avoid that confusion.
>>>
>>> The point of this little side-track is simply to note the existence of two
>>> different version 2 series so I won't be confused when I revisit this
>>> thread in the future.
>>>
>>>> -Simplify (and fix) __of_changeset_add_update_property_copy OOM handling
>>>> -Remove (by manual inlining) these 2 static helpers:
>>>> __of_changeset_add_update_property_u32
>>>> __of_changeset_add_update_property_bool
>>>> -Remove the following exported helper method:
>>>> of_changeset_node_move_to
>>>
>>> Not all comments were addressed.
>>>
>>> There are some other changes made that are not noted in the changelog.
>>>
>>> I am still reading through the patches. I will reply again either with
>>> a reviewed-by or specific comments when I finish.
>>
>> Replying here for the entire patchset (there was no patch 0 to reply to).
>>
>> After reading through the patches, my reply is meta instead of specific
>> comments about the code.
>>
>> There are very few users of change sets in tree. I do not see the need to
>> add these helpers until such users are likely to appear.
>>
>> I would expect change sets to be _mostly_ used internally by the device tree
>> overlay framework, not directly by drivers. If change sets are an attractive
>> technology for drivers, I want to approach that usage very carefully to avoid
>> inappropriate use, which could be very difficult to reign in after the fact.
>>
>> Even if helpers should be added, this seems to be an overly complex approach.
>> If the need for these helpers becomes apparent I can provide review comments
>> with the specifics about how it appears to be overly complex.
>>
>> Can you please provide some more insights into the needs driving the desire
>> to have change set helpers and the expected use cases of them? Please put
>> your architect's hat on when replying to this question.
>
> My use case for this is discussed in this thread:
> https://www.spinics.net/lists/arm-kernel/msg536111.html
>
> With the dt-bindings for the hardware-manager I want to add here:
> https://www.spinics.net/lists/arm-kernel/msg536109.html
>
> Note that there is a lot of discussion in this thread whether or
> not this belongs in the kernel. I strongly believe though that
> some functionality like this will be needed in the kernel for
> ARM+dt devices going forward, just like there is plenty of x86
> code which adjusts itself to specific hardware, because whether
> we like it or not hardware (revisions) will always have quirks.
Thanks! That context should have been provided with the patches.
The use case discussion is important and I am paying a lot of
attention to that discussion and many other discussions about
dynamic device trees. I don't think it makes sense to apply the
change set helper patches yet, given the unsettled state of the
various dynamic device tree discussions.
-Frank
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
