lkml.org 
[lkml]   [2020]   [Dec]   [1]   [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
    Hi Laurent
    On 01/12/2020 22:30, Laurent Pinchart wrote:
    >>>> +}
    >>>> +
    >>>> +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.

    I don't see anything in the SSDB that might refer to that, though of
    course we're lacking documentation for it so it could be a part that we
    don't understand yet.


    >>>> + dev_info(&bridge->cio2->dev,
    >>>> + "Found supported sensor %s\n",
    >>>> + acpi_dev_name(adev));
    >>>> +
    >>>> + bridge->n_sensors++;
    >>> We probably want a check here to avoid overflowing bridge->sensors. The
    >>> other option is to make bridge->sensors a struct list_head and allocate
    >>> sensors dynamically.
    >> Err - agree on a check. There's only 4 ports in a CIO2 device, so that's
    >> the maximum. Seems easier to just do a check, unless the wasted memory
    >> is enough that it's worth allocating dynamically. I don't mind either
    >> approach.
    > In theory we could route multiple sensors to the same receiver, as long
    > as only one of them drives the lanes at any given time. It's one way to
    > support multiple sensors in cheap designs. I doubt we'll ever encounter
    > that with the IPU3, so we could just limit the count to 4.
    Ah, that's neat though. But I'll leave it at a check at the top of the
    loop for now.
    >>>> +
    >>>> + fwnode = software_node_fwnode(&bridge->cio2_hid_node);
    >>>> + if (!fwnode) {
    >>>> + dev_err(dev, "Error getting fwnode from cio2 software_node\n");
    >>>> + ret = -ENODEV;
    >>>> + goto err_unregister_sensors;
    >>> Can this happen ?
    >> It _shouldn't_ happen, as long as nothing else is touching the swnodes
    >> I've registered or anything. I've never seen it happen. That didn't feel
    >> like quite enough to say it can't ever happen - but I'm happy to skip
    >> the check if you think thats ok.
    > It seems a bit overkill to me, but I'm not a swnode specialist :-)
    I'm going to keep it, if you have no strong feelings, partly through
    caution but also because the other place swnodes are most heavily used
    (drivers/platform/x86/intel_cht_int33fe_typec.c) _does_ perform the
    check, so consistency too.
    >>>> @@ -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 ;-)
    You're not the only one - maybe I need to spend the time and it'll save
    time in the future

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