[{"id":11840,"web_url":"https://patchwork.libcamera.org/comment/11840/","msgid":"<20200804165405.GF6075@pendragon.ideasonboard.com>","date":"2020-08-04T16:54:05","subject":"Re: [libcamera-devel] [PATCH v7 2/9] libcamera: sysfs: Add method\n\tto lookup firmware ID","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Tue, Aug 04, 2020 at 06:13:51PM +0200, Niklas Söderlund wrote:\n> A systems firmware description is recorded differently in sysfs\n\ns/systems/system's/ or just system.\n\n> depending if the system uses OF or ACPI. Add a helper to abstract\n> this, allowing users not to care which of the two are used.\n> \n> For OF-based systems the ID is the full path of the device in the\n> device tree description. For ACPI-based systems the ID is the ACPI\n> firmware nodes path. Both ID sources are guaranteed to be unique and\n> persistent as long as the firmware of the system is not changed.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/internal/sysfs.h | 2 ++\n> src/libcamera/sysfs.cpp | 57 ++++++++++++++++++++++++++++++\n> 2 files changed, 59 insertions(+)\n> \n> diff --git a/include/libcamera/internal/sysfs.h b/include/libcamera/internal/sysfs.h\n> index 72f436205d8d30e8..ef8ec55909d85bf9 100644\n> --- a/include/libcamera/internal/sysfs.h\n> +++ b/include/libcamera/internal/sysfs.h\n> @@ -15,6 +15,8 @@ namespace sysfs {\n> \n> std::string charDevPath(const std::string &devicePath);\n> \n> +int firmwareId(const std::string &path, std::string *id);\n> +\n> } /* namespace sysfs */\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/sysfs.cpp b/src/libcamera/sysfs.cpp\n> index 3b2920663e9c3bcc..98be4df9d38e6fa8 100644\n> --- a/src/libcamera/sysfs.cpp\n> +++ b/src/libcamera/sysfs.cpp\n> @@ -7,9 +7,11 @@\n> \n> #include \"libcamera/internal/sysfs.h\"\n> \n> +#include \n> #include \n> #include \n> \n> +#include \"libcamera/internal/file.h\"\n> #include \"libcamera/internal/log.h\"\n> \n> /**\n> @@ -43,6 +45,61 @@ std::string charDevPath(const std::string &devicePath)\n> \treturn dev.str();\n> }\n> \n> +/**\n> + * \\brief Try to read a device firmware ID from sysfs\n\n\"Retrieve the path of the firmware node for a device\"\n\n?\n\n> + * \\param[in] path Path in sysfs to search\n\nI think \"path\" is a bit ambiguous. Should the parameter be called\n\"device\" or \"devicePath\" ?\n\n> + * \\param[out] id Location to store ID if found\n\nHow about returning the string instead of passing it by pointer ? Or do\nyou need to differentiate between -EINVAL and -ENODEV ? The -EINVAL case\nshould really not happen.\n\nThis is another case where returning a std::tuple\ncould make sense if you don't dislike that.\n\n> + *\n> + * A systems firmware description is recorded differently in sysfs depending if\n> + * the system uses OF or ACPI. Add a helper to abstract this, allowing users not\n> + * to care which of the two are used.\n\n\"Add a helper\" ? :-) Bad copy & paste from the commit message ? I think\nyou can replace this with\n\n * Physical devices in a system are described by the system firmware. Depending\n * on the type of platform, devices are identified using different naming\n * schemes. The Linux kernel abstract those differences with \"firmware nodes\".\n * This function retrieves the firmware node path corresponding to the\n * \\a device.\n\n> + *\n> + * For OF-based systems the ID is the full path of the device in the device tree\n\nI'd say DT instead of OF, as DT is a subset of OF.\n\n> + * description. For ACPI-based systems the ID is the ACPI firmware nodes path.\n> + * Both ID sources are guaranteed to be unique and persistent as long as the\n> + * firmware of the system is not changed.\n\nBeing a bit pendantic, for ACPI, it's the ACPI object absolute namespace\npath. Both DT and ACPI are abstracted by the \"firmware node\" API in the\nkernel. Maybe\n\n * For DT-based systems, the path is the full name of the DT node that\n * represents the device. For ACPI-based systems, the path is the absolute\n * namespace path to the ACPI object that represents the device. In both cases,\n * the path is guaranteed to be unique and persistent as long as the system\n * firmware is not modified.\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EINVAL Error when looking up firmware ID\n> + * \\retval -ENODEV No firmware ID available for \\a path\n> + */\n> +int firmwareId(const std::string &path, std::string *id)\n\nI'm not sure \"Id\" is a proper name for this. Based on the above comment,\nshould the function thus be named firmwareNodePath() ?\n\n> +{\n> +\tASSERT(id);\n> +\n> +\t/* ID lookup for OF-based systems */\n> +\tFile ofFile(path + \"/of_node\");\n> +\tif (ofFile.exists()) {\n\nYou don't need to construct a File object, there's a File::exists()\nstatic function.\n\n\tstd::string node = path + \"/of_node\";\n\tif (File::exists(node)) {\n\n> +\t\tchar *ofPath = realpath(ofFile.fileName().c_str(), nullptr);\n> +\t\tif (!ofPath)\n> +\t\t\treturn -EINVAL;\n> +\n> +\t\t*id = ofPath;\n> +\t\tfree(ofPath);\n> +\n> +\t\tstatic const std::string dropStr = \"/sys/firmware/devicetree/\";\n\nShouldn't we keep the leading \"/\" in the returned path ?\n\n> +\t\tif (id->find(dropStr) == 0)\n> +\t\t\tid->erase(0, dropStr.length());\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\t/* ID lookup for ACPI-based systems */\n> +\tFile acpiFile(path + \"/firmware_node/path\");\n> +\tif (acpiFile.exists()) {\n> +\t\tstd::ifstream file(acpiFile.fileName());\n> +\t\tif (!file.is_open())\n> +\t\t\treturn -EINVAL;\n> +\n> +\t\tstd::getline(file, *id);\n> +\t\tfile.close();\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\treturn -ENODEV;\n> +}\n> +\n> } /* namespace sysfs */\n> \n> } /* namespace libcamera */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AF6F9BD87A\n\tfor ;\n\tTue, 4 Aug 2020 16:54:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 283E06054A;\n\tTue, 4 Aug 2020 18:54:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7A53060545\n\tfor ;\n\tTue, 4 Aug 2020 18:54:17 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EDC6527B;\n\tTue, 4 Aug 2020 18:54:16 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EyrAi7Jq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596560057;\n\tbh=pgO+rBJCyq1FSGQNzg6SFY12ytBDfuXMr0KuEI9p1zI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EyrAi7JqYWJ5GTjl61ExibBfOiWcJ/gg2jJlYwpnnftNHfi86MnHYHgUZgTtb/MDd\n\tXv/fxwkjo6Q5L3C6DJcK4tmo4ot+IyA8I+9Yn7vZOSh80newEi/8OKPdEWDal+NIeH\n\ttXYrYGoMWxFBeqwoyKnOoXoA6nbj69edg//7x774=","Date":"Tue, 4 Aug 2020 19:54:05 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200804165405.GF6075@pendragon.ideasonboard.com>","References":"<20200804161358.1628962-1-niklas.soderlund@ragnatech.se>\n\t<20200804161358.1628962-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200804161358.1628962-3-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v7 2/9] libcamera: sysfs: Add method\n\tto lookup firmware ID","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]