* Re: [PATCH v3 4/4] arm64: dts: rockchip: enable HDMI RX audio capture on Orange Pi 5 Plus
From: sashiko-bot @ 2026-07-18 9:11 UTC (permalink / raw)
To: Igor Paunovic; +Cc: media-ci, conor+dt, devicetree, robh
In-Reply-To: <20260718085728.6797-5-royalnet026@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The audio_work delayed work is not cancelled during system suspend, leading to a fatal bus fault.
- [Medium] Incorrect use of be32_to_cpu() for unconditional byte swapping breaks audio capture on big-endian architectures.
--
--- Patch [4]: [PATCH v3 4/4] arm64: dts: rockchip: enable HDMI RX audio capture on Orange Pi 5 Plus ---
commit 030cd8fd5334b79e1751dec57b1de1f5fa4e3df3
Author: Igor Paunovic <royalnet026@gmail.com>
arm64: dts: rockchip: enable HDMI RX audio capture on Orange Pi 5 Plus
This patch routes the HDMI receiver audio to the i2s7_8ch interface. It creates
a simple-audio-card configuration to expose a capture-only ALSA card driven
by the HDMI input. The setup uses the receiver as the bitclock and frame master.
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts b/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
> index 145986695720a..2a68610cddbc3 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
> @@ -37,6 +37,22 @@ hdmi1_con_in: endpoint {
> };
> };
>
> + hdmiin-sound {
> + compatible = "simple-audio-card";
> + simple-audio-card,name = "rockchip,hdmiin";
> + simple-audio-card,format = "i2s";
> + simple-audio-card,bitclock-master = <&hdmiin_codec>;
> + simple-audio-card,frame-master = <&hdmiin_codec>;
> +
> + simple-audio-card,cpu {
> + sound-dai = <&i2s7_8ch>;
> + };
> +
> + hdmiin_codec: simple-audio-card,codec {
> + sound-dai = <&hdmi_receiver 0>;
[Severity: High]
Does exposing this codec to the ALSA subsystem trigger a suspend regression in
the hdmirx driver?
Looking at the underlying codec driver hdmirx_suspend() in snps_hdmirx.c:
drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c:hdmirx_suspend()
{
struct snps_hdmirx_dev *hdmirx_dev = dev_get_drvdata(dev);
hdmirx_disable_irq(dev);
/* TODO store CEC HW state */
disable_irq(hdmirx_dev->cec->irq);
return hdmirx_disable(dev);
}
The audio_work delayed work is queued on system_unbound_wq, which is not
freezable, but this suspend callback doesn't cancel it.
If system suspend is initiated while audio capture is active, the worker
could fire after clocks are disabled via hdmirx_disable(), resulting in
a bus fault when it performs MMIO reads.
[Severity: Medium]
Will this audio capture work correctly on big-endian architectures?
The underlying hdmirx_audio_fs() driver function uses be32_to_cpu() for an
unconditional byte swap when calculating the ACR subpacket:
drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c:hdmirx_audio_fs()
{
...
acr_cts = be32_to_cpu((__force __be32)acr_pb3_0) & 0xfffff;
acr_n = (be32_to_cpu((__force __be32)acr_pb7_4) & 0x0fffff00) >> 8;
...
}
Since be32_to_cpu() behaves as a no-op on big-endian CPUs, the bytes remain
unswapped, causing the bitwise masking to extract the wrong bytes for the
ACR CTS and N fields.
Should swab32() be used here instead to ensure the byte swap happens
unconditionally on all architectures?
> + };
> + };
> +
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260718085728.6797-1-royalnet026@gmail.com?part=4
^ permalink raw reply
* Re: [PATCH v3 2/4] media: synopsys: hdmirx: add HDMI audio capture support
From: sashiko-bot @ 2026-07-18 9:09 UTC (permalink / raw)
To: Igor Paunovic; +Cc: conor+dt, robh, devicetree, media-ci
In-Reply-To: <20260718085728.6797-3-royalnet026@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect byte-swapping logic completely scrambles the extraction of Audio Clock Regeneration (ACR) CTS and N values, breaking sample rate recovery on little-endian architectures.
- [High] The `audio_work` delayed work is not explicitly cancelled in `hdmirx_remove()`, leading to a Use-After-Free if the driver is unbound while an audio stream is active.
--
--- Patch [2]: [PATCH v3 2/4] media: synopsys: hdmirx: add HDMI audio capture support ---
commit b04bcd22f40b424aac824100715c46f764531875
Author: Igor Paunovic <royalnet026@gmail.com>
media: synopsys: hdmirx: add HDMI audio capture support
The Synopsys DesignWare HDMI RX controller extracts the audio stream
embedded in the incoming HDMI signal and feeds it to an on-SoC I2S
controller. Expose it as an ALSA capture device by registering the
generic hdmi-codec as a child of the controller.
> diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> index 25f8ca0d6d946..22f50cbf3fa7a 100644
> --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
[ ... ]
> +/* Recover the incoming audio sample rate from the ACR N/CTS + TMDS clock. */
> +static u32 hdmirx_audio_fs(struct snps_hdmirx_dev *hdmirx_dev)
> +{
> + u64 tmds_clk, fs_audio = 0;
> + u32 acr_cts, acr_n, tmdsqpclk_freq;
> + u32 acr_pb3_0, acr_pb7_4;
> +
> + tmdsqpclk_freq = hdmirx_readl(hdmirx_dev, CMU_TMDSQPCLK_FREQ);
> + hdmirx_readl(hdmirx_dev, PKTDEC_ACR_PH2_1);
> + acr_pb3_0 = hdmirx_readl(hdmirx_dev, PKTDEC_ACR_PB3_0);
> + acr_pb7_4 = hdmirx_readl(hdmirx_dev, PKTDEC_ACR_PB7_4);
> + /*
> + * The packet decoder stores the ACR subpacket bytes with packet byte
> + * 0 in register bits [7:0], so byte-swap each word to line the bytes
> + * up: CTS is packet bytes 1-3 (PKTDEC_ACR_PB3_0) and N is packet
> + * bytes 4-6 (PKTDEC_ACR_PB7_4), 20 bits each.
> + */
> + acr_cts = be32_to_cpu((__force __be32)acr_pb3_0) & 0xfffff;
> + acr_n = (be32_to_cpu((__force __be32)acr_pb7_4) & 0x0fffff00) >> 8;
[Severity: High]
Does this byte-swapping logic scramble the extraction of the CTS and N values
on little-endian architectures?
Since the HDMI ACR packet transmits the 20-bit CTS value across PB0, PB1, and
PB2, and because it is little-endian, applying a bitmask like
acr_pb3_0 & 0xfffff natively yields the correct CTS value without any swaps.
By byte-swapping the 32-bit register, the bytes are completely reversed.
The subsequent bitmask extracts the lower 20 bits of the swapped word, which
correspond to the lower nibble of PB1, all of PB2, and all of PB3, discarding
PB0 entirely.
The same applies to acr_pb7_4 to extract N. Will this regression result in
garbage values for both acr_cts and acr_n, forcing the driver to blindly
fall back to the default sample rate?
[ ... ]
> @@ -2784,6 +3052,9 @@ static void hdmirx_remove(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> struct snps_hdmirx_dev *hdmirx_dev = dev_get_drvdata(dev);
>
> + if (hdmirx_dev->audio_pdev)
> + platform_device_unregister(hdmirx_dev->audio_pdev);
> +
[Severity: High]
Can this lead to a use-after-free if the driver is unbound while an audio
stream is active?
The v3 changelog notes that the synchronous cancel for the audio_work
delayed work was removed here under the assumption that audio_shutdown()
handles it.
However, ALSA does not synchronously invoke the PCM .shutdown callback
when a device is unregistered if a userspace application holds the file
descriptor open.
Because hdmirx_dev is allocated using devm_kzalloc, it is freed immediately
after hdmirx_remove() returns. Will the un-cancelled audio_work delayed work
subsequently fire and access the freed hdmirx_dev memory?
> v4l2_debugfs_if_free(hdmirx_dev->infoframes);
> debugfs_remove_recursive(hdmirx_dev->debugfs_dir);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260718085728.6797-1-royalnet026@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH net-next v9 12/12] net: airoha: add phylink support
From: sashiko-bot @ 2026-07-18 9:01 UTC (permalink / raw)
To: Christian Marangi; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260717065448.1498335-13-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Medium] Missing phy-mode property now unconditionally causes probe failure, breaking old device trees.
Pre-existing issues:
- [High] The spinlock `port->lock` is acquired using `spin_lock()` in process contexts without disabling softirqs, creating a deadlock risk if interrupted by a softirq calling `ndo_get_stats64`.
--
--- Patch [12]: [PATCH net-next v9 12/12] net: airoha: add phylink support ---
commit bb8a7d4b36e6cc8dd02385bcf6f62f3da412aa5a
Author: Christian Marangi <ansuelsmth@gmail.com>
net: airoha: add phylink support
Add phylink support for each GDM port. For GDM1 add the internal interface
mode as the only supported mode. For GDM2/3/4 add the required
configuration of the PCS to make the external PHY or attached SFP cage
work.
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 59001fd4b6f74..ed1ac032f3374 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
[ ... ]
> @@ -1837,7 +1838,7 @@ static void airoha_update_hw_stats(struct airoha_gdm_dev *dev)
> struct airoha_gdm_port *port = dev->port;
> int i;
>
> - spin_lock(&port->stats_lock);
> + spin_lock(&port->lock);
[Severity: High]
This is a pre-existing issue, but using spin_lock() here without disabling
softirqs creates a deadlock risk.
This function is called from process context (like ethtool callbacks), but it
is also called by airoha_dev_get_stats64() which can run in softirq context.
If a softirq interrupts the process-context caller while it holds port->lock,
the softirq handler will attempt to acquire the same lock and spin forever.
Should all process-context acquisitions of this lock be updated to use
spin_lock_bh() instead?
[ ... ]
> +static void airoha_mac_link_up(struct phylink_config *config, struct phy_device *phy,
> + unsigned int mode, phy_interface_t interface,
> + int speed, int duplex, bool tx_pause, bool rx_pause)
> +{
> + struct airoha_gdm_dev *dev = container_of(config, struct airoha_gdm_dev,
> + phylink_config);
> + struct airoha_gdm_port *port = dev->port;
> + struct airoha_eth *eth = dev->eth;
> + u32 frag_size_tx, frag_size_rx;
> + u32 mask, val;
> +
> + /* TX/RX frag is configured only for GDM4 */
> + if (port->id != AIROHA_GDM4_IDX)
> + return;
[ ... ]
> + spin_lock(&port->lock);
> +
> + /* Configure TX/RX frag based on speed */
[Severity: High]
This is a pre-existing issue with the locking design, but since
airoha_mac_link_up() is a new process-context caller, using spin_lock() here
introduces a new instance of the deadlock surface mentioned above against
softirq callers like airoha_dev_get_stats64().
Should this be changed to spin_lock_bh() as well?
[ ... ]
> +static int airoha_setup_phylink(struct net_device *netdev)
> +{
> + struct airoha_gdm_dev *dev = netdev_priv(netdev);
> + struct device_node *np = netdev->dev.of_node;
> + struct airoha_gdm_port *port = dev->port;
> + struct phylink_config *config;
> + phy_interface_t phy_mode;
> + struct phylink *phylink;
> + int err;
> +
> + err = of_get_phy_mode(np, &phy_mode);
> + if (err) {
> + dev_err(&netdev->dev, "incorrect phy-mode\n");
> + return err;
> + }
[Severity: Medium]
Does this mandatory check break backwards compatibility with older device trees?
If a device tree is missing the phy-mode property, this will now unconditionally
fail the driver probe, whereas the previous driver version allowed successful
probe without it.
Is there a fallback mode we should apply here instead of returning the error?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717065448.1498335-1-ansuelsmth@gmail.com?part=12
^ permalink raw reply
* Re: [PATCH net-next v9 11/12] net: pcs: airoha: add PCS driver for Airoha AN7581 SoC
From: sashiko-bot @ 2026-07-18 9:01 UTC (permalink / raw)
To: Christian Marangi; +Cc: conor+dt, devicetree, robh
In-Reply-To: <20260717065448.1498335-12-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] SPEED_10 configuration incorrectly sets MAC link mode to 100M instead of 10M.
- [High] AN_FORCE_CL37 auto-negotiation register is mistakenly written to the hsgmii_pcs regmap instead of hsgmii_an.
- [High] Generic PHY is acquired for USB port types but never initialized or powered on.
- [High] Stack variables are used uninitialized due to unchecked `regmap_read()` return values.
- [High] Initialization of a single port incorrectly resets and modifies shared hardware state (reset lines and PLLs) used by other active ports.
- [High] Zero-delay MMIO polling loop immediately after a hardware trigger causes unreliable signal detection.
- [Medium] Read-modify-write operation on W1C (write-1-to-clear) interrupt status registers accidentally clears unmasked interrupts.
--
--- Patch [11]: [PATCH net-next v9 11/12] net: pcs: airoha: add PCS driver for Airoha AN7581 SoC ---
commit 87ad2d5e05008754a1a8736668451b5aa64ba606
Author: Christian Marangi <ansuelsmth@gmail.com>
Date: Fri Jul 17 06:54:11 2026 +0000
net: pcs: airoha: add PCS driver for Airoha AN7581 SoC
This commit adds a new PCS driver for the Airoha AN7581 SoC, supporting
Ethernet, PON, PCIe, and USB SERDES. The driver provides support for various
operating modes including USXGMII, 10G-BASER, 2500BASE-X, 1000BASE-X, and
SGMII.
> diff --git a/drivers/net/pcs/airoha/pcs-airoha-common.c b/drivers/net/pcs/airoha/pcs-airoha-common.c
> new file mode 100644
> index 0000000000000..781f89adf3abf
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/pcs-airoha-common.c
> @@ -0,0 +1,1324 @@
[ ... ]
> +static int airoha_pcs_setup_scu(struct airoha_pcs_priv *priv,
> + int index, phy_interface_t interface)
> +{
> + const struct airoha_pcs_match_data *data = priv->data;
> + int ret;
> +
> + switch (data->port_type) {
> + case AIROHA_PCS_ETH:
> + airoha_pcs_setup_scu_eth(priv, interface);
> + break;
> + case AIROHA_PCS_PON:
> + airoha_pcs_setup_scu_pon(priv, interface);
> + break;
> + case AIROHA_PCS_PCIE:
> + airoha_pcs_setup_scu_pcie(priv, index, interface);
> + break;
> + case AIROHA_PCS_USB:
> + break;
> + }
> +
> + /* TODO better handle reset from MAC */
> + ret = reset_control_bulk_assert(ARRAY_SIZE(priv->rsts),
> + priv->rsts);
[Severity: High]
Does this assert shared device-level reset lines that might affect other
active ports on the MAC? If so, this could interrupt connections on those
ports during initialization.
> + if (ret)
> + return ret;
> +
> + ret = reset_control_bulk_deassert(ARRAY_SIZE(priv->rsts),
> + priv->rsts);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
[ ... ]
> +static void airoha_pcs_interrupt_init_usxgmii(struct airoha_pcs_priv *priv,
> + int index)
> +{
> + struct airoha_pcs_maps *maps = &priv->maps[index];
> +
> + /* Disable every Interrupt */
> + regmap_clear_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_CTRL_0,
> + AIROHA_PCS_USXGMII_T_TYPE_T_INT_EN |
> + AIROHA_PCS_USXGMII_T_TYPE_D_INT_EN |
> + AIROHA_PCS_USXGMII_T_TYPE_C_INT_EN |
> + AIROHA_PCS_USXGMII_T_TYPE_S_INT_EN);
> +
> + regmap_clear_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_CTRL_1,
> + AIROHA_PCS_USXGMII_R_TYPE_C_INT_EN |
> + AIROHA_PCS_USXGMII_R_TYPE_S_INT_EN |
> + AIROHA_PCS_USXGMII_TXPCS_FSM_ENC_ERR_INT_EN |
> + AIROHA_PCS_USXGMII_T_TYPE_E_INT_EN);
> +
> + regmap_clear_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_CTRL_2,
> + AIROHA_PCS_USXGMII_RPCS_FSM_DEC_ERR_INT_EN |
> + AIROHA_PCS_USXGMII_R_TYPE_E_INT_EN |
> + AIROHA_PCS_USXGMII_R_TYPE_T_INT_EN |
> + AIROHA_PCS_USXGMII_R_TYPE_D_INT_EN);
> +
> + regmap_clear_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_CTRL_3,
> + AIROHA_PCS_USXGMII_FAIL_SYNC_XOR_ST_INT_EN |
> + AIROHA_PCS_USXGMII_RX_BLOCK_LOCK_ST_INT_EN |
> + AIROHA_PCS_USXGMII_LINK_UP_ST_INT_EN |
> + AIROHA_PCS_USXGMII_HI_BER_ST_INT_EN);
> +
> + regmap_clear_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_CTRL_4,
> + AIROHA_PCS_USXGMII_LINK_DOWN_ST_INT_EN);
> +
> + /* Clear any pending interrupt */
> + regmap_set_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_INT_STA_2,
> + AIROHA_PCS_USXGMII_RPCS_FSM_DEC_ERR_INT |
> + AIROHA_PCS_USXGMII_R_TYPE_E_INT |
> + AIROHA_PCS_USXGMII_R_TYPE_T_INT |
> + AIROHA_PCS_USXGMII_R_TYPE_D_INT);
[Severity: Medium]
Will using a read-modify-write operation like regmap_set_bits on a
write-1-to-clear interrupt status register inadvertently read all pending
interrupts as 1s and write them back, thereby clearing all pending
interrupts and not just the masked ones?
> +
> + regmap_set_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_INT_STA_3,
> + AIROHA_PCS_USXGMII_FAIL_SYNC_XOR_ST_INT |
> + AIROHA_PCS_USXGMII_RX_BLOCK_LOCK_ST_INT |
> + AIROHA_PCS_USXGMII_LINK_UP_ST_INT |
> + AIROHA_PCS_USXGMII_HI_BER_ST_INT);
[ ... ]
> +static void airoha_pcs_get_state_sgmii(struct airoha_pcs_priv *priv,
> + unsigned int neg_mode, int index,
> + struct phylink_link_state *state)
> +{
> + struct airoha_pcs_maps *maps = &priv->maps[index];
> + u32 bmsr, lpa;
> +
> + regmap_read(maps->hsgmii_an, AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_1,
> + &bmsr);
> + regmap_read(maps->hsgmii_an, AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_5,
> + &lpa);
> +
> + bmsr = (AIROHA_PCS_HSGMII_AN_SGMII_AN_COMPLETE |
[Severity: High]
If regmap_read() fails here, wouldn't bmsr and lpa be left uninitialized?
Evaluating uninitialized stack variables might lead to random state machine
behavior. Can we add return value checks here?
> + AIROHA_PCS_HSGMII_AN_SGMII_REMOTE_FAULT |
> + AIROHA_PCS_HSGMII_AN_SGMII_AN_ABILITY |
> + AIROHA_PCS_HSGMII_AN_SGMII_LINK_STATUS) & bmsr;
> + lpa = AIROHA_PCS_HSGMII_AN_SGMII_PARTNER_ABILITY & lpa;
> +
> + phylink_mii_c22_pcs_decode_state(state, neg_mode, bmsr, lpa);
> +}
[ ... ]
> +static int airoha_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> + phy_interface_t interface,
> + const unsigned long *advertising,
> + bool permit_pause_to_mac)
> +{
[ ... ]
> + if (interface == PHY_INTERFACE_MODE_1000BASEX &&
> + neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) {
> + regmap_set_bits(maps->hsgmii_pcs, AIROHA_PCS_HSGMII_PCS_CTROL_1,
> + AIROHA_PCS_SGMII_SEND_AN_ERR_EN);
> +
> + regmap_set_bits(maps->hsgmii_pcs, AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_FORCE_CL37,
> + AIROHA_PCS_HSGMII_AN_FORCE_AN_DONE);
[Severity: High]
Is AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_FORCE_CL37 being written to the wrong
regmap here? It appears to be an auto-negotiation register that should be
written to the hsgmii_an block, rather than hsgmii_pcs.
> + }
> +
> + if (interface == PHY_INTERFACE_MODE_2500BASEX) {
> + regmap_set_bits(maps->hsgmii_an, AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_0,
> + AIROHA_PCS_HSGMII_AN_SGMII_RESET_PHY);
> + }
[ ... ]
> +static void airoha_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
> + phy_interface_t interface, int speed, int duplex)
> +{
[ ... ]
> + if (interface == PHY_INTERFACE_MODE_SGMII ||
> + interface == PHY_INTERFACE_MODE_2500BASEX) {
> + u32 ck_gen_mode;
> + u32 speed_reg;
> + u32 if_mode;
> +
> + switch (speed) {
> + case SPEED_2500:
> + speed_reg = AIROHA_PCS_LINK_MODE_P0_2_5G;
> + break;
> + case SPEED_1000:
> + speed_reg = AIROHA_PCS_LINK_MODE_P0_1G;
> + if_mode = AIROHA_PCS_HSGMII_AN_SPEED_FORCE_MODE_1000;
> + ck_gen_mode = AIROHA_PCS_HSGMII_PCS_FORCE_CUR_SGMII_MODE_1000;
> + break;
> + case SPEED_100:
> + speed_reg = AIROHA_PCS_LINK_MODE_P0_100M;
> + if_mode = AIROHA_PCS_HSGMII_AN_SPEED_FORCE_MODE_100;
> + ck_gen_mode = AIROHA_PCS_HSGMII_PCS_FORCE_CUR_SGMII_MODE_100;
> + break;
> + case SPEED_10:
> + speed_reg = AIROHA_PCS_LINK_MODE_P0_100M;
> + if_mode = AIROHA_PCS_HSGMII_AN_SPEED_FORCE_MODE_10;
> + ck_gen_mode = AIROHA_PCS_HSGMII_PCS_FORCE_CUR_SGMII_MODE_10;
[Severity: High]
Does SPEED_10 mistakenly assign the 100M constant to speed_reg instead of
AIROHA_PCS_LINK_MODE_P0_10M? This looks like it might incorrectly program the
hardware to 100Mbps.
> + break;
> + }
> +
> + if (interface == PHY_INTERFACE_MODE_SGMII) {
> + regmap_update_bits(maps->hsgmii_an, AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_13,
> + AIROHA_PCS_HSGMII_AN_SPEED_FORCE_MODE,
> + if_mode);
[ ... ]
> +static int airoha_pcs_probe(struct platform_device *pdev)
> +{
> + const struct airoha_pcs_match_data *data;
> + struct device *dev = &pdev->dev;
> + struct airoha_pcs_priv *priv;
> + int index, ret;
> +
> + data = of_device_get_match_data(dev);
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->ports = devm_kcalloc(dev, data->num_port,
> + sizeof(*priv->ports), GFP_KERNEL);
> + if (!priv->ports)
> + return -ENOMEM;
> +
> + priv->dev = dev;
> + priv->data = data;
> +
> + if (data->port_type == AIROHA_PCS_USB) {
> + struct phy *phy;
> +
> + phy = devm_phy_get(dev, NULL);
> + if (IS_ERR(phy))
> + return dev_err_probe(dev, PTR_ERR(phy), "failed to get phy\n");
> +
> + priv->phy = phy;
[Severity: High]
The generic PHY is acquired for USB port types, but does it ever get
initialized or powered on anywhere else in the driver? It appears the hardware
might remain powered down without a call to phy_init() and phy_power_on().
> + }
> +
> + switch (data->port_type) {
[ ... ]
> diff --git a/drivers/net/pcs/airoha/pcs-an7581.c b/drivers/net/pcs/airoha/pcs-an7581.c
> new file mode 100644
> index 0000000000000..24fc15b6e29e1
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/pcs-an7581.c
> @@ -0,0 +1,2093 @@
[ ... ]
> +static void an7581_pcs_jcpll_bringup(struct airoha_pcs_priv *priv,
> + int index, phy_interface_t interface)
> +{
> + struct regmap_field **pcs_ana_fields = priv->pcs_ana_fields[index];
> + struct regmap *pcs_pma;
> + u32 kband_vref;
> +
> + switch (interface) {
> + case PHY_INTERFACE_MODE_SGMII:
> + case PHY_INTERFACE_MODE_1000BASEX:
> + case PHY_INTERFACE_MODE_2500BASEX:
> + kband_vref = 0x10;
> + break;
> + case PHY_INTERFACE_MODE_USXGMII:
> + case PHY_INTERFACE_MODE_10GBASER:
> + kband_vref = 0xf;
> + break;
> + default:
> + return;
> + }
> +
> + /* This comment only apply to Serdes PCIe that expose
> + * 2 PCS.
> + *
> + * The Serdes PCIe expose 2 PCS but always require
> + * the PMA for the first PCS to be configured
> + * for correct functionality for JCPLL.
> + */
> + pcs_pma = priv->pcs_pma[0];
> +
> + /* Setup LDO */
> + usleep_range(200, 300);
> +
> + regmap_field_set_bits(pcs_ana_fields[AN7581_PCS_JCPLL_SPARE_L],
> + AIROHA_PCS_ANA_JCPLL_SPARE_L_LDO);
> +
> + /* Setup RSTB */
> + regmap_field_write(pcs_ana_fields[AN7581_PCS_JCPLL_RST_DLY],
> + AIROHA_PCS_ANA_JCPLL_RST_DLY_150_200);
> + regmap_field_write(pcs_ana_fields[AN7581_PCS_JCPLL_PLL_RSTB], 0x1);
[Severity: High]
Does this code unconditionally modify port 0's JCPLL state and reset it, even
when port 1 is being configured? If so, this could potentially destroy the
active hardware state of the other port.
> +
> + /* Enable PLL force selection and Force Disable */
> + regmap_update_bits(pcs_pma, AIROHA_PCS_PMA_PXP_JCPLL_CKOUT_EN,
> + AIROHA_PCS_PMA_FORCE_SEL_DA_JCPLL_EN |
> + AIROHA_PCS_PMA_FORCE_DA_JCPLL_EN,
> + AIROHA_PCS_PMA_FORCE_SEL_DA_JCPLL_EN);
[ ... ]
> +static bool an7581_pcs_have_rx_signal(struct airoha_pcs_priv *priv, int index)
> +{
> + struct regmap *pcs_pma = priv->pcs_pma[index];
> + unsigned int count = 0;
> + u32 val;
> + int i;
> +
> + regmap_write(pcs_pma, AIROHA_PCS_PMA_DIG_RESERVE_0,
> + AIROHA_PCS_TRIGGER_RX_SIDGET_SCAN);
> +
> + /* Scan 6 times for RX sigdet module to detect RX signal */
> + for (i = 0; i < AIROHA_PCS_MAX_RX_SIGDET_TRY; i++) {
> + regmap_read(pcs_pma, AIROHA_PCS_PMA_DIG_RO_RESERVE_2,
> + &val);
[Severity: High]
Are these back-to-back regmap reads executed immediately after the hardware
trigger without any delay? Since analog signal scans usually require some
settling time, could the loop finish before the hardware completes the scan,
resulting in unreliable signal detection?
> + if (val & AIROHA_PCS_RX_SIGDET)
> + count++;
> + }
> +
> + /* Consider signal presence if we detect signal at least 4 times */
> + return count >= AIROHA_PCS_MAX_RX_SIGDET_PRESENCE_CNT;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717065448.1498335-1-ansuelsmth@gmail.com?part=11
^ permalink raw reply
* Re: [PATCH net-next v9 09/12] net: phylink: add .pcs_link_down PCS OP
From: sashiko-bot @ 2026-07-18 9:01 UTC (permalink / raw)
To: Christian Marangi; +Cc: devicetree, conor+dt, robh
In-Reply-To: <20260717065448.1498335-10-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The kernel-doc comment for `pcs_link_down()` incorrectly names the function as `pcs_link_up()`, due to a copy-paste error.
--
--- Patch [9]: [PATCH net-next v9 09/12] net: phylink: add .pcs_link_down PCS OP ---
commit 44243b57b5aa440d9df2c5da5110db53f51717a0
Author: Christian Marangi <ansuelsmth@gmail.com>
net: phylink: add .pcs_link_down PCS OP
This commit adds a .pcs_link_down operation to the phylink_pcs_ops
structure, allowing PCS drivers to define specific link teardown behavior.
The new callback is invoked during phylink_link_down() immediately after
the MAC link is brought down.
[ ... ]
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 15e6b1a39dfe2..ecf4c384fd31c 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -690,6 +692,16 @@ void pcs_an_restart(struct phylink_pcs *pcs);
> void pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
> phy_interface_t interface, int speed, int duplex);
>
> +/**
> + * pcs_link_up() - tear down link between MAC and PCS
[Severity: Low]
This isn't a bug, but should this kernel-doc name be pcs_link_down() to match
the function prototype below it? This copy-paste typo will likely cause a
kernel-doc documentation build warning.
> + * @pcs: a pointer to a &struct phylink_pcs.
> + *
> + * This call will be made just after mac_link_down() to inform the PCS the
> + * link has gone down. PCS should be configured to stop processing packets
> + * for transmission and reception.
> + */
> +void pcs_link_down(struct phylink_pcs *pcs);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717065448.1498335-1-ansuelsmth@gmail.com?part=9
^ permalink raw reply
* Re: [PATCH net-next v9 08/12] of: property: fw_devlink: Add support for "pcs-handle"
From: sashiko-bot @ 2026-07-18 9:01 UTC (permalink / raw)
To: Christian Marangi; +Cc: conor+dt, devicetree, robh
In-Reply-To: <20260717065448.1498335-9-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Adding `pcs-handle` to `fw_devlink` supplier bindings defeats the dynamic 'late PCS provider attach' and hotplug logic in `phylink`.
--
--- Patch [8]: [PATCH net-next v9 08/12] of: property: fw_devlink: Add support for "pcs-handle" ---
commit f6fc253d355d434feccac1a9dee68ebce25d5fe8
Author: Christian Marangi <ansuelsmth@gmail.com>
of: property: fw_devlink: Add support for "pcs-handle"
This commit adds support for parsing the "pcs-handle" binding, allowing
fw_devlink to enforce device dependencies with Ethernet ports.
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 72cf12907de03..91c3e3931fa6e 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
[ ... ]
> @@ -1571,6 +1572,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
> { .parse_prop = parse_interrupts, },
> { .parse_prop = parse_interrupt_map, },
> { .parse_prop = parse_access_controllers, },
> + { .parse_prop = parse_pcs_handle, },
[Severity: High]
Does enforcing a strict supplier-consumer dependency here conflict with the
dynamic hotplug and late-attach logic in phylink?
If fw_devlink creates a managed device link for pcs-handle, it enforces strict
boot ordering. This prevents the MAC from probing before the PCS, which blocks
phylink's late-attach feature.
Additionally, because fw_devlink applies DL_FLAG_AUTOREMOVE_CONSUMER, unloading
the PCS module would cause the driver core to unceremoniously unbind the MAC
driver and remove the network interface. This circumvents phylink's graceful
link down handling via phylink_release_pcs().
Since similar properties like phy-handle are intentionally omitted from
of_supplier_bindings to allow for dynamic PHY/PCS presence, should pcs-handle
also be excluded to preserve these phylink features?
> { .parse_prop = parse_regulators, },
> { .parse_prop = parse_gpio, },
> { .parse_prop = parse_gpios, },
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717065448.1498335-1-ansuelsmth@gmail.com?part=8
^ permalink raw reply
* Re: [PATCH net-next v9 06/12] net: Document PCS subsystem
From: sashiko-bot @ 2026-07-18 9:01 UTC (permalink / raw)
To: Christian Marangi; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260717065448.1498335-7-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] phylink_create() returns an invalid non-error pointer (or NULL) on invalid config->type.
--
--- Patch [6]: [PATCH net-next v9 06/12] net: Document PCS subsystem ---
commit 867ad4be39467be718a5df59cf2a9739b1c43531
Author: Christian Marangi <ansuelsmth@gmail.com>
net: phylink: introduce internal phylink PCS handling
This commit introduces internal handling of PCS for phylink, moving the
selection logic to phylink using the supported_interface value. It updates
phylink_create() to allocate and populate an array of PCS pointers from the
MAC, creating a linked list of available PCS devices. The changes also include
new validation to ensure a selected PCS supports the requested interfaces.
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 3464e3ec56997..d96941d4c5235 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -1854,13 +1964,17 @@ struct phylink *phylink_create(struct phylink_config *config,
[ ... ]
> + /* Fill the PCS list with available PCS from phylink config */
> + ret = phylink_fill_available_pcs(pl, config);
> + if (ret < 0)
> + goto free_pl;
[ ... ]
> pl->config = config;
> if (config->type == PHYLINK_NETDEV) {
> pl->netdev = to_net_dev(config->dev);
> @@ -1882,8 +2012,7 @@ struct phylink *phylink_create(struct phylink_config *config,
> } else if (config->type == PHYLINK_DEV) {
> pl->dev = config->dev;
> } else {
> - kfree(pl);
> - return ERR_PTR(-EINVAL);
> + goto unlink_pcs_list;
[Severity: High]
Can this jump to unlink_pcs_list return an invalid pointer instead of an error?
If config->type is invalid, the code jumps to unlink_pcs_list without setting
'ret = -EINVAL'. Because 'ret' still holds the non-negative result from
phylink_fill_available_pcs(), ERR_PTR(ret) will return a valid-looking pointer
(or NULL) instead of an error pointer.
This would bypass the caller's IS_ERR() check and could lead to a crash when
they attempt to use the returned phylink pointer.
> }
>
[ ... ]
> +unlink_pcs_list:
> + list_for_each_entry(pcs, &pl->pcs_list, list)
> + pcs->phylink = NULL;
> +free_pl:
> + kfree(pl);
> + return ERR_PTR(ret);
> }
> EXPORT_SYMBOL_GPL(phylink_create);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717065448.1498335-1-ansuelsmth@gmail.com?part=6
^ permalink raw reply
* Re: [PATCH net-next v9 04/12] net: pcs: implement Firmware node support for PCS driver
From: sashiko-bot @ 2026-07-18 9:01 UTC (permalink / raw)
To: Christian Marangi; +Cc: devicetree, robh, conor+dt
In-Reply-To: <20260717065448.1498335-5-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The PCS provider API lacks module reference counting or device linking, creating a use-after-free vulnerability.
- [High] Ignoring missing PCS providers breaks probe deferral and corrupts the mapping of the retrieved PCS array via unintended array squashing.
- [High] Potential AB-BA deadlock due to calling provider callback while holding global fwnode_pcs_mutex.
- [Low] API Misuse: Passing -1 to the unsigned int nargs parameter.
--
--- Patch [4]: [PATCH net-next v9 04/12] net: pcs: implement Firmware node support for PCS driver ---
commit 9d0c6df31924499c98bee1b3b6194d084ae4c002
Author: Christian Marangi <ansuelsmth@gmail.com>
net: pcs: implement Firmware node support for PCS driver
Implement the foundation of Firmware node support for PCS driver.
To support this, implement a simple Provider API where a PCS driver can
expose multiple PCS with an xlate .get function.
> diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
> new file mode 100644
> index 0000000000000..0cc4daf7beeae
> --- /dev/null
> +++ b/drivers/net/pcs/pcs.c
> @@ -0,0 +1,212 @@
[ ... ]
> +int fwnode_pcs_add_provider(struct fwnode_handle *fwnode,
> + struct phylink_pcs *(*get)(struct fwnode_reference_args *pcsspec,
> + void *data),
> + void *data)
> +{
[Severity: High]
Does this API need a way to track module ownership or establish device links?
Since providers don't pass their module owner (e.g., THIS_MODULE) or a device
pointer during registration, consumers can't hold a reference to the provider
module. If a provider module is unloaded, could it leave consumers with
dangling phylink_pcs pointers resulting in a use-after-free?
> + struct fwnode_pcs_provider *pp;
[ ... ]
> +static int fwnode_parse_pcsspec(const struct fwnode_handle *fwnode,
> + int index, const char *name,
> + struct fwnode_reference_args *out_args)
> +{
[ ... ]
> + ret = fwnode_property_get_reference_args(fwnode, "pcs-handle",
> + "#pcs-cells",
> + -1, index, out_args);
[Severity: Low]
Is it intentional to pass -1 to the nargs parameter of
fwnode_property_get_reference_args()? Since nargs is an unsigned int, this
will be implicitly cast to UINT_MAX.
> + if (ret || (name && index < 0))
> + return ret;
> +
> + return 0;
> +}
> +
> +static struct phylink_pcs *
> +fwnode_pcs_get_from_pcsspec(struct fwnode_reference_args *pcsspec)
> +{
> + struct fwnode_pcs_provider *provider;
> + struct phylink_pcs *pcs = ERR_PTR(-ENODEV);
[Severity: High]
Should this default to -EPROBE_DEFER instead of -ENODEV? If a provider hasn't
probed yet, returning -ENODEV will prevent the consumer from correctly
deferring its probe.
> +
> + if (!pcsspec)
> + return ERR_PTR(-EINVAL);
> +
> + mutex_lock(&fwnode_pcs_mutex);
> + list_for_each_entry(provider, &fwnode_pcs_providers, link) {
> + if (provider->fwnode == pcsspec->fwnode) {
> + pcs = provider->get(pcsspec, provider->data);
[Severity: High]
Can calling the provider's get() callback while holding the global
fwnode_pcs_mutex cause an AB-BA deadlock?
If an advanced provider needs to acquire a driver-local lock in its get()
callback, it creates a lock order of fwnode_pcs_mutex -> local_lock.
During provider removal, the driver might acquire local_lock before calling
fwnode_pcs_del_provider(), which then tries to acquire fwnode_pcs_mutex.
> + if (!IS_ERR(pcs))
> + break;
> + }
> + }
> + mutex_unlock(&fwnode_pcs_mutex);
> +
> + return pcs;
> +}
> +
> +static struct phylink_pcs *__fwnode_pcs_get(struct fwnode_handle *fwnode,
> + unsigned int index, const char *con_id)
> +{
> + struct fwnode_reference_args pcsspec;
> + struct phylink_pcs *pcs;
> + int ret;
> +
> + ret = fwnode_parse_pcsspec(fwnode, index, con_id, &pcsspec);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + pcs = fwnode_pcs_get_from_pcsspec(&pcsspec);
> + fwnode_handle_put(pcsspec.fwnode);
> +
> + return pcs;
[Severity: High]
Since this returns a bare pointer to the consumer without taking a reference
on the provider module, and there is no corresponding put() API, what prevents
the provider from freeing the PCS memory while the consumer is still using it?
> +}
[ ... ]
> +unsigned int fwnode_phylink_pcs_count(struct fwnode_handle *fwnode)
> +{
> + struct fwnode_reference_args out_args;
> + int index = 0;
> + int ret;
> +
> + while (true) {
> + ret = fwnode_property_get_reference_args(fwnode, "pcs-handle",
> + "#pcs-cells",
> + -1, index, &out_args);
[Severity: Low]
Is this another location where we are passing -1 to an unsigned int parameter?
> + /* We expect to reach an -ENOENT error while counting */
> + if (ret)
> + break;
[ ... ]
> +int fwnode_phylink_pcs_parse(struct fwnode_handle *fwnode,
> + struct phylink_pcs **available_pcs,
> + unsigned int num_pcs)
> +{
> + unsigned int i, found = 0;
[ ... ]
> + for (i = 0; i < num_pcs; i++) {
> + struct phylink_pcs *pcs;
> +
> + pcs = fwnode_pcs_get(fwnode, i);
> + if (IS_ERR(pcs)) {
> + /* Exit early if no PCS remain.*/
> + if (PTR_ERR(pcs) == -ENOENT)
> + break;
> +
> + /*
> + * Ignore -ENODEV error for PCS that still
> + * needs to probe.
> + */
> + if (PTR_ERR(pcs) == -ENODEV)
> + continue;
[Severity: High]
Does ignoring -ENODEV and continuing without incrementing the "found" index
corrupt the hardware mapping for the MAC driver?
By not storing a placeholder or deferring probe, any subsequently probed PCS
will be shifted into the wrong index in the available_pcs array. If the MAC
driver expects index i to map to physical lane i, won't this result in
incorrect routing?
> +
> + return PTR_ERR(pcs);
> + }
> +
> + available_pcs[found] = pcs;
> + found++;
> + }
> +
> + return found;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717065448.1498335-1-ansuelsmth@gmail.com?part=4
^ permalink raw reply
* Re: [PATCH net-next v9 02/12] net: phylink: introduce internal phylink PCS handling
From: sashiko-bot @ 2026-07-18 9:01 UTC (permalink / raw)
To: Christian Marangi; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260717065448.1498335-3-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] phylink_validate_mac_and_pcs() validates an interface as supported even when its required dedicated PCS is missing.
- [High] Missing list_del() in the unlink_pcs_list error path of phylink_create() leaves dangling list pointers in externally-owned PCS structures.
- [Medium] phylink_fill_available_pcs() uses list_add() instead of list_add_tail(), reversing the intended priority order of the provided PCS array.
--
--- Patch [2]: [PATCH net-next v9 02/12] net: phylink: introduce internal phylink PCS handling ---
commit 867ad4be39467be718a5df59cf2a9739b1c43531
Author: Christian Marangi <ansuelsmth@gmail.com>
Subject: net: phylink: introduce internal phylink PCS handling
Introduces an alternative to mac_select_pcs by shifting the PCS selection logic
entirely to phylink using the supported_interface value in the PCS struct.
A new pcs_list tracks available PCS instances.
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 3464e3ec56997..d96941d4c5235 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -530,9 +552,24 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,
> pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
> if (IS_ERR(pcs))
> return PTR_ERR(pcs);
> +
> + pcs_found = !!pcs;
> + /*
> + * Find a PCS in available PCS list for the requested interface.
> + *
> + * Skip searching if the MAC doesn't require a dedicated PCS for
> + * the requested interface.
> + */
> + } else if (test_bit(state->interface, pl->config->pcs_interfaces)) {
> + list_for_each_entry(pcs, &pl->pcs_list, list) {
> + if (!phylink_validate_pcs_interface(pcs, state->interface)) {
> + pcs_found = true;
> + break;
> + }
> + }
> }
>
> - if (pcs) {
> + if (pcs_found) {
[Severity: High]
If the MAC requires a dedicated PCS (test_bit returns true) but no available PCS
supports the requested interface, pcs_found remains false. Does this wrongly
skip the PCS validation block and proceed to MAC capabilities validation without
returning an error?
If so, it seems like phylink_major_config() will later fail when it performs
this same lookup, resulting in a delayed setup failure. Should we return -EINVAL
here if pcs_found is false but a dedicated PCS is required?
[ ... ]
> +static int phylink_fill_available_pcs(struct phylink *pl,
> + struct phylink_config *config)
> +{
[ ... ]
> + for (i = 0; i < config->num_possible_pcs; i++) {
> + struct phylink_pcs *pcs = pcss[i];
> +
> + if (!pcs)
> + continue;
> +
> + list_add(&pcs->list, &pl->pcs_list);
> + }
[Severity: Medium]
Since list_add() inserts elements at the head of the list, does this reverse the
priority order of the pcss array provided by the MAC driver?
Because lookups return the first matching PCS in the list, would using
list_add_tail() be needed to ensure the highest-priority PCS from the array is
correctly chosen?
[ ... ]
> @@ -1854,13 +1964,17 @@ struct phylink *phylink_create(struct phylink_config *config,
[ ... ]
> return pl;
> +
> +unlink_pcs_list:
> + list_for_each_entry(pcs, &pl->pcs_list, list)
> + pcs->phylink = NULL;
> +free_pl:
> + kfree(pl);
> + return ERR_PTR(ret);
> }
> EXPORT_SYMBOL_GPL(phylink_create);
[Severity: High]
If phylink_create() encounters an error and takes the unlink_pcs_list path, does
omitting list_del(&pcs->list) leave dangling pointers in the externally-owned
phylink_pcs structures once the phylink instance is freed?
If a MAC driver defers probe or retries initialization, passing these same
structures back to phylink_create() could trigger list corruption or a
use-after-free since their list nodes would still point to the freed
pl->pcs_list.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717065448.1498335-1-ansuelsmth@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH net-next v9 01/12] net: phylink: keep and use MAC supported_interfaces in phylink struct
From: sashiko-bot @ 2026-07-18 9:01 UTC (permalink / raw)
To: Christian Marangi; +Cc: devicetree, robh, conor+dt
In-Reply-To: <20260717065448.1498335-2-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] In `phylink_create()`, if `phylink_register_sfp()` fails, the function frees `pl` without releasing the GPIO descriptor `pl->link_gpio`.
--
--- Patch [1]: [PATCH net-next v9 01/12] net: phylink: keep and use MAC supported_interfaces in phylink struct ---
commit 34f151721de84035e589a96b730a7b88905f81c9
Author: Christian Marangi <ansuelsmth@gmail.com>
net: phylink: keep and use MAC supported_interfaces in phylink struct
Adds a copy of supported_interfaces to the phylink struct and makes use of
that instead of relying on the phylink_config value. This prepares for
internal PCS handling where a PCS can be added or removed after creation.
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 59dfe35afa54f..3464e3ec56997 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -1867,6 +1872,9 @@ struct phylink *phylink_create(struct phylink_config *config,
> mutex_init(&pl->state_mutex);
> INIT_WORK(&pl->resolve, phylink_resolve);
>
> + phy_interface_copy(pl->supported_interfaces,
> + config->supported_interfaces);
> +
> pl->config = config;
[Severity: High]
This isn't a bug introduced by this patch, but while reviewing
phylink_create(), I noticed a potential resource leak in its error handling.
If phylink_create() processes a fixed-link configuration that acquires a
GPIO descriptor (pl->link_gpio in phylink_parse_fixedlink()), and then the
subsequent call to phylink_register_sfp() returns an error like
-EPROBE_DEFER, the function frees pl but does not release the pl->link_gpio
descriptor:
drivers/net/phy/phylink.c:phylink_create() {
...
ret = phylink_register_sfp(pl, fwnode);
if (ret < 0) {
kfree(pl);
return ERR_PTR(ret);
}
...
}
Does this cause the GPIO descriptor to remain held, preventing subsequent
probe attempts from succeeding since the GPIO request will fail with -EBUSY?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717065448.1498335-1-ansuelsmth@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH net-next v9 03/12] net: phylink: add phylink_release_pcs() to externally release a PCS
From: sashiko-bot @ 2026-07-18 9:01 UTC (permalink / raw)
To: Christian Marangi; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260717065448.1498335-4-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unconditional call to phylink_link_down() without updating old_link_state leads to multiple mac_link_down() calls and state machine desynchronization.
- [Medium] The phylink instance's supported ethtool link modes and advertising mask are not updated after a PCS is removed.
- [Medium] Polling timer is not stopped when the active polling PCS is released, causing a runaway timer.
- [Low] Reusing the function parameter 'pcs' as a loop iterator variable violates C scoping best practices and kernel coding standards.
--
--- Patch [3]: [PATCH net-next v9 03/12] net: phylink: add phylink_release_pcs() to externally release a PCS ---
commit 3d6c63e283b60421054ef189e8d1a2deb1f29536
Author: Christian Marangi <ansuelsmth@gmail.com>
net: phylink: add phylink_release_pcs() to externally release a PCS
Add phylink_release_pcs() to externally release a PCS from a phylink
instance. This can be used to handle case when a single PCS needs to be
removed and the phylink instance needs to be refreshed.
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index d96941d4c5235..986e0c1e3d678 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -917,6 +919,60 @@ static void phylink_resolve_an_pause(struct phylink_link_state *state)
[ ... ]
> +void phylink_release_pcs(struct phylink_pcs *pcs)
> +{
> + struct phylink *pl;
> +
> + ASSERT_RTNL();
> +
> + pl = pcs->phylink;
> + if (!pl)
> + return;
> +
> + mutex_lock(&pl->state_mutex);
> +
> + list_del(&pcs->list);
> + pcs->phylink = NULL;
> +
> + /*
> + * Check if we are removing the PCS currently
> + * in use by phylink. If this is the case, tear down
> + * the link, force phylink resolve to reconfigure the
> + * interface mode, disable the current PCS and set the
> + * phylink PCS to NULL.
> + */
> + if (pl->pcs == pcs) {
> + phylink_link_down(pl);
> + phylink_pcs_disable(pl->pcs);
> +
> + pl->force_major_config = true;
> + pl->pcs = NULL;
> + }
[Severity: High]
Does calling phylink_link_down() here without also updating pl->old_link_state
cause state machine desynchronization?
If pl->old_link_state is true when the PCS is removed (such as for DSA switch
ports where there is no netdev), the link is logically brought down, but
pl->old_link_state remains true. This could cause subsequent link-up events to
be ignored because phylink thinks the link is already up, leaving the
interface permanently dead.
[Severity: Medium]
When the active PCS has polling enabled (pcs->poll is true), does clearing
pl->pcs and triggering a resolve leave a runaway timer?
If phylink_major_config() is called by the resolve task and no replacement PCS
is found, it may return early on error and bypass the phylink_pcs_poll_stop()
call. Could this cause the original timer to continue firing and re-arming
itself indefinitely?
> +
> + mutex_unlock(&pl->state_mutex);
> +
> + /* Refresh supported interfaces */
> + phy_interface_copy(pl->supported_interfaces,
> + pl->config->supported_interfaces);
> + list_for_each_entry(pcs, &pl->pcs_list, list)
> + phy_interface_or(pl->supported_interfaces,
> + pl->supported_interfaces,
> + pcs->supported_interfaces);
[Severity: Low]
This isn't a bug, but does reusing the function parameter pcs as the list
iterator variable overwrite its pointer value?
When the loop terminates, pcs will point to an invalid structure offset
derived from the list head. Using a separate iterator variable would avoid
corrupting the parameter.
[Severity: Medium]
While pl->supported_interfaces is updated here, is it also necessary to
update the phylink instance's supported ethtool link modes and advertising
mask?
Without calling phylink_validate() or similar to update pl->supported and
pl->link_config.advertising, these masks might continue to advertise the
capabilities of the removed PCS. Could this lead to runtime connection
failures if userspace or the PHY attempts to negotiate these now-unsupported
modes?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717065448.1498335-1-ansuelsmth@gmail.com?part=3
^ permalink raw reply
* Re: [PATCH net-next v9 05/12] net: phylink: support late PCS provider attach
From: sashiko-bot @ 2026-07-18 9:01 UTC (permalink / raw)
To: Christian Marangi; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260717065448.1498335-6-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Missing PCS removal notification leads to Use-After-Free when a PCS module is unloaded.
- [High] A TOCTOU race condition in `phylink_create()` can cause a newly added PCS provider to be permanently missed.
- [High] Unconditional `list_add()` in `pcs_provider_notify()` causes doubly-linked list corruption during concurrent additions.
- [High] Late PCS attachment fails for multi-port devices (e.g., DSA switches) due to incorrect fwnode lookup.
- [Medium] Synchronous acquisition of `rtnl_lock()` in the blocking notifier chain introduces a fragile self-deadlock risk.
--
--- Patch [5]: [PATCH net-next v9 05/12] net: phylink: support late PCS provider attach ---
commit 0738f7b893f648f2683558ff4efb6327359c5920
Author: Christian Marangi <ansuelsmth@gmail.com>
net: phylink: support late PCS provider attach
Add support for late PCS provider attachment to a phylink instance.
This works by creating a global notifier for the PCS provider and
making each phylink instance that makes use of fwnode subscribe to
this notifier.
[ ... ]
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 986e0c1e3d678..1d83b31874fb2 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -1999,6 +2001,53 @@ static int phylink_fill_available_pcs(struct phylink *pl,
> return ret;
> }
>
> +static int pcs_provider_notify(struct notifier_block *self,
> + unsigned long val, void *data)
> +{
> + struct phylink *pl = container_of(self, struct phylink, fwnode_pcs_nb);
> + struct fwnode_handle *pcs_fwnode = data;
> + struct phylink_pcs *pcs;
> +
> + rtnl_lock();
[Severity: Medium]
Does unconditional acquisition of rtnl_lock() here risk a deadlock?
If a network driver registers a PCS provider from a context that already
holds rtnl_lock() (which is common in network driver probe or open paths),
wouldn't the synchronous notifier callback attempt to recursively acquire
the lock and hang the system?
> +
> + /* Check if the just added PCS provider is
> + * in the phylink instance pcs-handle property.
> + */
> + pcs = fwnode_phylink_pcs_get_from_fwnode(dev_fwnode(pl->config->dev),
> + pcs_fwnode);
[Severity: High]
Will this lookup fail for multi-port devices like DSA switches?
It seems to fetch the fwnode from the parent device (pl->config->dev) rather
than the port-specific fwnode passed during phylink creation. If the
pcs-handle property is in the port's subnode, will this return an error and
silently abort the late attachment?
> + if (IS_ERR(pcs)) {
> + rtnl_unlock();
> + return NOTIFY_DONE;
> + }
> +
> + /* Add the PCS */
> + mutex_lock(&pl->state_mutex);
> +
> + /* Link PCS with phylink */
> + list_add(&pcs->list, &pl->pcs_list);
[Severity: High]
Could this corrupt the doubly-linked list if the PCS is already present?
If a PCS provider is added concurrently, it might already be picked up by
phylink_fill_available_pcs() and then passed to this notifier. Calling
list_add() unconditionally on a node already in the list overwrites its
pointers and causes list corruption.
> + pcs->phylink = pl;
> +
[ ... ]
> @@ -2116,6 +2165,12 @@ struct phylink *phylink_create(struct phylink_config *config,
> if (ret < 0)
> goto unlink_pcs_list;
>
> + /* Register notifier for late PCS attach */
> + if (!phy_interface_empty(config->pcs_interfaces)) {
> + pl->fwnode_pcs_nb.notifier_call = pcs_provider_notify;
> + register_fwnode_pcs_notifier(&pl->fwnode_pcs_nb);
> + }
[Severity: High]
Is there a race condition between fetching available PCS providers and
registering this notifier?
In phylink_create(), the list of available PCS providers is fetched earlier
via phylink_fill_available_pcs(). If a PCS provider calls
fwnode_pcs_add_provider() exactly after the list is fetched but before
register_fwnode_pcs_notifier() is called here, wouldn't the provider be
permanently missed since it wasn't in the global list initially and the
notification was missed?
> return pl;
>
> unlink_pcs_list:
[ ... ]
> diff --git a/include/linux/pcs/pcs.h b/include/linux/pcs/pcs.h
> index b7cfdd680b2ab..45e8f96662db2 100644
> --- a/include/linux/pcs/pcs.h
> +++ b/include/linux/pcs/pcs.h
> @@ -4,7 +4,36 @@
>
> #include <linux/phylink.h>
>
> +enum fwnode_pcs_notify_event {
> + FWNODE_PCS_PROVIDER_ADD,
> +};
[Severity: Critical]
Does the lack of a removal notification event create a use-after-free risk?
If a PCS provider module is unloaded, fwnode_pcs_del_provider() is called to
free internal state, but phylink is never notified because there is no
FWNODE_PCS_PROVIDER_DEL event.
Won't the freed pointer remain linked in pl->pcs_list and be dereferenced
during the next phylink_resolve() run triggered by a timer or link state
change?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717065448.1498335-1-ansuelsmth@gmail.com?part=5
^ permalink raw reply
* [PATCH v3 4/4] arm64: dts: rockchip: enable HDMI RX audio capture on Orange Pi 5 Plus
From: Igor Paunovic @ 2026-07-18 8:57 UTC (permalink / raw)
To: Dmitry Osipenko, Mauro Carvalho Chehab
Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, linux-media, linux-rockchip, linux-arm-kernel,
kernel, devicetree, linux-kernel, Igor Paunovic
In-Reply-To: <20260718085728.6797-1-royalnet026@gmail.com>
Route the HDMI receiver audio to i2s7_8ch, the receive-only I2S
interface dedicated to HDMI RX, through a simple-audio-card with the
receiver as bitclock and frame master. Together with the audio
capture support in the snps_hdmirx driver this exposes a capture-only
ALSA card fed by the HDMI input.
Tested on the Orange Pi 5 Plus with multiple HDMI sources: capture
follows the source sample rate and stays in sync via the FIFO-level
clock tracking in the driver.
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
New in v3.
.../dts/rockchip/rk3588-orangepi-5-plus.dts | 20 +++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts b/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
index 9950d11..d5840e1 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
@@ -37,6 +37,22 @@
};
};
+ hdmiin-sound {
+ compatible = "simple-audio-card";
+ simple-audio-card,name = "rockchip,hdmiin";
+ simple-audio-card,format = "i2s";
+ simple-audio-card,bitclock-master = <&hdmiin_codec>;
+ simple-audio-card,frame-master = <&hdmiin_codec>;
+
+ simple-audio-card,cpu {
+ sound-dai = <&i2s7_8ch>;
+ };
+
+ hdmiin_codec: simple-audio-card,codec {
+ sound-dai = <&hdmi_receiver 0>;
+ };
+ };
+
ir-receiver {
compatible = "gpio-ir-receiver";
gpios = <&gpio4 RK_PB3 GPIO_ACTIVE_LOW>;
@@ -247,6 +263,10 @@
status = "okay";
};
+&i2s7_8ch {
+ status = "okay";
+};
+
&led_blue_gpio {
gpios = <&gpio3 RK_PA6 GPIO_ACTIVE_HIGH>;
status = "okay";
--
2.53.0
^ permalink raw reply related
* [PATCH v3 3/4] arm64: dts: rockchip: add #sound-dai-cells to the RK3588 HDMI receiver
From: Igor Paunovic @ 2026-07-18 8:57 UTC (permalink / raw)
To: Dmitry Osipenko, Mauro Carvalho Chehab
Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, linux-media, linux-rockchip, linux-arm-kernel,
kernel, devicetree, linux-kernel, Igor Paunovic
In-Reply-To: <20260718085728.6797-1-royalnet026@gmail.com>
The Synopsys HDMI RX controller exposes two digital audio interfaces,
one for I2S (0) and one for S/PDIF (1). Add the #sound-dai-cells
property so audio cards can reference them as a codec, as documented
by the snps,dw-hdmi-rx binding.
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
New in v3.
| 1 +
1 file changed, 1 insertion(+)
--git a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
index a264001..fa100e3 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
@@ -338,6 +338,7 @@
reset-names = "axi", "apb", "ref", "biu";
rockchip,grf = <&sys_grf>;
rockchip,vo1-grf = <&vo1_grf>;
+ #sound-dai-cells = <1>;
status = "disabled";
};
--
2.53.0
^ permalink raw reply related
* [PATCH v3 2/4] media: synopsys: hdmirx: add HDMI audio capture support
From: Igor Paunovic @ 2026-07-18 8:57 UTC (permalink / raw)
To: Dmitry Osipenko, Mauro Carvalho Chehab
Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, linux-media, linux-rockchip, linux-arm-kernel,
kernel, devicetree, linux-kernel, Igor Paunovic
In-Reply-To: <20260718085728.6797-1-royalnet026@gmail.com>
The Synopsys DesignWare HDMI RX controller extracts the audio stream
embedded in the incoming HDMI signal and feeds it to an on-SoC I2S
controller. Expose it as an ALSA capture device by registering the
generic hdmi-codec as a child of the controller, so that a
simple-audio-card in the device tree can bind the HDMI RX audio DAI.
The sample rate is recovered from the ACR N/CTS values together with the
measured TMDS character rate. A periodic worker keeps the local audio
reference clock locked to the source by nudging it in small ppm steps to
hold the audio FIFO fill level near its target, which avoids FIFO
under/overflow and the resulting dropped samples.
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
Changes in v3:
- restore the v1 audio teardown in remove(): audio_shutdown() already
stops the worker when the stream closes, so the extra flag clear
and trailing cancel added in v2 were redundant (Dmitry Osipenko)
- rename the ACR read locals and add a comment documenting the
register byte packing
- drop the get_dai_id stub so OF-graph cards resolve the DAI index
from the reg property
Changes in v2:
- register the S/PDIF DAI so the indexes match the binding and reject
it with -EOPNOTSUPP until wired up (Sebastian Reichel)
- use platform_device_register_data() and drop the fixed 32-bit DMA
mask (Dmitry Osipenko)
- don't leave an ERR_PTR in audio_pdev on registration failure
- fix teardown ordering in remove()
- stop the worker before reprogramming shared state in hw_params()
- look up the "audio" clock by name instead of indexing clks[1]
- keep the worker on system_unbound_wq when re-arming
.../platform/synopsys/hdmirx/snps_hdmirx.c | 271 ++++++++++++++++++
.../platform/synopsys/hdmirx/snps_hdmirx.h | 8 +
2 files changed, 279 insertions(+)
diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
index 9cceffa..8636944 100644
--- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
+++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
@@ -41,6 +41,8 @@
#include <media/videobuf2-dma-contig.h>
#include <media/videobuf2-v4l2.h>
+#include <sound/hdmi-codec.h>
+
#include "snps_hdmirx.h"
#include "snps_hdmirx_cec.h"
@@ -132,6 +134,13 @@ struct snps_hdmirx_dev {
struct delayed_work delayed_work_hotplug;
struct delayed_work delayed_work_res_change;
struct hdmirx_cec *cec;
+ struct platform_device *audio_pdev;
+ struct clk *audio_clk;
+ struct delayed_work audio_work;
+ u32 audio_clkrate;
+ u32 audio_fs;
+ int audio_pre_state;
+ bool audio_streaming;
struct mutex phy_rw_lock; /* to protect phy r/w configuration */
struct mutex stream_lock; /* to lock video stream capture */
struct mutex work_lock; /* to lock the critical section of hotplug event */
@@ -2283,6 +2292,13 @@ static int hdmirx_parse_dt(struct snps_hdmirx_dev *hdmirx_dev)
if (hdmirx_dev->num_clks < 1)
return -ENODEV;
+ for (int i = 0; i < hdmirx_dev->num_clks; i++) {
+ if (!strcmp(hdmirx_dev->clks[i].id, "audio")) {
+ hdmirx_dev->audio_clk = hdmirx_dev->clks[i].clk;
+ break;
+ }
+ }
+
hdmirx_dev->resets[HDMIRX_RST_A].id = "axi";
hdmirx_dev->resets[HDMIRX_RST_P].id = "apb";
hdmirx_dev->resets[HDMIRX_RST_REF].id = "ref";
@@ -2650,6 +2666,253 @@ static int hdmirx_register_cec(struct snps_hdmirx_dev *hdmirx_dev,
return 0;
}
+#define HDMIRX_AUDIO_INIT_FIFO_STATE 128
+#define HDMIRX_AUDIO_INIT_STATE (HDMIRX_AUDIO_INIT_FIFO_STATE * 4)
+
+static const int hdmirx_supported_fs[] = {
+ 32000, 44100, 48000, 88200, 96000, 176400, 192000, 768000, -1
+};
+
+static int hdmirx_audio_closest_fs(int fs)
+{
+ int i = 0, fs_t = hdmirx_supported_fs[0];
+
+ while (fs_t > 0) {
+ if (abs(fs - fs_t) <= 2000)
+ return fs_t;
+ fs_t = hdmirx_supported_fs[++i];
+ }
+ return 0;
+}
+
+/* Recover the incoming audio sample rate from the ACR N/CTS + TMDS clock. */
+static u32 hdmirx_audio_fs(struct snps_hdmirx_dev *hdmirx_dev)
+{
+ u64 tmds_clk, fs_audio = 0;
+ u32 acr_cts, acr_n, tmdsqpclk_freq;
+ u32 acr_pb3_0, acr_pb7_4;
+
+ tmdsqpclk_freq = hdmirx_readl(hdmirx_dev, CMU_TMDSQPCLK_FREQ);
+ hdmirx_readl(hdmirx_dev, PKTDEC_ACR_PH2_1);
+ acr_pb3_0 = hdmirx_readl(hdmirx_dev, PKTDEC_ACR_PB3_0);
+ acr_pb7_4 = hdmirx_readl(hdmirx_dev, PKTDEC_ACR_PB7_4);
+ /*
+ * The packet decoder stores the ACR subpacket bytes with packet byte
+ * 0 in register bits [7:0], so byte-swap each word to line the bytes
+ * up: CTS is packet bytes 1-3 (PKTDEC_ACR_PB3_0) and N is packet
+ * bytes 4-6 (PKTDEC_ACR_PB7_4), 20 bits each.
+ */
+ acr_cts = be32_to_cpu((__force __be32)acr_pb3_0) & 0xfffff;
+ acr_n = (be32_to_cpu((__force __be32)acr_pb7_4) & 0x0fffff00) >> 8;
+ tmds_clk = tmdsqpclk_freq * 4 * 1000U;
+ if (acr_cts != 0) {
+ fs_audio = div_u64((tmds_clk * acr_n), acr_cts);
+ fs_audio /= 128;
+ fs_audio = hdmirx_audio_closest_fs(fs_audio);
+ }
+ return (u32)fs_audio;
+}
+
+/* Nudge the audio reference clock by +/- ppm to keep the FIFO balanced. */
+static void hdmirx_audio_clk_ppm_inc(struct snps_hdmirx_dev *hdmirx_dev, int ppm)
+{
+ int delta, inc;
+ long rate = hdmirx_dev->audio_clkrate;
+
+ if (ppm < 0) {
+ ppm = -ppm;
+ inc = -1;
+ } else {
+ inc = 1;
+ }
+ delta = (int)div64_u64((u64)rate * ppm + 500000, 1000000);
+ delta *= inc;
+ rate = hdmirx_dev->audio_clkrate + delta;
+ clk_set_rate(hdmirx_dev->audio_clk, rate);
+ hdmirx_dev->audio_clkrate = rate;
+}
+
+static int hdmirx_audio_clk_adjust(struct snps_hdmirx_dev *hdmirx_dev,
+ int total_offset, int single_offset)
+{
+ int schedule_time = 500;
+ int ppm = 10;
+ u32 offset_abs = abs(total_offset);
+
+ if (offset_abs > 200) {
+ ppm += 200;
+ schedule_time -= 100;
+ }
+ if (offset_abs > 100) {
+ ppm += 200;
+ schedule_time -= 100;
+ }
+ if (offset_abs > 32) {
+ ppm += 20;
+ schedule_time -= 100;
+ }
+ if (offset_abs > 16)
+ ppm += 20;
+ if (total_offset > 16 && single_offset > 0)
+ hdmirx_audio_clk_ppm_inc(hdmirx_dev, ppm);
+ else if (total_offset < -16 && single_offset < 0)
+ hdmirx_audio_clk_ppm_inc(hdmirx_dev, -ppm);
+ return schedule_time;
+}
+
+static void hdmirx_audio_fifo_reinit(struct snps_hdmirx_dev *hdmirx_dev)
+{
+ hdmirx_writel(hdmirx_dev, AUDIO_FIFO_CONTROL, 1);
+ usleep_range(200, 210);
+ hdmirx_writel(hdmirx_dev, AUDIO_FIFO_CONTROL, 0);
+}
+
+/*
+ * Periodic worker that locks the local audio clock to the source by keeping
+ * the audio FIFO fill level close to its target, avoiding under/overflow.
+ */
+static void hdmirx_audio_work(struct work_struct *work)
+{
+ struct snps_hdmirx_dev *hdmirx_dev =
+ container_of(to_delayed_work(work), struct snps_hdmirx_dev, audio_work);
+ unsigned long delay = 200;
+ int cur, total, single;
+ u32 fifo, fs;
+
+ fs = hdmirx_audio_fs(hdmirx_dev);
+ fifo = hdmirx_readl(hdmirx_dev, AUDIO_FIFO_STATUS2);
+
+ if (fifo & (AFIFO_UNDERFLOW_ST | AFIFO_OVERFLOW_ST)) {
+ if (fs) {
+ clk_set_rate(hdmirx_dev->audio_clk, fs * 128);
+ hdmirx_dev->audio_clkrate = fs * 128;
+ hdmirx_dev->audio_fs = fs;
+ }
+ hdmirx_audio_fifo_reinit(hdmirx_dev);
+ hdmirx_dev->audio_pre_state = 0;
+ goto out;
+ }
+
+ cur = fifo & 0xffff;
+ total = cur - HDMIRX_AUDIO_INIT_STATE;
+ single = cur - hdmirx_dev->audio_pre_state;
+
+ if (fs && abs((int)fs - (int)hdmirx_dev->audio_fs) > 1000) {
+ clk_set_rate(hdmirx_dev->audio_clk, fs * 128);
+ hdmirx_dev->audio_clkrate = fs * 128;
+ hdmirx_dev->audio_fs = fs;
+ hdmirx_audio_fifo_reinit(hdmirx_dev);
+ hdmirx_dev->audio_pre_state = 0;
+ goto out;
+ }
+
+ if (cur != 0)
+ delay = hdmirx_audio_clk_adjust(hdmirx_dev, total, single);
+ hdmirx_dev->audio_pre_state = cur;
+out:
+ /* Only re-arm while streaming; avoids a self-reschedule race with
+ * the cancel_delayed_work_sync() callers (hw_params and
+ * audio_shutdown).
+ */
+ if (READ_ONCE(hdmirx_dev->audio_streaming))
+ queue_delayed_work(system_unbound_wq, &hdmirx_dev->audio_work,
+ msecs_to_jiffies(delay));
+}
+
+static int hdmirx_audio_hw_params(struct device *dev, void *data,
+ struct hdmi_codec_daifmt *fmt,
+ struct hdmi_codec_params *hparms)
+{
+ struct snps_hdmirx_dev *hdmirx_dev = dev_get_drvdata(dev);
+ u32 fs;
+
+ /* Only the I2S interface (DAI 0) is wired up so far. */
+ if (fmt->fmt == HDMI_SPDIF)
+ return -EOPNOTSUPP;
+
+ /*
+ * Stop the worker before touching the shared audio state; it is
+ * re-armed below once the new parameters are in place.
+ */
+ WRITE_ONCE(hdmirx_dev->audio_streaming, false);
+ cancel_delayed_work_sync(&hdmirx_dev->audio_work);
+
+ fs = hdmirx_audio_fs(hdmirx_dev);
+ if (!fs)
+ fs = hparms ? hparms->sample_rate : 48000;
+ if (!fs)
+ fs = 48000;
+
+ hdmirx_dev->audio_fs = fs;
+ hdmirx_dev->audio_clkrate = fs * 128;
+ clk_set_rate(hdmirx_dev->audio_clk, fs * 128);
+
+ hdmirx_audio_fifo_reinit(hdmirx_dev);
+ hdmirx_writel(hdmirx_dev, AUDIO_FIFO_THR_PASS, HDMIRX_AUDIO_INIT_FIFO_STATE);
+ hdmirx_writel(hdmirx_dev, AUDIO_FIFO_THR,
+ AFIFO_THR_LOW_QST(0x20) | AFIFO_THR_HIGH_QST(0x160));
+ hdmirx_writel(hdmirx_dev, AUDIO_FIFO_MUTE_THR,
+ AFIFO_THR_MUTE_LOW_QST(0x8) | AFIFO_THR_MUTE_HIGH_QST(0x178));
+
+ hdmirx_update_bits(hdmirx_dev, AUDIO_PROC_CONFIG0, I2S_EN, I2S_EN);
+ hdmirx_update_bits(hdmirx_dev, GLOBAL_SWENABLE, AUDIO_ENABLE, AUDIO_ENABLE);
+
+ hdmirx_dev->audio_pre_state = 0;
+ WRITE_ONCE(hdmirx_dev->audio_streaming, true);
+ mod_delayed_work(system_unbound_wq, &hdmirx_dev->audio_work,
+ msecs_to_jiffies(200));
+
+ dev_dbg(dev, "audio hw_params: fs=%u\n", fs);
+ return 0;
+}
+
+static void hdmirx_audio_shutdown(struct device *dev, void *data)
+{
+ struct snps_hdmirx_dev *hdmirx_dev = dev_get_drvdata(dev);
+
+ WRITE_ONCE(hdmirx_dev->audio_streaming, false);
+ cancel_delayed_work_sync(&hdmirx_dev->audio_work);
+ hdmirx_update_bits(hdmirx_dev, GLOBAL_SWENABLE, AUDIO_ENABLE, 0);
+}
+
+static const struct hdmi_codec_ops hdmirx_audio_codec_ops = {
+ .hw_params = hdmirx_audio_hw_params,
+ .audio_shutdown = hdmirx_audio_shutdown,
+};
+
+static int hdmirx_register_audio_device(struct snps_hdmirx_dev *hdmirx_dev)
+{
+ struct hdmi_codec_pdata codec_data = {
+ .ops = &hdmirx_audio_codec_ops,
+ .i2s = 1,
+ .no_i2s_playback = 1,
+ .max_i2s_channels = 8,
+ /*
+ * The controller also has an S/PDIF audio interface (DAI 1 in
+ * the binding). Register it so DAI indexes match the binding,
+ * but reject its use in hw_params() until it is wired up.
+ */
+ .spdif = 1,
+ .no_spdif_playback = 1,
+ .data = hdmirx_dev,
+ };
+ struct platform_device *audio_pdev;
+
+ if (!hdmirx_dev->audio_clk)
+ return -ENODEV;
+
+ audio_pdev = platform_device_register_data(hdmirx_dev->dev,
+ HDMI_CODEC_DRV_NAME,
+ PLATFORM_DEVID_AUTO,
+ &codec_data, sizeof(codec_data));
+ if (IS_ERR(audio_pdev))
+ return PTR_ERR(audio_pdev);
+
+ hdmirx_dev->audio_pdev = audio_pdev;
+
+ return 0;
+}
+
static int hdmirx_probe(struct platform_device *pdev)
{
struct snps_hdmirx_dev *hdmirx_dev;
@@ -2701,6 +2964,7 @@ static int hdmirx_probe(struct platform_device *pdev)
hdmirx_delayed_work_hotplug);
INIT_DELAYED_WORK(&hdmirx_dev->delayed_work_res_change,
hdmirx_delayed_work_res_change);
+ INIT_DELAYED_WORK(&hdmirx_dev->audio_work, hdmirx_audio_work);
hdmirx_dev->cur_fmt_fourcc = V4L2_PIX_FMT_BGR24;
hdmirx_dev->timings = cea640x480;
@@ -2769,6 +3033,10 @@ static int hdmirx_probe(struct platform_device *pdev)
V4L2_DEBUGFS_IF_AVI, hdmirx_dev,
hdmirx_debugfs_if_read);
+ ret = hdmirx_register_audio_device(hdmirx_dev);
+ if (ret)
+ dev_warn(dev, "failed to register HDMI audio codec: %d\n", ret);
+
return 0;
err_unreg_video_dev:
@@ -2788,6 +3056,9 @@ static void hdmirx_remove(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct snps_hdmirx_dev *hdmirx_dev = dev_get_drvdata(dev);
+ if (hdmirx_dev->audio_pdev)
+ platform_device_unregister(hdmirx_dev->audio_pdev);
+
v4l2_debugfs_if_free(hdmirx_dev->infoframes);
debugfs_remove_recursive(hdmirx_dev->debugfs_dir);
diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
index 31b887e..a99f54f 100644
--- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
+++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
@@ -81,6 +81,7 @@
#define DATAPATH_ENABLE BIT(12)
#define PKTFIFO_ENABLE BIT(11)
#define AVPUNIT_ENABLE BIT(8)
+#define AUDIO_ENABLE BIT(9)
#define MAIN_ENABLE BIT(0)
#define GLOBAL_TIMER_REF_BASE 0x0028
#define CORE_CONFIG 0x0050
@@ -177,20 +178,27 @@
#define VPROC_FMT_OVR_VALUE(x) UPDATE(x, 6, 4)
#define VPROC_FMT_OVR_EN BIT(0)
+#define AUDIO_FIFO_CONFIG 0x0460
#define AFIFO_FILL_RESTART BIT(0)
+#define AUDIO_FIFO_CONTROL 0x0464
#define AFIFO_INIT_P BIT(0)
+#define AUDIO_FIFO_THR_PASS 0x0468
+#define AUDIO_FIFO_THR 0x046c
#define AFIFO_THR_LOW_QST_MASK GENMASK(25, 16)
#define AFIFO_THR_LOW_QST(x) UPDATE(x, 25, 16)
#define AFIFO_THR_HIGH_QST_MASK GENMASK(9, 0)
#define AFIFO_THR_HIGH_QST(x) UPDATE(x, 9, 0)
+#define AUDIO_FIFO_MUTE_THR 0x0470
#define AFIFO_THR_MUTE_LOW_QST_MASK GENMASK(25, 16)
#define AFIFO_THR_MUTE_LOW_QST(x) UPDATE(x, 25, 16)
#define AFIFO_THR_MUTE_HIGH_QST_MASK GENMASK(9, 0)
#define AFIFO_THR_MUTE_HIGH_QST(x) UPDATE(x, 9, 0)
+#define AUDIO_FIFO_STATUS2 0x0478
#define AFIFO_UNDERFLOW_ST BIT(25)
#define AFIFO_OVERFLOW_ST BIT(24)
+#define AUDIO_PROC_CONFIG0 0x0480
#define SPEAKER_ALLOC_OVR_EN BIT(16)
#define I2S_BPCUV_EN BIT(4)
#define SPDIF_EN BIT(2)
--
2.53.0
^ permalink raw reply related
* [PATCH v3 1/4] dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells
From: Igor Paunovic @ 2026-07-18 8:57 UTC (permalink / raw)
To: Dmitry Osipenko, Mauro Carvalho Chehab
Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, linux-media, linux-rockchip, linux-arm-kernel,
kernel, devicetree, linux-kernel, Igor Paunovic
In-Reply-To: <20260718085728.6797-1-royalnet026@gmail.com>
The HDMI RX controller can expose the audio embedded in the incoming
HDMI stream as an ALSA capture device. Document the #sound-dai-cells
property so that a sound card can reference the HDMI RX audio DAI.
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
Changes in v3:
- no changes
Changes in v2 (all suggested by Sebastian Reichel):
- reworded the commit message to describe the hardware
- property description now documents the DAI indexes (0 = I2S,
1 = S/PDIF)
- moved the consumer reference into the binding example
.../devicetree/bindings/media/snps,dw-hdmi-rx.yaml | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
index b7f6c87..fa6cd0d 100644
--- a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
+++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
@@ -78,6 +78,13 @@ properties:
The phandle of the syscon node for the Video Output GRF register
to enable EDID transfer through SDAIN and SCLIN.
+ "#sound-dai-cells":
+ const: 1
+ description:
+ The HDMI RX controller has two digital audio interfaces, one for
+ I2S and one for S/PDIF. The DAI cell selects the interface, 0 for
+ I2S and 1 for S/PDIF.
+
required:
- compatible
- reg
@@ -129,4 +136,6 @@ examples:
pinctrl-0 = <&hdmim1_rx_cec &hdmim1_rx_hpdin &hdmim1_rx_scl &hdmim1_rx_sda &hdmirx_5v_detection>;
pinctrl-names = "default";
hpd-gpios = <&gpio1 22 GPIO_ACTIVE_LOW>;
+ /* referenced by a sound card as <&hdmi_receiver 0> (0: I2S, 1: S/PDIF) */
+ #sound-dai-cells = <1>;
};
--
2.53.0
^ permalink raw reply related
* [PATCH v3 0/4] media: synopsys: hdmirx: add HDMI audio capture support
From: Igor Paunovic @ 2026-07-18 8:57 UTC (permalink / raw)
To: Dmitry Osipenko, Mauro Carvalho Chehab
Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, linux-media, linux-rockchip, linux-arm-kernel,
kernel, devicetree, linux-kernel, Igor Paunovic
This series adds audio capture support to the Synopsys DesignWare HDMI
RX controller used on the Rockchip RK3588: the controller's audio FIFO
is exposed through an ASoC hdmi-codec device, with a periodic worker
that keeps the local audio clock locked to the incoming stream by
tracking the FIFO fill level. Together with the two new dts patches
this yields a capture-only ALSA card fed by the HDMI input.
Validated on the Orange Pi 5 Plus against multiple HDMI sources:
capture follows the source sample rate (44.1/48 kHz switches
included), survives repeated stream start/stop cycles and SIGKILL of
the capturing process, and stays free of FIFO under/overruns via the
clock tracking.
Changes in v3:
- Restored the v1 audio teardown in remove(): the worker is only
armed while a capture stream is active and audio_shutdown() already
stops it, so the extra flag clear and trailing cancel added in v2
were redundant (Dmitry Osipenko's review; the v2 change came from
an incorrect automated-review finding)
- Renamed the ACR read locals and added a comment documenting the
register byte packing that makes the swap correct
- Dropped the get_dai_id stub so OF-graph cards resolve the DAI index
from the reg property naturally
- New patches 3 and 4: dts enablement, so the series is testable end
to end (requested by Dmitry). Patch 3 adds #sound-dai-cells to the
RK3588 hdmi_receiver node, patch 4 wires up the Orange Pi 5 Plus
hdmiin sound card. Both can go through the rockchip tree instead if
preferred.
Link to v2: https://lore.kernel.org/linux-media/20260715200834.8486-1-royalnet026@gmail.com/
Link to v1 (RFC): https://lore.kernel.org/linux-media/20260715051939.64652-1-royalnet026@gmail.com/
Igor Paunovic (4):
dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells
media: synopsys: hdmirx: add HDMI audio capture support
arm64: dts: rockchip: add #sound-dai-cells to the RK3588 HDMI receiver
arm64: dts: rockchip: enable HDMI RX audio capture on Orange Pi 5 Plus
.../bindings/media/snps,dw-hdmi-rx.yaml | 9 +
.../arm64/boot/dts/rockchip/rk3588-extra.dtsi | 1 +
.../dts/rockchip/rk3588-orangepi-5-plus.dts | 20 ++
.../platform/synopsys/hdmirx/snps_hdmirx.c | 271 ++++++++++++++++++
.../platform/synopsys/hdmirx/snps_hdmirx.h | 8 +
5 files changed, 309 insertions(+)
--
2.53.0
^ permalink raw reply
* Re: [PATCH 5/6] iommu: Add Broadcom BCM2712 IOMMU driver
From: Daniel Drake @ 2026-07-18 8:39 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Joerg Roedel (AMD), Will Deacon, Robin Murphy, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Florian Fainelli,
Broadcom internal kernel review list, iommu, linux-kernel,
devicetree, linux-rpi-kernel, linux-arm-kernel, nick.hollinghurst
In-Reply-To: <20260712221125.GD1835788@ziepe.ca>
Hi,
On 12/07/2026 23:11, Jason Gunthorpe wrote:
>> +static int bcm2712_iommu_map_pages(struct iommu_domain *domain,
>> + unsigned long iova, phys_addr_t paddr,
>> + size_t pgsize, size_t pgcount, int prot,
>> + gfp_t gfp, size_t *mapped)
>> +{
>> + struct bcm2712_iommu_domain *mydomain = to_bcm2712_domain(domain);
>> + struct pt_iommu *pt = &mydomain->pt.iommu;
>> +
>> + return pt->ops->map_range(pt, bcm2712_iova_to_offset(mydomain, iova),
>> + paddr, pgsize * pgcount, prot, gfp, mapped);
>> +}
>
> These ops should not be present, the iommpt provides the ops directly
> through a macro;
>
>> +static const struct iommu_ops bcm2712_iommu_ops = {
>> + .identity_domain = &bcm2712_identity_domain,
>> + .domain_alloc_paging = bcm2712_iommu_domain_alloc,
>> + .probe_device = bcm2712_iommu_probe_device,
>> + .device_group = generic_single_device_group,
>> + .of_xlate = bcm2712_iommu_of_xlate,
>> + .default_domain_ops = &(const struct iommu_domain_ops) {
>> + .attach_dev = bcm2712_iommu_attach_dev,
>> + .iotlb_sync = bcm2712_iommu_sync,
>> + .iotlb_sync_map = bcm2712_iommu_sync_map,
>> + .flush_iotlb_all = bcm2712_iommu_sync_all,
>> + .free = bcm2712_iommu_domain_free,
>> + .map_pages = bcm2712_iommu_map_pages,
>> + .unmap_pages = bcm2712_iommu_unmap_pages,
>> + .iova_to_phys = bcm2712_iova_to_phys,
>> + },
>
> Then use something like:
>
> IOMMU_PT_DOMAIN_OPS(bcm2712),
>
> To define all the page table related ops automatically.
I don't think I can use that because this setup uses an IOVA aperture at
base 0xA00000000, whereas generic_pt assumes it is managing a 0-indexed
virtual address space. So the driver has to intercept every incoming
IOVA and translate for the aperture, see how map_pages calls:
static inline unsigned long
bcm2712_iova_to_offset(struct bcm2712_iommu_domain *domain, unsigned
long iova)
{
return iova - domain->mmu->aperture_start;
}
That's why I also set is_iommupt=false. I will add a comment to make
this clear. Let me know if you see a better approach. I was wondering
about making iommupt understand apertures and translate accordingly, but
I imagine you would want to keep that kind of thing out of the generic
fast path?
Thanks for all the other feedback too - working on it!
Daniel
^ permalink raw reply
* Re: [PATCH v10 0/2] iio: health: add MAX86150 ECG and PPG biosensor driver
From: Siratul Islam @ 2026-07-18 8:36 UTC (permalink / raw)
To: Md Shofiqul Islam
Cc: jic23, devicetree, robh, krzk+dt, conor+dt, andriy.shevchenko,
u.kleine-koenig, joshua.crofts1, nuno.sa, Michael.Hennerich,
dlechner, linux, linux-iio, linux-kernel
In-Reply-To: <20260717201138.1078019-1-shofiqtest@gmail.com>
On Fri, 2026-07-17 at 23:11 +0300, Md Shofiqul Islam wrote:
> Sorry for the long gap since v9. Between then and now the driver's
> buffer implementation was rewritten from the kfifo-based design onto
> the standard IIO hardware-trigger and triggered-buffer framework, and
> I read through every open comment from every reviewer before sending
> this rather than sending again too quickly.
>
...
>
> Changes since v9:
> - Rewrote the buffer implementation around devm_iio_trigger_alloc() +
> devm_iio_triggered_buffer_setup() instead of the kfifo buffer and
> manual postenable/predisable pair. The hard-irq handler now only
> reads and clears INT_STATUS1 before calling iio_trigger_poll();
> the threaded trigger handler does the FIFO drain. This relies on
> the trigger core's own attach/detach synchronization instead of an
> explicit iio_buffer_enabled() guard or synchronize_irq(), which the
> old design needed to avoid a NULL active_scan_mask race on teardown
> (Sashiko, both HIGH severity findings)
>
...
>
> --
> 2.51.1
Not sure if it was said already, but please include change log from older versions too.
You could keep the description short, and include Links to each version as well. It is
difficult to hunt them down one by one from lore.
Also, don't send "To" a list. send your patches to Maintainers and
Cc Reviewers and lists.
--
Best regards,
Sirat
^ permalink raw reply
* Re: [PATCH] dt-bindings: leds: lacie,ns2-leds: Convert to DT schema
From: Ninad Naik @ 2026-07-18 8:15 UTC (permalink / raw)
To: Rob Herring, sashiko-reviews; +Cc: linux-leds, devicetree, lee, conor+dt
In-Reply-To: <20260716173939.GB534222-robh@kernel.org>
On 16/07/26 11:09 pm, Rob Herring wrote:
> On Wed, Jul 01, 2026 at 06:05:52PM +0000,sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> - [Low] The schema conversion incorrectly restricts LED child node names to `^led(-[0-9a-z]+)?$`, which breaks `dtbs_check` validation for all existing upstream device trees using this binding.
>> --
>>
>> commit 6c59295f22b17d02a2b26847db54458f820fa4f3
>> Author: Ninad Naik<ninadnaik07@gmail.com>
>>
>> dt-bindings: leds: lacie,ns2-leds: Convert to DT schema
>>
>> Convert lacie,ns2-leds to DT schema.
>>
>>> diff --git a/Documentation/devicetree/bindings/leds/lacie,ns2-leds.yaml b/Documentation/devicetree/bindings/leds/lacie,ns2-leds.yaml
>>> new file mode 100644
>>> index 0000000000000..679a618906d80
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/lacie,ns2-leds.yaml
>>> @@ -0,0 +1,87 @@
>> [ ... ]
>>> +properties:
>>> + compatible:
>>> + const: lacie,ns2-leds
>>> +
>>> +patternProperties:
>>> + "^led(-[0-9a-z]+)?$":
>> [Severity: Low]
>> Will this restrictive regex cause existing device trees to fail dtbs_check?
>>
>> The old text binding example used a child node named blue-sata, which does
>> not match this pattern. Since additionalProperties is set to false below,
>> nodes in existing device trees that don't start with led will be rejected
>> during validation.
>>
>> Should this regex be relaxed to allow existing node names, or should those
>> device tree files be updated alongside this conversion?
> While yes we prefer what you did, it's perhaps not worth changing these
> old .dts files to match the preference.
>
> Rob
I understand this. Should I relax the regex to allow any pattern so that
the existing as well
as new DTS files could be properly validated?
Thanks,
Ninad Naik
^ permalink raw reply
* Re: [PATCH v7 01/13] dt-bindings: phy: Add zx297520v3 USB phy documentation
From: Stefan Dösinger @ 2026-07-18 7:50 UTC (permalink / raw)
To: Vinod Koul
Cc: Krzysztof Kozlowski, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Brian Masney,
Neil Armstrong, Russell King, linux-clk, devicetree, linux-kernel,
linux-arm-kernel, linux-phy
In-Reply-To: <20260717-uncovered-feathered-groundhog-eab16e@quoll>
[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]
Am Freitag, 17. Juli 2026, 09:32:18 Ostafrikanische Zeit schrieb Krzysztof
Kozlowski:
> On Fri, Jul 17, 2026 at 12:35:37AM +0300, Stefan Dösinger wrote:
> > + resets:
> > + items:
> > + - description: USB phy reset
> > + - description: HSIC phy reset
>
> Why do you reference here resets from the parent? That's a clear signal
> you have one device, so one device node regardless of Linux phy core
> behavior.
This is a follow-up question for the PHY maintainers, if they notice the E-
Mail thread: I'm trying to resolve an apparent conflict between DT
expectations and a check in the PHY core:
device_set_of_node_from_dev(dev, dev->parent);
...
devm_of_phy_provider_register(dev, xlate);
Passes the letter of the check in __of_phy_provider_register(), but I am not
sure about the spirit.
"dev" here is a MFD child node without a DT node in accordance to DT's
writing-bindings instructions ("DON’T create nodes just for the sake of
instantiating drivers"). The PHY core prohibits a child to register a phy
provider to its parent node. Why?
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 870 bytes --]
^ permalink raw reply
* Re: [RFC PATCH v2 1/8] dt-bindings: npu: rockchip: add rockchip,rk3576-rknn-core
From: Krzysztof Kozlowski @ 2026-07-18 7:49 UTC (permalink / raw)
To: Jiaxing Hu, tomeu, heiko, robh, krzk+dt, conor+dt, joro, will,
robin.murphy, ulfh, p.zabel, ogabbay
Cc: chaoyi.chen, dri-devel, linux-rockchip, iommu, linux-pm,
devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20260718031146.3368811-2-gahing@gahingwoo.com>
On 18/07/2026 05:11, Jiaxing Hu wrote:
> Document the RK3576 NPU core: add rockchip,rk3576-rknn-core to the
> compatible enum. The RK3576 has no NPU SRAM supply, so make sram-supply
> required only for rockchip,rk3588-rknn-core
Full stop here.
via an allOf/if-then instead
> of dropping it from the shared required list (which would have weakened
> validation for RK3588).
This is redundant.
...
> - npu-supply
> - - sram-supply
> +
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: rockchip,rk3588-rknn-core
> + then:
> + required:
> + - sram-supply
>
else:
properties:
sram-supply: false
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: arm: rockchip: Add Vicharak Vaaman board
From: Krzysztof Kozlowski @ 2026-07-18 7:47 UTC (permalink / raw)
To: Hrushiraj Gandhi, heiko
Cc: devicetree, linux-arm-kernel, linux-rockchip, linux-kernel, robh,
krzk+dt, conor+dt
In-Reply-To: <20260718065900.755631-2-hrushirajg23@gmail.com>
On 18/07/2026 08:58, Hrushiraj Gandhi wrote:
> Document the compatible string for the Vicharak Vaaman, an RK3399-based
> single-board computer.
>
> Signed-off-by: Hrushiraj Gandhi <hrushirajg23@gmail.com>
> ---
> Documentation/devicetree/bindings/arm/rockchip.yaml | 5 +++++
> 1 file changed, 5 insertions(+)
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: arm: rockchip: Add Vicharak Vaaman board
From: sashiko-bot @ 2026-07-18 7:02 UTC (permalink / raw)
To: Hrushiraj Gandhi; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260718065900.755631-2-hrushirajg23@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The new vendor prefix 'vicharak' is not documented in vendor-prefixes.yaml.
--
--- Patch [1]: [PATCH v2 1/2] dt-bindings: arm: rockchip: Add Vicharak Vaaman board ---
commit 83c52c932b9201d9be9114ede15a03da032999a5
Author: Hrushiraj Gandhi <hrushirajg23@gmail.com>
dt-bindings: arm: rockchip: Add Vicharak Vaaman board
This commit documents the compatible string for the Vicharak Vaaman, an
RK3399-based single-board computer, in the rockchip bindings.
> diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml b/Documentation/devicetree/bindings/arm/rockchip.yaml
> --- a/Documentation/devicetree/bindings/arm/rockchip.yaml
> +++ b/Documentation/devicetree/bindings/arm/rockchip.yaml
[ ... ]
> @@ -1306,6 +1306,11 @@ properties:
> - const: turing,rk1
> - const: rockchip,rk3588
>
> + - description: Vicharak Vaaman
> + items:
> + - const: vicharak,vaaman
> + - const: rockchip,rk3399
[Severity: Low]
Does the new vendor prefix vicharak need to be documented in
vendor-prefixes.yaml?
When introducing a new vendor compatible string, the prefix must be
registered in Documentation/devicetree/bindings/vendor-prefixes.yaml.
Without this entry, schema validation tooling will report an error
when make dt_binding_check is run.
> +
> - description: WolfVision PF5 mainboard
> items:
> - const: wolfvision,rk3568-pf5
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260718065900.755631-1-hrushirajg23@gmail.com?part=1
^ permalink raw reply
* [PATCH v2 2/2] arm64: dts: rockchip: Add Vicharak Vaaman board
From: Hrushiraj Gandhi @ 2026-07-18 6:59 UTC (permalink / raw)
To: heiko
Cc: devicetree, linux-arm-kernel, linux-rockchip, linux-kernel, robh,
krzk+dt, conor+dt, Hrushiraj Gandhi
In-Reply-To: <20260718065900.755631-1-hrushirajg23@gmail.com>
Add initial devicetree support for the Vicharak Vaaman, an RK3399-based
single-board computer.
Supported peripherals:
- RK808 PMIC with core/logic/IO regulators
- SYR827/SYR828 (vdd_cpu_b/vdd_gpu) CPU-big and GPU regulators
- Mali GPU
- Gigabit Ethernet (RGMII, via &gmac)
- eMMC (HS400, enhanced strobe)
- microSD card slot
- SARADC and TSADC
- PWM-based vdd_log regulator
- UART2 serial console
Signed-off-by: Hrushiraj Gandhi <hrushirajg23@gmail.com>
---
Changes in v2:
- Fix fixed-regulator node naming to use 'regulator-[voltage]' prefix
consistently throughout
---
arch/arm64/boot/dts/rockchip/Makefile | 1 +
.../dts/rockchip/rk3399-vicharak-vaaman.dts | 438 ++++++++++++++++++
2 files changed, 439 insertions(+)
create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-vicharak-vaaman.dts
diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
index 761d82b4f4f2..e1b974502915 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -90,6 +90,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rockpro64.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rockpro64-screen.dtbo
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-sapphire.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-sapphire-excavator.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-vicharak-vaaman.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399pro-rock-pi-n10.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3528-armsom-sige1.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3528-nanopi-zero2.dtb
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-vicharak-vaaman.dts b/arch/arm64/boot/dts/rockchip/rk3399-vicharak-vaaman.dts
new file mode 100644
index 000000000000..dbf79b996500
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-vicharak-vaaman.dts
@@ -0,0 +1,438 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2026 Vicharak Computers Pvt Ltd
+ */
+
+/dts-v1/;
+#include "rk3399.dtsi"
+
+/ {
+ model = "Vicharak Vaaman";
+ compatible = "vicharak,vaaman", "rockchip,rk3399";
+
+ aliases {
+ ethernet0 = &gmac;
+ mmc0 = &sdhci;
+ mmc1 = &sdmmc;
+ };
+
+ chosen {
+ stdout-path = "serial2:1500000n8";
+ };
+
+ clkin_gmac: external-gmac-clock {
+ compatible = "fixed-clock";
+ clock-frequency = <125000000>;
+ clock-output-names = "clkin_gmac";
+ #clock-cells = <0>;
+ };
+
+ vcc12v_dcin: regulator-dc-12v {
+ compatible = "regulator-fixed";
+ regulator-name = "vcc12v_dcin";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <12000000>;
+ regulator-max-microvolt = <12000000>;
+ };
+
+ vcc3v3_lan: regulator-vcc3v3-lan {
+ compatible = "regulator-fixed";
+ regulator-name = "vcc3v3_lan";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ vin-supply = <&vcc3v3_sys>;
+ };
+
+ vcc3v3_sys: regulator-vcc3v3-sys {
+ compatible = "regulator-fixed";
+ regulator-name = "vcc3v3_sys";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ vin-supply = <&vcc5v0_sys>;
+ };
+
+ vcc5v0_sys: regulator-vcc-sys {
+ compatible = "regulator-fixed";
+ regulator-name = "vcc5v0_sys";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ vin-supply = <&vcc12v_dcin>;
+ };
+
+ vdd_log: regulator-vdd-log {
+ compatible = "pwm-regulator";
+ pwms = <&pwm2 0 25000 1>;
+ pwm-supply = <&vcc5v0_sys>;
+ regulator-name = "vdd_log";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <800000>;
+ regulator-max-microvolt = <1400000>;
+ };
+};
+
+&cpu_l0 {
+ cpu-supply = <&vdd_cpu_l>;
+};
+
+&cpu_l1 {
+ cpu-supply = <&vdd_cpu_l>;
+};
+
+&cpu_l2 {
+ cpu-supply = <&vdd_cpu_l>;
+};
+
+&cpu_l3 {
+ cpu-supply = <&vdd_cpu_l>;
+};
+
+&cpu_b0 {
+ cpu-supply = <&vdd_cpu_b>;
+};
+
+&cpu_b1 {
+ cpu-supply = <&vdd_cpu_b>;
+};
+
+&emmc_phy {
+ rockchip,enable-strobe-pulldown;
+ status = "okay";
+};
+
+&gmac {
+ assigned-clocks = <&cru SCLK_RMII_SRC>;
+ assigned-clock-parents = <&clkin_gmac>;
+ clock_in_out = "input";
+ phy-supply = <&vcc3v3_lan>;
+ /* TX/RX delays tuned for the Vaaman board PCB trace lengths */
+ phy-mode = "rgmii";
+ pinctrl-names = "default";
+ pinctrl-0 = <&rgmii_pins>;
+ snps,reset-gpio = <&gpio3 RK_PB7 GPIO_ACTIVE_LOW>;
+ snps,reset-active-low;
+ snps,reset-delays-us = <0 10000 50000>;
+ tx_delay = <0x28>;
+ rx_delay = <0x11>;
+ status = "okay";
+};
+
+&gpu {
+ mali-supply = <&vdd_gpu>;
+ status = "okay";
+};
+
+&i2c0 {
+ clock-frequency = <400000>;
+ i2c-scl-rising-time-ns = <168>;
+ i2c-scl-falling-time-ns = <4>;
+ status = "okay";
+
+ rk808: pmic@1b {
+ compatible = "rockchip,rk808";
+ reg = <0x1b>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <RK_PC5 IRQ_TYPE_LEVEL_LOW>;
+ #clock-cells = <1>;
+ clock-output-names = "xin32k", "rk808-clkout2";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pmic_int_l>;
+ system-power-controller;
+ wakeup-source;
+
+ vcc1-supply = <&vcc5v0_sys>;
+ vcc2-supply = <&vcc5v0_sys>;
+ vcc3-supply = <&vcc5v0_sys>;
+ vcc4-supply = <&vcc5v0_sys>;
+ vcc6-supply = <&vcc5v0_sys>;
+ vcc7-supply = <&vcc5v0_sys>;
+ vcc8-supply = <&vcc3v3_sys>;
+ vcc9-supply = <&vcc5v0_sys>;
+ vcc10-supply = <&vcc5v0_sys>;
+ vcc11-supply = <&vcc5v0_sys>;
+ vcc12-supply = <&vcc3v3_sys>;
+ vddio-supply = <&vcc_3v0>;
+
+ regulators {
+ vdd_center: DCDC_REG1 {
+ regulator-name = "vdd_center";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <750000>;
+ regulator-max-microvolt = <1350000>;
+ regulator-ramp-delay = <6001>;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ regulator-suspend-microvolt = <900000>;
+ };
+ };
+
+ vdd_cpu_l: DCDC_REG2 {
+ regulator-name = "vdd_cpu_l";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-initial-mode = <0x2>;
+ regulator-min-microvolt = <750000>;
+ regulator-max-microvolt = <1350000>;
+ regulator-ramp-delay = <6001>;
+
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
+ };
+
+ vcc_ddr: DCDC_REG3 {
+ regulator-name = "vcc_ddr";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-state-mem {
+ regulator-on-in-suspend;
+ };
+ };
+
+ vcc_1v8: DCDC_REG4 {
+ regulator-name = "vcc_1v8";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-state-mem {
+ regulator-on-in-suspend;
+ regulator-suspend-microvolt = <1800000>;
+ };
+ };
+
+ vcca1v8_codec: LDO_REG1 {
+ regulator-name = "vcca1v8_codec";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+
+ regulator-state-mem {
+ regulator-on-in-suspend;
+ regulator-suspend-microvolt = <1800000>;
+ };
+ };
+
+ vcca1v8_hdmi: LDO_REG2 {
+ regulator-name = "vcca1v8_hdmi";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
+ };
+
+ vcca_1v8: LDO_REG3 {
+ regulator-name = "vcca_1v8";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-state-mem {
+ regulator-on-in-suspend;
+ regulator-suspend-microvolt = <1800000>;
+ };
+ };
+
+ vcc_sdio: LDO_REG4 {
+ regulator-name = "vcc_sdio";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <3000000>;
+ regulator-max-microvolt = <3000000>;
+ regulator-state-mem {
+ regulator-on-in-suspend;
+ regulator-suspend-microvolt = <3000000>;
+ };
+ };
+
+ vcca3v0_codec: LDO_REG5 {
+ regulator-name = "vcca3v0_codec";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <3000000>;
+ regulator-max-microvolt = <3000000>;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
+ };
+
+ vcc_1v5: LDO_REG6 {
+ regulator-name = "vcc_1v5";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <1500000>;
+ regulator-max-microvolt = <1500000>;
+ regulator-state-mem {
+ regulator-on-in-suspend;
+ regulator-suspend-microvolt = <1500000>;
+ };
+ };
+
+ vcca0v9_hdmi: LDO_REG7 {
+ regulator-name = "vcca0v9_hdmi";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <900000>;
+ regulator-max-microvolt = <900000>;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ regulator-suspend-microvolt = <900000>;
+ };
+ };
+
+ vcc_3v0: LDO_REG8 {
+ regulator-name = "vcc_3v0";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <3000000>;
+ regulator-max-microvolt = <3000000>;
+ regulator-state-mem {
+ regulator-on-in-suspend;
+ regulator-suspend-microvolt = <3000000>;
+ };
+ };
+
+ vcc_cam: SWITCH_REG1 {
+ regulator-name = "vcc_cam";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
+ };
+
+ vcc_mipi: SWITCH_REG2 {
+ regulator-name = "vcc_mipi";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
+ };
+ };
+ };
+
+ vdd_cpu_b: regulator@40 {
+ compatible = "silergy,syr827";
+ reg = <0x40>;
+ fcs,suspend-voltage-selector = <1>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&vsel1_pin>;
+ regulator-name = "vdd_cpu_b";
+ regulator-min-microvolt = <712500>;
+ regulator-max-microvolt = <1500000>;
+ regulator-ramp-delay = <1000>;
+ regulator-always-on;
+ regulator-boot-on;
+ vin-supply = <&vcc5v0_sys>;
+
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
+ };
+
+ vdd_gpu: regulator@41 {
+ compatible = "silergy,syr828";
+ reg = <0x41>;
+ fcs,suspend-voltage-selector = <1>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&vsel2_pin>;
+ regulator-name = "vdd_gpu";
+ regulator-min-microvolt = <712500>;
+ regulator-max-microvolt = <1500000>;
+ regulator-ramp-delay = <1000>;
+ regulator-always-on;
+ regulator-boot-on;
+ vin-supply = <&vcc5v0_sys>;
+
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
+ };
+};
+
+&io_domains {
+ audio-supply = <&vcca1v8_codec>;
+ bt656-supply = <&vcc_3v0>;
+ gpio1830-supply = <&vcc_3v0>;
+ sdmmc-supply = <&vcc_sdio>;
+ status = "okay";
+};
+
+&pinctrl {
+ pmic {
+ pmic_int_l: pmic-int-l {
+ rockchip,pins =
+ <1 RK_PC5 RK_FUNC_GPIO &pcfg_pull_up>;
+ };
+
+ vsel1_pin: vsel1-pin {
+ rockchip,pins =
+ <1 RK_PC1 RK_FUNC_GPIO &pcfg_pull_down>;
+ };
+
+ vsel2_pin: vsel2-pin {
+ rockchip,pins =
+ <1 RK_PB6 RK_FUNC_GPIO &pcfg_pull_down>;
+ };
+ };
+};
+
+&pmu_io_domains {
+ pmu1830-supply = <&vcc_3v0>;
+ status = "okay";
+};
+
+&pwm2 {
+ status = "okay";
+};
+
+&saradc {
+ vref-supply = <&vcc_1v8>;
+ status = "okay";
+};
+
+&sdhci {
+ max-frequency = <150000000>;
+ bus-width = <8>;
+ mmc-hs400-1_8v;
+ mmc-hs400-enhanced-strobe;
+ non-removable;
+ status = "okay";
+};
+
+&sdmmc {
+ bus-width = <4>;
+ cap-mmc-highspeed;
+ cap-sd-highspeed;
+ cd-gpios = <&gpio0 RK_PA7 GPIO_ACTIVE_LOW>;
+ disable-wp;
+ max-frequency = <150000000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&sdmmc_clk &sdmmc_cd &sdmmc_cmd &sdmmc_bus4>;
+ status = "okay";
+};
+
+&tsadc {
+ /* tshut mode 0:CRU 1:GPIO */
+ rockchip,hw-tshut-mode = <1>;
+ /* tshut polarity 0:LOW 1:HIGH */
+ rockchip,hw-tshut-polarity = <1>;
+ status = "okay";
+};
+
+&uart2 {
+ status = "okay";
+};
--
2.47.3
^ permalink raw reply related
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