From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752871AbcLLPRx (ORCPT ); Mon, 12 Dec 2016 10:17:53 -0500 Received: from mail.kernel.org ([198.145.29.136]:56064 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752207AbcLLPRv (ORCPT ); Mon, 12 Dec 2016 10:17:51 -0500 MIME-Version: 1.0 In-Reply-To: <584B442D.4020104@gmail.com> References: <1479396775-32033-1-git-send-email-sudeep.holla@arm.com> <20161118144651.275xz4gu6jaefhp7@rob-hp-laptop> <582F5DC0.4080804@gmail.com> <5834921F.2020809@gmail.com> <195d492b-c674-e096-4f84-d37ca5448db2@arm.com> <584B442D.4020104@gmail.com> From: Rob Herring Date: Mon, 12 Dec 2016 09:17:27 -0600 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 1/2] of: base: add support to get machine model name To: Frank Rowand Cc: Sudeep Holla , "linux-kernel@vger.kernel.org" , Arnd Bergmann , "devicetree@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Dec 9, 2016 at 5:54 PM, Frank Rowand wrote: > On 12/09/16 08:03, Rob Herring wrote: >> On Wed, Nov 23, 2016 at 4:25 AM, Sudeep Holla wrote: >>> >>> >>> On 22/11/16 21:35, Rob Herring wrote: >>>> >>>> On Tue, Nov 22, 2016 at 12:44 PM, Frank Rowand >>>> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rob Herring Subject: Re: [PATCH 1/2] of: base: add support to get machine model name Date: Mon, 12 Dec 2016 09:17:27 -0600 Message-ID: References: <1479396775-32033-1-git-send-email-sudeep.holla@arm.com> <20161118144651.275xz4gu6jaefhp7@rob-hp-laptop> <582F5DC0.4080804@gmail.com> <5834921F.2020809@gmail.com> <195d492b-c674-e096-4f84-d37ca5448db2@arm.com> <584B442D.4020104@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: <584B442D.4020104-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Frank Rowand Cc: Sudeep Holla , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Arnd Bergmann , "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" List-Id: devicetree@vger.kernel.org On Fri, Dec 9, 2016 at 5:54 PM, Frank Rowand wrote: > On 12/09/16 08:03, Rob Herring wrote: >> On Wed, Nov 23, 2016 at 4:25 AM, Sudeep Holla wrote: >>> >>> >>> On 22/11/16 21:35, Rob Herring wrote: >>>> >>>> On Tue, Nov 22, 2016 at 12:44 PM, Frank Rowand >>>> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html