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

Groups > linux.kernel > #1477299 > unrolled thread

Re: [v11, 7/8] base: soc: introduce soc_device_match() interface

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: [v11, 7/8] base: soc: introduce soc_device_match() interface Ulf Hansson <ulf.hansson@linaro.org> - 2016-09-06 13:50 +0200
    Re: [v11, 7/8] base: soc: introduce soc_device_match() interface Arnd Bergmann <arnd@arndb.de> - 2016-09-06 14:50 +0200
      RE: [v11, 7/8] base: soc: introduce soc_device_match() interface "Y.B. Lu" <yangbo.lu@nxp.com> - 2016-09-07 06:20 +0200

#1477299 — Re: [v11, 7/8] base: soc: introduce soc_device_match() interface

On 6 September 2016 at 10:28, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> We keep running into cases where device drivers want to know the exact
> version of the a SoC they are currently running on. In the past, this has
> usually been done through a vendor specific API that can be called by a
> driver, or by directly accessing some kind of version register that is
> not part of the device itself but that belongs to a global register area
> of the chip.
>
> Common reasons for doing this include:
>
> - A machine is not using devicetree or similar for passing data about
>   on-chip devices, but just announces their presence using boot-time
>   platform devices, and the machine code itself does not care about the
>   revision.
>
> - There is existing firmware or boot loaders with existing DT binaries
>   with generic compatible strings that do not identify the particular
>   revision of each device, but the driver knows which SoC revisions
>   include which part.
>
> - A prerelease version of a chip has some quirks and we are using the same
>   version of the bootloader and the DT blob on both the prerelease and the
>   final version. An update of the DT binding seems inappropriate because
>   that would involve maintaining multiple copies of the dts and/or
>   bootloader.
>
> This patch introduces the soc_device_match() interface that is meant to
> work like of_match_node() but instead of identifying the version of a
> device, it identifies the SoC itself using a vendor-agnostic interface.
>
> Unlike of_match_node(), we do not do an exact string compare but instead
> use glob_match() to allow wildcards in strings.

Overall, this change make sense to me, although some minor comment below.

>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> ---
> Changes for v11:
>         - Added this patch for soc match
> ---
>  drivers/base/Kconfig    |  1 +
>  drivers/base/soc.c      | 61 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sys_soc.h |  3 +++
>  3 files changed, 65 insertions(+)
>
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 98504ec..f1591ad2 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -225,6 +225,7 @@ config GENERIC_CPU_AUTOPROBE
>
>  config SOC_BUS
>         bool
> +       select GLOB
>
>  source "drivers/base/regmap/Kconfig"
>
> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> index 75b98aa..5c4e84a 100644
> --- a/drivers/base/soc.c
> +++ b/drivers/base/soc.c
> @@ -14,6 +14,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/sys_soc.h>
>  #include <linux/err.h>
> +#include <linux/glob.h>
>
>  static DEFINE_IDA(soc_ida);
>
> @@ -168,3 +169,63 @@ static void __exit soc_bus_unregister(void)
>         bus_unregister(&soc_bus_type);
>  }
>  module_exit(soc_bus_unregister);
> +
> +static int soc_device_match_one(struct device *dev, void *arg)
> +{
> +       struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
> +       const struct soc_device_attribute *match = arg;
> +
> +       if (match->machine &&
> +           !glob_match(match->machine, soc_dev->attr->machine))
> +               return 0;
> +
> +       if (match->family &&
> +           !glob_match(match->family, soc_dev->attr->family))
> +               return 0;
> +
> +       if (match->revision &&
> +           !glob_match(match->revision, soc_dev->attr->revision))
> +               return 0;
> +
> +       if (match->soc_id &&
> +           !glob_match(match->soc_id, soc_dev->attr->soc_id))
> +               return 0;
> +
> +       return 1;
> +}
> +
> +/*
> + * soc_device_match - identify the SoC in the machine
> + * @matches: zero-terminated array of possible matches

Perhaps also express the constraint on the matching entries. As you
need at least one of the ->machine(), ->family(), ->revision() or
->soc_id() callbacks implemented, right!?

> + *
> + * returns the first matching entry of the argument array, or NULL
> + * if none of them match.
> + *
> + * This function is meant as a helper in place of of_match_node()
> + * in cases where either no device tree is available or the information
> + * in a device node is insufficient to identify a particular variant
> + * by its compatible strings or other properties. For new devices,
> + * the DT binding should always provide unique compatible strings
> + * that allow the use of of_match_node() instead.
> + *
> + * The calling function can use the .data entry of the
> + * soc_device_attribute to pass a structure or function pointer for
> + * each entry.

I don't get the use case behind this, could you elaborate?

Perhaps we should postpone adding the .data entry until we actually
see a need for it?

> + */
> +const struct soc_device_attribute *soc_device_match(
> +       const struct soc_device_attribute *matches)
> +{
> +       struct device *dev;
> +       int ret;
> +
> +       for (ret = 0; ret == 0; matches++) {

This loop looks a bit weird and unsafe.

1) Perhaps using a while loop makes this more readable?
2) As this is an exported API, I guess validation of the ->matches
pointer needs to be done before accessing it.

> +               if (!(matches->machine || matches->family ||
> +                     matches->revision || matches->soc_id))
> +                       return NULL;
> +               dev = NULL;

There's no need to use a struct device just to assign it to NULL.
Instead just provide the function below with NULL.

> +               ret = bus_for_each_dev(&soc_bus_type, dev, (void *)matches,
> +                                      soc_device_match_one);
> +       }
> +       return matches;
> +}
> +EXPORT_SYMBOL_GPL(soc_device_match);
> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> index 2739ccb..9f5eb06 100644
> --- a/include/linux/sys_soc.h
> +++ b/include/linux/sys_soc.h
> @@ -13,6 +13,7 @@ struct soc_device_attribute {
>         const char *family;
>         const char *revision;
>         const char *soc_id;
> +       const void *data;
>  };
>
>  /**
> @@ -34,4 +35,6 @@ void soc_device_unregister(struct soc_device *soc_dev);
>   */
>  struct device *soc_device_to_device(struct soc_device *soc);
>
> +const struct soc_device_attribute *soc_device_match(
> +       const struct soc_device_attribute *matches);
>  #endif /* __SOC_BUS_H */
> --
> 2.1.0.27.g96db324
>

Kind regards
Uffe

[toc] | [next] | [standalone]

#1477332

On Tuesday, September 6, 2016 1:44:23 PM CEST Ulf Hansson wrote:
> On 6 September 2016 at 10:28, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> > We keep running into cases where device drivers want to know the exact
> > version of the a SoC they are currently running on. In the past, this has
> > usually been done through a vendor specific API that can be called by a
> > driver, or by directly accessing some kind of version register that is
> > not part of the device itself but that belongs to a global register area
> > of the chip.

Please add "From: Arnd Bergmann <arnd@arndb.de>" as the first line, to
preserve authorship. If you use "git send-email" or "git format-patch",
that should happen automatically if the author field is set right
(if not, use 'git commit --amend --author="Arnd Bergmann <arnd@arndb.de>"'
to fix it).

> > +
> > +/*
> > + * soc_device_match - identify the SoC in the machine
> > + * @matches: zero-terminated array of possible matches
> 
> Perhaps also express the constraint on the matching entries. As you
> need at least one of the ->machine(), ->family(), ->revision() or
> ->soc_id() callbacks implemented, right!?

They are not callbacks, just strings. Having an empty entry indicates
the end of the array, and this is not called.

> > + *
> > + * returns the first matching entry of the argument array, or NULL
> > + * if none of them match.
> > + *
> > + * This function is meant as a helper in place of of_match_node()
> > + * in cases where either no device tree is available or the information
> > + * in a device node is insufficient to identify a particular variant
> > + * by its compatible strings or other properties. For new devices,
> > + * the DT binding should always provide unique compatible strings
> > + * that allow the use of of_match_node() instead.
> > + *
> > + * The calling function can use the .data entry of the
> > + * soc_device_attribute to pass a structure or function pointer for
> > + * each entry.
> 
> I don't get the use case behind this, could you elaborate?
> 
> Perhaps we should postpone adding the .data entry until we actually
> see a need for it?

I think the interface is rather useless without a way to figure
out which entry you got. Almost all users of of_match_node()
actually use the returned ->data field, and I expect this to
be the same here.

> > + */
> > +const struct soc_device_attribute *soc_device_match(
> > +       const struct soc_device_attribute *matches)
> > +{
> > +       struct device *dev;
> > +       int ret;
> > +
> > +       for (ret = 0; ret == 0; matches++) {
> 
> This loop looks a bit weird and unsafe.

Ah, and I thought I was being clever ;-)

> 1) Perhaps using a while loop makes this more readable?
> 2) As this is an exported API, I guess validation of the ->matches
> pointer needs to be done before accessing it.

Sounds fine.

> > +               if (!(matches->machine || matches->family ||
> > +                     matches->revision || matches->soc_id))
> > +                       return NULL;
> > +               dev = NULL;
> 
> There's no need to use a struct device just to assign it to NULL.
> Instead just provide the function below with NULL.
>
> > +               ret = bus_for_each_dev(&soc_bus_type, dev, (void *)matches,
> > +                                      soc_device_match_one);


I don't remember what led to this, I think you are right, we should
just pass NULL as most other callers.

Thanks for the review.

	ARnd

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

#1477954

Hi Anrd and Uffe,

Thank you for your comment.
Please see my comment inline.



Best regards,
Yangbo Lu

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Tuesday, September 06, 2016 8:46 PM
> To: Ulf Hansson
> Cc: Y.B. Lu; linux-mmc; Scott Wood; linuxppc-dev@lists.ozlabs.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; linux-clk; linux-i2c@vger.kernel.org;
> iommu@lists.linux-foundation.org; netdev@vger.kernel.org; Mark Rutland;
> Rob Herring; Russell King; Jochen Friedrich; Joerg Roedel; Claudiu Manoil;
> Bhupesh Sharma; Qiang Zhao; Kumar Gala; Santosh Shilimkar; Leo Li; X.B.
> Xie
> Subject: Re: [v11, 7/8] base: soc: introduce soc_device_match() interface
> 
> On Tuesday, September 6, 2016 1:44:23 PM CEST Ulf Hansson wrote:
> > On 6 September 2016 at 10:28, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> > > We keep running into cases where device drivers want to know the
> > > exact version of the a SoC they are currently running on. In the
> > > past, this has usually been done through a vendor specific API that
> > > can be called by a driver, or by directly accessing some kind of
> > > version register that is not part of the device itself but that
> > > belongs to a global register area of the chip.
> 
> Please add "From: Arnd Bergmann <arnd@arndb.de>" as the first line, to
> preserve authorship. If you use "git send-email" or "git format-patch",
> that should happen automatically if the author field is set right (if not,
> use 'git commit --amend --author="Arnd Bergmann <arnd@arndb.de>"'
> to fix it).
> 

[Lu Yangbo-B47093] Oh, I'm sorry for my careless. Will correct it in next version.

> > > +
> > > +/*
> > > + * soc_device_match - identify the SoC in the machine
> > > + * @matches: zero-terminated array of possible matches
> >
> > Perhaps also express the constraint on the matching entries. As you
> > need at least one of the ->machine(), ->family(), ->revision() or
> > ->soc_id() callbacks implemented, right!?
> 
> They are not callbacks, just strings. Having an empty entry indicates the
> end of the array, and this is not called.
> 
> > > + *
> > > + * returns the first matching entry of the argument array, or NULL
> > > + * if none of them match.
> > > + *
> > > + * This function is meant as a helper in place of of_match_node()
> > > + * in cases where either no device tree is available or the
> > > + information
> > > + * in a device node is insufficient to identify a particular
> > > + variant
> > > + * by its compatible strings or other properties. For new devices,
> > > + * the DT binding should always provide unique compatible strings
> > > + * that allow the use of of_match_node() instead.
> > > + *
> > > + * The calling function can use the .data entry of the
> > > + * soc_device_attribute to pass a structure or function pointer for
> > > + * each entry.
> >
> > I don't get the use case behind this, could you elaborate?
> >
> > Perhaps we should postpone adding the .data entry until we actually
> > see a need for it?
> 
> I think the interface is rather useless without a way to figure out which
> entry you got. Almost all users of of_match_node() actually use the
> returned ->data field, and I expect this to be the same here.
> 
> > > + */
> > > +const struct soc_device_attribute *soc_device_match(
> > > +       const struct soc_device_attribute *matches) {
> > > +       struct device *dev;
> > > +       int ret;
> > > +
> > > +       for (ret = 0; ret == 0; matches++) {
> >
> > This loop looks a bit weird and unsafe.
> 
> Ah, and I thought I was being clever ;-)
> 
> > 1) Perhaps using a while loop makes this more readable?
> > 2) As this is an exported API, I guess validation of the ->matches
> > pointer needs to be done before accessing it.
> 
> Sounds fine.

[Lu Yangbo-B47093] Ok, Will change this according to Uffe. 
And actually there is issue with this for() when I verified it again this morning.
We will get matches++ rather than matches which is correct finally :)

> 
> > > +               if (!(matches->machine || matches->family ||
> > > +                     matches->revision || matches->soc_id))
> > > +                       return NULL;
> > > +               dev = NULL;
> >
> > There's no need to use a struct device just to assign it to NULL.
> > Instead just provide the function below with NULL.
> >
> > > +               ret = bus_for_each_dev(&soc_bus_type, dev, (void
> *)matches,
> > > +                                      soc_device_match_one);
> 
> 
> I don't remember what led to this, I think you are right, we should just
> pass NULL as most other callers.

[Lu Yangbo-B47093] Will correct it. Thanks. :)

> 
> Thanks for the review.
> 
> 	ARnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web