Re: [PATCH 13/18] ipu3-cio2: Add functionality allowing software_node connections to sensors on platforms designed for Windows

[Dan Scally]

Hi Laurent
On 01/12/2020 22:30, Laurent Pinchart wrote:
>>>> +}
>>>> +
>>>> +static void cio2_bridge_create_fwnode_properties(struct cio2_sensor *sensor)
>>>> +{
>>>> +	unsigned int i;
>>>> +
>>>> +	cio2_bridge_init_property_names(sensor);
>>>> +
>>>> +	for (i = 0; i < 4; i++)
>>>> +		sensor->data_lanes[i] = i + 1;
>>> Is there no provision in the SSDB for data lane remapping ?
>> Sorry; don't follow what you mean by data lane remapping here.
> Some CSI-2 receivers can remap data lanes. The routing inside the SoC
> from the data lane input pins to the PHYs is configurable. This makes
> board design easier as you can route the data lanes to any of the
> inputs. That's why the data lanes DT property is a list of lane numbers
> instead of a number of lanes. I'm actually not sure if the CIO2 supports
> this.

I don't see anything in the SSDB that might refer to that, though of
course we're lacking documentation for it so it could be a part that we
don't understand yet.


>>>> +			dev_info(&bridge->cio2->dev,
>>>> +				 "Found supported sensor %s\n",
>>>> +				 acpi_dev_name(adev));
>>>> +
>>>> +			bridge->n_sensors++;
>>> We probably want a check here to avoid overflowing bridge->sensors. The
>>> other option is to make bridge->sensors a struct list_head and allocate
>>> sensors dynamically.
>> Err - agree on a check. There's only 4 ports in a CIO2 device, so that's
>> the maximum. Seems easier to just do a check, unless the wasted memory
>> is enough that it's worth allocating dynamically. I don't mind either
>> approach.
> In theory we could route multiple sensors to the same receiver, as long
> as only one of them drives the lanes at any given time. It's one way to
> support multiple sensors in cheap designs. I doubt we'll ever encounter
> that with the IPU3, so we could just limit the count to 4.
Ah, that's neat though. But I'll leave it at a check at the top of the
loop for now.
>>>> +
>>>> +	fwnode = software_node_fwnode(&bridge->cio2_hid_node);
>>>> +	if (!fwnode) {
>>>> +		dev_err(dev, "Error getting fwnode from cio2 software_node\n");
>>>> +		ret = -ENODEV;
>>>> +		goto err_unregister_sensors;
>>> Can this happen ?
>> It _shouldn't_ happen, as long as nothing else is touching the swnodes
>> I've registered or anything. I've never seen it happen. That didn't feel
>> like quite enough to say it can't ever happen - but I'm happy to skip
>> the check if you think thats ok.
> It seems a bit overkill to me, but I'm not a swnode specialist :-)
I'm going to keep it, if you have no strong feelings, partly through
caution but also because the other place swnodes are most heavily used
(drivers/platform/x86/intel_cht_int33fe_typec.c) _does_ perform the
check, so consistency too.
>>>> @@ -0,0 +1,108 @@
>>>> +/* 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
>>> There are a few rogue spaces before '4'.
>> Argh, thanks, this is the curse of using VS code on multiple machines...
> I recommend vim ;-)
You're not the only one - maybe I need to spend the time and it'll save
time in the future