* Re: (subset) [PATCH] dt-bindings: mfd: syscon: Revert renesas,r9a08g046-lvds-cmn
From: Lee Jones @ 2026-06-15 10:21 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Tommaso Merciai, Biju Das,
devicetree, linux-kernel, linux-renesas-soc, Krzysztof Kozlowski
In-Reply-To: <20260608115507.134969-2-krzysztof.kozlowski@oss.qualcomm.com>
On Mon, 08 Jun 2026 13:55:08 +0200, Krzysztof Kozlowski wrote:
> Revert commit 51284d8b1dbc ("dt-bindings: mfd: syscon: Document the
> LVDS_CMN syscon for the RZ/G3L") because it is completely not matching
> reality and clearly incorrect in respect of renesas,r9a08g046-lvds-cmn.
>
> It wasn't ever build-tested by author on their DTS, either.
>
> The documented renesas,r9a08g046-lvds-cmn compatible clearly disallows
> any children and simple-mfd fallback, however its only use in original
> patchset is with simple-mfd and children, so this could have never
> worked.
>
> [...]
Applied, thanks!
[1/1] dt-bindings: mfd: syscon: Revert renesas,r9a08g046-lvds-cmn
commit: 4143734f197c0065bf5fce7da22f4d0eaf404753
--
Lee Jones [李琼斯]
^ permalink raw reply
* RE: [PATCH 7/7] ASoC: dt-bindings: ES8389: Add members about HPF and clock
From: Zhang Yi @ 2026-06-15 10:14 UTC (permalink / raw)
To: krzk
Cc: alsa-devel, broonie, conor+dt, devicetree, krzk+dt, robh, tiwai,
zhangyi
In-Reply-To: <241d0fab-26ea-4c06-9928-e256f2cc5d1c@kernel.org>
> >>> Yes, setting mclk-from-sclk does not affect the MCLK connection.
> >>
> >> I am asking about wiring of the device. If MCLK is used from SCLK, but
> >> SCLK is used as the internal clock, then how can you still have MCLK
> >> connected?
> >
> > If MCLK is derived from SCLK, whether the 'MCLK' pin on the device is
> > connected or not has no effect on operation.
>
> If MCLK is connected, why would you derive it from SCLK? That's the
> point - your clocks property already might be telling everything needed
> here.
This is because sometimes the codec does not work properly
when using the MCLK provided by the CPU.
> >>>>> +
> >>>>> + everest,hpfl:
> >>>>> + $ref: /schemas/types.yaml#/definitions/uint8
> >>>>> + description:
> >>>>> + the HPF value of ADCL.
> >>>>
> >>>> Is HPF value in dB? If so, use proper unit suffix and proper units.
> >>>
> >>> No, the values here correspond to the values in the registers.
> >>> The value is not in dB
> >>
> >> What are the meanings of the register values?
> >
> > The value of everest,hpfl is equal to the value of the corresponding register,
> > not the cutoff frequency of the HPF.
>
> You keep avoiding answers.
>
> I don't want you to encode standard units as register values.
I think I understand what you mean.
Do you want me to enter the physical values in DTS and then convert them to register values in the code?
^ permalink raw reply
* Re: [PATCH v2 3/3] ASoC: qcom: sc8280xp: ASoC: qcom: sc8280xp: enhance machine driver for board-specific config
From: Mohammad Rafi Shaik @ 2026-06-15 10:11 UTC (permalink / raw)
To: Mark Brown
Cc: Srinivas Kandagatla, Liam Girdwood, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jaroslav Kysela, Takashi Iwai,
Krzysztof Kozlowski, linux-arm-msm, linux-sound, devicetree,
linux-kernel
In-Reply-To: <b09da7e1-ddc1-452b-9380-964f9ed3b733@sirena.org.uk>
On 6/8/2026 5:38 PM, Mark Brown wrote:
> On Mon, Jun 08, 2026 at 08:00:11AM +0530, Mohammad Rafi Shaik wrote:
>> The sc8280xp machine driver is currently written with a largely
>> SoC-centric view and assumes a uniform audio topology across all boards.
>
>> +static inline int sc8280xp_get_mclk_freq(struct snd_pcm_hw_params *params)
>> +{
>> + int rate = params_rate(params);
>> +
>> + switch (rate) {
>> + case SNDRV_PCM_RATE_11025:
>> + case SNDRV_PCM_RATE_44100:
>> + case SNDRV_PCM_RATE_88200:
>
> rate is in Hz but these are bitmasks.
>
Yes right, thanks for pointing out.
Comparing the sampling rate value from params_rate(params) against
bitmasks like SNDRV_PCM_RATE_44100 is an error. I will update the
switch-case to use literal frequency values (e.g., 44100, 88200) or
appropriate rate constants to ensure the sampling rate is correctly
identified.
>> + return I2S_MCLK_RATE(44100);
>> + default:
>> + break;
>> + }
>
> The function only works since it ignores invalid values.
>
Ack, will fix in next version.
>> +static int sc8280xp_snd_hw_params(struct snd_pcm_substream *substream,
>> + struct snd_pcm_hw_params *params)
>> +{
>
>> +
>> + if (data->snd_soc_common_priv->codec_dai_fmt)
>> + snd_soc_dai_set_fmt(codec_dai,
>> + data->snd_soc_common_priv->codec_dai_fmt);
>
> Should we error check the functions we call here?
>
Agreed. I will add proper return value checks for snd_soc_dai_set_fmt
and snd_soc_dai_set_sysclk to ensure any configuration failures are
caught and reported during hw_params.
>> + if (data->snd_soc_common_priv->mi2s_mclk_enable)
>> + snd_soc_dai_set_sysclk(cpu_dai,
>> + LPAIF_MI2S_MCLK, mclk_freq,
>> + SND_SOC_CLOCK_IN);
>> +
>> + if (data->snd_soc_common_priv->mi2s_bclk_enable)
>> + snd_soc_dai_set_sysclk(cpu_dai,
>> + LPAIF_MI2S_BCLK, bclk_freq,
>> + SND_SOC_CLOCK_IN);
>
> Is SND_SOC_CLK_IN right here? The flag sounds like it's enabling the
> clock on the DAI but this is configuring the DAI to consume a clock.
Good catch. Since the CPU DAI is providing the MCLK/BCLK to the codec in
this configuration (Master mode), it should be configured as
SND_SOC_CLOCK_OUT. I will change these to SND_SOC_CLOCK_OUT to correctly
reflect that the DAI is the clock source.
>
>> + if (data->snd_soc_common_priv->codec_sysclk_set)
>> + snd_soc_dai_set_sysclk(cpu_dai,
>> + 0, mclk_freq,
>> + SND_SOC_CLOCK_IN);
>
> This is configuring the CPU not CODEC.
will fix typo error. The call should indeed target the codec_dai when
the codec_sysclk_set flag is enabled. I will fix the function call to
use the correct DAI pointer.
Thanks & Regards,
Rafi.
^ permalink raw reply
* Re: [PATCH v2 1/4] iio: dac: ad3530r: Refactor setup to table-driven register bank approach
From: Andy Shevchenko @ 2026-06-15 10:08 UTC (permalink / raw)
To: Kim Seer Paller
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-iio, linux-kernel, linux, devicetree
In-Reply-To: <20260615-iio-ad3532r-support-v2-1-84a0af8b83fa@analog.com>
On Mon, Jun 15, 2026 at 02:20:15PM +0800, Kim Seer Paller wrote:
> Replace direct register calls in ad3530r_setup() with per-chip register
> address arrays and bank helpers (ad3530r_set_reg_bank_bits,
> ad3530r_write_reg_banks). Convert sw_ldac_trig_reg from a static
> register address to a function pointer for per-bank LDAC trigger
> register selection. Switch spi_device_id to named initializers.
...
> +static int ad3530r_set_reg_bank_bits(const struct ad3530r_state *st,
> + const unsigned int *regs,
> + unsigned int num_regs,
> + unsigned int mask)
> +{
> + int ret;
> +
> + for (unsigned int i = 0; i < num_regs; i++) {
> + ret = regmap_update_bits(st->regmap, regs[i], mask, mask);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int ad3530r_write_reg_banks(const struct ad3530r_state *st,
> + const unsigned int *regs,
> + unsigned int num_regs,
> + unsigned int val)
> +{
> + int ret;
> +
> + for (unsigned int i = 0; i < num_regs; i++) {
> + ret = regmap_write(st->regmap, regs[i], val);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
Can the above helpers use bulk operations or regmap_multi_reg_write()?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 4/4] iio: dac: ad3530r: Add support for AD3532R/AD3532
From: Andy Shevchenko @ 2026-06-15 10:05 UTC (permalink / raw)
To: Kim Seer Paller
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-iio, linux-kernel, linux, devicetree
In-Reply-To: <20260615-iio-ad3532r-support-v2-4-84a0af8b83fa@analog.com>
On Mon, Jun 15, 2026 at 02:20:18PM +0800, Kim Seer Paller wrote:
> The AD3532R/AD3532 is a 16-channel, 16-bit voltage output DAC with a
> dual-bank register architecture (bank 0 at 0x1000 for channels 0-7,
> bank 1 at 0x3000 for channels 8-15). It shares similar functionality
> with AD3530R (channel configuration, LDAC triggering, powerdown control),
> the main difference being the register address map due to the dual-bank
> architecture, handled by table-driven helpers.
>
> Add AD3532R-specific register definitions, channel specs, per-bank
> register arrays, a dedicated ad3532r_set_dac_powerdown(), and per-chip
> regmap_config to limit debugfs-exposed register space to each variant's
> actual address range.
...
> help
> - Say yes here to build support for Analog Devices AD3530R, AD3531R
> - Digital to Analog Converter.
> + Say yes here to build support for Analog Devices AD3530/AD3530R,
> + AD3531/AD3531R, and AD3532/AD3532R Digital to Analog Converters.
This just shows how unscalable the above text is. That's why we usually
recommend to make the list explicit and separated.
Say yes here to build support for the following Analog Devices
Digital to Analog Converters:
- AD3530/AD3530R (8-channel)
- AD3531/AD3531R (4-channel)
- AD3532/AD3532R (16-channel)
(and looking into the C-file change, perhaps add here as well distinctive
information, such as number of channels, in the parentheses).
> To compile this driver as a module, choose M here: the
> module will be called ad3530r.
...
> +#define AD3532R_INTERFACE_CONFIG_A_0 0x1000
> +#define AD3532R_INTERFACE_CONFIG_A_1 0x3000
> +#define AD3532R_OUTPUT_OPERATING_MODE_0 0x1020
> +#define AD3532R_OUTPUT_OPERATING_MODE_1 0x1021
> +#define AD3532R_OUTPUT_OPERATING_MODE_2 0x3020
> +#define AD3532R_OUTPUT_OPERATING_MODE_3 0x3021
> +#define AD3532R_OUTPUT_CONTROL_0 0x102A
> +#define AD3532R_OUTPUT_CONTROL_1 0x302A
> +#define AD3532R_REFERENCE_CONTROL_0 0x103C
> +#define AD3532R_REFERENCE_CONTROL_1 0x303C
> +#define AD3532R_SW_LDAC_TRIG_0 0x10E5
> +#define AD3532R_SW_LDAC_TRIG_1 0x30E5
> +#define AD3532R_INPUT_CH_0 0x10EB
> +#define AD3532R_INPUT_CH_1 0x30EB
> +#define AD3532R_MAX_REG_ADDR 0x30F9
Hmm... I dunno if it's better to sort by values (so the "bank" 0 goes together
followed by "bank" 1). Jonathan, what's your preference here? Nuno, David?
...
> +static ssize_t ad3532r_set_dac_powerdown(struct iio_dev *indio_dev,
> + uintptr_t private,
> + const struct iio_chan_spec *chan,
> + const char *buf, size_t len)
> +{
> + struct ad3530r_state *st = iio_priv(indio_dev);
> + unsigned int reg, pdmode, mask, val, local_ch;
> + bool powerdown;
> + int ret;
> +
> + ret = kstrtobool(buf, &powerdown);
Do you need to include kstrtox.h?
> + if (ret)
> + return ret;
> +
> + guard(mutex)(&st->lock);
+ blank line here.
> + local_ch = chan->channel % AD3530R_CH_PER_BANK;
> + reg = (chan->channel < AD3530R_CH_PER_BANK ? AD3532R_OUTPUT_OPERATING_MODE_0 :
> + AD3532R_OUTPUT_OPERATING_MODE_2) +
> + local_ch / AD3530R_CH_PER_REG;
This is unreadable. Can you refactor it?
> + mask = AD3530R_OP_MODE_CHAN_MSK(local_ch % AD3530R_CH_PER_REG);
> +
> + pdmode = powerdown ? st->chan[chan->channel].powerdown_mode : 0;
> + val = field_prep(mask, pdmode);
> +
> + ret = regmap_update_bits(st->regmap, reg, mask, val);
> + if (ret)
> + return ret;
> +
> + st->chan[chan->channel].powerdown = powerdown;
> +
> + return len;
> +}
...
> + .num_banks = ARRAY_SIZE(ad3532r_if_config),
Also check if array_size.h is included.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 2/2] hwmon: (pmbus/lm25066) add current limit configuration support
From: sashiko-bot @ 2026-06-15 10:03 UTC (permalink / raw)
To: Potin Lai; +Cc: robh, conor+dt, linux-hwmon, devicetree
In-Reply-To: <20260615-lm25066-cl-config-v3-2-decb4f5b0b77@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] The PMBus coefficient selection logic uses swapped Bit 4 semantics for non-LM25066 chips, causing incorrect telemetry scaling.
- [High] Missing `R` exponent update when alternative current limit (`_L`) coefficients are selected.
--
commit 5b618ea6bd8652d84454a36d0af72de96fc3501b
Author: Potin Lai <potin.lai.pt@gmail.com>
hwmon: (pmbus/lm25066) add current limit configuration support
Add support for the 'ti,current-range' devicetree property to configure
the current limit via the DEVICE_SETUP (0xD9) register, overriding the
physical CL pin setting.
> diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
> index dd7275a67a0ab..53d274033d66f 100644
> --- a/drivers/hwmon/pmbus/lm25066.c
> +++ b/drivers/hwmon/pmbus/lm25066.c
[ ... ]
> @@ -484,6 +485,42 @@ static int lm25066_probe(struct i2c_client *client)
>
> data->id = (enum chips)(unsigned long)i2c_get_match_data(client);
>
> + if (data->id != lm25056) {
> + int config_new = config;
> + const char *cl_setting;
> + int ret;
> +
> + if (!of_property_read_string(client->dev.of_node,
> + "ti,current-range", &cl_setting)) {
> + config_new |= LM25066_DEV_SETUP_CL_CFG;
> + if (strcmp(cl_setting, "high") == 0) {
> + if (data->id == lm25066)
> + config_new |= LM25066_DEV_SETUP_CL;
> + else
> + config_new &= ~LM25066_DEV_SETUP_CL;
[Severity: High]
This is a pre-existing issue, but does this bit swapping logic expose a
mismatch in how the PMBus coefficients are selected later in lm25066_probe()?
While this patch correctly clears LM25066_DEV_SETUP_CL for the 'high' range
on non-LM25066 chips, the pre-existing coefficient logic unconditionally
checks Bit 4 without accounting for the chip ID:
if (config & LM25066_DEV_SETUP_CL) {
info->m[PSC_CURRENT_IN] = coeff[PSC_CURRENT_IN_L].m;
...
} else {
info->m[PSC_CURRENT_IN] = coeff[PSC_CURRENT_IN].m;
...
For example, on an LM5066, requesting 'high' sets Bit 4 to 0. This causes
the logic to take the else branch and use the standard CURRENT_IN coefficients
(intended for the 'low' limit). Will this cause the hardware monitoring driver
to report current and power values that are off by roughly a factor of 2?
> + } else if (strcmp(cl_setting, "low") == 0) {
> + if (data->id == lm25066)
> + config_new &= ~LM25066_DEV_SETUP_CL;
> + else
> + config_new |= LM25066_DEV_SETUP_CL;
> + } else {
> + dev_err(&client->dev,
> + "invalid current-range setting: %s\n",
> + cl_setting);
> + return -EINVAL;
> + }
> + }
> +
> + if (config_new != config) {
> + ret = i2c_smbus_write_byte_data(client,
> + LM25066_DEVICE_SETUP,
> + config_new);
> + if (ret < 0)
> + return ret;
> + config = config_new;
> + }
> + }
[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the coefficient
selection logic in lm25066_probe(), is there a missing update for the R
exponent?
When the alternative current limit (_L) coefficients are selected, the m and b
values are updated but R is not:
info->R[PSC_POWER] = coeff[PSC_POWER].R;
if (config & LM25066_DEV_SETUP_CL) {
info->m[PSC_CURRENT_IN] = coeff[PSC_CURRENT_IN_L].m;
info->b[PSC_CURRENT_IN] = coeff[PSC_CURRENT_IN_L].b;
info->m[PSC_POWER] = coeff[PSC_POWER_L].m;
info->b[PSC_POWER] = coeff[PSC_POWER_L].b;
} else {
...
For the lm25056 chip, the POWER_L coefficient array entry has R = -4, while
the standard POWER coefficient has R = -3. Because info->R[PSC_POWER] is
only initialized from the base coefficient, could power readings be
miscalculated by a factor of 10 when the alternative limit is active?
> info = &data->info;
>
> info->pages = 1;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-lm25066-cl-config-v3-0-decb4f5b0b77@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH v2 2/3] ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control
From: Mohammad Rafi Shaik @ 2026-06-15 10:00 UTC (permalink / raw)
To: Mark Brown
Cc: Srinivas Kandagatla, Liam Girdwood, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jaroslav Kysela, Takashi Iwai,
Krzysztof Kozlowski, linux-arm-msm, linux-sound, devicetree,
linux-kernel, Srinivas Kandagatla
In-Reply-To: <e6567656-8745-4f07-9636-7779d32ccbcf@sirena.org.uk>
On 6/8/2026 5:31 PM, Mark Brown wrote:
> On Mon, Jun 08, 2026 at 08:00:10AM +0530, Mohammad Rafi Shaik wrote:
>> Add support for MI2S clock control within q6apm-lpass DAIs, including
>> handling of MCLK, BCLK, and ECLK via the DAI .set_sysclk callback.
>
>> +static int q6i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id, unsigned int freq, int dir)
>> +{
>> + struct q6apm_lpass_dai_data *dai_data = dev_get_drvdata(dai->dev);
>> + struct clk *sysclk;
>> + bool *enabled;
>
> Neither sysclk nor enabled are initialised by default...
>
Yes right, Thanks for pointing out.
I will initialize sysclk and enabled to NULL and add an explicit error
return in the default case of the switch statement to handle invalid
clock IDs safely.
>> + switch (clk_id) {
>
>> + case LPAIF_MI2S_ECLK:
>> + sysclk = dai_data->priv[dai->id].eclk;
>> + enabled = &dai_data->priv[dai->id].eclk_enabled;
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + if (sysclk) {
>
> ...but we just fall through the switch statement if the clock ID is
> invalid and use the values.
>
ack,
will fix this in next version.
>> + for_each_child_of_node(dev->of_node, node) {
>
>> + if (IS_ERR(priv->mclk)) {
>> + if (PTR_ERR(priv->mclk) == -EPROBE_DEFER)
>> + return dev_err_probe(dev, PTR_ERR(priv->mclk),
>> + "unable to get mi2s mclk\n");
>> + priv->mclk = NULL;
>> + }
>
> Do we need to drop any OF references on early return?
Yes, an of_node_put(node) is required before the early return to avoid a
reference leak.
I will update the code to use for_each_child_of_node_scoped() to handle
the cleanup automatically, or explicitly call of_node_put() before
returning.
Thanks & Regards,
Rafi.
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: hwmon: pmbus: ti,lm25066: add current limit properties
From: sashiko-bot @ 2026-06-15 9:59 UTC (permalink / raw)
To: Potin Lai; +Cc: linux-hwmon, robh, devicetree, conor+dt
In-Reply-To: <20260615-lm25066-cl-config-v3-1-decb4f5b0b77@gmail.com>
> Add a 'ti,current-range' string property to configure the device's Current
> Limit (CL) behavior to "high" or "low" via the register, overriding the
> physical CL pin setting.
>
> This configuration is supported on LM25066, LM5064, LM5066, and LM5066i.
> LM25056 is excluded because it does not support configuring the current
> limit via the DEVICE_SETUP register (bit 2 of DEVICE_SETUP is reserved).
>
> The values "low" and "high" map to the respective low/high threshold
> voltages of the chips:
> - LM25066: low = 25 mV, high = 46 mV
> - LM5064, LM5066, LM5066i: low = 26 mV, high = 50 mV
>
> Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-lm25066-cl-config-v3-0-decb4f5b0b77@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH 7/7] ASoC: dt-bindings: ES8389: Add members about HPF and clock
From: Krzysztof Kozlowski @ 2026-06-15 9:57 UTC (permalink / raw)
To: Zhang Yi; +Cc: alsa-devel, broonie, conor+dt, devicetree, krzk+dt, robh, tiwai
In-Reply-To: <20260615060744.18775-1-zhangyi@everest-semi.com>
On 15/06/2026 08:07, Zhang Yi wrote:
>>>>> + $ref: /schemas/types.yaml#/definitions/flag
>>>>> + description:
>>>>> + Indicates that SCLK is used as the internal clock.
>>>>
>>>> And what happens with mclk in such case? Is it still wired?
>>>
>>> Yes, setting mclk-from-sclk does not affect the MCLK connection.
>>
>> I am asking about wiring of the device. If MCLK is used from SCLK, but
>> SCLK is used as the internal clock, then how can you still have MCLK
>> connected?
>
> If MCLK is derived from SCLK, whether the 'MCLK' pin on the device is
> connected or not has no effect on operation.
If MCLK is connected, why would you derive it from SCLK? That's the
point - your clocks property already might be telling everything needed
here.
>
>>>>> +
>>>>> + everest,hpfl:
>>>>> + $ref: /schemas/types.yaml#/definitions/uint8
>>>>> + description:
>>>>> + the HPF value of ADCL.
>>>>
>>>> Is HPF value in dB? If so, use proper unit suffix and proper units.
>>>
>>> No, the values here correspond to the values in the registers.
>>> The value is not in dB
>>
>> What are the meanings of the register values?
>
> The value of everest,hpfl is equal to the value of the corresponding register,
> not the cutoff frequency of the HPF.
You keep avoiding answers.
I don't want you to encode standard units as register values.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 8/8] arm64: dts: freescale: imx95: Add NXP neoisp device tree node
From: Antoine Bouyer @ 2026-06-15 9:56 UTC (permalink / raw)
To: Francesco Dolcini
Cc: julien.vuillaumier, alexi.birlinger, daniel.baluta, peng.fan,
frank.li, jacopo.mondi, laurent.pinchart, mchehab, robh, krzk+dt,
conor+dt, michael.riesch, anthony.mcgivern, linux-media,
linux-kernel, devicetree, imx, ai.luthra, paul.elder, geert,
sakari.ailus, hverkuil+cisco
In-Reply-To: <20260614090517.GA7434@francesco-nb>
On 6/14/26 11:05 AM, Francesco Dolcini wrote:
>
>
> Hello Antoine,
> thanks for your patch.
>
> On Fri, Jun 12, 2026 at 03:20:39PM +0200, Antoine Bouyer wrote:
>> Add neoisp device tree node to imx95.dtsi and enable it by default in
>> 19x19 evk board.
>>
>> Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com>
>
> ...
>
>> diff --git a/arch/arm64/boot/dts/freescale/imx95.dtsi b/arch/arm64/boot/dts/freescale/imx95.dtsi
>> index d6c549c16047..5543a6cb1250 100644
>> --- a/arch/arm64/boot/dts/freescale/imx95.dtsi
>> +++ b/arch/arm64/boot/dts/freescale/imx95.dtsi
>> @@ -1867,6 +1867,17 @@ pmu@49252000 {
>> };
>> };
>>
>> + neoisp0: isp@4ae00000 {
>> + compatible = "nxp,imx95-neoisp";
>> + reg = <0x0 0x4ae00000 0x0 0x8000>,
>> + <0x0 0x4afe0000 0x0 0x10000>;
>> + interrupts = <GIC_SPI 222 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&scmi_clk IMX95_CLK_CAMCM0>;
>> + clock-names = "camcm0";
>> + power-domains = <&scmi_devpd IMX95_PD_CAMERA>;
>> + status = "disabled";
>> + };
>
> Why the node is disabled? If the node is wholly described in
> imx95.dtsi, it should be enabled.
Hi Francesco
Thanks for your review.
Actually, all nodes are disabled in the SoC dtsi, and enabled on the
board dts file, even if fully described on the dtsi. So I used same
approach for neoisp.
BR
Antoine
>
> Francesco
>
^ permalink raw reply
* Re: [PATCH] of: property: Fix of_fwnode_get_reference_args() with negative index
From: Krzysztof Kozlowski @ 2026-06-15 9:54 UTC (permalink / raw)
To: Alban Bedel
Cc: devicetree, Rob Herring, Saravana Kannan, driver-core,
linux-kernel, Tommaso Merciai
In-Reply-To: <20260615113330.11406fbd@OMT-CWNXR4TFW5-LHT>
On 15/06/2026 11:33, Alban Bedel wrote:
> On Mon, 15 Jun 2026 08:33:32 +0200
> Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
>> On Thu, Jun 11, 2026 at 12:28:06PM +0200, Alban Bedel wrote:
>>> fwnode_property_get_reference_args() should return -ENOENT when an out
>>> of bound index is passed. An issue arised with the OF backend because
>>> the OF API use signed indexes while the fwnode API use unsigned ones.
>>> When an index value greater the INT_MAX was passed to the OF backend
>>> it got casted to a negative value and it returned -EINVAL instead of
>>
>> INT_MAX is not out of bound for this function. It is invalid value,
>> because OF code expects signed.
>
> But this is fwnode code, it use the OF API but it should implement the
> fwnode API which, unlike the OF API, use unsigned index.
>>> -ENOENT. This patch add a check to of_fwnode_get_reference_args() to
>>> catch negative index before they are passed to the OF API and return
>>> -ENOENT right away.
>>
>> I do not understand why are you fixing this issue that way. For this
>> API, the INT_MAX is correct value, but you claim that it is wrong and
>> should be ENOENT (even if there is entry).
>>
>> Fine, if this is not a correct value, then EINVAL.
>
> Indices larger than INT_MAX are valid in the fwnode API, so returning
> -EINVAL is not appropriate here.
>
Then neither ENOENT are.
But really, EINVAL is correct here. This is OF implementation, so this
implementation decides what is EINVAL and what is right. Not fwnode API.
>> But more important I think this should be just fixed in different way -
>> why index in OF calls is signed in the first place? All indices are
>> supposed to be unsigned in general, because that is both logical and
>> readable when accessing arrays.
>
> I agree that would be the better fix in the long run. But that would
> impact a lot more code and I found it difficult to ensure it would not
> potentially break some of its users.
I don't see how this fixes anything. You basically replace correct
return value to a different one.
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH v3 2/2] hwmon: (pmbus/lm25066) add current limit configuration support
From: Potin Lai @ 2026-06-15 9:49 UTC (permalink / raw)
To: Guenter Roeck, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Zev Weiss
Cc: linux-hwmon, devicetree, linux-kernel, Cosmo Chou, Mike Hsieh,
Potin Lai, Potin Lai
In-Reply-To: <20260615-lm25066-cl-config-v3-0-decb4f5b0b77@gmail.com>
Add support for the 'ti,current-range' devicetree property to configure
the current limit via the DEVICE_SETUP (0xD9) register, overriding the
physical CL pin setting.
This configuration is supported on all chips in this driver (LM25066,
LM5064, LM5066, LM5066i) except LM25056.
The property values "low" and "high" map to:
- LM25066: low = 25 mV, high = 46 mV
- LM5064, LM5066, LM5066i: low = 26 mV, high = 50 mV
The Bit 4 mapping to High/Low current limit is handled dynamically on
probe because it is swapped for LM25066 compared to the other supported
chips.
Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
---
drivers/hwmon/pmbus/lm25066.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
index dd7275a67a0a..53d274033d66 100644
--- a/drivers/hwmon/pmbus/lm25066.c
+++ b/drivers/hwmon/pmbus/lm25066.c
@@ -34,6 +34,7 @@ enum chips { lm25056, lm25066, lm5064, lm5066, lm5066i };
#define LM25066_READ_AVG_PIN 0xdf
#define LM25066_DEV_SETUP_CL BIT(4) /* Current limit */
+#define LM25066_DEV_SETUP_CL_CFG BIT(2) /* Current limit configuration */
#define LM25066_SAMPLES_FOR_AVG_MAX 4096
@@ -484,6 +485,42 @@ static int lm25066_probe(struct i2c_client *client)
data->id = (enum chips)(unsigned long)i2c_get_match_data(client);
+ if (data->id != lm25056) {
+ int config_new = config;
+ const char *cl_setting;
+ int ret;
+
+ if (!of_property_read_string(client->dev.of_node,
+ "ti,current-range", &cl_setting)) {
+ config_new |= LM25066_DEV_SETUP_CL_CFG;
+ if (strcmp(cl_setting, "high") == 0) {
+ if (data->id == lm25066)
+ config_new |= LM25066_DEV_SETUP_CL;
+ else
+ config_new &= ~LM25066_DEV_SETUP_CL;
+ } else if (strcmp(cl_setting, "low") == 0) {
+ if (data->id == lm25066)
+ config_new &= ~LM25066_DEV_SETUP_CL;
+ else
+ config_new |= LM25066_DEV_SETUP_CL;
+ } else {
+ dev_err(&client->dev,
+ "invalid current-range setting: %s\n",
+ cl_setting);
+ return -EINVAL;
+ }
+ }
+
+ if (config_new != config) {
+ ret = i2c_smbus_write_byte_data(client,
+ LM25066_DEVICE_SETUP,
+ config_new);
+ if (ret < 0)
+ return ret;
+ config = config_new;
+ }
+ }
+
info = &data->info;
info->pages = 1;
--
2.52.0
^ permalink raw reply related
* [PATCH v3 1/2] dt-bindings: hwmon: pmbus: ti,lm25066: add current limit properties
From: Potin Lai @ 2026-06-15 9:49 UTC (permalink / raw)
To: Guenter Roeck, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Zev Weiss
Cc: linux-hwmon, devicetree, linux-kernel, Cosmo Chou, Mike Hsieh,
Potin Lai, Potin Lai
In-Reply-To: <20260615-lm25066-cl-config-v3-0-decb4f5b0b77@gmail.com>
Add a 'ti,current-range' string property to configure the device's Current
Limit (CL) behavior to "high" or "low" via the register, overriding the
physical CL pin setting.
This configuration is supported on LM25066, LM5064, LM5066, and LM5066i.
LM25056 is excluded because it does not support configuring the current
limit via the DEVICE_SETUP register (bit 2 of DEVICE_SETUP is reserved).
The values "low" and "high" map to the respective low/high threshold
voltages of the chips:
- LM25066: low = 25 mV, high = 46 mV
- LM5064, LM5066, LM5066i: low = 26 mV, high = 50 mV
Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
---
.../devicetree/bindings/hwmon/pmbus/ti,lm25066.yaml | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/ti,lm25066.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/ti,lm25066.yaml
index a20f140dc79a..fe42daabaaa8 100644
--- a/Documentation/devicetree/bindings/hwmon/pmbus/ti,lm25066.yaml
+++ b/Documentation/devicetree/bindings/hwmon/pmbus/ti,lm25066.yaml
@@ -46,12 +46,32 @@ properties:
additionalProperties: false
+ ti,current-range:
+ description: |
+ Configure the current limit setting. When present, this property
+ overrides the hardware setting of the physical CL pin by configuring
+ the DEVICE_SETUP register.
+ - "low": maps to 25 mV (LM25066) or 26 mV (LM5064, LM5066, LM5066i)
+ - "high": maps to 46 mV (LM25066) or 50 mV (LM5064, LM5066, LM5066i)
+ $ref: /schemas/types.yaml#/definitions/string
+ enum:
+ - low
+ - high
+
required:
- compatible
- reg
allOf:
- $ref: /schemas/hwmon/hwmon-common.yaml#
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: ti,lm25056
+ then:
+ properties:
+ ti,current-range: false
unevaluatedProperties: false
--
2.52.0
^ permalink raw reply related
* [PATCH v3 0/2] hwmon: (pmbus/lm25066) Support SMBus Current Limit configuration
From: Potin Lai @ 2026-06-15 9:49 UTC (permalink / raw)
To: Guenter Roeck, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Zev Weiss
Cc: linux-hwmon, devicetree, linux-kernel, Cosmo Chou, Mike Hsieh,
Potin Lai, Potin Lai
This series adds support for configuring the current limit behavior via
software override on LM25066-compatible devices (excluding LM25056) using
the DEVICE_SETUP (0xD9) register.
When the 'ti,current-range' property is specified in the device tree,
the driver configures the DEVICE_SETUP register's Current Limit Configuration
bit (bit 2) to activate SMBus/software override and sets the Current Limit
Setting bit (bit 4) to "low" or "high" threshold accordingly.
Since LM25056 does not support software override (bit 2 of DEVICE_SETUP is
reserved), it is explicitly excluded from this support in both the device
tree binding schema and the driver.
---
Changes in v3:
- Renamed property from 'ti,current-limit' to 'ti,current-range' to
resolve the global schema type conflict.
- Updated commit messages and bindings description to document supported
devices and their physical voltage mappings for the low/high settings.
- Link to v2: https://patch.msgid.link/20260615-lm25066-cl-config-v2-0-59be46e67d5a@gmail.com
Changes in v2:
- Replaced the boolean properties ('ti,cl-smbus-high' and 'ti,cl-smbus-low')
with a single string property 'ti,current-limit' ('low' or 'high')
- Excluded lm25056 in the driver from parsing/setting the current limit property.
- Link to v1: https://patch.msgid.link/20260611-lm25066-cl-config-v1-0-02e567bf3d91@gmail.com
To: Guenter Roeck <linux@roeck-us.net>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Zev Weiss <zev@bewilderbeest.net>
Cc: linux-hwmon@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Cosmo Chou <cosmo.chou@quantatw.com>
Cc: Mike Hsieh <Mike_Hsieh@quantatw.com>
Cc: Potin Lai <potin.lai@quantatw.com>
Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
---
Potin Lai (2):
dt-bindings: hwmon: pmbus: ti,lm25066: add current limit properties
hwmon: (pmbus/lm25066) add current limit configuration support
.../bindings/hwmon/pmbus/ti,lm25066.yaml | 20 ++++++++++++
drivers/hwmon/pmbus/lm25066.c | 37 ++++++++++++++++++++++
2 files changed, 57 insertions(+)
---
base-commit: 05f7e89ab9731565d8a62e3b5d1ec206485eeb0b
change-id: 20260611-lm25066-cl-config-f81925f7337e
Best regards,
--
Potin Lai <potin.lai.pt@gmail.com>
^ permalink raw reply
* Re: [PATCH net-next v2 2/2] net: pse-pd: add Realtek/Broadcom PSE MCU driver
From: Jonas Jelonek @ 2026-06-15 9:50 UTC (permalink / raw)
To: Oleksij Rempel
Cc: kory.maincent, andrew+netdev, davem, edumazet, kuba, pabeni, robh,
krzk+dt, conor+dt, netdev, devicetree, linux-kernel, daniel,
bjorn, Simon Horman
In-Reply-To: <ai_ITHd_xxt7an4q@pengutronix.de>
Hi Oleksij,
thanks for your message.
On 15.06.26 11:39, Oleksij Rempel wrote:
> Hi Jonas,
>
> On Mon, Jun 15, 2026 at 10:07:33AM +0100, Simon Horman wrote:
>> This is an AI-generated review of your patch. The human sending this
>> email has considered the AI review valid, or at least plausible.
>> Full review at: https://sashiko.dev
> It probably sounds scary, but sashiko finds not all issues in one time.
> Bigger patches and more different included frameworks - increase findings
> probability a lot. With other words, it may more rounds than expected.
Sure, I've noticed that in another series recently. I'll just be patient and
address/discuss the review/issues accordingly :)
> You may optimize it if you have access to some free or payed tokes,
> by using sashik-cli with LLM of you choice as backend.
>
> Best Regards,
> Oleksij
Best regards,
Jonas
^ permalink raw reply
* Re: [PATCH net-next v2 2/2] net: pse-pd: add Realtek/Broadcom PSE MCU driver
From: Jonas Jelonek @ 2026-06-15 9:47 UTC (permalink / raw)
To: Simon Horman
Cc: o.rempel, kory.maincent, andrew+netdev, davem, edumazet, kuba,
pabeni, robh, krzk+dt, conor+dt, netdev, devicetree, linux-kernel,
daniel, bjorn
In-Reply-To: <20260615090733.739423-2-horms@kernel.org>
Hi,
On 15.06.26 11:07, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> net: pse-pd: add Realtek/Broadcom PSE MCU driver
>
> This patch adds a driver for a microcontroller that fronts Realtek and
> Broadcom PSE silicon on PoE switches, exposing the MCU over I2C/SMBus or
> UART using a 12-byte fixed-frame protocol. The driver is split into a
> shared core plus two transport modules, with a per-dialect opcode table
> and parser hooks for the Realtek vs Broadcom variants.
>
>> diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig
>> index 7ef29657ee5d..b065b19db126 100644
>> --- a/drivers/net/pse-pd/Kconfig
>> +++ b/drivers/net/pse-pd/Kconfig
>> @@ -13,6 +13,34 @@ menuconfig PSE_CONTROLLER
>>
>> if PSE_CONTROLLER
>>
>> +config PSE_REALTEK
>> + tristate
>> + help
>> + Shared core for the Realtek/Broadcom PSE MCU driver. This is
>> + selected automatically by the transport options below.
>> +
>> +config PSE_REALTEK_I2C
>> + tristate "Realtek/Broadcom PSE MCU driver (I2C transport)"
>> + depends on I2C
>> + select PSE_REALTEK
>> + help
>> + Driver for the microcontroller (MCU) that fronts the PSE
>> + hardware on switches with Realtek or Broadcom PSE chips, attached
>> + via I2C/SMBus. The MCU exposes a message-based protocol; the actual
>> + PSE silicon is not accessed directly. To compile this driver as a
>> + module, choose M here: the module will be called realtek-pse-i2c.
>> +
>> +config PSE_REALTEK_UART
>> + tristate "Realtek/Broadcom PSE MCU driver (UART transport)"
>> + depends on SERIAL_DEV_BUS
>> + select PSE_REALTEK
>> + help
>> + Driver for the microcontroller (MCU) that fronts the PSE
>> + hardware on switches with Realtek or Broadcom PSE chips, attached
>> + via UART. The MCU exposes a message-based protocol; the actual PSE
>> + silicon is not accessed directly. To compile this driver as a
>> + module, choose M here: the module will be called realtek-pse-uart.
>> +
> [Medium]
> The realtek, vendor prefix is used for both the Realtek dialect and the
> Broadcom dialect compatibles (realtek,pse-mcu-rtk and
> realtek,pse-mcu-bcm), and the binding documentation explicitly notes
> that the prefix "reflects the platform" rather than the silicon vendor.
> DT compatibles are stable ABI once accepted.
>
> Would a brcm, prefix (or per-MCU-part compatibles) for the Broadcom
> dialect be more appropriate, given that the BCM variant uses Broadcom
> PSE silicon driven by an MCU built by Nuvoton or STMicro and has no
> Realtek involvement?
>
> Similarly, would describing the I2C wire framing (native vs SMBus) via
> separate compatibles be preferable to encoding it in the
> realtek,i2c-protocol DT property, and would describing the actual PSE
> chip in DT (rather than relying on runtime device-id detection) match
> the usual binding patterns?
This has been the hardest part so far but the solution I have right now
seems the "best" one so far. But this part needs some guidance from
DT maintainers.
I would hold against the AIs suggestions due to particular reasons. Using
specific compatibles for the PSE chips would be very wrong IMO because:
(1) It would claim those PSE chips are what is interfaced on the bus. But
this is not true, there's only the MCU which mostly hides the PSE chips.
(2) The PSE chips could be used without such an MCU. While this is rather
theoretical for Realtek PSE chips, it is definitely the case for the
Broadcom PSE chips. And in that case, they even use a different
for communication, called Broadcom Serial Control.
On the other side, using specific MCU compatibles would be wrong too:
(1) The MCU silicon is irrelevant to the PSE/PoE system. Those are just
general purpose MCUs where the supplied firmware on that MCU,
not the silicon, defines the interface to the host.
(2) The MCUs are from various vendors (here STMicro, GigaDevice,
Nuvoton), since they are general purpose they're likely used in various
other applications. Claiming compatibles here for a specific
application is IMO wrong.
Finally, why using the Realtek prefix/scope: The system that is described
by the bindings and supported by the driver has so far been seen solely
on Realtek-based switches. Some SDK sources already include a driver
implementation for that. Though there is no 100% evidence of that, it
seems quite reasonable that Realtek provides this MCU+firmware+PSE
silicon as kind of a package. Thus, Realtek defines and provides the
interface and behavior. The suffix then just denotes the protocol dialect
used, correlating with the PSE silicon behind the MCU.
In older switch generations, Realtek just decided to use BCM59xxx PSE
chips until they developed and shipped their owns. Likely because
the capabilities of BCM and Realtek PSE silicon slightly differs, they
decided to alter the protocol with different opcodes and slightly
different command set.
I've seen a similar review from Sashiko on the bindings patch, my
explanation above applies to it in the same way.
I'll try to improve the rationale a bit in the descriptions for the next
version.
>> config PSE_REGULATOR
>> tristate "Regulator based PSE controller"
>> help
> [ ... ]
>
>> diff --git a/drivers/net/pse-pd/realtek-pse-core.c b/drivers/net/pse-pd/realtek-pse-core.c
>> new file mode 100644
>> index 000000000000..3a601cfb6280
>> --- /dev/null
>> +++ b/drivers/net/pse-pd/realtek-pse-core.c
> [ ... ]
>
>> +static int rtpse_port_cmd(struct rtpse_ctrl *pse, unsigned int port, u8 opcode, u8 arg)
>> +{
>> + struct rtpse_mcu_msg req, resp;
>> + int ret;
>> +
>> + rtpse_mcu_msg_init(&req, opcode);
>> + req.payload[0] = port;
>> + req.payload[1] = arg;
>> +
>> + ret = rtpse_do_xfer(pse, &req, &resp);
>> + if (ret)
>> + return ret;
>> +
>> + if (resp.payload[0] != port || resp.payload[1] != 0)
>> + return -EIO;
>> +
>> + return 0;
>> +}
> [Medium]
> Can an all-zero 12-byte buffer be silently accepted as a successful
> PORT_ENABLE/PORT_DISABLE on the BCM dialect for port 0?
>
> For the BCM dialect, RTPSE_CMD_PORT_ENABLE maps to opcode 0x00, and the
> checksum of eleven zero bytes is 0, so an all-zero response passes the
> opcode and checksum checks in rtpse_do_xfer():
>
> if (resp->opcode != req->opcode ||
> resp->checksum != rtpse_checksum((u8 *)resp, RTPSE_MCU_MSG_SIZE - 1))
> return -EBADMSG;
>
> rtpse_port_cmd() then verifies payload[0] == port and payload[1] == 0,
> both of which pass when port == 0 and arg is 0 (disable) or when arg is
> 1 but the MCU never observed the command and the buffer is all zero
> anyway since payload[1] is checked against 0.
>
> The seq_num field exists in struct rtpse_mcu_msg and is filled with 0xff
> by rtpse_mcu_msg_init() but is never checked on the response side. Could
> a seq_num round-trip check (or a non-zero opcode sentinel for the BCM
> dialect) reject stale or zeroed-out frames here?
>
> [ ... ]
>
Valid, I will add a check for the seq_num field here.
>> +static int rtpse_discover(struct rtpse_ctrl *pse, struct rtpse_mcu_info *info)
>> +{
>> + struct rtpse_mcu_ext_config ext_config;
>> + unsigned long deadline;
>> + int ret;
>> +
>> + /*
>> + * The MCU may not answer on the bus yet right after power-up or
>> + * enable-gpios assertion: depending on the transport it either stays
>> + * silent (-ETIMEDOUT) or does not ACK its address at all (-ENXIO /
>> + * -EREMOTEIO). Retry within a bounded wall-time window so a slow boot
>> + * still probes, while a genuinely unresponsive MCU fails with its real
>> + * error instead of deferring forever and masking it.
>> + */
>> + deadline = jiffies + msecs_to_jiffies(RTPSE_MCU_BOOT_TIMEOUT_MS);
>> + do {
>> + ret = rtpse_mcu_get_info(pse, info);
>> + if (ret != -ETIMEDOUT && ret != -ENXIO &&
>> + ret != -EREMOTEIO && ret != -EAGAIN)
>> + break;
>> + msleep(RTPSE_MCU_BOOT_RETRY_MS);
>> + } while (time_before(jiffies, deadline));
> [Low]
> The comment says the loop is meant to tolerate a slow-boot MCU, but the
> retry whitelist excludes -EBADMSG and -EIO.
>
> rtpse_do_xfer() returns -EBADMSG when the opcode or checksum mismatch
> fires, and rtpse_port_query()/rtpse_port_cmd() return -EIO when the
> echoed port byte does not match the request. On a UART-attached MCU
> that emits a single garbled frame while its serial peripheral
> stabilises, or on an I2C-attached MCU that returns stale boot-ROM bytes
> right after enable-gpios assertion, the first attempt would surface as
> one of those errors rather than -ETIMEDOUT or -ENXIO, and the loop
> would break out immediately.
>
> Should -EBADMSG (and arguably -EIO) be added to the retry set so the
> documented intent matches the actual behaviour?
This would be speculative IMO, because 'no answer' was the only behavior
I could observe so far. It always came up as a timeout when the MCU
wasn't ready yet.
> [ ... ]
>
>> +int rtpse_register(struct rtpse_ctrl *pse)
>> +{
> [ ... ]
>
>> + if (!info.system_enable) {
>> + ret = rtpse_set_global_state(pse, true);
>> + /* Dialects without a global-state concept (e.g. BCM) return
>> + * -EOPNOTSUPP; treat that as "no separate enable required".
>> + */
>> + if (ret && ret != -EOPNOTSUPP)
>> + return dev_err_probe(pse->dev, ret,
>> + "failed to enable PSE system\n");
>> + }
>> +
>> + ret = regulator_enable(pse->poe_supply);
>> + if (ret)
>> + return dev_err_probe(pse->dev, ret, "failed to enable PoE supply\n");
>> +
>> + ret = devm_add_action_or_reset(pse->dev, rtpse_regulator_disable, pse->poe_supply);
>> + if (ret)
>> + return ret;
> [Low]
> If rtpse_set_global_state(pse, true) succeeds but a later step in
> rtpse_register() fails (regulator_enable, devm_add_action_or_reset, or
> devm_pse_controller_register), is the MCU left with the PSE subsystem
> enabled while the kernel detaches?
>
> The regulator is rolled back via rtpse_regulator_disable through
> devm_add_action_or_reset, but there does not seem to be a matching
> devm_add_action_or_reset to call rtpse_set_global_state(false) on
> probe-failure or driver-removal. Is that asymmetry intentional?
No, this is an inconsistency. I'll fix this in the next version.
> [ ... ]
>
>> diff --git a/drivers/net/pse-pd/realtek-pse-uart.c b/drivers/net/pse-pd/realtek-pse-uart.c
>> new file mode 100644
>> index 000000000000..785407b379a4
>> --- /dev/null
>> +++ b/drivers/net/pse-pd/realtek-pse-uart.c
> [ ... ]
>
>> +static size_t rtpse_uart_receive(struct serdev_device *serdev,
>> + const u8 *buf, size_t count)
>> +{
>> + struct rtpse_uart *ctx = serdev_device_get_drvdata(serdev);
>> + bool done = false;
>> + size_t take;
>> +
>> + scoped_guard(spinlock_irqsave, &ctx->rx_lock) {
>> + take = min(count, sizeof(ctx->rx_buf) - ctx->rx_len);
>> + if (take) {
>> + memcpy(ctx->rx_buf + ctx->rx_len, buf, take);
>> + ctx->rx_len += take;
>> + done = (ctx->rx_len == sizeof(ctx->rx_buf));
>> + }
>> + }
>> + if (done)
>> + complete(&ctx->rx_done);
>> +
>> + /* consume all to avoid desync/misalignment */
>> + return count;
>> +}
> [ ... ]
>
>> +static int rtpse_uart_send(struct rtpse_ctrl *pse, const struct rtpse_mcu_msg *req)
>> +{
>> + struct rtpse_uart *ctx = to_rtpse_uart(pse);
>> + int written;
>> +
>> + /* clear any leftover rx state before transmitting */
>> + reinit_completion(&ctx->rx_done);
>> + scoped_guard(spinlock_irqsave, &ctx->rx_lock)
>> + ctx->rx_len = 0;
> [High]
> Is there a race between reinit_completion() here and the asynchronous
> serdev receive_buf callback when a previous transaction has timed out
> with bytes still in flight?
>
> reinit_completion() is an unlocked x->done = 0 write, and the matching
> complete() in rtpse_uart_receive() is called outside rx_lock. The
> following interleave seems possible:
>
> rtpse_uart_send() runs reinit_completion() /* done = 0 */
> rtpse_uart_receive() takes rx_lock, fills rx_buf
> to 12 bytes, computes done=true, releases lock
> rtpse_uart_send() takes rx_lock, sets rx_len = 0,
> releases lock
> rtpse_uart_receive() calls complete(&ctx->rx_done) /* done = 1 */
> rtpse_uart_send() writes the new request
> rtpse_uart_recv() wait_for_completion_timeout()
> returns immediately (done was 1), then sees
> rx_len == 0 and returns -EIO
>
> rtpse_discover()'s retry whitelist covers -ETIMEDOUT, -ENXIO,
> -EREMOTEIO, and -EAGAIN but not -EIO, so a single occurrence of this
> race during the boot retry window would break out of the loop and turn
> into a permanent dev_err_probe() failure.
>
> Would moving complete() inside rx_lock in rtpse_uart_receive(), or
> performing reinit_completion() and rx_len = 0 together inside rx_lock
> while excluding the completer, close this window?
>
Valid, will fix it as suggested in the next version.
>> +
>> + written = serdev_device_write(ctx->serdev, (const u8 *)req, sizeof(*req),
>> + RTPSE_UART_TX_TIMEOUT);
>> + if (written < 0)
>> + return written;
>> + if (written != sizeof(*req))
>> + return -EIO;
>> +
>> + return 0;
>> +}
> [ ... ]
Best regards,
Jonas
^ permalink raw reply
* Re: [PATCH v2 2/3] ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control
From: Mohammad Rafi Shaik @ 2026-06-15 9:46 UTC (permalink / raw)
To: Val Packett, Srinivas Kandagatla, Liam Girdwood, Mark Brown,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jaroslav Kysela,
Takashi Iwai
Cc: Krzysztof Kozlowski, linux-arm-msm, linux-sound, devicetree,
linux-kernel, Srinivas Kandagatla
In-Reply-To: <8b2318ff-d07c-4c2d-9c2d-ef7c5c763096@packett.cool>
On 6/8/2026 8:19 AM, Val Packett wrote:
>
> On 6/7/26 11:30 PM, Mohammad Rafi Shaik wrote:
>> Add support for MI2S clock control within q6apm-lpass DAIs, including
>> handling of MCLK, BCLK, and ECLK via the DAI .set_sysclk callback.
>> Each MI2S port now retrieves its clock handles from the device tree,
>> allowing per-port clock configuration and proper enable/disable during
>> startup and shutdown.
>> [..]
>> @@ -297,6 +373,59 @@ static const struct snd_soc_component_driver
>> q6apm_lpass_dai_component = {
>> .remove_order = SND_SOC_COMP_ORDER_FIRST,
>> };
>> +static int of_q6apm_parse_dai_data(struct device *dev,
>> + struct q6apm_lpass_dai_data *data)
>> +{
>> + struct device_node *node;
>> + int ret;
>> +
>> + for_each_child_of_node(dev->of_node, node) {
>> + struct q6apm_dai_priv_data *priv;
>> + int id;
>> +
>> + ret = of_property_read_u32(node, "reg", &id);
>> + if (ret || id < 0 || id >= APM_PORT_MAX) {
>> + dev_err(dev, "valid dai id not found:%d\n", ret);
>> + continue;
>> + }
>> +
>> + switch (id) {
>> + /* MI2S specific properties */
>> + case PRIMARY_MI2S_RX ... QUATERNARY_MI2S_TX:
>> + case QUINARY_MI2S_RX ... QUINARY_MI2S_TX:
>
> SENARY is also a thing these days btw..
>
Ack, will include SENARY dai also.
Thanks & Regards,
Rafi.
>
> ~val
>
^ permalink raw reply
* Re: [PATCH net-next v2 2/2] net: pse-pd: add Realtek/Broadcom PSE MCU driver
From: Oleksij Rempel @ 2026-06-15 9:39 UTC (permalink / raw)
To: Simon Horman
Cc: jelonek.jonas, kory.maincent, andrew+netdev, davem, edumazet,
kuba, pabeni, robh, krzk+dt, conor+dt, netdev, devicetree,
linux-kernel, daniel, bjorn
In-Reply-To: <20260615090733.739423-2-horms@kernel.org>
Hi Jonas,
On Mon, Jun 15, 2026 at 10:07:33AM +0100, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
It probably sounds scary, but sashiko finds not all issues in one time.
Bigger patches and more different included frameworks - increase findings
probability a lot. With other words, it may more rounds than expected.
You may optimize it if you have access to some free or payed tokes,
by using sashik-cli with LLM of you choice as backend.
Best Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH v4 2/7] dt-bindings: display: bridge: Document Renesas R-Car V4H DSC bindings
From: Conor Dooley @ 2026-06-15 9:38 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Geert Uytterhoeven, Michael Turquette, Stephen Boyd,
Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Magnus Damm, Marek Vasut,
Laurent Pinchart, Kieran Bingham, Philipp Zabel,
linux-renesas-soc, linux-clk, linux-kernel, dri-devel, devicetree
In-Reply-To: <20260615-rcar-du-dsc-v4-2-93096a1b56a3@ideasonboard.com>
[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]
On Mon, Jun 15, 2026 at 09:28:07AM +0300, Tomi Valkeinen wrote:
> From: Marek Vasut <marek.vasut+renesas@mailbox.org>
>
> The Renesas DSC Display Stream Compression is a bridge embedded in the
> Renesas R-Car V4H SoC. The bridge performs VESA DSC encoding of up to
> 8k or 400 Mpixel/s .
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> [tomi.valkeinen: fix the example]
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> .../bindings/display/bridge/renesas,dsc.yaml | 99 ++++++++++++++++++++++
> 1 file changed, 99 insertions(+)
> +properties:
> + compatible:
> + items:
> + - enum:
> + - renesas,r8a779g0-dsc
> + - const: renesas,rcar-dsc
I didn't provide an ack for this FYI, I was giving the ack for the
file being called renesas,r8a779g0-dsc.yaml. Since Geert isn't happy
with what's here, could you drop my ack when you send another version?
Cheers,
Conor.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] ASoC: qcom: audioreach: compute active channel maps from channel_map
From: Srinivas Kandagatla @ 2026-06-15 9:36 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: <a5a957d0-a40c-424e-9d6d-622a4f624343@linaro.org>
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
--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] of: property: Fix of_fwnode_get_reference_args() with negative index
From: Alban Bedel @ 2026-06-15 9:33 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: devicetree, Rob Herring, Saravana Kannan, driver-core,
linux-kernel, Tommaso Merciai, Alban Bedel
In-Reply-To: <20260615-obedient-axolotl-of-argument-fb55ef@quoll>
On Mon, 15 Jun 2026 08:33:32 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Thu, Jun 11, 2026 at 12:28:06PM +0200, Alban Bedel wrote:
> > fwnode_property_get_reference_args() should return -ENOENT when an out
> > of bound index is passed. An issue arised with the OF backend because
> > the OF API use signed indexes while the fwnode API use unsigned ones.
> > When an index value greater the INT_MAX was passed to the OF backend
> > it got casted to a negative value and it returned -EINVAL instead of
>
> INT_MAX is not out of bound for this function. It is invalid value,
> because OF code expects signed.
But this is fwnode code, it use the OF API but it should implement the
fwnode API which, unlike the OF API, use unsigned index.
> > -ENOENT. This patch add a check to of_fwnode_get_reference_args() to
> > catch negative index before they are passed to the OF API and return
> > -ENOENT right away.
>
> I do not understand why are you fixing this issue that way. For this
> API, the INT_MAX is correct value, but you claim that it is wrong and
> should be ENOENT (even if there is entry).
>
> Fine, if this is not a correct value, then EINVAL.
Indices larger than INT_MAX are valid in the fwnode API, so returning
-EINVAL is not appropriate here.
> But more important I think this should be just fixed in different way -
> why index in OF calls is signed in the first place? All indices are
> supposed to be unsigned in general, because that is both logical and
> readable when accessing arrays.
I agree that would be the better fix in the long run. But that would
impact a lot more code and I found it difficult to ensure it would not
potentially break some of its users.
Alban
^ permalink raw reply
* Re: [PATCH v5 5/9] block: implement NVMEM provider
From: Loic Poulain @ 2026-06-15 9:33 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: linux-mmc, devicetree, linux-kernel, linux-arm-msm, linux-block,
linux-wireless, ath10k, linux-bluetooth, netdev, daniel,
Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Jens Axboe, Johannes Berg,
Jeff Johnson, Marcel Holtmann, Luiz Augusto von Dentz,
Balakrishna Godavarthi, Rocky Liao, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Srinivas Kandagatla,
Andrew Lunn, Heiner Kallweit, Russell King, Saravana Kannan
In-Reply-To: <CAFEp6-0qsqhcwnSjm3=bG21jsCktzn5-L5sk2pNTZcGuVXaiNA@mail.gmail.com>
On Mon, Jun 15, 2026 at 11:28 AM Loic Poulain
<loic.poulain@oss.qualcomm.com> wrote:
>
> On Mon, Jun 15, 2026 at 10:53 AM Bartosz Golaszewski <brgl@kernel.org> wrote:
> >
> > On Fri, 12 Jun 2026 15:20:57 +0200, Loic Poulain
> > <loic.poulain@oss.qualcomm.com> said:
> > > From: Daniel Golle <daniel@makrotopia.org>
> > >
> > > On embedded devices using an eMMC it is common that one or more partitions
> > > on the eMMC are used to store MAC addresses and Wi-Fi calibration EEPROM
> > > data. Allow referencing the partition in device tree for the kernel and
> > > Wi-Fi drivers accessing it via the NVMEM layer.
> > >
> > > For now, NVMEM is only registered for the whole disk block device, as the
> > > OF node is currently only associated to it.
> > >
> > > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > > Co-developed-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> > > Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> > > ---
> > > block/Kconfig | 9 ++++
> > > block/Makefile | 1 +
> > > block/blk-nvmem.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++
> > > block/blk.h | 8 ++++
> > > block/genhd.c | 4 ++
> > > include/linux/blk_types.h | 3 ++
> > > include/linux/blkdev.h | 1 +
> > > 7 files changed, 135 insertions(+)
> > >
> > > diff --git a/block/Kconfig b/block/Kconfig
> > > index 15027963472d7b40e27b9097a5993c457b5b3054..0b33747e16dc33473683706f75c92bdf8b648f7c 100644
> > > --- a/block/Kconfig
> > > +++ b/block/Kconfig
> > > @@ -209,6 +209,15 @@ config BLK_INLINE_ENCRYPTION_FALLBACK
> > > by falling back to the kernel crypto API when inline
> > > encryption hardware is not present.
> > >
> > > +config BLK_NVMEM
> > > + bool "Block device NVMEM provider"
> > > + depends on OF
> > > + depends on NVMEM
> > > + help
> > > + Allow block devices (or partitions) to act as NVMEM providers,
> > > + typically used with eMMC to store MAC addresses or Wi-Fi
> > > + calibration data on embedded devices.
> > > +
> > > source "block/partitions/Kconfig"
> > >
> > > config BLK_PM
> > > diff --git a/block/Makefile b/block/Makefile
> > > index 7dce2e44276c4274c11a0a61121c83d9c43d6e0c..d7ac389e71902bc091a8800ea266190a43b3e63d 100644
> > > --- a/block/Makefile
> > > +++ b/block/Makefile
> > > @@ -36,3 +36,4 @@ obj-$(CONFIG_BLK_INLINE_ENCRYPTION) += blk-crypto.o blk-crypto-profile.o \
> > > blk-crypto-sysfs.o
> > > obj-$(CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK) += blk-crypto-fallback.o
> > > obj-$(CONFIG_BLOCK_HOLDER_DEPRECATED) += holder.o
> > > +obj-$(CONFIG_BLK_NVMEM) += blk-nvmem.o
> > > diff --git a/block/blk-nvmem.c b/block/blk-nvmem.c
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..c005f059d9fe56242ebaef9905673dff902b5686
> > > --- /dev/null
> > > +++ b/block/blk-nvmem.c
> > > @@ -0,0 +1,109 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * block device NVMEM provider
> > > + *
> > > + * Copyright (c) 2024 Daniel Golle <daniel@makrotopia.org>
> > > + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> > > + *
> > > + * Useful on devices using a partition on an eMMC for MAC addresses or
> > > + * Wi-Fi calibration EEPROM data.
> > > + */
> > > +
> > > +#include <linux/file.h>
> > > +#include <linux/nvmem-provider.h>
> > > +#include <linux/nvmem-consumer.h>
> > > +#include <linux/of.h>
> > > +#include <linux/pagemap.h>
> > > +#include <linux/property.h>
> > > +
> > > +#include "blk.h"
> > > +
> > > +static int blk_nvmem_reg_read(void *priv, unsigned int from, void *val, size_t bytes)
> > > +{
> > > + blk_mode_t mode = BLK_OPEN_READ | BLK_OPEN_RESTRICT_WRITES;
> > > + dev_t devt = (dev_t)(uintptr_t)priv;
> > > + size_t bytes_left = bytes;
> > > + loff_t pos = from;
> > > + int ret = 0;
> > > +
> > > + struct file *bdev_file __free(fput) = bdev_file_open_by_dev(devt, mode, priv, NULL);
> > > + if (IS_ERR(bdev_file))
> > > + return PTR_ERR(bdev_file);
> > > +
> > > + while (bytes_left) {
> > > + pgoff_t f_index = pos >> PAGE_SHIFT;
> > > + struct folio *folio;
> > > + size_t folio_off;
> > > + size_t to_read;
> > > +
> > > + folio = read_mapping_folio(bdev_file->f_mapping, f_index, NULL);
> > > + if (IS_ERR(folio)) {
> > > + ret = PTR_ERR(folio);
> > > + break;
> > > + }
> > > +
> > > + folio_off = offset_in_folio(folio, pos);
> > > + to_read = min(bytes_left, folio_size(folio) - folio_off);
> > > + memcpy_from_folio(val, folio, folio_off, to_read);
> > > + pos += to_read;
> > > + bytes_left -= to_read;
> > > + val += to_read;
> > > + folio_put(folio);
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +void blk_nvmem_add(struct block_device *bdev)
> > > +{
> > > + struct device *dev = &bdev->bd_device;
> > > + struct nvmem_config config = {};
> > > +
> > > + /* skip devices which do not have a device tree node */
> > > + if (!dev_of_node(dev))
> > > + return;
> > > +
> > > + /* skip devices without an nvmem layout defined */
> > > + struct device_node *child __free(device_node) =
> > > + of_get_child_by_name(dev_of_node(dev), "nvmem-layout");
> > > + if (!child)
> > > + return;
> > > +
> > > + /*
> > > + * skip block device too large to be represented as NVMEM devices,
> > > + * the NVMEM reg_read callback uses an unsigned int offset
> > > + */
> > > + if (bdev_nr_bytes(bdev) > UINT_MAX) {
> > > + dev_warn(dev, "block device too large to be an NVMEM provider\n");
> > > + return;
> > > + }
> > > +
> > > + config.id = NVMEM_DEVID_NONE;
> > > + config.dev = dev;
> > > + config.name = dev_name(dev);
> > > + config.owner = THIS_MODULE;
> > > + config.priv = (void *)(uintptr_t)dev->devt;
> > > + config.reg_read = blk_nvmem_reg_read;
> > > + config.size = bdev_nr_bytes(bdev);
> > > + config.word_size = 1;
> > > + config.stride = 1;
> > > + config.read_only = true;
> > > + config.root_only = true;
> > > + config.ignore_wp = true;
> > > + config.of_node = to_of_node(dev->fwnode);
> > > +
> > > + bdev->bd_nvmem = nvmem_register(&config);
> > > + if (IS_ERR(bdev->bd_nvmem)) {
> > > + dev_err_probe(dev, PTR_ERR(bdev->bd_nvmem),
> > > + "Failed to register NVMEM device\n");
> >
> > Using dev_err_probe() only makes sense with a return value. Which makes me
> > think: we won't retry this after a probe deferral. I think we should return
>
> Yes, so here with the nvmem fixed-layout, there is no way to get a
> deferred probe error, but better to be ready to handle this anyway.
>
> > int from this function just for this use-case. Also: if we *do* have
> > a layout, shouldn't we treat a failure to register the nvmem provider as
> > a an error and propagate it up the stack?
>
> From an API perspective we should indeed return the error. From block
> core, Do we want to fail the entire disk addition just because the
> 'companion' NVMEM provider couldn't be registered, or should we only
> abort/return in case of EPROBE_DEFER?
Also we cannot safely return -EPROBE_DEFER from add_disk_final()
either. The NVMEM registration point is late in the sequence, too much
has already happened to easily unwind. The easiest is that the NVMEM
simply won't be available if registration fails, which looks
acceptable?
>
> >
> > > + bdev->bd_nvmem = NULL;
> > > + }
> > > +}
> > > +
> > > +void blk_nvmem_del(struct block_device *bdev)
> > > +{
> > > + if (bdev->bd_nvmem)
> >
> > Nvmem core already performs a NULL check.
>
> Ok, thanks!
>
>
> >
> > > + nvmem_unregister(bdev->bd_nvmem);
> > > +
> > > + bdev->bd_nvmem = NULL;
> > > +}
> > > diff --git a/block/blk.h b/block/blk.h
> > > index ec4674cdf2ead4fd259ff5fc42401f591e684ee9..cd3c7ca723391c40be56f1dd4810e641b7c8a2b3 100644
> > > --- a/block/blk.h
> > > +++ b/block/blk.h
> > > @@ -757,4 +757,12 @@ static inline void blk_debugfs_unlock(struct request_queue *q,
> > > memalloc_noio_restore(memflags);
> > > }
> > >
> > > +#ifdef CONFIG_BLK_NVMEM
> > > +void blk_nvmem_add(struct block_device *bdev);
> > > +void blk_nvmem_del(struct block_device *bdev);
> > > +#else
> > > +static inline void blk_nvmem_add(struct block_device *bdev) {}
> > > +static inline void blk_nvmem_del(struct block_device *bdev) {}
> > > +#endif
> > > +
> > > #endif /* BLK_INTERNAL_H */
> > > diff --git a/block/genhd.c b/block/genhd.c
> > > index 7d6854fd28e95ae9134309679a7c6a937f5b7db8..1b2382de6fb30c1e5f60f45c04dc03ed3bf5d5f2 100644
> > > --- a/block/genhd.c
> > > +++ b/block/genhd.c
> > > @@ -421,6 +421,8 @@ static void add_disk_final(struct gendisk *disk)
> > > */
> > > dev_set_uevent_suppress(ddev, 0);
> > > disk_uevent(disk, KOBJ_ADD);
> > > +
> > > + blk_nvmem_add(disk->part0);
> > > }
> > >
> > > blk_apply_bdi_limits(disk->bdi, &disk->queue->limits);
> > > @@ -704,6 +706,8 @@ static void __del_gendisk(struct gendisk *disk)
> > >
> > > disk_del_events(disk);
> > >
> > > + blk_nvmem_del(disk->part0);
> > > +
> > > /*
> > > * Prevent new openers by unlinked the bdev inode.
> > > */
> > > diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> > > index 8808ee76e73c09e0ceaac41ba59e86fb0c4efc64..ace6f59b860d0813665b2f62a1c03a1f4be94059 100644
> > > --- a/include/linux/blk_types.h
> > > +++ b/include/linux/blk_types.h
> > > @@ -73,6 +73,9 @@ struct block_device {
> > > int bd_writers;
> > > #ifdef CONFIG_SECURITY
> > > void *bd_security;
> > > +#endif
> > > +#ifdef CONFIG_BLK_NVMEM
> > > + struct nvmem_device *bd_nvmem;
> > > #endif
> > > /*
> > > * keep this out-of-line as it's both big and not needed in the fast
> > > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > > index 890128cdea1ce66863c5baa36f3b336ec4550807..f15d2b5bf9e4fd2368b8a70416a978e22c0d4333 100644
> > > --- a/include/linux/blkdev.h
> > > +++ b/include/linux/blkdev.h
> > > @@ -30,6 +30,7 @@
> > >
> > > struct module;
> > > struct request_queue;
> > > +struct nvmem_device;
> > > struct elevator_queue;
> > > struct blk_trace;
> > > struct request;
> > >
> > > --
> > > 2.34.1
> > >
> > >
> >
> > I like this approach better than the previous one.
> >
> > Thanks,
> > Bartosz
^ permalink raw reply
* Re: [PATCH 1/4] ASoC: qcom: audioreach: compute active channel maps from channel_map
From: Neil Armstrong @ 2026-06-15 9:31 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,
Srinivas Kandagatla
In-Reply-To: <937aed10-9ec6-4ca4-bc60-db892121a416@kernel.org>
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.
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 v4 2/7] dt-bindings: display: bridge: Document Renesas R-Car V4H DSC bindings
From: Geert Uytterhoeven @ 2026-06-15 9:24 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Michael Turquette, Stephen Boyd, Andrzej Hajda, Neil Armstrong,
Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Magnus Damm, Marek Vasut, Laurent Pinchart, Kieran Bingham,
Philipp Zabel, linux-renesas-soc, linux-clk, linux-kernel,
dri-devel, devicetree, Conor Dooley
In-Reply-To: <20260615-rcar-du-dsc-v4-2-93096a1b56a3@ideasonboard.com>
Hi Tomi,
On Mon, 15 Jun 2026 at 08:28, Tomi Valkeinen
<tomi.valkeinen+renesas@ideasonboard.com> wrote:
> From: Marek Vasut <marek.vasut+renesas@mailbox.org>
>
> The Renesas DSC Display Stream Compression is a bridge embedded in the
> Renesas R-Car V4H SoC. The bridge performs VESA DSC encoding of up to
> 8k or 400 Mpixel/s .
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> [tomi.valkeinen: fix the example]
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
Thanks for the update!
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/renesas,dsc.yaml
> @@ -0,0 +1,99 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/renesas,dsc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas R-Car DSC Display Stream Compression
> +
> +maintainers:
> + - Marek Vasut <marek.vasut+renesas@mailbox.org>
> +
> +description: |
> + This binding describes the VESA DSC Display Stream Compression encoder
> + embedded in the Renesas R-Car V4H SoC. The encoder supports all DSC1.1
> + encoding mechanisms, configurable bits-per-pixel, resolution up to 8k.
> +
> +properties:
> + compatible:
> + items:
> + - enum:
> + - renesas,r8a779g0-dsc
> + - const: renesas,rcar-dsc
Please do not introduce new "renesas,rcar" fallback compatible values,
unless you are adding new support for a hardware block that is present
in SoCs belonging to multiple (nowadays I would say at least three)
R-Car generations. The DSC is only found in R-Car Gen4 SoCs, so
"renesas,rcar-gen4-dsc" sounds more appropriate.
However, so far R-Car V4H is the only R-Car Gen4 SoC that has a DSC.
Even the very similar R-Car V4M does not seem to have it.
R-Car X5H has DSC-functionality integrated in its DisplayPort TX
controller, so that seems to be a different implementation?
Hence that rules out any family-specific compatible value for now.
The rest of the (system/core) SoC integration LGTM.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH 2/2] dmaengine: zynqmp_dma: Add per-channel reset support
From: Golla, Nagendra @ 2026-06-15 9:29 UTC (permalink / raw)
To: sashiko-reviews; +Cc: robh, devicetree, vkoul, Frank.Li, dmaengine, conor+dt
In-Reply-To: <20260525113349.68D6E1F00A3D@smtp.kernel.org>
On 5/25/2026 5:03 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] The probe error path leaks a runtime PM usage counter reference, permanently breaking runtime PM and leaving hardware clocks enabled.
> - [High] Dropping the spinlock inside `list_for_each_entry_safe` allows concurrent modification of the list, leading to memory corruption.
> - [High] Hardware registers are accessed in the IRQ handler without verifying if the device is active, risking a kernel panic on spurious interrupts.
> --
>
> commit b2d2a678f48614c7188f8d81cbc70143b2bdfc0f
> Author: Golla Nagendra <nagendra.golla@amd.com>
> Subject: dmaengine: zynqmp_dma: Add per-channel reset support
>
> This commit introduces per-channel reset support for Versal Gen 2 and
> Versal Net SoCs. It adds reset handling in the channel probe path using
> device_reset_optional() to trigger a reset pulse during initialization.
> Platforms without per-channel reset are unaffected.
>
>> diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
>> index f6a812e49ddca..51c831760372c 100644
>> --- a/drivers/dma/xilinx/zynqmp_dma.c
>> +++ b/drivers/dma/xilinx/zynqmp_dma.c
> [ ... ]
>> @@ -916,6 +917,11 @@ static int zynqmp_dma_chan_probe(struct zynqmp_dma_device *zdev,
>> if (IS_ERR(chan->regs))
>> return PTR_ERR(chan->regs);
>>
>> + err = device_reset_optional(&pdev->dev);
>> + if (err)
>> + return dev_err_probe(&pdev->dev, err,
>> + "failed to reset channel\n");
>> +
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does this error path leak a
> runtime PM usage counter reference?
>
> In zynqmp_dma_probe(), pm_runtime_resume_and_get() is called to increment
> the runtime PM usage counter and enable the device clocks. If a subsequent
> initialization step fails, such as the newly added device_reset_optional()
> returning -EPROBE_DEFER, the code jumps to the err_disable_pm label:
>
> zynqmp_dma_probe()
> ...
> ret = pm_runtime_resume_and_get(zdev->dev);
> ...
> ret = zynqmp_dma_chan_probe(zdev, pdev);
> if (ret) {
> dev_err_probe(&pdev->dev, ret, "Probing channel failed\n");
> goto err_disable_pm;
> }
> ...
> err_disable_pm:
> if (!pm_runtime_enabled(zdev->dev))
> zynqmp_dma_runtime_suspend(zdev->dev);
> pm_runtime_disable(zdev->dev);
> return ret;
>
> Since the device is still runtime PM enabled, it skips
> zynqmp_dma_runtime_suspend() and fails to drop the usage counter with
> pm_runtime_put_noidle().
>
> Because -EPROBE_DEFER is a normal occurrence during boot, this path can
> be hit multiple times. Will this permanently leave the usage counter above
> zero, preventing the device from ever entering runtime suspend and leaving
> its clocks enabled indefinitely?
>
>> chan->bus_width = ZYNQMP_DMA_BUS_WIDTH_64;
>> chan->dst_burst_len = ZYNQMP_DMA_MAX_DST_BURST_LEN;
>
> [Severity: High]
> This is a pre-existing issue, but does dropping the spinlock inside
> list_for_each_entry_safe() in zynqmp_dma_chan_desc_cleanup() allow concurrent
> modification of the list, leading to memory corruption?
>
> When zynqmp_dma_chan_desc_cleanup() temporarily drops the lock to invoke
> the DMA callback:
>
> zynqmp_dma_chan_desc_cleanup()
> ...
> list_for_each_entry_safe(desc, next, &chan->done_list, node) {
> ...
> if (dmaengine_desc_callback_valid(&cb)) {
> spin_unlock_irqrestore(&chan->lock, irqflags);
> dmaengine_desc_callback_invoke(&cb, NULL);
> spin_lock_irqsave(&chan->lock, irqflags);
> }
>
> A client driver can concurrently invoke dmaengine_terminate_sync(), which
> calls zynqmp_dma_device_terminate_all() and executes
> zynqmp_dma_free_descriptors(). This moves all remaining elements, including
> the prefetched next pointer, from done_list to free_list.
>
> When zynqmp_dma_chan_desc_cleanup() reacquires the lock, won't it continue
> iterating using the corrupted next pointer that now resides in free_list,
> eventually interpreting the list head &chan->free_list as a struct
> zynqmp_dma_desc_sw and resulting in out-of-bounds memory accesses?
>
> [Severity: High]
> This is a pre-existing issue, but are hardware registers accessed in the
> IRQ handler without verifying if the device is active, risking a kernel
> panic on spurious interrupts?
Hi,
Thanks for pointing this out.After reviewing the runtime PM guidelines
and the driver behavior, I believe this is not a strict requirement for
all IRQ handlers. The PM documentation does not mandate that
pm_runtime_get_if_active() must be used before every register access in
an ISR. It is mainly intended for cases where an interrupt can occur
while the device might be runtime-suspended, such as shared IRQ or
spurious interrupt scenarios.
In the case of the zynqmp DMA driver, the interrupt is generated only
when the hardware is active during a transfer, and the driver ensures
the device remains runtime-active while DMA operations are in progress.
Because of this, the ISR is not expected to run when the device is
suspended, and accessing registers directly should be safe.
So, in this context, adding pm_runtime_get_if_active() in the IRQ
handler may not be necessary. That said, please let me know if you see
a scenario where the interrupt could be triggered while the device is
runtime-suspended, and I can revisit accordingly.
Thanks,
Nagendra
>
> The IRQ handler zynqmp_dma_irq_handler() directly reads the ISR and IMR
> hardware registers:
>
> zynqmp_dma_irq_handler()
> ...
> isr = readl(chan->regs + ZYNQMP_DMA_ISR);
> imr = readl(chan->regs + ZYNQMP_DMA_IMR);
>
> According to the Power Management Subsystem guidelines, IRQ handlers must
> use pm_runtime_get_if_active() before accessing hardware registers.
>
> Although this IRQ is not shared, spurious interrupts can still occur. If a
> spurious interrupt fires while the device is in a suspended state with its
> clocks disabled via zynqmp_dma_runtime_suspend(), could this unprotected
> register read lead to a synchronous external abort and crash the kernel?
>
^ 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