* [PATCH v2 4/4] PCI: mediatek-gen3: Add 2-lanes mode support for Airoha AN7581
From: Christian Marangi @ 2026-06-26 9:20 UTC (permalink / raw)
To: Bjorn Helgaas, Lorenzo Pieralisi, Krzysztof Wilczyński,
Manivannan Sadhasivam, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Ryder Lee, Michael Turquette, Stephen Boyd,
Brian Masney, Philipp Zabel, Matthias Brugger,
AngeloGioacchino Del Regno, Christian Marangi, Jianjun Wang,
linux-pci, devicetree, linux-kernel, linux-mediatek, linux-clk,
linux-arm-kernel
In-Reply-To: <20260626092029.3525264-1-ansuelsmth@gmail.com>
The Airoha AN7581 SoC supports configuring the first PCIe0 line to 2-lanes
mode by bonding it with the second PCIe line. This is done by configuring
the PCIe MUX in the SCU register.
To correctly configure the line for 2-lanes mode, it's required to define
in DT an additional reg, 'sec-pcie-mac' for the secondary PCIe.
It's also needed to define the additional reset and the PERSTOUT reset.
Also 'airoha,scu' property is mandatory to correctly configure the SCU
register for the PCIe MUX.
Finally to toggle 2-lanes mode, it's needed to define in DT 'num-lanes' as
2.
In such configuration the EQ preset are configured to the same values.
To permit correct configuration of the PCIe line, additional logic is added
to assert and deassert the PERSTOUT resets.
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
drivers/pci/controller/pcie-mediatek-gen3.c | 101 ++++++++++++++++----
1 file changed, 82 insertions(+), 19 deletions(-)
diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index b0accd828589..14893cff135a 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -32,6 +32,11 @@
#include "../pci.h"
+/* AN7581 SCU register */
+#define SCU_PCIC 0x88
+#define SCU_PCIC_PCIE_CTRL GENMASK(7, 0)
+
+/* PCIe register */
#define PCIE_BASE_CFG_REG 0x14
#define PCIE_BASE_CFG_SPEED GENMASK(15, 8)
@@ -131,6 +136,7 @@
#define PCIE_ATR_TLP_TYPE_IO PCIE_ATR_TLP_TYPE(2)
#define MAX_NUM_PHY_RESETS 3
+#define MAX_NUM_PERSTOUT_RESETS 2
#define PCIE_MTK_RESET_TIME_US 10
@@ -203,9 +209,11 @@ struct mtk_msi_set {
struct mtk_gen3_pcie {
struct device *dev;
void __iomem *base;
+ void __iomem *sec_base;
phys_addr_t reg_base;
struct reset_control *mac_reset;
struct reset_control_bulk_data phy_resets[MAX_NUM_PHY_RESETS];
+ struct reset_control_bulk_data perstout_resets[MAX_NUM_PERSTOUT_RESETS];
struct phy *phy;
struct clk_bulk_data *clks;
int num_clks;
@@ -928,6 +936,14 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
if (ret)
return dev_err_probe(dev, ret, "failed to get PHY bulk reset\n");
+ pcie->perstout_resets[0].id = "perstout";
+ pcie->perstout_resets[1].id = "sec-perstout";
+
+ ret = devm_reset_control_bulk_get_optional_exclusive(dev, MAX_NUM_PERSTOUT_RESETS,
+ pcie->perstout_resets);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to get PERSTOUT bulk reset\n");
+
pcie->mac_reset = devm_reset_control_get_optional_exclusive(dev, "mac");
if (IS_ERR(pcie->mac_reset))
return dev_err_probe(dev, PTR_ERR(pcie->mac_reset), "failed to get MAC reset\n");
@@ -949,18 +965,38 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
pcie->num_lanes = num_lanes;
}
+ /* Map secondary PCIe for 2-lanes mode for EN7581 */
+ if (num_lanes == 2 && device_is_compatible(dev, "airoha,en7581-pcie")) {
+ regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sec-pcie-mac");
+ if (!regs)
+ return -EINVAL;
+ pcie->sec_base = devm_ioremap_resource(dev, regs);
+ if (IS_ERR(pcie->sec_base))
+ return dev_err_probe(dev, PTR_ERR(pcie->sec_base), "failed to map secondary register base\n");
+ }
+
return 0;
}
static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
{
struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+ unsigned int num_lanes = max(1, pcie->num_lanes);
+ struct regmap *pbus_regmap, *scu;
struct device *dev = pcie->dev;
struct resource_entry *entry;
- struct regmap *pbus_regmap;
u32 val, args[2], size;
resource_size_t addr;
- int err;
+ int i, err;
+
+ if (num_lanes > 2)
+ return dev_err_probe(dev, -EINVAL, "unsupported num-lanes, maximum 2 lanes supported\n");
+
+ if (num_lanes == 2) {
+ scu = syscon_regmap_lookup_by_phandle(dev->of_node, "airoha,scu");
+ if (IS_ERR(scu))
+ return dev_err_probe(dev, PTR_ERR(scu), "failed to map SCU regmap\n");
+ }
/*
* The controller may have been left out of reset by the bootloader
@@ -992,6 +1028,19 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
size = lower_32_bits(resource_size(entry->res));
regmap_write(pbus_regmap, args[1], GENMASK(31, __fls(size)));
+ /* Assert PERSTOUT for all relevant lines */
+ err = reset_control_bulk_assert(MAX_NUM_PERSTOUT_RESETS,
+ pcie->perstout_resets);
+ if (err) {
+ dev_err(dev, "failed to assert PERSTOUTs\n");
+ return err;
+ }
+
+ /* Configure SCU MUX to disable PCIE1 for 2 lines mode */
+ if (num_lanes == 2)
+ regmap_update_bits(scu, SCU_PCIC, SCU_PCIC_PCIE_CTRL,
+ FIELD_PREP(SCU_PCIC_PCIE_CTRL, BIT(1)));
+
/*
* Unlike the other MediaTek Gen3 controllers, the Airoha EN7581
* requires PHY initialization and power-on before PHY reset deassert.
@@ -1024,33 +1073,47 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
pm_runtime_enable(dev);
pm_runtime_get_sync(dev);
- val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) |
- FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) |
- FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) |
- FIELD_PREP(PCIE_VAL_LN1_UPSTREAM, 0x41);
- writel_relaxed(val, pcie->base + PCIE_EQ_PRESET_01_REG);
-
- val = PCIE_K_PHYPARAM_QUERY | PCIE_K_QUERY_TIMEOUT |
- FIELD_PREP(PCIE_K_PRESET_TO_USE_16G, 0x80) |
- FIELD_PREP(PCIE_K_PRESET_TO_USE, 0x2) |
- FIELD_PREP(PCIE_K_FINETUNE_MAX, 0xf);
- writel_relaxed(val, pcie->base + PCIE_PIPE4_PIE8_REG);
-
err = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks);
if (err) {
dev_err(dev, "failed to prepare clock\n");
goto err_clk_prepare_enable;
}
- /*
- * Airoha EN7581 performs PCIe reset via clk callbacks since it has a
- * hw issue with PCIE_PE_RSTB signal. Add wait for the time needed to
- * complete the PCIe reset.
- */
+ /* Wait for refclk to stabilize */
msleep(PCIE_T_PVPERL_MS);
+ /* Configure all the lines to the same EQ config */
+ for (i = 0; i < num_lanes; i++) {
+ void __iomem *base = pcie->base;
+
+ if (i == 1)
+ base = pcie->sec_base;
+
+ val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) |
+ FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) |
+ FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) |
+ FIELD_PREP(PCIE_VAL_LN1_UPSTREAM, 0x41);
+ writel_relaxed(val, base + PCIE_EQ_PRESET_01_REG);
+
+ val = PCIE_K_PHYPARAM_QUERY | PCIE_K_QUERY_TIMEOUT |
+ FIELD_PREP(PCIE_K_PRESET_TO_USE_16G, 0x80) |
+ FIELD_PREP(PCIE_K_PRESET_TO_USE, 0x2) |
+ FIELD_PREP(PCIE_K_FINETUNE_MAX, 0xf);
+ writel_relaxed(val, base + PCIE_PIPE4_PIE8_REG);
+ }
+
+ /* Deassert PERSTOUT for all relevant lines */
+ err = reset_control_bulk_deassert(MAX_NUM_PERSTOUT_RESETS,
+ pcie->perstout_resets);
+ if (err) {
+ dev_err(dev, "failed to deassert PERSTOUTs\n");
+ goto err_perstout_deassert;
+ }
+
return 0;
+err_perstout_deassert:
+ clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
err_clk_prepare_enable:
pm_runtime_put_sync(dev);
pm_runtime_disable(dev);
--
2.53.0
^ permalink raw reply related
* Re: [PATCH v2 3/4] dt-bindings: PCI: mediatek-gen3: Split Airoha schema and document 2-lanes
From: sashiko-bot @ 2026-06-26 9:26 UTC (permalink / raw)
To: Christian Marangi; +Cc: linux-pci, conor+dt, devicetree, robh
In-Reply-To: <20260626092029.3525264-4-ansuelsmth@gmail.com>
> To permit proper documentation of required property to support PCIe
> configured for 2-lanes mode, split the Airoha schema part from the
> mediatek-gen3 schema to a dedicated schema.
>
> A PCIe configured for 2-lanes mode require an additional reg for the
> secondary PCIe to be configured and the airoha,scu phandle to correctly
> configure the PCIe MUX.
>
> Rework the mediatek-gen3 schema to drop any redundant constraint previsouly
> introduced for Airoha PCIe properties.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260626092029.3525264-1-ansuelsmth@gmail.com?part=3
^ permalink raw reply
* Re: [PATCH v2 1/4] dt-bindings: clock: airoha: Add additional reset for PCIe PERSTOUT
From: sashiko-bot @ 2026-06-26 9:26 UTC (permalink / raw)
To: Christian Marangi; +Cc: robh, linux-pci, devicetree, conor+dt
In-Reply-To: <20260626092029.3525264-2-ansuelsmth@gmail.com>
> Add additional reset to control PCIe PERSTOUT reset line for each of the 3
> PCIe lines.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260626092029.3525264-1-ansuelsmth@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH v5 1/7] dt-bindings: display: verisilicon,dc: generalize for single-output variants
From: Conor Dooley @ 2026-06-26 9:26 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Conor Dooley, Joey Lu, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, robh, krzk+dt, conor+dt, ychuang3, schung, yclu4,
dri-devel, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <c0731801729713d35f97bec40ebc21cef31b72b5.camel@iscas.ac.cn>
[-- Attachment #1: Type: text/plain, Size: 3675 bytes --]
On Fri, Jun 26, 2026 at 05:00:35PM +0800, Icenowy Zheng wrote:
> 在 2026-06-26五的 08:19 +0100，Conor Dooley写道：
> > On Fri, Jun 26, 2026 at 01:27:21PM +0800, Icenowy Zheng wrote:
> > > 在 2026-06-25四的 17:33 +0100，Conor Dooley写道：
> > > > On Thu, Jun 25, 2026 at 05:44:43PM +0800, Joey Lu wrote:
> > > > > +allOf:
> > > > > + - if:
> > > > > + properties:
> > > > > + compatible:
> > > > > + contains:
> > > > > + const: thead,th1520-dc8200
> > > > > + then:
> > > > > + properties:
> > > > > + clocks:
> > > > > + minItems: 5
> > > > > + maxItems: 5
> > > > > +
> > > > > + clock-names:
> > > > > + minItems: 5
> > > > > + maxItems: 5
> > > >
> > > > All the maxItems here repeat the maximum constraint and do
> > > > nothing.
> > > >
> > > > Since you didn't change the minimum constraint at the top level,
> > > > your
> > > > minItems also do nothing.
> > > >
> > > > > +
> > > > > + resets:
> > > > > + minItems: 3
> > > > > + maxItems: 3
> > > > > +
> > > > > + reset-names:
> > > > > + minItems: 3
> > > > > + maxItems: 3
> > > > > +
> > > > > + required:
> > > > > + - resets
> > > > > + - reset-names
> > > >
> > > > Both conditional sections have this, but the original binding
> > > > doesn't
> > > > require these for the thead device. This is a functional change
> > > > therefore and shouldn't be in a patch calling itself "generalise
> > > > for
> > > > single ended variants".
> > >
> > > Well yes they're required.
> > >
> > > Should I send a patch adding the `thead,th1520-dc8200` part of the
> > > schema?
> >
> > If you mean the code above, no. Adding a conditional section when
> > there's only that compatible doesn't make sense.
> >
> > What you could do is just add it at the top level though, which would
> > also benefit this patch since it'd not have to be conditionally added
> > for the new nuvoton device.
> > Just note in your commit message about what the ABI impact of the
> > change
> > to required properties is (effectively nothing because it's optional
> > in
> > the driver and the only user has the properties).
>
> Okay, I will craft such a patch and send it.
>
> >
> > > > > +
> > > > > + resets:
> > > > > + minItems: 1
> > > > > + maxItems: 1
> > > > > +
> > > > > + reset-names:
> > > > > + items:
> > > > > + - const: core
> > > >
> > > > This is just maxItems: 1.
> > >
> > > Well the implicit rules of DT binding schemas are quite weird...
> >
> > I don't think it is that strange, as the binding has
> > reset-names:
> > items:
> > - const: core
> > - const: axi
> > - const: ahb
>
> Ah does the list constraint the order of items? If it constrains the
It does, yes.
Alternatively, using an enum permits free ordering.
> order, it partly breaks the intention of having names; if it does not
> constrain the order, it needs to be clarified that the required 1 reset
> is core instead of the other two.
Given the discussion we're having on the clocks, I wonder if this is
also an oversimplification and the IP has three resets inputs hooked up
to one output of the reset controller (or 3 outputs controlled by one
bit..).
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] arm64: tegra: Add Lenovo ThinkEdge SE70 carrier board DTS
From: sashiko-bot @ 2026-06-26 9:28 UTC (permalink / raw)
To: Jiqi Li; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260626091349.570091-3-lijq9@lenovo.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing `no-1-8-v` property or 1.8V-capable `vqmmc-supply` for `mmc@3440000` while UHS modes are enabled in the base DTSI.
- [High] Missing `bus-width` property for the SD card slot (`mmc@3440000`).
- [Medium] Device tree schema violation for 'fixed-regulators' wrapper and its child node.
- [Medium] The patch fails to disable the unused SPI peripheral as claimed in the commit message.
- [Medium] Missing `disable-wp` property in the SD card node (`mmc@3440000`).
--
commit d7f4007a1b474f4e8527dff37fcaa0da82f91d42
Author: Jiqi Li <lijq9@lenovo.com>
arm64: tegra: Add Lenovo ThinkEdge SE70 carrier board DTS
ThinkEdge SE70 is a fanless industrial edge gateway built around
NVIDIA Tegra194 Xavier NX SOM. This patch adds a standalone device
tree file describing core carrier board peripherals.
> No fan, PWM, tachometer, extra camera/spi peripherals are present
> on this passively cooled platform, so unused nodes are explicitly
> disabled following mainline device tree best practices.
[Severity: Medium]
Does this patch actually disable the extra SPI peripherals as claimed?
The tegra194-p3509-0000.dtsi explicitly enables the spi@3270000 node, but
there doesn't appear to be an override to disable it in this file. The
unpopulated SPI flash driver will probe on boot and waste resources.
> diff --git a/arch/arm64/boot/dts/nvidia/tegra194-lenovo-thinkedge-se70.dts b/arch/arm64/boot/dts/nvidia/tegra194-lenovo-thinkedge-se70.dts
> new file mode 100644
> index 0000000000000..44a7ae9a05bfb
> --- /dev/null
> +++ b/arch/arm64/boot/dts/nvidia/tegra194-lenovo-thinkedge-se70.dts
[ ... ]
> @@ -14,6 +14,6 @@
> + /* Fixed 3.3V regulator for external SD card slot */
> + fixed-regulators {
> + compatible = "simple-bus";
> + ap2306gn_3v3_sd: ap2306gn-3v3-sd {
> + compatible = "regulator-fixed";
> + regulator-name = "ap2306gn-3v3-sd";
[Severity: Medium]
Does this fixed-regulators node comply with the device tree schemas?
Using simple-bus requires the #address-cells, #size-cells, and ranges
properties, which are missing here. Additionally, the ap2306gn-3v3-sd child
node does not start with the regulator- prefix as required by the
fixed-regulator.yaml schema.
This seems to contradict the commit message claim that dt_binding_check
passed without errors.
[ ... ]
> @@ -95,6 +95,6 @@
> + /* SDMMC3 for external user SD card slot with dedicated 3.3V power */
> + mmc@3440000 {
> + vmmc-supply = <&ap2306gn_3v3_sd>;
> + cd-gpios = <&gpio_aon TEGRA194_AON_GPIO(CC, 4) GPIO_ACTIVE_HIGH>;
> + status = "okay";
> + };
[Severity: High]
Does this configuration need a no-1-8-v property or a 1.8V-capable
vqmmc-supply?
The tegra194.dtsi enables UHS-I modes for mmc@3440000, which require 1.8V
signaling. Since this node only defines a fixed 3.3V supply without disabling
1.8V modes, won't the MMC core try to switch the SoC pads to 1.8V signaling
while the card is powered at 3.3V? This could cause I/O failures and
hardware overstress.
[Severity: High]
Should this node specify a bus-width property?
If the bus-width property is omitted, the MMC subsystem defaults to 1-bit
mode. Since the parent sdmmc3 node in tegra194.dtsi does not specify a
bus-width, will this SD card operate in a degraded 1-bit mode, artificially
crippling read and write performance?
[Severity: Medium]
Could this node be missing a disable-wp property?
If the board leaves the internal WP pin floating and disable-wp is absent,
could the kernel randomly mount the SD card as read-only?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260626091349.570091-1-lijq9@lenovo.com?part=2
^ permalink raw reply
* Re: [PATCH v11] PCI: Add support for PCIe WAKE# interrupt
From: Bartosz Golaszewski @ 2026-06-26 9:31 UTC (permalink / raw)
To: Krishna Chaitanya Chundru
Cc: linux-pm, linux-kernel, linux-pci, linux-gpio, quic_vbadigan,
sherry.sun, driver-core, devicetree, Manivannan Sadhasivam,
Rafael J. Wysocki, Len Brown, Pavel Machek, Greg Kroah-Hartman,
Danilo Krummrich, Bjorn Helgaas, Bartosz Golaszewski,
Linus Walleij, Bartosz Golaszewski, Rob Herring, Saravana Kannan
In-Reply-To: <20260624-wakeirq_support-v11-1-120fbfaebe59@oss.qualcomm.com>
On Wed, 24 Jun 2026 13:25:49 +0200, Krishna Chaitanya Chundru
<krishna.chundru@oss.qualcomm.com> said:
> According to the PCI Express specification (PCIe r7.0, Section 5.3.3.2),
> two link wakeup mechanisms are defined: Beacon and WAKE#. Beacon is a
> hardware-only mechanism and is invisible to software (PCIe r7.0,
> Section 4.2.7.8.1). This change adds support for the WAKE# mechanism
> in the PCI core.
>
> According to the PCIe specification, multiple WAKE# signals can exist in
> a system or each component in the hierarchy could share a single WAKE#
> signal. In configurations involving a PCIe switch, each downstream port
> (DSP) of the switch may be connected to a separate WAKE# line, allowing
> each endpoint to signal WAKE# independently. From figure 5.4 in sec
> 5.3.3.2, WAKE# can also be terminated at the switch itself. Such topologies
> are typically not described in Device Tree, therefore it is out of scope
> for this series.
>
> To support this, the WAKE# should be described in the device tree node of
> the endpoint/bridge. If all endpoints share a single WAKE# line, then each
> endpoint node shall describe the same WAKE# signal or a single WAKE# in
> the Root Port node.
>
> In pci_device_add(), PCI framework will search for the WAKE# in device
> node. Once found, register for the wake IRQ through
> dev_pm_set_dedicated_wake_irq() associates a wakeup IRQ with a device
> and requests it, but the PM core keeps the IRQ disabled by default. The
> IRQ is enabled by the PM core, only when the device is permitted to wake
> the system, i.e. during system suspend and after runtime suspend, and
> only when device wakeup is enabled.
>
> If the same WAKE# GPIO is described in multiple device tree nodes, only the
> first device that successfully registers the wake IRQ will succeed, while
> subsequent registrations may fail. This limitation does not affect
> functional correctness, since WAKE# is only used to bring the link to D0,
> and endpoint-specific wakeup handling is resolved later through
> PME detection (PME_EN is set in suspend path by PCI core by default).
>
> When the wake IRQ fires, the wakeirq handler invokes pm_runtime_resume() to
> bring the device back to an active power state, such as transitioning from
> D3cold to D0. Once the device is active and the link is usable, the
> endpoint may generate a PME, which is then handled by the PCI core through
> PME polling or the PCIe PME service driver to complete the wakeup of the
> endpoint.
>
> WAKE# is added in dts schema and merged based on below links.
>
> Link: https://lore.kernel.org/all/20250515090517.3506772-1-krishna.chundru@oss.qualcomm.com/
> Link: https://github.com/devicetree-org/dt-schema/pull/170
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Acked-by: Manivannan Sadhasivam <mani@kernel.org>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
^ permalink raw reply
* Re: [PATCH v6 2/2] drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver
From: Maxime Ripard @ 2026-06-26 9:31 UTC (permalink / raw)
To: Sunyun Yang
Cc: Krzysztof Kozlowski, robh, krzk+dt, conor+dt, andrzej.hajda,
neil.armstrong, dmitry.baryshkov, maarten.lankhorst, rfoss,
Laurent.pinchart, tzimmermann, jonas, jernej.skrabec, devicetree,
dri-devel, linux-kernel, xmzhu, xmzhu, rlyu, xbpeng
In-Reply-To: <CAFQXuNYh95wTQex9zEUfxGBCsfK72+8eM3Mzm8pC2CFn6bJdGA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 7572 bytes --]
On Fri, Jun 26, 2026 at 05:05:38PM +0800, Sunyun Yang wrote:
> Sunyun Yang <syyang@lontium.com> 于2026年6月26日周五 16:40写道：
>
> >
> > Maxime Ripard <mripard@kernel.org> 于2026年6月26日周五 16:26写道：
> > >
> > > On Fri, Jun 26, 2026 at 04:13:18PM +0800, Sunyun Yang wrote:
> > > > Maxime Ripard <mripard@kernel.org> 于2026年6月26日周五 15:49写道：
> > > > >
> > > > > On Fri, Jun 26, 2026 at 10:15:03AM +0800, Sunyun Yang wrote:
> > > > > > Krzysztof Kozlowski <krzk@kernel.org> 于2026年6月25日周四 21:51写道：
> > > > > > >
> > > > > > > On 25/06/2026 15:40, Sunyun Yang wrote:
> > > > > > > > Sunyun Yang <syyang@lontium.com> 于2026年6月25日周四 21:26写道：
> > > > > > > >>
> > > > > > > >> Krzysztof Kozlowski <krzk@kernel.org> 于2026年6月25日周四 21:17写道：
> > > > > > > >>>
> > > > > > > >>> On 25/06/2026 15:14, Sunyun Yang wrote:
> > > > > > > >>>> Krzysztof Kozlowski <krzk@kernel.org> 于2026年6月25日周四 20:54写道：
> > > > > > > >>>>>
> > > > > > > >>>>> On 08/05/2026 15:40, syyang@lontium.com wrote:
> > > > > > > >>>>>> +
> > > > > > > >>>>>> +static void lt9611c_reset(struct lt9611c *lt9611c)
> > > > > > > >>>>>> +{
> > > > > > > >>>>>> + gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> > > > > > > >>>>>> + msleep(20);
> > > > > > > >>>>>> +
> > > > > > > >>>>>> + gpiod_set_value_cansleep(lt9611c->reset_gpio, 0);
> > > > > > > >>>>>> + msleep(20);
> > > > > > > >>>>>> +
> > > > > > > >>>>>> + gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> > > > > > > >>>>>
> > > > > > > >>>>> This is just plain wrong. Why do you assert, then de-assert and then
> > > > > > > >>>>> finally assert AGAIN the reset leaving the device in powerdown stage?
> > > > > > > >>>>>
> > > > > > > >>>> I am using software to emulate the hardware RESET button on our EVB.
> > > > > > > >>>> When the hardware RESET button is pressed while our chip is running,
> > > > > > > >>>> the signal level changes from HIGH to LOW and then back to HIGH.
> > > > > > > >>>>
> > > > > > > >>>> Of course, we can also use the following:
> > > > > > > >>>> static void lt9611c_reset(struct lt9611c *lt9611c)
> > > > > > > >>>> {
> > > > > > > >>>> gpiod_set_value_cansleep(lt9611c->reset_gpio, 0);
> > > > > > > >>>> msleep(50);
> > > > > > > >>>> gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> > > > > > > >>>> msleep(20);
> > > > > > > >>>> }
> > > > > > > >>>
> > > > > > > >>> Makes no sense either and you just did not get the point and did not
> > > > > > > >>> answer my question. I asked WHY you leave asserted. Answer "we emulate"
> > > > > > > >>> is just plain wrong.
> > > > > > > >>>
> > > > > > > >>> So again please answer:
> > > > > > > >>>
> > > > > > > >>> Why do you leave device with reset asserted?
> > > > > > > >>>
> > > > > > > >>
> > > > > > > >> devicetree: reset-gpios = <&tlmm 128 GPIO_ACTIVE_HIGH>;
> > > > > > > >>
> > > > > > > >> GPIO_ACTIVE_HIGH:
> > > > > > > >>
> > > > > > > >> gpiod_set_value_cansleep(lt9611c->reset_gpio, 0); ------ reset pin
> > > > > > > >> is Low level : Clear the register configuration in the chip to stop
> > > > > > > >> the chip from working.
> > > > > > > >>
> > > > > > > >> gpiod_set_value_cansleep(lt9611c->reset_gpio, 1); ------ reset pin
> > > > > > > >> is high level: The chip resumes operation.
> > > > > > > >>
> > > > > > > >>
> > > > > > > >
> > > > > > > > Our purpose is: pull the level low to clear the register configuration
> > > > > > > > in the chip, and then pull it high to allow the MCU inside the chip to
> > > > > > > > re‑initialize the registers.
> > > > > > >
> > > > > > >
> > > > > > > And you do completely opposite... so that confirms your code is just wrong.
> > > > > > >
> > > > > >
> > > > > > The lontium-lt9611.yaml uses GPIO_ACTIVE_HIGH. I am just following the
> > > > > > rule of this device tree. If I modify the device tree to use
> > > > > > GPIO_ACTIVE_LOW,
> > > > > > and use the following code in my driver, then my driver would be correct.
> > > > > > However, would the existing kernel drivers lontium-lt9611uxc.c and
> > > > > > lontium-lt9611.c be affected?
> > > > >
> > > > > It might, but then it's a DT problem. The GPIO API for drivers always
> > > > > considers the logical state of a GPIO, so if you need to assert a
> > > > > signal, you'll always need to set 1. That's what Krzysztof was trying to
> > > > > explain.
> > > > >
> > > > > The DT will provide with GPIO_ACTIVE_* how that logical state translates
> > > > > to a physical GPIO state.
> > > > >
> > > > > If the DT says that this particular GPIO is active-high, then it means
> > > > > that we need to set the GPIO to 1 to assert reset. Now of course, it
> > > > > might not make sense for the controller itself, but it might for the
> > > > > board if there's a GPIO inverter in the middle for example.
> > > > >
> > > > > Anyway, in the case you're raising, the issue definitely lies in the DT,
> > > > > and that's what would need to be fixed.
> > > > >
> > > > > I also wouldn't be too concerned about lontium-lt9611.yaml, it's just an
> > > > > example.
> > > > >
> > > > > Maxime
> > > >
> > > > thanks Maxime, I will modify this code in the next version of the
> > > > driver, and I hope you can accept these changes.
> > > >
> > > > Maxime:
> > > > I have another question I would like to ask you
> > > > regarding sashiko-bot@kernel.org. Since sashiko-bot sometimes has
> > > > opinions that differ from yours, whose advice should I follow?
> > > >
> > > > If I do not adopt sashiko-bot's suggestions, will my patches still be
> > > > accepted into the upstream Linux kernel?
> > >
> > > I can't give a blanket answer. It depends on what you ignore exactly.
> > >
> >
> > Okay, another question: sashiko-bot is an AI bot. Are its review
> > comments optional, or must they be followed?
> >
>
> For example, in my driver, there is a function for upgrading the chip
> firmware. During debugging or production, upgrading the chip firmware
> will acquire a lock, which will block the DRM callback and affect
> display. It will be fine after the upgrade is completed and some
> devices are restarted. As long as there is no subsequent upgrade,
> display can work normally.
>
> From a purely software perspective, the AI bot considered this
> approach unacceptable and proposed synchronizing the pre-upgrade state
> to the DRM framework. From my personal perspective, I think the AI
> bot's suggestion would only make my driver more complex and redundant.
> Do you think I need to adopt the AI bot's suggestion?
You're right it's wrong, but there's also no reason to allow that
firmware upgrade during the driver lifetime either through a debugfs
file. So you should get rid of the debugfs file and now you won't have
to bother with that comment anymore.
> In addition, if I follow the AI bot's suggestion, the
> lontium-lt9611uxc.c and lontium-lt8713sx.c drivers that have been
> merged into the upstream Linux kernel would not meet the AI bot's
> requirements.
Maybe, but it's not relevant either way. These drivers are merged, yours
isn't. Feel free to fix these if you spot any issue though.
> When I get a reviewer's Reviewed-by flag, can I ignore the opinion of
> sashiko-bot (the AI bot)?
Again, it depends on the comment. If you feel like one can be ignored,
you should at least argue why.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]
^ permalink raw reply
* Re: [PATCH v2 4/4] PCI: mediatek-gen3: Add 2-lanes mode support for Airoha AN7581
From: sashiko-bot @ 2026-06-26 9:32 UTC (permalink / raw)
To: Christian Marangi; +Cc: linux-pci, robh, devicetree, conor+dt
In-Reply-To: <20260626092029.3525264-5-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] Uninitialized stack variable `num_lanes` leads to unpredictable probe failures
Pre-existing issues:
- [High] The return value of `pm_runtime_get_sync()` is ignored, which can lead to hardware access on suspended devices.
--
commit b24fe645228ac8c1e16f2ba7617e18b13b23173d
Author: Christian Marangi <ansuelsmth@gmail.com>
PCI: mediatek-gen3: Add 2-lanes mode support for Airoha AN7581
This commit adds 2-lanes mode support for the Airoha AN7581 SoC by
configuring the PCIe MUX in the SCU register. It introduces logic to
parse the secondary PCIe MAC region and handle necessary PERSTOUT resets
for the newly supported lane configuration.
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index b0accd8285892..14893cff135a1 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
[ ... ]
> @@ -949,18 +965,38 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
> pcie->num_lanes = num_lanes;
> }
>
> + /* Map secondary PCIe for 2-lanes mode for EN7581 */
> + if (num_lanes == 2 && device_is_compatible(dev, "airoha,en7581-pcie")) {
[Severity: High]
If the num-lanes property is missing from the device tree,
of_property_read_u32() returns an error without modifying the num_lanes output
parameter, leaving it uninitialized. If the stack memory happens to contain 2,
could this enter the block and fail the probe by returning -EINVAL since the
sec-pcie-mac resource would be missing in a 1-lane configuration?
Should this evaluate pcie->num_lanes instead?
> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sec-pcie-mac");
> + if (!regs)
> + return -EINVAL;
> + pcie->sec_base = devm_ioremap_resource(dev, regs);
[ ... ]
> @@ -1024,33 +1073,47 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
> pm_runtime_enable(dev);
> pm_runtime_get_sync(dev);
[Severity: High]
This is a pre-existing issue, but is it safe to ignore the return value of
pm_runtime_get_sync()?
If the runtime PM framework fails to power up or clock the device, accessing
hardware registers directly afterward might cause a synchronous external abort
and kernel panic on ARM platforms. The same pre-existing behavior is also
present in the legacy mtk_pcie_power_up() path.
> - val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) |
> - FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) |
> - FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) |
> - FIELD_PREP(PCIE_VAL_LN1_UPSTREAM, 0x41);
> - writel_relaxed(val, pcie->base + PCIE_EQ_PRESET_01_REG);
> -
> - val = PCIE_K_PHYPARAM_QUERY | PCIE_K_QUERY_TIMEOUT |
> - FIELD_PREP(PCIE_K_PRESET_TO_USE_16G, 0x80) |
> - FIELD_PREP(PCIE_K_PRESET_TO_USE, 0x2) |
> - FIELD_PREP(PCIE_K_FINETUNE_MAX, 0xf);
> - writel_relaxed(val, pcie->base + PCIE_PIPE4_PIE8_REG);
> -
> err = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260626092029.3525264-1-ansuelsmth@gmail.com?part=4
^ permalink raw reply
* Re: [PATCH v2 2/4] clk: en7523: add support for dedicated PCIe PERSTOUT reset
From: sashiko-bot @ 2026-06-26 9:32 UTC (permalink / raw)
To: Christian Marangi; +Cc: conor+dt, linux-pci, robh, devicetree
In-Reply-To: <20260626092029.3525264-3-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [High] Concurrent read-modify-write accesses to the shared register 0x88 between the clock and reset subsystems will cause lost updates.
- [Medium] Removing the PERSTOUT reset deassertion from the clock enable path breaks PCIe initialization for existing Device Trees.
Pre-existing issues:
- [High] Missing spinlock in the reset controller to serialize read-modify-write operations.
- [High] Call to the sleeping function usleep_range() inside the atomic clock disable callback.
- [High] Stale register state written back after sleeping, clobbering concurrent hardware updates.
--
commit f7fec9791d69805c2bca6f3b03a2e7c0e7f0a28c
Author: Christian Marangi <ansuelsmth@gmail.com>
clk: en7523: add support for dedicated PCIe PERSTOUT reset
This commit adds support for resetting the PCIe lines using the PERSTOUT reset
by updating the reset map and moving the handling of these bits from the clock
enable and disable paths to the reset controller framework. The reset logic is
inverted for these specific bits and handled internally.
> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 1ab0e2eca5d32..c9b21d9bf2f3f 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c
[ ... ]
> @@ -635,9 +641,7 @@ static int en7581_pci_enable(struct clk_hw *hw)
> void __iomem *np_base = cg->base;
> u32 val, mask;
>
> - mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1 |
> - REG_PCI_CONTROL_PERSTOUT1 | REG_PCI_CONTROL_PERSTOUT2 |
> - REG_PCI_CONTROL_PERSTOUT;
> + mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1;
[Severity: Medium]
Does removing the PERSTOUT reset deassertion from the clock enable path
break PCIe initialization for existing Device Trees?
When booting with an older DTB built before the resets property was updated
to explicitly list the PERSTOUT lines, the clock driver will no longer
automatically clear these resets in en7581_pci_enable(). Since the PCIe
driver also lacks references to them in the older DTB, it seems the PCIe
controller will remain in reset.
> val = readl(np_base + REG_PCI_CONTROL);
> writel(val | mask, np_base + REG_PCI_CONTROL);
[Severity: High]
Can concurrent read-modify-write accesses to this shared register between
the clock and reset subsystems cause lost updates?
The clock API protects operations with its global enable_lock, while the
reset framework operates lockless at the driver level. If PCIe host 0 enables
its clock in en7581_pci_enable() concurrently with PCIe host 1 toggling its
reset in en7523_reset_update(), their read-modify-write operations on the
exact same physical register (REG_PCI_CONTROL / REG_NP_SCU_PCIC at offset
0x88) will race and silently overwrite each other.
[ ... ]
> @@ -650,9 +654,7 @@ static void en7581_pci_disable(struct clk_hw *hw)
> void __iomem *np_base = cg->base;
> u32 val, mask;
>
> - mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1 |
> - REG_PCI_CONTROL_PERSTOUT1 | REG_PCI_CONTROL_PERSTOUT2 |
> - REG_PCI_CONTROL_PERSTOUT;
> + mask = REG_PCI_CONTROL_REFCLK_EN0 | REG_PCI_CONTROL_REFCLK_EN1;
> val = readl(np_base + REG_PCI_CONTROL);
> writel(val & ~mask, np_base + REG_PCI_CONTROL);
> usleep_range(1000, 2000);
[Severity: High]
This is a pre-existing issue, but is it safe to call the sleeping function
usleep_range() inside the atomic clock disable callback?
The clk_ops->disable callback executes in atomic context with the clock
framework's enable_lock spinlock held and local interrupts disabled. Calling
usleep_range() here attempts a context switch, which will result in a
scheduling while atomic bug.
[ ... ]
> @@ -754,14 +756,21 @@ static int en7523_reset_update(struct reset_controller_dev *rcdev,
> unsigned long id, bool assert)
> {
> struct en_rst_data *rst_data = container_of(rcdev, struct en_rst_data, rcdev);
> - void __iomem *addr = rst_data->base + rst_data->bank_ofs[id / RST_NR_PER_BANK];
> + u32 offset = rst_data->bank_ofs[id / RST_NR_PER_BANK];
> + void __iomem *addr = rst_data->base + offset;
> + bool inverted = false;
> u32 val;
>
> + /* For PCIC reset logic is inverted, 0:assert 1:deassert*/
> + if (offset == REG_NP_SCU_PCIC)
> + inverted = true;
> +
> val = readl(addr);
> + val &= ~BIT(id % RST_NR_PER_BANK);
> if (assert)
> - val |= BIT(id % RST_NR_PER_BANK);
> + val |= inverted ? 0 : BIT(id % RST_NR_PER_BANK);
> else
> - val &= ~BIT(id % RST_NR_PER_BANK);
> + val |= inverted ? BIT(id % RST_NR_PER_BANK) : 0;
> writel(val, addr);
[Severity: High]
This isn't a bug introduced by this patch, but does this function need a
spinlock to serialize the read-modify-write operations?
The en7523_reset_update() callback performs unguarded read-modify-writes on
shared reset banks. If two distinct device drivers (such as I2C and PCIe)
probe concurrently and assert or deassert their respective resets, the
lockless read-modify-write cycles will race, overwriting one of the
operations and leading to silent hardware state corruption.
[ ... ]
[Severity: High]
This isn't a bug introduced by this patch, but could the caching of register
state in en7523_pci_prepare() clobber concurrent hardware updates?
val = readl(np_base + REG_RESET_CONTROL1);
mask = REG_RESET_CONTROL_PCIE1 | REG_RESET_CONTROL_PCIE2 | REG_RESET_CONTROL_PCIEHB;
writel(val & ~mask, np_base + REG_RESET_CONTROL1);
usleep_range(1000, 2000);
writel(val | mask, np_base + REG_RESET_CONTROL1);
The cached register state val is re-used to write after a sleep. If any
other driver toggles a reset bit in the same register (which controls up to
32 different resets) during the sleep duration, their reset state will be
unconditionally reverted and corrupted by this stale writeback.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260626092029.3525264-1-ansuelsmth@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH v5 1/7] dt-bindings: display: verisilicon,dc: generalize for single-output variants
From: Icenowy Zheng @ 2026-06-26 9:33 UTC (permalink / raw)
To: Conor Dooley
Cc: Conor Dooley, Joey Lu, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, robh, krzk+dt, conor+dt, ychuang3, schung, yclu4,
dri-devel, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20260626-everybody-epilogue-8fb298a54981@wendy>
在 2026-06-26五的 10:26 +0100，Conor Dooley写道：
> On Fri, Jun 26, 2026 at 05:00:35PM +0800, Icenowy Zheng wrote:
> > 在 2026-06-26五的 08:19 +0100，Conor Dooley写道：
> > > On Fri, Jun 26, 2026 at 01:27:21PM +0800, Icenowy Zheng wrote:
> > > > 在 2026-06-25四的 17:33 +0100，Conor Dooley写道：
> > > > > On Thu, Jun 25, 2026 at 05:44:43PM +0800, Joey Lu wrote:
> > > > > > +allOf:
> > > > > > + - if:
> > > > > > + properties:
> > > > > > + compatible:
> > > > > > + contains:
> > > > > > + const: thead,th1520-dc8200
> > > > > > + then:
> > > > > > + properties:
> > > > > > + clocks:
> > > > > > + minItems: 5
> > > > > > + maxItems: 5
> > > > > > +
> > > > > > + clock-names:
> > > > > > + minItems: 5
> > > > > > + maxItems: 5
> > > > >
> > > > > All the maxItems here repeat the maximum constraint and do
> > > > > nothing.
> > > > >
> > > > > Since you didn't change the minimum constraint at the top
> > > > > level,
> > > > > your
> > > > > minItems also do nothing.
> > > > >
> > > > > > +
> > > > > > + resets:
> > > > > > + minItems: 3
> > > > > > + maxItems: 3
> > > > > > +
> > > > > > + reset-names:
> > > > > > + minItems: 3
> > > > > > + maxItems: 3
> > > > > > +
> > > > > > + required:
> > > > > > + - resets
> > > > > > + - reset-names
> > > > >
> > > > > Both conditional sections have this, but the original binding
> > > > > doesn't
> > > > > require these for the thead device. This is a functional
> > > > > change
> > > > > therefore and shouldn't be in a patch calling itself
> > > > > "generalise
> > > > > for
> > > > > single ended variants".
> > > >
> > > > Well yes they're required.
> > > >
> > > > Should I send a patch adding the `thead,th1520-dc8200` part of
> > > > the
> > > > schema?
> > >
> > > If you mean the code above, no. Adding a conditional section when
> > > there's only that compatible doesn't make sense.
> > >
> > > What you could do is just add it at the top level though, which
> > > would
> > > also benefit this patch since it'd not have to be conditionally
> > > added
> > > for the new nuvoton device.
> > > Just note in your commit message about what the ABI impact of the
> > > change
> > > to required properties is (effectively nothing because it's
> > > optional
> > > in
> > > the driver and the only user has the properties).
> >
> > Okay, I will craft such a patch and send it.
> >
> > >
> > > > > > +
> > > > > > + resets:
> > > > > > + minItems: 1
> > > > > > + maxItems: 1
> > > > > > +
> > > > > > + reset-names:
> > > > > > + items:
> > > > > > + - const: core
> > > > >
> > > > > This is just maxItems: 1.
> > > >
> > > > Well the implicit rules of DT binding schemas are quite
> > > > weird...
> > >
> > > I don't think it is that strange, as the binding has
> > > reset-names:
> > > items:
> > > - const: core
> > > - const: axi
> > > - const: ahb
> >
> > Ah does the list constraint the order of items? If it constrains
> > the
>
> It does, yes.
> Alternatively, using an enum permits free ordering.
Ah in this case this should be converted to an enum, I think.
Should I send a patch for converting it?
Thanks,
Icenowy
>
> > order, it partly breaks the intention of having names; if it does
> > not
> > constrain the order, it needs to be clarified that the required 1
> > reset
> > is core instead of the other two.
>
> Given the discussion we're having on the clocks, I wonder if this is
> also an oversimplification and the IP has three resets inputs hooked
> up
> to one output of the reset controller (or 3 outputs controlled by one
> bit..).
^ permalink raw reply
* Re: [PATCH v6 2/2] drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver
From: Sunyun Yang @ 2026-06-26 9:45 UTC (permalink / raw)
To: Maxime Ripard
Cc: Krzysztof Kozlowski, robh, krzk+dt, conor+dt, andrzej.hajda,
neil.armstrong, dmitry.baryshkov, maarten.lankhorst, rfoss,
Laurent.pinchart, tzimmermann, jonas, jernej.skrabec, devicetree,
dri-devel, linux-kernel, xmzhu, xmzhu, rlyu, xbpeng
In-Reply-To: <20260626-tungsten-capuchin-of-serendipity-24c0a9@houat>
Maxime Ripard <mripard@kernel.org> 于2026年6月26日周五 17:31写道：
>
> On Fri, Jun 26, 2026 at 05:05:38PM +0800, Sunyun Yang wrote:
> > Sunyun Yang <syyang@lontium.com> 于2026年6月26日周五 16:40写道：
> >
> > >
> > > Maxime Ripard <mripard@kernel.org> 于2026年6月26日周五 16:26写道：
> > > >
> > > > On Fri, Jun 26, 2026 at 04:13:18PM +0800, Sunyun Yang wrote:
> > > > > Maxime Ripard <mripard@kernel.org> 于2026年6月26日周五 15:49写道：
> > > > > >
> > > > > > On Fri, Jun 26, 2026 at 10:15:03AM +0800, Sunyun Yang wrote:
> > > > > > > Krzysztof Kozlowski <krzk@kernel.org> 于2026年6月25日周四 21:51写道：
> > > > > > > >
> > > > > > > > On 25/06/2026 15:40, Sunyun Yang wrote:
> > > > > > > > > Sunyun Yang <syyang@lontium.com> 于2026年6月25日周四 21:26写道：
> > > > > > > > >>
> > > > > > > > >> Krzysztof Kozlowski <krzk@kernel.org> 于2026年6月25日周四 21:17写道：
> > > > > > > > >>>
> > > > > > > > >>> On 25/06/2026 15:14, Sunyun Yang wrote:
> > > > > > > > >>>> Krzysztof Kozlowski <krzk@kernel.org> 于2026年6月25日周四 20:54写道：
> > > > > > > > >>>>>
> > > > > > > > >>>>> On 08/05/2026 15:40, syyang@lontium.com wrote:
> > > > > > > > >>>>>> +
> > > > > > > > >>>>>> +static void lt9611c_reset(struct lt9611c *lt9611c)
> > > > > > > > >>>>>> +{
> > > > > > > > >>>>>> + gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> > > > > > > > >>>>>> + msleep(20);
> > > > > > > > >>>>>> +
> > > > > > > > >>>>>> + gpiod_set_value_cansleep(lt9611c->reset_gpio, 0);
> > > > > > > > >>>>>> + msleep(20);
> > > > > > > > >>>>>> +
> > > > > > > > >>>>>> + gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> > > > > > > > >>>>>
> > > > > > > > >>>>> This is just plain wrong. Why do you assert, then de-assert and then
> > > > > > > > >>>>> finally assert AGAIN the reset leaving the device in powerdown stage?
> > > > > > > > >>>>>
> > > > > > > > >>>> I am using software to emulate the hardware RESET button on our EVB.
> > > > > > > > >>>> When the hardware RESET button is pressed while our chip is running,
> > > > > > > > >>>> the signal level changes from HIGH to LOW and then back to HIGH.
> > > > > > > > >>>>
> > > > > > > > >>>> Of course, we can also use the following:
> > > > > > > > >>>> static void lt9611c_reset(struct lt9611c *lt9611c)
> > > > > > > > >>>> {
> > > > > > > > >>>> gpiod_set_value_cansleep(lt9611c->reset_gpio, 0);
> > > > > > > > >>>> msleep(50);
> > > > > > > > >>>> gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> > > > > > > > >>>> msleep(20);
> > > > > > > > >>>> }
> > > > > > > > >>>
> > > > > > > > >>> Makes no sense either and you just did not get the point and did not
> > > > > > > > >>> answer my question. I asked WHY you leave asserted. Answer "we emulate"
> > > > > > > > >>> is just plain wrong.
> > > > > > > > >>>
> > > > > > > > >>> So again please answer:
> > > > > > > > >>>
> > > > > > > > >>> Why do you leave device with reset asserted?
> > > > > > > > >>>
> > > > > > > > >>
> > > > > > > > >> devicetree: reset-gpios = <&tlmm 128 GPIO_ACTIVE_HIGH>;
> > > > > > > > >>
> > > > > > > > >> GPIO_ACTIVE_HIGH:
> > > > > > > > >>
> > > > > > > > >> gpiod_set_value_cansleep(lt9611c->reset_gpio, 0); ------ reset pin
> > > > > > > > >> is Low level : Clear the register configuration in the chip to stop
> > > > > > > > >> the chip from working.
> > > > > > > > >>
> > > > > > > > >> gpiod_set_value_cansleep(lt9611c->reset_gpio, 1); ------ reset pin
> > > > > > > > >> is high level: The chip resumes operation.
> > > > > > > > >>
> > > > > > > > >>
> > > > > > > > >
> > > > > > > > > Our purpose is: pull the level low to clear the register configuration
> > > > > > > > > in the chip, and then pull it high to allow the MCU inside the chip to
> > > > > > > > > re‑initialize the registers.
> > > > > > > >
> > > > > > > >
> > > > > > > > And you do completely opposite... so that confirms your code is just wrong.
> > > > > > > >
> > > > > > >
> > > > > > > The lontium-lt9611.yaml uses GPIO_ACTIVE_HIGH. I am just following the
> > > > > > > rule of this device tree. If I modify the device tree to use
> > > > > > > GPIO_ACTIVE_LOW,
> > > > > > > and use the following code in my driver, then my driver would be correct.
> > > > > > > However, would the existing kernel drivers lontium-lt9611uxc.c and
> > > > > > > lontium-lt9611.c be affected?
> > > > > >
> > > > > > It might, but then it's a DT problem. The GPIO API for drivers always
> > > > > > considers the logical state of a GPIO, so if you need to assert a
> > > > > > signal, you'll always need to set 1. That's what Krzysztof was trying to
> > > > > > explain.
> > > > > >
> > > > > > The DT will provide with GPIO_ACTIVE_* how that logical state translates
> > > > > > to a physical GPIO state.
> > > > > >
> > > > > > If the DT says that this particular GPIO is active-high, then it means
> > > > > > that we need to set the GPIO to 1 to assert reset. Now of course, it
> > > > > > might not make sense for the controller itself, but it might for the
> > > > > > board if there's a GPIO inverter in the middle for example.
> > > > > >
> > > > > > Anyway, in the case you're raising, the issue definitely lies in the DT,
> > > > > > and that's what would need to be fixed.
> > > > > >
> > > > > > I also wouldn't be too concerned about lontium-lt9611.yaml, it's just an
> > > > > > example.
> > > > > >
> > > > > > Maxime
> > > > >
> > > > > thanks Maxime, I will modify this code in the next version of the
> > > > > driver, and I hope you can accept these changes.
> > > > >
> > > > > Maxime:
> > > > > I have another question I would like to ask you
> > > > > regarding sashiko-bot@kernel.org. Since sashiko-bot sometimes has
> > > > > opinions that differ from yours, whose advice should I follow?
> > > > >
> > > > > If I do not adopt sashiko-bot's suggestions, will my patches still be
> > > > > accepted into the upstream Linux kernel?
> > > >
> > > > I can't give a blanket answer. It depends on what you ignore exactly.
> > > >
> > >
> > > Okay, another question: sashiko-bot is an AI bot. Are its review
> > > comments optional, or must they be followed?
> > >
> >
> > For example, in my driver, there is a function for upgrading the chip
> > firmware. During debugging or production, upgrading the chip firmware
> > will acquire a lock, which will block the DRM callback and affect
> > display. It will be fine after the upgrade is completed and some
> > devices are restarted. As long as there is no subsequent upgrade,
> > display can work normally.
> >
> > From a purely software perspective, the AI bot considered this
> > approach unacceptable and proposed synchronizing the pre-upgrade state
> > to the DRM framework. From my personal perspective, I think the AI
> > bot's suggestion would only make my driver more complex and redundant.
> > Do you think I need to adopt the AI bot's suggestion?
>
> You're right it's wrong, but there's also no reason to allow that
> firmware upgrade during the driver lifetime either through a debugfs
> file. So you should get rid of the debugfs file and now you won't have
> to bother with that comment anymore.
>
> > In addition, if I follow the AI bot's suggestion, the
> > lontium-lt9611uxc.c and lontium-lt8713sx.c drivers that have been
> > merged into the upstream Linux kernel would not meet the AI bot's
> > requirements.
>
> Maybe, but it's not relevant either way. These drivers are merged, yours
> isn't. Feel free to fix these if you spot any issue though.
>
> > When I get a reviewer's Reviewed-by flag, can I ignore the opinion of
> > sashiko-bot (the AI bot)?
>
> Again, it depends on the comment. If you feel like one can be ignored,
> you should at least argue why.
>
sashiko‑bot (AI bot) does not communicate or interact with me.
Even if I explain the reasons, the AI bot never replies to me, and it
only sends its review comments to me alone.
If I do not make changes according to the AI bot's suggestions, will
that affect my driver being merged into the upstream Linux kernel? I
would like to know the answer to this question.
> Maxime
^ permalink raw reply
* Re: [PATCH 1/5] media: i2c: vd55g1: Fix media bus code initialization
From: Benjamin Mugnier @ 2026-06-26 9:48 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Sylvain Petinot, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Hans Verkuil, linux-media,
linux-kernel, devicetree
In-Reply-To: <aj4u42ppuHSRqLIn@zed>
Hi Jacopo,
Le 26/06/2026 à 09:55, Jacopo Mondi a écrit :
> Hi Benjamin
>
> On Thu, Jun 25, 2026 at 01:41:48PM +0200, Benjamin Mugnier wrote:
>> Hi Jacopo,
>>
>> Thank you for your review.
>>
>> Le 22/06/2026 à 11:28, Jacopo Mondi a écrit :
>>> Hi Benjamin
>>>
>>> On Tue, Apr 28, 2026 at 10:40:55AM +0200, Benjamin Mugnier wrote:
>>>> In the driver initialization, the index of the default media bus code
>>>> from the supported media bus code array is passed directly to the
>>>> vd55g1_get_fmt_code() function instead of the proper media bus code.
>>>>
>>>> This works correctly as a proper media bus code is set after
>>>> initialization but could not have been the case. This also resulted in
>>>> mutliple "Unsupported mbus format" error messages.
>>>>
>>>> Retrieve the media bus code from the media bus code array, and pass this
>>>> media bus code to vd55g1_get_fmt_code() instead of the code index.
>>>>
>>>> Rename VD55G1_MBUS_CODE_DEF to VD55G1_MBUS_CODE_IDX_DEF and
>>>> VD55G1_MODE_DEF to VD55G1_MODE_IDX_DEF while at it to avoid future
>>>> confusions. Display the guilty error code in warning message.
>>>>
>>>> Fixes: e138e7f00042 ("media: i2c: vd55g1: Add support for vd65g4 RGB variant")
>>>>
>>> You should cc stable for fixes
>>>
>>> Cc: stable@vger.kernel.org
>>>
>>
>> We talked about this very recently and somehow I still forgot.
>>
>>>
>>> The CI should have flagged that, but for some reason it didn't run
>>> properly on your series
>>> https://gitlab.freedesktop.org/linux-media/users/patchwork/-/pipelines/1655147
>>>
>>>> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>>>> ---
>>>> drivers/media/i2c/vd55g1.c | 17 +++++++++++------
>>>> 1 file changed, 11 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
>>>> index 78d18c028154..1e9db21322e3 100644
>>>> --- a/drivers/media/i2c/vd55g1.c
>>>> +++ b/drivers/media/i2c/vd55g1.c
>>>> @@ -114,9 +114,9 @@
>>>>
>>>> #define VD55G1_WIDTH 804
>>>> #define VD55G1_HEIGHT 704
>>>> -#define VD55G1_MODE_DEF 0
>>>> +#define VD55G1_MODE_IDX_DEF 0
>>>> #define VD55G1_NB_GPIOS 4
>>>> -#define VD55G1_MBUS_CODE_DEF 0
>>>> +#define VD55G1_MBUS_CODE_IDX_DEF 0
>>>> #define VD55G1_DGAIN_DEF 256
>>>> #define VD55G1_AGAIN_DEF 19
>>>> #define VD55G1_EXPO_MAX_TERM 64
>>>> @@ -634,7 +634,7 @@ static u32 vd55g1_get_fmt_code(struct vd55g1 *sensor, u32 code)
>>>
>>> Unrelated, but it seems you now have 2 codes for MONO. Does
>>>
>>> if (sensor->id == VD55G1_MODEL_ID_VD55G1)
>>> return code;
>>>
>>> need an update ?>
>>
>> Not in this patch because it does not add the new MONO sensor, but in
>
> Not in this patch ofc
>
>> 4/5 I separated the model ID from the color code. Example for the vd55g4 :
>>
>> .name = "vd55g4",
>> .id = VD55G1_MODEL_ID_3,
>> .color = VD55G1_COLOR_VERSION_MONO,
>>
>> So the patch 4/5 updates the previous 'if' you mentioned to check the
>> color member instead of the model :
>>
>> if (sensor->version->color != VD55G1_COLOR_VERSION_BAYER)
>>
>> Which IMO is a good way to handle this problematic. Tell me if you're
>> thinking about something else.
>>
>
> Yes, I was thinking about the unconditional return of 'code'.
>
> You know have two mono codes, shouldn't you do the same as you do with
> the bayer ones ?
>
Ah, got you. You're correct, I thought it was not possible to pass a
code that is not supported by the driver by design. I'll add the same
check for mono codes.
>>>> goto adapt_bayer_pattern;
>>>> }
>>>> }
>>>> - dev_warn(sensor->dev, "Unsupported mbus format\n");
>>>> + dev_warn(sensor->dev, "Unsupported mbus format: 0x%x\n", code);
>>>>
>>>> return code;
>>>>
>>>> @@ -1347,6 +1347,7 @@ static int vd55g1_init_state(struct v4l2_subdev *sd,
>>>> {
>>>> struct vd55g1 *sensor = to_vd55g1(sd);
>>>> struct v4l2_subdev_format fmt = { 0 };
>>>> + int code;
>>>> struct v4l2_subdev_route routes[] = {
>>>> { .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE }
>>>> };
>>>> @@ -1361,9 +1362,13 @@ static int vd55g1_init_state(struct v4l2_subdev *sd,
>>>> if (ret)
>>>> return ret;
>>>>
>>>> - vd55g1_update_pad_fmt(sensor, &vd55g1_supported_modes[VD55G1_MODE_DEF],
>>>> - vd55g1_get_fmt_code(sensor, VD55G1_MBUS_CODE_DEF),
>>>> - &fmt.format);
>>>> + if (sensor->id == VD55G1_MODEL_ID_VD55G1)
>>>> + code = vd55g1_mbus_formats_mono[VD55G1_MBUS_CODE_IDX_DEF];
>>>> + else
>>>> + code = vd55g1_mbus_formats_bayer[VD55G1_MBUS_CODE_IDX_DEF][0];
>>>
>>> Being this a multi-dimensional array, I don't seem much value in
>>> defining VD55G1_MBUS_CODE_IDX_DEF if this is the only place where it
>>> is used. What's the meaning of VD55G1_MBUS_CODE_IDX_DEF for
>>> vd55g1_mbus_formats_bayer ? Does it represent the bitwidth or does it
>>> represent the bayer pattern ?
>>
>> For vd55g1_mbus_formats_bayer, the first dimension of the array is the
>> bitwidth, and the second one is the bayer pattern.
>>
>>>
>>> I would rather define a
>>> VD55G1_DEF_MBUS_CODE_MONO MEDIA_BUS_FMT_Y8_1X8
>>> VD55G1_DEF_MBUS_CODE_BAYER MEDIA_BUS_FMT_SRGGB8_1X8
>>>
>>> Or maybe do
>>>
>>> code = vd55g1_mbus_formats_bayer[VD55G1_MBUS_CODE_IDX_DEF]
>>> [VD55G1_MBUS_CODE_IDX_DEF];
>>>
>>> if easier.
>>>
>>> I understand it's a minor, so up to you.
>>
>> As you mentioned it's only used here. I won't mind removing
>> VD55G1_MBUS_CODE_IDX_DEF entirely and do :
>>
>> code = vd55g1_mbus_formats_bayer[0][0];
>>
>> Does that sound okay ?
>>
>
> It does, thanks
>
>>>
>>>
>>>
>>>> + vd55g1_update_pad_fmt(sensor,
>>>> + &vd55g1_supported_modes[VD55G1_MODE_IDX_DEF],
>>>> + vd55g1_get_fmt_code(sensor, code), &fmt.format);
>>>>
>>>> return vd55g1_set_pad_fmt(sd, sd_state, &fmt);
>>>> }
>>>>
>>>> --
>>>> 2.43.0
>>>>
>>>>
>>
>> --
>> Regards,
>> Benjamin
>>
--
Regards,
Benjamin
^ permalink raw reply
* Re: [PATCH 4/5] media: i2c: vd55g1: Add support for vd55g4
From: Benjamin Mugnier @ 2026-06-26 9:49 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Sylvain Petinot, Sakari Ailus, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Hans Verkuil, linux-media,
linux-kernel, devicetree
In-Reply-To: <aj4xDDy0UubUzCVx@zed>
Hi Jacopo,
Le 26/06/2026 à 09:58, Jacopo Mondi a écrit :
> Hi Benjamin
>
> On Thu, Jun 25, 2026 at 01:41:56PM +0200, Benjamin Mugnier wrote:
>> Hi Jacopo,
>>
>> Le 22/06/2026 à 12:16, Jacopo Mondi a écrit :
>>> Hi Benjamin
>>>
>>> On Tue, Apr 28, 2026 at 10:40:58AM +0200, Benjamin Mugnier wrote:
>>>> vd55g4 is the same device as vd65g4 but outputs in monochrome instead of
>>>> RGB. Adapt the driver structure according to this new variant, and add
>>>> its support.
>>>>
>>>> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>>>> ---
>>>> drivers/media/i2c/vd55g1.c | 110 ++++++++++++++++++++++++++++++---------------
>>>> 1 file changed, 74 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
>>>> index 2c962fcb41d2..9f62fc0428a1 100644
>>>> --- a/drivers/media/i2c/vd55g1.c
>>>> +++ b/drivers/media/i2c/vd55g1.c
>>>> @@ -29,11 +29,7 @@
>>>>
>>>> /* Register Map */
>>>> #define VD55G1_REG_MODEL_ID CCI_REG32_LE(0x0000)
>>>> -#define VD55G1_MODEL_ID_VD55G1 0x53354731 /* Mono */
>>>> -#define VD55G1_MODEL_ID_VD65G4 0x53354733 /* RGB */
>>>> -#define VD55G1_REG_REVISION CCI_REG16_LE(0x0004)
>>>> -#define VD55G1_REVISION_CCB 0x2020
>>>> -#define VD55G1_REVISION_BAYER 0x3030
>>>> +#define VD55G1_REG_COLOR_VERSION CCI_REG32_LE(0x0670)
>>>> #define VD55G1_REG_FWPATCH_REVISION CCI_REG16_LE(0x0012)
>>>> #define VD55G1_REG_FWPATCH_START_ADDR CCI_REG8(0x2000)
>>>> #define VD55G1_REG_SYSTEM_FSM CCI_REG8(0x001c)
>>>> @@ -138,8 +134,39 @@
>>>> #define VD55G1_MIPI_RATE_MIN (250 * MEGA)
>>>> #define VD55G1_MIPI_RATE_MAX (1200 * MEGA)
>>>>
>>>> -#define VD55G1_MODEL_ID_NAME(id) \
>>>> - ((id) == VD55G1_MODEL_ID_VD55G1 ? "vd55g1" : "vd65g4")
>>>> +enum vd55g1_model_id {
>>>> + VD55G1_MODEL_ID_2 = 0x53354731,
>>>> + VD55G1_MODEL_ID_3 = 0x53354733,
>>>> +};
>>>> +
>>>> +enum vd55g1_color_version {
>>>> + VD55G1_COLOR_VERSION_MONO = 0x0,
>>>> + VD55G1_COLOR_VERSION_BAYER = 0x1,
>>>
>>> nit: you don't need to initialize the enum members here
>>
>> Thank you, will do.
>>
>>>
>>>> +};
>>>> +
>>>> +struct vd55g1_version {
>>>> + char *name;
>>>> + enum vd55g1_model_id id;
>>>> + enum vd55g1_color_version color;
>>>> +};
>>>> +
>>>> +static const struct vd55g1_version vd55g1_versions[] = {
>>>> + {
>>>> + .name = "vd55g1",
>>>> + .id = VD55G1_MODEL_ID_2,
>>>> + .color = VD55G1_COLOR_VERSION_MONO,
>>>> + },
>>>> + {
>>>> + .name = "vd55g4",
>>>> + .id = VD55G1_MODEL_ID_3,
>>>> + .color = VD55G1_COLOR_VERSION_MONO,
>>>> + },
>>>> + {
>>>> + .name = "vd65g4",
>>>> + .id = VD55G1_MODEL_ID_3,
>>>> + .color = VD55G1_COLOR_VERSION_BAYER,
>>>> + },
>>>> +};
>>>>
>>>> static const u8 vd55g1_patch_array[] = {
>>>> 0x44, 0x03, 0x09, 0x02, 0xe6, 0x01, 0x42, 0x00, 0xea, 0x01, 0x42, 0x00,
>>>> @@ -535,7 +562,7 @@ struct vd55g1_vblank_limits {
>>>>
>>>> struct vd55g1 {
>>>> struct device *dev;
>>>> - unsigned int id;
>>>> + const struct vd55g1_version *version;
>>>> struct v4l2_subdev sd;
>>>> struct media_pad pad;
>>>> struct regulator_bulk_data supplies[ARRAY_SIZE(vd55g1_supply_name)];
>>>> @@ -628,7 +655,7 @@ static u32 vd55g1_get_fmt_code(struct vd55g1 *sensor, u32 code)
>>>> {
>>>> unsigned int i, j;
>>>>
>>>> - if (sensor->id == VD55G1_MODEL_ID_VD55G1)
>>>> + if (sensor->version->color != VD55G1_COLOR_VERSION_BAYER)
>>>> return code;
>>>
>>> As pointed out in the previous patch, you seem to have 2 mono formats.
>>> Is this still ok ?
>>>
>>>>
>>>> for (i = 0; i < ARRAY_SIZE(vd55g1_mbus_formats_bayer); i++) {
>>>> @@ -1183,8 +1210,8 @@ static int vd55g1_patch(struct vd55g1 *sensor)
>>>> u64 patch;
>>>> int ret = 0;
>>>>
>>>> - /* vd55g1 needs a patch while vd65g4 does not */
>>>> - if (sensor->id == VD55G1_MODEL_ID_VD55G1) {
>>>> + /* Version 2 needs a patch while version 3 does not */
>>>> + if (sensor->version->id == VD55G1_MODEL_ID_2) {
>>>> vd55g1_write_array(sensor, VD55G1_REG_FWPATCH_START_ADDR,
>>>> sizeof(vd55g1_patch_array),
>>>> vd55g1_patch_array, &ret);
>>>
>>> You might want to consider renaming vd55g1_patch_array ?
>>
>> It doesn't really patch an array, it writes an array of values into
>
> My only point here was that it has 'vd55g1' in the name.
Sorry I thought you were talking about vd55g1_write_array() and not
vd55g1_patch_array for some reason.
My rule of thumb is that a static global variable must always be
prefixed by the driver name to avoid collisions. What rename are you
thinking about ?
>
>> continuous i2c registers. Just like before converting to cci_write() I
>> used to have a vd55g1_write() function to write a register, I derived a
>> vd55g1_write_array() function. Here it happens to be a firmware patch,
>> but it could be anything you want ;)
>>
>>>
>>>> @@ -1256,7 +1283,7 @@ static int vd55g1_enum_mbus_code(struct v4l2_subdev *sd,
>>>> struct vd55g1 *sensor = to_vd55g1(sd);
>>>> u32 base_code;
>>>>
>>>> - if (sensor->id == VD55G1_MODEL_ID_VD55G1) {
>>>> + if (sensor->version->color != VD55G1_COLOR_VERSION_BAYER) {
>>>> if (code->index >= ARRAY_SIZE(vd55g1_mbus_formats_mono))
>>>> return -EINVAL;
>>>> base_code = vd55g1_mbus_formats_mono[code->index];
>>>> @@ -1372,7 +1399,7 @@ static int vd55g1_init_state(struct v4l2_subdev *sd,
>>>> if (ret)
>>>> return ret;
>>>>
>>>> - if (sensor->id == VD55G1_MODEL_ID_VD55G1)
>>>> + if (sensor->version->color != VD55G1_COLOR_VERSION_BAYER)
>>>> code = vd55g1_mbus_formats_mono[VD55G1_MBUS_CODE_IDX_DEF];
>>>> else
>>>> code = vd55g1_mbus_formats_bayer[VD55G1_MBUS_CODE_IDX_DEF][0];
>>>> @@ -1659,38 +1686,48 @@ static int vd55g1_init_ctrls(struct vd55g1 *sensor)
>>>> return ret;
>>>> }
>>>>
>>>> +static const struct vd55g1_version *
>>>> + vd55g1_get_version(enum vd55g1_model_id id,
>>>> + enum vd55g1_color_version color)
>>>
>>> Should you indent one tab left ?
>>>
>>
>> checkpatch.pl is fine with both. I can indent left, it looks cleaner.
>>
>>>> +{
>>>> + unsigned int i;
>>>> +
>>>> + for (i = 0; i < ARRAY_SIZE(vd55g1_versions); i++) {
>>>
>>> You can declare i inside the for loop
>>>
>>
>> Yes, thank you.
>>
>>>> + if (vd55g1_versions[i].id == id &&
>>>> + vd55g1_versions[i].color == color)
>>>> + return &vd55g1_versions[i];
>>>> + }
>>>> +
>>>> + return NULL;
>>>> +}
>>>> +
>>>> static int vd55g1_detect(struct vd55g1 *sensor)
>>>> {
>>>> - unsigned int dt_id = (uintptr_t)device_get_match_data(sensor->dev);
>>>> - u64 rev, id;
>>>> - int ret;
>>>> + const struct vd55g1_version *dt_version =
>>>> + device_get_match_data(sensor->dev);
>>>> + const struct vd55g1_version *version;
>>>> + u64 color, id;
>>>> + int ret = 0;
>>>>
>>>> - ret = vd55g1_read(sensor, VD55G1_REG_MODEL_ID, &id, NULL);
>>>> + vd55g1_read(sensor, VD55G1_REG_MODEL_ID, &id, &ret);
>>>> + vd55g1_read(sensor, VD55G1_REG_COLOR_VERSION, &color, &ret);
>>>> if (ret)
>>>> return ret;
>>>>
>>>> - if (id != VD55G1_MODEL_ID_VD55G1 && id != VD55G1_MODEL_ID_VD65G4) {
>>>> - dev_warn(sensor->dev, "Unsupported sensor id 0x%x\n",
>>>> - (u32)id);
>>>> + version = vd55g1_get_version(id, color);
>>>> + if (!version) {
>>>> + dev_warn(sensor->dev, "Unsupported sensor version, expected %s\n",
>>>> + dt_version->name);
>>>> return -ENODEV;
>>>> }
>>>> - if (id != dt_id) {
>>>> - dev_err(sensor->dev, "Probed sensor %s and device tree definition (%s) mismatch",
>>>> - VD55G1_MODEL_ID_NAME(id), VD55G1_MODEL_ID_NAME(dt_id));
>>>> + if (version->id != dt_version->id ||
>>>> + version->color != dt_version->color) {
>>>> + dev_err(sensor->dev, "Probed sensor version %s and device tree definition %s mismatch",
>>>> + version->name, dt_version->name);
>>>> return -ENODEV;
>>>> }
>>>> - sensor->id = id;
>>>>
>>>> - ret = vd55g1_read(sensor, VD55G1_REG_REVISION, &rev, NULL);
>>>> - if (ret)
>>>> - return ret;
>>>> -
>>>> - if ((id == VD55G1_MODEL_ID_VD55G1 && rev != VD55G1_REVISION_CCB) &&
>>>> - (id == VD55G1_MODEL_ID_VD65G4 && rev != VD55G1_REVISION_BAYER)) {
>>>> - dev_err(sensor->dev, "Unsupported sensor revision 0x%x for sensor %s\n",
>>>> - (u16)rev, VD55G1_MODEL_ID_NAME(id));
>>>> - return -ENODEV;
>>>> - }
>>>> + sensor->version = version;
>>>>
>>>> return 0;
>>>> }
>>>> @@ -2048,8 +2085,9 @@ static void vd55g1_remove(struct i2c_client *client)
>>>> }
>>>>
>>>> static const struct of_device_id vd55g1_dt_ids[] = {
>>>> - { .compatible = "st,vd55g1", .data = (void *)VD55G1_MODEL_ID_VD55G1 },
>>>> - { .compatible = "st,vd65g4", .data = (void *)VD55G1_MODEL_ID_VD65G4 },
>>>> + { .compatible = "st,vd55g1", .data = (void *)&vd55g1_versions[0] },
>>>> + { .compatible = "st,vd55g4", .data = (void *)&vd55g1_versions[1] },
>>>> + { .compatible = "st,vd65g4", .data = (void *)&vd55g1_versions[2] },
>>>> { /* sentinel */ }
>>>> };
>>>
>>> All minors
>>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>>
>>> Thanks
>>> j
>>>
>>>> MODULE_DEVICE_TABLE(of, vd55g1_dt_ids);
>>>>
>>>> --
>>>> 2.43.0
>>>>
>>>>
>>
>> --
>> Regards,
>> Benjamin
>>
--
Regards,
Benjamin
^ permalink raw reply
* Re: [PATCH v3 00/15] drm/tidss: Add BeagleY-AI display support (and some more)
From: Shubham @ 2026-06-26 9:49 UTC (permalink / raw)
To: tomi.valkeinen
Cc: a-bhatia1, afd, airlied, aradhya.bhatia, conor+dt, devarsht,
devicetree, dri-devel, j-choudhary, krzk+dt, lee,
linux-arm-kernel, linux-kernel, louis.chauvet, maarten.lankhorst,
mripard, nm, robertcnelson, robh, s-jain1, simona, tzimmermann,
vigneshr
In-Reply-To: <20260529-beagley-ai-display-v3-0-7fefdc5d1adf@ideasonboard.com>
Hi Tomi,
A big update on the Raspberry Pi Touch Display 2 (5-inch, native ILI9881C
MIPI-DSI, 720x1280, 2 lanes) bring-up on BeagleY-AI (AM67A/J722S), still on
Robert's v7.0.x-arm64-k3 tree (7.0.11-arm64-k3-r20): tidss + cdns-dsi +
cdns-dphy, DSS = ti,am62p-dss (dispc_am625_feats).
The earlier "tidss comes up before the DSI bridge is added" problem is
resolved (cdns-dsi now adds its input bridge in probe, so tidss attaches it),
and after fixing two more issues the panel is now alive -- but I'm stuck on
one last step and would value your read.
What is now confirmed working
-----------------------------
- DSI-1 connector is created, tidss initializes, no probe errors, nothing
left in /sys/kernel/debug/devices_deferred.
- The panel is ALIVE and initialized: a DCS read of GET_POWER_MODE (0x0A)
returns 0x9c (booster on, sleep-out, display-on). So the LP command path
over the cdns works and the ILI9881C processed its init sequence.
- cdns-dsi looks healthy while a test pattern streams:
MCTL_MAIN_STS = 0x0000000F (PLL_LOCKED + CLK_LANE_RDY + DATA_LANE_RDY 0/1)
MCTL_DPHY_ERR = 0x00000000 (no D-PHY errors)
VID_MODE_STS = 0x00000001 (VSG_RUNNING, no error bits)
MCTL_LANE_STS -> clock lane in HS
VID_MAIN_CTL = 0x80A0FE00 (RGB888, packed-pixel DT 0x3E)
VID_HSIZE2 hact = 2160 (720*3), VID_VSIZE2 vact = 1280
cdns-dphy PLL locks (no "Failed to lock PLL").
- The DSS is scanning out:
modetest -M tidss -s <dsi>@<crtc>:720x1280 -v -> steady 60.01 Hz.
(For reference, the two fixes that got the panel this far were: nesting the
DSS port@1 under the controller's "ports" node in my overlay, and correcting
the BeagleY-AI MIPI mux OE polarity -- it is active-low, and the data-lane
mux had been left disabled while i2c still worked through a separate path.)
The problem: cdns data lanes never transmit
-------------------------------------------
Sampling MCTL_LANE_STS 20000 times while the pattern is actively streaming
(modetest -v at 60 Hz, from a second session):
DATA0 states: {'IDLE': 20000}
DATA1 states: {'IDLE': 20000}
So the clock lane is in HS and the VSG reports "running", but the data lanes
are 100% IDLE -- the cdns is in video mode yet never emits pixel packets.
The DSS VP does not appear to feed the cdns DPI input even though the VP
timing generator runs at 60 Hz, and the panel (correctly initialized) just
shows backlight, no image.
What I have ruled out
---------------------
- Videoport selection: I tried the DSI on DSS port@1 (VP2 = DISPC_VP_DPI)
and on DSS port@0 (VP1 = DISPC_VP_INTERNAL). Identical result, data lanes
IDLE in both.
- tidss correctly treats the cdns as internal: the bridge of_node
compatible "ti,j721e-dsi" is in tidss_internal_bridge_compatibles[], so
dpi_output is false and it does not touch ti,am625-dss-dpi0-clk-ctrl for
this videoport.
- Overlay/graph: cdns DSI_OUTPUT_PORT=0 (panel) / DSI_INPUT_PORT=1 (DSS)
wired accordingly; panel attaches; no -ENODEV; VID timing is correct.
Questions
---------
1. On AM67A/J722S, what internally connects a DSS video port's DPI to the
cdns-dsi DPI input, and is there a clock/enable/routing step (beyond the
DT graph and dpi0-clk-ctrl, which tidss skips for the internal path)
required for the DSS to actually feed the cdns?
2. Which DSS VP is wired to the internal DSI on this SoC (VP1/INTERNAL vs
VP2/DPI), and does the DISPC_VP_INTERNAL path need any handling that the
DISPC_VP_DPI path does not get?
3. Any reason the cdns VSG would report VSG_RUNNING with the clock lane in
HS and both data lanes "ready", yet never drive the data lanes (no pixel
packets) while the DSS VP is visibly scanning at 60 Hz?
I'm happy to share the full overlay, dmesg and register dumps, and to test
patches.
Thanks,
Shubham
^ permalink raw reply
* Re: [PATCH v6 2/2] drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver
From: Maxime Ripard @ 2026-06-26 9:53 UTC (permalink / raw)
To: Sunyun Yang
Cc: Krzysztof Kozlowski, robh, krzk+dt, conor+dt, andrzej.hajda,
neil.armstrong, dmitry.baryshkov, maarten.lankhorst, rfoss,
Laurent.pinchart, tzimmermann, jonas, jernej.skrabec, devicetree,
dri-devel, linux-kernel, xmzhu, xmzhu, rlyu, xbpeng
In-Reply-To: <CAFQXuNYOcvaiknrFwmxKQdiuWMOZo7mV_Rv7TrMScO_7Y_p8_Q@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 8781 bytes --]
On Fri, Jun 26, 2026 at 05:45:08PM +0800, Sunyun Yang wrote:
> Maxime Ripard <mripard@kernel.org> 于2026年6月26日周五 17:31写道：
> >
> > On Fri, Jun 26, 2026 at 05:05:38PM +0800, Sunyun Yang wrote:
> > > Sunyun Yang <syyang@lontium.com> 于2026年6月26日周五 16:40写道：
> > >
> > > >
> > > > Maxime Ripard <mripard@kernel.org> 于2026年6月26日周五 16:26写道：
> > > > >
> > > > > On Fri, Jun 26, 2026 at 04:13:18PM +0800, Sunyun Yang wrote:
> > > > > > Maxime Ripard <mripard@kernel.org> 于2026年6月26日周五 15:49写道：
> > > > > > >
> > > > > > > On Fri, Jun 26, 2026 at 10:15:03AM +0800, Sunyun Yang wrote:
> > > > > > > > Krzysztof Kozlowski <krzk@kernel.org> 于2026年6月25日周四 21:51写道：
> > > > > > > > >
> > > > > > > > > On 25/06/2026 15:40, Sunyun Yang wrote:
> > > > > > > > > > Sunyun Yang <syyang@lontium.com> 于2026年6月25日周四 21:26写道：
> > > > > > > > > >>
> > > > > > > > > >> Krzysztof Kozlowski <krzk@kernel.org> 于2026年6月25日周四 21:17写道：
> > > > > > > > > >>>
> > > > > > > > > >>> On 25/06/2026 15:14, Sunyun Yang wrote:
> > > > > > > > > >>>> Krzysztof Kozlowski <krzk@kernel.org> 于2026年6月25日周四 20:54写道：
> > > > > > > > > >>>>>
> > > > > > > > > >>>>> On 08/05/2026 15:40, syyang@lontium.com wrote:
> > > > > > > > > >>>>>> +
> > > > > > > > > >>>>>> +static void lt9611c_reset(struct lt9611c *lt9611c)
> > > > > > > > > >>>>>> +{
> > > > > > > > > >>>>>> + gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> > > > > > > > > >>>>>> + msleep(20);
> > > > > > > > > >>>>>> +
> > > > > > > > > >>>>>> + gpiod_set_value_cansleep(lt9611c->reset_gpio, 0);
> > > > > > > > > >>>>>> + msleep(20);
> > > > > > > > > >>>>>> +
> > > > > > > > > >>>>>> + gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> > > > > > > > > >>>>>
> > > > > > > > > >>>>> This is just plain wrong. Why do you assert, then de-assert and then
> > > > > > > > > >>>>> finally assert AGAIN the reset leaving the device in powerdown stage?
> > > > > > > > > >>>>>
> > > > > > > > > >>>> I am using software to emulate the hardware RESET button on our EVB.
> > > > > > > > > >>>> When the hardware RESET button is pressed while our chip is running,
> > > > > > > > > >>>> the signal level changes from HIGH to LOW and then back to HIGH.
> > > > > > > > > >>>>
> > > > > > > > > >>>> Of course, we can also use the following:
> > > > > > > > > >>>> static void lt9611c_reset(struct lt9611c *lt9611c)
> > > > > > > > > >>>> {
> > > > > > > > > >>>> gpiod_set_value_cansleep(lt9611c->reset_gpio, 0);
> > > > > > > > > >>>> msleep(50);
> > > > > > > > > >>>> gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> > > > > > > > > >>>> msleep(20);
> > > > > > > > > >>>> }
> > > > > > > > > >>>
> > > > > > > > > >>> Makes no sense either and you just did not get the point and did not
> > > > > > > > > >>> answer my question. I asked WHY you leave asserted. Answer "we emulate"
> > > > > > > > > >>> is just plain wrong.
> > > > > > > > > >>>
> > > > > > > > > >>> So again please answer:
> > > > > > > > > >>>
> > > > > > > > > >>> Why do you leave device with reset asserted?
> > > > > > > > > >>>
> > > > > > > > > >>
> > > > > > > > > >> devicetree: reset-gpios = <&tlmm 128 GPIO_ACTIVE_HIGH>;
> > > > > > > > > >>
> > > > > > > > > >> GPIO_ACTIVE_HIGH:
> > > > > > > > > >>
> > > > > > > > > >> gpiod_set_value_cansleep(lt9611c->reset_gpio, 0); ------ reset pin
> > > > > > > > > >> is Low level : Clear the register configuration in the chip to stop
> > > > > > > > > >> the chip from working.
> > > > > > > > > >>
> > > > > > > > > >> gpiod_set_value_cansleep(lt9611c->reset_gpio, 1); ------ reset pin
> > > > > > > > > >> is high level: The chip resumes operation.
> > > > > > > > > >>
> > > > > > > > > >>
> > > > > > > > > >
> > > > > > > > > > Our purpose is: pull the level low to clear the register configuration
> > > > > > > > > > in the chip, and then pull it high to allow the MCU inside the chip to
> > > > > > > > > > re‑initialize the registers.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > And you do completely opposite... so that confirms your code is just wrong.
> > > > > > > > >
> > > > > > > >
> > > > > > > > The lontium-lt9611.yaml uses GPIO_ACTIVE_HIGH. I am just following the
> > > > > > > > rule of this device tree. If I modify the device tree to use
> > > > > > > > GPIO_ACTIVE_LOW,
> > > > > > > > and use the following code in my driver, then my driver would be correct.
> > > > > > > > However, would the existing kernel drivers lontium-lt9611uxc.c and
> > > > > > > > lontium-lt9611.c be affected?
> > > > > > >
> > > > > > > It might, but then it's a DT problem. The GPIO API for drivers always
> > > > > > > considers the logical state of a GPIO, so if you need to assert a
> > > > > > > signal, you'll always need to set 1. That's what Krzysztof was trying to
> > > > > > > explain.
> > > > > > >
> > > > > > > The DT will provide with GPIO_ACTIVE_* how that logical state translates
> > > > > > > to a physical GPIO state.
> > > > > > >
> > > > > > > If the DT says that this particular GPIO is active-high, then it means
> > > > > > > that we need to set the GPIO to 1 to assert reset. Now of course, it
> > > > > > > might not make sense for the controller itself, but it might for the
> > > > > > > board if there's a GPIO inverter in the middle for example.
> > > > > > >
> > > > > > > Anyway, in the case you're raising, the issue definitely lies in the DT,
> > > > > > > and that's what would need to be fixed.
> > > > > > >
> > > > > > > I also wouldn't be too concerned about lontium-lt9611.yaml, it's just an
> > > > > > > example.
> > > > > > >
> > > > > > > Maxime
> > > > > >
> > > > > > thanks Maxime, I will modify this code in the next version of the
> > > > > > driver, and I hope you can accept these changes.
> > > > > >
> > > > > > Maxime:
> > > > > > I have another question I would like to ask you
> > > > > > regarding sashiko-bot@kernel.org. Since sashiko-bot sometimes has
> > > > > > opinions that differ from yours, whose advice should I follow?
> > > > > >
> > > > > > If I do not adopt sashiko-bot's suggestions, will my patches still be
> > > > > > accepted into the upstream Linux kernel?
> > > > >
> > > > > I can't give a blanket answer. It depends on what you ignore exactly.
> > > > >
> > > >
> > > > Okay, another question: sashiko-bot is an AI bot. Are its review
> > > > comments optional, or must they be followed?
> > > >
> > >
> > > For example, in my driver, there is a function for upgrading the chip
> > > firmware. During debugging or production, upgrading the chip firmware
> > > will acquire a lock, which will block the DRM callback and affect
> > > display. It will be fine after the upgrade is completed and some
> > > devices are restarted. As long as there is no subsequent upgrade,
> > > display can work normally.
> > >
> > > From a purely software perspective, the AI bot considered this
> > > approach unacceptable and proposed synchronizing the pre-upgrade state
> > > to the DRM framework. From my personal perspective, I think the AI
> > > bot's suggestion would only make my driver more complex and redundant.
> > > Do you think I need to adopt the AI bot's suggestion?
> >
> > You're right it's wrong, but there's also no reason to allow that
> > firmware upgrade during the driver lifetime either through a debugfs
> > file. So you should get rid of the debugfs file and now you won't have
> > to bother with that comment anymore.
> >
> > > In addition, if I follow the AI bot's suggestion, the
> > > lontium-lt9611uxc.c and lontium-lt8713sx.c drivers that have been
> > > merged into the upstream Linux kernel would not meet the AI bot's
> > > requirements.
> >
> > Maybe, but it's not relevant either way. These drivers are merged, yours
> > isn't. Feel free to fix these if you spot any issue though.
> >
> > > When I get a reviewer's Reviewed-by flag, can I ignore the opinion of
> > > sashiko-bot (the AI bot)?
> >
> > Again, it depends on the comment. If you feel like one can be ignored,
> > you should at least argue why.
> >
>
> sashiko‑bot (AI bot) does not communicate or interact with me.
> Even if I explain the reasons, the AI bot never replies to me, and it
> only sends its review comments to me alone.
Then put it into the cover letter?
> If I do not make changes according to the AI bot's suggestions, will
> that affect my driver being merged into the upstream Linux kernel? I
> would like to know the answer to this question.
I told you twice already.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]
^ permalink raw reply
* Re: [PATCH v3 0/4] soc: amlogic: clk-measure: add A1 and T7 support
From: Jian Hu @ 2026-06-26 9:54 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Kevin Hilman, Jerome Brunet, Martin Blumenstingl
Cc: devicetree, linux-arm-kernel, linux-amlogic, linux-kernel,
Conor Dooley
In-Reply-To: <20260421-clkmsr_a1_t7-v3-0-efc00b0f9e6b@amlogic.com>
Hi Neil,
On 4/21/2026 4:17 PM, Jian Hu via B4 Relay wrote:
> [ EXTERNAL EMAIL ]
>
> This series adds Amlogic clock measurement support for A1 and T7 SoCs,
> including binding updates, driver additions, and device tree enablement.
Gentle ping. The patches have received review, could you please take
another look when you have time?
>
> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
> ---
> Changes in v3:
> - Place the clock-measure node after pwm_ao_gh node for t7.
> - Link to v2: https://lore.kernel.org/r/20260415-clkmsr_a1_t7-v2-0-02b6314427e6@amlogic.com
>
> Changes in v2:
> - Add const for a1 and t7 clock measure table.
> - Use b4 to send this series.
> - Link to v1: https://lore.kernel.org/all/20260410100329.3167482-1-jian.hu@amlogic.com
>
> ---
> Jian Hu (4):
> dt-bindings: soc: amlogic: clk-measure: Add A1 and T7 compatible
> soc: amlogic: clk-measure: Add A1 and T7 support
> arm64: dts: meson: a1: Add clk measure support
> arm64: dts: amlogic: t7: Add clk measure support
>
> .../soc/amlogic/amlogic,meson-gx-clk-measure.yaml | 2 +
> arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi | 5 +
> arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 5 +
> drivers/soc/amlogic/meson-clk-measure.c | 272 +++++++++++++++++++++
> 4 files changed, 284 insertions(+)
> ---
> base-commit: 401e5c73eedde8225e87bd11c794b8409248ff41
> change-id: 20260415-clkmsr_a1_t7-9820984d0af1
>
> Best regards,
> --
> Jian Hu <jian.hu@amlogic.com>
>
>
^ permalink raw reply
* Re: [PATCH v3 7/9] ASoC: dt-bindings: loongson,ls-audio-card: Add ATK-DL2K0300B compatible
From: Krzysztof Kozlowski @ 2026-06-26 10:01 UTC (permalink / raw)
To: Binbin Zhou
Cc: Binbin Zhou, Huacai Chen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Liam Girdwood, Mark Brown, Jaroslav Kysela,
Takashi Iwai, Keguang Zhang, Huacai Chen, Xuerui Wang, loongarch,
devicetree, linux-sound
In-Reply-To: <71430fcee5951fb7a7d52e2091a87707db85e06c.1782439646.git.zhoubinbin@loongson.cn>
On Fri, Jun 26, 2026 at 10:27:28AM +0800, Binbin Zhou wrote:
> Add new compatible for the ATK-DL2K0300B development board based on
> Loongson-2K0300.
>
> Unlike others, this board features GPIO-controlled headphone detection,
> headphone control, and speaker enable.
>
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
> .../sound/loongson,ls-audio-card.yaml | 38 +++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/sound/loongson,ls-audio-card.yaml b/Documentation/devicetree/bindings/sound/loongson,ls-audio-card.yaml
> index 8c214e5d04b1..dc7f4afbb777 100644
> --- a/Documentation/devicetree/bindings/sound/loongson,ls-audio-card.yaml
> +++ b/Documentation/devicetree/bindings/sound/loongson,ls-audio-card.yaml
> @@ -23,6 +23,7 @@ properties:
> enum:
> - loongson,ls-audio-card # Loongson-2K1000/Loongson-2K2000/LS7A
> - loongson,ls2k0300-forever-pi-audio-card # CTCISZ Forever Pi
> + - loongson,ls2k0300-dl2k0300b-audio-card # ATK-DL2K0300B
>
> mclk-fs:
> $ref: simple-card.yaml#/definitions/mclk-fs
> @@ -47,6 +48,18 @@ properties:
> required:
> - sound-dai
>
> + spkr-en-gpios:
> + maxItems: 1
> + description: The GPIO that enables the speakers
> +
> + hp-ctl-gpios:
> + maxItems: 1
> + description: The GPIO that control the headphones
> +
> + hp-det-gpios:
> + maxItems: 1
> + description: The GPIO that detect headphones are plugged in
> +
Add allOf:if:then: block after "required:" block and disallow these
("...: false") for other variants.
If these stay, because I have doubts. spkr-en-gpios sounds like
enable-gpios of each speaker, so not suitable here but should go to
individual speaker codec nodes.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: hwmon: chipcap2: Add label property
From: Krzysztof Kozlowski @ 2026-06-26 10:05 UTC (permalink / raw)
To: Flaviu Nistor
Cc: Guenter Roeck, Javier Carrasco, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Corbet, Shuah Khan, linux-hwmon,
linux-kernel, devicetree, linux-doc
In-Reply-To: <20260625160423.17882-1-flaviu.nistor@gmail.com>
On Thu, Jun 25, 2026 at 07:04:22PM +0300, Flaviu Nistor wrote:
> Add support for an optional label property similar to other hwmon devices.
> This allows, in case of boards with multiple CHIPCAP2 sensors, to assign
> distinct names to each instance.
>
> Signed-off-by: Flaviu Nistor <flaviu.nistor@gmail.com>
> ---
> Changes in v2:
> - Implement suggestion from Javier Carrasco as proposed by Krzysztof Kozlowski.
> - Link to v1: https://lore.kernel.org/all/20260622122200.14245-1-flaviu.nistor@gmail.com/
>
> .../devicetree/bindings/hwmon/amphenol,chipcap2.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v6 2/2] drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver
From: Sunyun Yang @ 2026-06-26 10:08 UTC (permalink / raw)
To: Maxime Ripard
Cc: Krzysztof Kozlowski, robh, krzk+dt, conor+dt, andrzej.hajda,
neil.armstrong, dmitry.baryshkov, maarten.lankhorst, rfoss,
Laurent.pinchart, tzimmermann, jonas, jernej.skrabec, devicetree,
dri-devel, linux-kernel, xmzhu, xmzhu, rlyu, xbpeng
In-Reply-To: <20260626-hopping-savvy-seagull-cc9626@houat>
Maxime Ripard <mripard@kernel.org> 于2026年6月26日周五 17:53写道：
>
> On Fri, Jun 26, 2026 at 05:45:08PM +0800, Sunyun Yang wrote:
> > Maxime Ripard <mripard@kernel.org> 于2026年6月26日周五 17:31写道：
> > >
> > > On Fri, Jun 26, 2026 at 05:05:38PM +0800, Sunyun Yang wrote:
> > > > Sunyun Yang <syyang@lontium.com> 于2026年6月26日周五 16:40写道：
> > > >
> > > > >
> > > > > Maxime Ripard <mripard@kernel.org> 于2026年6月26日周五 16:26写道：
> > > > > >
> > > > > > On Fri, Jun 26, 2026 at 04:13:18PM +0800, Sunyun Yang wrote:
> > > > > > > Maxime Ripard <mripard@kernel.org> 于2026年6月26日周五 15:49写道：
> > > > > > > >
> > > > > > > > On Fri, Jun 26, 2026 at 10:15:03AM +0800, Sunyun Yang wrote:
> > > > > > > > > Krzysztof Kozlowski <krzk@kernel.org> 于2026年6月25日周四 21:51写道：
> > > > > > > > > >
> > > > > > > > > > On 25/06/2026 15:40, Sunyun Yang wrote:
> > > > > > > > > > > Sunyun Yang <syyang@lontium.com> 于2026年6月25日周四 21:26写道：
> > > > > > > > > > >>
> > > > > > > > > > >> Krzysztof Kozlowski <krzk@kernel.org> 于2026年6月25日周四 21:17写道：
> > > > > > > > > > >>>
> > > > > > > > > > >>> On 25/06/2026 15:14, Sunyun Yang wrote:
> > > > > > > > > > >>>> Krzysztof Kozlowski <krzk@kernel.org> 于2026年6月25日周四 20:54写道：
> > > > > > > > > > >>>>>
> > > > > > > > > > >>>>> On 08/05/2026 15:40, syyang@lontium.com wrote:
> > > > > > > > > > >>>>>> +
> > > > > > > > > > >>>>>> +static void lt9611c_reset(struct lt9611c *lt9611c)
> > > > > > > > > > >>>>>> +{
> > > > > > > > > > >>>>>> + gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> > > > > > > > > > >>>>>> + msleep(20);
> > > > > > > > > > >>>>>> +
> > > > > > > > > > >>>>>> + gpiod_set_value_cansleep(lt9611c->reset_gpio, 0);
> > > > > > > > > > >>>>>> + msleep(20);
> > > > > > > > > > >>>>>> +
> > > > > > > > > > >>>>>> + gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> > > > > > > > > > >>>>>
> > > > > > > > > > >>>>> This is just plain wrong. Why do you assert, then de-assert and then
> > > > > > > > > > >>>>> finally assert AGAIN the reset leaving the device in powerdown stage?
> > > > > > > > > > >>>>>
> > > > > > > > > > >>>> I am using software to emulate the hardware RESET button on our EVB.
> > > > > > > > > > >>>> When the hardware RESET button is pressed while our chip is running,
> > > > > > > > > > >>>> the signal level changes from HIGH to LOW and then back to HIGH.
> > > > > > > > > > >>>>
> > > > > > > > > > >>>> Of course, we can also use the following:
> > > > > > > > > > >>>> static void lt9611c_reset(struct lt9611c *lt9611c)
> > > > > > > > > > >>>> {
> > > > > > > > > > >>>> gpiod_set_value_cansleep(lt9611c->reset_gpio, 0);
> > > > > > > > > > >>>> msleep(50);
> > > > > > > > > > >>>> gpiod_set_value_cansleep(lt9611c->reset_gpio, 1);
> > > > > > > > > > >>>> msleep(20);
> > > > > > > > > > >>>> }
> > > > > > > > > > >>>
> > > > > > > > > > >>> Makes no sense either and you just did not get the point and did not
> > > > > > > > > > >>> answer my question. I asked WHY you leave asserted. Answer "we emulate"
> > > > > > > > > > >>> is just plain wrong.
> > > > > > > > > > >>>
> > > > > > > > > > >>> So again please answer:
> > > > > > > > > > >>>
> > > > > > > > > > >>> Why do you leave device with reset asserted?
> > > > > > > > > > >>>
> > > > > > > > > > >>
> > > > > > > > > > >> devicetree: reset-gpios = <&tlmm 128 GPIO_ACTIVE_HIGH>;
> > > > > > > > > > >>
> > > > > > > > > > >> GPIO_ACTIVE_HIGH:
> > > > > > > > > > >>
> > > > > > > > > > >> gpiod_set_value_cansleep(lt9611c->reset_gpio, 0); ------ reset pin
> > > > > > > > > > >> is Low level : Clear the register configuration in the chip to stop
> > > > > > > > > > >> the chip from working.
> > > > > > > > > > >>
> > > > > > > > > > >> gpiod_set_value_cansleep(lt9611c->reset_gpio, 1); ------ reset pin
> > > > > > > > > > >> is high level: The chip resumes operation.
> > > > > > > > > > >>
> > > > > > > > > > >>
> > > > > > > > > > >
> > > > > > > > > > > Our purpose is: pull the level low to clear the register configuration
> > > > > > > > > > > in the chip, and then pull it high to allow the MCU inside the chip to
> > > > > > > > > > > re‑initialize the registers.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > And you do completely opposite... so that confirms your code is just wrong.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > The lontium-lt9611.yaml uses GPIO_ACTIVE_HIGH. I am just following the
> > > > > > > > > rule of this device tree. If I modify the device tree to use
> > > > > > > > > GPIO_ACTIVE_LOW,
> > > > > > > > > and use the following code in my driver, then my driver would be correct.
> > > > > > > > > However, would the existing kernel drivers lontium-lt9611uxc.c and
> > > > > > > > > lontium-lt9611.c be affected?
> > > > > > > >
> > > > > > > > It might, but then it's a DT problem. The GPIO API for drivers always
> > > > > > > > considers the logical state of a GPIO, so if you need to assert a
> > > > > > > > signal, you'll always need to set 1. That's what Krzysztof was trying to
> > > > > > > > explain.
> > > > > > > >
> > > > > > > > The DT will provide with GPIO_ACTIVE_* how that logical state translates
> > > > > > > > to a physical GPIO state.
> > > > > > > >
> > > > > > > > If the DT says that this particular GPIO is active-high, then it means
> > > > > > > > that we need to set the GPIO to 1 to assert reset. Now of course, it
> > > > > > > > might not make sense for the controller itself, but it might for the
> > > > > > > > board if there's a GPIO inverter in the middle for example.
> > > > > > > >
> > > > > > > > Anyway, in the case you're raising, the issue definitely lies in the DT,
> > > > > > > > and that's what would need to be fixed.
> > > > > > > >
> > > > > > > > I also wouldn't be too concerned about lontium-lt9611.yaml, it's just an
> > > > > > > > example.
> > > > > > > >
> > > > > > > > Maxime
> > > > > > >
> > > > > > > thanks Maxime, I will modify this code in the next version of the
> > > > > > > driver, and I hope you can accept these changes.
> > > > > > >
> > > > > > > Maxime:
> > > > > > > I have another question I would like to ask you
> > > > > > > regarding sashiko-bot@kernel.org. Since sashiko-bot sometimes has
> > > > > > > opinions that differ from yours, whose advice should I follow?
> > > > > > >
> > > > > > > If I do not adopt sashiko-bot's suggestions, will my patches still be
> > > > > > > accepted into the upstream Linux kernel?
> > > > > >
> > > > > > I can't give a blanket answer. It depends on what you ignore exactly.
> > > > > >
> > > > >
> > > > > Okay, another question: sashiko-bot is an AI bot. Are its review
> > > > > comments optional, or must they be followed?
> > > > >
> > > >
> > > > For example, in my driver, there is a function for upgrading the chip
> > > > firmware. During debugging or production, upgrading the chip firmware
> > > > will acquire a lock, which will block the DRM callback and affect
> > > > display. It will be fine after the upgrade is completed and some
> > > > devices are restarted. As long as there is no subsequent upgrade,
> > > > display can work normally.
> > > >
> > > > From a purely software perspective, the AI bot considered this
> > > > approach unacceptable and proposed synchronizing the pre-upgrade state
> > > > to the DRM framework. From my personal perspective, I think the AI
> > > > bot's suggestion would only make my driver more complex and redundant.
> > > > Do you think I need to adopt the AI bot's suggestion?
> > >
> > > You're right it's wrong, but there's also no reason to allow that
> > > firmware upgrade during the driver lifetime either through a debugfs
> > > file. So you should get rid of the debugfs file and now you won't have
> > > to bother with that comment anymore.
> > >
> > > > In addition, if I follow the AI bot's suggestion, the
> > > > lontium-lt9611uxc.c and lontium-lt8713sx.c drivers that have been
> > > > merged into the upstream Linux kernel would not meet the AI bot's
> > > > requirements.
> > >
> > > Maybe, but it's not relevant either way. These drivers are merged, yours
> > > isn't. Feel free to fix these if you spot any issue though.
> > >
> > > > When I get a reviewer's Reviewed-by flag, can I ignore the opinion of
> > > > sashiko-bot (the AI bot)?
> > >
> > > Again, it depends on the comment. If you feel like one can be ignored,
> > > you should at least argue why.
> > >
> >
> > sashiko‑bot (AI bot) does not communicate or interact with me.
> > Even if I explain the reasons, the AI bot never replies to me, and it
> > only sends its review comments to me alone.
>
> Then put it into the cover letter?
>
thanks Maxime,
I will modify the driver patch, include the relevant explanation in
the cover letter, and then submit it.
> > If I do not make changes according to the AI bot's suggestions, will
> > that affect my driver being merged into the upstream Linux kernel? I
> > would like to know the answer to this question.
>
> I told you twice already.
^ permalink raw reply
* Re: [PATCH v2 4/4] iio: dac: ad3530r: Add support for AD3532R/AD3532
From: Andy Shevchenko @ 2026-06-26 10:11 UTC (permalink / raw)
To: Paller, Kim Seer
Cc: Jonathan Cameron, David Lechner, Sa, Nuno, Andy Shevchenko,
Hennerich, Michael, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-iio@vger.kernel.org,
linux-kernel@vger.kernel.org, linux, devicetree@vger.kernel.org
In-Reply-To: <PH0PR03MB7141482F87D8416DBDB90136F9EC2@PH0PR03MB7141.namprd03.prod.outlook.com>
On Thu, Jun 25, 2026 at 10:06:26AM +0000, Paller, Kim Seer wrote:
> > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > Sent: Monday, June 15, 2026 6:06 PM
> > On Mon, Jun 15, 2026 at 02:20:18PM +0800, Kim Seer Paller wrote:
...
> > > + local_ch = chan->channel % AD3530R_CH_PER_BANK;
> >
> > > + reg = (chan->channel < AD3530R_CH_PER_BANK ?
> > AD3532R_OUTPUT_OPERATING_MODE_0 :
> > > + AD3532R_OUTPUT_OPERATING_MODE_2) +
> > > + local_ch / AD3530R_CH_PER_REG;
> >
> > This is unreadable. Can you refactor it?
>
> Would this be clearer?
>
> unsigned int bank_base;
>
> local_ch = chan->channel % AD3530R_CH_PER_BANK;
> bank_base = chan->channel < AD3530R_CH_PER_BANK ?
> AD3532R_OUTPUT_OPERATING_MODE_0 : AD3532R_OUTPUT_OPERATING_MODE_2;
> reg = bank_base + local_ch / AD3530R_CH_PER_REG;
No. Too much voodoo arithmetics and comparisons.
> > > + mask = AD3530R_OP_MODE_CHAN_MSK(local_ch %
> > AD3530R_CH_PER_REG);
Including this one.
I would expect to see maybe two more variables to hold
local_ch % BANK and local_ch / BANK.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 1/4] iio: dac: ad3530r: Refactor setup to table-driven register bank approach
From: Andy Shevchenko @ 2026-06-26 10:12 UTC (permalink / raw)
To: Paller, Kim Seer
Cc: Jonathan Cameron, David Lechner, Sa, Nuno, Andy Shevchenko,
Hennerich, Michael, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-iio@vger.kernel.org,
linux-kernel@vger.kernel.org, linux, devicetree@vger.kernel.org
In-Reply-To: <PH0PR03MB7141B6D49AA897B29C42A121F9EB2@PH0PR03MB7141.namprd03.prod.outlook.com>
On Fri, Jun 26, 2026 at 08:44:57AM +0000, Paller, Kim Seer wrote:
> > -----Original Message-----
> > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > Sent: Monday, June 15, 2026 6:08 PM
> > On Mon, Jun 15, 2026 at 02:20:15PM +0800, Kim Seer Paller wrote:
...
> > > + for (unsigned int i = 0; i < num_regs; i++) {
> > > + ret = regmap_write(st->regmap, regs[i], val);
> > > + if (ret)
> > > + return ret;
> > > + }
> >
> > Can the above helpers use bulk operations or regmap_multi_reg_write()?
>
> I think bulk operations don't apply for the AD3532R case, since bank 0 is around 0x102x
> and bank 1 around 0x302x two register banks, not one continuous block.
> For regmap_multi_reg_write(), since all the registers get the same value, we would
> have to build a reg_sequence, and it ends up being the same number of writes with
> just more code. So I think the simple loop helper is better here, but happy to switch if
> regmap_multi_reg_write() is the preferred form.
No need, thanks for the explanation.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 2/2] hwmon: (chipcap2) Add support for label
From: Javier Carrasco @ 2026-06-26 10:14 UTC (permalink / raw)
To: Flaviu Nistor, Guenter Roeck, Javier Carrasco, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Shuah Khan
Cc: linux-hwmon, linux-kernel, devicetree, linux-doc
In-Reply-To: <20260625160423.17882-2-flaviu.nistor@gmail.com>
On Thu Jun 25, 2026 at 6:04 PM CEST, Flaviu Nistor wrote:
> Add support for label sysfs attribute similar to other hwmon devices.
> This is particularly useful for systems with multiple sensors on the
> same board, where identifying individual sensors is much easier since
> labels can be defined via device tree.
>
> Signed-off-by: Flaviu Nistor <flaviu.nistor@gmail.com>
> ---
> Changes in v2:
> - No change for this patch in the patch series.
> - Link to v1: https://lore.kernel.org/all/20260622122200.14245-1-flaviu.nistor@gmail.com/
>
> Documentation/hwmon/chipcap2.rst | 2 ++
> drivers/hwmon/chipcap2.c | 25 +++++++++++++++++++++++--
> 2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/hwmon/chipcap2.rst b/Documentation/hwmon/chipcap2.rst
> index dc165becc64c..c38d87b91b69 100644
> --- a/Documentation/hwmon/chipcap2.rst
> +++ b/Documentation/hwmon/chipcap2.rst
> @@ -70,4 +70,6 @@ humidity1_min_hyst: RW humidity low hystersis
> humidity1_max_hyst: RW humidity high hystersis
> humidity1_min_alarm: RO humidity low alarm indicator
> humidity1_max_alarm: RO humidity high alarm indicator
> +humidity1_label: RO descriptive name for the sensor
> +temp1_label: RO descriptive name for the sensor
> =============================== ======= ========================================
> diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
> index 4aecf463180f..086571d556b7 100644
> --- a/drivers/hwmon/chipcap2.c
> +++ b/drivers/hwmon/chipcap2.c
> @@ -22,6 +22,8 @@
> #include <linux/irq.h>
> #include <linux/module.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
>
> #define CC2_START_CM 0xA0
> #define CC2_START_NOM 0x80
> @@ -83,6 +85,7 @@ struct cc2_data {
> struct i2c_client *client;
> struct regulator *regulator;
> const char *name;
> + const char *label;
> int irq_ready;
> int irq_low;
> int irq_high;
> @@ -449,6 +452,8 @@ static umode_t cc2_is_visible(const void *data, enum hwmon_sensor_types type,
> switch (attr) {
> case hwmon_humidity_input:
> return 0444;
> + case hwmon_humidity_label:
> + return cc2->label ? 0444 : 0;
> case hwmon_humidity_min_alarm:
> return cc2->rh_alarm.low_alarm_visible ? 0444 : 0;
> case hwmon_humidity_max_alarm:
> @@ -466,6 +471,8 @@ static umode_t cc2_is_visible(const void *data, enum hwmon_sensor_types type,
> switch (attr) {
> case hwmon_temp_input:
> return 0444;
> + case hwmon_temp_label:
> + return cc2->label ? 0444 : 0;
> default:
> return 0;
> }
> @@ -552,6 +559,16 @@ static int cc2_humidity_max_alarm_status(struct cc2_data *data, long *val)
> return 0;
> }
>
> +static int cc2_read_string(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, const char **str)
> +{
> + struct cc2_data *data = dev_get_drvdata(dev);
> +
> + *str = data->label;
> +
> + return 0;
> +}
> +
> static int cc2_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> int channel, long *val)
> {
> @@ -670,8 +687,9 @@ static int cc2_request_alarm_irqs(struct cc2_data *data, struct device *dev)
> }
>
> static const struct hwmon_channel_info *cc2_info[] = {
> - HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> - HWMON_CHANNEL_INFO(humidity, HWMON_H_INPUT | HWMON_H_MIN | HWMON_H_MAX |
> + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_LABEL),
> + HWMON_CHANNEL_INFO(humidity, HWMON_H_INPUT | HWMON_H_LABEL |
> + HWMON_H_MIN | HWMON_H_MAX |
> HWMON_H_MIN_HYST | HWMON_H_MAX_HYST |
> HWMON_H_MIN_ALARM | HWMON_H_MAX_ALARM),
> NULL
> @@ -680,6 +698,7 @@ static const struct hwmon_channel_info *cc2_info[] = {
> static const struct hwmon_ops cc2_hwmon_ops = {
> .is_visible = cc2_is_visible,
> .read = cc2_read,
> + .read_string = cc2_read_string,
> .write = cc2_write,
> };
>
> @@ -710,6 +729,8 @@ static int cc2_probe(struct i2c_client *client)
> return dev_err_probe(dev, PTR_ERR(data->regulator),
> "Failed to get regulator\n");
>
> + device_property_read_string(dev, "label", &data->label);
> +
> ret = cc2_request_ready_irq(data, dev);
> if (ret)
> return dev_err_probe(dev, ret, "Failed to request ready irq\n");
Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 1/2] regulator: dt-bindings: rtq2208: Label mtp-sel-high property as deprecated
From: Krzysztof Kozlowski @ 2026-06-26 10:19 UTC (permalink / raw)
To: cy_huang
Cc: Mark Brown, Krzysztof Kozlowski, Rob Herring, Conor Dooley,
Liam Girdwood, Yoon Dong Min, edward_kim, devicetree,
linux-kernel
In-Reply-To: <594ebe167b33ca885c040984624e4b5d1382c0e2.1782444299.git.cy_huang@richtek.com>
On Fri, Jun 26, 2026 at 11:38:52AM +0800, cy_huang@richtek.com wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
>
> Since it can be identified by hardware register, label the unnecessary
> property 'richtek,mtp-sel-high' as deprecated.
>
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/3] dt-bindings: arm: qcom: Add HP EliteBook X G2q 14 AI
From: Krzysztof Kozlowski @ 2026-06-26 10:20 UTC (permalink / raw)
To: Jason Pettit
Cc: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-arm-msm, devicetree, linux-kernel,
Akhil P Oommen, Mahadevan P, Sibi Sankar, Jingyi Wang,
Ananthu C V
In-Reply-To: <20260625-glymur-send-v2-1-00905324ffbf@oss.qualcomm.com>
On Thu, Jun 25, 2026 at 10:47:20PM -0700, Jason Pettit wrote:
> The HP EliteBook X G2q 14 AI is a Snapdragon X2 Elite (Glymur) laptop.
> Document its top-level "hp,elitebook-x-g2q" compatible.
>
> Signed-off-by: Jason Pettit <jason.pettit@oss.qualcomm.com>
> ---
> Documentation/devicetree/bindings/arm/qcom.yaml | 1 +
> 1 file changed, 1 insertion(+)
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 1/4] dt-bindings: display: tegra: Make non-vm registers optional
From: Krzysztof Kozlowski @ 2026-06-26 10:21 UTC (permalink / raw)
To: Mikko Perttunen
Cc: Thierry Reding, Jonathan Hunter, David Airlie, Simona Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-tegra, dri-devel,
devicetree, linux-kernel
In-Reply-To: <NWz2vh3HRYqBDySgTFAF_g@nvidia.com>
On Thu, Jun 25, 2026 at 06:17:39PM +0900, Mikko Perttunen wrote:
> On Thursday, June 25, 2026 5:34 PM Krzysztof Kozlowski wrote:
> > On Mon, Jun 22, 2026 at 05:22:50PM +0900, Mikko Perttunen wrote:
> > > Host1x can be used without accessing the hypervisor or common
> > > registers. Adjust the bindings to make them optional.
> >
> > You still have these registers in the hardware, so it is not optional,
> > not flexible. IOW, DTS represents the view of real hardware for the
> > software, and that hardware still has this address space regardless if
> > OS should use it or not.
> >
> > Otherwise you need to provide better background here.
> >
> > Best regards,
> > Krzysztof
> >
> >
>
> The intent is that the vm-only version is used in situations where the
> other register regions are not available to software. Typically that
> means running as a virtual machine, and the hypervisor owns the non-vm
> register regions and doesn't pass them to the guest.
>
> Since the guest runs under the hypervisor, its view of 'real hardware'
> is what the hypervisor is exposing to it -- in this case with just the
> "vm" register region.
>
> I'll add further explanation along above lines to the commit message
> if you're OK with it.
Yeah, it's fine. Please extend the commit msg to make it more obvious.
Best regards,
Krzysztof
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox