| Message ID | 1479396775-32033-1-git-send-email-sudeep.holla@arm.com |
|---|---|
| State | Accepted |
| Commit | e5269794d2e9046dd45be15bdb213a229df46b7e |
| Headers | show |
On 11/17/16 07:32, Sudeep Holla wrote: > Currently platforms/drivers needing to get the machine model name are > replicating the same snippet of code. In some case, the OF reference > counting is either missing or incorrect. > > This patch adds support to read the machine model name either using > the "model" or the "compatible" property in the device tree root node > to the core OF/DT code. > > This can be used to remove all the duplicate code snippets doing exactly > same thing later. I find five instances of reading only property "model": arch/arm/mach-imx/cpu.c arch/arm/mach-mxs/mach-mxs.c arch/c6x/kernel/setup.c arch/mips/cavium-octeon/setup.c arch/sh/boards/of-generic.c I find one instance of reading property "model", then if that does not exist, property "compatible": arch/mips/generic/proc.c The proposed patch matches the code used in one place, and thus current usage does not match the patch description. Is my search bad? Are you planning to add additional instances of reading "model" then "compatible"? -Frank > > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Frank Rowand <frowand.list@gmail.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/of/base.c | 32 ++++++++++++++++++++++++++++++++ > include/linux/of.h | 6 ++++++ > 2 files changed, 38 insertions(+) > > Hi Rob, > > It would be good if we can target this for v4.10, so that we have no > dependencies to push PATCH 2/2 in v4.11 > > Regards, > Sudeep > > diff --git a/drivers/of/base.c b/drivers/of/base.c > index a0bccb54a9bd..0810c5ecf1aa 100644 > --- a/drivers/of/base.c > +++ b/drivers/of/base.c > @@ -546,6 +546,38 @@ int of_machine_is_compatible(const char *compat) > EXPORT_SYMBOL(of_machine_is_compatible); > > /** > + * of_machine_get_model_name - Find and read the model name or the compatible > + * value for the machine. > + * @model: pointer to null terminated return string, modified only if > + * return value is 0. > + * > + * Returns a string containing either the model name or the compatible value > + * of the machine if found, else return error. > + * > + * Search for a machine model name or the compatible if model name is missing > + * in a device tree node and retrieve a null terminated string value (pointer > + * to data, not a copy). Returns 0 on success, -EINVAL if root of the device > + * tree is not found and other error returned by of_property_read_string on > + * failure. > + */ > +int of_machine_get_model_name(const char **model) > +{ > + int error; > + > + if (!of_node_get(of_root)) > + return -EINVAL; > + > + error = of_property_read_string(of_root, "model", model); > + if (error) > + error = of_property_read_string_index(of_root, "compatible", > + 0, model); > + of_node_put(of_root); > + > + return error; > +} > +EXPORT_SYMBOL(of_machine_get_model_name); > + > +/** > * __of_device_is_available - check if a device is available for use > * > * @device: Node to check for availability, with locks already held > diff --git a/include/linux/of.h b/include/linux/of.h > index d72f01009297..13fc66531f1b 100644 > --- a/include/linux/of.h > +++ b/include/linux/of.h > @@ -367,6 +367,7 @@ extern int of_alias_get_id(struct device_node *np, const char *stem); > extern int of_alias_get_highest_id(const char *stem); > > extern int of_machine_is_compatible(const char *compat); > +extern int of_machine_get_model_name(const char **model); > > extern int of_add_property(struct device_node *np, struct property *prop); > extern int of_remove_property(struct device_node *np, struct property *prop); > @@ -788,6 +789,11 @@ static inline int of_machine_is_compatible(const char *compat) > return 0; > } > > +static inline int of_machine_get_model_name(const char **model) > +{ > + return -EINVAL; > +} > + > static inline bool of_console_check(const struct device_node *dn, const char *name, int index) > { > return false; > -- > 2.7.4 > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/17/16 13:00, Frank Rowand wrote: > On 11/17/16 07:32, Sudeep Holla wrote: >> Currently platforms/drivers needing to get the machine model name are >> replicating the same snippet of code. In some case, the OF reference >> counting is either missing or incorrect. >> >> This patch adds support to read the machine model name either using >> the "model" or the "compatible" property in the device tree root node >> to the core OF/DT code. >> >> This can be used to remove all the duplicate code snippets doing exactly >> same thing later. > > I find five instances of reading only property "model": > > arch/arm/mach-imx/cpu.c > arch/arm/mach-mxs/mach-mxs.c > arch/c6x/kernel/setup.c > arch/mips/cavium-octeon/setup.c > arch/sh/boards/of-generic.c My initial search was a little too strict. With a less restrictive search I find 16 more instances of reading property "model" and not reading property "compatible". > > I find one instance of reading property "model", then if > that does not exist, property "compatible": > > arch/mips/generic/proc.c > > The proposed patch matches the code used in one place, and thus > current usage does not match the patch description. > > Is my search bad? Are you planning to add additional instances > of reading "model" then "compatible"? > > -Frank > >> >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: Frank Rowand <frowand.list@gmail.com> >> Cc: Arnd Bergmann <arnd@arndb.de> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> drivers/of/base.c | 32 ++++++++++++++++++++++++++++++++ >> include/linux/of.h | 6 ++++++ >> 2 files changed, 38 insertions(+) >> >> Hi Rob, >> >> It would be good if we can target this for v4.10, so that we have no >> dependencies to push PATCH 2/2 in v4.11 >> >> Regards, >> Sudeep >> >> diff --git a/drivers/of/base.c b/drivers/of/base.c >> index a0bccb54a9bd..0810c5ecf1aa 100644 >> --- a/drivers/of/base.c >> +++ b/drivers/of/base.c >> @@ -546,6 +546,38 @@ int of_machine_is_compatible(const char *compat) >> EXPORT_SYMBOL(of_machine_is_compatible); >> >> /** >> + * of_machine_get_model_name - Find and read the model name or the compatible >> + * value for the machine. >> + * @model: pointer to null terminated return string, modified only if >> + * return value is 0. >> + * >> + * Returns a string containing either the model name or the compatible value >> + * of the machine if found, else return error. >> + * >> + * Search for a machine model name or the compatible if model name is missing >> + * in a device tree node and retrieve a null terminated string value (pointer >> + * to data, not a copy). Returns 0 on success, -EINVAL if root of the device >> + * tree is not found and other error returned by of_property_read_string on >> + * failure. >> + */ >> +int of_machine_get_model_name(const char **model) >> +{ >> + int error; >> + >> + if (!of_node_get(of_root)) >> + return -EINVAL; >> + >> + error = of_property_read_string(of_root, "model", model); >> + if (error) >> + error = of_property_read_string_index(of_root, "compatible", >> + 0, model); >> + of_node_put(of_root); >> + >> + return error; >> +} >> +EXPORT_SYMBOL(of_machine_get_model_name); >> + >> +/** >> * __of_device_is_available - check if a device is available for use >> * >> * @device: Node to check for availability, with locks already held >> diff --git a/include/linux/of.h b/include/linux/of.h >> index d72f01009297..13fc66531f1b 100644 >> --- a/include/linux/of.h >> +++ b/include/linux/of.h >> @@ -367,6 +367,7 @@ extern int of_alias_get_id(struct device_node *np, const char *stem); >> extern int of_alias_get_highest_id(const char *stem); >> >> extern int of_machine_is_compatible(const char *compat); >> +extern int of_machine_get_model_name(const char **model); >> >> extern int of_add_property(struct device_node *np, struct property *prop); >> extern int of_remove_property(struct device_node *np, struct property *prop); >> @@ -788,6 +789,11 @@ static inline int of_machine_is_compatible(const char *compat) >> return 0; >> } >> >> +static inline int of_machine_get_model_name(const char **model) >> +{ >> + return -EINVAL; >> +} >> + >> static inline bool of_console_check(const struct device_node *dn, const char *name, int index) >> { >> return false; >> -- >> 2.7.4 >> >> > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17/11/16 21:00, Frank Rowand wrote: > On 11/17/16 07:32, Sudeep Holla wrote: >> Currently platforms/drivers needing to get the machine model name are >> replicating the same snippet of code. In some case, the OF reference >> counting is either missing or incorrect. >> >> This patch adds support to read the machine model name either using >> the "model" or the "compatible" property in the device tree root node >> to the core OF/DT code. >> >> This can be used to remove all the duplicate code snippets doing exactly >> same thing later. > > I find five instances of reading only property "model": > > arch/arm/mach-imx/cpu.c > arch/arm/mach-mxs/mach-mxs.c > arch/c6x/kernel/setup.c > arch/mips/cavium-octeon/setup.c > arch/sh/boards/of-generic.c > Ah sorry you were not Cc-ed in 2/2, but that shows all the instances that this will be used for. > I find one instance of reading property "model", then if > that does not exist, property "compatible": > > arch/mips/generic/proc.c > Correct as you can check in patch 2/2 > The proposed patch matches the code used in one place, and thus > current usage does not match the patch description. > Yes, but does it matter ? compatibles are somewhat informative about the model IMO. > Is my search bad? Are you planning to add additional instances > of reading "model" then "compatible"? > No, just replacing the existing ones as in patch 2/2 -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/18/16 06:46, Rob Herring wrote: > On Thu, Nov 17, 2016 at 03:32:54PM +0000, Sudeep Holla wrote: >> Currently platforms/drivers needing to get the machine model name are >> replicating the same snippet of code. In some case, the OF reference >> counting is either missing or incorrect. >> >> This patch adds support to read the machine model name either using >> the "model" or the "compatible" property in the device tree root node >> to the core OF/DT code. >> >> This can be used to remove all the duplicate code snippets doing exactly >> same thing later. >> >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: Frank Rowand <frowand.list@gmail.com> >> Cc: Arnd Bergmann <arnd@arndb.de> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> drivers/of/base.c | 32 ++++++++++++++++++++++++++++++++ >> include/linux/of.h | 6 ++++++ >> 2 files changed, 38 insertions(+) >> >> Hi Rob, >> >> It would be good if we can target this for v4.10, so that we have no >> dependencies to push PATCH 2/2 in v4.11 > > Applied. > > Rob > A little fast on the trigger Rob. -Frank -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/18/16 02:41, Sudeep Holla wrote: > > > On 17/11/16 21:00, Frank Rowand wrote: >> On 11/17/16 07:32, Sudeep Holla wrote: >>> Currently platforms/drivers needing to get the machine model name are >>> replicating the same snippet of code. In some case, the OF reference >>> counting is either missing or incorrect. >>> >>> This patch adds support to read the machine model name either using >>> the "model" or the "compatible" property in the device tree root node >>> to the core OF/DT code. >>> >>> This can be used to remove all the duplicate code snippets doing exactly >>> same thing later. >> >> I find five instances of reading only property "model": >> >> arch/arm/mach-imx/cpu.c >> arch/arm/mach-mxs/mach-mxs.c >> arch/c6x/kernel/setup.c >> arch/mips/cavium-octeon/setup.c >> arch/sh/boards/of-generic.c >> > > Ah sorry you were not Cc-ed in 2/2, but that shows all the instances > that this will be used for. I have not seen 2/2. I do not see it on the devicetree list or on lkml. I did see a list of drivers in the RFC patch that you sent several hours before this patch. In that patch you replaced reading the model name from the _flat_ device tree with the new function in at least one location. That is not correct. > >> I find one instance of reading property "model", then if >> that does not exist, property "compatible": >> >> arch/mips/generic/proc.c >> > > Correct as you can check in patch 2/2 > >> The proposed patch matches the code used in one place, and thus >> current usage does not match the patch description. >> > > Yes, but does it matter ? compatibles are somewhat informative about the > model IMO. Yes it does matter. That is just sloppy and makes devicetree yet harder to understand. It hurts clarity. The new function name says get "model", not get "model" or "first element of the compatible list". And using the _first_ element only of the compatible list to determine model is not a good paradigm. It is yet another hidden, special case, undocumented trap to lure in the unwary. It is extremely unlikely that the change actually changes behavior for an existing device tree because there is probably no dts that does not contain the model property but does contain the proper magic value in the compatible property. But did you actually check for that? > >> Is my search bad? Are you planning to add additional instances >> of reading "model" then "compatible"? >> > > No, just replacing the existing ones as in patch 2/2 > You also ignored Arnd's comment in reply to your RFC patch. -Frank -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sudeep, On 11/18/16 12:22, Frank Rowand wrote: > On 11/18/16 02:41, Sudeep Holla wrote: >> >> >> On 17/11/16 21:00, Frank Rowand wrote: >>> On 11/17/16 07:32, Sudeep Holla wrote: >>>> Currently platforms/drivers needing to get the machine model name are >>>> replicating the same snippet of code. In some case, the OF reference >>>> counting is either missing or incorrect. >>>> >>>> This patch adds support to read the machine model name either using >>>> the "model" or the "compatible" property in the device tree root node >>>> to the core OF/DT code. >>>> >>>> This can be used to remove all the duplicate code snippets doing exactly >>>> same thing later. >>> >>> I find five instances of reading only property "model": >>> >>> arch/arm/mach-imx/cpu.c >>> arch/arm/mach-mxs/mach-mxs.c >>> arch/c6x/kernel/setup.c >>> arch/mips/cavium-octeon/setup.c >>> arch/sh/boards/of-generic.c >>> >> >> Ah sorry you were not Cc-ed in 2/2, but that shows all the instances >> that this will be used for. > > I have not seen 2/2. I do not see it on the devicetree list or on lkml. Can you please re-send patch 2/2? -Frank > > I did see a list of drivers in the RFC patch that you sent several hours > before this patch. > > In that patch you replaced reading the model name from the _flat_ device > tree with the new function in at least one location. That is not > correct. > > >> >>> I find one instance of reading property "model", then if >>> that does not exist, property "compatible": >>> >>> arch/mips/generic/proc.c >>> >> >> Correct as you can check in patch 2/2 >> >>> The proposed patch matches the code used in one place, and thus >>> current usage does not match the patch description. >>> >> >> Yes, but does it matter ? compatibles are somewhat informative about the >> model IMO. > > Yes it does matter. That is just sloppy and makes devicetree yet harder > to understand. It hurts clarity. The new function name says get "model", > not get "model" or "first element of the compatible list". > > And using the _first_ element only of the compatible list to determine > model is not a good paradigm. It is yet another hidden, special case, > undocumented trap to lure in the unwary. > > It is extremely unlikely that the change actually changes behavior for an > existing device tree because there is probably no dts that does not > contain the model property but does contain the proper magic value in > the compatible property. But did you actually check for that? > >> >>> Is my search bad? Are you planning to add additional instances >>> of reading "model" then "compatible"? >>> >> >> No, just replacing the existing ones as in patch 2/2 >> > > You also ignored Arnd's comment in reply to your RFC patch. > > -Frank > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21/11/16 16:05, Frank Rowand wrote: > Hi Sudeep, > > On 11/18/16 12:22, Frank Rowand wrote: >> On 11/18/16 02:41, Sudeep Holla wrote: >>> >>> >>> On 17/11/16 21:00, Frank Rowand wrote: >>>> On 11/17/16 07:32, Sudeep Holla wrote: >>>>> Currently platforms/drivers needing to get the machine model name are >>>>> replicating the same snippet of code. In some case, the OF reference >>>>> counting is either missing or incorrect. >>>>> >>>>> This patch adds support to read the machine model name either using >>>>> the "model" or the "compatible" property in the device tree root node >>>>> to the core OF/DT code. >>>>> >>>>> This can be used to remove all the duplicate code snippets doing exactly >>>>> same thing later. >>>> >>>> I find five instances of reading only property "model": >>>> >>>> arch/arm/mach-imx/cpu.c >>>> arch/arm/mach-mxs/mach-mxs.c >>>> arch/c6x/kernel/setup.c >>>> arch/mips/cavium-octeon/setup.c >>>> arch/sh/boards/of-generic.c >>>> >>> >>> Ah sorry you were not Cc-ed in 2/2, but that shows all the instances >>> that this will be used for. >> >> I have not seen 2/2. I do not see it on the devicetree list or on lkml. > > Can you please re-send patch 2/2? > Since it is based on -next, I would prefer to wait until next merge window to resend. You should be able to check in the link I sent if that's OK. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/21/16 08:23, Sudeep Holla wrote: > > > On 21/11/16 16:05, Frank Rowand wrote: >> Hi Sudeep, >> >> On 11/18/16 12:22, Frank Rowand wrote: >>> On 11/18/16 02:41, Sudeep Holla wrote: >>>> >>>> >>>> On 17/11/16 21:00, Frank Rowand wrote: >>>>> On 11/17/16 07:32, Sudeep Holla wrote: >>>>>> Currently platforms/drivers needing to get the machine model name are >>>>>> replicating the same snippet of code. In some case, the OF reference >>>>>> counting is either missing or incorrect. >>>>>> >>>>>> This patch adds support to read the machine model name either using >>>>>> the "model" or the "compatible" property in the device tree root node >>>>>> to the core OF/DT code. >>>>>> >>>>>> This can be used to remove all the duplicate code snippets doing exactly >>>>>> same thing later. >>>>> >>>>> I find five instances of reading only property "model": >>>>> >>>>> arch/arm/mach-imx/cpu.c >>>>> arch/arm/mach-mxs/mach-mxs.c >>>>> arch/c6x/kernel/setup.c >>>>> arch/mips/cavium-octeon/setup.c >>>>> arch/sh/boards/of-generic.c >>>>> >>>> >>>> Ah sorry you were not Cc-ed in 2/2, but that shows all the instances >>>> that this will be used for. >>> >>> I have not seen 2/2. I do not see it on the devicetree list or on lkml. >> >> Can you please re-send patch 2/2? >> > > Since it is based on -next, I would prefer to wait until next merge > window to resend. You should be able to check in the link I sent if > that's OK. I am missing or misunderstanding something. I do not know what "the link I sent" means. For some reason, the devicetree mail list and lmkl mail failed to send me a copy of patch 2/2. Or my mail server failed to receive them. That is why I asked you to resend the patch. I just now looked in the devicetree archive and found it there. So I now can see how you plan to use the new function. -Frank -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/21/16 08:20, Sudeep Holla wrote: > > > On 18/11/16 20:22, Frank Rowand wrote: >> On 11/18/16 02:41, Sudeep Holla wrote: >>> >>> >>> On 17/11/16 21:00, Frank Rowand wrote: >>>> On 11/17/16 07:32, Sudeep Holla wrote: >>>>> Currently platforms/drivers needing to get the machine model name are >>>>> replicating the same snippet of code. In some case, the OF reference >>>>> counting is either missing or incorrect. >>>>> >>>>> This patch adds support to read the machine model name either using >>>>> the "model" or the "compatible" property in the device tree root node >>>>> to the core OF/DT code. >>>>> >>>>> This can be used to remove all the duplicate code snippets doing exactly >>>>> same thing later. >>>> >>>> I find five instances of reading only property "model": >>>> >>>> arch/arm/mach-imx/cpu.c >>>> arch/arm/mach-mxs/mach-mxs.c >>>> arch/c6x/kernel/setup.c >>>> arch/mips/cavium-octeon/setup.c >>>> arch/sh/boards/of-generic.c >>>> >>> >>> Ah sorry you were not Cc-ed in 2/2, but that shows all the instances >>> that this will be used for. >> >> I have not seen 2/2. I do not see it on the devicetree list or on lkml. >> > > Yes on both [1][2] > >> I did see a list of drivers in the RFC patch that you sent several hours >> before this patch. >> >> In that patch you replaced reading the model name from the _flat_ device >> tree with the new function in at least one location. That is not >> correct. >> >> >>> >>>> I find one instance of reading property "model", then if >>>> that does not exist, property "compatible": >>>> >>>> arch/mips/generic/proc.c Just for completeness, now that I have seen patch 2/2, there is a second location that currently uses "compatible" if "model" does not exist: drivers/soc/fsl/guts.c >>>> >>> >>> Correct as you can check in patch 2/2 >>> >>>> The proposed patch matches the code used in one place, and thus >>>> current usage does not match the patch description. >>>> >>> >>> Yes, but does it matter ? compatibles are somewhat informative about the >>> model IMO. >> >> Yes it does matter. That is just sloppy and makes devicetree yet harder >> to understand. It hurts clarity. The new function name says get "model", >> not get "model" or "first element of the compatible list". An example of a function name that would not hurt clarity would be of_model_or_1st_compatible(). >> > > This is a implementation in the Linux and it doesn't change anything in > DT semantics. I am not able to get your concern. The existing code in five locations that patch 2/2 changes only attempt to read the value of property "model". Changing those five locations to use of_machine_get_model_name() results in those locations using the first string of the "compatible" property if "model" does not exist. The value found is potentially used to determine whether to execute model specific code. An example of this is: octeon_pcie_pcibios_map_irq(). Can you guarantee that there is no device tree that does not contain a "model" property in the root node, but does contains a "compatible" property in the root node whose first value is "EBH5600"? I have pasted the relevant code from octeon_pcie_pcibios_map_irq() below for convenient reference: 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) { My point is that it is not possible to review patch 2/2 to verify whether any change in kernel behavior results from the change, because we do not have access to all device tree sources. patch 2/2 is intended to clean up code, not to change behavior. > >> And using the _first_ element only of the compatible list to determine >> model is not a good paradigm. It is yet another hidden, special case, >> undocumented trap to lure in the unwary. >> > > The function is documented and again this doesn't enforce anything in > the bindings. It's just the way it's used by the Linux kernel. > > [...] > >> >> You also ignored Arnd's comment in reply to your RFC patch. >> > > OK, all I can see is that Arnd wanted to reuse of_root, which I did. > Did I miss anything else ? > My mistake, sorry. I misread the patch. -Frank -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/21/16 11:24, Frank Rowand wrote: > On 11/21/16 08:23, Sudeep Holla wrote: >> >> >> On 21/11/16 16:05, Frank Rowand wrote: >>> Hi Sudeep, >>> >>> On 11/18/16 12:22, Frank Rowand wrote: >>>> On 11/18/16 02:41, Sudeep Holla wrote: >>>>> >>>>> >>>>> On 17/11/16 21:00, Frank Rowand wrote: >>>>>> On 11/17/16 07:32, Sudeep Holla wrote: >>>>>>> Currently platforms/drivers needing to get the machine model name are >>>>>>> replicating the same snippet of code. In some case, the OF reference >>>>>>> counting is either missing or incorrect. >>>>>>> >>>>>>> This patch adds support to read the machine model name either using >>>>>>> the "model" or the "compatible" property in the device tree root node >>>>>>> to the core OF/DT code. >>>>>>> >>>>>>> This can be used to remove all the duplicate code snippets doing exactly >>>>>>> same thing later. >>>>>> >>>>>> I find five instances of reading only property "model": >>>>>> >>>>>> arch/arm/mach-imx/cpu.c >>>>>> arch/arm/mach-mxs/mach-mxs.c >>>>>> arch/c6x/kernel/setup.c >>>>>> arch/mips/cavium-octeon/setup.c >>>>>> arch/sh/boards/of-generic.c >>>>>> >>>>> >>>>> Ah sorry you were not Cc-ed in 2/2, but that shows all the instances >>>>> that this will be used for. >>>> >>>> I have not seen 2/2. I do not see it on the devicetree list or on lkml. >>> >>> Can you please re-send patch 2/2? >>> >> >> Since it is based on -next, I would prefer to wait until next merge >> window to resend. You should be able to check in the link I sent if >> that's OK. > > I am missing or misunderstanding something. > > I do not know what "the link I sent" means. Ah, the links were in the email you sent before this one, but I read this one first. Got it now. > > For some reason, the devicetree mail list and lmkl mail failed to send > me a copy of patch 2/2. Or my mail server failed to receive them. That > is why I asked you to resend the patch. I just now looked in the devicetree > archive and found it there. > > So I now can see how you plan to use the new function. > > -Frank > > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rob, On 11/18/16 12:00, Frank Rowand wrote: > On 11/18/16 06:46, Rob Herring wrote: >> On Thu, Nov 17, 2016 at 03:32:54PM +0000, Sudeep Holla wrote: >>> Currently platforms/drivers needing to get the machine model name are >>> replicating the same snippet of code. In some case, the OF reference >>> counting is either missing or incorrect. >>> >>> This patch adds support to read the machine model name either using >>> the "model" or the "compatible" property in the device tree root node >>> to the core OF/DT code. >>> >>> This can be used to remove all the duplicate code snippets doing exactly >>> same thing later. >>> >>> Cc: Rob Herring <robh+dt@kernel.org> >>> Cc: Frank Rowand <frowand.list@gmail.com> >>> Cc: Arnd Bergmann <arnd@arndb.de> >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >>> --- >>> drivers/of/base.c | 32 ++++++++++++++++++++++++++++++++ >>> include/linux/of.h | 6 ++++++ >>> 2 files changed, 38 insertions(+) >>> >>> Hi Rob, >>> >>> It would be good if we can target this for v4.10, so that we have no >>> dependencies to push PATCH 2/2 in v4.11 >> >> Applied. >> >> Rob >> > > A little fast on the trigger Rob. > > -Frank 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? 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().) -Frank -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/of/base.c b/drivers/of/base.c index a0bccb54a9bd..0810c5ecf1aa 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -546,6 +546,38 @@ int of_machine_is_compatible(const char *compat) EXPORT_SYMBOL(of_machine_is_compatible); /** + * of_machine_get_model_name - Find and read the model name or the compatible + * value for the machine. + * @model: pointer to null terminated return string, modified only if + * return value is 0. + * + * Returns a string containing either the model name or the compatible value + * of the machine if found, else return error. + * + * Search for a machine model name or the compatible if model name is missing + * in a device tree node and retrieve a null terminated string value (pointer + * to data, not a copy). Returns 0 on success, -EINVAL if root of the device + * tree is not found and other error returned by of_property_read_string on + * failure. + */ +int of_machine_get_model_name(const char **model) +{ + int error; + + if (!of_node_get(of_root)) + return -EINVAL; + + error = of_property_read_string(of_root, "model", model); + if (error) + error = of_property_read_string_index(of_root, "compatible", + 0, model); + of_node_put(of_root); + + return error; +} +EXPORT_SYMBOL(of_machine_get_model_name); + +/** * __of_device_is_available - check if a device is available for use * * @device: Node to check for availability, with locks already held diff --git a/include/linux/of.h b/include/linux/of.h index d72f01009297..13fc66531f1b 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -367,6 +367,7 @@ extern int of_alias_get_id(struct device_node *np, const char *stem); extern int of_alias_get_highest_id(const char *stem); extern int of_machine_is_compatible(const char *compat); +extern int of_machine_get_model_name(const char **model); extern int of_add_property(struct device_node *np, struct property *prop); extern int of_remove_property(struct device_node *np, struct property *prop); @@ -788,6 +789,11 @@ static inline int of_machine_is_compatible(const char *compat) return 0; } +static inline int of_machine_get_model_name(const char **model) +{ + return -EINVAL; +} + static inline bool of_console_check(const struct device_node *dn, const char *name, int index) { return false;
Currently platforms/drivers needing to get the machine model name are replicating the same snippet of code. In some case, the OF reference counting is either missing or incorrect. This patch adds support to read the machine model name either using the "model" or the "compatible" property in the device tree root node to the core OF/DT code. This can be used to remove all the duplicate code snippets doing exactly same thing later. Cc: Rob Herring <robh+dt@kernel.org> Cc: Frank Rowand <frowand.list@gmail.com> Cc: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/of/base.c | 32 ++++++++++++++++++++++++++++++++ include/linux/of.h | 6 ++++++ 2 files changed, 38 insertions(+) Hi Rob, It would be good if we can target this for v4.10, so that we have no dependencies to push PATCH 2/2 in v4.11 Regards, Sudeep -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html