Hi Andy
On 04/01/2021 12:09, Andy Shevchenko wrote:
> On Sun, Jan 03, 2021 at 11:12:35PM +0000, Daniel Scally wrote:
>> Currently on platforms designed for Windows, connections between CIO2 and
>> sensors are not properly defined in DSDT. This patch extends the ipu3-cio2
>> driver to compensate by building software_node connections, parsing the
>> connection properties from the sensor's SSDB buffer.
> Few nitpicks below (I consider it's good enough as is, though).
Thanks!
>> +static int cio2_bridge_connect_sensor(const struct cio2_sensor_config *cfg,
>> + struct cio2_bridge *bridge,
>> + struct pci_dev *cio2)
>> +{
>> + struct fwnode_handle *fwnode;
>> + struct cio2_sensor *sensor;
>> + struct acpi_device *adev;
>> + int ret;
>> + for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> (1)
>
>> + if (bridge->n_sensors >= CIO2_NUM_PORTS) {
>> + dev_err(&cio2->dev, "Exceeded available CIO2 ports\n");
>> + cio2_bridge_unregister_sensors(bridge);
>> + ret = -EINVAL;
>> + goto err_out;
>> + }
>> + if (!adev->status.enabled)
>> + continue;
> A nit: this would be better to be at (1) location.
Yep, agreed
>
> Then possible to factor out the rest of the body of this loop as well.
>
> (Also can be considered as a hint for the future improvement)
Yeah I can look at this, there will probably be some future changes
anyway as we discover more details about the data in the SSDB buffer and
so on
> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.h
> b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> new file mode 100644
> index 000000000000..3ec4ed44aced
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Author: Dan Scally <djrsca...@gmail.com> */
> +#ifndef __CIO2_BRIDGE_H
> +#define __CIO2_BRIDGE_H
> +
> +#include <linux/property.h>
> +#include <linux/types.h>
> +
> +#include "ipu3-cio2.h"
> +
> +#define CIO2_HID "INT343E"
> +#define CIO2_MAX_LANES 4
> +#define MAX_NUM_LINK_FREQS 3
> +
> +#define CIO2_SENSOR_CONFIG(_HID, _NR, ...) \
> + { \
> + .hid = _HID, \
> + .nr_link_freqs = _NR, \
> + .link_freqs = { __VA_ARGS__ } \
> + }
> Perhaps also good to declare it as a compound literal.
>
> (Means to add (struct ...) to the initializer.
>
Yep ok
>> +#define NODE_SENSOR(_HID, _PROPS) \
>> + ((const struct software_node) { \
>> + .name = _HID, \
>> + .properties = _PROPS, \
>> + })
>> +
>> +#define NODE_PORT(_PORT, _SENSOR_NODE) \
>> + ((const struct software_node) { \
>> + .name = _PORT, \
>> + .parent = _SENSOR_NODE, \
>> + })
>> +
>> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS) \
>> + ((const struct software_node) { \
>> + .name = _EP, \
>> + .parent = _PORT, \
>> + .properties = _PROPS, \
>> + })
> In all three I didn't get why you need outer parentheses. Without them it will
> be well defined compound literal and should work as is.
The code works fine, but checkpatch complains that macros with complex
values should be enclosed in parentheses. I guess now that I'm more
familiar with the code I'd call that a false-positive though, as nowhere
else in the kernel that I've seen encloses them the same way.
>> static int cio2_pci_probe(struct pci_dev *pci_dev,
>> const struct pci_device_id *id)
>> {
>> + struct fwnode_handle *fwnode = dev_fwnode(&pci_dev->dev);
>> struct cio2_device *cio2;
>> int r;
>>
>> @@ -1715,6 +1732,22 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>> return -ENOMEM;
>> cio2->pci_dev = pci_dev;
>>
>> + /*
>> + * On some platforms no connections to sensors are defined in firmware,
>> + * if the device has no endpoints then we can try to build those as
>> + * software_nodes parsed from SSDB.
>> + */
>> + if (!cio2_check_fwnode_graph(fwnode)) {
> A nit:
> I prefer form of
>
> r = cio2_check_fwnode_graph(fwnode);
> if (!r) { // alternatively if (r == 0), depends on maintainer's taste
This is fine by me; I can switch to that
>
>> + if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) {
>> + dev_err(&pci_dev->dev, "fwnode graph has no endpoints
>> connected\n");
>> + return -EINVAL;
>> + }
>> +
>> + r = cio2_bridge_init(pci_dev);
>> + if (r)
>> + return r;
>> + }
>> +
>> r = pcim_enable_device(pci_dev);
>> if (r) {
>> dev_err(&pci_dev->dev, "failed to enable device (%d)\n", r);
>> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
>> b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
>> index 62187ab5ae43..dc3e343a37fb 100644
>> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
>> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
>> @@ -455,4 +455,10 @@ static inline struct cio2_queue
>> *vb2q_to_cio2_queue(struct vb2_queue *vq)
>> return container_of(vq, struct cio2_queue, vbq);
>> }
>>
>> +#if IS_ENABLED(CONFIG_CIO2_BRIDGE)
>> +int cio2_bridge_init(struct pci_dev *cio2);
>> +#else
>> +int cio2_bridge_init(struct pci_dev *cio2) { return 0; }
> static inline?
Ah, yes - thanks. Hadn't read about inline until now
