[{"id":11806,"web_url":"https://patchwork.libcamera.org/comment/11806/","msgid":"<20200804073154.jxxrro7vqcxdihmu@uno.localdomain>","date":"2020-08-04T07:31:54","subject":"Re: [libcamera-devel] [PATCH v6 1/9] libcamera: device_enumerator:\n\tAdd method to lookup sysfs path","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Mon, Aug 03, 2020 at 11:17:25PM +0200, Niklas Söderlund wrote:\n> Add an implementation to lookup a device nodes sysfs interface path\n> based on the assumption that device name under /dev matches the one\n> under /sysfs. Subclasses of the DeviceEnumerator may override this to\n> create more clever methods to lookup the path.\n\ns/method/implementation\n\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/internal/device_enumerator.h | 2 ++\n> src/libcamera/device_enumerator.cpp | 14 ++++++++++++++\n> 2 files changed, 16 insertions(+)\n>\n> diff --git a/include/libcamera/internal/device_enumerator.h b/include/libcamera/internal/device_enumerator.h\n> index a9850400229d2b53..11961f4f9304f6d7 100644\n> --- a/include/libcamera/internal/device_enumerator.h\n> +++ b/include/libcamera/internal/device_enumerator.h\n> @@ -45,6 +45,8 @@ public:\n>\n> \tstd::shared_ptr search(const DeviceMatch &dm);\n>\n> +\tvirtual std::string lookupSysfsPath(const std::string &deviceNode);\n\nThat's a quite peculiar name I would say. Any reason I have missed why\nyou retained the 'lookup' part ? As we don't have 'get' in getters,\nI'm not sure if 'lookup' should be there.\n\nWhy not just 'sysfsPath()' ?\n\n> +\n> \tSignal<> devicesAdded;\n>\n> protected:\n> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\n> index 647974b17341f44b..ef2451ac29159bf0 100644\n> --- a/src/libcamera/device_enumerator.cpp\n> +++ b/src/libcamera/device_enumerator.cpp\n> @@ -318,4 +318,18 @@ std::shared_ptr DeviceEnumerator::search(const DeviceMatch &dm)\n> \treturn nullptr;\n> }\n>\n> +/**\n> + * \\brief Lookup a nodes sysfs path\n\nLookup a video device node path in sysfs\n\n> + * \\param[in] deviceNode device node to lookup\n\nThe video device node path in /dev/\n\n> + *\n> + * Lookup \\a deviceNode sysfs interface.\n\nI know nothing about sysfs, but to me a device node has several\nrepresentation in sysfs, by device hierarchy, by class, by device node\ndevnums etc.\n\nThis one returns a very precise one, I would mention that\n\n> + *\n> + * \\return Path in sysfs\n> + */\n> +std::string DeviceEnumerator::lookupSysfsPath(const std::string &deviceNode)\n> +{\n> +\tstd::string dev = basename(deviceNode.c_str());\n> +\treturn \"/sys/class/video4linux/\" + dev;\n> +}\n> +\n> } /* namespace libcamera */\n> --\n> 2.28.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 20A08BD86F\n\tfor ;\n\tTue, 4 Aug 2020 07:28:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9718161B02;\n\tTue, 4 Aug 2020 09:28:14 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E1801609B9\n\tfor ;\n\tTue, 4 Aug 2020 09:28:13 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 3975EFF807;\n\tTue, 4 Aug 2020 07:28:12 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 4 Aug 2020 09:31:54 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200804073154.jxxrro7vqcxdihmu@uno.localdomain>","References":"<20200803211733.1037019-1-niklas.soderlund@ragnatech.se>\n\t<20200803211733.1037019-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200803211733.1037019-2-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v6 1/9] libcamera: device_enumerator:\n\tAdd method to lookup sysfs path","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\" "}},{"id":11821,"web_url":"https://patchwork.libcamera.org/comment/11821/","msgid":"<20200804112441.GA6075@pendragon.ideasonboard.com>","date":"2020-08-04T11:24:41","subject":"Re: [libcamera-devel] [PATCH v6 1/9] libcamera: device_enumerator:\n\tAdd method to lookup sysfs path","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 09:31:54AM +0200, Jacopo Mondi wrote:\n> On Mon, Aug 03, 2020 at 11:17:25PM +0200, Niklas Söderlund wrote:\n> > Add an implementation to lookup a device nodes sysfs interface path\n\ns/nodes/node's/\n\n> > based on the assumption that device name under /dev matches the one\n> > under /sysfs. Subclasses of the DeviceEnumerator may override this to\n> > create more clever methods to lookup the path.\n> \n> s/method/implementation\n\nIsn't \"method\" an appropriate term too ?\n\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/internal/device_enumerator.h | 2 ++\n> > src/libcamera/device_enumerator.cpp | 14 ++++++++++++++\n> > 2 files changed, 16 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/device_enumerator.h b/include/libcamera/internal/device_enumerator.h\n> > index a9850400229d2b53..11961f4f9304f6d7 100644\n> > --- a/include/libcamera/internal/device_enumerator.h\n> > +++ b/include/libcamera/internal/device_enumerator.h\n> > @@ -45,6 +45,8 @@ public:\n> >\n> > \tstd::shared_ptr search(const DeviceMatch &dm);\n> >\n> > +\tvirtual std::string lookupSysfsPath(const std::string &deviceNode);\n> \n> That's a quite peculiar name I would say. Any reason I have missed why\n> you retained the 'lookup' part ? As we don't have 'get' in getters,\n> I'm not sure if 'lookup' should be there.\n> \n> Why not just 'sysfsPath()' ?\n\nI also think it would be better.\n\nNiklas, conceptual question, would it make sense to move this\nimplementation to the sysfs enumerator, keeping the function purely\nvirtual in the base class ?\n\n> > +\n> > \tSignal<> devicesAdded;\n> >\n> > protected:\n> > diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\n> > index 647974b17341f44b..ef2451ac29159bf0 100644\n> > --- a/src/libcamera/device_enumerator.cpp\n> > +++ b/src/libcamera/device_enumerator.cpp\n> > @@ -318,4 +318,18 @@ std::shared_ptr DeviceEnumerator::search(const DeviceMatch &dm)\n> > \treturn nullptr;\n> > }\n> >\n> > +/**\n> > + * \\brief Lookup a nodes sysfs path\n> \n> Lookup a video device node path in sysfs\n\nI'd talk about character device instead of video device, as this is\ngeneric.\n\nExcept it's not now that I read the implementation below :-) I should\nhave thought about it, but how about\n\nstd::string DeviceEnumerator::lookupSysfsPath(const std::string &deviceNode)\n{\n\tstruct stat st;\n\tint ret = stat(deviceNode.c_str(), &st);\n\tif (ret < 0) {\n\t\tLOG(DeviceEnumerator, Error)\n\t\t\t<< \"Unable to stat '\" << deviceNode << \"'\";\n\t\treturn {};\n\t}\n\n\tstd::ostringstream dev(\"/sys/dev/char/\", std::ios_base::ate);\n\tdev << major(st.st_rdev) << \":\" << minor(st.st_rdev);\n\treturn dev.str();\n}\n\nThis should work in all cases and wouldn't require udev at all. The code\ncould even be moved to V4L2Device::devicePath(), as it's the only user.\nI think this would have my preference for now.\n\nAnother option is to add a sysfs.cpp file for this function and\ntryLookupFirmwareID(). No need to add classes there, a simple\nlibcamera::sysfs namespace would do (maybe we'll refactor that in a\nCharDev class later, if the need arises).\n\n> > + * \\param[in] deviceNode device node to lookup\n> \n> The video device node path in /dev/\n> \n> > + *\n> > + * Lookup \\a deviceNode sysfs interface.\n> \n> I know nothing about sysfs, but to me a device node has several\n> representation in sysfs, by device hierarchy, by class, by device node\n> devnums etc.\n> \n> This one returns a very precise one, I would mention that\n> \n> > + *\n> > + * \\return Path in sysfs\n> > + */\n> > +std::string DeviceEnumerator::lookupSysfsPath(const std::string &deviceNode)\n> > +{\n> > +\tstd::string dev = basename(deviceNode.c_str());\n> > +\treturn \"/sys/class/video4linux/\" + dev;\n> > +}\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 E5DFDBD87A\n\tfor ;\n\tTue, 4 Aug 2020 11:25:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7DFB66043C;\n\tTue, 4 Aug 2020 13:25:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3CFE9603C9\n\tfor ;\n\tTue, 4 Aug 2020 13:24:59 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-79-50-nat.elisa-mobile.fi\n\t[85.76.79.50])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 86B5827B;\n\tTue, 4 Aug 2020 13:24:54 +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=\"jePpedrx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596540298;\n\tbh=9AleTY5wjruLIrKF8ct2g56Nlih6YoJj9NTw1Enbm9Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jePpedrxnLOgoOCNDKt8HDgEbrwRoYgqLHbQg2ox0t5kEZZ9Ib+tBkObs7oDlWvLY\n\tz0wHGl9Cx24OA4lJYnzxdjb0crartqrx/vsjUm5+dhAFL5VAUNQ4AgYiRdTUu3aNzj\n\tSMMxqv3YsKlYONiuh/tewWy2dKU2pXSrbfjpT2Ng=","Date":"Tue, 4 Aug 2020 14:24:41 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200804112441.GA6075@pendragon.ideasonboard.com>","References":"<20200803211733.1037019-1-niklas.soderlund@ragnatech.se>\n\t<20200803211733.1037019-2-niklas.soderlund@ragnatech.se>\n\t<20200804073154.jxxrro7vqcxdihmu@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200804073154.jxxrro7vqcxdihmu@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v6 1/9] libcamera: device_enumerator:\n\tAdd method to lookup sysfs path","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\" "}}]