Re: [PATCH 1/2] of: base: add support to get machine model name

[Frank Rowand]

Hi Rob,

On 11/18/16 12:00, Frank Rowand wrote:
> On 11/18/16 06:46, Rob Herring wrote:
>> On Thu, Nov 17, 2016 at 03:32:54PM +0000, Sudeep Holla wrote:
>>> Currently platforms/drivers needing to get the machine model name are
>>> replicating the same snippet of code. In some case, the OF reference
>>> counting is either missing or incorrect.
>>>
>>> This patch adds support to read the machine model name either using
>>> the "model" or the "compatible" property in the device tree root node
>>> to the core OF/DT code.
>>>
>>> This can be used to remove all the duplicate code snippets doing exactly
>>> same thing later.
>>>
>>> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
>>> Cc: Frank Rowand <frowand.list@xxxxxxxxx>
>>> Cc: Arnd Bergmann <arnd@xxxxxxxx>
>>> Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx>
>>> ---
>>>  drivers/of/base.c  | 32 ++++++++++++++++++++++++++++++++
>>>  include/linux/of.h |  6 ++++++
>>>  2 files changed, 38 insertions(+)
>>>
>>> Hi Rob,
>>>
>>> It would be good if we can target this for v4.10, so that we have no
>>> dependencies to push PATCH 2/2 in v4.11
>>
>> Applied.
>>
>> Rob
>>
> 
> A little fast on the trigger Rob.
> 
> -Frank

This patch adds a function that leads to conflating the "model" property
and the "compatible" property. This leads to opaque, confusing and unclear
code where ever it is used.   I think it is not good for the device tree
framework to contribute to writing unclear code.

Further, only two of the proposed users of this new function appear to
be proper usage.  I do not think that the small amount of reduced lines
of code is a good trade off for the reduced code clarity and for the
potential for future mis-use of this function.

Can I convince you to revert this patch?

If not, will you accept a patch to change the function name to more
clearly indicate what it does?  (One possible name would be
of_model_or_1st_compatible().)

-Frank