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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 9 Oct 2020 16:02:28 +0100
From:   Dan Scally <djrscally@...il.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     yong.zhi@...el.com, sakari.ailus@...ux.intel.com,
        bingbu.cao@...el.com, tian.shu.qiu@...el.com, mchehab@...nel.org,
        davem@...emloft.net, robh@...nel.org, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        jorhand@...ux.microsoft.com, kitakar@...il.com,
        kieran.bingham@...asonboard.com, dan.carpenter@...cle.com
Subject: Re: [RFC PATCH v2] Add functionality to ipu3-cio2 driver allowing
 software_node connections to sensors on platforms designed for Windows

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

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