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

[Andy Shevchenko]

On Thu, Oct 08, 2020 at 10:51:46PM +0100, Dan Scally wrote:
> 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.

I forgot to mention module rename as a separate patch. So, something like 6
(or more) in a series I would expect.

...

> >> +// 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>

...

> >> +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 {
		...
	}

...

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

Just use a common sense. Simple extrapolate it to the next change, if any,
among the same lines.

...

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

Yes, thanks!

...

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

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.

...

> >> +	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);


-- 
With Best Regards,
Andy Shevchenko