* [PATCH v3 0/4] AUXADC driver for the MediaTek mt6323 PMIC
From: Roman Vivchar via B4 Relay @ 2026-06-16 14:15 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Lee Jones
Cc: linux-iio, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, Ben Grisdale, Roman Vivchar, Andy Shevchenko
This series adds support for the 15-bit AUXADC hardware block found on
the MediaTek mt6323 PMIC.
The previous version of the series for all AUXADC, EFUSE and thermal
drivers was split after Krzysztof's comment [1].
Tested on the MediaTek mt6572 and mt8163 SoCs (Ben), both paired with a
mt6323.
The other parts (EFUSE and thermal) will probably be sent this week.
[1]: https://lore.kernel.org/linux-mediatek/20260504-mt6323-v1-0-799b58b355ff@protonmail.com/T/#med30fad67a090be35f549231336b2dec295233f6
Tested-by: Ben Grisdale <bengris32@protonmail.ch> # Amazon Echo Dot (2nd Generation)
Signed-off-by: Roman Vivchar <rva333@protonmail.com>
---
Changes in v3:
- AUXADC driver:
- Add comment for channels table about voltage and channel IDs (Jonathan)
- Add comment for mutex in the 'mt6323_auxadc' struct (Jonathan)
- Break 'regmap_read_poll_timeout' on logical boundaries (Andy)
- Switch to 'guard' from 'scoped_guard' (Andy)
- Link to v2: https://patch.msgid.link/20260609-mt6323-adc-v2-0-aa93a22309f9@protonmail.com
Changes in v2:
- AUXADC driver:
- Drop channel type from the MTK_PMIC_IIO_CHAN macro (Nuno)
- Drop kerneldoc for the mt6323_auxadc struct (Nuno)
- Add channel release to save power (Sashiko, Jonathan)
- Drop 'reg' variable in the mt6323_auxadc_read (Jonathan)
- Sort variables in the mt6323_auxadc_probe (Jonathan)
- Maintainers:
- Drop linux-mediatek list (Andy)
- Split between dt-bindings and driver to avoid missing file (Nuno)
- Link to v1: https://patch.msgid.link/20260602-mt6323-adc-v1-0-68ec737508ee@protonmail.com
Changes after split:
- dt-bindings: Change 'MT63xx' to 'MT6350 series and similar' (Jonathan)
- AUXADC driver:
- Add missing headers (Andy)
- Fix AUXADC_TRIM_CH* values (Andy)
- Rename masks to include their register name (Jonathan)
- Fix formatting (Andy, Jonathan)
- Replace channel address with actual register value (Jonathan), align the table
- Replace IIO_TEMP with IIO_VOLTAGE, since the actual output is still mV, not mC
- Rename constants to match their registers (Jonathan)
- Remove 'if/else if/else' in the mt6323_auxadc_read_raw (Andy)
- Add comments for fsleep, ADC range and resolution (Andy, Jonathan)
- Remove useless error messages (Andy)
- Maintainers:
- Explicitly include mt6323 in the name (Jonathan)
- Squash with AUXADC driver commit (Krzysztof)
- Set status back to 'Maintained'
- Link to a previous series: https://patch.msgid.link/20260512-mt6323-v2-0-3efcba579e88@protonmail.com
---
Roman Vivchar (4):
dt-bindings: iio: adc: mediatek,mt6359-auxadc: add mt6323 PMIC AUXADC
iio: adc: mt6323-auxadc: add mt6323 PMIC AUXADC driver
mfd: mt6397-core: add mt6323 AUXADC support
ARM: dts: mediatek: mt6323: add AUXADC support
.../bindings/iio/adc/mediatek,mt6359-auxadc.yaml | 3 +-
MAINTAINERS | 7 +
arch/arm/boot/dts/mediatek/mt6323.dtsi | 5 +
drivers/iio/adc/Kconfig | 11 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/mt6323-auxadc.c | 314 +++++++++++++++++++++
drivers/mfd/mt6397-core.c | 3 +
.../dt-bindings/iio/adc/mediatek,mt6323-auxadc.h | 24 ++
8 files changed, 367 insertions(+), 1 deletion(-)
---
base-commit: 028ef9c96e96197026887c0f092424679298aae8
change-id: 20260525-mt6323-adc-3befce36cbf2
Best regards,
--
Roman Vivchar <rva333@protonmail.com>
^ permalink raw reply
* [PATCH v3 1/4] dt-bindings: iio: adc: mediatek,mt6359-auxadc: add mt6323 PMIC AUXADC
From: Roman Vivchar via B4 Relay @ 2026-06-16 14:15 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Lee Jones
Cc: linux-iio, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, Ben Grisdale, Roman Vivchar
In-Reply-To: <20260616-mt6323-adc-v3-0-1c27c588185d@protonmail.com>
From: Roman Vivchar <rva333@protonmail.com>
The MediaTek mt6323 PMIC includes an AUXADC used for battery voltage,
temperature, and other internal measurements. The IP block is not
register-compatible with mt6359 and should use a separate driver.
Add the devicetree binding documentation and the associated header file
defining the ADC channel constants.
Also change the description to 'MT6350 series and similar' because
the binding already includes more than mt635x series PMICs.
Finally, add the MAINTAINERS entry for the header with ADC constants.
Signed-off-by: Roman Vivchar <rva333@protonmail.com>
---
.../bindings/iio/adc/mediatek,mt6359-auxadc.yaml | 3 ++-
MAINTAINERS | 6 ++++++
.../dt-bindings/iio/adc/mediatek,mt6323-auxadc.h | 24 ++++++++++++++++++++++
3 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
index 5d4ab701f51a..852eb7336a5a 100644
--- a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
@@ -4,7 +4,7 @@
$id: http://devicetree.org/schemas/iio/adc/mediatek,mt6359-auxadc.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
-title: MediaTek MT6350 series PMIC AUXADC
+title: MediaTek MT6350 series and similar PMIC AUXADC
maintainers:
- AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
@@ -19,6 +19,7 @@ description:
properties:
compatible:
enum:
+ - mediatek,mt6323-auxadc
- mediatek,mt6357-auxadc
- mediatek,mt6358-auxadc
- mediatek,mt6359-auxadc
diff --git a/MAINTAINERS b/MAINTAINERS
index d1cc0e12fe1f..2551c8cd9e9d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16256,6 +16256,12 @@ S: Maintained
F: Documentation/devicetree/bindings/mmc/mtk-sd.yaml
F: drivers/mmc/host/mtk-sd.c
+MEDIATEK MT6323 PMIC AUXADC DRIVER
+M: Roman Vivchar <rva333@protonmail.com>
+L: linux-iio@vger.kernel.org
+S: Maintained
+F: include/dt-bindings/iio/adc/mediatek,mt6323-auxadc.h
+
MEDIATEK MT6735 CLOCK & RESET DRIVERS
M: Yassine Oudjana <y.oudjana@protonmail.com>
L: linux-clk@vger.kernel.org
diff --git a/include/dt-bindings/iio/adc/mediatek,mt6323-auxadc.h b/include/dt-bindings/iio/adc/mediatek,mt6323-auxadc.h
new file mode 100644
index 000000000000..6ee9a9ecffc1
--- /dev/null
+++ b/include/dt-bindings/iio/adc/mediatek,mt6323-auxadc.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+
+#ifndef _DT_BINDINGS_MEDIATEK_MT6323_AUXADC_H
+#define _DT_BINDINGS_MEDIATEK_MT6323_AUXADC_H
+
+#define MT6323_AUXADC_BATON2 0
+#define MT6323_AUXADC_CH6 1
+#define MT6323_AUXADC_BAT_TEMP 2
+#define MT6323_AUXADC_CHIP_TEMP 3
+#define MT6323_AUXADC_VCDT 4
+#define MT6323_AUXADC_BATON1 5
+#define MT6323_AUXADC_ISENSE 6
+#define MT6323_AUXADC_BATSNS 7
+#define MT6323_AUXADC_ACCDET 8
+#define MT6323_AUXADC_AUDIO0 9
+#define MT6323_AUXADC_AUDIO1 10
+#define MT6323_AUXADC_AUDIO2 11
+#define MT6323_AUXADC_AUDIO3 12
+#define MT6323_AUXADC_AUDIO4 13
+#define MT6323_AUXADC_AUDIO5 14
+#define MT6323_AUXADC_AUDIO6 15
+#define MT6323_AUXADC_AUDIO7 16
+
+#endif
--
2.54.0
^ permalink raw reply related
* [PATCH v3 2/4] iio: adc: mt6323-auxadc: add mt6323 PMIC AUXADC driver
From: Roman Vivchar via B4 Relay @ 2026-06-16 14:15 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Lee Jones
Cc: linux-iio, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, Ben Grisdale, Roman Vivchar, Andy Shevchenko
In-Reply-To: <20260616-mt6323-adc-v3-0-1c27c588185d@protonmail.com>
From: Roman Vivchar <rva333@protonmail.com>
The mt6323 AUXADC is a 15-bit ADC used for system monitoring. This driver
provides support for reading various channels including battery and
charger voltages, battery and chip temperature, current sensing and
accessory detection.
Add a driver for the AUXADC found in the MediaTek mt6323 PMIC.
Tested-by: Ben Grisdale <bengris32@protonmail.ch> # Amazon Echo Dot (2nd Generation)
Signed-off-by: Roman Vivchar <rva333@protonmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
---
MAINTAINERS | 1 +
drivers/iio/adc/Kconfig | 11 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/mt6323-auxadc.c | 314 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 327 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 2551c8cd9e9d..fb40128451dd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16260,6 +16260,7 @@ MEDIATEK MT6323 PMIC AUXADC DRIVER
M: Roman Vivchar <rva333@protonmail.com>
L: linux-iio@vger.kernel.org
S: Maintained
+F: drivers/iio/adc/mt6323-auxadc.c
F: include/dt-bindings/iio/adc/mediatek,mt6323-auxadc.h
MEDIATEK MT6735 CLOCK & RESET DRIVERS
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 60038ae8dfc4..a03614b46041 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1137,6 +1137,17 @@ config MCP3911
This driver can also be built as a module. If so, the module will be
called mcp3911.
+config MEDIATEK_MT6323_AUXADC
+ tristate "MediaTek MT6323 PMIC AUXADC driver"
+ depends on MFD_MT6397
+ help
+ Say yes here to enable support for MediaTek MT6323 PMIC Auxiliary ADC.
+ This driver provides multiple channels for system monitoring,
+ such as battery voltage, PMIC temperature, and others.
+
+ This driver can also be built as a module. If so, the module will be
+ called mt6323-auxadc.
+
config MEDIATEK_MT6359_AUXADC
tristate "MediaTek MT6359 PMIC AUXADC driver"
depends on MFD_MT6397
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index c76550415ff1..58161750d6e3 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -99,6 +99,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o
obj-$(CONFIG_MCP3422) += mcp3422.o
obj-$(CONFIG_MCP3564) += mcp3564.o
obj-$(CONFIG_MCP3911) += mcp3911.o
+obj-$(CONFIG_MEDIATEK_MT6323_AUXADC) += mt6323-auxadc.o
obj-$(CONFIG_MEDIATEK_MT6359_AUXADC) += mt6359-auxadc.o
obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
obj-$(CONFIG_MEDIATEK_MT6370_ADC) += mt6370-adc.o
diff --git a/drivers/iio/adc/mt6323-auxadc.c b/drivers/iio/adc/mt6323-auxadc.c
new file mode 100644
index 000000000000..572466c3f375
--- /dev/null
+++ b/drivers/iio/adc/mt6323-auxadc.c
@@ -0,0 +1,314 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2026 Roman Vivchar <rva333@protonmail.com>
+ *
+ * Based on drivers/iio/adc/mt6359-auxadc.c
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/iio/iio.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/stringify.h>
+#include <linux/time.h>
+#include <linux/types.h>
+
+#include <linux/mfd/mt6323/registers.h>
+
+#include <dt-bindings/iio/adc/mediatek,mt6323-auxadc.h>
+
+#define AUXADC_STRUP_CON10_RSTB_SEL BIT(7)
+#define AUXADC_STRUP_CON10_RSTB_SW BIT(5)
+
+#define AUXADC_TOP_CKPDN2_CTL_CK BIT(5)
+
+#define AUXADC_TRIM_CH2_MASK GENMASK(11, 10)
+#define AUXADC_TRIM_CH4_MASK GENMASK(9, 8)
+#define AUXADC_TRIM_CH5_MASK GENMASK(5, 4)
+#define AUXADC_TRIM_CH6_MASK GENMASK(3, 2)
+
+#define AUXADC_CON27_VREF18_ENB_MD BIT(15)
+#define AUXADC_CON27_MD_STATUS BIT(0)
+
+#define AUXADC_CON19_GPS_STATUS BIT(1)
+
+#define AUXADC_CON26_VREF18_SELB BIT(1)
+#define AUXADC_CON26_DECI_GDLY_SEL BIT(0)
+
+#define AUXADC_CON11_VBUF_EN BIT(4)
+
+#define AUXADC_CON19_DECI_GDLY_MASK GENMASK(15, 14)
+#define AUXADC_ADC19_BUSY_MASK GENMASK(15, 1)
+#define AUXADC_READY_MASK BIT(15)
+#define AUXADC_DATA_MASK GENMASK(14, 0)
+
+#define AUXADC_CON9_OSR_MASK GENMASK(12, 10)
+#define AUXADC_DEFAULT_OSR 3
+
+#define MTK_PMIC_IIO_CHAN(_name, _chan, _addr) \
+{ \
+ .type = IIO_VOLTAGE, \
+ .indexed = 1, \
+ .channel = _chan, \
+ .address = _addr, \
+ .datasheet_name = __stringify(_name), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+}
+
+/*
+ * AUXADC reports everything in mV, including temperature and
+ * current channels. Channel macros are mapped such that their
+ * ID matches their respective hardware bit position in CON22.
+ */
+static const struct iio_chan_spec mt6323_auxadc_channels[] = {
+ MTK_PMIC_IIO_CHAN(baton2, MT6323_AUXADC_BATON2, MT6323_AUXADC_ADC6),
+ MTK_PMIC_IIO_CHAN(ch6, MT6323_AUXADC_CH6, MT6323_AUXADC_ADC11),
+ MTK_PMIC_IIO_CHAN(bat_temp, MT6323_AUXADC_BAT_TEMP, MT6323_AUXADC_ADC5),
+ MTK_PMIC_IIO_CHAN(chip_temp, MT6323_AUXADC_CHIP_TEMP, MT6323_AUXADC_ADC4),
+ MTK_PMIC_IIO_CHAN(vcdt, MT6323_AUXADC_VCDT, MT6323_AUXADC_ADC2),
+ MTK_PMIC_IIO_CHAN(baton1, MT6323_AUXADC_BATON1, MT6323_AUXADC_ADC3),
+ MTK_PMIC_IIO_CHAN(isense, MT6323_AUXADC_ISENSE, MT6323_AUXADC_ADC1),
+ MTK_PMIC_IIO_CHAN(batsns, MT6323_AUXADC_BATSNS, MT6323_AUXADC_ADC0),
+ MTK_PMIC_IIO_CHAN(accdet, MT6323_AUXADC_ACCDET, MT6323_AUXADC_ADC7),
+};
+
+/*
+ * The MediaTek MT6323 (as well as a lot of other PMICs) has the following hierarchy:
+ * PMIC AUXADC <- PMIC MFD <- SoC PWRAP (wrapper for PWRAP FSM)
+ *
+ * Therefore, PWRAP regmap should be obtained using dev->parent->parent.
+ */
+struct mt6323_auxadc {
+ struct regmap *regmap;
+ /* AUXADC doesn't support reading multiple channels simultaneously. */
+ struct mutex lock;
+};
+
+static int mt6323_auxadc_prepare_channel(struct mt6323_auxadc *auxadc)
+{
+ struct regmap *map = auxadc->regmap;
+ u32 val;
+ int ret;
+
+ ret = regmap_read(map, MT6323_AUXADC_CON19, &val);
+ if (ret)
+ return ret;
+
+ /* The ADC is idle. */
+ if (!(val & AUXADC_CON19_DECI_GDLY_MASK))
+ return 0;
+
+ ret = regmap_read_poll_timeout(map, MT6323_AUXADC_ADC19, val,
+ !(val & AUXADC_ADC19_BUSY_MASK),
+ 10, 500);
+ if (ret)
+ return ret;
+
+ return regmap_clear_bits(map, MT6323_AUXADC_CON19,
+ AUXADC_CON19_DECI_GDLY_MASK);
+}
+
+static int mt6323_auxadc_request(struct mt6323_auxadc *auxadc,
+ unsigned long channel)
+{
+ struct regmap *map = auxadc->regmap;
+ int ret;
+
+ ret = regmap_set_bits(map, MT6323_AUXADC_CON11, AUXADC_CON11_VBUF_EN);
+ if (ret)
+ return ret;
+
+ return regmap_set_bits(map, MT6323_AUXADC_CON22, BIT(channel));
+}
+
+static int mt6323_auxadc_release(struct mt6323_auxadc *auxadc,
+ unsigned long channel)
+{
+ struct regmap *map = auxadc->regmap;
+ int ret;
+
+ ret = regmap_clear_bits(map, MT6323_AUXADC_CON22, BIT(channel));
+ if (ret)
+ return ret;
+
+ return regmap_clear_bits(map, MT6323_AUXADC_CON11, AUXADC_CON11_VBUF_EN);
+}
+
+static int mt6323_auxadc_read(struct mt6323_auxadc *auxadc,
+ const struct iio_chan_spec *chan, int *out)
+{
+ struct regmap *map = auxadc->regmap;
+ u32 val;
+ int ret;
+
+ ret = regmap_read_poll_timeout(map, chan->address,
+ val, (val & AUXADC_READY_MASK),
+ 1 * USEC_PER_MSEC, 100 * USEC_PER_MSEC);
+ if (ret)
+ return ret;
+
+ *out = FIELD_GET(AUXADC_DATA_MASK, val);
+
+ return 0;
+}
+
+static int mt6323_auxadc_read_raw(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ int *val, int *val2, long mask)
+{
+ struct mt6323_auxadc *auxadc = iio_priv(indio_dev);
+ int ret, mult;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ if (chan->channel == MT6323_AUXADC_ISENSE ||
+ chan->channel == MT6323_AUXADC_BATSNS)
+ mult = 4;
+ else
+ mult = 1;
+
+ /* 1800mV full range with 15-bit resolution. */
+ *val = mult * 1800;
+ *val2 = 15;
+
+ return IIO_VAL_FRACTIONAL_LOG2;
+ case IIO_CHAN_INFO_RAW: {
+ guard(mutex)(&auxadc->lock);
+
+ ret = mt6323_auxadc_prepare_channel(auxadc);
+ if (ret)
+ return ret;
+
+ ret = mt6323_auxadc_request(auxadc, chan->channel);
+ if (ret)
+ return ret;
+
+ /* Hardware limitation: the AUXADC needs a delay to become ready. */
+ fsleep(300);
+
+ ret = mt6323_auxadc_read(auxadc, chan, val);
+
+ if (mt6323_auxadc_release(auxadc, chan->channel))
+ dev_err(&indio_dev->dev,
+ "failed to release channel %d\n", chan->channel);
+
+ if (ret)
+ return ret;
+
+ return IIO_VAL_INT;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
+static int mt6323_auxadc_init(struct mt6323_auxadc *auxadc)
+{
+ struct regmap *map = auxadc->regmap;
+ int ret;
+
+ ret = regmap_set_bits(map, MT6323_STRUP_CON10,
+ AUXADC_STRUP_CON10_RSTB_SW |
+ AUXADC_STRUP_CON10_RSTB_SEL);
+ if (ret)
+ return ret;
+
+ ret = regmap_set_bits(map, MT6323_TOP_CKPDN2, AUXADC_TOP_CKPDN2_CTL_CK);
+ if (ret)
+ return ret;
+
+ ret = regmap_update_bits(map, MT6323_AUXADC_CON10,
+ AUXADC_TRIM_CH2_MASK | AUXADC_TRIM_CH4_MASK |
+ AUXADC_TRIM_CH5_MASK | AUXADC_TRIM_CH6_MASK,
+ FIELD_PREP(AUXADC_TRIM_CH2_MASK, 1) |
+ FIELD_PREP(AUXADC_TRIM_CH4_MASK, 1) |
+ FIELD_PREP(AUXADC_TRIM_CH5_MASK, 1) |
+ FIELD_PREP(AUXADC_TRIM_CH6_MASK, 1));
+ if (ret)
+ return ret;
+
+ ret = regmap_set_bits(map, MT6323_AUXADC_CON27,
+ AUXADC_CON27_VREF18_ENB_MD |
+ AUXADC_CON27_MD_STATUS);
+ if (ret)
+ return ret;
+
+ ret = regmap_set_bits(map, MT6323_AUXADC_CON19, AUXADC_CON19_GPS_STATUS);
+ if (ret)
+ return ret;
+
+ ret = regmap_set_bits(map, MT6323_AUXADC_CON26,
+ AUXADC_CON26_VREF18_SELB |
+ AUXADC_CON26_DECI_GDLY_SEL);
+ if (ret)
+ return ret;
+
+ return regmap_update_bits(map, MT6323_AUXADC_CON9, AUXADC_CON9_OSR_MASK,
+ FIELD_PREP(AUXADC_CON9_OSR_MASK, AUXADC_DEFAULT_OSR));
+}
+
+static const struct iio_info mt6323_auxadc_iio_info = {
+ .read_raw = mt6323_auxadc_read_raw,
+};
+
+static int mt6323_auxadc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct mt6323_auxadc *auxadc;
+ struct regmap *regmap;
+ struct iio_dev *iio;
+ int ret;
+
+ regmap = dev_get_regmap(dev->parent->parent, NULL);
+ if (!regmap)
+ return dev_err_probe(dev, -ENODEV, "failed to get regmap\n");
+
+ iio = devm_iio_device_alloc(dev, sizeof(*auxadc));
+ if (!iio)
+ return -ENOMEM;
+
+ auxadc = iio_priv(iio);
+ auxadc->regmap = regmap;
+
+ ret = devm_mutex_init(dev, &auxadc->lock);
+ if (ret)
+ return ret;
+
+ ret = mt6323_auxadc_init(auxadc);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to initialize auxadc\n");
+
+ iio->name = "mt6323-auxadc";
+ iio->info = &mt6323_auxadc_iio_info;
+ iio->modes = INDIO_DIRECT_MODE;
+ iio->channels = mt6323_auxadc_channels;
+ iio->num_channels = ARRAY_SIZE(mt6323_auxadc_channels);
+
+ return devm_iio_device_register(dev, iio);
+}
+
+static const struct of_device_id mt6323_auxadc_of_match[] = {
+ { .compatible = "mediatek,mt6323-auxadc" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, mt6323_auxadc_of_match);
+
+static struct platform_driver mt6323_auxadc_driver = {
+ .driver = {
+ .name = "mt6323-auxadc",
+ .of_match_table = mt6323_auxadc_of_match,
+ },
+ .probe = mt6323_auxadc_probe,
+};
+module_platform_driver(mt6323_auxadc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("MediaTek MT6323 PMIC AUXADC Driver");
--
2.54.0
^ permalink raw reply related
* [PATCH v3 3/4] mfd: mt6397-core: add mt6323 AUXADC support
From: Roman Vivchar via B4 Relay @ 2026-06-16 14:15 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Lee Jones
Cc: linux-iio, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, Ben Grisdale, Roman Vivchar
In-Reply-To: <20260616-mt6323-adc-v3-0-1c27c588185d@protonmail.com>
From: Roman Vivchar <rva333@protonmail.com>
The mt6323 PMIC includes an AUXADC. Register the AUXADC in the mt6323
devices array to allow the corresponding driver to probe using compatible
string.
Tested-by: Ben Grisdale <bengris32@protonmail.ch> # Amazon Echo Dot (2nd Generation)
Signed-off-by: Roman Vivchar <rva333@protonmail.com>
---
drivers/mfd/mt6397-core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
index 3e58d0764c7e..013b0857fb54 100644
--- a/drivers/mfd/mt6397-core.c
+++ b/drivers/mfd/mt6397-core.c
@@ -125,6 +125,9 @@ static const struct resource mt6323_pwrc_resources[] = {
static const struct mfd_cell mt6323_devs[] = {
{
+ .name = "mt6323-auxadc",
+ .of_compatible = "mediatek,mt6323-auxadc",
+ }, {
.name = "mt6323-rtc",
.num_resources = ARRAY_SIZE(mt6323_rtc_resources),
.resources = mt6323_rtc_resources,
--
2.54.0
^ permalink raw reply related
* [PATCH v3 4/4] ARM: dts: mediatek: mt6323: add AUXADC support
From: Roman Vivchar via B4 Relay @ 2026-06-16 14:15 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Lee Jones
Cc: linux-iio, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, Ben Grisdale, Roman Vivchar
In-Reply-To: <20260616-mt6323-adc-v3-0-1c27c588185d@protonmail.com>
From: Roman Vivchar <rva333@protonmail.com>
Add the devicetree node for the mt6323 AUXADC.
Tested-by: Ben Grisdale <bengris32@protonmail.ch> # Amazon Echo Dot (2nd Generation)
Signed-off-by: Roman Vivchar <rva333@protonmail.com>
---
arch/arm/boot/dts/mediatek/mt6323.dtsi | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/mediatek/mt6323.dtsi b/arch/arm/boot/dts/mediatek/mt6323.dtsi
index c230c865116d..c070f4b0936c 100644
--- a/arch/arm/boot/dts/mediatek/mt6323.dtsi
+++ b/arch/arm/boot/dts/mediatek/mt6323.dtsi
@@ -14,6 +14,11 @@ pmic: mt6323 {
interrupt-controller;
#interrupt-cells = <2>;
+ mt6323_adc: adc {
+ compatible = "mediatek,mt6323-auxadc";
+ #io-channel-cells = <1>;
+ };
+
mt6323_leds: leds {
compatible = "mediatek,mt6323-led";
#address-cells = <1>;
--
2.54.0
^ permalink raw reply related
* Re: [PATCH 6/7] hwmon: adm1275: Support ROHM BD12790
From: Guenter Roeck @ 2026-06-16 14:17 UTC (permalink / raw)
To: sashiko-reviews, Matti Vaittinen; +Cc: conor+dt, devicetree, linux-hwmon, robh
In-Reply-To: <20260616065944.95FF11F00A3A@smtp.kernel.org>
On 6/15/26 23:59, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] The driver compares the device-returned PMBUS_MFR_MODEL string using strncasecmp without verifying the returned length, leading to uninitialized stack memory reads.
> - [High] Multiplication of power and current coefficients by `shunt` overflows a 32-bit unsigned integer for larger shunt resistors.
> --
>
> commit 8dff4bf5659b589eabe7fc876665de9339faae1f
> Author: Matti Vaittinen <mazziesaccount@gmail.com>
>
> hwmon: adm1275: Support ROHM BD12790
>
> Add support for ROHM BD12790 hot-swap controller which is largely
> similar to Analog Devices adm1272.
>
>> diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
>> --- a/drivers/hwmon/pmbus/adm1275.c
>> +++ b/drivers/hwmon/pmbus/adm1275.c
> [ ... ]
>> @@ -581,7 +606,8 @@ static int adm1275_probe(struct i2c_client *client)
>> if (mid->driver_data == adm1272 || mid->driver_data == adm1273 ||
>> mid->driver_data == adm1278 || mid->driver_data == adm1281 ||
>> mid->driver_data == adm1293 || mid->driver_data == adm1294 ||
>> - mid->driver_data == bd12780 || mid->driver_data == sq24905c)
>> + mid->driver_data == bd12780 || mid->driver_data == bd12790 ||
>> + mid->driver_data == sq24905c)
>> config_read_fn = i2c_smbus_read_word_data;
>> else
>
> [Severity: High]
> This is a pre-existing issue, but I noticed that just before this block in
> adm1275_probe(), there might be an out-of-bounds stack read:
>
> ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, block_buffer);
> if (ret < 0) { ... }
> for (mid = adm1275_id; mid->name[0]; mid++) {
> if (!strncasecmp(mid->name, block_buffer, strlen(mid->name)))
> break;
> }
>
> Does this code read uninitialized stack memory if the device returns a
> string that matches a valid model name prefix, but is shorter than the valid
> model name length?
>
> Since i2c_smbus_read_block_data() reads up to 32 bytes into the uninitialized
> stack array block_buffer without appending a null terminator, strncasecmp()
> could read past the valid bytes returned in ret.
>
> For example, if the device returns a shorter string like "adm12", checking
> it against "adm1275" up to the length of "adm1275" will continue reading
> into uninitialized stack bounds.
>
> [ ... ]
>> @@ -685,12 +722,23 @@ static int adm1275_probe(struct i2c_client *client)
>> break;
>> case adm1272:
>> case adm1273:
>> + case bd12790:
>> + {
> [ ... ]
>
> [Severity: High]
> This isn't a bug introduced by this patch, but looking further down in
> adm1275_probe(), could the coefficient calculations for current and power
> overflow a 32-bit unsigned integer?
>
> info->m[PSC_CURRENT_OUT] =
> coefficients[cindex].m * shunt / 1000;
>
> info->m[PSC_POWER] =
> coefficients[pindex].m * shunt / 1000;
>
> If a larger shunt resistor value is provided via device tree
> (e.g., 200,000 micro-ohms to measure lower currents), multiplying this by
> larger coefficients like 30631 (used for ADM1293) evaluates to 6,126,200,000.
>
> Since shunt is a u32 and the coefficient promotes to int, this operation uses
> 32-bit unsigned arithmetic.
>
> Does this wrap-around the maximum 32-bit value of 4,294,967,295 and result
> in drastically incorrect telemetry scaling factors?
>
Feel freew to fix (or not to fix) those problems. They should get fixed, but
not necessarily as part of this series.
Thanks,
Guenter
^ permalink raw reply
* Re: [PATCH v3 3/4] mfd: mt6397-core: add mt6323 AUXADC support
From: sashiko-bot @ 2026-06-16 14:28 UTC (permalink / raw)
To: Roman Vivchar via B4 Relay; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260616-mt6323-adc-v3-3-1c27c588185d@protonmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mt6323_auxadc_init incorrectly powers down the AUXADC clock instead of enabling it.
- [Medium] Failure during mt6323_auxadc_request() does not revert the hardware voltage buffer enablement, leading to a permanent hardware state leak.
--
commit 6b92fe47672fd5e88ade7a2d9b916034b0dc7bca
Author: Roman Vivchar <rva333@protonmail.com>
mfd: mt6397-core: add mt6323 AUXADC support
The mt6323 PMIC includes an AUXADC. Register the AUXADC in the mt6323
devices array to allow the corresponding driver to probe using compatible
string.
> diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
> index 3e58d0764c7e0..013b0857fb548 100644
> --- a/drivers/mfd/mt6397-core.c
> +++ b/drivers/mfd/mt6397-core.c
> @@ -125,6 +125,9 @@ static const struct resource mt6323_pwrc_resources[] = {
>
> static const struct mfd_cell mt6323_devs[] = {
> {
> + .name = "mt6323-auxadc",
> + .of_compatible = "mediatek,mt6323-auxadc",
> + }, {
[Severity: High]
While reviewing the driver this enables, does mt6323_auxadc_init()
incorrectly power down the AUXADC clock?
drivers/iio/adc/mt6323-auxadc.c:mt6323_auxadc_init() {
...
ret = regmap_set_bits(map, MT6323_TOP_CKPDN2, AUXADC_TOP_CKPDN2_CTL_CK);
...
}
The Mediatek PMIC TOP_CKPDN2 register is a "Clock Power Down" register,
meaning that setting a bit to 1 disables the corresponding clock. By using
regmap_set_bits() here, the clock appears to be explicitly powered down
instead of enabled. This would prevent the AUXADC hardware from functioning
and cause subsequent reads in mt6323_auxadc_read() to perpetually time out.
[Severity: Medium]
Also, does a failure during mt6323_auxadc_request() leak the hardware
voltage buffer state?
drivers/iio/adc/mt6323-auxadc.c:mt6323_auxadc_request() {
...
ret = regmap_set_bits(map, MT6323_AUXADC_CON11, AUXADC_CON11_VBUF_EN);
if (ret)
return ret;
return regmap_set_bits(map, MT6323_AUXADC_CON22, BIT(channel));
}
If the second regmap_set_bits() fails, the function returns without undoing
the enablement of AUXADC_CON11_VBUF_EN.
This error then propagates up to mt6323_auxadc_read_raw():
drivers/iio/adc/mt6323-auxadc.c:mt6323_auxadc_read_raw() {
...
ret = mt6323_auxadc_request(auxadc, chan->channel);
if (ret)
return ret;
...
}
Because this returns immediately on error, it skips the
mt6323_auxadc_release() cleanup step. This leaves the hardware voltage
buffer permanently enabled until the device is reset or a subsequent read
succeeds.
> .name = "mt6323-rtc",
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-mt6323-adc-v3-0-1c27c588185d@protonmail.com?part=3
^ permalink raw reply
* Re: [PATCH v3 2/4] iio: adc: mt6323-auxadc: add mt6323 PMIC AUXADC driver
From: sashiko-bot @ 2026-06-16 14:30 UTC (permalink / raw)
To: Roman Vivchar via B4 Relay; +Cc: devicetree, robh, conor+dt
In-Reply-To: <20260616-mt6323-adc-v3-2-1c27c588185d@protonmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The driver incorrectly powers down the AUXADC clock during initialization, preventing the ADC from functioning.
- [High] Unchecked dereference of `dev->parent` when fetching the regmap could lead to a kernel panic.
- [Medium] The voltage buffer (`VBUF_EN`) is permanently left enabled if an error occurs during channel request or release.
--
commit ec67a1445925e2d4ca9ef40c52f5417793b977c8
Author: Roman Vivchar <rva333@protonmail.com>
Subject: iio: adc: mt6323-auxadc: add mt6323 PMIC AUXADC driver
The mt6323 AUXADC is a 15-bit ADC used for system monitoring. This driver
provides support for reading various channels including battery and
charger voltages, battery and chip temperature, current sensing and
accessory detection. Add a driver for the AUXADC found in the MediaTek
mt6323 PMIC.
Tested-by: Ben Grisdale <bengris32@protonmail.ch>
Signed-off-by: Roman Vivchar <rva333@protonmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
[ ... ]
> diff --git a/drivers/iio/adc/mt6323-auxadc.c b/drivers/iio/adc/mt6323-auxadc.c
> new file mode 100644
> index 0000000000000..572466c3f375f
> --- /dev/null
> +++ b/drivers/iio/adc/mt6323-auxadc.c
[ ... ]
> +static int mt6323_auxadc_request(struct mt6323_auxadc *auxadc,
> + unsigned long channel)
> +{
> + struct regmap *map = auxadc->regmap;
> + int ret;
> +
> + ret = regmap_set_bits(map, MT6323_AUXADC_CON11, AUXADC_CON11_VBUF_EN);
> + if (ret)
> + return ret;
> +
> + return regmap_set_bits(map, MT6323_AUXADC_CON22, BIT(channel));
[Severity: Medium]
If regmap_set_bits() for MT6323_AUXADC_CON22 fails here, does this
leave the AUXADC_CON11_VBUF_EN bit permanently enabled?
It looks like mt6323_auxadc_read_raw() aborts on a request error without
calling mt6323_auxadc_release().
> +}
> +
> +static int mt6323_auxadc_release(struct mt6323_auxadc *auxadc,
> + unsigned long channel)
> +{
> + struct regmap *map = auxadc->regmap;
> + int ret;
> +
> + ret = regmap_clear_bits(map, MT6323_AUXADC_CON22, BIT(channel));
> + if (ret)
> + return ret;
> +
> + return regmap_clear_bits(map, MT6323_AUXADC_CON11, AUXADC_CON11_VBUF_EN);
Similarly, if mt6323_auxadc_release() fails to clear the channel bit, it
returns early and skips disabling VBUF_EN. Should these error paths ensure
VBUF_EN is disabled before returning?
> +}
[ ... ]
> +static int mt6323_auxadc_init(struct mt6323_auxadc *auxadc)
> +{
> + struct regmap *map = auxadc->regmap;
> + int ret;
> +
> + ret = regmap_set_bits(map, MT6323_STRUP_CON10,
> + AUXADC_STRUP_CON10_RSTB_SW |
> + AUXADC_STRUP_CON10_RSTB_SEL);
> + if (ret)
> + return ret;
> +
> + ret = regmap_set_bits(map, MT6323_TOP_CKPDN2, AUXADC_TOP_CKPDN2_CTL_CK);
[Severity: High]
Does setting this bit actually disable the AUXADC clock?
In MediaTek PMICs, CKPDN typically stands for Clock Power Down. If setting
this bit to 1 disables the clock, and it is never cleared, wouldn't the
clock remain powered down indefinitely and cause all subsequent ADC reads
to time out or hang the hardware?
Should this use regmap_clear_bits() instead to enable the clock?
> + if (ret)
> + return ret;
[ ... ]
> +static int mt6323_auxadc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct mt6323_auxadc *auxadc;
> + struct regmap *regmap;
> + struct iio_dev *iio;
> + int ret;
> +
> + regmap = dev_get_regmap(dev->parent->parent, NULL);
[Severity: High]
Can dev->parent or dev->parent->parent be NULL here if the device is
instantiated outside of its expected MFD hierarchy?
For example, if instantiated via a misconfigured Device Tree overlay or
direct sysfs binding, dev->parent could be &platform_bus, which has a NULL
parent.
Since dev_get_regmap() invokes devres_find() which unconditionally
dereferences the device pointer, would passing a NULL parent cause a
kernel panic during probe? Should the parent chain be validated before
dereferencing?
> + if (!regmap)
> + return dev_err_probe(dev, -ENODEV, "failed to get regmap\n");
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-mt6323-adc-v3-0-1c27c588185d@protonmail.com?part=2
^ permalink raw reply
* Re: [PATCH net-next v7 01/12] net: phylink: keep and use MAC supported_interfaces in phylink struct
From: sashiko-bot @ 2026-06-16 14:32 UTC (permalink / raw)
To: Christian Marangi; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260615122950.22281-2-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch makes a one-time copy of `supported_interfaces` during `phylink_create()`, which breaks drivers like `mtk_eth_soc` that modify `phylink_config.supported_interfaces` after calling `phylink_create()`.
--
commit c809e12abb9cfbf31d18f5e71f4e3d15bb4f6a01
Author: Christian Marangi <ansuelsmth@gmail.com>
net: phylink: keep and use MAC supported_interfaces in phylink struct
This patch introduces a cached copy of the supported_interfaces mask in the
phylink struct. It initializes this copy during phylink_create() and
transitions the phylink core to validate against this internal mask rather
than the original phylink_config.
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 087ac63f9193d..4d59c0dd78dbf 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -1868,6 +1873,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);
> +
[Severity: High]
Does this one-time copy in phylink_create() break drivers that modify their
supported_interfaces after initialization?
For example, in drivers/net/ethernet/mediatek/mtk_eth_soc.c:mtk_add_mac(),
the driver appends PHY_INTERFACE_MODE_INTERNAL to
mac->phylink_config.supported_interfaces after phylink_create() has already
been called:
drivers/net/ethernet/mediatek/mtk_eth_soc.c:mtk_add_mac() {
...
mac->phylink = phylink_create(&mac->phylink_config, ...);
...
if (MTK_HAS_CAPS(mac->hw->soc->caps, MTK_2P5GPHY) &&
id == MTK_GMAC2_ID)
__set_bit(PHY_INTERFACE_MODE_INTERNAL,
mac->phylink_config.supported_interfaces);
...
}
Since phylink now validates against the cached pl->supported_interfaces mask,
will it reject PHY_INTERFACE_MODE_INTERNAL as unsupported and prevent the
internal 2.5G PHY from linking successfully on affected Mediatek platforms?
> pl->config = config;
> if (config->type == PHYLINK_NETDEV) {
> pl->netdev = to_net_dev(config->dev);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615122950.22281-1-ansuelsmth@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH net-next v7 05/12] net: phylink: support late PCS provider attach
From: sashiko-bot @ 2026-06-16 14:32 UTC (permalink / raw)
To: Christian Marangi; +Cc: conor+dt, devicetree, robh
In-Reply-To: <20260615122950.22281-6-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Use-After-Free in PCS global notifier list due to missing unregister in `phylink_create()` error paths.
- [High] NULL pointer dereference due to uninitialized `pl->config` in the PCS notifier callback.
- [High] List corruption race between `phylink_create()` and `fwnode_pcs_add_provider()`.
- [High] Use-After-Free of `phylink_pcs` structure in `pcs_provider_notify()` due to missing reference counting.
- [Medium] TOCTOU race condition in `phylink_create()` causes missed PCS attachments.
--
commit e3c74d4cdf92d51d1f16bc8736f6fb1e1800f91d
Author: Christian Marangi <ansuelsmth@gmail.com>
net: phylink: support late PCS provider attach
This patch adds support for late PCS provider attachment by introducing a
global notifier. Phylink instances subscribe to this notifier and react to
FWNODE_PCS_PROVIDER_ADD events by linking the new PCS and refreshing
supported interfaces.
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index ca4dad4b140a0..0734c98498a99 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -1997,6 +1999,51 @@ 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;
> +
> + /* 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]
Could this cause a NULL pointer dereference?
The notifier is registered in phylink_create() before pl->config is
initialized. If a concurrent thread adds a PCS provider immediately after
registration, won't pcs_provider_notify() attempt to access pl->config->dev
while it is still NULL?
> + if (IS_ERR(pcs))
> + return NOTIFY_DONE;
> +
> + /* Add the PCS */
> + rtnl_lock();
[Severity: High]
Does this code risk a use-after-free?
The phylink_pcs pointer is retrieved without incrementing a reference count,
and then the thread can sleep while waiting for rtnl_lock().
If the module providing the PCS is concurrently unbound during this wait,
freeing the phylink_pcs structure, will this code dereference the freed
pointer once the lock is acquired?
> +
> + mutex_lock(&pl->state_mutex);
> +
> + /* Link PCS with phylink */
> + list_add(&pcs->list, &pl->pcs_list);
[Severity: High]
Is it possible for the list to become corrupted here?
If phylink_fill_available_pcs() finds the newly added PCS and links it to
pl->pcs_list during phylink_create(), and then the delayed
FWNODE_PCS_PROVIDER_ADD notification fires right after, won't
pcs_provider_notify() unconditionally add the exact same pcs structure
to the list again?
[ ... ]
> @@ -2068,6 +2115,12 @@ struct phylink *phylink_create(struct phylink_config *config,
> pl->supported_interfaces,
> pcs->supported_interfaces);
>
> + /* 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: Medium]
Is there a race condition window here?
If a new PCS provider is added concurrently between the earlier call to
phylink_fill_available_pcs() and the registration of the notifier here,
will the FWNODE_PCS_PROVIDER_ADD event fire before the notifier is active,
causing the PCS to be permanently missed?
> pl->config = config;
> if (config->type == PHYLINK_NETDEV) {
> pl->netdev = to_net_dev(config->dev);
[Severity: High]
If phylink_create() encounters an error later in the function (like an
invalid config->type or phylink_parse_mode() failing), does this leave
a dangling pointer in the global notifier chain since
unregister_fwnode_pcs_notifier() isn't called before kfree(pl)?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615122950.22281-1-ansuelsmth@gmail.com?part=5
^ permalink raw reply
* Re: [PATCH net-next v7 09/12] net: phylink: add .pcs_link_down PCS OP
From: sashiko-bot @ 2026-06-16 14:32 UTC (permalink / raw)
To: Christian Marangi; +Cc: devicetree, conor+dt, robh
In-Reply-To: <20260615122950.22281-10-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Data Race and Use-After-Free in `phylink_destroy()` due to incorrect cancellation order
- [Medium] Missing Link Teardown Callback (pcs_link_down) on PCS release
- [Low] Missing Kernel-Doc for `pcs_link_down`
--
commit 33892cef9bd3c637bb8d5d91bf19b186872ff8bf
Author: Christian Marangi <ansuelsmth@gmail.com>
net: phylink: add .pcs_link_down PCS OP
Permit for PCS driver to define specific operation to tear down the link
between the MAC and the PCS. This might be needed for some PCS that reset
counter or require special reset to correctly work if the link needs to be
restored later.
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 0734c98498a99..1bedac517d213 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -1736,6 +1742,9 @@ static void phylink_link_down(struct phylink *pl)
>
> pl->mac_ops->mac_link_down(pl->config, pl->act_link_an_mode,
> pl->cur_interface);
> +
> + phylink_pcs_link_down(pl->pcs);
[Severity: Medium]
Will this callback be missed if a PCS is released?
In phylink_release_pcs(), pl->pcs is disabled and set to NULL while the link
is logically UP:
if (pl->pcs == pcs) {
phylink_pcs_disable(pl->pcs);
pl->force_major_config = true;
pl->pcs = NULL;
}
When this triggers phylink_resolve(), which attempts to bring the link down
using phylink_link_down(), pl->pcs is already NULL. The released PCS never
receives its .pcs_link_down() callback, which might break link lifecycle
symmetry and fail to reset internal counters.
> +
> phylink_info(pl, "Link is Down\n");
> }
>
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 15e6b1a39dfe2..eb6c6ca34147f 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -528,6 +528,7 @@ struct phylink_pcs {
> * @pcs_an_restart: restart 802.3z BaseX autonegotiation.
> * @pcs_link_up: program the PCS for the resolved link configuration
> * (where necessary).
> + * @pcs_link_down: tear down link between MAC and PCS.
[Severity: Low]
Is there a missing kernel-doc entry for the function signature of
pcs_link_down?
The @pcs_link_down callback is added here, but the detailed function signature
appears to be missing from the corresponding "#if 0" block in
include/linux/phylink.h (for kernel-doc purposes only), where other PCS
callbacks like pcs_link_up() are fully documented.
> * @pcs_disable_eee: optional notification to PCS that EEE has been disabled
> * at the MAC.
> * @pcs_enable_eee: optional notification to PCS that EEE will be enabled at
[ ... ]
[Severity: High]
Does the incorrect cancellation order in phylink_destroy() cause a data race
and use-after-free?
In phylink_destroy(), pl->pcs_list is modified without holding
pl->state_mutex:
/* Remove every PCS from phylink PCS list */
list_for_each_entry_safe(pcs, tmp, &pl->pcs_list, list)
list_del(&pcs->list);
cancel_work_sync(&pl->resolve);
If the background work pl->resolve is currently running, the worker thread
executes phylink_major_config() and iterates over pl->pcs_list while holding
pl->state_mutex. Because phylink_destroy() modifies the list concurrently
without synchronization and before canceling the work, the worker thread can
traverse corrupted list pointers, resulting in a use-after-free. Should
cancel_work_sync(&pl->resolve) be called before modifying the list?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615122950.22281-1-ansuelsmth@gmail.com?part=9
^ permalink raw reply
* Re: [PATCH net-next v7 04/12] net: pcs: implement Firmware node support for PCS driver
From: sashiko-bot @ 2026-06-16 14:32 UTC (permalink / raw)
To: Christian Marangi; +Cc: devicetree, robh, conor+dt
In-Reply-To: <20260615122950.22281-5-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Type mismatch in `fwnode_parse_pcsspec()` where a negative error code is assigned to an unsigned integer, making the error check dead code.
- [Medium] `fwnode_phylink_pcs_parse()` incorrectly aborts on out-of-bounds instead of returning the parsed count, violating its 'up to num_pcs' contract.
- [Low] The fallback stub for `fwnode_pcs_get()` uses an incorrect parameter type.
--
commit 5e32951da9dab6acca331c331c1e41d55a580883
Author: Christian Marangi <ansuelsmth@gmail.com>
net: pcs: implement Firmware node support for PCS driver
This commit implements the foundation of Firmware node support for PCS
drivers. It introduces a simple Provider API where a PCS driver can expose
multiple PCS and consumers can access them using fwnode_pcs_get().
> diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/pcs/pcs.c
[ ... ]
> +static int fwnode_parse_pcsspec(const struct fwnode_handle *fwnode,
> + unsigned int index, const char *name,
> + struct fwnode_reference_args *out_args)
> +{
> + int ret;
> +
> + if (!fwnode)
> + return -EINVAL;
> +
> + if (name) {
> + index = fwnode_property_match_string(fwnode, "pcs-names",
> + name);
> + if (index < 0)
> + return index;
[Severity: Medium]
Does this error check work as intended? fwnode_property_match_string()
returns negative error codes like -ENODATA, but it is assigned to index
which is an unsigned int. The check for index < 0 will be dead code,
potentially leading to an out-of-bounds query that returns -ENOENT.
> + }
> +
> + ret = fwnode_property_get_reference_args(fwnode, "pcs-handle",
> + "#pcs-cells",
> + -1, index, out_args);
[ ... ]
> +int fwnode_phylink_pcs_parse(struct fwnode_handle *fwnode,
> + struct phylink_pcs **available_pcs,
> + unsigned int num_pcs)
> +{
> + unsigned int i, found = 0;
> +
> + if (!available_pcs)
> + return -EINVAL;
> +
> + if (!fwnode_property_present(fwnode, "pcs-handle"))
> + return -ENODEV;
> +
> + for (i = 0; i < num_pcs; i++) {
> + struct phylink_pcs *pcs;
> +
> + pcs = fwnode_pcs_get(fwnode, i);
> + if (IS_ERR(pcs)) {
> + /*
> + * Ignore -ENODEV error for PCS that still
> + * needs to probe.
> + */
> + if (PTR_ERR(pcs) == -ENODEV)
> + continue;
> +
> + return PTR_ERR(pcs);
[Severity: Medium]
If the firmware node contains fewer pcs-handle properties than num_pcs,
won't fwnode_pcs_get() eventually return -ENOENT? Since this loop only
ignores -ENODEV, does this cause the function to abort entirely and return
an error instead of returning the successfully parsed count?
> + }
> +
> + available_pcs[found] = pcs;
> + found++;
> + }
> +
> + return found;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_phylink_pcs_parse);
[ ... ]
> diff --git a/include/linux/pcs/pcs.h b/include/linux/pcs/pcs.h
> new file mode 100644
> --- /dev/null
> +++ b/include/linux/pcs/pcs.h
[ ... ]
> +#else
> +static inline struct phylink_pcs *fwnode_pcs_get(struct fwnode_handle *fwnode,
> + int index)
[Severity: Low]
Is this intentional? The fallback stub for fwnode_pcs_get() uses int index,
but the active declaration and implementation use unsigned int index.
> +{
> + return ERR_PTR(-ENOENT);
> +}
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615122950.22281-1-ansuelsmth@gmail.com?part=4
^ permalink raw reply
* Re: [PATCH net-next v7 06/12] net: Document PCS subsystem
From: sashiko-bot @ 2026-06-16 14:32 UTC (permalink / raw)
To: Christian Marangi; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260615122950.22281-7-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Error paths in `phylink_create()` free the `phylink` struct without unregistering the PCS notifier.
- [High] The PCS notifier is registered before the `phylink` instance is fully initialized, allowing concurrent notifications to dereference a NULL pointer.
- [Medium] Always-false unsigned comparison in `fwnode_parse_pcsspec()` masks actual error returns.
- [Medium] `fwnode_phylink_pcs_parse()` fails completely if `num_pcs` exceeds the actual number of entries in the device tree.
--
commit 8a4a6df4b5091984f40b8415d9bca38cf005d9f4
Author: Christian Marangi <ansuelsmth@gmail.com>
net: Document PCS subsystem
Add extensive documentation of the new PCS subsystem and the fwnode
implementation with producer/consumer API.
> diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
> new file mode 100644
> index 0000000000000..713b2ec22c97e
> --- /dev/null
> +++ b/drivers/net/pcs/pcs.c
> @@ -0,0 +1,257 @@
[ ... ]
> +static int fwnode_parse_pcsspec(const struct fwnode_handle *fwnode,
> + unsigned int index, const char *name,
> + struct fwnode_reference_args *out_args)
> +{
> + int ret;
> +
> + if (!fwnode)
> + return -EINVAL;
> +
> + if (name) {
> + index = fwnode_property_match_string(fwnode, "pcs-names",
> + name);
> + if (index < 0)
[Severity: Medium]
Since index is declared as an unsigned int, will this check for < 0 always be
false, potentially masking error returns from fwnode_property_match_string()?
> + return index;
> + }
> +
> + ret = fwnode_property_get_reference_args(fwnode, "pcs-handle",
> + "#pcs-cells",
> + -1, index, out_args);
> + if (ret || (name && index < 0))
> + return ret;
> +
> + return 0;
> +}
[ ... ]
> +int fwnode_phylink_pcs_parse(struct fwnode_handle *fwnode,
> + struct phylink_pcs **available_pcs,
> + unsigned int num_pcs)
> +{
> + unsigned int i, found = 0;
> +
> + if (!available_pcs)
> + return -EINVAL;
> +
> + if (!fwnode_property_present(fwnode, "pcs-handle"))
> + return -ENODEV;
> +
> + for (i = 0; i < num_pcs; i++) {
> + struct phylink_pcs *pcs;
> +
> + pcs = fwnode_pcs_get(fwnode, i);
> + if (IS_ERR(pcs)) {
> + /*
> + * Ignore -ENODEV error for PCS that still
> + * needs to probe.
> + */
> + if (PTR_ERR(pcs) == -ENODEV)
> + continue;
> +
> + return PTR_ERR(pcs);
[Severity: Medium]
If num_pcs exceeds the actual number of entries in the device tree array,
fwnode_pcs_get() will return -ENOENT for the out-of-bounds index.
Will returning the error here cause the entire function to abort instead of
breaking gracefully and returning the valid entries found so far?
> + }
> +
> + available_pcs[found] = pcs;
> + found++;
> + }
> +
> + return found;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_phylink_pcs_parse);
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 087ac63f9193d..0734c98498a99 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -1867,6 +2093,33 @@ struct phylink *phylink_create(struct phylink_config *config,
[ ... ]
> + /* 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);
> + }
>
> pl->config = config;
[Severity: High]
If a PCS provider is registered concurrently right after the notifier is added,
pcs_provider_notify() could execute before pl->config is initialized here.
Will the callback attempt to dereference a NULL pl->config->dev and crash?
[Severity: High]
If an error occurs later in phylink_create() (for example, if
phylink_parse_mode(), phylink_parse_fixedlink(), or phylink_register_sfp()
fail), the function calls kfree(pl) and returns.
Does this error path leave the notifier registered to freed memory, leading
to a use-after-free regression when the next PCS provider is added?
> if (config->type == PHYLINK_NETDEV) {
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615122950.22281-1-ansuelsmth@gmail.com?part=6
^ permalink raw reply
* Re: [PATCH net-next v7 03/12] net: phylink: add phylink_release_pcs() to externally release a PCS
From: sashiko-bot @ 2026-06-16 14:32 UTC (permalink / raw)
To: Christian Marangi; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260615122950.22281-4-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Lockless modification of `pl->pcs_list` races with the `phylink_resolve` workqueue, leading to list corruption and use-after-free.
- [High] The active PCS is disabled prematurely before the MAC link is brought down, violating the standard teardown sequence.
- [Medium] `pl->supported` link modes are not updated when `pl->supported_interfaces` shrinks, leading to stale ethtool capabilities.
- [Medium] `pl->sfp_interfaces` is not updated when a PCS is released.
--
commit 0240edfdad0af399cf16b6105ab8d1a7a62254fb
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 cb07184ce82f3..ca4dad4b140a0 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -918,6 +919,58 @@ static void phylink_resolve_an_pause(struct phylink_link_state *state)
> }
> }
>
> +/**
> + * phylink_release_pcs - Removes a PCS from the phylink PCS available list
> + * @pcs: a pointer to the phylink_pcs struct to be released
> + *
> + * This function release a PCS from the phylink PCS available list if
> + * actually in use. It also refreshes the supported interfaces of the
> + * phylink instance by copying the supported interfaces from the phylink
> + * conf and merging the supported interfaces of the remaining available PCS
> + * in the list and trigger a resolve.
> + */
> +void phylink_release_pcs(struct phylink_pcs *pcs)
> +{
> + struct phylink *pl;
> +
> + ASSERT_RTNL();
> +
> + pl = pcs->phylink;
> + if (!pl)
> + return;
> +
> + list_del(&pcs->list);
> + pcs->phylink = NULL;
> +
> + mutex_lock(&pl->state_mutex);
[Severity: High]
Does this list_del() race with the phylink_resolve() workqueue?
Since phylink_resolve() does not hold rtnl_lock, it takes pl->state_mutex
and traverses pl->pcs_list in phylink_major_config().
If list_del() executes completely locklessly with respect to the
phylink_resolve() traversal, will the iterator follow poisoned list pointers,
resulting in a page fault and kernel panic?
Also, in phylink_destroy(), does clearing the list before canceling the resolve
workqueue without state_mutex open another identical race window?
> +
> + /* Check if we are removing the PCS currently
> + * in use by phylink. If this is the case,
> + * force phylink resolve to reconfigure the interface
> + * mode, disable the current PCS and set the
> + * phylink PCS to NULL.
> + */
> + if (pl->pcs == pcs) {
> + phylink_pcs_disable(pl->pcs);
[Severity: High]
Is it safe to disable the active PCS here before the MAC link is brought down?
At this point, the MAC link has not been brought down (i.e. netif_carrier_off
and MAC teardown haven't occurred) because phylink_link_down() is deferred to
run inside phylink_resolve().
Could disabling the PCS while the MAC is still actively transmitting/receiving
violate hardware sequencing and trigger bus stalls, dropped packets, or
hardware hangs?
> +
> + pl->force_major_config = true;
> + pl->pcs = NULL;
> + }
> +
> + 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: Medium]
Will ethtool advertise stale link modes since pl->supported is not updated?
This code correctly shrinks pl->supported_interfaces to exclude the released
PCS's capabilities, but fails to correspondingly re-evaluate the global
pl->supported link mode mask.
[Severity: Medium]
Should pl->sfp_interfaces also be updated when a PCS is released?
sfp_interfaces represents the intersection between the currently inserted
SFP module's capabilities and pl->supported_interfaces. Since
pl->supported_interfaces shrinks, not updating pl->sfp_interfaces might lead
phylink to incorrectly validate or attempt to configure an SFP link interface
mode that is now impossible to satisfy with the remaining hardware.
> +
> + phylink_run_resolve(pl);
> +}
> +EXPORT_SYMBOL_GPL(phylink_release_pcs);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615122950.22281-1-ansuelsmth@gmail.com?part=3
^ permalink raw reply
* Re: [PATCH net-next v7 02/12] net: phylink: introduce internal phylink PCS handling
From: sashiko-bot @ 2026-06-16 14:32 UTC (permalink / raw)
To: Christian Marangi; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260615122950.22281-3-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Dereferencing the uninitialized pointer `pl` in `phylink_create()` causes a kernel panic.
- [High] Error paths in `phylink_create()` fail to unlink PCS objects and unregister notifiers, leading to Use-After-Free and list corruption.
- [Medium] `phylink_validate_mac_and_pcs()` falsely reports an interface as supported when a required PCS is not found.
- [Low] Lockless data race on `pl->pcs` between process context and interrupt context.
--
commit 5bb4efcfcad2f214630e951f75ab343ec05780b5
Author: Christian Marangi <ansuelsmth@gmail.com>
Subject: net: phylink: introduce internal phylink PCS handling
This commit introduces internal handling of PCS for phylink, moving the
selection logic of the PCS entirely to phylink. MAC drivers can now provide
a callback to fill the available PCS into an array which is then parsed
into a linked list.
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 4d59c0dd78dbf..cb07184ce82f3 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -531,9 +553,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: Medium]
If the MAC explicitly requires a dedicated PCS for the requested interface
mode but the loop doesn't find a compatible one, pcs_found remains false.
Because the code then skips the pcs_found block and validates only the MAC
capabilities, it seems it could return 0 (success) instead of an error like
-EINVAL.
Does this unintentionally mark the interface as supported despite missing a
required hardware component?
[ ... ]
> @@ -1295,11 +1365,13 @@ static void phylink_major_config(struct phylink *pl, bool restart,
> if (pcs_changed) {
> phylink_pcs_disable(pl->pcs);
>
> - if (pl->pcs)
> - pl->pcs->phylink = NULL;
> + if (pl->mac_ops->mac_select_pcs) {
> + if (pl->pcs)
> + pl->pcs->phylink = NULL;
>
> - if (pcs)
> - pcs->phylink = pl;
> + if (pcs)
> + pcs->phylink = pl;
> + }
>
> pl->pcs = pcs;
[Severity: Low]
Is it possible for the compiler to optimize this assignment inappropriately?
Since phylink_pcs_change() reads this pointer locklessly from interrupt
context, should we use WRITE_ONCE() here to prevent a potential data race?
[ ... ]
> @@ -1865,6 +1976,16 @@ struct phylink *phylink_create(struct phylink_config *config,
> return ERR_PTR(-EINVAL);
> }
>
> + /*
> + * Make sure either PCS internal validation or .mac_select_pcs
> + * is used. Return error if both are defined.
> + */
> + if (config->num_possible_pcs && pl->mac_ops->mac_select_pcs) {
[Severity: Critical]
Does this dereference an uninitialized pointer? The local variable pl has not
been allocated yet. Should this use the function argument
mac_ops->mac_select_pcs instead of pl->mac_ops->mac_select_pcs?
> + dev_err(config->dev,
> + "phylink: error: either phylink_config .num_possible_pcs or .mac_select_pcs must be used\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> pl = kzalloc_obj(*pl);
> if (!pl)
> return ERR_PTR(-ENOMEM);
> @@ -1872,10 +1993,28 @@ struct phylink *phylink_create(struct phylink_config *config,
> mutex_init(&pl->phydev_mutex);
> mutex_init(&pl->state_mutex);
> INIT_WORK(&pl->resolve, phylink_resolve);
> + INIT_LIST_HEAD(&pl->pcs_list);
> +
> + /* Fill the PCS list with available PCS from phylink config */
> + ret = phylink_fill_available_pcs(pl, config);
> + if (ret < 0) {
> + kfree(pl);
> + return ERR_PTR(ret);
> + }
> +
> + /* Link available PCS to phylink */
> + list_for_each_entry(pcs, &pl->pcs_list, list)
> + pcs->phylink = pl;
>
> phy_interface_copy(pl->supported_interfaces,
> config->supported_interfaces);
>
> + /* Update supported interfaces */
> + list_for_each_entry(pcs, &pl->pcs_list, list)
> + phy_interface_or(pl->supported_interfaces,
> + pl->supported_interfaces,
> + pcs->supported_interfaces);
> +
> pl->config = config;
> if (config->type == PHYLINK_NETDEV) {
> pl->netdev = to_net_dev(config->dev);
[Severity: High]
If the configuration validation fails later in phylink_create() (for example,
if config->type is invalid or phylink_parse_mode() fails), the error paths
call kfree(pl) and return.
Does this leave the previously populated PCS objects with dangling phylink
pointers and corrupted list nodes? If a MAC driver defers probe and retries,
could this result in list corruption or a use-after-free when accessing the
dangling pcs->phylink pointer?
[ ... ]
> @@ -2413,8 +2562,15 @@ void phylink_pcs_change(struct phylink_pcs *pcs, bool up)
> {
> struct phylink *pl = pcs->phylink;
>
> - if (pl)
> - phylink_link_changed(pl, up, "pcs");
> + /*
> + * Ignore PCS link state change if the PCS is not
> + * attached to a phylink instance or the phylink
> + * instance is not currently using this PCS.
> + */
> + if (!pl || pl->pcs != pcs)
> + return;
[Severity: Low]
Since pl->pcs is updated in process context without WRITE_ONCE(), and we read
it locklessly here in interrupt context, should this use READ_ONCE(pl->pcs) to
avoid a data race? Even if it's benign, the compiler might perform unexpected
optimizations.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615122950.22281-1-ansuelsmth@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH net-next v7 11/12] net: pcs: airoha: add PCS driver for Airoha AN7581 SoC
From: sashiko-bot @ 2026-06-16 14:32 UTC (permalink / raw)
To: Christian Marangi; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260615122950.22281-12-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] Off-by-one error and lack of signedness check in `airoha_pcs_get` allows out-of-bounds array access.
- [High] Unconditional dereference of `match_data` during probe can cause a kernel panic if the driver is bound without a device tree node.
- [High] Switch statements for link speed leave critical register configuration variables completely uninitialized for unexpected speeds.
- [High] The acquired generic PHY for USB is never initialized or powered on.
- [High] Per-port interface mode is stored in the shared `priv->interface` global state, causing race conditions in multi-port devices.
- [High] Incorrect TX constant used for RX mode configuration, which breaks SGMII and 1000BASE-X on PCIe.
- [High] Per-port PCS operations modify shared device-level hardware state without synchronization, disrupting active ports.
- [Medium] Multiple state-reading functions fail to check the return value of `regmap_read()`, resulting in the use of uninitialized stack variables.
- [Low] The signal detection loop executes one more iteration than intended, potentially causing false positives.
- [Low] Assignments are separated by commas instead of semicolons, creating an unintended massive expression statement.
- [Low] The result of `airoha_pcs_setup_scu`, which performs critical reset assertions and deassertions, is silently ignored by `airoha_pcs_pre_config`.
--
commit 7fccf4b912ca13e1f4f141393b6432c311a781e1
Author: Christian Marangi <ansuelsmth@gmail.com>
net: pcs: airoha: add PCS driver for Airoha AN7581 SoC
Add PCS driver for Airoha AN7581 SoC for Ethernet/PON/PCIe/USB SERDES
and permit usage of external PHY or connected SFP cage. Supported modes
are 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..5d3b136ed2b59
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/pcs-airoha-common.c
> @@ -0,0 +1,1318 @@
[ ... ]
> +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 the global bulk reset lines for the entire block regardless
of which port is being configured?
Since the hardware supports up to two ports for PCIe, bringing up the second
port will assert the shared MAC and PHY block reset lines, which could disrupt
the already active port.
> + 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_get_state_usxgmii(struct airoha_pcs_priv *priv, int index,
> + struct phylink_link_state *state)
> +{
> + const struct airoha_pcs_match_data *data = priv->data;
> + struct airoha_pcs_maps *maps = &priv->maps[index];
> + u32 an_done, lpa;
> +
> + /* Trigger HW workaround if needed. If an error is reported,
> + * consider link down and test again later.
> + */
> + if (data->rxlock_workaround && data->rxlock_workaround(priv, index)) {
> + state->link = false;
> + return;
> + }
> +
> + /* Toggle AN Status */
> + regmap_set_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_AN_CONTROL_6,
> + AIROHA_PCS_USXGMII_TOG_PCS_AUTONEG_STS);
> + regmap_clear_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_AN_CONTROL_6,
> + AIROHA_PCS_USXGMII_TOG_PCS_AUTONEG_STS);
> +
> + regmap_read(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_AN_STATS_0, &lpa);
> + regmap_read(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_AN_STATS_2, &an_done);
> +
> + state->link = !!(lpa & MDIO_USXGMII_LINK);
[Severity: Medium]
Can regmap_read() fail and leave lpa and an_done completely uninitialized?
The return values of the read operations are not checked. If a read fails, the
uninitialized stack memory in these variables is then used in bitwise
operations to report the link state, leading to unpredictable results.
> + state->an_complete = !!(an_done & AIROHA_PCS_USXGMII_PCS_AN_COMPLETE);
> +
> + phylink_decode_usxgmii_word(state, 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)
> +{
> + struct airoha_pcs_port *port = to_airoha_pcs_port(pcs);
> + struct airoha_pcs_priv *priv = port->priv;
> + const struct airoha_pcs_match_data *data;
> + struct airoha_pcs_maps *maps;
> + int index = port->index;
> + u32 rate_adapt;
> + int ret;
> +
> + maps = &priv->maps[port->index];
> + priv->interface = interface;
[Severity: High]
Does storing the interface mode in the shared priv structure cause race
conditions or state corruption for multi-port devices?
Since PCIe configuration uses two ports, configuring the second port will
overwrite the mode for the first port. When airoha_pcs_an_restart() is later
called, it will read the wrong interface mode. Should this state be stored
per-port in the airoha_pcs_port structure instead?
> + data = priv->data;
[ ... ]
> +static void airoha_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
> + phy_interface_t interface, int speed, int duplex)
> +{
> + struct airoha_pcs_port *port = to_airoha_pcs_port(pcs);
> + struct airoha_pcs_priv *priv = port->priv;
> + const struct airoha_pcs_match_data *data;
> + struct airoha_pcs_maps *maps;
> +
> + maps = &priv->maps[port->index];
> + data = priv->data;
> +
> + if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
[ ... ]
> + } else {
> + if (interface == PHY_INTERFACE_MODE_USXGMII ||
> + interface == PHY_INTERFACE_MODE_10GBASER) {
> + u32 mode;
> + u32 rate_adapt;
> +
> + switch (speed) {
> + case SPEED_10000:
> + rate_adapt = AIROHA_PCS_HSGMII_RATE_ADPT_FORCE_RATE_ADAPT_MODE_10000;
> + mode = AIROHA_PCS_USXGMII_MODE_10000;
> + break;
> + case SPEED_5000:
> + rate_adapt = AIROHA_PCS_HSGMII_RATE_ADPT_FORCE_RATE_ADAPT_MODE_5000;
> + mode = AIROHA_PCS_USXGMII_MODE_5000;
> + break;
> + case SPEED_2500:
> + rate_adapt = AIROHA_PCS_HSGMII_RATE_ADPT_FORCE_RATE_ADAPT_MODE_2500;
> + mode = AIROHA_PCS_USXGMII_MODE_2500;
> + break;
> + case SPEED_1000:
> + rate_adapt = AIROHA_PCS_HSGMII_RATE_ADPT_FORCE_RATE_ADAPT_MODE_1000;
> + mode = AIROHA_PCS_USXGMII_MODE_1000;
> + break;
> + case SPEED_100:
> + rate_adapt = AIROHA_PCS_HSGMII_RATE_ADPT_FORCE_RATE_ADAPT_MODE_100;
> + mode = AIROHA_PCS_USXGMII_MODE_100;
> + break;
> + }
> +
> + /* Force USXGMII to selected speed */
> + regmap_update_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_AN_CONTROL_7,
> + AIROHA_PCS_USXGMII_MODE, mode);
[Severity: High]
Can an unhandled speed leave these local variables completely uninitialized?
The switch statements for speed lack a default case. For example, if SPEED_10
is passed in USXGMII mode, variables like mode and rate_adapt are never set.
The code then proceeds to write these uninitialized stack variables into
hardware control registers via regmap_update_bits().
> +
> + if (interface == PHY_INTERFACE_MODE_10GBASER)
> + regmap_update_bits(maps->hsgmii_rate_adp, AIROHA_PCS_HSGMII_RATE_ADAPT_CTRL_11,
[ ... ]
> +static void airoha_pcs_pre_config(struct phylink_pcs *pcs,
> + phy_interface_t interface)
> +{
> + struct airoha_pcs_port *port = to_airoha_pcs_port(pcs);
> + struct airoha_pcs_priv *priv = port->priv;
> + struct airoha_pcs_maps *maps;
> +
> + maps = &priv->maps[port->index];
> +
> + /* Select HSGMII or USXGMII in SCU regs */
> + airoha_pcs_setup_scu(priv, port->index, interface);
[Severity: Low]
Is it safe to ignore the return value of airoha_pcs_setup_scu() here?
That function performs important reset assertions and deassertions. If
reset_control_bulk_deassert() fails, the hardware stays in reset, but this
function will silently proceed to perform register writes against a reset
block.
> +
> + /* MPI MBI disable */
> + regmap_set_bits(maps->pcs_mac, AIROHA_PCS_XFI_MAC_XFI_GIB_CFG,
[ ... ]
> +static int airoha_pcs_init_named_regmap(struct platform_device *pdev,
> + const char *name, struct regmap **regmap)
> +{
> + struct regmap_config regmap_config = { };
> + void *base;
> +
> + base = devm_platform_ioremap_resource_byname(pdev, name);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + regmap_config.name = name;
> + regmap_config.reg_bits = 32,
> + regmap_config.val_bits = 32,
> + regmap_config.reg_stride = 4,
[Severity: Low]
Are these comma operators intentional?
The regmap_config fields are being initialized using commas instead of
semicolons. This creates a single expression rather than distinct statements,
which might confuse human readers and static analysis tools.
> +
> + *regmap = devm_regmap_init_mmio(&pdev->dev, base, ®map_config);
> +
> + return PTR_ERR_OR_ZERO(*regmap);
> +}
[ ... ]
> +static struct phylink_pcs *airoha_pcs_get(struct fwnode_reference_args *pcsspec,
> + void *data)
> +{
> + struct airoha_pcs_priv *priv = data;
> + struct device *dev = priv->dev;
> + int index = 0;
> +
> + switch (priv->data->port_type) {
> + case AIROHA_PCS_ETH:
> + case AIROHA_PCS_PON:
> + case AIROHA_PCS_USB:
> + if (pcsspec->nargs) {
> + dev_err(dev, "invalid number of cells in 'pcs-handle' property\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + break;
> + case AIROHA_PCS_PCIE:
> + if (pcsspec->nargs != 1) {
> + dev_err(dev, "invalid number of cells in 'pcs-handle' property\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + break;
> + }
> +
> + if (pcsspec->nargs)
> + index = pcsspec->args[0];
> +
> + if (index > priv->data->num_port) {
> + dev_err(dev, "invalid index cell in 'pcs-handle' property\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + return &priv->ports[index].pcs;
[Severity: High]
Does this check allow an out-of-bounds array access?
The priv->ports array is sized by data->num_port. The check evaluates to false
when index equals data->num_port, allowing access one element past the end of
the array.
Also, since index is a signed integer initialized from a u64, could a very
large unsigned value truncate to a negative number and bypass this check
entirely?
> +}
> +
> +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,
[Severity: High]
Can of_device_get_match_data() return NULL here?
If the driver is bound manually via sysfs instead of through a device tree
node, this will return NULL. The code then unconditionally dereferences
data->num_port, which would cause a kernel panic. Should this include a NULL
check?
> + 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]
Does this code acquire the PHY without ever initializing or powering it on?
The driver calls devm_phy_get() but there are no corresponding calls to
phy_init() or phy_power_on(). Without these, the PHY hardware might remain
unclocked or powered down, which could cause bus hangs when phy_set_mode_ext()
is called later in an7581_pcs_usb_bringup().
> + }
> +
> + 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..638e20cac97ea
> --- /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];
[Severity: High]
Is it intentional to hardcode priv->pcs_pma[0] here even when configuring
port 1?
This re-initializes the shared PLL whenever any port is brought up. For a
dual-port PCIe configuration, initializing the second port will disrupt the
shared PLL and cause clock loss for the first port.
> +
> + /* 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);
[ ... ]
> +static void an7581_pcs_rx_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 = priv->pcs_pma[index];
> + u32 phyck_div, phyck_sel;
> + u32 pr_cdr_beta_dac;
> + u32 cdr_pr_buf_in_sr;
> + bool cdr_pr_cap_en;
> + u32 sigdet_vth_sel;
> + u32 rx_rate_ctrl;
> + u32 xfi_rx_mode;
> + u32 osr;
> +
> + switch (interface) {
> + case PHY_INTERFACE_MODE_SGMII:
> + case PHY_INTERFACE_MODE_1000BASEX:
> + osr = BIT(1) | BIT(0); /* 1.25G */
> + pr_cdr_beta_dac = BIT(3);
> + rx_rate_ctrl = 0;
> + cdr_pr_cap_en = false;
> + cdr_pr_buf_in_sr = BIT(2) | BIT(1) | BIT(0);
> + sigdet_vth_sel = BIT(2) | BIT(1);
> + phyck_div = BIT(5) | BIT(3) | BIT(0);
> + phyck_sel = BIT(0);
> + xfi_rx_mode = AIROHA_PCS_PMA_XFI_TX_MODE_1G25;
[Severity: High]
Is this using a TX mode constant for an RX configuration?
AIROHA_PCS_PMA_XFI_TX_MODE_1G25 has a different bit layout (shifted to bit 3)
compared to the RX mode mask which operates on bits 11 to 9.
When this is applied, the TX bits fall outside the RX mask and get filtered
out. This will write 0 to the RX mode field, improperly configuring it to 10G3
instead of 1G25.
> + break;
> + case PHY_INTERFACE_MODE_2500BASEX:
[ ... ]
> +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 5 times for RX sigdet module to detect RX signal */
> + for (i = 0; i <= 5; i++) {
[Severity: Low]
Does this loop execute six times instead of the five times mentioned in the
comment?
The loop uses i <= 5, which iterates for 0 through 5. Since the logic looks
for count >= 4 to determine a valid signal, checking six times lowers the
threshold percentage and might return true on marginal or false signals.
> + regmap_read(pcs_pma, AIROHA_PCS_PMA_DIG_RO_RESERVE_2,
> + &val);
> + if (val & AIROHA_PCS_RX_SIGDET)
> + count++;
> + }
> +
> + /* Consider signal presence if we detect signal at least 4 times */
> + return count >= 4;
> +}
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615122950.22281-1-ansuelsmth@gmail.com?part=11
^ permalink raw reply
* Re: [PATCH net-next v7 12/12] net: airoha: add phylink support
From: sashiko-bot @ 2026-06-16 14:32 UTC (permalink / raw)
To: Christian Marangi; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260615122950.22281-13-ansuelsmth@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] NULL pointer dereference in `phylink_destroy()` on probe error path.
- [High] Data race on shared GDM4 fragment size registers causes configuration corruption.
--
commit a159dc684c14d93e9de96a620bb343b8bb77e730
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 5f1a118875fbe..9a42fb991bd72 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
[ ... ]
> @@ -3168,6 +3180,151 @@ bool airoha_is_valid_gdm_dev(struct airoha_eth *eth,
> +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;
[ ... ]
> + /* Configure TX/RX frag based on speed */
> + if (dev->nbq == 1) {
> + mask = GDMA4_SGMII1_TX_FRAG_SIZE_MASK;
> + val = FIELD_PREP(GDMA4_SGMII1_TX_FRAG_SIZE_MASK,
> + frag_size_tx);
> + } else {
> + mask = GDMA4_SGMII0_TX_FRAG_SIZE_MASK;
> + val = FIELD_PREP(GDMA4_SGMII0_TX_FRAG_SIZE_MASK,
> + frag_size_tx);
> + }
> + airoha_fe_rmw(eth, REG_GDMA4_TMBI_FRAG, mask, val);
[Severity: High]
Does this lockless read-modify-write on shared GDM4 registers cause data
corruption?
Because the SGMII lanes of the GDM4 port operate as separate phylink instances,
their link states are resolved independently.
If both lanes undergo a link state change simultaneously, phylink's workqueues
can execute airoha_mac_link_up() concurrently on different CPUs.
Without a lock around airoha_fe_rmw(), these concurrent updates to
REG_GDMA4_TMBI_FRAG and REG_GDMA4_RMBI_FRAG could overwrite and corrupt each
other's settings.
> +
> + if (dev->nbq == 1) {
> + mask = GDMA4_SGMII1_RX_FRAG_SIZE_MASK;
> + val = FIELD_PREP(GDMA4_SGMII1_RX_FRAG_SIZE_MASK,
> + frag_size_rx);
> + } else {
> + mask = GDMA4_SGMII0_RX_FRAG_SIZE_MASK;
> + val = FIELD_PREP(GDMA4_SGMII0_RX_FRAG_SIZE_MASK,
> + frag_size_rx);
> + }
> + airoha_fe_rmw(eth, REG_GDMA4_RMBI_FRAG, mask, val);
> +}
[ ... ]
> @@ -3231,7 +3388,7 @@ static int airoha_alloc_gdm_device(struct airoha_eth *eth,
> dev->nbq = nbq;
> port->devs[index] = dev;
>
> - return 0;
> + return airoha_setup_phylink(netdev);
> }
[ ... ]
> @@ -3457,6 +3614,7 @@ static int airoha_probe(struct platform_device *pdev)
> netdev = netdev_from_priv(dev);
> if (netdev->reg_state == NETREG_REGISTERED)
> unregister_netdev(netdev);
> + phylink_destroy(dev->phylink);
[Severity: High]
Could this cause a NULL pointer dereference during a probe failure?
In airoha_alloc_gdm_device(), the port->devs[index] = dev assignment happens
before calling airoha_setup_phylink(). If airoha_setup_phylink() fails,
dev->phylink remains NULL.
The error propagates back up to airoha_probe(), which then unconditionally
calls phylink_destroy() on all populated port->devs slots.
Because phylink_destroy() dereferences its argument without checking if it is
NULL, this will result in a kernel panic.
> of_node_put(netdev->dev.of_node);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615122950.22281-1-ansuelsmth@gmail.com?part=12
^ permalink raw reply
* Re: [PATCH 3/4] input: misc: Add Qualcomm SPMI PMIC haptics driver
From: Julian Braha @ 2026-06-16 14:37 UTC (permalink / raw)
To: Fenglin Wu, linux-arm-msm, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Lee Jones, Stephen Boyd,
Bjorn Andersson, Konrad Dybcio
Cc: David Collins, Subbaraman Narayanamurthy, Kamal Wadhwa, kernel,
linux-input, devicetree, linux-kernel
In-Reply-To: <20260616-qcom-spmi-haptics-v1-3-d24e422de6b4@oss.qualcomm.com>
Hi Fenglin,
On 6/16/26 11:08, Fenglin Wu wrote:
> +config INPUT_QCOM_SPMI_HAPTICS
> + tristate "Qualcomm SPMI PMIC haptics support"
> + depends on INPUT && MFD_SPMI_PMIC
The dependency on INPUT is unnecessary, all config options in this
Kconfig file already depend on INPUT due to an 'if INPUT..endif' in
drivers/input/Kconfig
(Yes, a few of the other config options in this file also have this
duplicate dependency on INPUT due to an explicit 'depends on'
attribute, but this file is in need of a cleanup.)
- Julian Braha
^ permalink raw reply
* Re: [PATCH v3 4/4] ARM: dts: mediatek: mt6323: add AUXADC support
From: sashiko-bot @ 2026-06-16 14:38 UTC (permalink / raw)
To: Roman Vivchar via B4 Relay; +Cc: devicetree, robh, conor+dt
In-Reply-To: <20260616-mt6323-adc-v3-4-1c27c588185d@protonmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Unconditional dereference of `dev->parent->parent` can lead to a kernel panic.
- [High] The driver actively powers down the AUXADC clock and disables the 1.8V voltage reference during initialization.
--
commit 0ad66b352744a9242754a09a7f9af3042d3da009
Author: Roman Vivchar <rva333@protonmail.com>
ARM: dts: mediatek: mt6323: add AUXADC support
Add the devicetree node for the mt6323 AUXADC.
> diff --git a/arch/arm/boot/dts/mediatek/mt6323.dtsi b/arch/arm/boot/dts/mediatek/mt6323.dtsi
> index c230c865116d0..c070f4b0936c2 100644
> --- a/arch/arm/boot/dts/mediatek/mt6323.dtsi
> +++ b/arch/arm/boot/dts/mediatek/mt6323.dtsi
> @@ -14,6 +14,11 @@ pmic: mt6323 {
> interrupt-controller;
> #interrupt-cells = <2>;
>
> + mt6323_adc: adc {
> + compatible = "mediatek,mt6323-auxadc";
> + #io-channel-cells = <1>;
> + };
[Severity: High]
This isn't a bug introduced by this patch, but adding this devicetree node
exposes a pre-existing issue in the mt6323-auxadc driver. Can the driver
panic if it evaluates dev->parent->parent as NULL?
In drivers/iio/adc/mt6323-auxadc.c:mt6323_auxadc_probe():
regmap = dev_get_regmap(dev->parent->parent, NULL);
If the device is instantiated directly without a parent (such as via a
standalone devicetree node or manual sysfs binding), platform_device_add()
sets dev->parent to &platform_bus. Because platform_bus.parent is NULL,
passing NULL to dev_get_regmap() will cause an immediate kernel panic when
devres_find() attempts to acquire the devres_lock spinlock.
[Severity: High]
This is also a pre-existing issue, but does the driver actively power down
the AUXADC clock and disable the 1.8V voltage reference during initialization?
In drivers/iio/adc/mt6323-auxadc.c:mt6323_auxadc_init():
ret = regmap_set_bits(map, MT6323_TOP_CKPDN2, AUXADC_TOP_CKPDN2_CTL_CK);
And later in the same function:
ret = regmap_set_bits(map, MT6323_AUXADC_CON27,
AUXADC_CON27_VREF18_ENB_MD |
AUXADC_CON27_MD_STATUS);
In MediaTek PMIC terminology, CKPDN stands for Clock Power Down (setting the
bit to 1 actively powers down the clock), and ENB stands for Enable Bar
(an active-low enable signal, where setting the bit to 1 disables the
reference voltage). By setting these bits to 1 instead of clearing them to 0,
the driver appears to disable the hardware it is attempting to initialize,
which could cause subsequent ADC read operations to time out and fail.
> mt6323_leds: leds {
> compatible = "mediatek,mt6323-led";
> #address-cells = <1>;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-mt6323-adc-v3-0-1c27c588185d@protonmail.com?part=4
^ permalink raw reply
* Re: [PATCH 1/4] ASoC: qcom: audioreach: compute active channel maps from channel_map
From: Srinivas Kandagatla @ 2026-06-16 14:59 UTC (permalink / raw)
To: Neil Armstrong, Srinivas Kandagatla, Liam Girdwood, Mark Brown,
Jaroslav Kysela, Takashi Iwai, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: kancy2333, linux-sound, linux-arm-msm, linux-kernel, devicetree
In-Reply-To: <16d2e8e8-91bc-437d-8225-eb6eedb4bd8a@linaro.org>
On 6/15/26 2:54 PM, Neil Armstrong wrote:
> On 6/15/26 11:36, Srinivas Kandagatla wrote:
>> On 6/15/26 10:31 AM, Neil Armstrong wrote:
>>> On 6/15/26 10:38, Srinivas Kandagatla wrote:
>>>>
>>>>
>>>> On 6/10/26 8:41 AM, Neil Armstrong wrote:
>>>>> The Qualcom SM8650 based Ayaneo Pocket S2 gaming device has a set
>>>>> of 2 WSA speakers connected on the WSA2 lines.
>>>>>
>>>>> But the Audioreach DSP only handles WSA2 in pair with the WSA
>>>>> interface by using the upper bits of the active_channels_mask
>>>>> for WSA2 and the lower bits for WSA:
>>>>>
>>>>> /-------------------------------------------------\
>>>>> | Bits | 3 | 2 | 1 | 0 |
>>>>> |-------------------------------------------------|
>>>>> | Line | WSA2 Ch2 | WSA2 Ch1 | WSA Ch2 | WSA Ch1 |
>>>>> \-------------------------------------------------/
>>>>>
>>>> No, this is not totally correct, if the setup only has WSA2, then
>>>> channel 0 and 1 should be WSA2 channels.
>>>>
>>>> What is the backend dai id that is in DT, it should be
>>>>
>>>> sound-dai = <&q6apmbedai WSA2_CODEC_DMA_RX_0>;
>>>>
>>>> I also noticed that you are using
>>>> https://github.com/linux-msm/audioreach-topology/blob/main/SM8550-
>>>> HDK.m4
>>>> which has WSA as backend dai, that is not correct, you should have
>>>> WSA2.
>>>
>>> So I did try that, and DSP would error out when using the
>>> LPAIF_INTF_TYPE_WSA2,
>>> but I'm retrying from scratch right now.
>>
>> Please share the failure logs, we need to change
>> 1. dt : bedai id, codec dais with correct soundwire wsa2 instance, the
>> routes.
>> 2. tplg
>>
>
> So I did all the changes as you suggested:
>
> Resurected Krzk's serie: https://
> patch.msgid.link/20231019153541.49753-1-krzysztof.kozlowski@linaro.org
>
> Adapted/Fixes it to apply on v7.1:
> https://gitlab.com/superna9999/linux/-/commit/
> fd8cf1922d10175c5bcd8cf2a444c5825392d994
> https://gitlab.com/superna9999/linux/-/
> commit/0c4e89e167b9ca9c7b500577c030e550ec2a6e73
> https://gitlab.com/superna9999/linux/-/
> commit/6364a0a45a3f0985b872d9f504e9ea1d1f3f2a35
>
> ```
> +#define WSA2_CODEC_DMA_RX_0 147
> +#define WSA2_CODEC_DMA_TX_0 148
> +#define WSA2_CODEC_DMA_RX_1 149
> +#define WSA2_CODEC_DMA_TX_1 150
> +#define WSA2_CODEC_DMA_TX_2 151
> ```
>
> https://gitlab.com/superna9999/linux/-/
> commit/9bd0ce21f73df92fb35e3db7ef570f561a106478
>
> DT:
> https://gitlab.com/superna9999/linux/-/
> commit/2fc270860e3b77ccae28e0c38228cba3e39ea78a
>
> ```
> - sound-dai = <&q6apmbedai
> WSA_CODEC_DMA_RX_0>;
> + sound-dai = <&q6apmbedai
> WSA2_CODEC_DMA_RX_0>;
> };
> ```
>
> Topology, copied the SM8550-HDK into a new one, dropped I2S and changed
> all WSA to WSA
> and added the WSA defines:
> https://github.com/superna9999/audioreach-topology/
> commit/12adc76859cde606c67e5a95df204b8d407038df
>
>
> ```
> +define(`WSA2_CODEC_DMA_RX_0', `147') dnl
> +define(`WSA2_CODEC_DMA_TX_0', `148') dnl
> +define(`WSA2_CODEC_DMA_RX_1', `149') dnl
> +define(`WSA2_CODEC_DMA_TX_1', `150') dnl
> +define(`WSA2_CODEC_DMA_TX_2', `151') dnl
> ```
>
> Extract of the SM8650-APS2.m4 concerning WSA2:
> ```
> ...
> +dnl WSA Playback
> +DEVICE_SG_ADD(audioreach/subgraph-device-codec-dma-playback.m4,
> `WSA2_CODEC_DMA_RX_0', WSA2_CODEC_DMA_RX_0,
> + `S16_LE', 48000, 48000, 2, 2,
> + LPAIF_INTF_TYPE_WSA2, CODEC_INTF_IDX_RX0, 0,
> DATA_FORMAT_FIXED_POINT,
> + 0x00004006, 0x00004006, 0x00006050)
> +dnl
> ...
> +STREAM_DEVICE_PLAYBACK_MIXER(WSA2_CODEC_DMA_RX_0,
> ``WSA2_CODEC_DMA_RX_0'', ``MultiMedia1'', ``MultiMedia2'', ``MultiMedia5'')
> ...
> +STREAM_DEVICE_PLAYBACK_ROUTE(WSA2_CODEC_DMA_RX_0, ``WSA2_CODEC_DMA_RX_0
> Audio Mixer'', ``MultiMedia1, stream0.logger1'', ``MultiMedia2,
> stream1.logger1'', ``MultiMedia5, stream4.logger1'')
> ...
> ```
>
> On device, all sets up without errors:
> ```
> [ 20.710228] qcom-apm gprsvc:service:2:1: CMD timeout for [1001021]
> opcode
> [ 20.720234] platform 6800000.remoteproc:glink-
> edge:gpr:service@1:dais: Adding to iommu group 30
> [ 20.763797] va_macro 6d44000.codec: qcom,dmic-sample-rate dt entry
> missing
> [ 20.791279] wsa_macro 6aa0000.codec: using zero-initialized flat
> cache, this may cause unexpected behavior
> [ 20.912445] wcd939x_codec audio-codec: bound sdw:2:0:0217:010e:00:4
> (ops wcd_sdw_component_ops [snd_soc_wcd_common])
> [ 20.923343] wcd939x_codec audio-codec: bound sdw:3:0:0217:010e:00:3
> (ops wcd_sdw_component_ops [snd_soc_wcd_common])
> [ 20.960741] snd-sc8280xp sound: ASoC: Parent card not yet available,
> widget card binding deferred
> [ 20.972182] va_macro 6d44000.codec: supply vdd-micb not found, using
> dummy regulator
> [ 20.985751] ALSA: Control name 'stream0.vol_ctrl0 MultiMedia1
> Playback Volume' truncated to 'stream0.vol_ctrl0 MultiMedia1 Playback Volu'
> [ 20.998589] ALSA: Control name 'stream1.vol_ctrl1 MultiMedia2
> Playback Volume' truncated to 'stream1.vol_ctrl1 MultiMedia2 Playback Volu'
> [ 21.011536] ALSA: Control name 'stream4.vol_ctrl4 MultiMedia5
> Playback Volume' truncated to 'stream4.vol_ctrl4 MultiMedia5 Playback Volu'
> [ 21.026510] input: SM8650-APS2 Headset Jack as /devices/platform/
> sound/sound/card0/input7
> [ 21.035151] input: SM8650-APS2 DP0 Jack as /devices/platform/sound/
> sound/card0/input8
> ```
>
> Available mixer elements:
> ```
> # amixer | grep WSA
> Simple mixer control 'SpkrLeft WSA MODE',0
> Simple mixer control 'SpkrRight WSA MODE',0
> Simple mixer control 'WSA RX0 MUX',0
> Simple mixer control 'WSA RX1 MUX',0
> Simple mixer control 'WSA RX_MIX EC0_MUX',0
> Simple mixer control 'WSA RX_MIX EC1_MUX',0
> Simple mixer control 'WSA RX_MIX0 MUX',0
> Simple mixer control 'WSA RX_MIX1 MUX',0
> Simple mixer control 'WSA2_CODEC_DMA_RX_0 Audio Mixer MultiMedia1',0
> Simple mixer control 'WSA2_CODEC_DMA_RX_0 Audio Mixer MultiMedia2',0
> Simple mixer control 'WSA2_CODEC_DMA_RX_0 Audio Mixer MultiMedia5',0
> Simple mixer control 'WSA_AIF_VI Mixer WSA_SPKR_VI_1',0
> Simple mixer control 'WSA_AIF_VI Mixer WSA_SPKR_VI_2',0
> Simple mixer control 'WSA_COMP1',0
> Simple mixer control 'WSA_COMP2',0
> Simple mixer control 'WSA_RX0 Digital',0
> Simple mixer control 'WSA_RX0 Digital Mute',0
> Simple mixer control 'WSA_RX0 EC_HQ',0
> Simple mixer control 'WSA_RX0 INP0',0
> Simple mixer control 'WSA_RX0 INP1',0
> Simple mixer control 'WSA_RX0 INP2',0
> Simple mixer control 'WSA_RX0 INT0 SIDETONE MIX',0
> Simple mixer control 'WSA_RX0 MIX INP',0
> Simple mixer control 'WSA_RX0_MIX Digital',0
> Simple mixer control 'WSA_RX0_MIX Digital Mute',0
> Simple mixer control 'WSA_RX1 Digital',0
> Simple mixer control 'WSA_RX1 Digital Mute',0
> Simple mixer control 'WSA_RX1 EC_HQ',0
> Simple mixer control 'WSA_RX1 INP0',0
> Simple mixer control 'WSA_RX1 INP1',0
> Simple mixer control 'WSA_RX1 INP2',0
> Simple mixer control 'WSA_RX1 MIX INP',0
> Simple mixer control 'WSA_RX1_MIX Digital',0
> Simple mixer control 'WSA_RX1_MIX Digital Mute',0
> Simple mixer control 'WSA_Softclip0 Enable',0
> Simple mixer control 'WSA_Softclip1 Enable',0
> ```
>
> I setup the speaker with (no errors):
> ```
> amixer -c 0 cset name='SpkrLeft PA Volume' 20
> amixer -c 0 cset name='SpkrRight PA Volume' 20
> amixer -c 0 cset name='WSA RX0 MUX' AIF1_PB
> amixer -c 0 cset name='WSA RX1 MUX' AIF1_PB
> amixer -c 0 cset name='WSA_RX0 INP0' RX0
> amixer -c 0 cset name='WSA_RX1 INP0' RX1
> amixer -c 0 cset name='SpkrLeft DAC Switch' 1
> amixer -c 0 cset name='SpkrRight DAC Switch' 1
> amixer -c 0 cset name='WSA_RX0 Digital Volume' 85
> amixer -c 0 cset name='WSA_RX1 Digital Volume' 85
> ```
>
> and finally:
> ```
> amixer -c 0 cset name='WSA2_CODEC_DMA_RX_0 Audio Mixer MultiMedia1' 1
> numid=216,iface=MIXER,name='WSA2_CODEC_DMA_RX_0 Audio Mixer MultiMedia1'
> ; type=BOOLEAN,access=rw------,values=2
> : values=on,off
>
> ```
>
> When playing sound, it just timeouts, no printed errors:
> ```
> # speaker-test -D plughw:0,0 -c 2
>
> speaker-test 1.2.14
>
> Playback device is plughw:0,0
> Stream parameters are 48000Hz, S16_LE, 2 channels
> Using 16 octaves of pink noise
> Rate set to 48000Hz (requested 48000Hz)
> Buffer size range from 960 to 130560
> Period size range from 480 to 16320
> Periods = 4
> was set period_size = 12000
> was set buffer_size = 48000
> 0 - Front Left
> Write error: -5,Input/output error
> xrun_recovery failed: -5,Input/output error
> Transfer failed: Input/output error
> ```
Thanks Neil for trying this out, I had pretty much simillar behavoir on
Glymur device too, My understanding was that there are some codec DMA
instances where we can use 2 channels as default for WSA2.
But on Glymur all the DMA instances are 8 channel wide. Am trying to
check internally to see if its actually doable to get this working with
the ch1 and ch2 mask.
Am working on this to get more details on what it takes to get this working.
Only concern with this patch is that the channel mapping, which does not
reflect the channel mask. can you confirm that in your working setup you
have chmask as 0xC0 and channel_map setup for ch1 and ch2?
From DSP docs:
https://github.com/AudioReach/audioreach-engine/blob/master/fwk/api/modules/media_fmt_api_basic.h#L780
Channel[i] mapping describes channel i. Each element i of the array
describes channel i inside the buffer where i is less than num_channels.
An unused channel is set to 0.
So unused channels should be set to zero, but the patch padding the
channels starting form zero.
-srini
>
> Neil
>
>>
>> --srini
>>>
>>> Thanks,
>>> Neil
>>>
>>>>
>>>>
>>>>> Setting only the WSA2 upper bits is perfectly valid and
>>>>> functional but the current Audioreach code builds the bitmask
>>>>> from the channels count with:
>>>>> active_channels_mask = (1 << num_channels) - 1;
>>>>>
>>>>> In order to enable the WSA2 bits the channel count should be 4,
>>>>> but the lower WSA bits are then also enabled and the DSP errors
>>>>> out when trying to play on the disabled WSA interface.
>>>>>
>>>>> A solution would've been to add a fake WSA2 topology element which
>>>>> would be translated into the top bits only, but it's not clean and
>>>>> add some special exceptions in the generic Audioreach code.
>>>>>
>>>>> The solution suggested by Srinivas is to use the channel mapping to
>>>>> set this bitmask.
>>>>>
>>>>> This works but makes all the other calls using the channel mapping
>>>>> fail
>>>>> because the DSP requires the channel_mapping table to start from
>>>>> index 0
>>>>> and using num_channel length in order to apply the mapping on the
>>>>> active_channels_mask bits in order.
>>>>>
>>>>> So we need to skip the empty channel mapping entries in all other
>>>>> users of the channel_map to build valid channel_mapping tables.
>>>>>
>>>>> This should not break any other usecases since the default channel
>>>>> mapping always start from index 0, and will add flexibilty to allow
>>>>> some special non linear mapping for other interfaces as well.
>>>>>
>>>>> Suggested-by: Srinivas Kandagatla
>>>>> <srinivas.kandagatla@oss.qualcomm.com>
>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>> ---
>>>>> sound/soc/qcom/qdsp6/audioreach.c | 47 ++++++++++++++++++++++++++++
>>>>> ++---------
>>>>> 1 file changed, 37 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/sound/soc/qcom/qdsp6/audioreach.c b/sound/soc/qcom/
>>>>> qdsp6/audioreach.c
>>>>> index a13f753eff98..9b80cfa56e8a 100644
>>>>> --- a/sound/soc/qcom/qdsp6/audioreach.c
>>>>> +++ b/sound/soc/qcom/qdsp6/audioreach.c
>>>>> @@ -703,6 +703,7 @@ static int
>>>>> audioreach_codec_dma_set_media_format(struct q6apm_graph *graph,
>>>>> int pm_sz = APM_HW_EP_PMODE_CFG_PSIZE;
>>>>> int size = ic_sz + ep_sz + fs_sz + pm_sz;
>>>>> void *p;
>>>>> + int i;
>>>>> struct gpr_pkt *pkt __free(kfree) =
>>>>> audioreach_alloc_apm_cmd_pkt(size, APM_CMD_SET_CFG, 0);
>>>>> if (IS_ERR(pkt))
>>>>> @@ -741,7 +742,12 @@ static int
>>>>> audioreach_codec_dma_set_media_format(struct q6apm_graph *graph,
>>>>> intf_cfg->cfg.lpaif_type = module->hw_interface_type;
>>>>> intf_cfg->cfg.intf_index = module->hw_interface_idx;
>>>>> - intf_cfg->cfg.active_channels_mask = (1 << cfg->num_channels)
>>>>> - 1;
>>>>> + intf_cfg->cfg.active_channels_mask = 0;
>>>>> + /* Convert the physical channel mapping into a bit field */
>>>>> + for (i = 0; i < AR_PCM_MAX_NUM_CHANNEL; i++)
>>>>> + if (cfg->channel_map[i])
>>>>> + intf_cfg->cfg.active_channels_mask |= BIT(i);
>>>>> +
>>>>
>>>> This one looks good, this should be a bug fix patch.
>>>>
>>>>> p += ic_sz;
>>>>> pm_cfg = p;
>>>>> @@ -840,7 +846,7 @@ static int audioreach_mfc_set_media_format(struct
>>>>> q6apm_graph *graph,
>>>>> uint32_t num_channels = cfg->num_channels;
>>>>> int payload_size = APM_MFC_CFG_PSIZE(media_format,
>>>>> num_channels) +
>>>>> APM_MODULE_PARAM_DATA_SIZE;
>>>>> - int i;
>>>>> + int i, j;
>>>>> void *p;
>>>>> struct gpr_pkt *pkt __free(kfree) =
>>>>> audioreach_alloc_apm_cmd_pkt(payload_size, APM_CMD_SET_CFG, 0);
>>>>> @@ -860,8 +866,12 @@ static int
>>>>> audioreach_mfc_set_media_format(struct q6apm_graph *graph,
>>>>> media_format->sample_rate = cfg->sample_rate;
>>>>> media_format->bit_width = cfg->bit_width;
>>>>> media_format->num_channels = cfg->num_channels;
>>>>> - for (i = 0; i < num_channels; i++)
>>>>> - media_format->channel_mapping[i] = cfg->channel_map[i];
>>>>> + /* Convert the physical mapping to a logical mapping of the
>>>>> channels */
>>>>> + for (i = 0, j = 0; i < AR_PCM_MAX_NUM_CHANNEL && j < cfg-
>>>>>> num_channels; i++) {
>>>>> + if (!cfg->channel_map[i])
>>>>> + continue;
>>>>> + media_format->channel_mapping[j++] = cfg->channel_map[i];
>>>> Each element i of the channel_mapping[i] array, describes the channel i
>>>> inside the buffer where i is less than num_channels. An unused channel
>>>> is set to 0.
>>>>
>>>> For some reason I get impression that user is trying to set a 4
>>>> channels
>>>> instead of 2 channel.
>>>>
>>>> Can you fix the backend-dai id and play it directly on WSA2 instead of
>>>> WSA.
>>>> Or was there a reason for not doing it otherwise?
>>>>
>>>> --srini
>>>>
>>>>> + }
>>>>> return q6apm_send_cmd_sync(graph->apm, pkt, 0);
>>>>> }
>>>>> @@ -1080,6 +1090,7 @@ static int
>>>>> audioreach_pcm_set_media_format(struct q6apm_graph *graph,
>>>>> struct apm_pcm_module_media_fmt_cmd *cfg;
>>>>> struct apm_module_param_data *param_data;
>>>>> int payload_size;
>>>>> + int i, j;
>>>>> if (num_channels > 4) {
>>>>> dev_err(graph->dev, "Error: Invalid channels (%d)!\n",
>>>>> num_channels);
>>>>> @@ -1113,7 +1124,12 @@ static int
>>>>> audioreach_pcm_set_media_format(struct q6apm_graph *graph,
>>>>> media_cfg->num_channels = mcfg->num_channels;
>>>>> media_cfg->q_factor = mcfg->bit_width - 1;
>>>>> media_cfg->bits_per_sample = mcfg->bit_width;
>>>>> - memcpy(media_cfg->channel_mapping, mcfg->channel_map, mcfg-
>>>>>> num_channels);
>>>>> + /* Convert the physical mapping to a logical mapping of the
>>>>> channels */
>>>>> + for (i = 0, j = 0; i < AR_PCM_MAX_NUM_CHANNEL && j < mcfg-
>>>>>> num_channels; i++) {
>>>>> + if (!mcfg->channel_map[i])
>>>>> + continue;
>>>>> + media_cfg->channel_mapping[j++] = mcfg->channel_map[i];
>>>>> + }
>>>>> return q6apm_send_cmd_sync(graph->apm, pkt, 0);
>>>>> }
>>>>> @@ -1127,6 +1143,7 @@ static int
>>>>> audioreach_shmem_set_media_format(struct q6apm_graph *graph,
>>>>> struct payload_media_fmt_pcm *cfg;
>>>>> struct media_format *header;
>>>>> int rc, payload_size;
>>>>> + int i, j;
>>>>> void *p;
>>>>> if (num_channels > 4) {
>>>>> @@ -1166,7 +1183,12 @@ static int
>>>>> audioreach_shmem_set_media_format(struct q6apm_graph *graph,
>>>>> cfg->q_factor = mcfg->bit_width - 1;
>>>>> cfg->endianness = PCM_LITTLE_ENDIAN;
>>>>> cfg->num_channels = mcfg->num_channels;
>>>>> - memcpy(cfg->channel_mapping, mcfg->channel_map, mcfg-
>>>>>> num_channels);
>>>>> + /* Convert the physical mapping to a logical mapping of the
>>>>> channels */
>>>>> + for (i = 0, j = 0; i < AR_PCM_MAX_NUM_CHANNEL && j < cfg-
>>>>>> num_channels; i++) {
>>>>> + if (!mcfg->channel_map[i])
>>>>> + continue;
>>>>> + cfg->channel_mapping[j++] = mcfg->channel_map[i];
>>>>> + }
>>>>> } else {
>>>>> rc = audioreach_set_compr_media_format(header, p, mcfg);
>>>>> if (rc)
>>>>> @@ -1243,7 +1265,7 @@ static int
>>>>> audioreach_speaker_protection_vi(struct q6apm_graph *graph,
>>>>> struct apm_module_sp_vi_ex_mode_cfg *ex_cfg;
>>>>> int op_sz, cm_sz, ex_sz;
>>>>> struct apm_module_param_data *param_data;
>>>>> - int rc, i, payload_size;
>>>>> + int rc, i, payload_size, j;
>>>>> struct gpr_pkt *pkt;
>>>>> void *p;
>>>>> @@ -1284,14 +1306,19 @@ static int
>>>>> audioreach_speaker_protection_vi(struct q6apm_graph *graph,
>>>>> param_data->param_size = cm_sz - APM_MODULE_PARAM_DATA_SIZE;
>>>>> cm_cfg->cfg.num_channels = num_channels * 2;
>>>>> - for (i = 0; i < num_channels; i++) {
>>>>> + /* Convert the physical mapping to a logical mapping of the
>>>>> channels */
>>>>> + for (i = 0, j = 0; i < AR_PCM_MAX_NUM_CHANNEL && j <
>>>>> num_channels; i++) {
>>>>> + if (!mcfg->channel_map[i])
>>>>> + continue;
>>>>> /*
>>>>> * Map speakers into Vsense and then Isense of each
>>>>> channel.
>>>>> * E.g. for PCM_CHANNEL_FL and PCM_CHANNEL_FR to:
>>>>> * [1, 2, 3, 4]
>>>>> */
>>>>> - cm_cfg->cfg.channel_mapping[2 * i] = (mcfg->channel_map[i] -
>>>>> 1) * 2 + 1;
>>>>> - cm_cfg->cfg.channel_mapping[2 * i + 1] = (mcfg-
>>>>>> channel_map[i] - 1) * 2 + 2;
>>>>> + cm_cfg->cfg.channel_mapping[2 * j] = (mcfg->channel_map[i] -
>>>>> 1) * 2 + 1;
>>>>> + cm_cfg->cfg.channel_mapping[2 * j + 1] = (mcfg-
>>>>>> channel_map[i] - 1) * 2 + 2;
>>>>> +
>>>>> + ++j;
>>>>> }
>>>>> p += cm_sz;
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply
* Re: [PATCH 1/4] ASoC: qcom: audioreach: compute active channel maps from channel_map
From: Srinivas Kandagatla @ 2026-06-16 15:05 UTC (permalink / raw)
To: Neil Armstrong, Srinivas Kandagatla, Liam Girdwood, Mark Brown,
Jaroslav Kysela, Takashi Iwai, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Prasad Kumpatla
Cc: kancy2333, linux-sound, linux-arm-msm, linux-kernel, devicetree,
Srinivas Kandagatla
In-Reply-To: <20260610-topic-sm8650-ayaneo-pocket-s2-wsa2-fix-v1-1-18bb19c5ca22@linaro.org>
+add Prasad to comment on dsp side.
On 6/10/26 8:41 AM, Neil Armstrong wrote:
> The Qualcom SM8650 based Ayaneo Pocket S2 gaming device has a set
> of 2 WSA speakers connected on the WSA2 lines.
>
> But the Audioreach DSP only handles WSA2 in pair with the WSA
> interface by using the upper bits of the active_channels_mask
> for WSA2 and the lower bits for WSA:
>
> /-------------------------------------------------\
> | Bits | 3 | 2 | 1 | 0 |
> |-------------------------------------------------|
> | Line | WSA2 Ch2 | WSA2 Ch1 | WSA Ch2 | WSA Ch1 |
> \-------------------------------------------------/
>
> Setting only the WSA2 upper bits is perfectly valid and
> functional but the current Audioreach code builds the bitmask
> from the channels count with:
> active_channels_mask = (1 << num_channels) - 1;
>
> In order to enable the WSA2 bits the channel count should be 4,
> but the lower WSA bits are then also enabled and the DSP errors
> out when trying to play on the disabled WSA interface.
>
> A solution would've been to add a fake WSA2 topology element which
> would be translated into the top bits only, but it's not clean and
> add some special exceptions in the generic Audioreach code.
>
> The solution suggested by Srinivas is to use the channel mapping to
> set this bitmask.
>
> This works but makes all the other calls using the channel mapping fail
> because the DSP requires the channel_mapping table to start from index 0
> and using num_channel length in order to apply the mapping on the
> active_channels_mask bits in order.
>
> So we need to skip the empty channel mapping entries in all other
> users of the channel_map to build valid channel_mapping tables.
>
> This should not break any other usecases since the default channel
> mapping always start from index 0, and will add flexibilty to allow
> some special non linear mapping for other interfaces as well.
>
> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
> sound/soc/qcom/qdsp6/audioreach.c | 47 ++++++++++++++++++++++++++++++---------
> 1 file changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/sound/soc/qcom/qdsp6/audioreach.c b/sound/soc/qcom/qdsp6/audioreach.c
> index a13f753eff98..9b80cfa56e8a 100644
> --- a/sound/soc/qcom/qdsp6/audioreach.c
> +++ b/sound/soc/qcom/qdsp6/audioreach.c
> @@ -703,6 +703,7 @@ static int audioreach_codec_dma_set_media_format(struct q6apm_graph *graph,
> int pm_sz = APM_HW_EP_PMODE_CFG_PSIZE;
> int size = ic_sz + ep_sz + fs_sz + pm_sz;
> void *p;
> + int i;
>
> struct gpr_pkt *pkt __free(kfree) = audioreach_alloc_apm_cmd_pkt(size, APM_CMD_SET_CFG, 0);
> if (IS_ERR(pkt))
> @@ -741,7 +742,12 @@ static int audioreach_codec_dma_set_media_format(struct q6apm_graph *graph,
>
> intf_cfg->cfg.lpaif_type = module->hw_interface_type;
> intf_cfg->cfg.intf_index = module->hw_interface_idx;
> - intf_cfg->cfg.active_channels_mask = (1 << cfg->num_channels) - 1;
> + intf_cfg->cfg.active_channels_mask = 0;
> + /* Convert the physical channel mapping into a bit field */
> + for (i = 0; i < AR_PCM_MAX_NUM_CHANNEL; i++)
> + if (cfg->channel_map[i])
> + intf_cfg->cfg.active_channels_mask |= BIT(i);
> +
> p += ic_sz;
>
> pm_cfg = p;
> @@ -840,7 +846,7 @@ static int audioreach_mfc_set_media_format(struct q6apm_graph *graph,
> uint32_t num_channels = cfg->num_channels;
> int payload_size = APM_MFC_CFG_PSIZE(media_format, num_channels) +
> APM_MODULE_PARAM_DATA_SIZE;
> - int i;
> + int i, j;
> void *p;
>
> struct gpr_pkt *pkt __free(kfree) = audioreach_alloc_apm_cmd_pkt(payload_size, APM_CMD_SET_CFG, 0);
> @@ -860,8 +866,12 @@ static int audioreach_mfc_set_media_format(struct q6apm_graph *graph,
> media_format->sample_rate = cfg->sample_rate;
> media_format->bit_width = cfg->bit_width;
> media_format->num_channels = cfg->num_channels;
> - for (i = 0; i < num_channels; i++)
> - media_format->channel_mapping[i] = cfg->channel_map[i];
> + /* Convert the physical mapping to a logical mapping of the channels */
> + for (i = 0, j = 0; i < AR_PCM_MAX_NUM_CHANNEL && j < cfg->num_channels; i++) {
> + if (!cfg->channel_map[i])
> + continue;
> + media_format->channel_mapping[j++] = cfg->channel_map[i];
> + }
>
> return q6apm_send_cmd_sync(graph->apm, pkt, 0);
> }
> @@ -1080,6 +1090,7 @@ static int audioreach_pcm_set_media_format(struct q6apm_graph *graph,
> struct apm_pcm_module_media_fmt_cmd *cfg;
> struct apm_module_param_data *param_data;
> int payload_size;
> + int i, j;
>
> if (num_channels > 4) {
> dev_err(graph->dev, "Error: Invalid channels (%d)!\n", num_channels);
> @@ -1113,7 +1124,12 @@ static int audioreach_pcm_set_media_format(struct q6apm_graph *graph,
> media_cfg->num_channels = mcfg->num_channels;
> media_cfg->q_factor = mcfg->bit_width - 1;
> media_cfg->bits_per_sample = mcfg->bit_width;
> - memcpy(media_cfg->channel_mapping, mcfg->channel_map, mcfg->num_channels);
> + /* Convert the physical mapping to a logical mapping of the channels */
> + for (i = 0, j = 0; i < AR_PCM_MAX_NUM_CHANNEL && j < mcfg->num_channels; i++) {
> + if (!mcfg->channel_map[i])
> + continue;
> + media_cfg->channel_mapping[j++] = mcfg->channel_map[i];
> + }
>
> return q6apm_send_cmd_sync(graph->apm, pkt, 0);
> }
> @@ -1127,6 +1143,7 @@ static int audioreach_shmem_set_media_format(struct q6apm_graph *graph,
> struct payload_media_fmt_pcm *cfg;
> struct media_format *header;
> int rc, payload_size;
> + int i, j;
> void *p;
>
> if (num_channels > 4) {
> @@ -1166,7 +1183,12 @@ static int audioreach_shmem_set_media_format(struct q6apm_graph *graph,
> cfg->q_factor = mcfg->bit_width - 1;
> cfg->endianness = PCM_LITTLE_ENDIAN;
> cfg->num_channels = mcfg->num_channels;
> - memcpy(cfg->channel_mapping, mcfg->channel_map, mcfg->num_channels);
> + /* Convert the physical mapping to a logical mapping of the channels */
> + for (i = 0, j = 0; i < AR_PCM_MAX_NUM_CHANNEL && j < cfg->num_channels; i++) {
> + if (!mcfg->channel_map[i])
> + continue;
> + cfg->channel_mapping[j++] = mcfg->channel_map[i];
> + }
> } else {
> rc = audioreach_set_compr_media_format(header, p, mcfg);
> if (rc)
> @@ -1243,7 +1265,7 @@ static int audioreach_speaker_protection_vi(struct q6apm_graph *graph,
> struct apm_module_sp_vi_ex_mode_cfg *ex_cfg;
> int op_sz, cm_sz, ex_sz;
> struct apm_module_param_data *param_data;
> - int rc, i, payload_size;
> + int rc, i, payload_size, j;
> struct gpr_pkt *pkt;
> void *p;
>
> @@ -1284,14 +1306,19 @@ static int audioreach_speaker_protection_vi(struct q6apm_graph *graph,
> param_data->param_size = cm_sz - APM_MODULE_PARAM_DATA_SIZE;
>
> cm_cfg->cfg.num_channels = num_channels * 2;
> - for (i = 0; i < num_channels; i++) {
> + /* Convert the physical mapping to a logical mapping of the channels */
> + for (i = 0, j = 0; i < AR_PCM_MAX_NUM_CHANNEL && j < num_channels; i++) {
> + if (!mcfg->channel_map[i])
> + continue;
> /*
> * Map speakers into Vsense and then Isense of each channel.
> * E.g. for PCM_CHANNEL_FL and PCM_CHANNEL_FR to:
> * [1, 2, 3, 4]
> */
> - cm_cfg->cfg.channel_mapping[2 * i] = (mcfg->channel_map[i] - 1) * 2 + 1;
> - cm_cfg->cfg.channel_mapping[2 * i + 1] = (mcfg->channel_map[i] - 1) * 2 + 2;
> + cm_cfg->cfg.channel_mapping[2 * j] = (mcfg->channel_map[i] - 1) * 2 + 1;
> + cm_cfg->cfg.channel_mapping[2 * j + 1] = (mcfg->channel_map[i] - 1) * 2 + 2;
> +
> + ++j;
> }
>
> p += cm_sz;
>
^ permalink raw reply
* Re: [PATCH v3 2/2] drm/tiny: add support for PIXPAPER 4.26 monochrome e-ink panel
From: Devarsh Thakkar @ 2026-06-16 15:08 UTC (permalink / raw)
To: LiangCheng Wang, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Wig Cheng
Cc: dri-devel, devicetree, linux-kernel, Tomi Valkeinen
In-Reply-To: <20260616083921.4066690-1-zaq14760@gmail.com>
Hi LiangCheng,
On 16/06/26 14:09, LiangCheng Wang wrote:
> Hi Devarsh,
>
> Thanks for the detailed review.
>
>> 1) Could you please share the datasheet for the display controller used
>> inside this pixpaper version ?
>
> Unfortunately I'm not able to share the controller datasheet publicly; it
> was provided to us under NDA.
>
>> 2) Does 0xFF or 0xF7 mode work as well for your display or is it strictly
>> 0xF4 which seems to mean that analog and osc bits are disabled ?
>
> I'll test whether the standard 0xF7/0xFF sequences also work on this panel
> and follow up with the result.
>
>> 3) Also could you confirm which display controller IC does the PIXPAPER
>> 4.26 use ?
>
> The PIXPAPER 4.26 uses the Solomon SSD1677, so you're right that it is an
> SSD16xx-family controller -- the commands it uses (0x01, 0x0C, 0x18, 0x20,
> 0x22, 0x24, 0x3C, 0x44/0x45, 0x4E/0x4F) match the standard SSD16xx set.
>
>> it would be appropriate to add this panel as a new display panel entry in
>> panel-ssd16xx.c rather than a separate driver to avoid code duplication.
>
> I agree that consolidating SSD16xx panels under panel-ssd16xx.c is the right
> long-term direction, and I'd be glad to converge there.
>
> I'm not yet sure how SSD1677 would fit with the controllers your series
> currently targets (SSD1673/SSD1680/SSD1681/SSD1683)
Thanks for sharing this information, yes it makes sense now that it is
using ssd1677.I have the SSD1677 datasheet and I think there should be
minimal change in the driver to support this controller, I will be
adding that in v2.
-- ours is a larger
> 4.26" 800x480 panel, so I suspect it may need a new controller variant.
> Would you expect panel-ssd16xx.c to be able to support SSD1677?
>
Yes, I will be adding SSD1677 controller support in V2 of my series,
hopefully that should help and after that it's just a matter of adding
panel entry for your pixpaper panel. I can share you my branch with
ssd1677 support once I have it ready.
Also, I don't have PIXPAPER 4.26 panel but if you want I can share you
my branch having ssd1677 support and additionally I can quickly add
boilerplate pixpaper 4.26 panel entries on top of my V2 series referring
from your patch so that it switches to using standard ssd16xx commands
and ssd1677 quirks wherever necessary and you can then validate and modify.
> In the meantime, would it be reasonable to take this smaller standalone
> driver, and migrate the panel into panel-ssd16xx.c once that driver lands
> with SSD1677 support? I'd be happy to help with the migration, and of course
> I'll defer to your and the maintainers' preference here.
>
I think it makes more sense to use already posted unified ssd16xx driver
which already supports standard controller flow for the ssd16xx family
with additional controller/panel specific quirks and has interface to
support different panels thus avoiding massive code duplication and
leveraging already developed functionalities.
Regards
Devarsh
> Regards,
> LiangCheng
^ permalink raw reply
* Re: [PATCH 1/4] ASoC: qcom: audioreach: compute active channel maps from channel_map
From: Neil Armstrong @ 2026-06-16 15:12 UTC (permalink / raw)
To: Srinivas Kandagatla, Liam Girdwood, Mark Brown, Jaroslav Kysela,
Takashi Iwai, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: kancy2333, linux-sound, linux-arm-msm, linux-kernel, devicetree
In-Reply-To: <f18fa22c-131c-409c-9c8b-3153e2272b89@kernel.org>
On 6/16/26 16:59, Srinivas Kandagatla wrote:
>
>
> On 6/15/26 2:54 PM, Neil Armstrong wrote:
>> On 6/15/26 11:36, Srinivas Kandagatla wrote:
>>> On 6/15/26 10:31 AM, Neil Armstrong wrote:
>>>> On 6/15/26 10:38, Srinivas Kandagatla wrote:
>>>>>
>>>>>
>>>>> On 6/10/26 8:41 AM, Neil Armstrong wrote:
>>>>>> The Qualcom SM8650 based Ayaneo Pocket S2 gaming device has a set
>>>>>> of 2 WSA speakers connected on the WSA2 lines.
>>>>>>
>>>>>> But the Audioreach DSP only handles WSA2 in pair with the WSA
>>>>>> interface by using the upper bits of the active_channels_mask
>>>>>> for WSA2 and the lower bits for WSA:
>>>>>>
>>>>>> /-------------------------------------------------\
>>>>>> | Bits | 3 | 2 | 1 | 0 |
>>>>>> |-------------------------------------------------|
>>>>>> | Line | WSA2 Ch2 | WSA2 Ch1 | WSA Ch2 | WSA Ch1 |
>>>>>> \-------------------------------------------------/
>>>>>>
>>>>> No, this is not totally correct, if the setup only has WSA2, then
>>>>> channel 0 and 1 should be WSA2 channels.
>>>>>
>>>>> What is the backend dai id that is in DT, it should be
>>>>>
>>>>> sound-dai = <&q6apmbedai WSA2_CODEC_DMA_RX_0>;
>>>>>
>>>>> I also noticed that you are using
>>>>> https://github.com/linux-msm/audioreach-topology/blob/main/SM8550-
>>>>> HDK.m4
>>>>> which has WSA as backend dai, that is not correct, you should have
>>>>> WSA2.
>>>>
>>>> So I did try that, and DSP would error out when using the
>>>> LPAIF_INTF_TYPE_WSA2,
>>>> but I'm retrying from scratch right now.
>>>
>>> Please share the failure logs, we need to change
>>> 1. dt : bedai id, codec dais with correct soundwire wsa2 instance, the
>>> routes.
>>> 2. tplg
>>>
>>
>> So I did all the changes as you suggested:
>>
>> Resurected Krzk's serie: https://
>> patch.msgid.link/20231019153541.49753-1-krzysztof.kozlowski@linaro.org
>>
>> Adapted/Fixes it to apply on v7.1:
>> https://gitlab.com/superna9999/linux/-/commit/
>> fd8cf1922d10175c5bcd8cf2a444c5825392d994
>> https://gitlab.com/superna9999/linux/-/
>> commit/0c4e89e167b9ca9c7b500577c030e550ec2a6e73
>> https://gitlab.com/superna9999/linux/-/
>> commit/6364a0a45a3f0985b872d9f504e9ea1d1f3f2a35
>>
>> ```
>> +#define WSA2_CODEC_DMA_RX_0 147
>> +#define WSA2_CODEC_DMA_TX_0 148
>> +#define WSA2_CODEC_DMA_RX_1 149
>> +#define WSA2_CODEC_DMA_TX_1 150
>> +#define WSA2_CODEC_DMA_TX_2 151
>> ```
>>
>> https://gitlab.com/superna9999/linux/-/
>> commit/9bd0ce21f73df92fb35e3db7ef570f561a106478
>>
>> DT:
>> https://gitlab.com/superna9999/linux/-/
>> commit/2fc270860e3b77ccae28e0c38228cba3e39ea78a
>>
>> ```
>> - sound-dai = <&q6apmbedai
>> WSA_CODEC_DMA_RX_0>;
>> + sound-dai = <&q6apmbedai
>> WSA2_CODEC_DMA_RX_0>;
>> };
>> ```
>>
>> Topology, copied the SM8550-HDK into a new one, dropped I2S and changed
>> all WSA to WSA
>> and added the WSA defines:
>> https://github.com/superna9999/audioreach-topology/
>> commit/12adc76859cde606c67e5a95df204b8d407038df
>>
>>
>> ```
>> +define(`WSA2_CODEC_DMA_RX_0', `147') dnl
>> +define(`WSA2_CODEC_DMA_TX_0', `148') dnl
>> +define(`WSA2_CODEC_DMA_RX_1', `149') dnl
>> +define(`WSA2_CODEC_DMA_TX_1', `150') dnl
>> +define(`WSA2_CODEC_DMA_TX_2', `151') dnl
>> ```
>>
>> Extract of the SM8650-APS2.m4 concerning WSA2:
>> ```
>> ...
>> +dnl WSA Playback
>> +DEVICE_SG_ADD(audioreach/subgraph-device-codec-dma-playback.m4,
>> `WSA2_CODEC_DMA_RX_0', WSA2_CODEC_DMA_RX_0,
>> + `S16_LE', 48000, 48000, 2, 2,
>> + LPAIF_INTF_TYPE_WSA2, CODEC_INTF_IDX_RX0, 0,
>> DATA_FORMAT_FIXED_POINT,
>> + 0x00004006, 0x00004006, 0x00006050)
>> +dnl
>> ...
>> +STREAM_DEVICE_PLAYBACK_MIXER(WSA2_CODEC_DMA_RX_0,
>> ``WSA2_CODEC_DMA_RX_0'', ``MultiMedia1'', ``MultiMedia2'', ``MultiMedia5'')
>> ...
>> +STREAM_DEVICE_PLAYBACK_ROUTE(WSA2_CODEC_DMA_RX_0, ``WSA2_CODEC_DMA_RX_0
>> Audio Mixer'', ``MultiMedia1, stream0.logger1'', ``MultiMedia2,
>> stream1.logger1'', ``MultiMedia5, stream4.logger1'')
>> ...
>> ```
>>
>> On device, all sets up without errors:
>> ```
>> [ 20.710228] qcom-apm gprsvc:service:2:1: CMD timeout for [1001021]
>> opcode
>> [ 20.720234] platform 6800000.remoteproc:glink-
>> edge:gpr:service@1:dais: Adding to iommu group 30
>> [ 20.763797] va_macro 6d44000.codec: qcom,dmic-sample-rate dt entry
>> missing
>> [ 20.791279] wsa_macro 6aa0000.codec: using zero-initialized flat
>> cache, this may cause unexpected behavior
>> [ 20.912445] wcd939x_codec audio-codec: bound sdw:2:0:0217:010e:00:4
>> (ops wcd_sdw_component_ops [snd_soc_wcd_common])
>> [ 20.923343] wcd939x_codec audio-codec: bound sdw:3:0:0217:010e:00:3
>> (ops wcd_sdw_component_ops [snd_soc_wcd_common])
>> [ 20.960741] snd-sc8280xp sound: ASoC: Parent card not yet available,
>> widget card binding deferred
>> [ 20.972182] va_macro 6d44000.codec: supply vdd-micb not found, using
>> dummy regulator
>> [ 20.985751] ALSA: Control name 'stream0.vol_ctrl0 MultiMedia1
>> Playback Volume' truncated to 'stream0.vol_ctrl0 MultiMedia1 Playback Volu'
>> [ 20.998589] ALSA: Control name 'stream1.vol_ctrl1 MultiMedia2
>> Playback Volume' truncated to 'stream1.vol_ctrl1 MultiMedia2 Playback Volu'
>> [ 21.011536] ALSA: Control name 'stream4.vol_ctrl4 MultiMedia5
>> Playback Volume' truncated to 'stream4.vol_ctrl4 MultiMedia5 Playback Volu'
>> [ 21.026510] input: SM8650-APS2 Headset Jack as /devices/platform/
>> sound/sound/card0/input7
>> [ 21.035151] input: SM8650-APS2 DP0 Jack as /devices/platform/sound/
>> sound/card0/input8
>> ```
>>
>> Available mixer elements:
>> ```
>> # amixer | grep WSA
>> Simple mixer control 'SpkrLeft WSA MODE',0
>> Simple mixer control 'SpkrRight WSA MODE',0
>> Simple mixer control 'WSA RX0 MUX',0
>> Simple mixer control 'WSA RX1 MUX',0
>> Simple mixer control 'WSA RX_MIX EC0_MUX',0
>> Simple mixer control 'WSA RX_MIX EC1_MUX',0
>> Simple mixer control 'WSA RX_MIX0 MUX',0
>> Simple mixer control 'WSA RX_MIX1 MUX',0
>> Simple mixer control 'WSA2_CODEC_DMA_RX_0 Audio Mixer MultiMedia1',0
>> Simple mixer control 'WSA2_CODEC_DMA_RX_0 Audio Mixer MultiMedia2',0
>> Simple mixer control 'WSA2_CODEC_DMA_RX_0 Audio Mixer MultiMedia5',0
>> Simple mixer control 'WSA_AIF_VI Mixer WSA_SPKR_VI_1',0
>> Simple mixer control 'WSA_AIF_VI Mixer WSA_SPKR_VI_2',0
>> Simple mixer control 'WSA_COMP1',0
>> Simple mixer control 'WSA_COMP2',0
>> Simple mixer control 'WSA_RX0 Digital',0
>> Simple mixer control 'WSA_RX0 Digital Mute',0
>> Simple mixer control 'WSA_RX0 EC_HQ',0
>> Simple mixer control 'WSA_RX0 INP0',0
>> Simple mixer control 'WSA_RX0 INP1',0
>> Simple mixer control 'WSA_RX0 INP2',0
>> Simple mixer control 'WSA_RX0 INT0 SIDETONE MIX',0
>> Simple mixer control 'WSA_RX0 MIX INP',0
>> Simple mixer control 'WSA_RX0_MIX Digital',0
>> Simple mixer control 'WSA_RX0_MIX Digital Mute',0
>> Simple mixer control 'WSA_RX1 Digital',0
>> Simple mixer control 'WSA_RX1 Digital Mute',0
>> Simple mixer control 'WSA_RX1 EC_HQ',0
>> Simple mixer control 'WSA_RX1 INP0',0
>> Simple mixer control 'WSA_RX1 INP1',0
>> Simple mixer control 'WSA_RX1 INP2',0
>> Simple mixer control 'WSA_RX1 MIX INP',0
>> Simple mixer control 'WSA_RX1_MIX Digital',0
>> Simple mixer control 'WSA_RX1_MIX Digital Mute',0
>> Simple mixer control 'WSA_Softclip0 Enable',0
>> Simple mixer control 'WSA_Softclip1 Enable',0
>> ```
>>
>> I setup the speaker with (no errors):
>> ```
>> amixer -c 0 cset name='SpkrLeft PA Volume' 20
>> amixer -c 0 cset name='SpkrRight PA Volume' 20
>> amixer -c 0 cset name='WSA RX0 MUX' AIF1_PB
>> amixer -c 0 cset name='WSA RX1 MUX' AIF1_PB
>> amixer -c 0 cset name='WSA_RX0 INP0' RX0
>> amixer -c 0 cset name='WSA_RX1 INP0' RX1
>> amixer -c 0 cset name='SpkrLeft DAC Switch' 1
>> amixer -c 0 cset name='SpkrRight DAC Switch' 1
>> amixer -c 0 cset name='WSA_RX0 Digital Volume' 85
>> amixer -c 0 cset name='WSA_RX1 Digital Volume' 85
>> ```
>>
>> and finally:
>> ```
>> amixer -c 0 cset name='WSA2_CODEC_DMA_RX_0 Audio Mixer MultiMedia1' 1
>> numid=216,iface=MIXER,name='WSA2_CODEC_DMA_RX_0 Audio Mixer MultiMedia1'
>> ; type=BOOLEAN,access=rw------,values=2
>> : values=on,off
>>
>> ```
>>
>> When playing sound, it just timeouts, no printed errors:
>> ```
>> # speaker-test -D plughw:0,0 -c 2
>>
>> speaker-test 1.2.14
>>
>> Playback device is plughw:0,0
>> Stream parameters are 48000Hz, S16_LE, 2 channels
>> Using 16 octaves of pink noise
>> Rate set to 48000Hz (requested 48000Hz)
>> Buffer size range from 960 to 130560
>> Period size range from 480 to 16320
>> Periods = 4
>> was set period_size = 12000
>> was set buffer_size = 48000
>> 0 - Front Left
>> Write error: -5,Input/output error
>> xrun_recovery failed: -5,Input/output error
>> Transfer failed: Input/output error
>> ```
>
> Thanks Neil for trying this out, I had pretty much simillar behavoir on
> Glymur device too, My understanding was that there are some codec DMA
> instances where we can use 2 channels as default for WSA2.
>
> But on Glymur all the DMA instances are 8 channel wide. Am trying to
> check internally to see if its actually doable to get this working with
> the ch1 and ch2 mask.
>
> Am working on this to get more details on what it takes to get this working.
>
>
> Only concern with this patch is that the channel mapping, which does not
> reflect the channel mask. can you confirm that in your working setup you
> have chmask as 0xC0 and channel_map setup for ch1 and ch2?
Yeah 0xC0 for active_channels_mask and channel_mapping = { PCM_CHANNEL_FL, PCM_CHANNEL_FR };
I tried to keep the leading 0, but since the channel_mapping table is allocated with
the size of the num_channels parameter, you can ony have 2 entries.
>
> From DSP docs:
> https://github.com/AudioReach/audioreach-engine/blob/master/fwk/api/modules/media_fmt_api_basic.h#L780
>
> Channel[i] mapping describes channel i. Each element i of the array
> describes channel i inside the buffer where i is less than num_channels.
> An unused channel is set to 0.
>
>
> So unused channels should be set to zero, but the patch padding the
> channels starting form zero.
My understanding in my trial and error is that the DSP will map each
entry of the channel_mapping to the active_channels_mask bits in order
so you can have a non linear active_channels_mask like b10101010 which
has 4 channels active and pass a channel_mapping table with 4 entries.
The is what I implemented here, and it gives a lot of flexibility on how
to connect speakers to the interface.
Neil
>
> -srini
>
>
>>
>> Neil
>>
>>>
>>> --srini
>>>>
>>>> Thanks,
>>>> Neil
>>>>
>>>>>
>>>>>
>>>>>> Setting only the WSA2 upper bits is perfectly valid and
>>>>>> functional but the current Audioreach code builds the bitmask
>>>>>> from the channels count with:
>>>>>> active_channels_mask = (1 << num_channels) - 1;
>>>>>>
>>>>>> In order to enable the WSA2 bits the channel count should be 4,
>>>>>> but the lower WSA bits are then also enabled and the DSP errors
>>>>>> out when trying to play on the disabled WSA interface.
>>>>>>
>>>>>> A solution would've been to add a fake WSA2 topology element which
>>>>>> would be translated into the top bits only, but it's not clean and
>>>>>> add some special exceptions in the generic Audioreach code.
>>>>>>
>>>>>> The solution suggested by Srinivas is to use the channel mapping to
>>>>>> set this bitmask.
>>>>>>
>>>>>> This works but makes all the other calls using the channel mapping
>>>>>> fail
>>>>>> because the DSP requires the channel_mapping table to start from
>>>>>> index 0
>>>>>> and using num_channel length in order to apply the mapping on the
>>>>>> active_channels_mask bits in order.
>>>>>>
>>>>>> So we need to skip the empty channel mapping entries in all other
>>>>>> users of the channel_map to build valid channel_mapping tables.
>>>>>>
>>>>>> This should not break any other usecases since the default channel
>>>>>> mapping always start from index 0, and will add flexibilty to allow
>>>>>> some special non linear mapping for other interfaces as well.
>>>>>>
>>>>>> Suggested-by: Srinivas Kandagatla
>>>>>> <srinivas.kandagatla@oss.qualcomm.com>
>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>>> ---
>>>>>> sound/soc/qcom/qdsp6/audioreach.c | 47 ++++++++++++++++++++++++++++
>>>>>> ++---------
>>>>>> 1 file changed, 37 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> diff --git a/sound/soc/qcom/qdsp6/audioreach.c b/sound/soc/qcom/
>>>>>> qdsp6/audioreach.c
>>>>>> index a13f753eff98..9b80cfa56e8a 100644
>>>>>> --- a/sound/soc/qcom/qdsp6/audioreach.c
>>>>>> +++ b/sound/soc/qcom/qdsp6/audioreach.c
>>>>>> @@ -703,6 +703,7 @@ static int
>>>>>> audioreach_codec_dma_set_media_format(struct q6apm_graph *graph,
>>>>>> int pm_sz = APM_HW_EP_PMODE_CFG_PSIZE;
>>>>>> int size = ic_sz + ep_sz + fs_sz + pm_sz;
>>>>>> void *p;
>>>>>> + int i;
>>>>>> struct gpr_pkt *pkt __free(kfree) =
>>>>>> audioreach_alloc_apm_cmd_pkt(size, APM_CMD_SET_CFG, 0);
>>>>>> if (IS_ERR(pkt))
>>>>>> @@ -741,7 +742,12 @@ static int
>>>>>> audioreach_codec_dma_set_media_format(struct q6apm_graph *graph,
>>>>>> intf_cfg->cfg.lpaif_type = module->hw_interface_type;
>>>>>> intf_cfg->cfg.intf_index = module->hw_interface_idx;
>>>>>> - intf_cfg->cfg.active_channels_mask = (1 << cfg->num_channels)
>>>>>> - 1;
>>>>>> + intf_cfg->cfg.active_channels_mask = 0;
>>>>>> + /* Convert the physical channel mapping into a bit field */
>>>>>> + for (i = 0; i < AR_PCM_MAX_NUM_CHANNEL; i++)
>>>>>> + if (cfg->channel_map[i])
>>>>>> + intf_cfg->cfg.active_channels_mask |= BIT(i);
>>>>>> +
>>>>>
>>>>> This one looks good, this should be a bug fix patch.
>>>>>
>>>>>> p += ic_sz;
>>>>>> pm_cfg = p;
>>>>>> @@ -840,7 +846,7 @@ static int audioreach_mfc_set_media_format(struct
>>>>>> q6apm_graph *graph,
>>>>>> uint32_t num_channels = cfg->num_channels;
>>>>>> int payload_size = APM_MFC_CFG_PSIZE(media_format,
>>>>>> num_channels) +
>>>>>> APM_MODULE_PARAM_DATA_SIZE;
>>>>>> - int i;
>>>>>> + int i, j;
>>>>>> void *p;
>>>>>> struct gpr_pkt *pkt __free(kfree) =
>>>>>> audioreach_alloc_apm_cmd_pkt(payload_size, APM_CMD_SET_CFG, 0);
>>>>>> @@ -860,8 +866,12 @@ static int
>>>>>> audioreach_mfc_set_media_format(struct q6apm_graph *graph,
>>>>>> media_format->sample_rate = cfg->sample_rate;
>>>>>> media_format->bit_width = cfg->bit_width;
>>>>>> media_format->num_channels = cfg->num_channels;
>>>>>> - for (i = 0; i < num_channels; i++)
>>>>>> - media_format->channel_mapping[i] = cfg->channel_map[i];
>>>>>> + /* Convert the physical mapping to a logical mapping of the
>>>>>> channels */
>>>>>> + for (i = 0, j = 0; i < AR_PCM_MAX_NUM_CHANNEL && j < cfg-
>>>>>>> num_channels; i++) {
>>>>>> + if (!cfg->channel_map[i])
>>>>>> + continue;
>>>>>> + media_format->channel_mapping[j++] = cfg->channel_map[i];
>>>>> Each element i of the channel_mapping[i] array, describes the channel i
>>>>> inside the buffer where i is less than num_channels. An unused channel
>>>>> is set to 0.
>>>>>
>>>>> For some reason I get impression that user is trying to set a 4
>>>>> channels
>>>>> instead of 2 channel.
>>>>>
>>>>> Can you fix the backend-dai id and play it directly on WSA2 instead of
>>>>> WSA.
>>>>> Or was there a reason for not doing it otherwise?
>>>>>
>>>>> --srini
>>>>>
>>>>>> + }
>>>>>> return q6apm_send_cmd_sync(graph->apm, pkt, 0);
>>>>>> }
>>>>>> @@ -1080,6 +1090,7 @@ static int
>>>>>> audioreach_pcm_set_media_format(struct q6apm_graph *graph,
>>>>>> struct apm_pcm_module_media_fmt_cmd *cfg;
>>>>>> struct apm_module_param_data *param_data;
>>>>>> int payload_size;
>>>>>> + int i, j;
>>>>>> if (num_channels > 4) {
>>>>>> dev_err(graph->dev, "Error: Invalid channels (%d)!\n",
>>>>>> num_channels);
>>>>>> @@ -1113,7 +1124,12 @@ static int
>>>>>> audioreach_pcm_set_media_format(struct q6apm_graph *graph,
>>>>>> media_cfg->num_channels = mcfg->num_channels;
>>>>>> media_cfg->q_factor = mcfg->bit_width - 1;
>>>>>> media_cfg->bits_per_sample = mcfg->bit_width;
>>>>>> - memcpy(media_cfg->channel_mapping, mcfg->channel_map, mcfg-
>>>>>>> num_channels);
>>>>>> + /* Convert the physical mapping to a logical mapping of the
>>>>>> channels */
>>>>>> + for (i = 0, j = 0; i < AR_PCM_MAX_NUM_CHANNEL && j < mcfg-
>>>>>>> num_channels; i++) {
>>>>>> + if (!mcfg->channel_map[i])
>>>>>> + continue;
>>>>>> + media_cfg->channel_mapping[j++] = mcfg->channel_map[i];
>>>>>> + }
>>>>>> return q6apm_send_cmd_sync(graph->apm, pkt, 0);
>>>>>> }
>>>>>> @@ -1127,6 +1143,7 @@ static int
>>>>>> audioreach_shmem_set_media_format(struct q6apm_graph *graph,
>>>>>> struct payload_media_fmt_pcm *cfg;
>>>>>> struct media_format *header;
>>>>>> int rc, payload_size;
>>>>>> + int i, j;
>>>>>> void *p;
>>>>>> if (num_channels > 4) {
>>>>>> @@ -1166,7 +1183,12 @@ static int
>>>>>> audioreach_shmem_set_media_format(struct q6apm_graph *graph,
>>>>>> cfg->q_factor = mcfg->bit_width - 1;
>>>>>> cfg->endianness = PCM_LITTLE_ENDIAN;
>>>>>> cfg->num_channels = mcfg->num_channels;
>>>>>> - memcpy(cfg->channel_mapping, mcfg->channel_map, mcfg-
>>>>>>> num_channels);
>>>>>> + /* Convert the physical mapping to a logical mapping of the
>>>>>> channels */
>>>>>> + for (i = 0, j = 0; i < AR_PCM_MAX_NUM_CHANNEL && j < cfg-
>>>>>>> num_channels; i++) {
>>>>>> + if (!mcfg->channel_map[i])
>>>>>> + continue;
>>>>>> + cfg->channel_mapping[j++] = mcfg->channel_map[i];
>>>>>> + }
>>>>>> } else {
>>>>>> rc = audioreach_set_compr_media_format(header, p, mcfg);
>>>>>> if (rc)
>>>>>> @@ -1243,7 +1265,7 @@ static int
>>>>>> audioreach_speaker_protection_vi(struct q6apm_graph *graph,
>>>>>> struct apm_module_sp_vi_ex_mode_cfg *ex_cfg;
>>>>>> int op_sz, cm_sz, ex_sz;
>>>>>> struct apm_module_param_data *param_data;
>>>>>> - int rc, i, payload_size;
>>>>>> + int rc, i, payload_size, j;
>>>>>> struct gpr_pkt *pkt;
>>>>>> void *p;
>>>>>> @@ -1284,14 +1306,19 @@ static int
>>>>>> audioreach_speaker_protection_vi(struct q6apm_graph *graph,
>>>>>> param_data->param_size = cm_sz - APM_MODULE_PARAM_DATA_SIZE;
>>>>>> cm_cfg->cfg.num_channels = num_channels * 2;
>>>>>> - for (i = 0; i < num_channels; i++) {
>>>>>> + /* Convert the physical mapping to a logical mapping of the
>>>>>> channels */
>>>>>> + for (i = 0, j = 0; i < AR_PCM_MAX_NUM_CHANNEL && j <
>>>>>> num_channels; i++) {
>>>>>> + if (!mcfg->channel_map[i])
>>>>>> + continue;
>>>>>> /*
>>>>>> * Map speakers into Vsense and then Isense of each
>>>>>> channel.
>>>>>> * E.g. for PCM_CHANNEL_FL and PCM_CHANNEL_FR to:
>>>>>> * [1, 2, 3, 4]
>>>>>> */
>>>>>> - cm_cfg->cfg.channel_mapping[2 * i] = (mcfg->channel_map[i] -
>>>>>> 1) * 2 + 1;
>>>>>> - cm_cfg->cfg.channel_mapping[2 * i + 1] = (mcfg-
>>>>>>> channel_map[i] - 1) * 2 + 2;
>>>>>> + cm_cfg->cfg.channel_mapping[2 * j] = (mcfg->channel_map[i] -
>>>>>> 1) * 2 + 1;
>>>>>> + cm_cfg->cfg.channel_mapping[2 * j + 1] = (mcfg-
>>>>>>> channel_map[i] - 1) * 2 + 2;
>>>>>> +
>>>>>> + ++j;
>>>>>> }
>>>>>> p += cm_sz;
>>>>>>
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply
* Re: [PATCH 6/9] nvmem: imx-ocotp-ele: Support the ELE API
From: Frieder Schrempf @ 2026-06-16 15:13 UTC (permalink / raw)
To: sashiko-reviews, Frieder Schrempf
Cc: robh, devicetree, Frank.Li, conor+dt, imx
In-Reply-To: <20260616120436.5908D1F00A3A@smtp.kernel.org>
On 16.06.26 14:04, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The dynamically allocated read buffer `p` is leaked when `imx_se_read_fuse()` returns an error in `imx_ocotp_reg_read()`.
> - [High] Lack of unbind protection for the supplier Secure Enclave (`se_dev`) device leads to a potential Use-After-Free of its cached private data.
> --
>
> commit ce01a1e44e5619e1342b66aca8b1bf267d01e93e
> Author: Frieder Schrempf <frieder.schrempf@kontron.de>
>
> nvmem: imx-ocotp-ele: Support the ELE API
>
> The fuses inside the Edgelock Secure Enclave are currently not accessed via
> its API but through the FSB block which provides limited access to some
> fuses. The ELE API allows us to access all fuses with read/write
> permissions.
>
>> diff --git a/drivers/nvmem/imx-ocotp-ele.c b/drivers/nvmem/imx-ocotp-ele.c
>> index 9d3f94e35508f..92cc061b9b347 100644
>> --- a/drivers/nvmem/imx-ocotp-ele.c
>> +++ b/drivers/nvmem/imx-ocotp-ele.c
>
> [ ... ]
>
>> @@ -92,6 +97,19 @@ static int imx_ocotp_reg_read(void *context, unsigned int offset, void *val, siz
>> buf = p;
>>
>> for (i = index; i < (index + count); i++) {
>> + /*
>> + * All fuse registers can be read via ELE. If the SE device is
>> + * available, always prefer it.
>> + */
>> + if (priv->se_data) {
>> + ret = imx_se_read_fuse(priv->se_data, i, buf++);
>> + if (ret) {
>> + mutex_unlock(&priv->lock);
>> + return ret;
>
> [Severity: High]
> Does this error path leak the memory allocated for p earlier in the function?
>
> The allocation happens here:
>
> p = kzalloc(num_bytes, GFP_KERNEL);
> if (!p)
> return -ENOMEM;
>
> Returning directly here will bypass the kfree(p) at the end of the function.
Will be fixed using __free(kfree) in v2.
>
>> + }
>> + continue;
>> + }
>> +
>
> [ ... ]
>
>> @@ -152,16 +203,36 @@ static int imx_ele_ocotp_probe(struct platform_device *pdev)
>> if (IS_ERR(priv->base))
>> return PTR_ERR(priv->base);
>>
>> + np = of_parse_phandle(pdev->dev.of_node, "secure-enclave", 0);
>> + if (!np) {
>> + dev_info(&pdev->dev, "missing or invalid SE handle, using readonly FSB\n");
>> + } else {
>> + priv->se_dev = of_find_device_by_node(np);
>> + of_node_put(np);
>> + if (!priv->se_dev)
>> + return dev_err_probe(&pdev->dev, -ENODEV, "failed to find SE device\n");
>> +
>> + ret = devm_add_action_or_reset(&pdev->dev, imx_ocotp_put_se_dev,
>> + priv->se_dev);
>> + if (ret)
>> + return ret;
>> +
>> + priv->se_data = platform_get_drvdata(priv->se_dev);
>
> [Severity: High]
> Is there anything preventing the SE driver from being unbound via sysfs after we
> cache its private data in priv->se_data here?
>
> If the SE driver is unbound, its devres allocations (which likely include the
> se_data) would be freed. Since we haven't established a device link
> (e.g. using device_link_add) to enforce unbind ordering, we might be left
> with a dangling pointer in priv->se_data.
>
> Could this result in a use-after-free when imx_ocotp_reg_read() or
> imx_ocotp_reg_write() later attempts to dereference priv->se_data?
Will be fixed by creating a device link in v2.
>
>> + if (!priv->se_data)
>> + return dev_err_probe(&pdev->dev, -EPROBE_DEFER,
>> + "SE device not ready\n");
>> + }
>>
>
^ permalink raw reply
* Re: [PATCH 0/4] iio: adc: new ti-ads112c14 driver
From: David Lechner @ 2026-06-16 15:21 UTC (permalink / raw)
To: Kurt Borja, Jonathan Cameron, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Nguyen Minh Tien, linux-iio, devicetree, linux-kernel
In-Reply-To: <DJA1J8D91ESA.2XU7OCVKN7LXU@gmail.com>
On 6/15/26 7:18 PM, Kurt Borja wrote:
> Hi David,
>
> On Mon Jun 15, 2026 at 4:59 PM -05, David Lechner (TI) wrote:
>> This adds support for TI ADS112C14 and ADS122C14 ADC chips.
>>
>> The closest thing we've seen to this in the kernel already is ads124s08.
>> However, that has a completely different register map and the DT
>> bindings are incomplete and the driver is extremely basic. So I've just
>> started from scratch here.
>>
>> We've also had a similar submission recently for ADS1220 [1]. That chip
>> is in a similar situation to ads124s08 in that it has a different
>> register map (but the submitted DT bindings are better than the ones for
>> ads124s08, even if still a bit incomplete). And literally as I was
>> writing the previous sentence, another series [2] was sent for yet
>> another similar family of chips (ADS1262). That one is even more complex
>> in the feature set than the ones I am working on. I was going to polish
>> up the driver a bit more before submitting it, but now it seems more
>> urgent to coordinate with the other two series to align on how we would
>> like to handle all of these.
>>
>> [1]: https://lore.kernel.org/linux-iio/20260610151342.44274-1-zizuzacker@gmail.com/
>> [2]: https://lore.kernel.org/linux-iio/20260612-ads126x-v1-0-894c788d03ed@gmail.com/
>>
>> All of these chips have in common that they are designed for use with
>> RTDs and thermocouples and so they look very similar to each other in
>> terms of wiring and feature set, even if the register maps are
>> different. They are in the gray area where we could either keep them
>> separate because they are just different enough, or we could do like
>> we've done before with ad_sigma_delta and have a bit of an abstraction
>> layer for the register differences and otherwise try to share as much
>> code as possible. Normally, I would lean towards keeping them separate,
>> but in this case, I'm considering trying to share code because the
>> devicetree bindings for the inputs is complex and is going to be mostly
>> the same across all of these chips.
>
> The channel configuration is indeed very similar for the three chips.
> All three have IDAC, BOC and VREF configurations.
Hmm... I forgot to include the burnout current in the DT bindings. Following
the channel = "conditions for measurement" pattern that I have set out here
I guess that would mean that we would need to have the same inputs twice
when using the burnout. One "channel" would be the one used to do a "precision"
measurement and the other would be the one to do open/short circuit detection.
i2c {
#address-cells = <1>;
#size-cells = <0>;
adc@40 {
compatible = "ti,ads112c14";
reg = <0x40>;
avdd-supply = <&avdd>;
dvdd-supply = <&dvdd>;
refp-supply = <&avdd>;
#address-cells = <1>;
#size-cells = <0>;
channel@0 {
reg = <0>;
diff-channels = <1>, <2>;
excitation-channels = <0>, <3>;
excitation-current-microamp = <500>;
current-chopping;
ti,vref-source = <ADS112C14_VREF_SOURCE_EXTERNAL>;
label = "rtd-precision";
};
channel@1 {
reg = <0>;
diff-channels = <1>, <2>;
excitation-channels = <0>, <3>;
excitation-current-microamp = <500>;
burnout-current-nanoamp = <1000>;
ti,vref-source = <ADS112C14_VREF_SOURCE_EXTERNAL>;
label = "rtd-diagnostic";
};
};
};
>
>>
>> If we decide to go the route of sharing code, we could still merge this
>> series as-is and then do the refactoring to add the abstraction layer in
>> a follow-up series that also adds support for the first of the other
>> chips.
>
> Do you have a proposal of how such an abstraction would look like? I do
> like the idea of abstracting the firmware parsing, scales and shared
> calculations.
As mentioned, it would look a bit like the ad_sigma_delta ADC driver.
Basically, each chip (family with same registers) would still have it's
own driver that include the chip-specific const info structs. In this
case, these would also include function pointers for all of the functions
that need to access registers. And there will be lots of supports_... bool
flags for the shared code to use.
>
>>
>> This series includes just basic support for reading single measurements
>> from the ADC and gain selection via the scale attribute. I plan to
>> follow this up with additional series to add support for buffered reads,
>> filtering/oversampling configuration, event support, gpio controller
>> support and perhaps a few other things that are slipping my mind right
>> now.
>>
>> The most interesting part about this (that I alluded to above) is the
>> way channels are handled. These are multipling ADCs with differential
>> and single-ended inputs. But what sets them apart from other similar
>> chips is that since they are designed for use with RTDs, there can also
>> be a current output required to excite the RTD and this current output
>> might be different for different channels. So the way I conceptualized
>> the channels is that the devicetree specifies the conditions needed
>> to take a particular measurement rather than being purely a physical
>> channel.
>>
>> This makes things more flexible, but does make the driver a bit more
>> complex. For example, knowing when the current output needs to be
>> enabled or disabled. For now, I have chosen a lazy-enable where they
>> are not turned on until the first measurement is taken that requires
>> them, but then they stay on until another measurement is taken that
>> doesn't require them. This can lead to some oddness with the diagnostic
>> channels that may be measuring something that indirectly requires the
>> current output (i.e. the external reference voltage when it is connected
>> to a resistor rather than a power supply). This means you need to take
>> a measurement that requires the current output to be enabled before the
>> diagnostic channels will give accurate readings.
>
> This is the same approach I took around the BOC, it feels kinda hacky
> but it makes sense. Just an idea I thought about just now: What if we
> have an additional write-only "_enable" sysfs attribute for these
> channels?
I would not want to make a write-only attribute, we always want to be
able to read back what the current state is.
Do you mean an _enable for just the BOC? I think I would do it like I
suggested above instead.
>
> The approach I took for the IDAC was to have a single configuration that
> it's enabled for all channels. This makes some sense in my device when
> thinking about optimal software sequencial reads, because of the
> register layout, but I also see the value in having per-channel IDAC
> configuration. I think I will take your approach, so we have the same
> channel configuration around this.
There are only two IDACs but on chips with 8 AIN pins we could have two
RTDs wired up and if both require 2 IDACs, then we can't have a fixed
setting for the IDACs. This is why I made it per-channel.
>
> Have you thought about how to implement the BOC? In the ADS1262 the
> feature can be found "Sensor Bias". What I did was add per channel DT
> properties for this too.
See above.
>
> Another question. When you implement power management in the future,
> will you enable autosuspend? IDAC currents will be lost if autosuspend
> is enabled. Is this acceptable? In my case I did enable autosuspend, but
> I have some doubts about this.
I don't like to implement power management unless I have an application
that actually requires it. Otherwise, I consider it premature optimization.
It is difficult to know if we would be implementing it in a way that is
actually useful for a real-world use case. These chips seem like they
would mostly be used in an industrial setting, not in battery powered
applications, so I'm not sure anyone is going to worry about saving a few
milliamps.
>
>>
>> I have also pushed a branch to [3] that contains the start of some
>> documentation for this driver that can give some more insight into how
>> the implementation works. It still needs some work and also documents
>> some things that haven't been implemented yet, so I haven't included it
>> in this series yet.
>>
>> [3]: https://github.com/dlech/linux/blob/b4/iio-adc-ti-ads122c14/Documentation/iio/ads112c14.rst
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
>> David Lechner (TI) (4):
>> dt-bindings: iio: adc: add ti,ads122c14
>> iio: adc: add ti-ads112c14 driver
>> iio: adc: ti-ads112c14: implement gain on internal short SYS_MON channel
>> iio: adc: ti-ads112c14: add measurement channel support
>>
>> .../devicetree/bindings/iio/adc/ti,ads112c14.yaml | 224 +++++
>> MAINTAINERS | 8 +
>> drivers/iio/adc/Kconfig | 12 +
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/ti-ads112c14.c | 1053 ++++++++++++++++++++
>> include/dt-bindings/iio/adc/ti,ads112c14.h | 11 +
>> 6 files changed, 1309 insertions(+)
>> ---
>> base-commit: ec039126b7fac4e3af35ebccaa7c6f9b6875ba81
>> change-id: 20260514-iio-adc-ti-ads122c14-d0b92479334e
>>
>> Best regards,
>> --
>> David Lechner (TI) <dlechner@baylibre.com>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox