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

[Rob Herring]

On Tue, Nov 22, 2016 at 12:44 PM, Frank Rowand <frowand.list@xxxxxxxxx> wrote:
> 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?

Yes, I will revert.

> 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().)

I took it as there's already the FDT equivalent function. I don't have
an issue with the name as the purpose is to get the best name string
for the machine which is model if present and most specific compatible
if not. However, any use of it beyond informational purpose is wrong.
For matching purposes, only compatible should be used.

Rob