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

[Dan Scally <djrscally@gmail.com>]

Hi Andy - sorry for delayed reply, busy week

On 01/10/2020 18:37, Andy Shevchenko wrote:
> On Thu, Oct 01, 2020 at 10:33:26AM +0100, Daniel Scally wrote:
>
> Awesome work!
> My, almost minor, comments below.
Thanks as always for your help - great comments. I'll work through and
make the changes you suggest to this code and also...
>> * Built against media_tree instead of linus's tree - there's no T: entry in
>> maintainers for the ipu3-cio2 driver but I see there're recent changes in 
>> media_tree so thought this was the better option.
> Make sense to include T: entry as well (maybe as a separate patch).

...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.


Just a couple comments / queries:

>> +// SPDX-License-Identifier: GPL-2.0
> Author line perhaps?

You mean literally just like /* Authored By: Dan Scally */ or something?
OK, no problem.

>> +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.
>> +static struct software_node cio2_hid_node = { CIO2_HID, };
> Here, nevertheless, comma can be removed, since update will need to change
> entire line anyway.
Trailing commas where lists can be extended, otherwise none - got it.
>> +static int read_acpi_block(struct device *dev, char *id, void *data, u32 size)
>> +{
>> +	union acpi_object *obj;
>> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>> +	struct acpi_handle *handle = ACPI_HANDLE(dev);
>> +	acpi_status status;
>> +	int ret;
>> +
>> +	status = acpi_evaluate_object(handle, id, NULL, &buffer);
>> +	if (ACPI_FAILURE(status))
>> +		return -ENODEV;
>> +
>> +	obj = buffer.pointer;
>> +	if (!obj || obj->type != ACPI_TYPE_BUFFER) {
> !obj case doesn't require freeing and I would say the error message can be
> amended in such case.
Oops - you suggested that in the last version too and I missed it, sorry
about that. I'll split those two failure modes out.
>> +		for (j = 4; j >= 0; j--)
>> +			software_node_unregister(&sensor->swnodes[j]);
> Seems we may need a simple helper for this (test_printf.c has similar case),
> so, what about do it here for now and probably then move to somewhere like
> swnode.h or whatever holds it.
>
> static inline software_node_unregister_nodes_reverse(const struct software_node *nodes)
> {
> 	unsigned int i = 0;
>
> 	while (nodes[i].name)
> 		i++;
> 	while (i--)
> 		software_node_unregister(&nodes[i]);
> }
Yeah this is a good idea; I see you suggest a new patch for it in your
other email; I'll do a series in the future and add this to the swnode
source file at the same time as doing the changes that you, Heikki and
Sakari suggested for the other patch.
>> +#define __NO_VERSION__
> What is this for?

Somehow I got on the outdated page for compiling modules spanning
multiple files in the linux kernel module programming guide; it's
suggested there but not in the newer version. I shall remove it.

>> +	struct device *dev;
>> +	struct software_node swnodes[6];
>> +	struct property_entry dev_props[3];
>> +	struct property_entry ep_props[4];
>> +	struct property_entry cio2_props[3];
> I'm now wondering why you can't simply put properties directly to here and do
> that kcalloc / memcpy() in few functions? I mean to drop those calls and assign
> properties directly. You even won't need to memset() and stack for them!
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.
>>  	struct cio2_device *cio2;
>> +	struct fwnode_handle *endpoint;
>>  	int r;
> Reverse xmas tree order.
TIL - thanks, I'll do a pass over the rest of it and make sure that I
follow that everywhere else too
>> +	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.