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

[Laurent Pinchart]

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 at 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 */

-- 
Regards,

Laurent Pinchart