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
