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. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html