[libcamera-devel] [PATCH v6 4/9] libcamera: utils: Add method to lookup firmware ID in sysfs

[Jacopo Mondi]

Hi Niklas,

On Mon, Aug 03, 2020 at 11:17:28PM +0200, Niklas Söderlund wrote:
> A devices firmware description is recorded differently in sysfs
> depending on if the devices uses OF or ACPI. Add a helper to abstract

s/on if/if/
s/devices/device (and that's more likely a system decision, not a
device one)

> this, allowing users to not care which of the two are used.

users not to care

>
> 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/utils.h |  2 +
>  src/libcamera/utils.cpp            | 61 ++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+)
>
> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h
> index 45cd6f120c51586b..69977340e2593db6 100644
> --- a/include/libcamera/internal/utils.h
> +++ b/include/libcamera/internal/utils.h
> @@ -200,6 +200,8 @@ details::StringSplitter split(const std::string &str, const std::string &delim);
>  std::string libcameraBuildPath();
>  std::string libcameraSourcePath();
>
> +int tryLookupFirmwareID(const std::string &devPath, std::string *id);
> +
>  constexpr unsigned int alignDown(unsigned int value, unsigned int alignment)
>  {
>  	return value / alignment * alignment;
> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> index 615df46ac142a2a9..07ebce61f230e5f0 100644
> --- a/src/libcamera/utils.cpp
> +++ b/src/libcamera/utils.cpp
> @@ -9,6 +9,7 @@
>
>  #include <dlfcn.h>
>  #include <elf.h>
> +#include <fstream>
>  #include <iomanip>
>  #include <limits.h>
>  #include <link.h>
> @@ -444,6 +445,66 @@ std::string libcameraSourcePath()
>  	return path + "/";
>  }
>
> +/**
> + * \brief Try to read a device firmware ID from sysfs
> + * \param[in] devPath Path in sysfs to search
> + * \param[out] id Location to store ID if found
> + *
> + * The ID recorded in the devices firmware description is recorded differently
> + * in sysfs depending on if the devices uses OF or ACPI. This helper abstracts
> + * this away, allowing callers to not care which of the two are used.

Same comments as for the commit message

> + *
> + * 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.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -ENOMEM \a id is nullptr
> + * \retval -EINVAL Error when looking up firmware ID
> + * \retval -ENODEV No firmware ID aviable for device

available

> + */
> +int tryLookupFirmwareID(const std::string &devPath, std::string *id)

I'm going to be picky on names, I know, but why 'try' ? Beause this
function can fail ? Can't the other ones ? I would just 'firwareId()'

> +{
> +	struct stat statbuf;
> +	std::string path;
> +
> +	if (!id)
> +		return -EINVAL;

How can you pass a null string pointer ? By calling this method with
nullptr explicitely only, isn't it ? Is this worth this check ?

> +
> +	/* Try to lookup ID as if the system where OF-based. */

I don't get what this comment means. Maybe 'as is the system was
OF-based' ? Why not 'ID lookup for OF-based systems' ?

> +	path = devPath + "/of_node";
> +	if (stat(path.c_str(), &statbuf) == 0) {

Can stat fail for other reasons and not just because the file is not
there ? Should be check the return value and propagate errors which
are not caused by the file not being there ? Looking at the stat man
page it's not trivial though as I see both ENOENT and ENODIR could be
returned..

> +		char *ofPath = realpath(path.c_str(), nullptr);
> +		if (!ofPath)
> +			return -EINVAL;
> +
> +		*id = ofPath;
> +		free(ofPath);
> +
> +		static const std::string dropStr = "/sys/firmware/devicetree/";
> +		if (id->find(dropStr) == 0)
> +			id->erase(0, dropStr.length());
> +
> +		return 0;
> +	}
> +
> +	/* Try to lookup ID as if the system where ACPI-based. */

Same

> +	path = devPath + "/firmware_node/path";
> +	if (stat(path.c_str(), &statbuf) == 0) {

And same here as well :)

> +		std::ifstream file(path.c_str());
> +		if (!file.is_open())
> +			return -EINVAL;
> +
> +		std::getline(file, *id);
> +		file.close();
> +
> +		return 0;
> +	}
> +
> +	return -ENODEV;
> +}
> +
>  /**
>   * \fn alignDown(unsigned int value, unsigned int alignment)
>   * \brief Align \a value down to \a alignment
> --
> 2.28.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel