linux-kernel - Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 28 Dec 2020 22:37:38 +0000
From:   Daniel Scally <djrscally@...il.com>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        devel@...ica.org, "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Yong Zhi <yong.zhi@...el.com>,
        Bingbu Cao <bingbu.cao@...el.com>,
        Tian Shu Qiu <tian.shu.qiu@...el.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Robert Moore <robert.moore@...el.com>,
        Erik Kaneda <erik.kaneda@...el.com>,
        Petr Mladek <pmladek@...e.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        Jacopo Mondi <jacopo+renesas@...ndi.org>,
        kieran.bingham+renesas@...asonboard.com,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Marco Felsch <m.felsch@...gutronix.de>,
        niklas.soderlund+renesas@...natech.se,
        Steve Longerbeam <slongerbeam@...il.com>,
        "Krogerus, Heikki" <heikki.krogerus@...ux.intel.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Jordan Hand <jorhand@...ux.microsoft.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>
Subject: Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

Hi Sakari, thanks for comments

On 28/12/2020 17:05, Sakari Ailus wrote:
> Hi Andy, Daniel,
> 
> On Thu, Dec 24, 2020 at 02:54:44PM +0200, Andy Shevchenko wrote:
>>> +static void cio2_bridge_init_swnode_names(struct cio2_sensor *sensor)
>>> +{
>>> +       snprintf(sensor->node_names.remote_port, sizeof(sensor->node_names.remote_port),
>>> +                FWNODE_GRAPH_PORT_NAME_FORMAT, sensor->ssdb.link);
>>> +       snprintf(sensor->node_names.port, sizeof(sensor->node_names.port),
>>> +                FWNODE_GRAPH_PORT_NAME_FORMAT, 0); /* Always port 0 */
>>> +       snprintf(sensor->node_names.endpoint, sizeof(sensor->node_names.endpoint),
>>> +                FWNODE_GRAPH_ENDPOINT_NAME_FORMAT, 0); /* And endpoint 0 */
> 
> Please wrap before 80, there's no need here to do otherwise. You could
> argue about cio2_bridge_create_fwnode_properties() though. I might just
> wrap that, too.
> 
> Applies to the rest of the patch.

I shall wrap such cases then - I thought I read somewhere that the
wrapped line needed to be shorter than the parent which is why I wrapped
after 80...but I can't find the reference now so possibly I imagined that.

>>> +static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge,
>>> +                                      struct pci_dev *cio2)
>>> +{
>>> +       struct fwnode_handle *fwnode;
>>> +       struct cio2_sensor *sensor;
>>> +       struct acpi_device *adev;
>>> +       unsigned int i;
>>> +       int ret = 0;
>>
>> You may drop this assignment and...
>>
>>> +       for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
>>> +               const struct cio2_sensor_config *cfg = &cio2_supported_sensors[i];
> 
> You could move the inner loop into a new function called e.g.
> cio2_bridge_connect_sensor.

Yeah good idea, I'll do that.

>>> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.h b/drivers/media/pci/intel/ipu3/cio2-bridge.h
>>> new file mode 100644
>>> index 000000000000..004b608f322f
>>> --- /dev/null
>>> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
>>> @@ -0,0 +1,122 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/* Author: Dan Scally <djrscally@...il.com> */
>>> +#ifndef __CIO2_BRIDGE_H
>>> +#define __CIO2_BRIDGE_H
>>> +
>>> +#include <linux/property.h>
>>> +
>>> +#define CIO2_HID                               "INT343E"
>>> +#define CIO2_NUM_PORTS                         4
> 
> This is already defined in ipu3-cio2.h. Could you include that instead?

Yes; but I'd need to also include media/v4l2-device.h and
media/videobuf2-dma-sg.h (they're included in ipu3-cio2-main.c at the
moment). It didn't seem worth it; but I can move those two includes from
the .c to the .h and then include ipu3-cio2.h in cio2-bridge.h

Which do you prefer?

>>> +#define MAX_NUM_LINK_FREQS                     3
>>> +
>>> +#define CIO2_SENSOR_CONFIG(_HID, _NR, ...)     \
>>> +       {                                       \
>>> +               .hid = _HID,                    \
>>> +               .nr_link_freqs = _NR,           \
>>> +               .link_freqs = { __VA_ARGS__ }   \
>>> +       }
>>> +
>>> +#define NODE_SENSOR(_HID, _PROPS)              \
>>> +       ((const struct software_node) {         \
>>> +               .name = _HID,                   \
>>> +               .properties = _PROPS,           \
>>> +       })
>>> +
>>> +#define NODE_PORT(_PORT, _SENSOR_NODE)         \
>>> +       ((const struct software_node) {         \
>>> +               _PORT,                          \
>>> +               _SENSOR_NODE,                   \
> 
> Could you use explicit assignments to fields here, please?
> 
>>> +       })
>>> +
>>> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS)      \
>>> +       ((const struct software_node) {         \
>>> +               _EP,                            \
>>> +               _PORT,                          \
>>> +               _PROPS,                         \
> 
> Ditto.
> 

Will do