From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Dan Scally <djrscally@gmail.com>
Cc: linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
linus.walleij@linaro.org,
prabhakar.mahadev-lad.rj@bp.renesas.com,
heikki.krogerus@linux.intel.com, dmitry.torokhov@gmail.com,
laurent.pinchart+renesas@ideasonboard.com,
kieran.bingham+renesas@ideasonboard.com,
jacopo+renesas@jmondi.org, robh@kernel.org, davem@davemloft.net,
linux@rasmusvillemoes.dk, andriy.shevchenko@linux.intel.com,
sergey.senozhatsky@gmail.com, rostedt@goodmis.org,
pmladek@suse.com, mchehab@kernel.org, tian.shu.qiu@intel.com,
bingbu.cao@intel.com, sakari.ailus@linux.intel.com,
yong.zhi@intel.com, rafael@kernel.org,
gregkh@linuxfoundation.org, kitakar@gmail.com
Subject: Re: [RFC PATCH v3 9/9] ipu3-cio2: Add functionality allowing software_node connections to sensors on platforms designed for Windows
Date: Sat, 24 Oct 2020 12:37:02 +0300 [thread overview]
Message-ID: <20201024093702.GA3939@pendragon.ideasonboard.com> (raw)
In-Reply-To: <d188f8b5-ed3b-f91b-171a-26afeb7d213e@gmail.com>
Hi Daniel,
On Sat, Oct 24, 2020 at 09:50:07AM +0100, Dan Scally wrote:
> On 24/10/2020 02:24, Laurent Pinchart wrote:
>
> > On Mon, Oct 19, 2020 at 11:59:03PM +0100, 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.
> >>
> >> Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>
> >> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> >> ---
> >> Changes in v3:
> >> - Rather than overwriting the device's primary fwnode, we now
> >> simply assign a secondary. Some of the preceding patches alter the
> >> existing driver code and v4l2 framework to allow for that.
> >> - Rather than reprobe() the sensor after connecting the devices in
> >> cio2-bridge we create the software_nodes right away. In this case,
> >> sensor drivers will have to defer probing until they detect that a
> >> fwnode graph is connecting them to the CIO2 device.
> >> - Error paths in connect_supported_devices() moved outside the
> >> loop
> >> - Replaced pr_*() with dev_*() throughout
> >> - Moved creation of software_node / property_entry arrays to stack
> >> - A lot of formatting changes.
> >>
> >> MAINTAINERS | 1 +
> >> drivers/media/pci/intel/ipu3/Kconfig | 18 +
> >> drivers/media/pci/intel/ipu3/Makefile | 3 +-
> >> drivers/media/pci/intel/ipu3/cio2-bridge.c | 327 ++++++++++++++++++
> >> drivers/media/pci/intel/ipu3/cio2-bridge.h | 94 +++++
> >> drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 21 ++
> >> drivers/media/pci/intel/ipu3/ipu3-cio2.h | 9 +
> >> 7 files changed, 472 insertions(+), 1 deletion(-)
> >> create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c
> >> create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 5d768d5ad..4c9c646c7 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -8848,6 +8848,7 @@ INTEL IPU3 CSI-2 CIO2 DRIVER
> >> M: Yong Zhi <yong.zhi@intel.com>
> >> M: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> M: Bingbu Cao <bingbu.cao@intel.com>
> >> +M: Dan Scally <djrscally@gmail.com>
> >> R: Tianshu Qiu <tian.shu.qiu@intel.com>
> >> L: linux-media@vger.kernel.org
> >> S: Maintained
> >> diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig
> >> index 82d7f17e6..d14cbceae 100644
> >> --- a/drivers/media/pci/intel/ipu3/Kconfig
> >> +++ b/drivers/media/pci/intel/ipu3/Kconfig
> >> @@ -16,3 +16,21 @@ config VIDEO_IPU3_CIO2
> >> Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
> >> connected camera.
> >> The module will be called ipu3-cio2.
> >> +
> >> +config CIO2_BRIDGE
> >> + bool "IPU3 CIO2 Sensors Bridge"
> >> + depends on VIDEO_IPU3_CIO2
> >> + help
> >> + This extension provides an API for the ipu3-cio2 driver to create
> >> + connections to cameras that are hidden in SSDB buffer in ACPI. It
> >> + can be used to enable support for cameras in detachable / hybrid
> >> + devices that ship with Windows.
> >> +
> >> + Say Y here if your device is a detachable / hybrid laptop that comes
> >> + with Windows installed by the OEM, for example:
> >> +
> >> + - Some Microsoft Surface models
> >> + - The Lenovo Miix line
> >> + - Dell 7285
> >> +
> >> + If in doubt, say N here.
> >> diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile
> >> index b4e3266d9..933777e6e 100644
> >> --- a/drivers/media/pci/intel/ipu3/Makefile
> >> +++ b/drivers/media/pci/intel/ipu3/Makefile
> >> @@ -1,4 +1,5 @@
> >> # SPDX-License-Identifier: GPL-2.0-only
> >> obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
> >>
> >> -ipu3-cio2-y += ipu3-cio2-main.o
> >> \ No newline at end of file
> >> +ipu3-cio2-y += ipu3-cio2-main.o
> >> +ipu3-cio2-$(CONFIG_CIO2_BRIDGE) += cio2-bridge.o
> >> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> >> new file mode 100644
> >> index 000000000..bbe072f04
> >> --- /dev/null
> >> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> >> @@ -0,0 +1,327 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +// Author: Dan Scally <djrscally@gmail.com>
> >> +#include <linux/acpi.h>
> >> +#include <linux/device.h>
> >> +#include <linux/fwnode.h>
> >> +#include <linux/i2c.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/pci.h>
> >> +#include <linux/property.h>
> >> +#include <media/v4l2-subdev.h>
> >> +
> >> +#include "cio2-bridge.h"
> >> +
> >> +/*
> >> + * Extend this array with ACPI Hardware ID's of devices known to be
> >> + * working
> >> + */
> >> +static const char * const supported_devices[] = {
> >> + "INT33BE",
> >> + "OVTI2680",
> >> +};
> >> +
> >> +static struct software_node cio2_hid_node = { CIO2_HID };
> >> +
> >> +static struct cio2_bridge bridge;
> >
> > While there shouldn't be more than one CIO2 instance, we try to develop
> > drivers in a way that avoids global per-device variables. Could all this
> > be allocated dynamically, with the pointer returned by
> > cio2_bridge_build() and stored in the cio2_device structure ?
>
> Yes, ok, I'll make that change.
>
> >> +
> >> +static const char * const port_names[] = {
> >> + "port0", "port1", "port2", "port3"
> >> +};
> >> +
> >> +static const struct property_entry remote_endpoints[] = {
> >> + PROPERTY_ENTRY_REF("remote-endpoint", /* Sensor 0, Sensor Property */
> >> + &bridge.sensors[0].swnodes[SWNODE_CIO2_ENDPOINT]),
> >> + PROPERTY_ENTRY_REF("remote-endpoint", /* Sensor 0, CIO2 Property */
> >> + &bridge.sensors[0].swnodes[SWNODE_SENSOR_ENDPOINT]),
> >> + PROPERTY_ENTRY_REF("remote-endpoint",
> >> + &bridge.sensors[1].swnodes[SWNODE_CIO2_ENDPOINT]),
> >> + PROPERTY_ENTRY_REF("remote-endpoint",
> >> + &bridge.sensors[1].swnodes[SWNODE_SENSOR_ENDPOINT]),
> >> + PROPERTY_ENTRY_REF("remote-endpoint",
> >> + &bridge.sensors[2].swnodes[SWNODE_CIO2_ENDPOINT]),
> >> + PROPERTY_ENTRY_REF("remote-endpoint",
> >> + &bridge.sensors[2].swnodes[SWNODE_SENSOR_ENDPOINT]),
> >> + PROPERTY_ENTRY_REF("remote-endpoint",
> >> + &bridge.sensors[3].swnodes[SWNODE_CIO2_ENDPOINT]),
> >> + PROPERTY_ENTRY_REF("remote-endpoint",
> >> + &bridge.sensors[3].swnodes[SWNODE_SENSOR_ENDPOINT]),
> >> +};
> >
> > For the same reason, I would move this to the sensor structure (with two
> > property_entry per sensor). That will simplify the code below, avoiding
> > indexing this array with bridge.n_sensors * 2.
>
> I had some trouble with that which is why I ended up doing things this
> way; I'll revisit it and see if I can resolve that.
>
> >> +
> >> +static int read_acpi_block(struct device *dev, char *id, void *data, u32 size)
> >
> > To avoid potential future namespace classes, I'd advise naming the
> > functions with a cio2_bridge_ prefix, even the static ones.
> >
> > And maybe cio2_bridge_read_acpi_buffer(), as this function reads a
> > buffer ?
>
> Ack to both; and to the similar comments re: variable naming below.
>
> >> +{
> >> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> >> + struct acpi_handle *handle;
> >> + union acpi_object *obj;
> >> + acpi_status status;
> >> + int ret;
> >> +
> >> + handle = ACPI_HANDLE(dev);
> >> +
> >> + status = acpi_evaluate_object(handle, id, NULL, &buffer);
> >> + if (ACPI_FAILURE(status))
> >> + return -ENODEV;
> >> +
> >> + obj = buffer.pointer;
> >> + if (!obj) {
> >> + dev_err(dev, "Couldn't locate ACPI buffer\n");
> >> + return -ENODEV;
> >> + }
> >> +
> >> + if (obj->type != ACPI_TYPE_BUFFER) {
> >> + dev_err(dev, "Couldn't read ACPI buffer\n");
> >> + ret = -ENODEV;
> >> + goto out_free_buff;
> >> + }
> >> +
> >> + if (obj->buffer.length > size) {
> >> + dev_err(dev, "Given buffer is too small\n");
> >> + ret = -ENODEV;
> >> + goto out_free_buff;
> >> + }
> >> +
> >> + memcpy(data, obj->buffer.pointer, obj->buffer.length);
> >> + ret = obj->buffer.length;
> >> +
> >> +out_free_buff:
> >> + kfree(buffer.pointer);
> >> + return ret;
> >> +}
> >> +
> >> +static int get_acpi_ssdb_sensor_data(struct device *dev,
> >> + struct sensor_bios_data *sensor)
> >> +{
> >> + struct sensor_bios_data_packed sensor_data;
> >> + int ret;
> >> +
> >> + ret = read_acpi_block(dev, "SSDB", &sensor_data, sizeof(sensor_data));
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + sensor->link = sensor_data.link;
> >> + sensor->lanes = sensor_data.lanes;
> >> + sensor->mclkspeed = sensor_data.mclkspeed;
> >> + sensor->degree = sensor_data.degree;
> >
> > How about storing a sensor_bios_data_packed inside sensor_bios_data ?
> > That will avoid copying fields individually, with manual addition of
> > extra fields as they become useful. Usage of the sensor_bios_data
> > structure would turn from sensor->degree to sensor->ssdb.degree, which
> > is slightly longer, but I think more maintainable.
>
> Inside the struct sensor you mean (confusingly, the variable named
> sensor here is _not_ of type struct sensor, which I acknowledge is plain
> silly)? If so, agreed, I'll change it to that. That's also consistent
> with what I'm doing with the equivalent struct for the PMIC's CLDB in
> the regulator work so that makes sense anyway.
Yes. Looking at it again, the sensor_bios_data structure can likely be
dropped, as it bundles a struct device pointer, which is unused, with
fields copied from sensor_bios_data_packed, which should be stored in
the sensor struct.
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int create_fwnode_properties(struct sensor *sensor,
> >> + struct sensor_bios_data *ssdb)
> >> +{
> >> + struct property_entry *cio2_properties = sensor->cio2_properties;
> >> + struct property_entry *dev_properties = sensor->dev_properties;
> >> + struct property_entry *ep_properties = sensor->ep_properties;
> >> + int i;
> >
> > i never takes negative values, you can make it an unsigned int. Same for
> > other occurrences below.
> >
> >> +
> >> + /* device fwnode properties */
> >> + memset(dev_properties, 0, sizeof(struct property_entry) * 3);
> >
> > I would memset() bridge to 0 in one go and avoid individual memsets. And
> > if you allocate it with kzalloc() it will be initialized to 0.
>
> Yep ok, I'll initialize the whole thing with kzalloc at the start then
>
> >> +
> >> + dev_properties[0] = PROPERTY_ENTRY_U32("clock-frequency",
> >> + ssdb->mclkspeed);
> >> + dev_properties[1] = PROPERTY_ENTRY_U8("rotation", ssdb->degree);
> >> +
> >> + /* endpoint fwnode properties */
> >> + memset(ep_properties, 0, sizeof(struct property_entry) * 4);
> >> +
> >> + sensor->data_lanes = kmalloc_array(ssdb->lanes, sizeof(u32),
> >> + GFP_KERNEL);
> >
> > Given that there can't be more than 4 data lanes, how about turning
> > data_lanes into an array with 4 entries, to avoid the dynamic allocation
> > ? You will have to validate ssdb->lanes in connect_supported_devices(),
> > to make sure not to overflow the array. This and the next function can
> > then be turned into void functions.
>
> OK - is that generally better then (I.E. avoiding dynamic allocation),
> or just when the "wasted" memory is so small?
Just when the amount of wasted memory is small.
> >> +
> >> + if (!sensor->data_lanes)
> >> + return -ENOMEM;
> >> +
> >> + for (i = 0; i < ssdb->lanes; i++)
> >> + sensor->data_lanes[i] = i + 1;
> >> +
> >> + ep_properties[0] = PROPERTY_ENTRY_U32("bus-type", 5);
> >> + ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes",
> >> + sensor->data_lanes,
> >> + ssdb->lanes);
> >> + ep_properties[2] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_SENSOR];
> >> +
> >> + /* cio2 endpoint props */
> >> + memset(cio2_properties, 0, sizeof(struct property_entry) * 3);
> >> +
> >> + cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes",
> >> + sensor->data_lanes,
> >> + ssdb->lanes);
> >> + cio2_properties[1] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_CIO2];
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int create_connection_swnodes(struct sensor *sensor,
> >> + struct sensor_bios_data *ssdb)
> >> +{
> >> + struct software_node *nodes = sensor->swnodes;
> >> +
> >> + memset(nodes, 0, sizeof(struct software_node) * 6);
> >> +
> >> + nodes[SWNODE_SENSOR_HID] = NODE_SENSOR(sensor->name,
> >> + sensor->dev_properties);
> >> + nodes[SWNODE_SENSOR_PORT] = NODE_PORT("port0",
> >> + &nodes[SWNODE_SENSOR_HID]);
> >> + nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT("endpoint0",
> >> + &nodes[SWNODE_SENSOR_PORT],
> >> + sensor->ep_properties);
> >> + nodes[SWNODE_CIO2_PORT] = NODE_PORT(port_names[ssdb->link],
> >> + &cio2_hid_node);
> >> + nodes[SWNODE_CIO2_ENDPOINT] = NODE_ENDPOINT("endpoint0",
> >> + &nodes[SWNODE_CIO2_PORT],
> >> + sensor->cio2_properties);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void cio2_bridge_unregister_sensors(void)
> >> +{
> >> + struct sensor *sensor;
> >> + int i;
> >> +
> >> + for (i = 0; i < bridge.n_sensors; i++) {
> >> + sensor = &bridge.sensors[i];
> >> +
> >> + software_node_unregister_nodes_reverse(sensor->swnodes);
> >> +
> >> + kfree(sensor->data_lanes);
> >> +
> >> + put_device(sensor->dev);
> >> + acpi_dev_put(sensor->adev);
> >> + }
> >> +}
> >> +
> >> +static int connect_supported_devices(struct pci_dev *cio2)
> >> +{
> >> + struct sensor_bios_data ssdb;
> >> + struct fwnode_handle *fwnode;
> >> + struct acpi_device *adev;
> >> + struct sensor *sensor;
> >> + struct device *dev;
> >> + int i, ret;
> >> +
> >> + ret = 0;
> >
> > You can initialize ret to 0 when declaring the variable.
>
> Is that ok on the same like as i's declaration, or should I split them?
I usually prefer splitting them, but that's a matter of personal taste I
suppose. This being said, as i should be an unsigned int, they will have
to be split anyway.
> >> + for (i = 0; i < ARRAY_SIZE(supported_devices); i++) {
> >> + adev = acpi_dev_get_first_match_dev(supported_devices[i], NULL, -1);
> >
> > What if there are multiple sensor of the same model ?
>
> Hmm, yeah, that would be a bit of a pickle. I guess the newer
> smartphones have multiple sensors on the back, which I presume are the
> same model. So that will probably crop up at some point. How about
> instead I use bus_for_each_dev() and in the applied function check if
> the _HID is in the supported list?
Sounds good to me.
> >> + if (!adev)
> >> + continue;
> >> +
> >
> > Does acpi_dev_get_first_match_dev() skip disabled devices (as reported
> > by _STA) ?
>
> Yes.
>
> >> + dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);
> >> + if (!dev) {
> >> + ret = -EPROBE_DEFER;
> >> + goto err_rollback;
> >> + }
> >> +
> >> + sensor = &bridge.sensors[bridge.n_sensors];
> >> + sensor->dev = dev;
> >> + sensor->adev = adev;
> >> +
> >> + snprintf(sensor->name, ACPI_ID_LEN, "%s",
> >> + supported_devices[i]);
> >
> > How about strlcpy() ?
>
> Sure
>
> >> +
> >> + ret = get_acpi_ssdb_sensor_data(dev, &ssdb);
> >> + if (ret)
> >> + goto err_free_dev;
> >> +
> >> + ret = create_fwnode_properties(sensor, &ssdb);
> >> + if (ret)
> >> + goto err_free_dev;
> >> +
> >> + ret = create_connection_swnodes(sensor, &ssdb);
> >> + if (ret)
> >> + goto err_free_dev;
> >> +
> >> + ret = software_node_register_nodes(sensor->swnodes);
> >> + if (ret)
> >> + goto err_free_dev;
> >> +
> >> + fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_SENSOR_HID]);
> >> + if (!fwnode) {
> >> + ret = -ENODEV;
> >> + goto err_free_swnodes;
> >> + }
> >> +
> >> + set_secondary_fwnode(dev, fwnode);
> >
> > I wonder if we could avoid depending on the I2C device being created by
> > getting the fwnode from adev, and setting ->secondary manually. adev
> > would need to be passed to get_acpi_ssdb_sensor_data() instead of dev.
>
> Let me try that; I initially wanted to do
> set_secondary_fwnode(&adev->dev, fwnode) to avoid depending on the I2C
> dev being created but it turns out &adev->dev isn't the same pointer. I
> shall try it and see.
>
> >> +
> >> + dev_info(&cio2->dev, "Found supported device %s\n",
> >> + supported_devices[i]);
> >> +
> >> + bridge.n_sensors++;
> >> + continue;
> >> + }
> >> +
> >> + return ret;
> >> +
> >> +err_free_swnodes:
> >> + software_node_unregister_nodes_reverse(sensor->swnodes);
> >> +err_free_dev:
> >> + put_device(dev);
> >> +err_rollback:
> >> + acpi_dev_put(adev);
> >
> > I think you'll leak sensor->data_lanes here. It won't be a problem if
> > you don't allocate it manually, as proposed above. I however wonder if
> > error handling couldn't be simplified by increasing bridge.n_sensors
> > earlier, and skipping cleanup in cio2_bridge_unregister_sensors() for
> > the fields that haven't been initialized (for instance kfree() is a
> > no-op on NULL pointers, so that's already handled).
>
> Errrr the only sticky bit there is the desire _not_ to unwind all the
> sensors if it managed to successfully connect one of them; if I'm just
> calling cio2_bridge_unregister_sensors() on any error then a failure for
> one sensor will result in no working cameras, where this way at least
> one of them might be available.
Good point. I expect this to be reworked anyway if we can stop depending
on the I2C device being created, there should be no need to defer
probing in that case.
> >> +
> >> + /*
> >> + * If an iteration of this loop results in -EPROBE_DEFER then
> >> + * we need to roll back any sensors that were successfully
> >> + * registered. Any other error and we'll skip that step, as
> >> + * it seems better to have one successfully connected sensor.
> >> + */
> >> +
> >> + if (ret == -EPROBE_DEFER)
> >> + cio2_bridge_unregister_sensors();
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +int cio2_bridge_build(struct pci_dev *cio2)
> >> +{
> >> + struct fwnode_handle *fwnode;
> >> + int ret;
> >> +
> >> + pci_dev_get(cio2);
> >> +
> >> + ret = software_node_register(&cio2_hid_node);
> >> + if (ret < 0) {
> >> + dev_err(&cio2->dev, "Failed to register the CIO2 HID node\n");
> >> + goto err_put_cio2;
> >> + }
> >> +
> >> + ret = connect_supported_devices(cio2);
> >> + if (ret == -EPROBE_DEFER)
> >> + goto err_unregister_cio2;
> >> +
> >> + if (bridge.n_sensors == 0) {
> >> + ret = -EPROBE_DEFER;
> >> + goto err_unregister_cio2;
> >> + }
> >> +
> >> + dev_info(&cio2->dev, "Connected %d cameras\n", bridge.n_sensors);
> >> +
> >> + fwnode = software_node_fwnode(&cio2_hid_node);
> >> + if (!fwnode) {
> >> + dev_err(&cio2->dev,
> >> + "Error getting fwnode from cio2 software_node\n");
> >> + ret = -ENODEV;
> >> + goto err_unregister_sensors;
> >> + }
> >> +
> >> + set_secondary_fwnode(&cio2->dev, fwnode);
> >> +
> >> + return 0;
> >> +
> >> +err_unregister_sensors:
> >> + cio2_bridge_unregister_sensors();
> >> +err_unregister_cio2:
> >> + software_node_unregister(&cio2_hid_node);
> >> +err_put_cio2:
> >> + pci_dev_put(cio2);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +void cio2_bridge_burn(struct pci_dev *cio2)
> >
> > Interesting function name :-) I like the creativity, but I think
> > consistency with the rest of the kernel code should unfortunately be
> > favoured.
>
> Heh yep - already changed to _init/_clean per Andy's comments. Couldn't
> resist the pun!
>
> >> + struct sensor sensors[MAX_CONNECTED_DEVICES];
> >> +};
> >> +
> >> +/* Data representation as it is in ACPI SSDB buffer */
> >> +struct sensor_bios_data_packed {
> >
> > Similarly as above, I'd use a cio2_ prefix, and I think you can drop the
> > _packed suffix. How about naming it cio2_sensor_ssdb_data (or even just
> > cio2_sensor_ssdb) to make it clearer that it contains the SSDB data ?
>
> Already done (well, sensor_ssdb currently) to keep consistent with
> struct pmic_cldb that was introduced. I'll add the cio2 prefix.
>
> >> + u8 version;
> >> + u8 sku;
> >> + u8 guid_csi2[16];
> >> + u8 devfunction;
> >> + u8 bus;
> >> + u32 dphylinkenfuses;
> >> + u32 clockdiv;
> >> + u8 link;
> >> + u8 lanes;
> >> + u32 csiparams[10];
> >> + u32 maxlanespeed;
> >> + u8 sensorcalibfileidx;
> >> + u8 sensorcalibfileidxInMBZ[3];
> >> + u8 romtype;
> >> + u8 vcmtype;
> >> + u8 platforminfo;
> >> + u8 platformsubinfo;
> >> + u8 flash;
> >> + u8 privacyled;
> >> + u8 degree;
> >> + u8 mipilinkdefined;
> >> + u32 mclkspeed;
> >> + u8 controllogicid;
> >> + u8 reserved1[3];
> >> + u8 mclkport;
> >> + u8 reserved2[13];
> >> +} __packed__;
> >> +
> >> +/* Fields needed by bridge driver */
> >> +struct sensor_bios_data {
> >
> > And cio2_sensor_data ?
>
> Ack
>
> >> + struct device *dev;
> >> + u8 link;
> >> + u8 lanes;
> >> + u8 degree;
> >> + u32 mclkspeed;
> >> +};
> >> +
> >> +#endif
> >> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> >> index f68ef0f6b..827457110 100644
> >> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> >> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> >> @@ -1715,9 +1715,27 @@ static void cio2_queues_exit(struct cio2_device *cio2)
> >> static int cio2_pci_probe(struct pci_dev *pci_dev,
> >> const struct pci_device_id *id)
> >> {
> >> + struct fwnode_handle *endpoint;
> >> struct cio2_device *cio2;
> >> int r;
> >>
> >> + /*
> >> + * 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.
> >> + *
> >> + * This may EPROBE_DEFER if supported devices are found defined in ACPI
> >> + * but not yet ready for use (either not attached to the i2c bus yet,
> >> + * or not done probing themselves).
> >
> > Why do we need for the I2C devices to be probed, isn't it enough to have
> > them created ?
>
> Ooops - a relic of the prior version that I missed out when cleaning up
> - I'll fix that
>
> > No need for an extra indentation level, neither here, not below.
> > NO need for this blank line.
>
> Both fixed - thanks very much
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2020-10-24 9:37 UTC|newest]
Thread overview: 82+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-19 22:58 [RFC PATCH v3 0/9] Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
2020-10-19 22:58 ` [RFC PATCH v3 1/9] software_node: Add helper function to unregister arrays of software_nodes ordered parent to child Daniel Scally
2020-10-20 9:22 ` Andy Shevchenko
2020-10-20 10:05 ` Sakari Ailus
2020-10-20 11:01 ` Andy Shevchenko
2020-10-20 11:02 ` Andy Shevchenko
2020-10-20 11:04 ` Heikki Krogerus
2020-10-20 22:52 ` Dan Scally
2020-10-21 9:40 ` Andy Shevchenko
2020-10-21 9:54 ` Dan Scally
2020-10-19 22:58 ` [RFC PATCH v3 2/9] lib/test_printf.c: Use helper function to unwind array of software_nodes Daniel Scally
2020-10-20 7:23 ` Petr Mladek
2020-10-20 9:20 ` Andy Shevchenko
2020-10-19 22:58 ` [RFC PATCH v3 3/9] software_node: Fix failure to hold refcount in software_node_get_next_child Daniel Scally
2020-10-20 12:44 ` Rafael J. Wysocki
2020-10-20 13:31 ` Sakari Ailus
2020-10-20 23:25 ` Dan Scally
2020-10-21 9:33 ` Andy Shevchenko
2020-10-21 9:37 ` Sakari Ailus
2020-10-21 9:56 ` Dan Scally
2020-10-19 22:58 ` [RFC PATCH v3 4/9] software_node: Add support for fwnode_graph*() family of functions Daniel Scally
2020-10-20 9:17 ` Andy Shevchenko
2020-10-20 12:35 ` Sakari Ailus
2020-10-20 13:32 ` Sakari Ailus
2020-10-19 22:58 ` [RFC PATCH v3 5/9] ipu3-cio2: Add T: entry to MAINTAINERS Daniel Scally
2020-10-20 9:16 ` Andy Shevchenko
2020-10-24 0:28 ` Laurent Pinchart
2020-10-19 22:59 ` [RFC PATCH v3 6/9] ipu3-cio2: Rename ipu3-cio2.c to allow module to be built from multiple sources files retaining ipu3-cio2 name Daniel Scally
2020-10-20 9:15 ` Andy Shevchenko
2020-10-20 20:41 ` Dan Scally
2020-10-24 0:34 ` Laurent Pinchart
2020-10-19 22:59 ` [RFC PATCH v3 7/9] ipu3-cio2: Check if pci_dev->dev's fwnode is a software_node in cio2_parse_firmware() and set FWNODE_GRAPH_DEVICE_DISABLED if so Daniel Scally
2020-10-20 9:19 ` Andy Shevchenko
2020-10-20 12:06 ` Sakari Ailus
2020-10-20 19:56 ` Dan Scally
2020-10-20 22:49 ` Sakari Ailus
2020-10-20 22:55 ` Dan Scally
2020-10-24 0:39 ` Laurent Pinchart
2020-10-24 14:29 ` Sakari Ailus
2020-10-24 16:33 ` Dan Scally
2020-10-24 16:55 ` Laurent Pinchart
2020-10-19 22:59 ` [RFC PATCH v3 8/9] media: v4l2-core: v4l2-async: Check possible match in match_fwnode based on sd->fwnode->secondary Daniel Scally
2020-10-19 22:59 ` [RFC PATCH v3 9/9] ipu3-cio2: Add functionality allowing software_node connections to sensors on platforms designed for Windows Daniel Scally
2020-10-20 9:41 ` Andy Shevchenko
2020-10-21 22:05 ` Daniel Scally
2020-10-22 13:40 ` Andy Shevchenko
2020-10-23 10:06 ` Dan Scally
2020-10-24 1:24 ` Laurent Pinchart
2020-10-24 8:50 ` Dan Scally
2020-10-24 9:37 ` Laurent Pinchart [this message]
2020-10-24 22:28 ` Daniel Scally
2020-10-24 22:36 ` Laurent Pinchart
2020-10-24 22:50 ` Daniel Scally
2020-10-26 8:20 ` Dan Scally
2020-10-26 16:05 ` Andy Shevchenko
2020-10-29 20:17 ` Laurent Pinchart
2020-10-29 22:36 ` Dan Scally
2020-10-26 16:10 ` Andy Shevchenko
2020-10-29 20:19 ` Laurent Pinchart
2020-10-29 20:26 ` Andy Shevchenko
2020-10-29 21:29 ` Laurent Pinchart
2020-10-29 22:22 ` Andy Shevchenko
2020-10-29 22:51 ` Laurent Pinchart
2020-11-13 10:02 ` Dan Scally
2020-11-13 16:22 ` Laurent Pinchart
2020-11-13 19:45 ` Andy Shevchenko
2020-11-15 8:45 ` Daniel Scally
2020-11-16 8:53 ` Laurent Pinchart
2020-11-16 13:57 ` Andy Shevchenko
2020-11-16 14:10 ` Laurent Pinchart
2020-11-16 14:15 ` Dan Scally
2020-11-16 16:16 ` Andy Shevchenko
2020-11-17 12:01 ` Dan Scally
2020-11-17 16:42 ` Andy Shevchenko
2020-11-17 22:59 ` Dan Scally
2020-10-24 15:11 ` Sakari Ailus
2020-10-24 15:14 ` Sakari Ailus
2020-10-24 20:28 ` Dan Scally
2020-10-25 11:18 ` Sakari Ailus
2020-10-20 9:24 ` [RFC PATCH v3 0/9] Add functionality to ipu3-cio2 driver " Andy Shevchenko
2020-10-20 13:38 ` Rafael J. Wysocki
2020-10-21 20:59 ` Daniel Scally
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201024093702.GA3939@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=bingbu.cao@intel.com \
--cc=davem@davemloft.net \
--cc=djrscally@gmail.com \
--cc=dmitry.torokhov@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=heikki.krogerus@linux.intel.com \
--cc=jacopo+renesas@jmondi.org \
--cc=kieran.bingham+renesas@ideasonboard.com \
--cc=kitakar@gmail.com \
--cc=laurent.pinchart+renesas@ideasonboard.com \
--cc=linus.walleij@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux@rasmusvillemoes.dk \
--cc=mchehab@kernel.org \
--cc=pmladek@suse.com \
--cc=prabhakar.mahadev-lad.rj@bp.renesas.com \
--cc=rafael@kernel.org \
--cc=robh@kernel.org \
--cc=rostedt@goodmis.org \
--cc=sakari.ailus@linux.intel.com \
--cc=sergey.senozhatsky@gmail.com \
--cc=tian.shu.qiu@intel.com \
--cc=yong.zhi@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).