Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1539453 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] of: base: add support to get machine model name Rob Herring <robh@kernel.org> - 2016-12-09 17:10 +0100
    Re: [PATCH 1/2] of: base: add support to get machine model name Frank Rowand <frowand.list@gmail.com> - 2016-12-10 01:00 +0100
      Re: [PATCH 1/2] of: base: add support to get machine model name Rob Herring <robh@kernel.org> - 2016-12-12 16:20 +0100

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

On Wed, Nov 23, 2016 at 4:25 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 22/11/16 21:35, Rob Herring wrote:
>>
>> On Tue, Nov 22, 2016 at 12:44 PM, Frank Rowand <frowand.list@gmail.com>
>> 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
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.

Rob

[toc] | [next] | [standalone]

#1539739

On 12/09/16 08:03, Rob Herring wrote:
> On Wed, Nov 23, 2016 at 4:25 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>>
>> On 22/11/16 21:35, Rob Herring wrote:
>>>
>>> On Tue, Nov 22, 2016 at 12:44 PM, Frank Rowand <frowand.list@gmail.com>
>>> 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
> 

[toc] | [prev] | [next] | [standalone]

#1540380

On Fri, Dec 9, 2016 at 5:54 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 12/09/16 08:03, Rob Herring wrote:
>> On Wed, Nov 23, 2016 at 4:25 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>
>>>
>>> On 22/11/16 21:35, Rob Herring wrote:
>>>>
>>>> On Tue, Nov 22, 2016 at 12:44 PM, Frank Rowand <frowand.list@gmail.com>
>>>> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web