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

[Daniel Scally]

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@xxxxxxxxx> */
>>> +#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