Re: [PATCH 3/5] of: Only call notifiers when node is attached

[Grant Likely <grant.likely@secretlab.ca>]

On Thu, Nov 6, 2014 at 12:33 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Grant,
>
>> On Nov 5, 2014, at 23:39 , Grant Likely <grant.likely@secretlab.ca> wrote:
>>
>> On Tue, 28 Oct 2014 22:33:51 +0200
>> , Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> wrote:
>>> Make sure we call notifier only when the node is attached.
>>> When a detatched tree is being constructed we do not want the
>>> notifiers to fire at all.
>>
>> The description does not match what the patch does. The patch moves the
>> test into of_{add,remove,update}_property() and out of
>> of_property_notify() itself. That leaves one other caller of
>> of_property_notify(); __of_changeset_entry_notify(). The effect of this
>> patch is that applying a changeset will cause notifiers to be fired for
>> each property modified in a changeset. The comment says nothing about
>> the change in behaviour and it sounds like it is a bug fix when it
>> doesn't actually change the behaviour at all for the
>> of_{add,remove,update}_property() paths.
>>
>> This needs a better changelog. It needs to describe what the effects of
>> the patch are and why the change is being made. When someone is
>> bisecting a problem and they land on this change, the changelog needs to
>> give them a good idea about what is going on and why.
>>
>
> Valid points. In fact I performed some tests and with this reverted things
> still work.
>
> The rationale behind this is for when nodes/properties are removed in the overlay,
> but since we don't support this for now, we never hit the case where it was
> needed.
>
> Please remove from the patch series, I'll revisit this when I add the removal
> functionality.

Cool, thanks.

g.