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

From: Rob Herring
Date: Mon Dec 12 2016 - 10:17:55 EST

On Fri, Dec 9, 2016 at 5:54 PM, Frank Rowand <frowand.list@xxxxxxxxx> wrote:
> 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) {

True, it is more than informational, but let's think about what would
have to happen for the change here to matter. We would have to have a
board with no model property. Then we'd have to have a compatible
string of "EBH5600" on a board which is not the same one as model
EBH5600 and wouldn't be valid anyway with no vendor prefix. IMO, this
code should be using of_machine_is_compatible() anyway. MIPS SoC and
board code is a mess anyway. Linus needs to yell at them.