[libcamera-devel,v7,2/9] libcamera: sysfs: Add method to lookup firmware ID

9185 diff mbox series

A systems firmware description is recorded differently in sysfs
depending if the system uses OF or ACPI. Add a helper to abstract
this, allowing users not to care which of the two are used.

For OF-based systems the ID is the full path of the device in the
device tree description. For ACPI-based systems the ID is the ACPI
firmware nodes path. Both ID sources are guaranteed to be unique and
persistent as long as the firmware of the system is not changed.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/libcamera/internal/sysfs.h |  2 ++
 src/libcamera/sysfs.cpp            | 57 ++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

Hi Niklas,

Thank you for the patch.

On Tue, Aug 04, 2020 at 06:13:51PM +0200, Niklas Söderlund wrote:
> A systems firmware description is recorded differently in sysfs

s/systems/system's/ or just system.

> depending if the system uses OF or ACPI. Add a helper to abstract
> this, allowing users not to care which of the two are used.
> 
> For OF-based systems the ID is the full path of the device in the
> device tree description. For ACPI-based systems the ID is the ACPI
> firmware nodes path. Both ID sources are guaranteed to be unique and
> persistent as long as the firmware of the system is not changed.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/libcamera/internal/sysfs.h |  2 ++
>  src/libcamera/sysfs.cpp            | 57 ++++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/include/libcamera/internal/sysfs.h b/include/libcamera/internal/sysfs.h
> index 72f436205d8d30e8..ef8ec55909d85bf9 100644
> --- a/include/libcamera/internal/sysfs.h
> +++ b/include/libcamera/internal/sysfs.h
> @@ -15,6 +15,8 @@ namespace sysfs {
>  
>  std::string charDevPath(const std::string &devicePath);
>  
> +int firmwareId(const std::string &path, std::string *id);
> +
>  } /* namespace sysfs */
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/sysfs.cpp b/src/libcamera/sysfs.cpp
> index 3b2920663e9c3bcc..98be4df9d38e6fa8 100644
> --- a/src/libcamera/sysfs.cpp
> +++ b/src/libcamera/sysfs.cpp
> @@ -7,9 +7,11 @@
>  
>  #include "libcamera/internal/sysfs.h"
>  
> +#include <fstream>
>  #include <sys/stat.h>
>  #include <sys/sysmacros.h>
>  
> +#include "libcamera/internal/file.h"
>  #include "libcamera/internal/log.h"
>  
>  /**
> @@ -43,6 +45,61 @@ std::string charDevPath(const std::string &devicePath)
>  	return dev.str();
>  }
>  
> +/**
> + * \brief Try to read a device firmware ID from sysfs

"Retrieve the path of the firmware node for a device"

?

> + * \param[in] path Path in sysfs to search

I think "path" is a bit ambiguous. Should the parameter be called
"device" or "devicePath" ?

> + * \param[out] id Location to store ID if found

How about returning the string instead of passing it by pointer ? Or do
you need to differentiate between -EINVAL and -ENODEV ? The -EINVAL case
should really not happen.

This is another case where returning a std::tuple<int, std::string>
could make sense if you don't dislike that.

> + *
> + * A systems firmware description is recorded differently in sysfs depending if
> + * the system uses OF or ACPI. Add a helper to abstract this, allowing users not
> + * to care which of the two are used.

"Add a helper" ? :-) Bad copy & paste from the commit message ? I think
you can replace this with

 * Physical devices in a system are described by the system firmware. Depending
 * on the type of platform, devices are identified using different naming
 * schemes. The Linux kernel abstract those differences with "firmware nodes".
 * This function retrieves the firmware node path corresponding to the
 * \a device.

> + *
> + * For OF-based systems the ID is the full path of the device in the device tree

I'd say DT instead of OF, as DT is a subset of OF.

> + * description. For ACPI-based systems the ID is the ACPI firmware nodes path.
> + * Both ID sources are guaranteed to be unique and persistent as long as the
> + * firmware of the system is not changed.

Being a bit pendantic, for ACPI, it's the ACPI object absolute namespace
path. Both DT and ACPI are abstracted by the "firmware node" API in the
kernel. Maybe

 * For DT-based systems, the path is the full name of the DT node that
 * represents the device. For ACPI-based systems, the path is the absolute
 * namespace path to the ACPI object that represents the device. In both cases,
 * the path is guaranteed to be unique and persistent as long as the system
 * firmware is not modified.

> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EINVAL Error when looking up firmware ID
> + * \retval -ENODEV No firmware ID available for \a path
> + */
> +int firmwareId(const std::string &path, std::string *id)

I'm not sure "Id" is a proper name for this. Based on the above comment,
should the function thus be named firmwareNodePath() ?

> +{
> +	ASSERT(id);
> +
> +	/* ID lookup for OF-based systems */
> +	File ofFile(path + "/of_node");
> +	if (ofFile.exists()) {

You don't need to construct a File object, there's a File::exists()
static function.

	std::string node = path + "/of_node";
	if (File::exists(node)) {

> +		char *ofPath = realpath(ofFile.fileName().c_str(), nullptr);
> +		if (!ofPath)
> +			return -EINVAL;
> +
> +		*id = ofPath;
> +		free(ofPath);
> +
> +		static const std::string dropStr = "/sys/firmware/devicetree/";

Shouldn't we keep the leading "/" in the returned path ?

> +		if (id->find(dropStr) == 0)
> +			id->erase(0, dropStr.length());
> +
> +		return 0;
> +	}
> +
> +	/* ID lookup for ACPI-based systems */
> +	File acpiFile(path + "/firmware_node/path");
> +	if (acpiFile.exists()) {
> +		std::ifstream file(acpiFile.fileName());
> +		if (!file.is_open())
> +			return -EINVAL;
> +
> +		std::getline(file, *id);
> +		file.close();
> +
> +		return 0;
> +	}
> +
> +	return -ENODEV;
> +}
> +
>  } /* namespace sysfs */
>  
>  } /* namespace libcamera */