Re: [RFC PATCH v2] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows

[Dan Scally]

On 09/10/2020 13:44, Andy Shevchenko wrote:
>> ...I agree with your other email re. turning this into a series and
>> making the additional changes you suggested, so I'll do that too for the v3.
> I forgot to mention module rename as a separate patch. So, something like 6
> (or more) in a series I would expect.

Yeah no problem, I'll do that too.

>
>>>> +// SPDX-License-Identifier: GPL-2.0
>>> Author line perhaps?
>> You mean literally just like /* Authored By: Dan Scally */ or something?
>> OK, no problem.
> * Author: Dan Scally <foo@xxxxxxx>

Okedokey

>>>> +static const struct ipu3_sensor supported_devices[] = {
>>>> +	IPU3_SENSOR("INT33BE", "INT33BE:00"),
>>>> +	IPU3_SENSOR("OVTI2680", "OVTI2680:00"),
>>>> +	IPU3_SENSOR("OVTI5648", "OVTI5648:00")
>>> In such cases please leave comma at the last item as well. Easier to extend w/o
>>> an additional churn.
>>>
>>> On top of that, please avoid putting *instance* names, i.e. the second
>>> parameters in your macro call. What code should do is to take _HID (first
>>> parameter) and call acpi_dev_match_first_dev() or so.
>> Yeah I was originally using the i2c_client's name field (this comes into
>> play during cio2_bridge_reprobe_sensor()) but the matching refused to
>> work using anything but a string literal. Let me take another look at
>> this then.
> I meant that you get an instance name from the first found device, like
>
> 	char instance_name[I2C...];
>
> 	adev = first_match_dev();
> 	if (adev) {
> 		snprintf(instance_name, ..., acpi_dev_name(adev));
> 		...
> 	} else {
> 		...
> 	}
Ah, yes, this is better than messing around getting the i2c_client - thanks
>> I thought you were hinting that I should use kcalloc in the comments
>> from the last patch to get an array of zero valued entries but I guess I
>> misunderstood - I can just memset these arrays to 0 and assign all but
>> the last entry directly and that seems to work fine, so I'll switch to that.
> The idea behind that any kcalloc() or kzalloc() against a container (whatever
> data structure that has those property arrays) will do it for you. So, I think
> neither kcalloc() nor stack is needed. Instantiate properties directly in the
> arrays of sensor data structure.
Will do
>>>> +	endpoint = fwnode_graph_get_next_endpoint(pci_dev->dev.fwnode, NULL);
>>> dev_fwnode()
>> I avoided that thinking there might be a case where a CIO2 device has a
>> fwnode but not endpoints defined for some reason, but I'm not familiar
>> enough to judge whether that situation will ever occur - if it's safe to
>> do it that way then I'll switch it over.
> I meant
> 	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&pci_dev->dev), NULL);
Ah! Of course, I'll make that change too, thanks