[PATCH v4 0/8] Add support for GE SUNH hot-pluggable connector

[PATCH v4 0/8] Add support for GE SUNH hot-pluggable connector

[Luca Ceresoli]

Hello,

this series aims at supporting a Linux device with a connector to
physically add and remove an add-on to/from the main device to augment its
features at runtime, adding devices on non-discoverable busses, using
device tree overlays.

Changes since v4 are limited, but I'm sending this to have the state of the
art public before the discussion at the Lonux Plumbers Conference tomorrow
(https://lpc.events/event/18/contributions/1750/). See the changelog at the
end for the details.

The high-level description below is an updated version of to the one in v3.

Use case
========

This series targets a professional product (GE SUNH) that is composed of a
"main" part running on battery, with the main SoC and able to work
autonomously with limited features, and an optional "add-on" that enables
more features by adding more hardware peripherals, some of which are on
non-discoverable busses such as I2C and MIPI DSI.

The add-on can be connected and disconnected at runtime at any moment by
the end user, and add-on features need to be enabled and disabled
automatically at runtime.

The add-on has status pins that are connected to GPIOs on the main board,
allowing the CPU to detect add-on insertion and removal. It also has a
reset GPIO allowing to reset all peripherals on the add-on at once.

The features provided by the add-on include a display and a battery charger
to recharge the battery of the main part. The display on the add-on has an
LVDS input but the connector between the base and the add-on has a MIPI DSI
bus, so a DSI-to-LVDS bridge is present on the add-on.

Different add-on models can be connected to the main part, and for this a
model ID is stored in the add-on itself so the software running on the CPU
on the main part knows which non-discoverable hardware to probe.

Overall approach
================

Device tree overlays appear as the most natural solution to support the
addition and removal of devices from a running system.

Several features are missing from the mainline Linux kernel in order to
support this use case:

 1. runtime (un)loading of device tree overlays is currently not exposed
 2. if enabled, overlay (un)loading exposes several issues and limitations
 3. the DRM subsystem assumes video bridges are non-removable

This series targets items 1 and 3 and some of the issues mentioned in item
2. Other issues are being handled separately (see "Device tree overlay
issues" below).

Device tree representation and connector driver
===============================================

The device tree description we propose involves 3 main parts.

1: the main (fixed) device tree

The main device tree describes the connector itself along with the status
and reset GPIOs. It also provides specific nodes for the various interfaces
(I2C and DSI). Here is how the connector is represented in the fixed part
of the device tree:

    / {
        #include <dt-bindings/gpio/gpio.h>

        addon_connector: addon-connector {
            compatible = "ge,sunh-addon-connector";
            reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
            plugged-gpios = <&gpio1 2 GPIO_ACTIVE_LOW>;

            i2c-dbat {
                i2c-parent = <&i2c2_ch1>;
                #address-cells = <1>;
                #size-cells = <0>;
            };

            i2c-gp {
                i2c-parent = <&i2c5>;
                #address-cells = <1>;
                #size-cells = <0>;
            };

            i2c-btp {
                i2c-parent = <&i2c3>;
                #address-cells = <1>;
                #size-cells = <0>;
            };

            dsi {
                ports {
                    #address-cells = <1>;
                    #size-cells = <0>;

                    port@0 {
                        reg = <0>;

                        hotplug_bridge_sink: endpoint {
                            remote-endpoint = <&dsi_to_hotplug_bridge>;
                        };
                    };
                };
            };
        };
    };

The connector has a specific compatible string, and this series adds a
driver supporting it. This driver uses the device tree overlay loading and
unloading facilities already implemented by the kernel but not currently
exposed.

The driver detects the connection status from the "plugged" GPIO and reacts
to a connection event by loading a first overlay (the "base" overlay, see
below).

The 'i2c-*' nodes represent the hot-pluggable section of I2C busses
crossing the connector, whose controller is on the main board but which
have devices on the add-on. There is one node per each such bus: this
allows full decoupling between the base board and the overlay, but requires
an additional info to associate the 'i2c-*' node to the physical bus
controller.

Similarly, the 'dsi' node describes how the two sides of the MIPI DSI bus
connect to each other. The base device tree describes the fixed part of the
video pipeline as 'port@0', i.e. the physical bus that terminates on the
connector. 'port@1', representing the continuation of the video bus on the
add-on, will be added by an overlay.

The 'dsi' node would also allow to describe a similar connector having
multiple video busses: these would have one node each, such as 'dsi-foo',
'dsi-bar', 'lvds-foo', 'lvds-bar' etc while keeping the ports for each
connector appropriately separated.

2: the "base" overlay

The "base" overlay describes the common components that are required to
read the model ID. These components are identical for all add-on models,
thus only one "base" overlay is needed:

    /dts-v1/;
    /plugin/;

    / {
        fragment@0 {
            target-path = "";

            __overlay__ {
                nvmem-cells = <&addon_id>;
                nvmem-cell-names = "id";

                i2c-dbat {
                    addon_eeprom: eeprom@51 {
                        compatible = "atmel,24c64";
                        reg = <0x51>;
                        pagesize = <32>;

                        nvmem-layout {
                            compatible = "fixed-layout";
                            #address-cells = <1>;
                            #size-cells = <1>;

                            /* Data cells */
                            addon_id: addon-id@400 {
                                reg = <0x400 0x1>;
                            };
                        };
                    };
                };
            };
        };
    };

Note the overlay does not have a target node. This allows the overlay to be
fully decoupled from the base tree, and to allow multiple compatible
connectors on the same base board. It also avoids the need to add
properties to nodes in the base tree by avoiding phandle references across
the overlay boundaries. With an exception. Indeed the 'nvmem-cells' and
'nvmem-cell-names' are the only two properties added to a node that is in
the base tree. This is still waiting for a different representation to
avoid adding such properties and all the deadprops and leaks thereof.

Here an I2C device is added by a subnode of 'i2c-dbat' for an EEPROM. The
i2c-dbat node itself is already present in the base tree, carrying the link
to the actual I2C adapter node.

The EEPROM holds the model ID of each add-on, using always the same I2C
address and memory offset.

3: the "add-on-specific" overlay

Based on the model ID, the connector driver loads the second overlay, which
describes all the add-on hardware not yet described by the base
overlay. This overlay is model-specific.

Excerpt:

    / {
        fragment@0 {
            target-path = "";

            __overlay__ {
                dsi {
                    ports {
                        port@1 {
                            reg = <1>;
                            hotplug_bridge_source: endpoint {
                                remote-endpoint = <&sn65dsi84_from_bridge>;
                            };
                        };
                    };
                };

                i2c-gp {
                    #address-cells = <1>;
                    #size-cells = <0>;

                    dsi-lvds-bridge@2c {
                        compatible = "ti,sn65dsi84";
                        reg = <0x2c>;

                        ports {
                            port@0 {
                                reg = <0>;
                                sn65dsi84_from_bridge: endpoint {
                                    remote-endpoint = <&hotplug_bridge_source>;
                                    data-lanes = <1 2 3 4>;
                                };
                            };
                            port@2 {
                                reg = <2>;
                                sn65dsi84_out0: endpoint {
                                    remote-endpoint = <&panel_dsi_lvds_in0>;
                                };
                            };
                            port@3 {
                                reg = <3>;
                                sn65dsi84_out1: endpoint {
                                    remote-endpoint = <&panel_dsi_lvds_in1>;
                                };
                            };
                        };
                    };
                };

                devices {
                    reg_addon_3v3_lcd: regulator-addon-3v3-lcd {
                        compatible = "regulator-fixed";
                        regulator-name = "3V3_LCD_ADDON";
                        ...
                    };

                    backlight_addon: backlight-addon {
                        compatible = "led-backlight";
                        ...
                    };

                    addon_panel_dsi_lvds: panel-dsi-lvds {
                        compatible = "...";
                        power-supply = <&reg_addon_3v3_lcd>;
                        backlight = <&backlight_addon>;

                        ports {
                            #address-cells = <1>;
                            #size-cells = <0>;
                            port@0{
                                reg = <0>;
                                dual-lvds-odd-pixels;
                                panel_dsi_lvds_in0: endpoint {
                                    remote-endpoint = <&sn65dsi84_out0>;
                                };
                            };

                            port@1{
                                reg = <1>;
                                dual-lvds-even-pixels;
                                panel_dsi_lvds_in1: endpoint {
                                    remote-endpoint = <&sn65dsi84_out1>;
                                };
                            };
                        };
                    };
                };
            };
        };
    };

Here the 'dsi/ports/port@1' node is completing the 'dsi' section already
present in the base tree, thus describing that this add-on is connecting
those DSI lines to something, in this case the SN65DSI84 DSI-to-LVDS bridge
and from to an LVDS panel.

The 'devices' node, containts one subnode for each device that is not on
any CPU-reachable bus (I2C, DSI, etc): fixed/GPIO regulators, backlight,
the panel etc. In normal (no overlay) device tree systems nodes for these
devices are children of the root node, and are probed as platform devices
by kernel code. With the connector we need to have them under the connector
node, and the choice was to let them be children of the connector node or
to group them into a new subnode. We chose the latter because that provides
a more explicit representation of reality, and is coherent with the 'dsi'
and 'i2c-*' nodes. As a good side effect for the implementation, this means
other nodes under the connector node (dsi, i2c-*) are not considered when
populating platform devices.

After these steps, the add-on is fully described and working on the
system. When the "plugged" GPIO reports a disconnection, the overlays are
unloaded in reverse order and devices removed.

DRM hotplug bridge driver
=========================

DRM natively supports pipelines whose display can be removed, but all the
components preceding it (all the display controller and any bridges) are
assumed to be fixed and cannot be plugged, removed or modified at runtime.

This series adds support for DRM pipelines having a removable part after
the encoder, thus also allowing bridges to be removed and reconnected at
runtime, possibly with different components.

This picture summarizes the DRM structure implemented by this series:

 .------------------------.
 |   DISPLAY CONTROLLER   |
 | .---------.   .------. |
 | | ENCODER |<--| CRTC | |
 | '---------'   '------' |
 '------|-----------------'
        |
        |DSI            HOTPLUG
        V              CONNECTOR
   .---------.        .--.    .-.        .---------.         .-------.
   | 0 to N  |        | _|   _| |        | 1 to N  |         |       |
   | BRIDGES |--DSI-->||_   |_  |--DSI-->| BRIDGES |--LVDS-->| PANEL |
   |         |        |  |    | |        |         |         |       |
   '---------'        '--'    '-'        '---------'         '-------'

 [--- fixed components --]  [----------- removable add-on -----------]

Fixed components include:

 * all components up to the DRM encoder, usually part of the SoC
 * optionally some bridges, in the SoC and/or as external chips

Components on the removable add-on include:

 * one or more bridges
 * a fixed connector (not one natively supporting hotplug such as HDMI)
 * the panel

The video bus is MIPI DSI in the example and in the implementation provided
by this series, but the implementation is meant to allow generalization to
other video busses without native hotplug support, such as parallel video
and LVDS.

Note that the term "connector" in this context is different from the "DRM
connector" abstraction already present in the DRM subsystem (struct
drm_connector).

More details in the commit message of patch 4.

That's all
==========

Thanks for you patience in reading this!

Luca

Changes in v4:
- Replaced DRM bridge notifier with a new callback in struct drm_bridge_funcs
- Added patch for missing devlink (LEDs used by backlight)
- Various cleanups
- Rebased on v6.11
- Link to v3: https://lore.kernel.org/r/20240809-hotplug-drm-bridge-v3-0-b4c178380bc9@bootlin.com

Changes in v3 (too many changes in v3 to mention them all, but here are the
big ones):
- Rewrote the DT format to allow fully decoupled overlays and to avoid
  adding properties (with the NVMEM exception still to be solved)
- Implemented device instantiation based on the new DT format: i2c in
  i2c-core-of.c nobus-devices in the connector driver
- DRM: insert/remove an LVDS DRM connector on hot(un)plug events
- Added patch for a devlink issue on overlay removal (mostly to start
  discussion)
- Link to v2: https://lore.kernel.org/r/20240510-hotplug-drm-bridge-v2-0-ec32f2c66d56@bootlin.com

Changes in v2:
- Added bindings and driver for ge,sunh-addon-connector
- Removed bindings for the hotplug-video-connector, this is now represented
  in DT as part of the ge,sunh-addon-connector
- Various monior improvements to the DRM hotplug-bridge driver
- Link to v1: https://lore.kernel.org/r/20240326-hotplug-drm-bridge-v1-0-4b51b5eb75d5@bootlin.com

Co-developed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Luca Ceresoli (8):
      dt-bindings: connector: add GE SUNH hotplug addon connector
      drm/bridge: allow bridges to be informed about added and removed bridges
      drm/encoder: add drm_encoder_cleanup_from()
      drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges
      i2c: i2c-core-of: follow i2c-parent phandle to probe devices from added nodes
      backlight: led-backlight: add devlink to supplier LEDs
      [RFC] driver core: devlink: do not unblock consumers without any drivers found
      misc: add ge-addon-connector driver

 .../connector/ge,sunh-addon-connector.yaml         | 177 ++++++
 MAINTAINERS                                        |  11 +
 drivers/base/core.c                                |  21 -
 drivers/gpu/drm/bridge/Kconfig                     |  17 +
 drivers/gpu/drm/bridge/Makefile                    |   1 +
 drivers/gpu/drm/bridge/hotplug-bridge.c            | 661 +++++++++++++++++++++
 drivers/gpu/drm/drm_bridge.c                       |  12 +
 drivers/gpu/drm/drm_encoder.c                      |  21 +
 drivers/i2c/i2c-core-of.c                          |   9 +
 drivers/misc/Kconfig                               |  18 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/ge-sunh-connector.c                   | 481 +++++++++++++++
 drivers/video/backlight/led_bl.c                   |  13 +
 include/drm/drm_bridge.h                           |  23 +
 include/drm/drm_encoder.h                          |   1 +
 15 files changed, 1446 insertions(+), 21 deletions(-)
---
base-commit: c377ce116b054708c8106e58a89123f1eb43e426
change-id: 20240319-hotplug-drm-bridge-16b86e67fe92

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>

[PATCH v4 1/8] dt-bindings: connector: add GE SUNH hotplug addon connector

[Luca Ceresoli]

Add bindings for the GE SUNH add-on connector. This is a physical,
hot-pluggable connector that allows to attach and detach at runtime an
add-on adding peripherals on non-discoverable busses.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changed in v4:
 - rename 'nobus-devices' to 'devices'
 - use 'additionalProperties: true' for the 'devices' node (nodes are added
   by overlays)
 - document GPIO polarity
 - add '|' for descriptions to preserve line breaks
 - remove powergood-gpios (removed in hardware design)
 - Omit "/" node, not needed and cause of warnings
 - remove reference to v2 examples from example comment
 - remove unneeded "addon_connector" label from example

Changed in v3:
 - change the layout to only add subnodes, not properties
 - add the 'nobus-devices' node description to hold devices not on any bus
 - add 'i2c-*' nodes for the I2C busses, using a i2c-parent phandle
 - and the 'dsi' node for the DSI bus
 - move the entire port@1 node to the overlay (not only the remote-endpoint
   property)
 - remove the overlay examples (Overlays in examples are not supported)
 - add more clarifying descriptions and comments for examples
 - some rewording

This patch was added in v2.
---
 .../connector/ge,sunh-addon-connector.yaml         | 177 +++++++++++++++++++++
 MAINTAINERS                                        |   5 +
 2 files changed, 182 insertions(+)

diff --git a/Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml b/Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml
new file mode 100644
index 000000000000..68d7d7d7cf7e
--- /dev/null
+++ b/Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml
@@ -0,0 +1,177 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/connector/ge,sunh-addon-connector.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GE SUNH hotplug add-on connector
+
+maintainers:
+  - Luca Ceresoli <luca.ceresoli@bootlin.com>
+
+description: |
+  Represent the physical connector present on GE SUNH devices that allows
+  to attach and detach at runtime an add-on adding peripherals on
+  non-discoverable busses. Peripherals on the add-on include I2C sensors
+  and a video bridge controlled via I2C.
+
+  The connector has status GPIOs to notify the connection status to the CPU
+  and a reset GPIO to allow the CPU to reset all the peripherals on the
+  add-on. It also has I2C busses and a 4-lane MIPI DSI bus.
+
+  Different add-on models can be connected, each having different
+  peripherals. For this reason each add-on has a model ID stored in a
+  non-volatile memory, which is accessed in the same way on all add-ons.
+
+  Add-on removal can happen at any moment under user control and without
+  prior notice to the CPU, making all of its components not usable
+  anymore. Later on, the same or a different add-on model can be connected.
+
+properties:
+  compatible:
+    const: ge,sunh-addon-connector
+
+  reset-gpios:
+    description: An output GPIO to reset the peripherals on the
+      add-on. Active low.
+    maxItems: 1
+
+  plugged-gpios:
+    description: An input GPIO that is asserted if and only if an add-on is
+      physically connected. Active low.
+    maxItems: 1
+
+  devices:
+    description: |
+      A container for devices not accessible via any data bus. Common use
+      cases include fixed and GPIO regulators, simple video panels and LED
+      or GPIO backlight devices. When not hot-pluggable, nodes such devices
+      are children of the root node.
+
+      This node should not be present in the connector description in the
+      base device tree. It should be added by overlays along with a subnode
+      per device.
+
+    type: object
+    additionalProperties: true
+
+  dsi:
+    type: object
+    additionalProperties: false
+
+    properties:
+      ports:
+        $ref: /schemas/graph.yaml#/properties/ports
+
+        description: |
+          OF graph bindings modeling the MIPI DSI bus across the connector. The
+          connector splits the video pipeline in a fixed part and a removable
+          part.
+
+          The fixed part of the video pipeline includes all components up to
+          the display controller and 0 or more bridges. The removable part
+          includes any bridges and any other components up to the panel.
+
+        properties:
+          port@0:
+            $ref: /schemas/graph.yaml#/properties/port
+            description:
+              The last point of the non-removable part of the MIPI DSI bus
+              line. The remote-endpoint sub-node must point to the last
+              non-removable video component of the video pipeline.
+
+          port@1:
+            $ref: /schemas/graph.yaml#/properties/port
+            description:
+              The first point of the removable part of the MIPI DSI bus
+              line.  The remote-endpoint sub-node must point to the first
+              video pipeline component on the add-on. As it describes the
+              hot-pluggable hardware, the endpoint node cannot be filled
+              until an add-on is detected, so this node needs to be added
+              by a device tree overlay at runtime.
+
+        required:
+          - port@0
+          # port@1 is added by the overlay for any add-on using the DSI lines
+
+    required:
+      - ports
+
+patternProperties:
+  '^i2c-(dbat|gp|btp)$':
+    description:
+      An I2C bus that goes through the connector. The adapter (and possibly
+      some clients) are on the fixed side. Add-ons that have any clients on
+      this bus have to be added by the add-on overlay, inside this node.
+
+    $ref: /schemas/i2c/i2c-controller.yaml#
+    unevaluatedProperties: false
+    type: object
+
+    properties:
+      i2c-parent:
+        $ref: /schemas/types.yaml#/definitions/phandle
+        description:
+          Phandle pointing to the I2C bus controller on the fixed side that
+          drives this bus
+
+required:
+  - compatible
+  - i2c-dbat
+  - i2c-gp
+  - i2c-btp
+  - dsi
+
+unevaluatedProperties: false
+
+examples:
+  # This is the description of the connector as it should appear in the
+  # main DTS describing the "main" board up to the connector. This is
+  # supposed to be used together with the overlays that describe the add-on
+  # components.
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    addon-connector {
+        compatible = "ge,sunh-addon-connector";
+        reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
+        plugged-gpios = <&gpio1 2 GPIO_ACTIVE_LOW>;
+        powergood-gpios = <&gpio1 3 GPIO_ACTIVE_HIGH>;
+
+        i2c-dbat {
+            i2c-parent = <&i2c5_ch2>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            // device subnodes to be added by overlays
+        };
+
+        i2c-gp {
+            i2c-parent = <&i2c4>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            // device subnodes to be added by overlays
+        };
+
+        i2c-btp {
+            i2c-parent = <&i2c3>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            // device subnodes to be added by overlays
+        };
+
+        dsi {
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+
+                    endpoint {
+                        remote-endpoint = <&previous_bridge_out>;
+                    };
+                };
+
+                // port@1 to be added by overlay
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index cc40a9d9b8cd..b939284d1350 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10270,6 +10270,11 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
 F:	drivers/iio/pressure/mprls0025pa*
 
+HOTPLUG CONNECTOR FOR GE SUNH ADDONS
+M:	Luca Ceresoli <luca.ceresoli@bootlin.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml
+
 HP BIOSCFG DRIVER
 M:	Jorge Lopez <jorge.lopez2@hp.com>
 L:	platform-driver-x86@vger.kernel.org

-- 
2.34.1

[PATCH v4 2/8] drm/bridge: allow bridges to be informed about added and removed bridges

[Luca Ceresoli]

In preparation for allowing bridges to be added to and removed from a DRM
card without destroying the whole card, add a new DRM bridge function
called on addition and removal of bridges.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/drm_bridge.c | 12 ++++++++++++
 include/drm/drm_bridge.h     | 23 +++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index d44f055dbe3e..c98117f1abec 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -205,8 +205,14 @@ static LIST_HEAD(bridge_list);
  */
 void drm_bridge_add(struct drm_bridge *bridge)
 {
+	struct drm_bridge *br, *tmp;
+
 	mutex_init(&bridge->hpd_mutex);
 
+	list_for_each_entry_safe(br, tmp, &bridge_list, list)
+		if (br->funcs->bridge_event_notify)
+			br->funcs->bridge_event_notify(br, DRM_EVENT_BRIDGE_ADD, bridge);
+
 	mutex_lock(&bridge_lock);
 	list_add_tail(&bridge->list, &bridge_list);
 	mutex_unlock(&bridge_lock);
@@ -243,10 +249,16 @@ EXPORT_SYMBOL(devm_drm_bridge_add);
  */
 void drm_bridge_remove(struct drm_bridge *bridge)
 {
+	struct drm_bridge *br, *tmp;
+
 	mutex_lock(&bridge_lock);
 	list_del_init(&bridge->list);
 	mutex_unlock(&bridge_lock);
 
+	list_for_each_entry_safe(br, tmp, &bridge_list, list)
+		if (br->funcs->bridge_event_notify)
+			br->funcs->bridge_event_notify(br, DRM_EVENT_BRIDGE_REMOVE, bridge);
+
 	mutex_destroy(&bridge->hpd_mutex);
 }
 EXPORT_SYMBOL(drm_bridge_remove);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 75019d16be64..635cc24bdeba 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -54,6 +54,11 @@ enum drm_bridge_attach_flags {
 	DRM_BRIDGE_ATTACH_NO_CONNECTOR = BIT(0),
 };
 
+enum drm_bridge_event_type {
+	DRM_EVENT_BRIDGE_ADD,
+	DRM_EVENT_BRIDGE_REMOVE,
+};
+
 /**
  * struct drm_bridge_funcs - drm_bridge control functions
  */
@@ -676,6 +681,24 @@ struct drm_bridge_funcs {
 				    enum hdmi_infoframe_type type,
 				    const u8 *buffer, size_t len);
 
+	/**
+	 * @bridge_event_notify:
+	 *
+	 * Notify that another bridge is being added or removed.
+	 *
+	 * This callback is optional. Bridges implementing it must always
+	 * check whether the event refers to a bridge they actually need to
+	 * interact with.
+	 *
+	 * @bridge: bridge being notified
+	 * @event: event happened (add/remove bridge)
+	 * @new_bridge: the bridge mentioned by the event (i.e. the bridge
+	 *              being added or removed)
+	 */
+	void (*bridge_event_notify)(struct drm_bridge *bridge,
+				    enum drm_bridge_event_type event,
+				    struct drm_bridge *event_bridge);
+
 	/**
 	 * @debugfs_init:
 	 *

-- 
2.34.1

[PATCH v4 3/8] drm/encoder: add drm_encoder_cleanup_from()

[Luca Ceresoli]

Supporting hardware whose final part of the DRM pipeline can be physically
removed requires the ability to detach all bridges from a given point to
the end of the pipeline.

Introduce a variant of drm_encoder_cleanup() for this.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changes in v3: none

Changed in v2:
 - fix a typo in a comment
---
 drivers/gpu/drm/drm_encoder.c | 21 +++++++++++++++++++++
 include/drm/drm_encoder.h     |  1 +
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index 8f2bc6a28482..472dfbefe296 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -207,6 +207,27 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
 }
 EXPORT_SYMBOL(drm_encoder_cleanup);
 
+/**
+ * drm_encoder_cleanup_from - remove a given bridge and all the following
+ * @encoder: encoder whole list of bridges shall be pruned
+ * @bridge: first bridge to remove
+ *
+ * Removes from an encoder all the bridges starting with a given bridge
+ * and until the end of the chain.
+ *
+ * This should not be used in "normal" DRM pipelines. It is only useful for
+ * devices whose final part of the DRM chain can be physically removed and
+ * later reconnected (possibly with different hardware).
+ */
+void drm_encoder_cleanup_from(struct drm_encoder *encoder, struct drm_bridge *bridge)
+{
+	struct drm_bridge *next;
+
+	list_for_each_entry_safe_from(bridge, next, &encoder->bridge_chain, chain_node)
+		drm_bridge_detach(bridge);
+}
+EXPORT_SYMBOL(drm_encoder_cleanup_from);
+
 static void drmm_encoder_alloc_release(struct drm_device *dev, void *ptr)
 {
 	struct drm_encoder *encoder = ptr;
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index 977a9381c8ba..bafcabb24267 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -320,6 +320,7 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
 }
 
 void drm_encoder_cleanup(struct drm_encoder *encoder);
+void drm_encoder_cleanup_from(struct drm_encoder *encoder, struct drm_bridge *bridge);
 
 /**
  * drm_for_each_encoder_mask - iterate over encoders specified by bitmask

-- 
2.34.1

[PATCH v4 4/8] drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges

[Luca Ceresoli]

This driver implements the point of a DRM pipeline where a connector allows
removal of all the following bridges up to the panel.

The DRM subsystem currently allows hotplug of the monitor but not preceding
components. However there are embedded devices where the "tail" of the DRM
pipeline, including one or more bridges, can be physically removed:

 .------------------------.
 |   DISPLAY CONTROLLER   |
 | .---------.   .------. |
 | | ENCODER |<--| CRTC | |
 | '---------'   '------' |
 '------|-----------------'
        |
        |               HOTPLUG
        V              CONNECTOR
   .---------.        .--.    .-.        .---------.         .-------.
   | 0 to N  |        | _|   _| |        | 1 to N  |         |       |
   | BRIDGES |--DSI-->||_   |_  |--DSI-->| BRIDGES |--LVDS-->| PANEL |
   |         |        |  |    | |        |         |         |       |
   '---------'        '--'    '-'        '---------'         '-------'

 [--- fixed components --]  [----------- removable add-on -----------]

This driver supports such a device, where the final segment of a MIPI DSI
bus, including one or more bridges, can be physically disconnected and
reconnected at runtime, possibly with a different model.

The add-on supported by this driver has a MIPI DSI bus traversing the
hotplug connector and a DSI to LVDS bridge and an LVDS panel on the add-on.
Hovever this driver is designed to be as far as possible generic and
extendable to other busses that have no native hotplug and model ID
discovery.

This driver does not itself add and remove the bridges or panel on the
add-on: this needs to be done by other means, e.g. device tree overlay
runtime insertion and removal. The hotplug-bridge gets notified by the DRM
bridge core after a removable bridge gets added or before it is removed.

The hotplug-bridge role is to implement the "hot-pluggable connector" in
the bridge chain. In this position, what the hotplug-bridge should ideally
do is:

 * communicate with the previous component (bridge or encoder) so that it
   believes it always has a connected bridge following it and the DRM card
   is always present
 * be notified of the addition and removal of the following bridge and
   attach/detach to/from it
 * communicate with the following bridge so that it will attach and detach
   using the normal procedure (as if the entire pipeline were being created
   or destroyed, not only the tail)
 * instantiate two DRM connectors (similarly to what the DisplayPort MST
   code does):
   - a DSI connector representing the video lines of the hotplug connector;
     the status is always "disconnected" (no panel is ever attached
     directly to it)
   - an LSVD connector representing the classic connection to the panel;
     this gets added/removed whenever the add-on gets
     connected/disconnected; the status is always "connected" as the panel
     is always connected to the preceding bridge

However some aspects make it a bit more complex than that. Most notably:

 * the next bridge can be probed and removed at any moment and all probing
   sequences need to be handled
 * the DSI host/device registration process, which adds to the DRM bridge
   attach process, makes the initial card registration tricky
 * the need to register and deregister the following bridges at runtime
   without tearing down the whole DRM card prevents using some of the
   functions that are normally recommended
 * the automatic mechanism to call the appropriate .get_modes operation
   (typically provided by the panel bridge) cannot work as the panel can
   disappear and reappear as a different model, so an ad-hoc lookup is
   needed

The code handling these and other tricky aspects is accurately documented
by comments in the code.

The userspace representation when the add-on is not connected is:

  # modetest -c  | grep -i '^[a-z0-9]'
  Connectors:
  id    encoder status          name        size (mm)     modes   encoders
  38    0       disconnected    DSI-1       0x0           0       37

And when it is connected a new connector appears:

  # modetest -c  | grep -i '^[a-z0-9]'
  Connectors:
  id    encoder status          name        size (mm)     modes   encoders
  38    0       disconnected    DSI-1       0x0           0       37
  39    0       connected       LVDS-1      344x194       1       37

Co-developed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changed in v4:
 - convert from generic notifiers to the new bridge_event_notify bridge
   func
 - improved and updated commit message, adding 'modetest' output
   with/without add-on
 - select DRM_DISPLAY_HELPER and DRM_BRIDGE_CONNECTOR, required after
   commit 9da7ec9b19d8 ("drm/bridge-connector: move to DRM_DISPLAY_HELPER
   module")

Changed in v3:
 - dynamically add/remove the LVDS connector on hot(un)plug
 - take the firmware node normally via dev->of_node instead of using
   device_set_node(); this makes code more self-contained and generic
 - minor rewordings and cleanups

Changed in v2:
 - change to be a platform device instantiated from the connector driver
   instead of a self-standing OF driver
 - add missing error handling for devm_drm_bridge_add()
 - various cleanups and style improvements
---
 MAINTAINERS                             |   5 +
 drivers/gpu/drm/bridge/Kconfig          |  17 +
 drivers/gpu/drm/bridge/Makefile         |   1 +
 drivers/gpu/drm/bridge/hotplug-bridge.c | 661 ++++++++++++++++++++++++++++++++
 4 files changed, 684 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index b939284d1350..95a05eb75202 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7010,6 +7010,11 @@ T:	git https://gitlab.freedesktop.org/drm/misc/kernel.git
 F:	Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
 F:	drivers/gpu/drm/panel/panel-himax-hx8394.c
 
+DRM DRIVER FOR HOTPLUG VIDEO CONNECTOR BRIDGE
+M:	Luca Ceresoli <luca.ceresoli@bootlin.com>
+S:	Maintained
+F:	drivers/gpu/drm/bridge/hotplug-bridge.c
+
 DRM DRIVER FOR HX8357D PANELS
 S:	Orphan
 T:	git https://gitlab.freedesktop.org/drm/misc/kernel.git
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 3eb955333c80..95ad3dec9d97 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -90,6 +90,23 @@ config DRM_FSL_LDB
 	help
 	  Support for i.MX8MP DPI-to-LVDS on-SoC encoder.
 
+config DRM_HOTPLUG_BRIDGE
+	tristate "Hotplug DRM bridge support"
+	depends on OF
+	select DRM_PANEL_BRIDGE
+	select DRM_MIPI_DSI
+	select DRM_KMS_HELPER
+	select DRM_DISPLAY_HELPER
+	select DRM_BRIDGE_CONNECTOR
+	help
+	  Driver for a DRM bridge representing a physical connector that
+	  splits a DRM pipeline into a fixed part and a physically
+	  removable part. The fixed part includes up to the encoder and
+	  zero or more bridges. The removable part includes any following
+	  bridges up to the connector and panel and can be physically
+	  removed and connected at runtime, possibly with different
+	  components.
+
 config DRM_ITE_IT6505
 	tristate "ITE IT6505 DisplayPort bridge"
 	depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 7df87b582dca..58f59e5a543a 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_DRM_CHRONTEL_CH7033) += chrontel-ch7033.o
 obj-$(CONFIG_DRM_CROS_EC_ANX7688) += cros-ec-anx7688.o
 obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o
 obj-$(CONFIG_DRM_FSL_LDB) += fsl-ldb.o
+obj-$(CONFIG_DRM_HOTPLUG_BRIDGE) += hotplug-bridge.o
 obj-$(CONFIG_DRM_ITE_IT6505) += ite-it6505.o
 obj-$(CONFIG_DRM_LONTIUM_LT8912B) += lontium-lt8912b.o
 obj-$(CONFIG_DRM_LONTIUM_LT9211) += lontium-lt9211.o
diff --git a/drivers/gpu/drm/bridge/hotplug-bridge.c b/drivers/gpu/drm/bridge/hotplug-bridge.c
new file mode 100644
index 000000000000..e4073a23e919
--- /dev/null
+++ b/drivers/gpu/drm/bridge/hotplug-bridge.c
@@ -0,0 +1,661 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * A DRM bridge representing the split point between a fixed part of the
+ * DRM pipeline and a physically removable part. The fixed part includes up
+ * to the encoder and zero or more bridges. Insertion and removal of the
+ * "downstream" components happens via device driver probe/removal.
+ *
+ * Copyright (C) 2024, GE HealthCare
+ *
+ * Authors:
+ * Luca Ceresoli <luca.ceresoli@bootlin.com>
+ * Paul Kocialkowski <paul.kocialkowski@bootlin.com>
+ */
+
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_bridge_connector.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_of.h>
+#include <drm/drm_probe_helper.h>
+
+/*
+ * Internal hotplug-bridge data.
+ *
+ * We have two 'struct drm_connector' here:
+ * - fixconn represents the DSI video lines on the hotplug connector where
+ *   the removable part attaches. It is thus always instantiated.
+ * - dynconn represents the LVDS videl lines where the panel is attached.
+ *   It is part of the removable part of the video pipeline and as such is
+ *   added and removed dynamically based on when the downstream devices
+ *   appear and disappear.
+ */
+struct hotplug_bridge {
+	struct device *dev;
+
+	/* Local bridge */
+	struct drm_bridge bridge;
+
+	/* Always-present connector (where the removal part will connect to) */
+	struct drm_connector *fixconn;
+
+	/* Downstream bridge (next in the chain) */
+	struct drm_bridge *next_bridge;
+	struct mutex next_bridge_mutex;
+
+	/* Pointer to the last bridge exposing OP_MODES */
+	struct drm_bridge *bridge_modes;
+
+	/* The "tail" connector that gets added/removed at runtime */
+	struct drm_connector dynconn;
+
+	/* Local DSI host, for the downstream DSI device to attach to */
+	struct mipi_dsi_host dsi_host;
+	/* Local DSI device, attached to the upstream DSI host */
+	struct mipi_dsi_device *dsi_dev;
+	/* Upstream DSI host (the actual DSI controller) */
+	struct mipi_dsi_host *prev_dsi_host;
+
+	struct work_struct hpd_work;
+};
+
+static struct hotplug_bridge *hotplug_bridge_from_drm_bridge(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct hotplug_bridge, bridge);
+}
+
+/* --------------------------------------------------------------------------
+ * dynconn implementation
+ */
+static struct hotplug_bridge *hotplug_bridge_from_dynconn(struct drm_connector *conn)
+{
+	return container_of(conn, struct hotplug_bridge, dynconn);
+}
+
+static int hotplug_bridge_dynconn_get_modes(struct drm_connector *connector)
+{
+	struct hotplug_bridge *hpb = hotplug_bridge_from_dynconn(connector);
+
+	if (hpb->bridge_modes)
+		return hpb->bridge_modes->funcs->get_modes(hpb->bridge_modes, connector);
+
+	return 0;
+}
+
+static const struct drm_connector_helper_funcs hotplug_bridge_dynconn_connector_helper_funcs = {
+	.get_modes = hotplug_bridge_dynconn_get_modes,
+};
+
+static void hotplug_bridge_dynconn_destroy(struct drm_connector *connector)
+{
+	struct hotplug_bridge *hpb = hotplug_bridge_from_dynconn(connector);
+
+	drm_connector_unregister(&hpb->dynconn);
+	drm_connector_cleanup(&hpb->dynconn);
+	hpb->bridge_modes = NULL;
+}
+
+static const struct drm_connector_funcs dynconn_funcs = {
+	.destroy                    = hotplug_bridge_dynconn_destroy,
+	.atomic_duplicate_state     = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state       = drm_atomic_helper_connector_destroy_state,
+	.fill_modes                 = drm_helper_probe_single_connector_modes,
+};
+
+/*
+ * In non-removable pipelines using a "bridge connector",
+ * drm_bridge_connector_init() stores in the bridge_connector a pointer to
+ * the last bridge having OP_MODES (typically the panel bridge), so the
+ * .get_modes op will automatically be called on that bridge (it also takes
+ * pointers to other bridges which we don't care about). The "bridge
+ * connector" is too restrictive for our use case so we cannot use it. But
+ * we need a pointer to the modes-providing bridge, so we need to replicate
+ * that bit of its logic.
+ */
+static void hotplug_bridge_dynconn_find_bridge(struct hotplug_bridge *hpb)
+{
+	struct drm_bridge *bridge;
+
+	if (WARN_ON(!hpb->next_bridge || !hpb->bridge.encoder))
+		return;
+
+	drm_for_each_bridge_in_chain(hpb->bridge.encoder, bridge)
+		if (bridge->ops & DRM_BRIDGE_OP_MODES)
+			hpb->bridge_modes = bridge;
+}
+
+static int hotplug_bridge_dynconn_add(struct hotplug_bridge *hpb)
+{
+	int err;
+
+	err = drm_connector_init(hpb->bridge.dev, &hpb->dynconn, &dynconn_funcs,
+				 DRM_MODE_CONNECTOR_LVDS);
+	if (err)
+		return err;
+
+	drm_atomic_helper_connector_reset(&hpb->dynconn);
+
+	drm_connector_helper_add(&hpb->dynconn,
+				 &hotplug_bridge_dynconn_connector_helper_funcs);
+
+	drm_connector_attach_encoder(&hpb->dynconn, hpb->bridge.encoder);
+	if (err)
+		goto err_cleanup;
+
+	hotplug_bridge_dynconn_find_bridge(hpb);
+
+	err = drm_connector_register(&hpb->dynconn);
+	if (err)
+		goto err_cleanup;
+
+	return 0;
+
+err_cleanup:
+	drm_connector_cleanup(&hpb->dynconn);
+	hpb->bridge_modes = NULL;
+	return err;
+}
+
+/* ----------------------------------------------------------------------- */
+
+/*
+ * Attach the remote bridge to the encoder and to the next bridge in the
+ * chain, if possible. For this to succeed, we need to know:
+ *
+ * - the encoder, which is set at the first drm_bridge_attach() time
+ * - the next bridge, which is obtained via a notifier whenever the next
+ *   bridge is (re)probed, or at probe time in case it was probed before us
+ *
+ * In order to handle different execution sequences, this function can be
+ * called from multiple places and needs to check all the prerequisites
+ * every time, and it will act only if both are met.
+ *
+ * Must be called with hpb->next_bridge_mutex held.
+ *
+ * Returns 0 if the encoder was attached successfully, -ENODEV if any of
+ * the two prerequisites above is not met (no encoder or no next bridge),
+ * the error returned by drm_bridge_attach() otherwise.
+ */
+static int hotplug_bridge_attach_to_encoder_chain(struct hotplug_bridge *hpb)
+{
+	int ret;
+
+	if (!hpb->next_bridge || !hpb->bridge.encoder)
+		return -ENODEV;
+
+	ret = drm_bridge_attach(hpb->bridge.encoder, hpb->next_bridge, &hpb->bridge,
+				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+	if (ret)
+		return dev_err_probe(hpb->dev, ret, "drm_bridge_attach failed\n");
+
+	dev_dbg(hpb->dev, "attached to encoder chain\n");
+
+	return 0;
+}
+
+/*
+ * Stop the video pipeline and detach next_bridge.
+ *
+ * Must be called with hpb->next_bridge_mutex held.
+ */
+static void hotplug_bridge_detach_from_encoder_chain(struct hotplug_bridge *hpb)
+{
+	WARN_ON_ONCE(!hpb->next_bridge);
+
+	dev_dbg(hpb->dev, "detaching from encoder chain\n");
+
+	drm_atomic_helper_shutdown(hpb->bridge.dev);
+
+	drm_encoder_cleanup_from(hpb->bridge.encoder, hpb->next_bridge);
+}
+
+static void hotplug_bridge_grab(struct hotplug_bridge *hpb)
+{
+	struct device *dev = hpb->dev;
+	struct drm_bridge *bridge;
+	struct drm_panel *panel;
+	int err;
+
+	mutex_lock(&hpb->next_bridge_mutex);
+
+	if (hpb->next_bridge)
+		goto out_unlock;
+
+	/*
+	 * This is supposed to be replaced by devm_drm_of_get_bridge(), but
+	 * that is a devm_, and we need to remove the panel bridge also on
+	 * next_bridge disconnect.
+	 */
+	err = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, &bridge);
+	if (err)
+		goto out_unlock;
+
+	/* Convert the remote panel to a bridge */
+	if (panel)
+		bridge = drm_panel_bridge_add(panel);
+	if (IS_ERR(bridge))
+		goto out_unlock;
+
+	hpb->next_bridge = bridge;
+
+	dev_dbg(dev, "grabbed next bridge (%pOFn)\n", hpb->next_bridge->of_node);
+
+	hpb->bridge.pre_enable_prev_first = hpb->next_bridge->pre_enable_prev_first;
+
+	err = hotplug_bridge_attach_to_encoder_chain(hpb);
+	if (err)
+		goto err_panel_bridge_remove;
+
+	err = hotplug_bridge_dynconn_add(hpb);
+	if (err)
+		goto err_detach_from_encoder_chain;
+
+	queue_work(system_wq, &hpb->hpd_work);
+	goto out_unlock;
+
+err_detach_from_encoder_chain:
+	hotplug_bridge_detach_from_encoder_chain(hpb);
+err_panel_bridge_remove:
+	drm_panel_bridge_remove(hpb->next_bridge);
+	hpb->next_bridge = NULL;
+out_unlock:
+	mutex_unlock(&hpb->next_bridge_mutex);
+}
+
+/*
+ * Detach from the next bridge and remove the panel bridge, either on
+ * release or when the downstream bridge is being removed.
+ *
+ * Can be called in these ways:
+ *
+ * - bridge_being_removed is NULL: detach unconditionally
+ *   (this is useful on .remove() to teardown everything)
+ * - bridge_being_removed == hpb->next_bridge: detach
+ *   (the downstream bridge is being removed)
+ * - bridge_being_removed != hpb->next_bridge: do nothing
+ *   (the bridge being removed is not the downstream bridge)
+ *
+ * In all cases, does nothing when there is no downstream bridge.
+ */
+static void hotplug_bridge_release(struct hotplug_bridge *hpb,
+				   struct drm_bridge *bridge_being_removed)
+{
+	mutex_lock(&hpb->next_bridge_mutex);
+
+	if (!hpb->next_bridge)
+		goto out;
+
+	if (bridge_being_removed && bridge_being_removed != hpb->next_bridge)
+		goto out;
+
+	hotplug_bridge_dynconn_destroy(&hpb->dynconn);
+
+	dev_dbg(hpb->dev, "releasing next bridge (%pOFn)\n", hpb->next_bridge->of_node);
+
+	hotplug_bridge_detach_from_encoder_chain(hpb);
+
+	/*
+	 * This will check that the bridge actually belongs to panel-bridge
+	 * before doing anything with it, so we can safely always call it.
+	 */
+	drm_panel_bridge_remove(hpb->next_bridge);
+	hpb->next_bridge = NULL;
+
+	queue_work(system_wq, &hpb->hpd_work);
+
+out:
+	mutex_unlock(&hpb->next_bridge_mutex);
+}
+
+static void hotplug_bridge_bridge_event_notify(struct drm_bridge *bridge,
+					       enum drm_bridge_event_type event,
+					       struct drm_bridge *event_bridge)
+{
+
+	struct hotplug_bridge *hpb = container_of(bridge, struct hotplug_bridge, bridge);
+
+	switch (event) {
+	case DRM_EVENT_BRIDGE_ADD:
+		hotplug_bridge_grab(hpb);
+		break;
+	case DRM_EVENT_BRIDGE_REMOVE:
+		hotplug_bridge_release(hpb, event_bridge);
+		break;
+	}
+}
+
+static int hotplug_bridge_attach(struct drm_bridge *bridge,
+				 enum drm_bridge_attach_flags flags)
+{
+	struct hotplug_bridge *hpb = hotplug_bridge_from_drm_bridge(bridge);
+	struct device *dev = hpb->dev;
+	struct drm_connector *connector;
+	struct drm_encoder *encoder = hpb->bridge.encoder;
+	int err;
+
+	/* Encoder was not yet provided to our bridge */
+	if (!encoder)
+		return -ENODEV;
+
+	/* Connector was already created */
+	if (hpb->fixconn)
+		return dev_err_probe(dev, -EBUSY, "connector already created\n");
+
+	connector = drm_bridge_connector_init(bridge->dev, encoder);
+	if (IS_ERR(connector))
+		return dev_err_probe(dev, PTR_ERR(connector), "failed to initialize connector\n");
+
+	drm_connector_attach_encoder(connector, encoder);
+
+	hpb->fixconn = connector;
+
+	drm_connector_register(connector);
+
+	mutex_lock(&hpb->next_bridge_mutex);
+	err = hotplug_bridge_attach_to_encoder_chain(hpb);
+	mutex_unlock(&hpb->next_bridge_mutex);
+
+	/* -ENODEV is acceptable, in case next_bridge is not yet known */
+	if (err == -ENODEV)
+		err = 0;
+
+	return err;
+}
+
+static void hotplug_bridge_detach(struct drm_bridge *bridge)
+{
+	struct hotplug_bridge *hpb = hotplug_bridge_from_drm_bridge(bridge);
+
+	mutex_lock(&hpb->next_bridge_mutex);
+	hotplug_bridge_detach_from_encoder_chain(hpb);
+	mutex_unlock(&hpb->next_bridge_mutex);
+
+	if (hpb->fixconn) {
+		drm_connector_unregister(hpb->fixconn);
+		drm_connector_cleanup(hpb->fixconn);
+		hpb->fixconn = NULL;
+	}
+}
+
+/*
+ * The fixed connector is never attached to a panel, so it should always be
+ * reported as disconnected.
+ */
+static enum drm_connector_status hotplug_bridge_detect(struct drm_bridge *bridge)
+{
+	return connector_status_disconnected;
+}
+
+static void hotplug_bridge_hpd_work_func(struct work_struct *work)
+{
+	struct hotplug_bridge *hpb = container_of(work, struct hotplug_bridge, hpd_work);
+
+	if (hpb->bridge.dev)
+		drm_helper_hpd_irq_event(hpb->bridge.dev);
+}
+
+static const struct drm_bridge_funcs hotplug_bridge_funcs = {
+	.attach			= hotplug_bridge_attach,
+	.detach			= hotplug_bridge_detach,
+	.detect			= hotplug_bridge_detect,
+	.bridge_event_notify	= hotplug_bridge_bridge_event_notify,
+};
+
+static int hotplug_bridge_dsi_detach(struct mipi_dsi_host *host,
+				     struct mipi_dsi_device *device_remote)
+{
+	struct hotplug_bridge *hpb = dev_get_drvdata(host->dev);
+
+	if (!hpb->dsi_dev)
+		return -ENODEV;
+
+	mipi_dsi_detach(hpb->dsi_dev);
+	mipi_dsi_device_unregister(hpb->dsi_dev);
+	hpb->dsi_dev = NULL;
+
+	return 0;
+}
+
+/*
+ * Attach the local DSI device to the upstream DSI host, possibly with a
+ * "null" format.
+ *
+ * In "normal" bridges this function should be _only_ used as the .attach
+ * callback of hotplug_bridge_dsi_ops. But "normal" bridges have their
+ * downstream DSI device always connected, which we don't. When booting
+ * without anything connected downstream, our upstream bridge could be not
+ * even calling drm_bridge_add() until we do attach ourselves as a DSI
+ * device, preventing the whole DRM card from being instantiated.
+ *
+ * In order to always have a DRM card after boot, we do call this same
+ * function while probing in order to attach as a DSI device to the DSI
+ * master. However during probe we don't know the bus format yet. It would
+ * be nice to be able to update the format afterwards when a downstream DSI
+ * device is attaching to our local host, but there is no callback for
+ * that. To overcome this limitation, this function can be called in two
+ * ways:
+ *
+ * - during probe, to make the upstream bridge happy, when there is no
+ *   next_dsi_dev yet and thus the lanes/format/etc are unknown
+ * - as the mipi_dsi_host_ops.attach callback proper, as soon as the
+ *   next_dsi_dev is known
+ *
+ * The resulting call sequence is:
+ *
+ * 1. hotplug_bridge_dsi_attach() called by hotplug_bridge_probe() with
+ *    next_dsi_dev == NULL: we attach to the host but with a fake format
+ *    so the DRM card can be populated. hpb->dsi_dev becomes non-NULL.
+ * 2. hotplug_bridge_dsi_attach() called as .attach callback from a
+ *    downstream device when it becomes available: we need to detach in
+ *    order to re-attach with the format of the device. hpb->dsi_dev
+ *    is found non-NULL, then reused so it will be non-NULL again.
+ * 3. hotplug_bridge_dsi_detach() called as the .detach callback by a
+ *    downstream device: cleans up everything normally. hpb->dsi_dev goes
+ *    from non-NULL to NULL.
+ * 4. hotplug_bridge_dsi_attach() called by a downstream device: attaches
+ *    normally to the upstream DSI host. hpb->dsi_dev goes from NULL to
+ *    non-NULL.
+ *
+ * Steps 3 and 4 are the "normal" attach/detach steps as on "normal"
+ * bridges.
+ *
+ * Steps 1 and 2 happen only the first time, steps 3 and 4 will happen
+ * every time the downstream bridge disconnects and reconnects.
+ */
+static int hotplug_bridge_dsi_attach(struct mipi_dsi_host *host,
+				     struct mipi_dsi_device *next_dsi_dev)
+{
+	struct device *dev = host->dev;
+	struct hotplug_bridge *hpb = dev_get_drvdata(dev);
+	struct mipi_dsi_device *dsi_dev;
+	const struct mipi_dsi_device_info dsi_info = {
+		.type = "hotplug-bridge",
+		.channel = 0,
+		.node = NULL,
+	};
+	int err;
+
+	/*
+	 * Step 2 only (first time we are called for an actual device
+	 * attaching): clean up the fake attach done at step 1
+	 */
+	if (hpb->dsi_dev)
+		hotplug_bridge_dsi_detach(&hpb->dsi_host, NULL);
+
+	/* Register a local DSI device with the remote DSI host */
+	dsi_dev = mipi_dsi_device_register_full(hpb->prev_dsi_host,
+						&dsi_info);
+	if (IS_ERR(dsi_dev))
+		return PTR_ERR(dsi_dev);
+
+	/* At step 1 we have no downstream device to get the format from */
+	if (next_dsi_dev) {
+		dsi_dev->channel    = next_dsi_dev->channel;
+		dsi_dev->lanes      = next_dsi_dev->lanes;
+		dsi_dev->format     = next_dsi_dev->format;
+		dsi_dev->mode_flags = next_dsi_dev->mode_flags;
+	}
+
+	/* Attach our local DSI device to the remote DSI host */
+	err = mipi_dsi_attach(dsi_dev);
+	if (err) {
+		mipi_dsi_device_unregister(dsi_dev);
+		return dev_err_probe(dev, err, "failed to attach hotplug dsi device to host\n");
+	}
+
+	hpb->dsi_dev = dsi_dev;
+
+	return 0;
+}
+
+/*
+ * Propagate mipi_dsi_device_transfer() to the upstream DSI host.
+ *
+ * Reimplements identically the minimal needed part of
+ * mipi_dsi_device_transfer(), including the -ENOSYS return value.
+ */
+static ssize_t hotplug_bridge_dsi_transfer(struct mipi_dsi_host *host,
+					   const struct mipi_dsi_msg *msg)
+{
+	struct hotplug_bridge *hpb = dev_get_drvdata(host->dev);
+	const struct mipi_dsi_host_ops *ops;
+
+	if (!hpb->dsi_dev)
+		return -ENODEV;
+
+	ops = hpb->dsi_dev->host->ops;
+
+	if (!ops || !ops->transfer)
+		return -ENOSYS;
+
+	return ops->transfer(hpb->dsi_dev->host, msg);
+}
+
+static const struct mipi_dsi_host_ops hotplug_bridge_dsi_ops = {
+	.attach		= hotplug_bridge_dsi_attach,
+	.detach		= hotplug_bridge_dsi_detach,
+	.transfer	= hotplug_bridge_dsi_transfer,
+};
+
+/*
+ * Find the upstream DSI host and register our downstream-facing DSI host.
+ */
+static int hotplug_bridge_dsi_setup(struct hotplug_bridge *hpb)
+{
+	struct device *dev = hpb->dev;
+	struct device_node *endpoint;
+	struct device_node *node;
+
+	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
+	node = of_graph_get_remote_port_parent(endpoint);
+
+	hpb->prev_dsi_host = of_find_mipi_dsi_host_by_node(node);
+
+	of_node_put(node);
+	of_node_put(endpoint);
+
+	if (!hpb->prev_dsi_host)
+		return -EPROBE_DEFER;
+
+	hpb->dsi_host.dev = dev;
+	hpb->dsi_host.ops = &hotplug_bridge_dsi_ops;
+
+	return mipi_dsi_host_register(&hpb->dsi_host);
+}
+
+static void hotplug_bridge_dsi_cleanup(struct hotplug_bridge *hpb)
+{
+	mipi_dsi_host_unregister(&hpb->dsi_host);
+}
+
+static int hotplug_bridge_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct hotplug_bridge *hpb;
+	struct drm_bridge *bridge;
+	int err;
+
+	hpb = devm_kzalloc(dev, sizeof(*hpb), GFP_KERNEL);
+	if (!hpb)
+		return -ENOMEM;
+
+	hpb->dev = dev;
+
+	mutex_init(&hpb->next_bridge_mutex);
+	INIT_WORK(&hpb->hpd_work, hotplug_bridge_hpd_work_func);
+
+	err = hotplug_bridge_dsi_setup(hpb);
+	if (err)
+		return dev_err_probe(dev, err, "failed to setup DSI\n");
+
+	bridge = &hpb->bridge;
+	bridge->of_node = dev->of_node;
+	bridge->funcs = &hotplug_bridge_funcs;
+	bridge->type = DRM_MODE_CONNECTOR_DSI;
+	bridge->ops |= DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_HPD;
+
+	platform_set_drvdata(pdev, hpb);
+
+	err = devm_drm_bridge_add(dev, bridge);
+	if (err) {
+		dev_err_probe(dev, err, "failed adding bridge\n");
+		goto err_dsi_cleanup;
+	}
+
+	err = hotplug_bridge_dsi_attach(&hpb->dsi_host, NULL);
+	if (err) {
+		dev_err_probe(dev, err, "failed first attach to upstream DSI host\n");
+		goto err_dsi_cleanup;
+	}
+
+	/*
+	 * Since devm_drm_bridge_add() we can be notified of any bridges
+	 * appearing, but also check now, in case the next bridge was
+	 * probed earlier
+	 */
+	hotplug_bridge_grab(hpb);
+
+	return 0;
+
+err_dsi_cleanup:
+	hotplug_bridge_dsi_cleanup(hpb);
+	return err;
+}
+
+static void hotplug_bridge_remove(struct platform_device *pdev)
+{
+	struct hotplug_bridge *hpb = platform_get_drvdata(pdev);
+
+	cancel_work_sync(&hpb->hpd_work);
+
+	hotplug_bridge_release(hpb, NULL);
+
+	hotplug_bridge_dsi_cleanup(hpb);
+}
+
+static const struct platform_device_id hotplug_bridge_platform_ids[] = {
+	{ .name = "hotplug-dsi-bridge" },
+	{},
+};
+MODULE_DEVICE_TABLE(platform, hotplug_bridge_platform_ids);
+
+static struct platform_driver hotplug_bridge_driver = {
+	.probe		= hotplug_bridge_probe,
+	.remove_new	= hotplug_bridge_remove,
+	.id_table	= hotplug_bridge_platform_ids,
+	.driver		= {
+		.name		= "hotplug-drm-bridge",
+	},
+};
+
+module_platform_driver(hotplug_bridge_driver);
+
+MODULE_AUTHOR("Luca Ceresoli <luca.ceresoli@bootlin.com>");
+MODULE_AUTHOR("Paul Kocialkowski <paul.kocialkowski@bootlin.com>");
+MODULE_DESCRIPTION("Hotplug DRM Bridge");
+MODULE_LICENSE("GPL");

-- 
2.34.1

[PATCH v4 5/8] i2c: i2c-core-of: follow i2c-parent phandle to probe devices from added nodes

[Luca Ceresoli]

When device tree nodes are added, the I2C core tries to probe client
devices based on the classic DT structure:

  i2c@abcd0000 {
      some-client@42 { compatible = "xyz,blah"; ... };
  };

However for hotplug connectors described via device tree overlays there is
additional level of indirection, which is needed to decouple the overlay
and the base tree:

  --- base device tree ---

  i2c1: i2c@abcd0000 { compatible = "xyz,i2c-ctrl"; ... };
  i2c5: i2c@cafe0000 { compatible = "xyz,i2c-ctrl"; ... };

  connector {
      i2c-ctrl {
          i2c-parent = <&i2c1>;
          #address-cells = <1>;
          #size-cells = <0>;
      };

      i2c-sensors {
          i2c-parent = <&i2c5>;
          #address-cells = <1>;
          #size-cells = <0>;
      };
  };

  --- device tree overlay ---

  ...
  // This node will overlay on the i2c-ctrl node of the base tree
  i2c-ctrl {
      eeprom@50 { compatible = "atmel,24c64"; ... };
  };
  ...

  --- resulting device tree ---

  i2c1: i2c@abcd0000 { compatible = "xyz,i2c-ctrl"; ... };
  i2c5: i2c@cafe0000 { compatible = "xyz,i2c-ctrl"; ... };

  connector {
      i2c-ctrl {
          i2c-parent = <&i2c1>;
          #address-cells = <1>;
          #size-cells = <0>;

          eeprom@50 { compatible = "atmel,24c64"; ... };
      };

      i2c-sensors {
          i2c-parent = <&i2c5>;
          #address-cells = <1>;
          #size-cells = <0>;
      };
  };

Here i2c-ctrl (same goes for i2c-sensors) represent the part of I2C bus
that is on the hot-pluggable add-on. On hot-plugging it will physically
connect to the I2C adapter on the base board. Let's call the 'i2c-ctrl'
node an "extension node".

In order to decouple the overlay from the base tree, the I2C adapter
(i2c@abcd0000) and the extension node (i2c-ctrl) are separate
nodes. Rightfully, only the former will probe into an I2C adapter, and it
will do that perhaps during boot, long before overlay insertion.

The extension node won't probe into an I2C adapter or any other device or
bus, so its subnodes ('eeprom@50') won't be interpreted as I2C clients by
current I2C core code. However it has an 'i2c-parent' phandle to point to
the corresponding I2C adapter node. This tells those nodes are I2C clients
of the adapter in that other node.

Extend the i2c-core-of code to look for the adapter via the 'i2c-parent'
phandle when the regular adapter lookup does not find one. This allows all
clients to be probed: both those on the base board (described in the base
device tree) and those on the add-on and described by an overlay.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Note: while this patch works for normal hotplug and unplug, it has some
weaknesses too, due to the implementation being in a OF change
notifier. Two cases come to mind:

 1. In the base device tree there must be _no_ nodes under the "extension
    node" (i2c-ctrl), or they won't be picked up as they are not
    dynamically added.

 2. In case the I2C adapter is unbound and rebound, or it probes after
    overlay insertion, it will miss the OF notifier events and so it won't
    find the devices in the extension node.

The first case is not a limiting factor: fixed I2C devices should just stay
under the good old I2C adapter node.

The second case is a limiting factor, even though not happening in "normal"
use cases. I cannot see any solution without making the adapter aware of
the "bus extensions" it has, so on its probe it can always go look for any
devices there. Taking into account the case of multiple connectors each
having an extension of the same bus, this may look as follows in device
tree:

  --- base device tree ---

  i2c1: i2c@abcd0000 {
      compatible = "xyz,i2c-ctrl"; ...
      i2c-bus-extensions = <&i2c_ctrl_conn0, &i2c_ctrl_conn1>;
  };

  connector@0 {
      i2c_ctrl_conn0: i2c-ctrl {
          i2c-parent = <&i2c1>;
          #address-cells = <1>;
          #size-cells = <0>;
      };
  };

  connector@1 {
      i2c_ctrl_conn1: i2c-ctrl {
          i2c-parent = <&i2c1>;
          #address-cells = <1>;
          #size-cells = <0>;
      };
  };

I'd love to have some feedback and opinions about the basic idea before
digging into the details of this additional step.

---

Changes in v4:
 - fix a typo in commit message

This patch first appeared in v3.
---
 drivers/i2c/i2c-core-of.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index a6c407d36800..71c559539a13 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -170,6 +170,15 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
 	switch (of_reconfig_get_state_change(action, rd)) {
 	case OF_RECONFIG_CHANGE_ADD:
 		adap = of_find_i2c_adapter_by_node(rd->dn->parent);
+		if (adap == NULL) {
+			struct device_node *i2c_bus;
+
+			i2c_bus = of_parse_phandle(rd->dn->parent, "i2c-parent", 0);
+			if (i2c_bus) {
+				adap = of_find_i2c_adapter_by_node(i2c_bus);
+				of_node_put(i2c_bus);
+			}
+		}
 		if (adap == NULL)
 			return NOTIFY_OK;	/* not for us */
 

-- 
2.34.1

[PATCH v4 6/8] backlight: led-backlight: add devlink to supplier LEDs

[Luca Ceresoli]

led-backlight is a consumer of one or multiple LED class devices, but no
devlink is created for such supplier-producer relationship. One consequence
is that removal ordered is not correctly enforced.

Issues happen for example with the following sections in a device tree
overlay:

    // An LED driver chip
    pca9632@62 {
        compatible = "nxp,pca9632";
        reg = <0x62>;

	// ...

        addon_led_pwm: led-pwm@3 {
            reg = <3>;
            label = "addon:led:pwm";
        };
    };

    backlight-addon {
        compatible = "led-backlight";
        leds = <&addon_led_pwm>;
        brightness-levels = <255>;
        default-brightness-level = <255>;
    };

On removal of the above overlay, the LED driver can be removed before the
backlight device, resulting in:

    Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
    ...
    Call trace:
     led_put+0xe0/0x140
     devm_led_release+0x6c/0x98

Fix by adding a devlink between the consuming led-backlight device and the
supplying LED device.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

This patch first appeared in v4.
---
 drivers/video/backlight/led_bl.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
index c7aefcd6e4e3..bfbd80728036 100644
--- a/drivers/video/backlight/led_bl.c
+++ b/drivers/video/backlight/led_bl.c
@@ -209,6 +209,19 @@ static int led_bl_probe(struct platform_device *pdev)
 		return PTR_ERR(priv->bl_dev);
 	}
 
+	for (i = 0; i < priv->nb_leds; i++) {
+		struct device_link *link;
+
+		link = device_link_add(&pdev->dev, priv->leds[0]->dev->parent,
+				       DL_FLAG_AUTOREMOVE_CONSUMER);
+		if (!link) {
+			dev_err(&pdev->dev, "Failed to add devlink (consumer %s, supplier %s)\n",
+				dev_name(&pdev->dev), dev_name(priv->leds[0]->dev->parent));
+			backlight_device_unregister(priv->bl_dev);
+			return -EINVAL;
+		}
+	}
+
 	for (i = 0; i < priv->nb_leds; i++) {
 		mutex_lock(&priv->leds[i]->led_access);
 		led_sysfs_disable(priv->leds[i]);

-- 
2.34.1

[PATCH RFC v4 7/8] driver core: devlink: do not unblock consumers without any drivers found

[Luca Ceresoli]

Quick summary:

I have investigated a problem for a long time, I have a pretty good
understanding, I tried various fixes but none except this is working. The
goal of this patch is to discuss the problem to converge to the best
solution.

---------------------------------
Symptoms
---------------------------------

The problem appeared while testing the v3 addon connector driver that is
part of this series, and which is based on device tree overlays.

Note the symptom happens often, but not always. Changes to logging is a
typical way to make it appear/disappear, so it appears as time sensitive.

The relevant DT overlay snippet is:

/ {
    fragment@0 {
        target-path = "";

        __overlay__ {
            devices {
	        // nodes in here are populated as platform devices
                reg_addon_3v3_lcd: regulator-addon-3v3-lcd {
                    compatible = "regulator-fixed";
                    regulator-name = "3V3_LCD_ADDON";
                    gpios = <...>;
                };

                addon_panel_dsi_lvds: panel-dsi-lvds {
                    compatible = "...";
                    power-supply = <&reg_addon_3v3_lcd>;
                };
            };
        };
    };
};

So the regulator is a supplier to the panel. Nothing special here, except
we are in an overlay.

The overlay gets applied and all devices work correctly. Troubles start
appearing in the form of two messages on overlay removal, in this order:

 * WARNING: CPU: 1 PID: 189 at drivers/regulator/core.c:5856 regulator_unregister+0x1ec/0x208

   This is issued during removal of the 3V3_LCD_ADDON regulator because
   rdev->open_count is 1, while it should be 0. This is because the panel
   still hasn't closed the regulator.

 * Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
   Call trace:
    regulator_lock_recursive+0x5c/0x200
    regulator_lock_dependent+0xc0/0x140
    regulator_enable+0x44/0x98
    panel_simple_resume+0x38/0x108 [panel_simple]
    pm_generic_runtime_resume+0x34/0x58
    __rpm_callback+0x50/0x1f0
    rpm_callback+0x70/0x88
    rpm_resume+0x49c/0x678
    __pm_runtime_resume+0x54/0xa0
    device_release_driver_internal+0xd4/0x240
    device_release_driver+0x20/0x38
    bus_remove_device+0xd4/0x120
    device_del+0x154/0x388

   This happens while the panel driver is being removed and the devm infra
   tries to close the regulator which is already gone.

Both errors have the same origin: the regulator driver is removed before
the panel driver.

---------------------------------
Problem analysis
---------------------------------

My analysis showed that the problem originates from devlink manipulation
during overlay insertion, but shows its effects on removal. This is the
sequence of events:

 * During overlay insertion:
   1. the devlink code creates a devlink (A) with:
        supplier = regulator-addon-3v3-lcd
        consumer = panel-dsi-lvds
        flags    = DL_FLAG_INFERRED (+ possibly others) because it has
	           been inferred from firmware data
   2. soon after, devlink A is relaxed and then dropped
      - does not happen always, based on timing
      - see below for details
   3. the regulator-addon-3v3-lcd regulator gets probed as a platform device
      - the probe function for regulator-addon-3v3-lcd is
        reg_fixed_voltage_probe(), which calls devm_regulator_register() to
        register a single new regulator class device for the voltage output
        -  regulator_register() does, among others:
          - instantiate a new regulator class device (3V3_LCD_ADDON), with
            parent = regulator-addon-3v3-lcd
          - adds a devlink (B)  with:
              supplier = 3V3_LCD_ADDON
              consumer = panel-dsi-lvds

At this point we have these devices and devlinks:

 .---------------------------.
 |  regulator-addon-3v3-lcd  |
 | regulator platform device | supplier                consumer
 |      (struct device)      |<--------- devlink A -------------.
 '---------------------------'          (inferred)              |
     ^                                                          V
     |                                                  .-----------------.
     | parent                                           | panel-dsi-lvds  |
     |                                                  | (struct device) |
     |                                                  '-----------------'
 .---------------------------.                                  ^
 |       3V3_LCD_ADDON       | supplier                consumer |
 |   regulator class device  |<--------- devlink B -------------'
 |      (struct device)      |          (created by
 '---------------------------'        regulator core)

Depending on whether step 2 happens or not, devlink A will be still present
or not during overlay removal.

When step 2 happens and devlink A gets dropped (which happens to me almost
always), the removal code calls:

-> device_release_driver(dev = regulator-addon-3v3-lcd)
   -> device_release_driver_internal()
      -> __device_release_driver()
         -> if (device_links_busy()) // see below
	     {
	        device_links_unbind_consumers(dev = regulator-addon-3v3-lcd)
		 -> for each consumer for which 'dev' is a supplier:
		    {
		       device_release_driver_internal(dev = panel-dsi-lvds)
                    }
             }

The logic is pretty clear: before removing a device that is a supplier to
other devices (regulator-addon-3v3-lcd), use devlink to find all consumers
(panel-dsi-lvds) and remove them first, recursively.

However in case devlink A had been initially dropped, there is no devlink
between the two devices. The regulator removal will just proceed, and the
regulator device gets removed before its consumer.

Note devlink B is not at all examined within this removal
phase. device_links_busy() looks at the platform device
(regulator-addon-3v3-lcd), and it has no way to know about the class device
(3V3_LCD_ADDON).

Assuming the whole device_links_busy() / device_links_unbind_consumers()
logic is correct, let's move to why devlink A gets dropped.

---------------------------------
Why the devlink is dropped
---------------------------------

It all starts in the device_add() for regulator-addon-3v3-lcd:

  /*
   * If all driver registration is done and a newly added device doesn't
   * match with any driver, don't block its consumers from probing in
   * case the consumer device is able to operate without this supplier.
   */
   if (dev->fwnode && fw_devlink_drv_reg_done && !dev->can_match)
       fw_devlink_unblock_consumers(dev);

The three conditions in the if() mean:

 1. this device comes from firmware -> always true in my case (device tree)
 2. this global flag is set via the deferred_probe_timeout_work as soon as
    for 10 consecutive seconds there is no new driver being probed; it is
    never cleared
 3. no driver has been matched with this device so far (IOW the probe
    function of the driver for this device has never been called,
    regardless of the return value)

If all condtions apply, fw_devlink_unblock_consumers() will (after some
checks) call fw_devlink_relax_link() on every link to consumers and "relax"
it. Relaxing means setting link flags to DL_FLAG_MANAGED |
FW_DEVLINK_FLAGS_PERMISSIVE. Soon later, device_links_driver_bound() will
take devlinks with these flags and drop them.

I was unable to understand in full detail the flag manipulation logic
happening in the devlink code. However I think the high-level logic here
can be expressed as: if a devlink was inferred from firmware and its
supplier device did not probe after a while (*) because no potential driver
was found, then maybe that devlink was wrong or it is enforcing a supplier
that is optional for the consumer: let's drop the link and see whether the
(formerly devlink consumer) device can now probe.

(*) "after a while" is implemented by the fw_devlink_drv_reg_done flag,
     which typically gets set way less than a minute after boot

    Basically the fw_devlink_drv_reg_done flag splits the probing in two
    phases. In phase 1 we try to probe all inferred suppliers before
    probing consumers. Then we set fw_devlink_drv_reg_done and relax+drop
    the "dangling" inferred devlinks. Then in phase 2 we try to probe
    without inferred devlinks. This is to see if we can probe more devices
    due to incorrectly inferred devlinks or missing drivers for optional
    suppliers.

Overlays however can be loaded at any time, even a long time after
booting. This is totally normal when used for a hotplug connector, where
the devices get physically connected by the user. This implies the
fw_devlink_drv_reg_done flag is found already set when probing overlay
devices. And so, conditions 1 and 2 above are always set in the overlay
case.

So we are left with condition 3 only. Again I haven't done a full analysis
here, but it is perfectly fine that a driver is not immediately present
when adding a new device. It can just have not yet been matched, possibly
because a driver module is in process of being loaded from storage.

I think there is a race here: on one side the driver becoming available and
matched to the device to be probed, on the other side the
fw_devlink_unblock_consumers() logic to relax and drop inferred
devlinks. If the device probes first, the link won't be dropped.

---------------------------------
Same problem without DT overlays?
---------------------------------

Based on the above, I suspect the exact same problem exists even without
any overlay usage. Indeed, the conditions might exist in other corner
cases. One example is a device whose driver is a module and is not loaded
during boot: e.g. it is not on the root filesystem, it is being developed
and the programmer sends the driver via SSH to a tmpfs to load and test it.

As said, this is a matter of corner cases, but still possible.

Note that no problem should happen to natively removable devices such as
USB, because condition 1 defuses the whole if() above for devices not
described in firmware.

---------------------------------
Fixes I have tried (not working)
---------------------------------

I tried a couple approaches based on devlink to fix this issue.

One was augmenting the regulator core code to add a new devlink between the
regulator platform device (regulator-addon-3v3-lcd) and the regulator class
device (3V3_LCD_ADDON), to create a chain for device_links_busy() to
follow. The devlink is created and persists until removal time. However it
does not enforce the correct ordering: device_links_busy() ignores it
because the link status is always "dormant". The reason appears to be that
the "regulator output device" is a struct device but never has a
driver. Recently Saravana pointed out that:

> device links don't work correctly for "class" type devices
(https://lore.kernel.org/all/CAGETcx-ioF=jTbyQMeD2fsYKz8q5vw_TWYWS9m8H5=pCo5KFYA@mail.gmail.com/)

which is possibly related.

I tried a variant: change the devlink already created by _regulator_get()
to use the regulator platform device (regulator-addon-3v3-lcd) instead of
the regulator class device (3V3_LCD_ADDON) as the supplier. That would make
devlink B have the same endpoints as devlink A. However this did not work
due to the link state staying "not tracked" and thus again being ignored by
device_links_busy(). I haven't managed to find out the flag manipulations
that would make it work.

---------------------------------
Conclusions
---------------------------------

The current logic is clearly OK for "typical" current use cases (not
counting corner cases), but unsuitable for hotplugged devices described by
firmware.

The question is: do we have an underlying assumption that was valid so far
but is wrong when overlays are added?

One possible answer is: dropping inferred devlinks is wrong. Generally
speaking, inferred devlinks are sometimes useless but don't hurt, so there
is no need to drop them. This is what this patch changes.

However I realize there is a use case for dropping inferred devlink:
optional suppliers that prevent consumer probing until they are
dropped. Indeed, inferring devlinks from firmware data can create links
that prevent some device to probe. For this reason my first attempts have
been to add or change the devlinks that subsystem code creates.

So a more sophisticated idea is that after phase 1 we try to probe all
not-probed-yet consumers ignoring the relaxed devlinks, instead of removing
them. This would allow the same amount of devices to be probed using the
same amount of optional suppliers, but leaving the inferred devlinks in
place because they might be useful later on.

And then of course there are the above solutions I failed to get working,
which might be the right way but need some directions for me to have them
working.

I am very open to more answers and suggestions.

Best regards,
Luca

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changes in v4:
 - fix typos and update device tree names in commit message

This patch first appeared in v3.
---
 drivers/base/core.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 82cc4069a24a..32413dd330b1 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1876,19 +1876,6 @@ void __init wait_for_init_devices_probe(void)
 	fw_devlink_best_effort = false;
 }
 
-static void fw_devlink_unblock_consumers(struct device *dev)
-{
-	struct device_link *link;
-
-	if (!fw_devlink_flags || fw_devlink_is_permissive())
-		return;
-
-	device_links_write_lock();
-	list_for_each_entry(link, &dev->links.consumers, s_node)
-		fw_devlink_relax_link(link);
-	device_links_write_unlock();
-}
-
 #define get_dev_from_fwnode(fwnode)	get_device((fwnode)->dev)
 
 static bool fwnode_init_without_drv(struct fwnode_handle *fwnode)
@@ -3682,14 +3669,6 @@ int device_add(struct device *dev)
 
 	bus_probe_device(dev);
 
-	/*
-	 * If all driver registration is done and a newly added device doesn't
-	 * match with any driver, don't block its consumers from probing in
-	 * case the consumer device is able to operate without this supplier.
-	 */
-	if (dev->fwnode && fw_devlink_drv_reg_done && !dev->can_match)
-		fw_devlink_unblock_consumers(dev);
-
 	if (parent)
 		klist_add_tail(&dev->p->knode_parent,
 			       &parent->p->klist_children);

-- 
2.34.1

[PATCH v4 8/8] misc: add ge-addon-connector driver

[Luca Ceresoli]

Add a driver to support the runtime hot-pluggable add-on connector on the
GE SUNH device. This connector allows connecting and disconnecting an
add-on to/from the main device to augment its features. Connection and
disconnection can happen at runtime at any moment without notice.

Different add-on models can be connected, and each has an EEPROM with a
model identifier at a fixed address.

The add-on hardware is added and removed using device tree overlay loading
and unloading.

Co-developed-by: Herve Codina <herve.codina@bootlin.com>
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changed in v4:
 - rename 'nobus-devices' to 'devices' as per new bindings
 - remove the 'powergood' GPIO, removed from hardware design
 - rename "base" and "addon" overlays to "overlay 0" and "overlay 1" as the
   "addon" name is ambiguous
 - remove unused enum sunh_conn_overlay_level entries, then move the only
   remaining enum value to a define

Changed in v3:
 - update to the new overlay representation that now does not include the
   target node; instead the target node is the connector node itself and is
   now passed by the connector driver to of_overlay_fdt_apply(), so the
   overlay is now decoupled from the base device tree
 - update to explicitely probe devices not reachable by the CPU on any
   physical bus (which probe as platform devices) which are now inside a
   'nobus-devices' subnode of the connector node
 - change how the DRM bridge is populated to use the new device tree
   representation, having the video ports inside the 'dsi' node

   **NOTE** this specific change opens up a question about the
            .of_node_reused flag: setting it to true might be wrong now as
            the bridge will be handed the 'dsi' subnode of the connector
            node; however not setting it to true prevents the hotplug
            bridge module autoloading due to the alias string changing from
            "platform:hotplug-dsi-bridge" to "of:NdsiT(null)".

 - remove dev_info() and uninformative dev_dbg() calls
 - Kconfig: use 'depends on' instead of 'select'
 - Kconfig: improve help text and add module name

This patch first appeared in v2.
---
 MAINTAINERS                      |   1 +
 drivers/misc/Kconfig             |  18 ++
 drivers/misc/Makefile            |   1 +
 drivers/misc/ge-sunh-connector.c | 481 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 501 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 95a05eb75202..7dec59888df2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10278,6 +10278,7 @@ F:	drivers/iio/pressure/mprls0025pa*
 HOTPLUG CONNECTOR FOR GE SUNH ADDONS
 M:	Luca Ceresoli <luca.ceresoli@bootlin.com>
 S:	Maintained
+F:	drivers/misc/ge-sunh-connector.c
 F:	Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml
 
 HP BIOSCFG DRIVER
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 41c54051347a..e2a5bcce0073 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -600,6 +600,24 @@ config MARVELL_CN10K_DPI
 	  To compile this driver as a module, choose M here: the module
 	  will be called mrvl_cn10k_dpi.
 
+config GE_SUNH_CONNECTOR
+	tristate "GE SUNH hotplug add-on connector"
+	depends on OF_OVERLAY
+	depends on NVMEM
+	depends on DRM_HOTPLUG_BRIDGE
+	select FW_LOADER
+	help
+	  Driver for the runtime hot-pluggable add-on connector on the GE
+	  SUNH device. This connector allows connecting an add-on to the
+	  main device to augment its features, and to later disconnect
+	  it. Connection and disconnection can be done at runtime at any
+	  moment without notice. Different add-on models can be connected,
+	  and each has an EEPROM with a model identifier at a fixed
+	  address.
+
+	  To compile this driver as a module, choose M here.
+	  The module will be called ge-sunh-connector.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index c2f990862d2b..69747b048046 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -70,4 +70,5 @@ obj-$(CONFIG_TPS6594_ESM)	+= tps6594-esm.o
 obj-$(CONFIG_TPS6594_PFSM)	+= tps6594-pfsm.o
 obj-$(CONFIG_NSM)		+= nsm.o
 obj-$(CONFIG_MARVELL_CN10K_DPI)	+= mrvl_cn10k_dpi.o
+obj-$(CONFIG_GE_SUNH_CONNECTOR)	+= ge-sunh-connector.o
 obj-y				+= keba/
diff --git a/drivers/misc/ge-sunh-connector.c b/drivers/misc/ge-sunh-connector.c
new file mode 100644
index 000000000000..0e6064a9a519
--- /dev/null
+++ b/drivers/misc/ge-sunh-connector.c
@@ -0,0 +1,481 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * GE SUNH hotplug add-on connector
+ *
+ * Driver for the runtime hot-pluggable add-on connector on the GE SUNH
+ * device. Add-on connection is detected via GPIOs (+ a debugfs
+ * trigger). On connection, a "base" DT overlay is added that describes
+ * enough to reach the NVMEM cell with the model ID. Based on the ID, an
+ * add-on-specific overlay is loaded on top to describe everything else.
+ *
+ * Copyright (C) 2024, GE HealthCare
+ *
+ * Authors:
+ * Luca Ceresoli <luca.ceresoli@bootlin.com>
+ * Herve Codina <herve.codina@bootlin.com>
+ */
+
+#include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/firmware.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/workqueue.h>
+
+#define SUNH_CONN_OVERLAY_N_LEVELS       2
+
+struct sunh_conn {
+	struct device *dev;
+	struct gpio_desc *reset_gpio;
+	struct gpio_desc *plugged_gpio;
+
+	bool plugged;
+	int ovcs_id[SUNH_CONN_OVERLAY_N_LEVELS];
+	struct mutex ovl_mutex; // serialize overlay code
+	struct notifier_block nvmem_nb;
+	struct work_struct nvmem_notifier_work;
+
+	struct platform_device *hpb_pdev;
+	struct dentry *debugfs_root;
+};
+
+/*
+ * Populate all platform devices that are not on any bus.
+ *
+ * Populate devices without any I/O access from the CPU, (e.g. fixed
+ * regulators and gpio regulators). In the normal case of a device tree
+ * without runtime-loaded overlays these are direct children of the root
+ * node and as such they are populated as a special case.
+ *
+ * Within the hotplug connector they need to be at a deeper level of the
+ * tree. Moreover they are "segregated" in the "devices" node which allows
+ * to avoid trying of_platform_default_populate() on other kind of nodes.
+ *
+ * No need to depopulate them in this driver: of_platform_notify() will do
+ * that on overlay removal.
+ *
+ * In case a generalized framework for OF_based hotplug connector drivers
+ * will exist in the future, this function is definitely meant for the
+ * framework.
+ */
+static int sunh_conn_populate_nobus_devices(struct sunh_conn *conn)
+{
+	struct device_node *nobus_devs_dn;
+	int err;
+
+	nobus_devs_dn = of_get_child_by_name(conn->dev->of_node, "devices");
+	if (!nobus_devs_dn)
+		return 0;
+
+	err = of_platform_default_populate(nobus_devs_dn, NULL, conn->dev);
+	if (err)
+		dev_err(conn->dev, "Failed to populate nobus devices\n");
+
+	of_node_put(nobus_devs_dn);
+	return err;
+}
+
+static int sunh_conn_insert_overlay(struct sunh_conn *conn,
+				    unsigned int level,
+				    const char *filename)
+{
+	const struct firmware *fw;
+	int err;
+
+	err = request_firmware(&fw, filename, conn->dev);
+	if (err)
+		return dev_err_probe(conn->dev, err, "Error requesting overlay %s", filename);
+
+	dev_dbg(conn->dev, "insert overlay %d: %s", level, filename);
+	err = of_overlay_fdt_apply(fw->data, fw->size, &conn->ovcs_id[level], conn->dev->of_node);
+	if (err)
+		dev_err_probe(conn->dev, err, "Failed to apply overlay %s\n", filename);
+	else
+		err = sunh_conn_populate_nobus_devices(conn);
+
+	if (err) {
+		int err2;
+
+		/* changeset may be partially applied */
+		err2 = of_overlay_remove(&conn->ovcs_id[level]);
+		if (err2 < 0)
+			dev_err_probe(conn->dev, err2,
+				      "Failed to remove failed overlay %s\n", filename);
+	}
+
+	release_firmware(fw);
+
+	return err;
+}
+
+/* Load the overlay common to all add-ons (enough to read the model ID) */
+static int sunh_conn_load_overlay_0(struct sunh_conn *conn)
+{
+	int err = 0;
+
+	mutex_lock(&conn->ovl_mutex);
+
+	if (conn->ovcs_id[0] != 0) {
+		dev_dbg(conn->dev, "base overlay already loaded\n");
+		goto out_unlock;
+	}
+
+	err = sunh_conn_insert_overlay(conn, 0, "imx8mp-sundv1-addon-base.dtbo");
+
+out_unlock:
+	mutex_unlock(&conn->ovl_mutex);
+	return err;
+}
+
+/* Load the model-specific overlay describing everything not in the overlay 0 */
+static int sunh_conn_load_overlay_1(struct sunh_conn *conn)
+{
+	u8 addon_id;
+	const char *filename;
+	int err;
+
+	mutex_lock(&conn->ovl_mutex);
+
+	if (conn->ovcs_id[0] == 0) {
+		dev_dbg(conn->dev, "base overlay not loaded\n");
+		err = -EINVAL;
+		goto out_unlock;
+	}
+
+	if (conn->ovcs_id[1] != 0) {
+		dev_dbg(conn->dev, "addon overlay already loaded\n");
+		err = -EEXIST;
+		goto out_unlock;
+	}
+
+	err = nvmem_cell_read_u8(conn->dev, "id", &addon_id);
+	if (err)
+		goto out_unlock;
+
+	dev_dbg(conn->dev, "Found add-on ID %d\n", addon_id);
+
+	switch (addon_id) {
+	case 23:
+		filename = "imx8mp-sundv1-addon-13.dtbo";
+		break;
+	case 24:
+		filename = "imx8mp-sundv1-addon-15.dtbo";
+		break;
+	case 25:
+		filename = "imx8mp-sundv1-addon-18.dtbo";
+		break;
+	default:
+		dev_warn(conn->dev, "Unknown add-on ID %d\n", addon_id);
+		err = -ENODEV;
+		goto out_unlock;
+	}
+
+	err = sunh_conn_insert_overlay(conn, 1, filename);
+
+out_unlock:
+	mutex_unlock(&conn->ovl_mutex);
+	return err;
+}
+
+static void sunh_conn_unload_overlays(struct sunh_conn *conn)
+{
+	int level = SUNH_CONN_OVERLAY_N_LEVELS;
+	int err;
+
+	mutex_lock(&conn->ovl_mutex);
+	while (level) {
+		level--;
+
+		if (conn->ovcs_id[level] == 0)
+			continue;
+
+		dev_dbg(conn->dev, "remove overlay %d (ovcs id %d)",
+			level, conn->ovcs_id[level]);
+
+		err = of_overlay_remove(&conn->ovcs_id[level]);
+		if (err)
+			dev_err_probe(conn->dev, err, "Failed to remove overlay %d\n", level);
+	}
+	mutex_unlock(&conn->ovl_mutex);
+}
+
+static void sunh_conn_reset(struct sunh_conn *conn, bool keep_reset)
+{
+	gpiod_set_value_cansleep(conn->reset_gpio, 1);
+
+	if (keep_reset)
+		return;
+
+	mdelay(10);
+	gpiod_set_value_cansleep(conn->reset_gpio, 0);
+	mdelay(10);
+}
+
+static int sunh_conn_detach(struct sunh_conn *conn)
+{
+	/* Cancel any pending NVMEM notification jobs */
+	cancel_work_sync(&conn->nvmem_notifier_work);
+
+	/* Unload previouly loaded overlays */
+	sunh_conn_unload_overlays(conn);
+
+	/* Set reset signal to have it set on next plug */
+	sunh_conn_reset(conn, true);
+
+	return 0;
+}
+
+static int sunh_conn_attach(struct sunh_conn *conn)
+{
+	int err;
+
+	/* Reset the plugged board in order to start from a stable state */
+	sunh_conn_reset(conn, false);
+
+	err = sunh_conn_load_overlay_0(conn);
+	if (err)
+		goto err;
+
+	/*
+	 * -EPROBE_DEFER can be due to NVMEM cell not yet available, so
+	 * don't give up, an NVMEM event could arrive later
+	 */
+	err = sunh_conn_load_overlay_1(conn);
+	if (err && err != -EPROBE_DEFER)
+		goto err;
+
+	return 0;
+
+err:
+	sunh_conn_detach(conn);
+	return err;
+}
+
+static int sunh_conn_handle_event(struct sunh_conn *conn, bool plugged)
+{
+	int err;
+
+	if (plugged == conn->plugged)
+		return 0;
+
+	dev_dbg(conn->dev, "%s\n", plugged ? "connected" : "disconnected");
+
+	err = (plugged ?
+	       sunh_conn_attach(conn) :
+	       sunh_conn_detach(conn));
+
+	conn->plugged = plugged;
+
+	return err;
+}
+
+/*
+ * Return the current status of the connector as reported by the hardware,
+ * logging any errors.
+ */
+static int sunh_conn_get_connector_status(struct sunh_conn *conn)
+{
+	int val = gpiod_get_value_cansleep(conn->plugged_gpio);
+
+	if (val < 0)
+		dev_err(conn->dev, "Error reading plugged GPIO (%d)\n", val);
+
+	return val;
+}
+
+static irqreturn_t sunh_conn_gpio_irq(int irq, void *data)
+{
+	struct sunh_conn *conn = data;
+	int conn_status;
+
+	conn_status = sunh_conn_get_connector_status(conn);
+	if (conn_status >= 0)
+		sunh_conn_handle_event(conn, conn_status);
+
+	return IRQ_HANDLED;
+}
+
+static int plugged_read(void *dat, u64 *val)
+{
+	struct sunh_conn *conn = dat;
+
+	*val = conn->plugged;
+
+	return 0;
+}
+
+static int plugged_write(void *dat, u64 val)
+{
+	struct sunh_conn *conn = dat;
+
+	if (val > 1)
+		return -EINVAL;
+
+	return sunh_conn_handle_event(conn, val);
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(plugged_fops, plugged_read, plugged_write, "%lld\n");
+
+static void sunh_conn_nvmem_notifier_work(struct work_struct *work)
+{
+	struct sunh_conn *conn = container_of(work, struct sunh_conn, nvmem_notifier_work);
+
+	sunh_conn_load_overlay_1(conn);
+}
+
+static int sunh_conn_nvmem_notifier(struct notifier_block *nb, unsigned long action, void *arg)
+{
+	struct sunh_conn *conn = container_of(nb, struct sunh_conn, nvmem_nb);
+
+	if (action == NVMEM_CELL_ADD)
+		queue_work(system_power_efficient_wq, &conn->nvmem_notifier_work);
+
+	return NOTIFY_OK;
+}
+
+static int sunh_conn_register_drm_bridge(struct sunh_conn *conn)
+{
+	struct device *dev = conn->dev;
+	struct device_node *dsi_np;
+
+	dsi_np = of_get_child_by_name(dev->of_node, "dsi");
+	if (!dsi_np)
+		return dev_err_probe(dev, -ENOENT, "dsi node not found");
+
+	const struct platform_device_info hpb_info = {
+		.parent = dev,
+		.fwnode = of_fwnode_handle(dsi_np),
+		.of_node_reused = true,
+		.name = "hotplug-dsi-bridge",
+		.id = PLATFORM_DEVID_NONE,
+	};
+
+	conn->hpb_pdev = platform_device_register_full(&hpb_info);
+	of_node_put(dsi_np); // platform core gets/puts the device node
+	if (IS_ERR(conn->hpb_pdev))
+		return dev_err_probe(dev, PTR_ERR(conn->hpb_pdev),
+				     "Error registering DRM bridge\n");
+
+	return 0;
+}
+
+static void sunh_conn_unregister_drm_bridge(struct sunh_conn *conn)
+{
+	platform_device_unregister(conn->hpb_pdev);
+}
+
+static int sunh_conn_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sunh_conn *conn;
+	int conn_status;
+	int err;
+
+	/* Cannot load overlay from filesystem before rootfs is mounted */
+	if (system_state < SYSTEM_RUNNING)
+		return -EPROBE_DEFER;
+
+	conn = devm_kzalloc(dev, sizeof(*conn), GFP_KERNEL);
+	if (!conn)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, conn);
+	conn->dev = dev;
+
+	mutex_init(&conn->ovl_mutex);
+	INIT_WORK(&conn->nvmem_notifier_work, sunh_conn_nvmem_notifier_work);
+
+	conn->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(conn->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(conn->reset_gpio),
+				     "Error getting reset GPIO\n");
+
+	conn->plugged_gpio = devm_gpiod_get_optional(dev, "plugged", GPIOD_IN);
+	if (IS_ERR(conn->plugged_gpio))
+		return dev_err_probe(dev, PTR_ERR(conn->plugged_gpio),
+				     "Error getting plugged GPIO\n");
+
+	err = sunh_conn_register_drm_bridge(conn);
+	if (err)
+		return err;
+
+	conn->nvmem_nb.notifier_call = sunh_conn_nvmem_notifier;
+	err = nvmem_register_notifier(&conn->nvmem_nb);
+	if (err) {
+		dev_err_probe(dev, err, "Error registering NVMEM notifier\n");
+		goto err_unregister_drm_bridge;
+	}
+
+	if (conn->plugged_gpio) {
+		err = devm_request_threaded_irq(dev, gpiod_to_irq(conn->plugged_gpio),
+						NULL, sunh_conn_gpio_irq,
+						IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						dev_name(dev), conn);
+		if (err) {
+			dev_err_probe(dev, err, "Error getting plugged GPIO IRQ\n");
+			goto err_nvmem_unregister_notifier;
+		}
+	}
+
+	conn_status = sunh_conn_get_connector_status(conn);
+	if (conn_status < 0) {
+		err = conn_status;
+		goto err_nvmem_unregister_notifier;
+	}
+
+	/* Ensure initial state is known and overlay loaded if plugged */
+	sunh_conn_handle_event(conn, conn_status);
+
+	conn->debugfs_root = debugfs_create_dir(dev_name(dev), NULL);
+	debugfs_create_file("plugged", 0644, conn->debugfs_root, conn, &plugged_fops);
+
+	return 0;
+
+err_nvmem_unregister_notifier:
+	nvmem_unregister_notifier(&conn->nvmem_nb);
+	cancel_work_sync(&conn->nvmem_notifier_work);
+err_unregister_drm_bridge:
+	sunh_conn_unregister_drm_bridge(conn);
+	return err;
+}
+
+static void sunh_conn_remove(struct platform_device *pdev)
+{
+	struct sunh_conn *conn = platform_get_drvdata(pdev);
+
+	debugfs_remove(conn->debugfs_root);
+	sunh_conn_detach(conn);
+
+	nvmem_unregister_notifier(&conn->nvmem_nb);
+	cancel_work_sync(&conn->nvmem_notifier_work);
+
+	sunh_conn_unregister_drm_bridge(conn);
+}
+
+static const struct of_device_id sunh_conn_dt_ids[] = {
+	{ .compatible = "ge,sunh-addon-connector" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sunh_conn_dt_ids);
+
+static struct platform_driver sunh_conn_driver = {
+	.driver = {
+		.name = "sunh-addon-connector",
+		.of_match_table = sunh_conn_dt_ids,
+	},
+	.probe = sunh_conn_probe,
+	.remove_new = sunh_conn_remove,
+};
+module_platform_driver(sunh_conn_driver);
+
+MODULE_AUTHOR("Luca Ceresoli <luca.ceresoli@bootlin.com>");
+MODULE_AUTHOR("Herve Codina <herve.codina@bootlin.com>");
+MODULE_DESCRIPTION("GE SUNH hotplug add-on connector");
+MODULE_LICENSE("GPL");

-- 
2.34.1

Re: [PATCH v4 1/8] dt-bindings: connector: add GE SUNH hotplug addon connector

[Rob Herring (Arm)]

On Tue, 17 Sep 2024 10:53:05 +0200, Luca Ceresoli wrote:
> Add bindings for the GE SUNH add-on connector. This is a physical,
> hot-pluggable connector that allows to attach and detach at runtime an
> add-on adding peripherals on non-discoverable busses.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> Changed in v4:
>  - rename 'nobus-devices' to 'devices'
>  - use 'additionalProperties: true' for the 'devices' node (nodes are added
>    by overlays)
>  - document GPIO polarity
>  - add '|' for descriptions to preserve line breaks
>  - remove powergood-gpios (removed in hardware design)
>  - Omit "/" node, not needed and cause of warnings
>  - remove reference to v2 examples from example comment
>  - remove unneeded "addon_connector" label from example
> 
> Changed in v3:
>  - change the layout to only add subnodes, not properties
>  - add the 'nobus-devices' node description to hold devices not on any bus
>  - add 'i2c-*' nodes for the I2C busses, using a i2c-parent phandle
>  - and the 'dsi' node for the DSI bus
>  - move the entire port@1 node to the overlay (not only the remote-endpoint
>    property)
>  - remove the overlay examples (Overlays in examples are not supported)
>  - add more clarifying descriptions and comments for examples
>  - some rewording
> 
> This patch was added in v2.
> ---
>  .../connector/ge,sunh-addon-connector.yaml         | 177 +++++++++++++++++++++
>  MAINTAINERS                                        |   5 +
>  2 files changed, 182 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.example.dtb: addon-connector: Unevaluated properties are not allowed ('powergood-gpios' was unexpected)
	from schema $id: http://devicetree.org/schemas/connector/ge,sunh-addon-connector.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240917-hotplug-drm-bridge-v4-1-bc4dfee61be6@bootlin.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v4 1/8] dt-bindings: connector: add GE SUNH hotplug addon connector

[Luca Ceresoli]

Hi,

On Tue, 17 Sep 2024 05:29:51 -0500
"Rob Herring (Arm)" <robh@kernel.org> wrote:

> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.example.dtb: addon-connector: Unevaluated properties are not allowed ('powergood-gpios' was unexpected)

Ouch, a leftover from v3. Fixed queued for v5.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 6/8] backlight: led-backlight: add devlink to supplier LEDs

[Daniel Thompson]

On Tue, Sep 17, 2024 at 10:53:10AM +0200, Luca Ceresoli wrote:
> led-backlight is a consumer of one or multiple LED class devices, but no
> devlink is created for such supplier-producer relationship. One consequence
> is that removal ordered is not correctly enforced.
>
> Issues happen for example with the following sections in a device tree
> overlay:
>
>     // An LED driver chip
>     pca9632@62 {
>         compatible = "nxp,pca9632";
>         reg = <0x62>;
>
> 	// ...
>
>         addon_led_pwm: led-pwm@3 {
>             reg = <3>;
>             label = "addon:led:pwm";
>         };
>     };
>
>     backlight-addon {
>         compatible = "led-backlight";
>         leds = <&addon_led_pwm>;
>         brightness-levels = <255>;
>         default-brightness-level = <255>;
>     };
>
> On removal of the above overlay, the LED driver can be removed before the
> backlight device, resulting in:
>
>     Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
>     ...
>     Call trace:
>      led_put+0xe0/0x140
>      devm_led_release+0x6c/0x98

This looks like the object became invalid whilst we were holding a reference
to it. Is that reasonable? Put another way, is using devlink here fixing a
bug or merely hiding one?


Daniel.

Re: [PATCH v4 6/8] backlight: led-backlight: add devlink to supplier LEDs

[Luca Ceresoli]

Hello Daniel,

On Thu, 19 Sep 2024 14:43:23 +0200
Daniel Thompson <daniel.thompson@linaro.org> wrote:

> On Tue, Sep 17, 2024 at 10:53:10AM +0200, Luca Ceresoli wrote:
> > led-backlight is a consumer of one or multiple LED class devices, but no
> > devlink is created for such supplier-producer relationship. One consequence
> > is that removal ordered is not correctly enforced.
> >
> > Issues happen for example with the following sections in a device tree
> > overlay:
> >
> >     // An LED driver chip
> >     pca9632@62 {
> >         compatible = "nxp,pca9632";
> >         reg = <0x62>;
> >
> > 	// ...
> >
> >         addon_led_pwm: led-pwm@3 {
> >             reg = <3>;
> >             label = "addon:led:pwm";
> >         };
> >     };
> >
> >     backlight-addon {
> >         compatible = "led-backlight";
> >         leds = <&addon_led_pwm>;
> >         brightness-levels = <255>;
> >         default-brightness-level = <255>;
> >     };
> >
> > On removal of the above overlay, the LED driver can be removed before the
> > backlight device, resulting in:
> >
> >     Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
> >     ...
> >     Call trace:
> >      led_put+0xe0/0x140
> >      devm_led_release+0x6c/0x98  
> 
> This looks like the object became invalid whilst we were holding a reference
> to it. Is that reasonable? Put another way, is using devlink here fixing a
> bug or merely hiding one?

Thanks for your comment.

Hervé and I just had a look at the code and there actually might be a
bug here, which we will be investigating (probably next week).

Still I think the devlink needs to be added to describe the
relationship between the supplier (LED) and consumer (backlight).

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 4/8] drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges

[Luca Ceresoli]

Hello Simona, Maxime, Dmitry,

On Tue, 17 Sep 2024 10:53:08 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> This driver implements the point of a DRM pipeline where a connector allows
> removal of all the following bridges up to the panel.

This is a quick feedback of the discussions about hotpluggable DRM
bridges I had with Maxime and Dmitry during the Linux Plumbers
Conference last week, which happened during the discussion I led on
Wednesday afternoon and even more in the hallway.

It was clear that adding refcounting to DRM bridges and handling
gracefully atomic modeset operations during device removal are
prerequisites for a hotpluggable bridge in DRM.

DSI host drivers that keep a pointer to their device also need to be
fixed, but this is well known and was not  discussed.

Simona's proposal of letting removable bridges be owned by the hotplug
connector was not seen as necessarily needed, especially after adding
bridge refcounting. Also Maxime mentioned this idea might conflict with
a use case that had been brought to his attention, involving bridges
and DP MST. So I'm removing this idea from my roadmap -- it can be
re-discussed and brought back if needed.

There was no clear outcome about the question of whether the
hotplug-bridge should still exist as a drm_bridge or rather the DRM
core and the bridge drivers should be extended to handle everything.
Dmitry seemed to think no hotplug bridge should exist at all, and all
bridge drivers should be extended to cope with insertion and removal of
the follofing bridge. Ripard's opinion was "I don't care". In her
e-mail sent a few hours ago (in the v2 thread) Simona also appears as
not having a strong position about this. There will be some time to
clarify this as I'm addressing the previously mentioned issues.

So by now the roadmap involves the following main steps:
 1. add refcounting to DRM bridges (struct drm_bridge)
 2. handle gracefully atomic updates during bridge removal
 3. avoid DSI host drivers to have dangling pointers to DSI devices
 4. finish the hotplug bridge work, moving code to the core and
    potentially removing the hotplug-bridge itself

If you think any other major item is required to support hot-pluggable
DRM bridges, don't hesitate to speak out.

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 5/8] i2c: i2c-core-of: follow i2c-parent phandle to probe devices from added nodes

[Sverdlin, Alexander]

Hi Luca!

On Tue, 2024-09-17 at 10:53 +0200, Luca Ceresoli wrote:
> When device tree nodes are added, the I2C core tries to probe client
> devices based on the classic DT structure:
> 
>   i2c@abcd0000 {
>       some-client@42 { compatible = "xyz,blah"; ... };
>   };
> 
> However for hotplug connectors described via device tree overlays there is
> additional level of indirection, which is needed to decouple the overlay
> and the base tree:
> 
>   --- base device tree ---
> 
>   i2c1: i2c@abcd0000 { compatible = "xyz,i2c-ctrl"; ... };
>   i2c5: i2c@cafe0000 { compatible = "xyz,i2c-ctrl"; ... };
> 
>   connector {
>       i2c-ctrl {
>           i2c-parent = <&i2c1>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
> 
>       i2c-sensors {
>           i2c-parent = <&i2c5>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
>   };
> 
>   --- device tree overlay ---
> 
>   ...
>   // This node will overlay on the i2c-ctrl node of the base tree

Why don't you overlay it right over &i2c1?
It should have worked since commit ea7513bbc041
("i2c/of: Add OF_RECONFIG notifier handler").
Doesn't it work for your use-case?

>   i2c-ctrl {
>       eeprom@50 { compatible = "atmel,24c64"; ... };
>   };
>   ...
> 
>   --- resulting device tree ---
> 
>   i2c1: i2c@abcd0000 { compatible = "xyz,i2c-ctrl"; ... };
>   i2c5: i2c@cafe0000 { compatible = "xyz,i2c-ctrl"; ... };
> 
>   connector {
>       i2c-ctrl {
>           i2c-parent = <&i2c1>;
>           #address-cells = <1>;
>           #size-cells = <0>;
> 
>           eeprom@50 { compatible = "atmel,24c64"; ... };
>       };
> 
>       i2c-sensors {
>           i2c-parent = <&i2c5>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
>   };
> 
> Here i2c-ctrl (same goes for i2c-sensors) represent the part of I2C bus
> that is on the hot-pluggable add-on. On hot-plugging it will physically
> connect to the I2C adapter on the base board. Let's call the 'i2c-ctrl'
> node an "extension node".
> 
> In order to decouple the overlay from the base tree, the I2C adapter
> (i2c@abcd0000) and the extension node (i2c-ctrl) are separate
> nodes. Rightfully, only the former will probe into an I2C adapter, and it
> will do that perhaps during boot, long before overlay insertion.
> 
> The extension node won't probe into an I2C adapter or any other device or
> bus, so its subnodes ('eeprom@50') won't be interpreted as I2C clients by
> current I2C core code. However it has an 'i2c-parent' phandle to point to
> the corresponding I2C adapter node. This tells those nodes are I2C clients
> of the adapter in that other node.
> 
> Extend the i2c-core-of code to look for the adapter via the 'i2c-parent'
> phandle when the regular adapter lookup does not find one. This allows all
> clients to be probed: both those on the base board (described in the base
> device tree) and those on the add-on and described by an overlay.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
> 
> Note: while this patch works for normal hotplug and unplug, it has some
> weaknesses too, due to the implementation being in a OF change
> notifier. Two cases come to mind:
> 
>  1. In the base device tree there must be _no_ nodes under the "extension
>     node" (i2c-ctrl), or they won't be picked up as they are not
>     dynamically added.
> 
>  2. In case the I2C adapter is unbound and rebound, or it probes after
>     overlay insertion, it will miss the OF notifier events and so it won't
>     find the devices in the extension node.
> 
> The first case is not a limiting factor: fixed I2C devices should just stay
> under the good old I2C adapter node.
> 
> The second case is a limiting factor, even though not happening in "normal"
> use cases. I cannot see any solution without making the adapter aware of
> the "bus extensions" it has, so on its probe it can always go look for any
> devices there. Taking into account the case of multiple connectors each
> having an extension of the same bus, this may look as follows in device
> tree:
> 
>   --- base device tree ---
> 
>   i2c1: i2c@abcd0000 {
>       compatible = "xyz,i2c-ctrl"; ...
>       i2c-bus-extensions = <&i2c_ctrl_conn0, &i2c_ctrl_conn1>;
>   };
> 
>   connector@0 {
>       i2c_ctrl_conn0: i2c-ctrl {
>           i2c-parent = <&i2c1>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
>   };
> 
>   connector@1 {
>       i2c_ctrl_conn1: i2c-ctrl {
>           i2c-parent = <&i2c1>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
>   };
> 
> I'd love to have some feedback and opinions about the basic idea before
> digging into the details of this additional step.
> 
> ---
> 
> Changes in v4:
>  - fix a typo in commit message
> 
> This patch first appeared in v3.
> ---
>  drivers/i2c/i2c-core-of.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> index a6c407d36800..71c559539a13 100644
> --- a/drivers/i2c/i2c-core-of.c
> +++ b/drivers/i2c/i2c-core-of.c
> @@ -170,6 +170,15 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
>  	switch (of_reconfig_get_state_change(action, rd)) {
>  	case OF_RECONFIG_CHANGE_ADD:
>  		adap = of_find_i2c_adapter_by_node(rd->dn->parent);
> +		if (adap == NULL) {
> +			struct device_node *i2c_bus;
> +
> +			i2c_bus = of_parse_phandle(rd->dn->parent, "i2c-parent", 0);
> +			if (i2c_bus) {
> +				adap = of_find_i2c_adapter_by_node(i2c_bus);
> +				of_node_put(i2c_bus);
> +			}
> +		}
>  		if (adap == NULL)
>  			return NOTIFY_OK;	/* not for us */
>  

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH v4 5/8] i2c: i2c-core-of: follow i2c-parent phandle to probe devices from added nodes

[Luca Ceresoli]

Hello Alexander,

On Thu, 12 Dec 2024 19:12:02 +0000
"Sverdlin, Alexander" <alexander.sverdlin@siemens.com> wrote:

> Hi Luca!
> 
> On Tue, 2024-09-17 at 10:53 +0200, Luca Ceresoli wrote:
> > When device tree nodes are added, the I2C core tries to probe client
> > devices based on the classic DT structure:
> > 
> >   i2c@abcd0000 {
> >       some-client@42 { compatible = "xyz,blah"; ... };
> >   };
> > 
> > However for hotplug connectors described via device tree overlays there is
> > additional level of indirection, which is needed to decouple the overlay
> > and the base tree:
> > 
> >   --- base device tree ---
> > 
> >   i2c1: i2c@abcd0000 { compatible = "xyz,i2c-ctrl"; ... };
> >   i2c5: i2c@cafe0000 { compatible = "xyz,i2c-ctrl"; ... };
> > 
> >   connector {
> >       i2c-ctrl {
> >           i2c-parent = <&i2c1>;
> >           #address-cells = <1>;
> >           #size-cells = <0>;
> >       };
> > 
> >       i2c-sensors {
> >           i2c-parent = <&i2c5>;
> >           #address-cells = <1>;
> >           #size-cells = <0>;
> >       };
> >   };
> > 
> >   --- device tree overlay ---
> > 
> >   ...
> >   // This node will overlay on the i2c-ctrl node of the base tree  
> 
> Why don't you overlay it right over &i2c1?
> It should have worked since commit ea7513bbc041
> ("i2c/of: Add OF_RECONFIG notifier handler").
> Doesn't it work for your use-case?

One reason is decoupling the base board and addon. A different base
board may wire the same connector pins to 'i2c4' instead of 'i2c1'. We
want a single overlay to describe the addon, independently of the base
board, so it has to mention only connector pins, not base board
hardware.

Another reason is that using phandles to labels in the base tree in the
overlay (such as &i2c1) would need properties added by the __symbols__
node, and overlays adding properties to nodes in the live tree are not
welcome. This is both for a conceptual reason (adding an overlay ==
adding hardware and not _changing_ hardware, so adding nodes should be
enough) and an implementation one (properties added to nodes in the
live tree become deadprops and thus leak memory.

This topic was discussed at the latest Linux Plumbers Conference last
September. Slides and video of the discussion are available here:
https://lpc.events/event/18/contributions/1696/

More info are in the cover letter. Discussion leading to this
implementation started after v2:
https://lore.kernel.org/all/20240510163625.GA336987-robh@kernel.org/

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 5/8] i2c: i2c-core-of: follow i2c-parent phandle to probe devices from added nodes

[Sverdlin, Alexander]

Hi Luca!

On Fri, 2024-12-13 at 12:28 +0100, Luca Ceresoli wrote:
> > > However for hotplug connectors described via device tree overlays there is
> > > additional level of indirection, which is needed to decouple the overlay
> > > and the base tree:
> > > 
> > >    --- base device tree ---
> > > 
> > >    i2c1: i2c@abcd0000 { compatible = "xyz,i2c-ctrl"; ... };
> > >    i2c5: i2c@cafe0000 { compatible = "xyz,i2c-ctrl"; ... };
> > > 
> > >    connector {
> > >        i2c-ctrl {
> > >            i2c-parent = <&i2c1>;
> > >            #address-cells = <1>;
> > >            #size-cells = <0>;
> > >        };
> > > 
> > >        i2c-sensors {
> > >            i2c-parent = <&i2c5>;
> > >            #address-cells = <1>;
> > >            #size-cells = <0>;
> > >        };
> > >    };
> > > 
> > >    --- device tree overlay ---
> > > 
> > >    ...
> > >    // This node will overlay on the i2c-ctrl node of the base tree  
> > 
> > Why don't you overlay it right over &i2c1?
> > It should have worked since commit ea7513bbc041
> > ("i2c/of: Add OF_RECONFIG notifier handler").
> > Doesn't it work for your use-case?
> 
> One reason is decoupling the base board and addon. A different base
> board may wire the same connector pins to 'i2c4' instead of 'i2c1'. We
> want a single overlay to describe the addon, independently of the base
> board, so it has to mention only connector pins, not base board
> hardware.
> 
> Another reason is that using phandles to labels in the base tree in the
> overlay (such as &i2c1) would need properties added by the __symbols__
> node, and overlays adding properties to nodes in the live tree are not
> welcome. This is both for a conceptual reason (adding an overlay ==
> adding hardware and not _changing_ hardware, so adding nodes should be
> enough) and an implementation one (properties added to nodes in the
> live tree become deadprops and thus leak memory.
> 
> This topic was discussed at the latest Linux Plumbers Conference last
> September. Slides and video of the discussion are available here:
> https://lpc.events/event/18/contributions/1696/
> 
> More info are in the cover letter. Discussion leading to this
> implementation started after v2:
> https://lore.kernel.org/all/20240510163625.GA336987-robh@kernel.org/

I see! Thank you for the explanation and for the references!

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH v4 6/8] backlight: led-backlight: add devlink to supplier LEDs

[Sverdlin, Alexander]

Hi Luca,

On Tue, 2024-09-17 at 10:53 +0200, Luca Ceresoli wrote:
> led-backlight is a consumer of one or multiple LED class devices, but no
> devlink is created for such supplier-producer relationship. One consequence
> is that removal ordered is not correctly enforced.
> 
> Issues happen for example with the following sections in a device tree
> overlay:
> 
>     // An LED driver chip
>     pca9632@62 {
>         compatible = "nxp,pca9632";
>         reg = <0x62>;
> 
> 	// ...
> 
>         addon_led_pwm: led-pwm@3 {
>             reg = <3>;
>             label = "addon:led:pwm";
>         };
>     };
> 
>     backlight-addon {
>         compatible = "led-backlight";
>         leds = <&addon_led_pwm>;
>         brightness-levels = <255>;
>         default-brightness-level = <255>;
>     };
> 
> On removal of the above overlay, the LED driver can be removed before the
> backlight device, resulting in:
> 
>     Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
>     ...
>     Call trace:
>      led_put+0xe0/0x140
>      devm_led_release+0x6c/0x98
> 
> Fix by adding a devlink between the consuming led-backlight device and the
> supplying LED device.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

I've tested the patch wit LP8864 LED as a provider for led_bl, removing the
underlying I2C bus. The patch avoids the crash for me (by removing led_bl device as well),
thanks for fixing it!

Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

> ---
> 
> This patch first appeared in v4.
> ---
>  drivers/video/backlight/led_bl.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> index c7aefcd6e4e3..bfbd80728036 100644
> --- a/drivers/video/backlight/led_bl.c
> +++ b/drivers/video/backlight/led_bl.c
> @@ -209,6 +209,19 @@ static int led_bl_probe(struct platform_device *pdev)
>  		return PTR_ERR(priv->bl_dev);
>  	}
>  
> +	for (i = 0; i < priv->nb_leds; i++) {
> +		struct device_link *link;
> +
> +		link = device_link_add(&pdev->dev, priv->leds[0]->dev->parent,
> +				       DL_FLAG_AUTOREMOVE_CONSUMER);
> +		if (!link) {
> +			dev_err(&pdev->dev, "Failed to add devlink (consumer %s, supplier %s)\n",
> +				dev_name(&pdev->dev), dev_name(priv->leds[0]->dev->parent));
> +			backlight_device_unregister(priv->bl_dev);
> +			return -EINVAL;
> +		}
> +	}
> +
>  	for (i = 0; i < priv->nb_leds; i++) {
>  		mutex_lock(&priv->leds[i]->led_access);
>  		led_sysfs_disable(priv->leds[i]);

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH v4 6/8] backlight: led-backlight: add devlink to supplier LEDs

[Sverdlin, Alexander]

Hi Luca,

On Mon, 2025-05-12 at 13:39 +0200, Alexander Sverdlin wrote:
> > led-backlight is a consumer of one or multiple LED class devices, but no
> > devlink is created for such supplier-producer relationship. One consequence
> > is that removal ordered is not correctly enforced.
> > 
> > Issues happen for example with the following sections in a device tree
> > overlay:
> > 
> >     // An LED driver chip
> >     pca9632@62 {
> >         compatible = "nxp,pca9632";
> >         reg = <0x62>;
> > 
> > 	// ...
> > 
> >         addon_led_pwm: led-pwm@3 {
> >             reg = <3>;
> >             label = "addon:led:pwm";
> >         };
> >     };
> > 
> >     backlight-addon {
> >         compatible = "led-backlight";
> >         leds = <&addon_led_pwm>;
> >         brightness-levels = <255>;
> >         default-brightness-level = <255>;
> >     };
> > 
> > On removal of the above overlay, the LED driver can be removed before the
> > backlight device, resulting in:
> > 
> >     Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
> >     ...
> >     Call trace:
> >      led_put+0xe0/0x140
> >      devm_led_release+0x6c/0x98
> > 
> > Fix by adding a devlink between the consuming led-backlight device and the
> > supplying LED device.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> I've tested the patch wit LP8864 LED as a provider for led_bl, removing the
> underlying I2C bus. The patch avoids the crash for me (by removing led_bl device as well),
> thanks for fixing it!
> 
> Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Would it make sense to add

Fixes: ae232e45acf9 ("backlight: add led-backlight driver")

?

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH v4 6/8] backlight: led-backlight: add devlink to supplier LEDs

[Luca Ceresoli]

Hello Alexander,

On Mon, 12 May 2025 12:13:54 +0000
"Sverdlin, Alexander" <alexander.sverdlin@siemens.com> wrote:

> > I've tested the patch wit LP8864 LED as a provider for led_bl, removing the
> > underlying I2C bus. The patch avoids the crash for me (by removing led_bl device as well),
> > thanks for fixing it!
> > 
> > Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>  

Thanks for the feedback! I will have a look next week, perhaps sending
a new version of this patch alone (outside of the series).

> Would it make sense to add
> 
> Fixes: ae232e45acf9 ("backlight: add led-backlight driver")
> 
> ?

Probably.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 6/8] backlight: led-backlight: add devlink to supplier LEDs

[Luca Ceresoli]

Hello Daniel,

I wonder whether you remember about this conversation...

On Fri, 20 Sep 2024 14:41:13 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> Hello Daniel,
> 
> On Thu, 19 Sep 2024 14:43:23 +0200
> Daniel Thompson <daniel.thompson@linaro.org> wrote:
> 
> > On Tue, Sep 17, 2024 at 10:53:10AM +0200, Luca Ceresoli wrote:  
> > > led-backlight is a consumer of one or multiple LED class devices, but no
> > > devlink is created for such supplier-producer relationship. One consequence
> > > is that removal ordered is not correctly enforced.
> > >
> > > Issues happen for example with the following sections in a device tree
> > > overlay:
> > >
> > >     // An LED driver chip
> > >     pca9632@62 {
> > >         compatible = "nxp,pca9632";
> > >         reg = <0x62>;
> > >
> > > 	// ...
> > >
> > >         addon_led_pwm: led-pwm@3 {
> > >             reg = <3>;
> > >             label = "addon:led:pwm";
> > >         };
> > >     };
> > >
> > >     backlight-addon {
> > >         compatible = "led-backlight";
> > >         leds = <&addon_led_pwm>;
> > >         brightness-levels = <255>;
> > >         default-brightness-level = <255>;
> > >     };
> > >
> > > On removal of the above overlay, the LED driver can be removed before the
> > > backlight device, resulting in:
> > >
> > >     Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
> > >     ...
> > >     Call trace:
> > >      led_put+0xe0/0x140
> > >      devm_led_release+0x6c/0x98    
> > 
> > This looks like the object became invalid whilst we were holding a reference
> > to it. Is that reasonable? Put another way, is using devlink here fixing a
> > bug or merely hiding one?  
> 
> Thanks for your comment.
> 
> Hervé and I just had a look at the code and there actually might be a
> bug here, which we will be investigating (probably next week).
> 
> Still I think the devlink needs to be added to describe the
> relationship between the supplier (LED) and consumer (backlight).

It took "slightly more" than "next week", but we are here finally. In
reality this topics went pretty much forgotten until Alexander
Sverdlin's feedback [0].

About your concern, I'm not totally sure devlink is the tool expected
to solve this issue, but if it isn't I don't know any other tool that
should.

In other words, because devlink is exactly meant to represent
supplier-consumer relationships and enforce them to be respected, it
seems the appropriate tool. Moreover devlink already handles such
relationships quite well in many cases, and takes care of removing
consumers before their suppliers, when suppliers get removed.

One missing piece in devlink is it doesn't (yet) handle class devices
correctly. When the supplier is a class device (such as the LED device
in this case), then devlink creates a link to the parent of the
supplier, and not the supplier itself.

This problem is well known and it is under Saravana's radar. Adding
such devlinks at the device core level would be of course be the best
and most generic solution, but it seems to be much more tricky that it
may look. So other drivers and subsystems are "manually" creating
devlinks, to have the right links in place until devlink can figure
them out automatically. Some examples ('git grep device_link_add' for
more):

  https://elixir.bootlin.com/linux/v6.14.7/source/drivers/pwm/core.c#L1660
  https://elixir.bootlin.com/linux/v6.14.7/source/drivers/iio/industrialio-backend.c#L710
  https://elixir.bootlin.com/linux/v6.14.7/source/drivers/pmdomain/imx/gpc.c#L204

I hope this clarifies the need for this patch.

I am going to send this patch alone in a moment, detached from the
entire series because it is orthogonal.

[0]
https://lore.kernel.org/all/fa87471d31a62017067d4c3ba559cf79d6c3afec.camel@siemens.com/

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com