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
