[{"id":185,"web_url":"https://patchwork.libcamera.org/comment/185/","msgid":"<20190102225400.GB19161@bigcity.dyn.berto.se>","date":"2019-01-02T22:54:00","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: Use 'struct' for\n\tstructure types","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your patch.\n\nOn 2019-01-02 13:02:56 +0100, Jacopo Mondi wrote:\n> Add back the 'struct' keyword for structure types.\n> C++ allows omitting the 'struct' keywork. Add it back to make clear\n> we're dealing with structures and not class types.\n> \n> While at there re-align lines to first open brace, or angular brace for\n> casts, and re-sort lines to have the longest one first in populate()\n> function.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/media_device.cpp | 33 ++++++++++++++++-----------------\n> 1 file changed, 16 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index 34206c8..715d971 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -205,10 +205,10 @@ void MediaDevice::close()\n> int MediaDevice::populate()\n> {\n> \tstruct media_v2_topology topology = { };\n> +\tstruct media_v2_interface *interfaces = nullptr;\n> \tstruct media_v2_entity *ents = nullptr;\n> \tstruct media_v2_link *links = nullptr;\n> \tstruct media_v2_pad *pads = nullptr;\n> -\tstruct media_v2_interface *interfaces = nullptr;\n> \t__u64 version = -1;\n> \tint ret;\n\nI don't like this. I think the variables should be declared and \nprocessed in the same order as in struct media_v2_topology. It makes the \ncode so much more readable when comparing it with the kernel header.\n\n- ptr_entities\n- ptr_interfaces\n- ptr_pads\n- ptr_links\n\nI agree reverse xmas tree is usually the way to sort this but as this is \nreferences to a structure IMHO it takes precedence.\n\n> \n> @@ -219,10 +219,10 @@ int MediaDevice::populate()\n> \t */\n> \twhile (true) {\n> \t\ttopology.topology_version = 0;\n> +\t\ttopology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);\n> \t\ttopology.ptr_entities = reinterpret_cast<__u64>(ents);\n> \t\ttopology.ptr_links = reinterpret_cast<__u64>(links);\n> \t\ttopology.ptr_pads = reinterpret_cast<__u64>(pads);\n> -\t\ttopology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);\n> \n> \t\tret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);\n> \t\tif (ret < 0) {\n> @@ -235,15 +235,15 @@ int MediaDevice::populate()\n> \t\tif (version == topology.topology_version)\n> \t\t\tbreak;\n> \n> +\t\tdelete[] interfaces;\n> \t\tdelete[] links;\n> \t\tdelete[] ents;\n> \t\tdelete[] pads;\n> -\t\tdelete[] interfaces;\n> \n> -\t\tents = new media_v2_entity[topology.num_entities];\n> -\t\tlinks = new media_v2_link[topology.num_links];\n> -\t\tpads = new media_v2_pad[topology.num_pads];\n> -\t\tinterfaces = new media_v2_interface[topology.num_interfaces];\n> +\t\tinterfaces = new struct media_v2_interface[topology.num_interfaces];\n> +\t\tents = new struct media_v2_entity[topology.num_entities];\n> +\t\tlinks = new struct media_v2_link[topology.num_links];\n> +\t\tpads = new struct media_v2_pad[topology.num_pads];\n> \n> \t\tversion = topology.topology_version;\n> \t}\n> @@ -254,10 +254,10 @@ int MediaDevice::populate()\n> \t populateLinks(topology))\n> \t\tvalid_ = true;\n> \n> +\tdelete[] interfaces;\n> \tdelete[] links;\n> \tdelete[] ents;\n> \tdelete[] pads;\n> -\tdelete[] interfaces;\n> \n> \tif (!valid_) {\n> \t\tclear();\n> @@ -417,17 +417,16 @@ struct media_v2_interface *MediaDevice::findInterface(const struct media_v2_topo\n> */\n> bool MediaDevice::populateEntities(const struct media_v2_topology &topology)\n> {\n> -\tmedia_v2_entity *mediaEntities = reinterpret_cast\n> -\t\t\t\t\t (topology.ptr_entities);\n> +\tstruct media_v2_entity *mediaEntities = reinterpret_cast\n> +\t\t\t\t\t\t\t\t(topology.ptr_entities);\n> \n> \tfor (unsigned int i = 0; i < topology.num_entities; ++i) {\n> \t\t/*\n> \t\t * Find the interface linked to this entity to get the device\n> \t\t * node major and minor numbers.\n> \t\t */\n> -\t\tstruct media_v2_interface *iface =\n> -\t\t\tfindInterface(topology, mediaEntities[i].id);\n> -\n> +\t\tstruct media_v2_interface *iface = findInterface(topology,\n> +\t\t\t\t\t\t\t\t mediaEntities[i].id);\n\nNit-pic, I find the first way of writing this much more readable.\n\n> \t\tMediaEntity *entity;\n> \t\tif (iface)\n> \t\t\tentity = new MediaEntity(&mediaEntities[i],\n> @@ -449,8 +448,8 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology)\n> \n> bool MediaDevice::populatePads(const struct media_v2_topology &topology)\n> {\n> -\tmedia_v2_pad *mediaPads = reinterpret_cast\n> -\t\t\t\t (topology.ptr_pads);\n> +\tstruct media_v2_pad *mediaPads = reinterpret_cast\n> +\t\t\t\t\t\t\t (topology.ptr_pads);\n> \n> \tfor (unsigned int i = 0; i < topology.num_pads; ++i) {\n> \t\tunsigned int entity_id = mediaPads[i].entity_id;\n> @@ -478,8 +477,8 @@ bool MediaDevice::populatePads(const struct media_v2_topology &topology)\n> \n> bool MediaDevice::populateLinks(const struct media_v2_topology &topology)\n> {\n> -\tmedia_v2_link *mediaLinks = reinterpret_cast\n> -\t\t\t\t (topology.ptr_links);\n> +\tstruct media_v2_link *mediaLinks = reinterpret_cast\n> +\t\t\t\t\t\t\t (topology.ptr_links);\n> \n> \tfor (unsigned int i = 0; i < topology.num_links; ++i) {\n> \t\t/*\n> --\n> 2.20.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 953E460B2F\n\tfor ;\n\tWed, 2 Jan 2019 23:54:02 +0100 (CET)","by mail-lj1-x244.google.com with SMTP id c19-v6so28294437lja.5\n\tfor ;\n\tWed, 02 Jan 2019 14:54:02 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tz6sm10453035lfd.50.2019.01.02.14.54.01\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tWed, 02 Jan 2019 14:54:01 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=SLf5/EJRgXJeXNQzdLg2WalCgDfNAvxQu0pStx8FIr8=;\n\tb=aJbRADtkFZlFVICRH7rdxObLPzVTfaJsJZxI56RJFtQKptEMBD1Xp7EVYY8/hhAB7w\n\tSLLYrco4hrC8MZI1GUySoenboG71BMmqZN8fxrmAdj9FqxPc2Ayk1ddRnuQDe88OLuPP\n\thcm9rxwhfs0C4QYAWycjfMm1919f9L5ClXiK7YoQnVRVTHEvlmGlbwMz2r7xNPXAa+TD\n\t9Y09ICAno90/hNM+AFIuiOsQfmT5oIXJ4YoKMHwJUCKhPjPZlKC7pqLy9W9ECZOFznMl\n\thRgZNUCE9vLoGp66TaEO7TaYEPKYlVY5SWEpdWrZq8jtOPHNupZmFWtxmEGyGiJx0Lhl\n\tIjZw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=SLf5/EJRgXJeXNQzdLg2WalCgDfNAvxQu0pStx8FIr8=;\n\tb=URIZLmJ2qRvTJdFH3AgEMZHLyNGkfgJn3kyTI7/TEwttdRA9oWq/NBUqRZTuEaVb50\n\trzcUbGp2xuaLV91bTmXL311HeKYRwxrXhf7ELlv8ea7tUIg/WT9XFyip+7toLrJjDeC6\n\tQ1+GIX0PZqamMVN7SjMFtDnK+Yku9+NzJTmvs5rLU+aCzet+KMGwFms7PYf2ySn67p3S\n\tA1AyCmxxedJoNL4YWuZ3UQ7ZzTRQMXRkKa8+i+/bFSOEC3+C9wJnUdSJCFbU66wrheJN\n\tt1Qro/M4MADLTgjzJPQg3C9MZ1NYqgMt7M8cyb5SYdydXDxwzwsHsRqeRg3hwU9OrvIz\n\tm/SQ==","X-Gm-Message-State":"AJcUukdnB8/wcOH9MWeHMNB2pRdE4Y6Q968Zij5YVbMB4iy0gv1Rj8kK\n\t2EP1GdNalTtoW9XL+1fxokE3jA==","X-Google-Smtp-Source":"ALg8bN72UbJ5/Gs46oeSMC2QC1mPsrygCK21dDPnzfx93XDnhFJ7tIcCDnls2PM6uqw3huiOSvboDA==","X-Received":"by 2002:a2e:9bc3:: with SMTP id\n\tw3-v6mr24858679ljj.70.1546469641727; \n\tWed, 02 Jan 2019 14:54:01 -0800 (PST)","Date":"Wed, 2 Jan 2019 23:54:00 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190102225400.GB19161@bigcity.dyn.berto.se>","References":"<20190102120256.7769-1-jacopo@jmondi.org>\n\t<20190102120256.7769-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190102120256.7769-2-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: Use 'struct' for\n\tstructure types","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 02 Jan 2019 22:54:02 -0000"}},{"id":186,"web_url":"https://patchwork.libcamera.org/comment/186/","msgid":"<20190103075726.im7emjb6i4ssr43w@uno.localdomain>","date":"2019-01-03T07:57:26","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: Use 'struct' for\n\tstructure types","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Jan 02, 2019 at 11:54:00PM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your patch.\n>\n> On 2019-01-02 13:02:56 +0100, Jacopo Mondi wrote:\n> > Add back the 'struct' keyword for structure types.\n> > C++ allows omitting the 'struct' keywork. Add it back to make clear\n> > we're dealing with structures and not class types.\n> >\n> > While at there re-align lines to first open brace, or angular brace for\n> > casts, and re-sort lines to have the longest one first in populate()\n> > function.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/media_device.cpp | 33 ++++++++++++++++-----------------\n> > 1 file changed, 16 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> > index 34206c8..715d971 100644\n> > --- a/src/libcamera/media_device.cpp\n> > +++ b/src/libcamera/media_device.cpp\n> > @@ -205,10 +205,10 @@ void MediaDevice::close()\n> > int MediaDevice::populate()\n> > {\n> > \tstruct media_v2_topology topology = { };\n> > +\tstruct media_v2_interface *interfaces = nullptr;\n> > \tstruct media_v2_entity *ents = nullptr;\n> > \tstruct media_v2_link *links = nullptr;\n> > \tstruct media_v2_pad *pads = nullptr;\n> > -\tstruct media_v2_interface *interfaces = nullptr;\n> > \t__u64 version = -1;\n> > \tint ret;\n>\n> I don't like this. I think the variables should be declared and\n> processed in the same order as in struct media_v2_topology. It makes the\n> code so much more readable when comparing it with the kernel header.\n>\n> - ptr_entities\n> - ptr_interfaces\n> - ptr_pads\n> - ptr_links\n>\n> I agree reverse xmas tree is usually the way to sort this but as this is\n> references to a structure IMHO it takes precedence.\n>\n\nThanks, I see. I'll leave it out then.\nFrom now that we have stabilized a little more on a style, I'll waste less\ntime on this minor nits.\n\n> >\n> > @@ -219,10 +219,10 @@ int MediaDevice::populate()\n> > \t */\n> > \twhile (true) {\n> > \t\ttopology.topology_version = 0;\n> > +\t\ttopology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);\n> > \t\ttopology.ptr_entities = reinterpret_cast<__u64>(ents);\n> > \t\ttopology.ptr_links = reinterpret_cast<__u64>(links);\n> > \t\ttopology.ptr_pads = reinterpret_cast<__u64>(pads);\n> > -\t\ttopology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);\n> >\n> > \t\tret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);\n> > \t\tif (ret < 0) {\n> > @@ -235,15 +235,15 @@ int MediaDevice::populate()\n> > \t\tif (version == topology.topology_version)\n> > \t\t\tbreak;\n> >\n> > +\t\tdelete[] interfaces;\n> > \t\tdelete[] links;\n> > \t\tdelete[] ents;\n> > \t\tdelete[] pads;\n> > -\t\tdelete[] interfaces;\n> >\n> > -\t\tents = new media_v2_entity[topology.num_entities];\n> > -\t\tlinks = new media_v2_link[topology.num_links];\n> > -\t\tpads = new media_v2_pad[topology.num_pads];\n> > -\t\tinterfaces = new media_v2_interface[topology.num_interfaces];\n> > +\t\tinterfaces = new struct media_v2_interface[topology.num_interfaces];\n> > +\t\tents = new struct media_v2_entity[topology.num_entities];\n> > +\t\tlinks = new struct media_v2_link[topology.num_links];\n> > +\t\tpads = new struct media_v2_pad[topology.num_pads];\n> >\n> > \t\tversion = topology.topology_version;\n> > \t}\n> > @@ -254,10 +254,10 @@ int MediaDevice::populate()\n> > \t populateLinks(topology))\n> > \t\tvalid_ = true;\n> >\n> > +\tdelete[] interfaces;\n> > \tdelete[] links;\n> > \tdelete[] ents;\n> > \tdelete[] pads;\n> > -\tdelete[] interfaces;\n> >\n> > \tif (!valid_) {\n> > \t\tclear();\n> > @@ -417,17 +417,16 @@ struct media_v2_interface *MediaDevice::findInterface(const struct media_v2_topo\n> > */\n> > bool MediaDevice::populateEntities(const struct media_v2_topology &topology)\n> > {\n> > -\tmedia_v2_entity *mediaEntities = reinterpret_cast\n> > -\t\t\t\t\t (topology.ptr_entities);\n> > +\tstruct media_v2_entity *mediaEntities = reinterpret_cast\n> > +\t\t\t\t\t\t\t\t(topology.ptr_entities);\n> >\n> > \tfor (unsigned int i = 0; i < topology.num_entities; ++i) {\n> > \t\t/*\n> > \t\t * Find the interface linked to this entity to get the device\n> > \t\t * node major and minor numbers.\n> > \t\t */\n> > -\t\tstruct media_v2_interface *iface =\n> > -\t\t\tfindInterface(topology, mediaEntities[i].id);\n> > -\n> > +\t\tstruct media_v2_interface *iface = findInterface(topology,\n> > +\t\t\t\t\t\t\t\t mediaEntities[i].id);\n>\n> Nit-pic, I find the first way of writing this much more readable.\n\nOk, as you wish.\nI'll resubmit (and push?) only the addition of 'struct'\n\nThanks\n j\n\n>\n> > \t\tMediaEntity *entity;\n> > \t\tif (iface)\n> > \t\t\tentity = new MediaEntity(&mediaEntities[i],\n> > @@ -449,8 +448,8 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology)\n> >\n> > bool MediaDevice::populatePads(const struct media_v2_topology &topology)\n> > {\n> > -\tmedia_v2_pad *mediaPads = reinterpret_cast\n> > -\t\t\t\t (topology.ptr_pads);\n> > +\tstruct media_v2_pad *mediaPads = reinterpret_cast\n> > +\t\t\t\t\t\t\t (topology.ptr_pads);\n> >\n> > \tfor (unsigned int i = 0; i < topology.num_pads; ++i) {\n> > \t\tunsigned int entity_id = mediaPads[i].entity_id;\n> > @@ -478,8 +477,8 @@ bool MediaDevice::populatePads(const struct media_v2_topology &topology)\n> >\n> > bool MediaDevice::populateLinks(const struct media_v2_topology &topology)\n> > {\n> > -\tmedia_v2_link *mediaLinks = reinterpret_cast\n> > -\t\t\t\t (topology.ptr_links);\n> > +\tstruct media_v2_link *mediaLinks = reinterpret_cast\n> > +\t\t\t\t\t\t\t (topology.ptr_links);\n> >\n> > \tfor (unsigned int i = 0; i < topology.num_links; ++i) {\n> > \t\t/*\n> > --\n> > 2.20.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D508F60B31\n\tfor ;\n\tThu, 3 Jan 2019 08:57:22 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 629541BF209;\n\tThu, 3 Jan 2019 07:57:22 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 3 Jan 2019 08:57:26 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190103075726.im7emjb6i4ssr43w@uno.localdomain>","References":"<20190102120256.7769-1-jacopo@jmondi.org>\n\t<20190102120256.7769-2-jacopo@jmondi.org>\n\t<20190102225400.GB19161@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"brotpyxkz4yn2h4v\"","Content-Disposition":"inline","In-Reply-To":"<20190102225400.GB19161@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: Use 'struct' for\n\tstructure types","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 03 Jan 2019 07:57:23 -0000"}},{"id":190,"web_url":"https://patchwork.libcamera.org/comment/190/","msgid":"<20190103160952.GB22790@bigcity.dyn.berto.se>","date":"2019-01-03T16:09:52","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: Use 'struct' for\n\tstructure types","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2019-01-03 08:57:26 +0100, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Wed, Jan 02, 2019 at 11:54:00PM +0100, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for your patch.\n> >\n> > On 2019-01-02 13:02:56 +0100, Jacopo Mondi wrote:\n> > > Add back the 'struct' keyword for structure types.\n> > > C++ allows omitting the 'struct' keywork. Add it back to make clear\n> > > we're dealing with structures and not class types.\n> > >\n> > > While at there re-align lines to first open brace, or angular brace for\n> > > casts, and re-sort lines to have the longest one first in populate()\n> > > function.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/media_device.cpp | 33 ++++++++++++++++-----------------\n> > > 1 file changed, 16 insertions(+), 17 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> > > index 34206c8..715d971 100644\n> > > --- a/src/libcamera/media_device.cpp\n> > > +++ b/src/libcamera/media_device.cpp\n> > > @@ -205,10 +205,10 @@ void MediaDevice::close()\n> > > int MediaDevice::populate()\n> > > {\n> > > \tstruct media_v2_topology topology = { };\n> > > +\tstruct media_v2_interface *interfaces = nullptr;\n> > > \tstruct media_v2_entity *ents = nullptr;\n> > > \tstruct media_v2_link *links = nullptr;\n> > > \tstruct media_v2_pad *pads = nullptr;\n> > > -\tstruct media_v2_interface *interfaces = nullptr;\n> > > \t__u64 version = -1;\n> > > \tint ret;\n> >\n> > I don't like this. I think the variables should be declared and\n> > processed in the same order as in struct media_v2_topology. It makes the\n> > code so much more readable when comparing it with the kernel header.\n> >\n> > - ptr_entities\n> > - ptr_interfaces\n> > - ptr_pads\n> > - ptr_links\n> >\n> > I agree reverse xmas tree is usually the way to sort this but as this is\n> > references to a structure IMHO it takes precedence.\n> >\n> \n> Thanks, I see. I'll leave it out then.\n> From now that we have stabilized a little more on a style, I'll waste less\n> time on this minor nits.\n> \n> > >\n> > > @@ -219,10 +219,10 @@ int MediaDevice::populate()\n> > > \t */\n> > > \twhile (true) {\n> > > \t\ttopology.topology_version = 0;\n> > > +\t\ttopology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);\n> > > \t\ttopology.ptr_entities = reinterpret_cast<__u64>(ents);\n> > > \t\ttopology.ptr_links = reinterpret_cast<__u64>(links);\n> > > \t\ttopology.ptr_pads = reinterpret_cast<__u64>(pads);\n> > > -\t\ttopology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);\n> > >\n> > > \t\tret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);\n> > > \t\tif (ret < 0) {\n> > > @@ -235,15 +235,15 @@ int MediaDevice::populate()\n> > > \t\tif (version == topology.topology_version)\n> > > \t\t\tbreak;\n> > >\n> > > +\t\tdelete[] interfaces;\n> > > \t\tdelete[] links;\n> > > \t\tdelete[] ents;\n> > > \t\tdelete[] pads;\n> > > -\t\tdelete[] interfaces;\n> > >\n> > > -\t\tents = new media_v2_entity[topology.num_entities];\n> > > -\t\tlinks = new media_v2_link[topology.num_links];\n> > > -\t\tpads = new media_v2_pad[topology.num_pads];\n> > > -\t\tinterfaces = new media_v2_interface[topology.num_interfaces];\n> > > +\t\tinterfaces = new struct media_v2_interface[topology.num_interfaces];\n> > > +\t\tents = new struct media_v2_entity[topology.num_entities];\n> > > +\t\tlinks = new struct media_v2_link[topology.num_links];\n> > > +\t\tpads = new struct media_v2_pad[topology.num_pads];\n> > >\n> > > \t\tversion = topology.topology_version;\n> > > \t}\n> > > @@ -254,10 +254,10 @@ int MediaDevice::populate()\n> > > \t populateLinks(topology))\n> > > \t\tvalid_ = true;\n> > >\n> > > +\tdelete[] interfaces;\n> > > \tdelete[] links;\n> > > \tdelete[] ents;\n> > > \tdelete[] pads;\n> > > -\tdelete[] interfaces;\n> > >\n> > > \tif (!valid_) {\n> > > \t\tclear();\n> > > @@ -417,17 +417,16 @@ struct media_v2_interface *MediaDevice::findInterface(const struct media_v2_topo\n> > > */\n> > > bool MediaDevice::populateEntities(const struct media_v2_topology &topology)\n> > > {\n> > > -\tmedia_v2_entity *mediaEntities = reinterpret_cast\n> > > -\t\t\t\t\t (topology.ptr_entities);\n> > > +\tstruct media_v2_entity *mediaEntities = reinterpret_cast\n> > > +\t\t\t\t\t\t\t\t(topology.ptr_entities);\n> > >\n> > > \tfor (unsigned int i = 0; i < topology.num_entities; ++i) {\n> > > \t\t/*\n> > > \t\t * Find the interface linked to this entity to get the device\n> > > \t\t * node major and minor numbers.\n> > > \t\t */\n> > > -\t\tstruct media_v2_interface *iface =\n> > > -\t\t\tfindInterface(topology, mediaEntities[i].id);\n> > > -\n> > > +\t\tstruct media_v2_interface *iface = findInterface(topology,\n> > > +\t\t\t\t\t\t\t\t mediaEntities[i].id);\n> >\n> > Nit-pic, I find the first way of writing this much more readable.\n> \n> Ok, as you wish.\n> I'll resubmit (and push?) only the addition of 'struct'\n\nPlease do, for just the struct changes feel free to add\n\nReviewed-by: Niklas Söderlund \n\n> \n> Thanks\n> j\n> \n> >\n> > > \t\tMediaEntity *entity;\n> > > \t\tif (iface)\n> > > \t\t\tentity = new MediaEntity(&mediaEntities[i],\n> > > @@ -449,8 +448,8 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology)\n> > >\n> > > bool MediaDevice::populatePads(const struct media_v2_topology &topology)\n> > > {\n> > > -\tmedia_v2_pad *mediaPads = reinterpret_cast\n> > > -\t\t\t\t (topology.ptr_pads);\n> > > +\tstruct media_v2_pad *mediaPads = reinterpret_cast\n> > > +\t\t\t\t\t\t\t (topology.ptr_pads);\n> > >\n> > > \tfor (unsigned int i = 0; i < topology.num_pads; ++i) {\n> > > \t\tunsigned int entity_id = mediaPads[i].entity_id;\n> > > @@ -478,8 +477,8 @@ bool MediaDevice::populatePads(const struct media_v2_topology &topology)\n> > >\n> > > bool MediaDevice::populateLinks(const struct media_v2_topology &topology)\n> > > {\n> > > -\tmedia_v2_link *mediaLinks = reinterpret_cast\n> > > -\t\t\t\t (topology.ptr_links);\n> > > +\tstruct media_v2_link *mediaLinks = reinterpret_cast\n> > > +\t\t\t\t\t\t\t (topology.ptr_links);\n> > >\n> > > \tfor (unsigned int i = 0; i < topology.num_links; ++i) {\n> > > \t\t/*\n> > > --\n> > > 2.20.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","headers":{"Return-Path":"","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C4C7060B13\n\tfor ;\n\tThu, 3 Jan 2019 17:09:54 +0100 (CET)","by mail-lj1-x242.google.com with SMTP id x85-v6so30137354ljb.2\n\tfor ;\n\tThu, 03 Jan 2019 08:09:54 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\t12-v6sm11778214ljf.96.2019.01.03.08.09.53\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 03 Jan 2019 08:09:53 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=FpfM3O28BI8MzW4Dkb1Onugu0h+sgyUHD15GVKq4umo=;\n\tb=vGSDIVx9HKE5Ww9CYZvd3mi2pDHQ+yHk411e97LpktYBfgJRfAtLHYWtp7YVUHgp/L\n\tMKANWKu+P6dOkRCBefaIUKcrsnQ2ED7lekdpiG5xk1mdel80p/7M6o9PXjAVZzEp5hGF\n\tLxtopvB/N3vSCwgwXIeyvRafH55imXJmNeFhGZdz6zxqxB7XUDWm9nhdccGaPjpzHbnw\n\tNo0uhQD6iCzmUlmlbEyPzvS+4sHI7aEma6OWR5IzNwCFkeKADhPZPG+8im+9bpYbs9k0\n\tLYmh62kECZIBSYKNZHkKyAil6lnmhJhEEC67kmlC7FAY/GkrC4AM8RqB5Gfk/KB+uRoo\n\tRUWQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=FpfM3O28BI8MzW4Dkb1Onugu0h+sgyUHD15GVKq4umo=;\n\tb=sOgWXViLw1aTUt5dh1AWQA1rA6KZfzwpHMEXBwMNAYitrESGq6qDWX/QUtog9YLJq7\n\tD2RFXA9G4Rxh4GDFa/Gru8vAU5/5YkSyOgdYa2h8p5HSV7uFzbrmL4XIi9CVhYijVI1X\n\t4JVyPpxWZZ/L2gWFrrIeRDlh5hZRumIWsT2t+BSTfGshriHLB/Pbg2sJ4n6LOKhvVrWB\n\tqX90OiAkbBt/uP7DkbhpQDgYaWmZcpUKIR0ZUags+UnvroRwIBwWOuQpAYL5/aS4rSkH\n\tdC8oNwgwhZlp00+QUITLTRJTm3NJXwMMTEiUh+/Xd3mX8AxHs7sXUOHDaH5s9DXx2bIr\n\tNHpg==","X-Gm-Message-State":"AJcUukcwvulgPG92kw8tVEF3F1eB6f/nh+tD4loegkGY1qWa9AgcxTkx\n\tImBThAcdbDOKXF7J/K/3y2p16A==","X-Google-Smtp-Source":"ALg8bN5+73oN5Eozm6TxNVg8ZUSVHFHOIdUzHmvlV/L36pUQ1ey10macU9cQV+Uxv8UFCONGWVf6Dg==","X-Received":"by 2002:a2e:92ca:: with SMTP id\n\tk10-v6mr26399101ljh.63.1546531793844; \n\tThu, 03 Jan 2019 08:09:53 -0800 (PST)","Date":"Thu, 3 Jan 2019 17:09:52 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190103160952.GB22790@bigcity.dyn.berto.se>","References":"<20190102120256.7769-1-jacopo@jmondi.org>\n\t<20190102120256.7769-2-jacopo@jmondi.org>\n\t<20190102225400.GB19161@bigcity.dyn.berto.se>\n\t<20190103075726.im7emjb6i4ssr43w@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190103075726.im7emjb6i4ssr43w@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: Use 'struct' for\n\tstructure types","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 03 Jan 2019 16:09:55 -0000"}}]