lkml.org 
[lkml]   [2020]   [Dec]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 13/18] ipu3-cio2: Add functionality allowing software_node connections to sensors on platforms designed for Windows
    From
    Date

    On 02/12/2020 10:38, Sakari Ailus wrote:
    > Hi Laurent,
    >
    > On Wed, Dec 02, 2020 at 12:30:53AM +0200, Laurent Pinchart wrote:
    >> Hi Daniel,
    >>
    >> On Tue, Dec 01, 2020 at 10:08:25PM +0000, Dan Scally wrote:
    >>> On 30/11/2020 17:09, Laurent Pinchart wrote:
    >>>> On Mon, Nov 30, 2020 at 01:31:24PM +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.
    >>>>>
    >>>>> Suggested-by: Jordan Hand <jorhand@linux.microsoft.com>
    >>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
    >>>>> ---
    >>>>> Changes since RFC v3:
    >>>>>
    >>>>> - Removed almost all global variables, dynamically allocated
    >>>>> the cio2_bridge structure, plus a bunch of associated changes
    >>>>> like
    >>>>> - Added a new function to ipu3-cio2-main.c to check for an
    >>>>> existing fwnode_graph before calling cio2_bridge_init()
    >>>>> - Prefixed cio2_bridge_ to any variables and functions that
    >>>>> lacked it
    >>>>> - Assigned the new fwnode directly to the sensor's ACPI device
    >>>>> fwnode as secondary. This removes the requirement to delay until
    >>>>> the I2C devices are instantiated before ipu3-cio2 can probe, but
    >>>>> it has a side effect, which is that those devices then grab a ref
    >>>>> to the new software_node. This effectively prevents us from
    >>>>> unloading the driver, because we can't free the memory that they
    >>>>> live in whilst the device holds a reference to them. The work
    >>>>> around at the moment is to _not_ unregister the software_nodes
    >>>>> when ipu3-cio2 is unloaded; this becomes a one-time 'patch', that
    >>>>> is simply skipped if the module is reloaded.
    >>>>> - Moved the sensor's SSDB struct to be a member of cio2_sensor
    >>>>> - Replaced ints with unsigned ints where appropriate
    >>>>> - Iterated over all ACPI devices of a matching _HID rather than
    >>>>> just the first to ensure we handle a device with multiple sensors
    >>>>> of the same model.
    >>>>>
    >>>>> MAINTAINERS | 1 +
    >>>>> drivers/media/pci/intel/ipu3/Kconfig | 18 ++
    >>>>> drivers/media/pci/intel/ipu3/Makefile | 1 +
    >>>>> drivers/media/pci/intel/ipu3/cio2-bridge.c | 260 ++++++++++++++++++
    >>>>> drivers/media/pci/intel/ipu3/cio2-bridge.h | 108 ++++++++
    >>>>> drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 27 ++
    >>>>> drivers/media/pci/intel/ipu3/ipu3-cio2.h | 6 +
    >>>>> 7 files changed, 421 insertions(+)
    >>>>> 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 9702b886d6a4..188559a0a610 100644
    >>>>> --- a/MAINTAINERS
    >>>>> +++ b/MAINTAINERS
    >>>>> @@ -8927,6 +8927,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 82d7f17e6a02..2b3350d042be 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:
    >>>>> +
    >>>>> + - Microsoft Surface models (except Surface Pro 3)
    >>>>> + - The Lenovo Miix line (for example the 510, 520, 710 and 720)
    >>>>> + - 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 429d516452e4..933777e6ea8a 100644
    >>>>> --- a/drivers/media/pci/intel/ipu3/Makefile
    >>>>> +++ b/drivers/media/pci/intel/ipu3/Makefile
    >>>>> @@ -2,3 +2,4 @@
    >>>>> obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o
    >>>>>
    >>>>> 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 000000000000..fd3f8ba07274
    >>>>> --- /dev/null
    >>>>> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
    >>>>> @@ -0,0 +1,260 @@
    >>>>> +// SPDX-License-Identifier: GPL-2.0
    >>>>> +/* Author: Dan Scally <djrscally@gmail.com> */
    >>>> Could you please add a blank line here ?
    >>> Yes
    >>>
    >>>>> +#include <linux/acpi.h>
    >>>>> +#include <linux/device.h>
    >>>>> +#include <linux/i2c.h>
    >>>> Is this header needed ?
    >>>>
    >>>>> +#include <linux/kernel.h>
    >>>>> +#include <linux/module.h>
    >>>> And this one ?
    >>>>
    >>>>> +#include <linux/pci.h>
    >>>>> +#include <linux/property.h>
    >>>>> +#include <media/v4l2-subdev.h>
    >>>> And this one ?
    >>> Ah yes - bit sloppy, they're orphaned from earlier versions, sorry about
    >>> that.
    >>>
    >>>>> +
    >>>>> +#include "cio2-bridge.h"
    >>>>> +
    >>>>> +/*
    >>>>> + * Extend this array with ACPI Hardware ID's of devices known to be working.
    >>>>> + * Do not add a HID for a sensor that is not actually supported.
    >>>>> + */
    >>>>> +static const char * const cio2_supported_devices[] = {
    >>>> Maybe cio2_supported_sensors ?
    >>> Sure
    >>>
    >>>>> + "INT33BE",
    >>>>> + "OVTI2680",
    >>>>> +};
    >>>>> +
    >>>>> +static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
    >>>>> + void *data, u32 size)
    >>>>> +{
    >>>>> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
    >>>>> + union acpi_object *obj;
    >>>>> + acpi_status status;
    >>>>> + int ret;
    >>>>> +
    >>>>> + status = acpi_evaluate_object(adev->handle, id, NULL, &buffer);
    >>>>> + if (ACPI_FAILURE(status))
    >>>>> + return -ENODEV;
    >>>>> +
    >>>>> + obj = buffer.pointer;
    >>>>> + if (!obj) {
    >>>>> + dev_err(&adev->dev, "Couldn't locate ACPI buffer\n");
    >>>>> + return -ENODEV;
    >>>>> + }
    >>>>> +
    >>>>> + if (obj->type != ACPI_TYPE_BUFFER) {
    >>>>> + dev_err(&adev->dev, "Not an ACPI buffer\n");
    >>>>> + ret = -ENODEV;
    >>>>> + goto out_free_buff;
    >>>>> + }
    >>>>> +
    >>>>> + if (obj->buffer.length > size) {
    >>>>> + dev_err(&adev->dev, "Given buffer is too small\n");
    >>>>> + ret = -EINVAL;
    >>>>> + 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 void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
    >>>>> +{
    >>>>> + strcpy(sensor->prop_names.clock_frequency, "clock-frequency");
    >>>>> + strcpy(sensor->prop_names.rotation, "rotation");
    >>>>> + strcpy(sensor->prop_names.bus_type, "bus-type");
    >>>>> + strcpy(sensor->prop_names.data_lanes, "data-lanes");
    >>>>> + strcpy(sensor->prop_names.remote_endpoint, "remote-endpoint");
    >>>> This is a bit fragile, as there's no len check. How about the following
    >>>> ?
    >>>> static const struct cio2_property_names prop_names = {
    >>>> .clock_frequency = "clock-frequency",
    >>>> .rotation = "rotation",
    >>>> .bus_type = "bus-type",
    >>>> .data_lanes = "data-lanes",
    >>>> .remote_endpoint = "remote-endpoint",
    >>>> };
    >>>>
    >>>> static void cio2_bridge_init_property_names(struct cio2_sensor *sensor)
    >>>> {
    >>>> sensor->prop_names = prop_names;
    >>>> }
    >>>>
    >>>> This shoudl generate a compilation warning if the string is too long.
    >>>>
    >>>> You could even inline that line in
    >>>> cio2_bridge_create_fwnode_properties().
    >>> Yes, I like that, thanks - I'll make the change.
    >>>
    >>>>> +}
    >>>>> +
    >>>>> +static void cio2_bridge_create_fwnode_properties(struct cio2_sensor *sensor)
    >>>>> +{
    >>>>> + unsigned int i;
    >>>>> +
    >>>>> + cio2_bridge_init_property_names(sensor);
    >>>>> +
    >>>>> + for (i = 0; i < 4; i++)
    >>>>> + sensor->data_lanes[i] = i + 1;
    >>>> Is there no provision in the SSDB for data lane remapping ?
    >>> Sorry; don't follow what you mean by data lane remapping here.
    >> Some CSI-2 receivers can remap data lanes. The routing inside the SoC
    >> from the data lane input pins to the PHYs is configurable. This makes
    >> board design easier as you can route the data lanes to any of the
    >> inputs. That's why the data lanes DT property is a list of lane numbers
    >> instead of a number of lanes. I'm actually not sure if the CIO2 supports
    >> this.
    > To my knowledge it does not. Only the number of lanes allocated to
    > different ports matters.
    >
    So nothing to change here then I think?
    >>>>> @@ -0,0 +1,108 @@
    >>>>> +/* SPDX-License-Identifier: GPL-2.0 */
    >>>>> +/* Author: Dan Scally <djrscally@gmail.com> */
    >>>>> +#ifndef __CIO2_BRIDGE_H
    >>>>> +#define __CIO2_BRIDGE_H
    >>>>> +
    >>>>> +#include <linux/property.h>
    >>>>> +
    >>>>> +#define CIO2_HID "INT343E"
    >>>>> +#define CIO2_NUM_PORTS 4
    >>>> There are a few rogue spaces before '4'.
    >>> Argh, thanks, this is the curse of using VS code on multiple machines...
    >> I recommend vim ;-)
    > What is VS code? Very Serious Code?

    Visual Studio Code - it has some nice features, but the
    facepalm-to-productivity ratio is a bit high.

    > I can recommend Emacs; that could help, too.

    \
     
     \ /
      Last update: 2020-12-02 11:55    [W:4.613 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site