* Re: [PATCH v3 1/2] dt-bindings: Add GPIO-locked fixed clock
From: Conor Dooley @ 2026-06-16 15:39 UTC (permalink / raw)
To: Brian Masney
Cc: V.Yurkov.EXT, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-clk,
devicetree, Vyacheslav Yurkov
In-Reply-To: <ajB2PhmxvdtsXNnm@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 4276 bytes --]
On Mon, Jun 15, 2026 at 06:01:34PM -0400, Brian Masney wrote:
> Hi Vyacheslav,
>
> On Wed, Jun 03, 2026 at 11:16:42AM +0000, Vyacheslav Yurkov via B4 Relay wrote:
> > From: Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>
> >
> > Some hardware designs provide fixed-frequency clocks generated outside
> > software control, such as by FPGA-resident PLLs. While the clock rate is
> > fixed, a separate GPIO signal indicates whether the clock source is
> > locked and producing a valid output.
> >
> > Describe a GPIO-locked fixed clock provider that exposes a fixed-rate
> > clock whose availability depends on one or more GPIO lock-status
> > signals.
> >
> > Signed-off-by: Vyacheslav Yurkov <uvv.mail@gmail.com>
> > Signed-off-by: Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>
> > ---
> > .../bindings/clock/gpio-locked-fixed-clock.yaml | 70 ++++++++++++++++++++++
> > 1 file changed, 70 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/clock/gpio-locked-fixed-clock.yaml b/Documentation/devicetree/bindings/clock/gpio-locked-fixed-clock.yaml
> > new file mode 100644
> > index 000000000000..9106b800b673
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/gpio-locked-fixed-clock.yaml
> > @@ -0,0 +1,70 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/gpio-locked-fixed-clock.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: GPIO Locked Fixed Clock
> > +
> > +maintainers:
> > + - Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>
> > +
> > +description: |
> > + Provides a clock output whose availability depends on a set of
> > + prerequisite conditions. These conditions include the presence of
> > + one or more parent clocks and the asserted state of one or more
> > + GPIO lock indicators. An example of such clocks is FPGA clock that
> > + are outside CPU control, with the lock status exposed through GPIO
> > + signal.
> > +
> > + The output clock is considered available only when all configured
> > + prerequisites are satisfied.
>
> I'm stepping outside my usual review of just the clk drivers. Krzysztof
> in v1 and v2 asked for more detailed hardware explanation. This feels to
> me like this is a policy that says to not use these clocks until the
> GPIO says they are ready. My gut feeling is that details like this
> should live in a clk driver instead of a dt-binding.
>
> Alternatively, if this is generic enough, then could
> Documentation/devicetree/bindings/clock/gated-fixed-clock.yaml be
> extended?
FWIW this came up in an earlier revision:
https://lore.kernel.org/all/20260407-fling-scouring-dbe2141cc79b@spud/
IMO it's an inverted gpio-gate-clock, where the gpio is an input rather
than an output. I suppose you could extend gpio-gate-clock with it and
have mutually exclusive enable-gpios and status-gpios?
>
> Brian
>
>
> > +
> > +properties:
> > + compatible:
> > + const: gpio-locked-fixed-clock
> > +
> > + "#clock-cells":
> > + const: 0
> > +
> > + clocks:
> > + description: Input clocks whose validity is monitored by this provider.
> > +
> > + clock-output-names:
> > + description: Names of the clock provided by this controller.
> > + maxItems: 1
> > +
> > + locked-gpios:
> > + description: |
> > + GPIOs to check the lock state.
> > + minItems: 1
> > + maxItems: 32
> > +
> > +required:
> > + - compatible
> > + - "#clock-cells"
> > +
> > +anyOf:
> > + - required:
> > + - clocks
> > + - required:
> > + - locked-gpios
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/gpio/gpio.h>
> > +
> > + clk_gpio_locked: gpio-locked-fixed-clock {
> > + compatible = "gpio-locked-fixed-clock";
> > + #clock-cells = <0>;
> > +
> > + clocks = <&clk0 0>, <&pll 0>;
> > +
> > + locked-gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>,
> > + <&gpio0 5 GPIO_ACTIVE_HIGH>,
> > + <&gpio1 2 GPIO_ACTIVE_LOW>;
> > +
> > + clock-output-names = "clkout0";
> > + };
> >
> > --
> > 2.34.1
> >
> >
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 2/4] iio: adc: add ti-ads112c14 driver
From: David Lechner @ 2026-06-16 15:38 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Kurt Borja, Nguyen Minh Tien,
linux-iio, devicetree, linux-kernel
In-Reply-To: <ajD8Gxkp66kkDflE@ashevche-desk.local>
On 6/16/26 2:32 AM, Andy Shevchenko wrote:
> On Mon, Jun 15, 2026 at 05:00:00PM -0500, David Lechner (TI) wrote:
>> Add a new driver for the TI ADS112C14/ADS122C14 ADC chips.
>>
>> This first step is adding a very basic driver that only supports power
>> on/reset and reading the system monitor channels.
>>
>> ADS112C14_SYS_MON_CHANNEL_SHORT is the last channel rather than being in
>> logical order by address to keep the voltage channels together and in
>> case we find we need to add variants of this channel with different
>> voltage reference later.
>
> ...
>> +static int ads112c14_read_label(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan, char *label)
>> +{
>> + const char *label_source;
>
> I don't see the need of having this. Can't be returned directly?
Then this would have to be split into two functions. One to return
it directly and one to do the sysfs_emit(). I don't think it is
worth it.
>
>> + /* System monitor channels. */
>> + switch (chan->channel) {
>> + case ADS112C14_SYS_MON_CHANNEL_TEMP:
>> + label_source = "Internal temperature sensor";
>> + break;
>> + case ADS112C14_SYS_MON_CHANNEL_EXT_REF:
>> + label_source = "External reference";
>> + break;
>> + case ADS112C14_SYS_MON_CHANNEL_AVDD:
>> + label_source = "AVDD";
>> + break;
>> + case ADS112C14_SYS_MON_CHANNEL_DVDD:
>> + label_source = "DVDD";
>> + break;
>> + case ADS112C14_SYS_MON_CHANNEL_SHORT:
>> + label_source = "Internal short";
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return sysfs_emit(label, "%s\n", label_source);
>> +}
>
>> + /*
>> + * The reset may cause an -EREMOTEIO error because of failing to get the
>> + * I2C ACK at the end of the message. The device still gets reset.
>> + */
>> + if (ret != -EREMOTEIO)
>> + return ret;
>
> I would do it separately as
>
> if (ret == -EREMOTEIO)
> /* ...big comment here... */
> return 0;
We should not return early here. We just continue with the rest
of the function as normal. So I think the way I had it was
simplest. Otherwise we would need a goto or something like that.
> if (ret) // which is regular pattern and doesn't need any comment.
> return ret;
>
>> + fsleep(ADS112C14_DELAY_RESET_us);
>> +
>> + ret = regmap_read(data->regmap, ADS112C14_REG_STATUS_MSB, ®_val);
>> + if (ret)
>> + return ret;
>> +
>> + if (FIELD_GET(ADS112C14_STATUS_MSB_RESETN, reg_val))
>> + return dev_err_probe(dev, -EIO, "reset failed\n");
>> +
^ permalink raw reply
* Re: [PATCH 3/9] firmware: imx: ele: Add API functions for OCOTP fuse access
From: Frank Li @ 2026-06-16 15:36 UTC (permalink / raw)
To: Frieder Schrempf
Cc: Srinivas Kandagatla, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Shawn Guo, devicetree, imx, linux-arm-kernel,
linux-kernel, Frieder Schrempf
In-Reply-To: <20260616-upstreaming-next-20260609-imx-ocotp-ele-v1-3-cb7f3698c3e6@kontron.de>
On Tue, Jun 16, 2026 at 01:52:18PM +0200, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>
> The ELE S400 API provides read and write access to the OCOTP fuse
> registers. This adds the necessary API functions imx_se_read_fuse()
> and imx_se_write_fuse() to be used by other drivers such as the
> OCOTP S400 NVMEM driver.
>
> This is ported from the downstream vendor kernel.
>
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
> drivers/firmware/imx/ele_base_msg.c | 122 ++++++++++++++++++++++++++++++++++++
> drivers/firmware/imx/ele_base_msg.h | 6 ++
> include/linux/firmware/imx/se_api.h | 3 +
> 3 files changed, 131 insertions(+)
>
...
> +++ b/include/linux/firmware/imx/se_api.h
> @@ -11,4 +11,7 @@
> #define SOC_ID_OF_IMX8ULP 0x084d
> #define SOC_ID_OF_IMX93 0x9300
>
> +int imx_se_read_fuse(void *se_if_data, uint16_t fuse_id, u32 *value);
> +int imx_se_write_fuse(void *se_if_data, uint16_t fuse_id, u32 value);
> +
This API should implement in fuse drivers. Other consume should use standard
fuse API to get value. If put here, it may bypass fuse driver.
Frank
> #endif /* __SE_API_H__ */
>
> --
> 2.54.0
>
>
^ permalink raw reply
* Re: [PATCH v6 1/7] dt-bindings: mfd: mt6397: Add MT6392 PMIC
From: Conor Dooley @ 2026-06-16 15:35 UTC (permalink / raw)
To: Luca Leonardo Scorcia
Cc: linux-mediatek, Fabien Parent, Val Packett, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sen Chu,
Sean Wang, Macpaul Lin, Lee Jones, Matthias Brugger,
AngeloGioacchino Del Regno, Linus Walleij, Julien Massot,
Louis-Alexis Eyraud, Akari Tsuyukusa, Chen Zhong, linux-input,
devicetree, linux-kernel, linux-pm, linux-arm-kernel, linux-gpio
In-Reply-To: <CAORyz2+4EquYcUHEnoq0N_p7vCmDpPONEhVrmAfO1eX_RNMYbQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]
On Mon, Jun 15, 2026 at 07:09:39PM +0200, Luca Leonardo Scorcia wrote:
> Hi,
> yes, sorry about that, series v6 has been superseded by v7 (I replied
> to the thread and marked it as archived in patchwork, please let me
> know if I have to do something else to mark it as obsolete).
> Sashiko was correct, the regulators node is required for this device.
I have no idea what regulator node you're referring to here or what
patchwork. Please don't delete context when you reply.
If it's the devicetree patchwork I don't you need to do anything.
> Sashiko also has suggestions for v7, a few pre existing issues and a
> few nits here and there but some are actual improvements. One bit that
> caught my eye is the use of the modeset register in the
> mt6392_ldo_get_mode function. I have to double check that with the
> data sheet and the android kernel sources. Not sure if I can do that
> before next week though.
>
> Is there any way I can trigger a Sashiko review before sending patches
> to the ML?
I don't know sorry.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v7 9/9] arm64: dts: mediatek: Add MediaTek MT6392 PMIC dtsi
From: Luca Leonardo Scorcia @ 2026-06-16 15:32 UTC (permalink / raw)
To: Rob Herring
Cc: linux-mediatek, Val Packett, Dmitry Torokhov, Krzysztof Kozlowski,
Conor Dooley, Sen Chu, Sean Wang, Macpaul Lin, Lee Jones,
Matthias Brugger, AngeloGioacchino Del Regno, Liam Girdwood,
Mark Brown, Linus Walleij, Louis-Alexis Eyraud, Julien Massot,
Fabien Parent, Akari Tsuyukusa, Chen Zhong, linux-input,
devicetree, linux-kernel, linux-pm, linux-arm-kernel, linux-gpio
In-Reply-To: <20260616133918.GA2335264-robh@kernel.org>
> > arch/arm64/boot/dts/mediatek/mt6392.dtsi | 75 ++++++++++++++++++++++++
>
> Nothing is using this so it is a dead file that doesn't get tested.
Hi, it's not referenced as the dtsi inclusion was removed in the
original patch from 2019 for an easier merging of support for mt8516
pumpkin boards [1][2].
If you prefer in the next revision I can add another patch to readd it
to the existing pumpkin board.
I am working on a few boards with MT8167 (Xiaomi Mi Smart Clock,
Lenovo Smart Clock 2, Sony Playstation Classic) that reference it and
these have been used to test it locally too.
[1] https://lore.kernel.org/linux-mediatek/20190323211612.860-25-fparent@baylibre.com/
[2] https://lore.kernel.org/linux-mediatek/20200229170401.1287324-2-fparent@baylibre.com/
Thank you
Luca
^ permalink raw reply
* Re: [PATCH 6/7] riscv: dts: eswin: add I2C controller support
From: Conor Dooley @ 2026-06-16 15:31 UTC (permalink / raw)
To: Pinkesh Vaghela
Cc: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-riscv@lists.infradead.org, Min Lin, Yulin Lu,
Samuel Holland, Darshan Prajapati, Pritesh Patel
In-Reply-To: <SJ2PR04MB8896A5877CCFC177FA5D5A9583E52@SJ2PR04MB8896.namprd04.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 618 bytes --]
On Tue, Jun 16, 2026 at 11:57:33AM +0000, Pinkesh Vaghela wrote:
> On Mon, Jun 15, 2026 at 10:05 PM +0530, Conor Dooley wrote:
> > On Mon, Jun 15, 2026 at 05:50:15PM +0530, Pinkesh Vaghela wrote:
> > > + i2c0: i2c@50950000 {
> > > + compatible = "snps,designware-i2c";
> >
> > Missing a soc-specific compatible here for all i2c controllers.
>
> We are using generic DesignWare I2C driver for all I2C controllers with
> no changes required in the driver. Do we still need to add soc-specific
> compatible for all i2c controllers?
Yes. Just use "snps,designware-i2c" as a fallback.
Thanks,
Conor.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 4/4] iio: adc: ti-ads112c14: add measurement channel support
From: David Lechner @ 2026-06-16 15:30 UTC (permalink / raw)
To: Jonathan Cameron, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: Kurt Borja, Nguyen Minh Tien, linux-iio, devicetree, linux-kernel
In-Reply-To: <20260615-iio-adc-ti-ads122c14-v1-4-e6bdadf7cb2b@baylibre.com>
On 6/15/26 5:00 PM, David Lechner (TI) wrote:
> Add support for parsing devicetree properties for measurement channels
> and doing direct reads on these.
>
> There are quite a lot of conditions that have to be met for each
> measurement to be made, so quite a bit of state and algorithms are
> required to handle it.
>
> Channels are created dynamically since the number of possibilities is
> unreasonably large.
>
...
> static void ads112c14_populate_tables(struct ads112c14_data *data)
> {
> u32 vref_uV, fsr_bits;
> - int i;
> +
> + for (u32 i = 0; i < data->num_measurements; i++) {
> + struct ads112c14_measurement *measurement = &data->measurements[i];
> +
> + switch (measurement->vref_source) {
> + case ADS112C14_VREF_SOURCE_EXTERNAL:
> + if (data->ext_ref_ohms)
> + vref_uV = measurement->idac_current_uA *
> + measurement->iadc_count * data->ext_ref_ohms;
One thing I am considering is that if we have a resistor between
REFP and REFN rather than a voltage supply, then we should consider
the channel IIO_RESISTANCE rather than IIO_VOLTAGE.
In this case, the user doesn't care about voltage at all, but rather
the ratio of the REFP-REFN resistor to the measured resistance.
^ permalink raw reply
* Re: [PATCH 1/4] dt-bindings: iio: adc: add ti,ads122c14
From: David Lechner @ 2026-06-16 15:22 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: <DJA1PXNS9SFP.VAURJH1MR9BK@gmail.com>
On 6/15/26 7:26 PM, Kurt Borja wrote:
> On Mon Jun 15, 2026 at 4:59 PM -05, David Lechner (TI) wrote:
>> Add new bindings for ti,ads122c14 and similar devices.
>>
>> This is an ADC that is primarily intended for use with temperature
>> sensors. There are a few unusual properties because of this. In
>> particular, the reference voltage source and current output requirements
>> can be different for each measurement, so these are included in the
>> channel bindings.
>>
>> The REFP/REFN reference voltage is usually just connected to a resistor
>> that is being driven by the ADC's current outputs, so there is special
>> property for this case rather than requiring a regulator to be defined
>> to represent that.
>>
>> ti,vref-source is reused from ti,tlv320adcx140.yaml (otherwise might
>> have preferred an enum of strings).
>>
>> Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>
>> ---
>> .../devicetree/bindings/iio/adc/ti,ads112c14.yaml | 224 +++++++++++++++++++++
>> MAINTAINERS | 7 +
>> include/dt-bindings/iio/adc/ti,ads112c14.h | 11 +
>> 3 files changed, 242 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads112c14.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads112c14.yaml
>> new file mode 100644
>> index 000000000000..dc7f37cad772
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads112c14.yaml
>> @@ -0,0 +1,224 @@
>
> [...]
>
>> +patternProperties:
>> + ^channel@[0-7]$:
>> + $ref: adc.yaml
>> +
>> + unevaluatedProperties: false
>> +
>> + properties:
>> + reg:
>> + maximum: 16 # arbitrary limit, channel@ can be any combination of AIN0-AIN7
>> +
>> + single-channel:
>> + maximum: 7
>> +
>> + diff-channels:
>> + items:
>> + maximum: 7
>> +
>> + bipolar:
>> + description:
>> + Set this flag if the differential input can be negative.
>> +
>> + excitation-channels:
>
> I noticed this doesn't have the "ti," prefix. Is your plan to add this
> to adc.yaml?
I hadn't really though about it. I guess it could make sense.
>
> Also, do you think excitation-pins might be a better name?
I used -channels to match e.g. diff-channels. The same values apply to
both properties.
>
>> + description: AINx pins used as current output.
>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>> + minItems: 1
>> + maxItems: 2
>> + items:
>> + maximum: 7
>> +
>> + excitation-current-microamp:
>> + description: The current output of the excitation channels in microamps.
>> + minimum: 1
>> + maximum: 1000
>> +
>> + current-chopping:
>
> If you agree with the above comment, I think this too should be added.
> In that case, can we call this something like
> excitation-current-rotation for less ambiguity?
I think excitation-channel-rotation would be fine.
>
> I say this because my device has an "IDAC rotation mode", but it also has
> a "Chop Mode" which rotates analog inputs and averages consecutive
> conversions.
And this one sounds like it could be named input-channel-rotation. Would
this also need a 2nd set of single-channel or diff-channels to the extra
inputs?
>
>> + $ref: /schemas/types.yaml#/definitions/flag
>> + description:
>> + If provided, the two excitation channels are to be used with current
>> + chopping enabled.
>> +
>> + ti,vref-source:
>> + description: |
>> + Indicates the source for the reference voltage for this channel.
>> + 0 - Internal 2.5V reference
>> + 1 - Internal 1.25V reference
>> + 2 - External reference (REFP-REFN)
>> + 3 - AVDD as reference
>> +
>> + For convenience, macros for these values are available in
>> + dt-bindings/iio/adc/ti,ads112c14.h.
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + maximum: 3
>> + default: 0
>> +
>> + dependencies:
>> + excitation-channels: [ excitation-current-microamp ]
>> + excitation-current-microamp: [ excitation-channels ]
>> + current-chopping: [ excitation-channels ]
>> +
>> + oneOf:
>> + - required: [ single-channel ]
>> + - required: [ diff-channels ]
>
> [...]
>
^ 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
* 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 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 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: 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 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 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 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 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 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 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 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 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 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 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 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 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
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