diff mbox series

[net-next,v3,09/15] device property: Introduce fwnode_get_id()

Message ID 20210112134054.342-10-calvin.johnson@oss.nxp.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series ACPI support for dpaa2 driver | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 1 maintainers not CCed: mika.westerberg@linux.intel.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 8758 this patch: 8758
netdev/kdoc success Errors and warnings before: 5 this patch: 5
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 46 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 8627 this patch: 8627
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Calvin Johnson Jan. 12, 2021, 1:40 p.m. UTC
Using fwnode_get_id(), get the reg property value for DT node
or get the _ADR object value for ACPI node.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v3:
- Modified to retrieve reg property value for ACPI as well
- Resolved compilation issue with CONFIG_ACPI = n
- Added more info into documentation

Changes in v2: None

 drivers/base/property.c  | 33 +++++++++++++++++++++++++++++++++
 include/linux/property.h |  1 +
 2 files changed, 34 insertions(+)

Comments

Andy Shevchenko Jan. 12, 2021, 3:48 p.m. UTC | #1
On Tue, Jan 12, 2021 at 3:42 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Using fwnode_get_id(), get the reg property value for DT node
> or get the _ADR object value for ACPI node.
>
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> ---
>
> Changes in v3:
> - Modified to retrieve reg property value for ACPI as well
> - Resolved compilation issue with CONFIG_ACPI = n
> - Added more info into documentation
>
> Changes in v2: None
>
>  drivers/base/property.c  | 33 +++++++++++++++++++++++++++++++++
>  include/linux/property.h |  1 +
>  2 files changed, 34 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 35b95c6ac0c6..2d51108cb936 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -580,6 +580,39 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
>         return fwnode_call_ptr_op(fwnode, get_name_prefix);
>  }
>
> +/**
> + * fwnode_get_id - Get the id of a fwnode.
> + * @fwnode: firmware node
> + * @id: id of the fwnode
> + *
> + * This function provides the id of a fwnode which can be either
> + * DT or ACPI node. For ACPI, "reg" property value, if present will
> + * be provided or else _ADR value will be provided.
> + * Returns 0 on success or a negative errno.
> + */
> +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> +{
> +#ifdef CONFIG_ACPI
> +       unsigned long long adr;
> +       acpi_status status;
> +#endif
> +       int ret;
> +
> +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> +       if (!(ret && is_acpi_node(fwnode)))
> +               return ret;
> +
> +#ifdef CONFIG_ACPI
> +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> +                                      METHOD_NAME__ADR, NULL, &adr);
> +       if (ACPI_FAILURE(status))
> +               return -EINVAL;
> +       *id = (u32)adr;

Shouldn't be

       return 0;
#else
       return -EINVAL;
#endif

?

Yes, it's a theoretical case when is_acpi_node() returns true when
CONFIG_ACPI=n.

> +#endif
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_get_id);
> +
>  /**
>   * fwnode_get_parent - Return parent firwmare node
>   * @fwnode: Firmware whose parent is retrieved
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 0a9001fe7aea..3f41475f010b 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -82,6 +82,7 @@ struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode,
>
>  const char *fwnode_get_name(const struct fwnode_handle *fwnode);
>  const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode);
> +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id);
>  struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
>  struct fwnode_handle *fwnode_get_next_parent(
>         struct fwnode_handle *fwnode);
> --
> 2.17.1
>
Saravana Kannan Jan. 12, 2021, 5:30 p.m. UTC | #2
On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Using fwnode_get_id(), get the reg property value for DT node
> or get the _ADR object value for ACPI node.
>
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> ---
>
> Changes in v3:
> - Modified to retrieve reg property value for ACPI as well
> - Resolved compilation issue with CONFIG_ACPI = n
> - Added more info into documentation
>
> Changes in v2: None
>
>  drivers/base/property.c  | 33 +++++++++++++++++++++++++++++++++
>  include/linux/property.h |  1 +
>  2 files changed, 34 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 35b95c6ac0c6..2d51108cb936 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -580,6 +580,39 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
>         return fwnode_call_ptr_op(fwnode, get_name_prefix);
>  }
>
> +/**
> + * fwnode_get_id - Get the id of a fwnode.
> + * @fwnode: firmware node
> + * @id: id of the fwnode
> + *
> + * This function provides the id of a fwnode which can be either
> + * DT or ACPI node. For ACPI, "reg" property value, if present will
> + * be provided or else _ADR value will be provided.
> + * Returns 0 on success or a negative errno.
> + */
> +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> +{
> +#ifdef CONFIG_ACPI
> +       unsigned long long adr;
> +       acpi_status status;
> +#endif
> +       int ret;
> +
> +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> +       if (!(ret && is_acpi_node(fwnode)))
> +               return ret;
> +
> +#ifdef CONFIG_ACPI
> +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> +                                      METHOD_NAME__ADR, NULL, &adr);
> +       if (ACPI_FAILURE(status))
> +               return -EINVAL;
> +       *id = (u32)adr;
> +#endif
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_get_id);

Please don't do it this way. The whole point of fwnode_operations is
to avoid conditional stuff at the fwnode level. Also ACPI and DT
aren't mutually exclusive if I'm not mistaken.

Also, can you CC me on the entire series please? I want to reply to
some of your other patches too. Most of the fwnode changes don't seem
right. fwnode is lower level that the device-driver framework. Making
it aware of busses like mdio, etc doesn't sound right. Also, there's
already get_dev_from_fwnode() which is a much more efficient way to
look up/get a device from a fwnode instead of looping through a bus.

-Saravana
Andy Shevchenko Jan. 12, 2021, 6:03 p.m. UTC | #3
On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Using fwnode_get_id(), get the reg property value for DT node
> > or get the _ADR object value for ACPI node.

...

> > +/**
> > + * fwnode_get_id - Get the id of a fwnode.
> > + * @fwnode: firmware node
> > + * @id: id of the fwnode
> > + *
> > + * This function provides the id of a fwnode which can be either
> > + * DT or ACPI node. For ACPI, "reg" property value, if present will
> > + * be provided or else _ADR value will be provided.
> > + * Returns 0 on success or a negative errno.
> > + */
> > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > +{
> > +#ifdef CONFIG_ACPI
> > +       unsigned long long adr;
> > +       acpi_status status;
> > +#endif
> > +       int ret;
> > +
> > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > +       if (!(ret && is_acpi_node(fwnode)))
> > +               return ret;
> > +
> > +#ifdef CONFIG_ACPI
> > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > +                                      METHOD_NAME__ADR, NULL, &adr);
> > +       if (ACPI_FAILURE(status))
> > +               return -EINVAL;
> > +       *id = (u32)adr;
> > +#endif
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(fwnode_get_id);

> Please don't do it this way. The whole point of fwnode_operations is
> to avoid conditional stuff at the fwnode level.

Not fully true. We have non-POD getters that are conditional. Moreover,
we have additional layer of Primary / Secondary fwnodes on top of that.

The caller of fwnode API is indeed agnostic, but under the hood it differs by
the definition (obviously due to natural differences between ACPI and DT and
whatever else might come in the future.

> Also ACPI and DT
> aren't mutually exclusive if I'm not mistaken.

That's why we try 'reg' property for both cases first.

is_acpi_fwnode() conditional is that what I don't like though.

...

> fwnode is lower level that the device-driver framework.

Agree.

> Making
> it aware of busses like mdio, etc doesn't sound right.

Disagree. Conceptually resource providers can be quite different and fwnode API
*is* LCM for them.
Calvin Johnson Jan. 15, 2021, 10:47 a.m. UTC | #4
Hi,

On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Using fwnode_get_id(), get the reg property value for DT node
> > or get the _ADR object value for ACPI node.
> >
> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> > ---
> >
> > Changes in v3:
> > - Modified to retrieve reg property value for ACPI as well
> > - Resolved compilation issue with CONFIG_ACPI = n
> > - Added more info into documentation
> >
> > Changes in v2: None
> >
> >  drivers/base/property.c  | 33 +++++++++++++++++++++++++++++++++
> >  include/linux/property.h |  1 +
> >  2 files changed, 34 insertions(+)
> >
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > index 35b95c6ac0c6..2d51108cb936 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -580,6 +580,39 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
> >         return fwnode_call_ptr_op(fwnode, get_name_prefix);
> >  }
> >
> > +/**
> > + * fwnode_get_id - Get the id of a fwnode.
> > + * @fwnode: firmware node
> > + * @id: id of the fwnode
> > + *
> > + * This function provides the id of a fwnode which can be either
> > + * DT or ACPI node. For ACPI, "reg" property value, if present will
> > + * be provided or else _ADR value will be provided.
> > + * Returns 0 on success or a negative errno.
> > + */
> > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > +{
> > +#ifdef CONFIG_ACPI
> > +       unsigned long long adr;
> > +       acpi_status status;
> > +#endif
> > +       int ret;
> > +
> > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > +       if (!(ret && is_acpi_node(fwnode)))
> > +               return ret;
> > +
> > +#ifdef CONFIG_ACPI
> > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > +                                      METHOD_NAME__ADR, NULL, &adr);
> > +       if (ACPI_FAILURE(status))
> > +               return -EINVAL;
> > +       *id = (u32)adr;
> > +#endif
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(fwnode_get_id);
> 
> Please don't do it this way. The whole point of fwnode_operations is
> to avoid conditional stuff at the fwnode level. Also ACPI and DT
> aren't mutually exclusive if I'm not mistaken.
> 
> Also, can you CC me on the entire series please? I want to reply to
> some of your other patches too. Most of the fwnode changes don't seem
> right. fwnode is lower level that the device-driver framework. Making
> it aware of busses like mdio, etc doesn't sound right. Also, there's
> already get_dev_from_fwnode() which is a much more efficient way to
> look up/get a device from a fwnode instead of looping through a bus.

Thanks for reviewing the patch. I'll add you in the upcoming v4 series.
There is lot of history into patch series. It would be helpful, if you can
search for related submissions in the past and get some background.

Regards
Calvin
Rafael J. Wysocki Jan. 20, 2021, 6:17 p.m. UTC | #5
On Tue, Jan 12, 2021 at 4:47 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Jan 12, 2021 at 3:42 PM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Using fwnode_get_id(), get the reg property value for DT node
> > or get the _ADR object value for ACPI node.
> >
> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> > ---
> >
> > Changes in v3:
> > - Modified to retrieve reg property value for ACPI as well
> > - Resolved compilation issue with CONFIG_ACPI = n
> > - Added more info into documentation
> >
> > Changes in v2: None
> >
> >  drivers/base/property.c  | 33 +++++++++++++++++++++++++++++++++
> >  include/linux/property.h |  1 +
> >  2 files changed, 34 insertions(+)
> >
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > index 35b95c6ac0c6..2d51108cb936 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -580,6 +580,39 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
> >         return fwnode_call_ptr_op(fwnode, get_name_prefix);
> >  }
> >
> > +/**
> > + * fwnode_get_id - Get the id of a fwnode.
> > + * @fwnode: firmware node
> > + * @id: id of the fwnode
> > + *
> > + * This function provides the id of a fwnode which can be either
> > + * DT or ACPI node. For ACPI, "reg" property value, if present will
> > + * be provided or else _ADR value will be provided.
> > + * Returns 0 on success or a negative errno.
> > + */
> > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > +{
> > +#ifdef CONFIG_ACPI
> > +       unsigned long long adr;
> > +       acpi_status status;
> > +#endif
> > +       int ret;
> > +
> > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > +       if (!(ret && is_acpi_node(fwnode)))
> > +               return ret;
> > +
> > +#ifdef CONFIG_ACPI
> > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > +                                      METHOD_NAME__ADR, NULL, &adr);
> > +       if (ACPI_FAILURE(status))
> > +               return -EINVAL;
> > +       *id = (u32)adr;
>
> Shouldn't be
>
>        return 0;
> #else
>        return -EINVAL;
> #endif
>
> ?
>
> Yes, it's a theoretical case when is_acpi_node() returns true when
> CONFIG_ACPI=n.

How so?  is_acpi_node() is defined as a static inline returning false then.
Rafael J. Wysocki Jan. 20, 2021, 6:18 p.m. UTC | #6
On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> > <calvin.johnson@oss.nxp.com> wrote:
> > >
> > > Using fwnode_get_id(), get the reg property value for DT node
> > > or get the _ADR object value for ACPI node.
>
> ...
>
> > > +/**
> > > + * fwnode_get_id - Get the id of a fwnode.
> > > + * @fwnode: firmware node
> > > + * @id: id of the fwnode
> > > + *
> > > + * This function provides the id of a fwnode which can be either
> > > + * DT or ACPI node. For ACPI, "reg" property value, if present will
> > > + * be provided or else _ADR value will be provided.
> > > + * Returns 0 on success or a negative errno.
> > > + */
> > > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > > +{
> > > +#ifdef CONFIG_ACPI
> > > +       unsigned long long adr;
> > > +       acpi_status status;
> > > +#endif
> > > +       int ret;
> > > +
> > > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > +       if (!(ret && is_acpi_node(fwnode)))
> > > +               return ret;
> > > +
> > > +#ifdef CONFIG_ACPI
> > > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > +                                      METHOD_NAME__ADR, NULL, &adr);
> > > +       if (ACPI_FAILURE(status))
> > > +               return -EINVAL;
> > > +       *id = (u32)adr;
> > > +#endif
> > > +       return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(fwnode_get_id);
>
> > Please don't do it this way. The whole point of fwnode_operations is
> > to avoid conditional stuff at the fwnode level.
>
> Not fully true. We have non-POD getters that are conditional. Moreover,
> we have additional layer of Primary / Secondary fwnodes on top of that.
>
> The caller of fwnode API is indeed agnostic, but under the hood it differs by
> the definition (obviously due to natural differences between ACPI and DT and
> whatever else might come in the future.
>
> > Also ACPI and DT
> > aren't mutually exclusive if I'm not mistaken.
>
> That's why we try 'reg' property for both cases first.
>
> is_acpi_fwnode() conditional is that what I don't like though.

I'm not sure what you mean here, care to elaborate?
Andy Shevchenko Jan. 20, 2021, 6:45 p.m. UTC | #7
On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Jan 12, 2021 at 4:47 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Tue, Jan 12, 2021 at 3:42 PM Calvin Johnson
> > <calvin.johnson@oss.nxp.com> wrote:

...

> > > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > > +{
> > > +#ifdef CONFIG_ACPI
> > > +       unsigned long long adr;
> > > +       acpi_status status;
> > > +#endif
> > > +       int ret;
> > > +
> > > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > +       if (!(ret && is_acpi_node(fwnode)))
> > > +               return ret;
> > > +
> > > +#ifdef CONFIG_ACPI
> > > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > +                                      METHOD_NAME__ADR, NULL, &adr);
> > > +       if (ACPI_FAILURE(status))
> > > +               return -EINVAL;
> > > +       *id = (u32)adr;
> >
> > Shouldn't be
> >
> >        return 0;
> > #else
> >        return -EINVAL;
> > #endif
> >
> > ?
> >
> > Yes, it's a theoretical case when is_acpi_node() returns true when
> > CONFIG_ACPI=n.
>
> How so?  is_acpi_node() is defined as a static inline returning false then.

I understand that, that's why it's pure theoretical when, for example,
the semantics is changed. But I believe it's unlucky to happen.
Andy Shevchenko Jan. 20, 2021, 6:52 p.m. UTC | #8
On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> > > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> > > <calvin.johnson@oss.nxp.com> wrote:

...

> > > > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > > +       if (!(ret && is_acpi_node(fwnode)))
> > > > +               return ret;
> > > > +
> > > > +#ifdef CONFIG_ACPI
> > > > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > > +                                      METHOD_NAME__ADR, NULL, &adr);
> > > > +       if (ACPI_FAILURE(status))
> > > > +               return -EINVAL;
> > > > +       *id = (u32)adr;
> > > > +#endif
> > > > +       return 0;

> > > Also ACPI and DT
> > > aren't mutually exclusive if I'm not mistaken.
> >
> > That's why we try 'reg' property for both cases first.
> >
> > is_acpi_fwnode() conditional is that what I don't like though.
>
> I'm not sure what you mean here, care to elaborate?

I meant is_acpi_node(fwnode) in the conditional.

I think it's redundant and we can simple do something like this:

  if (ret) {
#ifdef ACPI
    ...
#else
    return ret;
#endif
  }
  return 0;
Rafael J. Wysocki Jan. 20, 2021, 7:12 p.m. UTC | #9
On Wed, Jan 20, 2021 at 7:44 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Tue, Jan 12, 2021 at 4:47 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Jan 12, 2021 at 3:42 PM Calvin Johnson
> > > <calvin.johnson@oss.nxp.com> wrote:
>
> ...
>
> > > > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > > > +{
> > > > +#ifdef CONFIG_ACPI
> > > > +       unsigned long long adr;
> > > > +       acpi_status status;
> > > > +#endif
> > > > +       int ret;
> > > > +
> > > > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > > +       if (!(ret && is_acpi_node(fwnode)))
> > > > +               return ret;
> > > > +
> > > > +#ifdef CONFIG_ACPI
> > > > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > > +                                      METHOD_NAME__ADR, NULL, &adr);
> > > > +       if (ACPI_FAILURE(status))
> > > > +               return -EINVAL;
> > > > +       *id = (u32)adr;
> > >
> > > Shouldn't be
> > >
> > >        return 0;
> > > #else
> > >        return -EINVAL;
> > > #endif
> > >
> > > ?
> > >
> > > Yes, it's a theoretical case when is_acpi_node() returns true when
> > > CONFIG_ACPI=n.
> >
> > How so?  is_acpi_node() is defined as a static inline returning false then.
>
> I understand that, that's why it's pure theoretical when, for example,
> the semantics is changed. But I believe it's unlucky to happen.

Changing the definition of it for CONFIG_ACPI=n would be a regression
given the current usage of it.
Rafael J. Wysocki Jan. 20, 2021, 7:15 p.m. UTC | #10
On Wed, Jan 20, 2021 at 7:51 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> > > > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> > > > <calvin.johnson@oss.nxp.com> wrote:
>
> ...
>
> > > > > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > > > +       if (!(ret && is_acpi_node(fwnode)))
> > > > > +               return ret;
> > > > > +
> > > > > +#ifdef CONFIG_ACPI
> > > > > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > > > +                                      METHOD_NAME__ADR, NULL, &adr);
> > > > > +       if (ACPI_FAILURE(status))
> > > > > +               return -EINVAL;
> > > > > +       *id = (u32)adr;
> > > > > +#endif
> > > > > +       return 0;
>
> > > > Also ACPI and DT
> > > > aren't mutually exclusive if I'm not mistaken.
> > >
> > > That's why we try 'reg' property for both cases first.
> > >
> > > is_acpi_fwnode() conditional is that what I don't like though.
> >
> > I'm not sure what you mean here, care to elaborate?
>
> I meant is_acpi_node(fwnode) in the conditional.
>
> I think it's redundant and we can simple do something like this:
>
>   if (ret) {
> #ifdef ACPI
>     ...
> #else
>     return ret;
> #endif
>   }
>   return 0;
>
> --

Right, that should work.  And I'd prefer it too.
Saravana Kannan Jan. 20, 2021, 8:01 p.m. UTC | #11
On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jan 20, 2021 at 7:51 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> > > > > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> > > > > <calvin.johnson@oss.nxp.com> wrote:
> >
> > ...
> >
> > > > > > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > > > > +       if (!(ret && is_acpi_node(fwnode)))
> > > > > > +               return ret;
> > > > > > +
> > > > > > +#ifdef CONFIG_ACPI
> > > > > > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > > > > +                                      METHOD_NAME__ADR, NULL, &adr);
> > > > > > +       if (ACPI_FAILURE(status))
> > > > > > +               return -EINVAL;
> > > > > > +       *id = (u32)adr;
> > > > > > +#endif
> > > > > > +       return 0;
> >
> > > > > Also ACPI and DT
> > > > > aren't mutually exclusive if I'm not mistaken.
> > > >
> > > > That's why we try 'reg' property for both cases first.
> > > >
> > > > is_acpi_fwnode() conditional is that what I don't like though.
> > >
> > > I'm not sure what you mean here, care to elaborate?
> >
> > I meant is_acpi_node(fwnode) in the conditional.
> >
> > I think it's redundant and we can simple do something like this:
> >
> >   if (ret) {
> > #ifdef ACPI
> >     ...
> > #else
> >     return ret;
> > #endif
> >   }
> >   return 0;
> >
> > --
>
> Right, that should work.  And I'd prefer it too.

Rafael,

I'd rather this new function be an ops instead of a bunch of #ifdef or
if (acpi) checks. Thoughts?

-Saravana
Rafael J. Wysocki Jan. 22, 2021, 4:34 p.m. UTC | #12
On Wed, Jan 20, 2021 at 9:01 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Jan 20, 2021 at 7:51 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> > > > > > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> > > > > > <calvin.johnson@oss.nxp.com> wrote:
> > >
> > > ...
> > >
> > > > > > > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > > > > > +       if (!(ret && is_acpi_node(fwnode)))
> > > > > > > +               return ret;
> > > > > > > +
> > > > > > > +#ifdef CONFIG_ACPI
> > > > > > > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > > > > > +                                      METHOD_NAME__ADR, NULL, &adr);
> > > > > > > +       if (ACPI_FAILURE(status))
> > > > > > > +               return -EINVAL;
> > > > > > > +       *id = (u32)adr;
> > > > > > > +#endif
> > > > > > > +       return 0;
> > >
> > > > > > Also ACPI and DT
> > > > > > aren't mutually exclusive if I'm not mistaken.
> > > > >
> > > > > That's why we try 'reg' property for both cases first.
> > > > >
> > > > > is_acpi_fwnode() conditional is that what I don't like though.
> > > >
> > > > I'm not sure what you mean here, care to elaborate?
> > >
> > > I meant is_acpi_node(fwnode) in the conditional.
> > >
> > > I think it's redundant and we can simple do something like this:
> > >
> > >   if (ret) {
> > > #ifdef ACPI
> > >     ...
> > > #else
> > >     return ret;
> > > #endif
> > >   }
> > >   return 0;
> > >
> > > --
> >
> > Right, that should work.  And I'd prefer it too.
>
> Rafael,
>
> I'd rather this new function be an ops instead of a bunch of #ifdef or
> if (acpi) checks. Thoughts?

Well, it looks more like a helper function than like an op and I'm not
even sure how many potential users of it will expect that _ADR should
be evaluated in the absence of the "reg" property.

It's just that the "reg" property happens to be kind of an _ADR
equivalent in this particular binding AFAICS.
Saravana Kannan Jan. 22, 2021, 8:58 p.m. UTC | #13
On Fri, Jan 22, 2021 at 8:34 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jan 20, 2021 at 9:01 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Wed, Jan 20, 2021 at 7:51 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > >
> > > > On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com> wrote:
> > > > > > On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> > > > > > > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> > > > > > > <calvin.johnson@oss.nxp.com> wrote:
> > > >
> > > > ...
> > > >
> > > > > > > > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > > > > > > +       if (!(ret && is_acpi_node(fwnode)))
> > > > > > > > +               return ret;
> > > > > > > > +
> > > > > > > > +#ifdef CONFIG_ACPI
> > > > > > > > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > > > > > > +                                      METHOD_NAME__ADR, NULL, &adr);
> > > > > > > > +       if (ACPI_FAILURE(status))
> > > > > > > > +               return -EINVAL;
> > > > > > > > +       *id = (u32)adr;
> > > > > > > > +#endif
> > > > > > > > +       return 0;
> > > >
> > > > > > > Also ACPI and DT
> > > > > > > aren't mutually exclusive if I'm not mistaken.
> > > > > >
> > > > > > That's why we try 'reg' property for both cases first.
> > > > > >
> > > > > > is_acpi_fwnode() conditional is that what I don't like though.
> > > > >
> > > > > I'm not sure what you mean here, care to elaborate?
> > > >
> > > > I meant is_acpi_node(fwnode) in the conditional.
> > > >
> > > > I think it's redundant and we can simple do something like this:
> > > >
> > > >   if (ret) {
> > > > #ifdef ACPI
> > > >     ...
> > > > #else
> > > >     return ret;
> > > > #endif
> > > >   }
> > > >   return 0;
> > > >
> > > > --
> > >
> > > Right, that should work.  And I'd prefer it too.
> >
> > Rafael,
> >
> > I'd rather this new function be an ops instead of a bunch of #ifdef or
> > if (acpi) checks. Thoughts?
>
> Well, it looks more like a helper function than like an op and I'm not
> even sure how many potential users of it will expect that _ADR should
> be evaluated in the absence of the "reg" property.
>
> It's just that the "reg" property happens to be kind of an _ADR
> equivalent in this particular binding AFAICS.

I agree it is not clear how useful this helper function is going to be.

But in general, to me, any time the wrapper/helper functions in
drivers/base/property.c need to do something like this:

if (ACPI)
   ACPI specific code
if (OF)
   OF specific code

I think the code should be pushed to the fwnode ops. That's one of the
main point of fwnode. So that firmware specific stuff is done by
firmware specific code. Also, when adding support for new firmware,
it's pretty clear what support the firmware needs to implement.
Instead of having to go fix up a bunch of code all over the place.

So fwnode_ops->get_id() would be the OP ACPI and OF would implement.
And then we can have a wrapper in drivers/base/property.c.

-Saravana
Andy Shevchenko Jan. 22, 2021, 9:06 p.m. UTC | #14
On Fri, Jan 22, 2021 at 10:59 PM Saravana Kannan <saravanak@google.com> wrote:
> On Fri, Jan 22, 2021 at 8:34 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Wed, Jan 20, 2021 at 9:01 PM Saravana Kannan <saravanak@google.com> wrote:
> > > On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote:


> > > I'd rather this new function be an ops instead of a bunch of #ifdef or
> > > if (acpi) checks. Thoughts?
> >
> > Well, it looks more like a helper function than like an op and I'm not
> > even sure how many potential users of it will expect that _ADR should
> > be evaluated in the absence of the "reg" property.
> >
> > It's just that the "reg" property happens to be kind of an _ADR
> > equivalent in this particular binding AFAICS.
>
> I agree it is not clear how useful this helper function is going to be.
>
> But in general, to me, any time the wrapper/helper functions in
> drivers/base/property.c need to do something like this:
>
> if (ACPI)
>    ACPI specific code
> if (OF)
>    OF specific code
>
> I think the code should be pushed to the fwnode ops. That's one of the
> main point of fwnode. So that firmware specific stuff is done by
> firmware specific code. Also, when adding support for new firmware,
> it's pretty clear what support the firmware needs to implement.
> Instead of having to go fix up a bunch of code all over the place.

Wishful thinking.
In the very case of GPIO it's related to framework using headers local
to framework. Are you suggesting to open its guts to the entire wild
world?
I don't think it's a good idea. You see, here we have different
layering POD types, which are natural and quite low level that ops
suits best for them and quite different resource types like GPIO. And
the latter is closer to certain framework rather than to POD handling
cases.

> So fwnode_ops->get_id() would be the OP ACPI and OF would implement.
> And then we can have a wrapper in drivers/base/property.c.
Saravana Kannan Jan. 22, 2021, 9:07 p.m. UTC | #15
On Fri, Jan 22, 2021 at 1:05 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, Jan 22, 2021 at 10:59 PM Saravana Kannan <saravanak@google.com> wrote:
> > On Fri, Jan 22, 2021 at 8:34 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > On Wed, Jan 20, 2021 at 9:01 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
>
> > > > I'd rather this new function be an ops instead of a bunch of #ifdef or
> > > > if (acpi) checks. Thoughts?
> > >
> > > Well, it looks more like a helper function than like an op and I'm not
> > > even sure how many potential users of it will expect that _ADR should
> > > be evaluated in the absence of the "reg" property.
> > >
> > > It's just that the "reg" property happens to be kind of an _ADR
> > > equivalent in this particular binding AFAICS.
> >
> > I agree it is not clear how useful this helper function is going to be.
> >
> > But in general, to me, any time the wrapper/helper functions in
> > drivers/base/property.c need to do something like this:
> >
> > if (ACPI)
> >    ACPI specific code
> > if (OF)
> >    OF specific code
> >
> > I think the code should be pushed to the fwnode ops. That's one of the
> > main point of fwnode. So that firmware specific stuff is done by
> > firmware specific code. Also, when adding support for new firmware,
> > it's pretty clear what support the firmware needs to implement.
> > Instead of having to go fix up a bunch of code all over the place.
>
> Wishful thinking.
> In the very case of GPIO it's related to framework using headers local
> to framework. Are you suggesting to open its guts to the entire wild
> world?

What are you even talking about? How is whatever you are saying
remotely related to getting an "id" for a fwnode?

> I don't think it's a good idea. You see, here we have different
> layering POD types,

POD?

> which are natural and quite low level that ops
> suits best for them and quite different resource types like GPIO. And
> the latter is closer to certain framework rather than to POD handling
> cases.
>

-Saravana
diff mbox series

Patch

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 35b95c6ac0c6..2d51108cb936 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -580,6 +580,39 @@  const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
 	return fwnode_call_ptr_op(fwnode, get_name_prefix);
 }
 
+/**
+ * fwnode_get_id - Get the id of a fwnode.
+ * @fwnode: firmware node
+ * @id: id of the fwnode
+ *
+ * This function provides the id of a fwnode which can be either
+ * DT or ACPI node. For ACPI, "reg" property value, if present will
+ * be provided or else _ADR value will be provided.
+ * Returns 0 on success or a negative errno.
+ */
+int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
+{
+#ifdef CONFIG_ACPI
+	unsigned long long adr;
+	acpi_status status;
+#endif
+	int ret;
+
+	ret = fwnode_property_read_u32(fwnode, "reg", id);
+	if (!(ret && is_acpi_node(fwnode)))
+		return ret;
+
+#ifdef CONFIG_ACPI
+	status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
+				       METHOD_NAME__ADR, NULL, &adr);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+	*id = (u32)adr;
+#endif
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_id);
+
 /**
  * fwnode_get_parent - Return parent firwmare node
  * @fwnode: Firmware whose parent is retrieved
diff --git a/include/linux/property.h b/include/linux/property.h
index 0a9001fe7aea..3f41475f010b 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -82,6 +82,7 @@  struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode,
 
 const char *fwnode_get_name(const struct fwnode_handle *fwnode);
 const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode);
+int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id);
 struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_next_parent(
 	struct fwnode_handle *fwnode);