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

[Frank Rowand]

On 12/09/16 08:03, Rob Herring wrote:
> On Wed, Nov 23, 2016 at 4:25 AM, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
>>
>>
>> On 22/11/16 21:35, Rob Herring wrote:
>>>
>>> On Tue, Nov 22, 2016 at 12:44 PM, Frank Rowand <frowand.list@xxxxxxxxx>
>>> wrote:
>>
>>
>> [...]
>>
>>>>
>>>> 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.
> 
> I looked at this again and the users. They are all informational, so

A comment in the function docbook header stating that the intent of the
returned value is for informational use only would make me happy.

There is at least on proposed use in patch 2/2 that is not just
informational.  init_octeon_system_type() sometimes uses the value of
the model property to create the value of variable octeon_system_type.
octeon_pcie_pcibios_map_irq() checks the value of octeon_system_type
(via the function octeon_board_type_string()) to determine whether
to apply a fixup:

int __init octeon_pcie_pcibios_map_irq(const struct pci_dev *dev,
                                       u8 slot, u8 pin)
{
        /*
         * The EBH5600 board with the PCI to PCIe bridge mistakenly
         * wires the first slot for both device id 2 and interrupt
         * A. According to the PCI spec, device id 2 should be C. The
         * following kludge attempts to fix this.
         */
        if (strstr(octeon_board_type_string(), "EBH5600") &&
            dev->bus && dev->bus->parent) {


> I'm not worried if a compatible string could be returned with this
> change. The function returns the best name for the machine and having
> consistency is a good thing.

> 
> I was considering not reverting (as I'd not yet gotten around to it),
> but I'm still going to revert for the naming.
> 
>>>
>>>> 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.
>>
>>
>> Yes it was mainly for non of_flat_* replacement for
>> of_flat_dt_get_machine_name
> 
> I would suggest just of_get_machine_name().
> 
> You might also add a fallback to return "unknown", and drop some of
> the custom strings. I don't think anyone should care about the actual
> string. However, it's an error to have a DT with no model or top level
> compatible, so maybe a WARN.

The name and other suggestions sound fine to me.

-Frank

> 
> Rob
>