* Re: [PATCH 4/4] arm64: dts: qcom: shikra: Add support for AudioCoreCC node
From: Konrad Dybcio @ 2026-06-11 11:25 UTC (permalink / raw)
To: Imran Shaik, Bjorn Andersson, Michael Turquette, Stephen Boyd,
Brian Masney, Philipp Zabel, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Konrad Dybcio
Cc: Ajit Pandey, Taniya Das, Jagadeesh Kona, linux-arm-msm, linux-clk,
linux-kernel, devicetree
In-Reply-To: <20260605-shikra-audiocorecc-v1-4-7ee6b5f2d928@oss.qualcomm.com>
On 6/5/26 1:26 PM, Imran Shaik wrote:
> Add support for Audio Core Clock Controller (AudioCoreCC) node on Qualcomm
> Shikra SoC. The CQM variant requires both clock and reset support, while
> the CQS variant requires only reset support. Update the respective device
> tree variants to enable and override the node as per variant requirements.
>
> Signed-off-by: Imran Shaik <imran.shaik@oss.qualcomm.com>
> ---
> arch/arm64/boot/dts/qcom/shikra-cqm-evk.dts | 4 ++++
> arch/arm64/boot/dts/qcom/shikra-cqs-evk.dts | 5 +++++
> arch/arm64/boot/dts/qcom/shikra.dtsi | 13 +++++++++++++
> 3 files changed, 22 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/shikra-cqm-evk.dts b/arch/arm64/boot/dts/qcom/shikra-cqm-evk.dts
> index 0a52ab9b7a4c34d371f5ac23efe59d1c9d2723f4..0883c480bfbc80d7bead966b9ba932dee8a77bbf 100644
> --- a/arch/arm64/boot/dts/qcom/shikra-cqm-evk.dts
> +++ b/arch/arm64/boot/dts/qcom/shikra-cqm-evk.dts
> @@ -23,6 +23,10 @@ chosen {
> };
> };
>
> +&audiocorecc {
> + status = "okay";
> +};
> +
> &sdhc_1 {
> vmmc-supply = <&pm4125_l20>;
> vqmmc-supply = <&pm4125_l14>;
> diff --git a/arch/arm64/boot/dts/qcom/shikra-cqs-evk.dts b/arch/arm64/boot/dts/qcom/shikra-cqs-evk.dts
> index b3f19a64d7aed3121ef092df684b19a4de39b497..b5e3d573868a836ad5e5e8eb3024cb5fb71dbb4e 100644
> --- a/arch/arm64/boot/dts/qcom/shikra-cqs-evk.dts
> +++ b/arch/arm64/boot/dts/qcom/shikra-cqs-evk.dts
> @@ -23,6 +23,11 @@ chosen {
> };
> };
>
> +&audiocorecc {
> + compatible = "qcom,shikra-cqs-audiocorecc";
> + status = "okay";
> +};
> +
> &sdhc_1 {
> vmmc-supply = <&pm4125_l20>;
> vqmmc-supply = <&pm4125_l14>;
> diff --git a/arch/arm64/boot/dts/qcom/shikra.dtsi b/arch/arm64/boot/dts/qcom/shikra.dtsi
> index a4334d99c1f35ee851ca8266ec37d4a200a07ee5..f15757d52af04d8cb5540354a239127cb0d174a3 100644
> --- a/arch/arm64/boot/dts/qcom/shikra.dtsi
> +++ b/arch/arm64/boot/dts/qcom/shikra.dtsi
> @@ -4,6 +4,7 @@
> */
>
> #include <dt-bindings/clock/qcom,rpmcc.h>
> +#include <dt-bindings/clock/qcom,shikra-audiocorecc.h>
> #include <dt-bindings/clock/qcom,shikra-gcc.h>
> #include <dt-bindings/interconnect/qcom,icc.h>
> #include <dt-bindings/interconnect/qcom,rpm-icc.h>
> @@ -640,6 +641,18 @@ &clk_virt SLAVE_QUP_CORE_0 RPM_ALWAYS_TAG>,
> };
> };
>
> + audiocorecc: clock-controller@a0a0000 {
> + compatible = "qcom,shikra-cqm-audiocorecc";
> + reg = <0x0 0x0a0a0000 0x0 0x10000>,
This is called AUDIO_CORE_CC where seemingly all the clocks live
> + <0x0 0x0a0b4000 0x0 0x1000>;
and this is called AUDIO_CORE_CSR where seemingly all the resets live
so it would make sense to split them
Konrad
^ permalink raw reply
* Re: [PATCH 3/4] clk: qcom: Add Audio Core clock controller support on Qualcomm Shikra SoC
From: Konrad Dybcio @ 2026-06-11 11:27 UTC (permalink / raw)
To: Imran Shaik, Bjorn Andersson, Michael Turquette, Stephen Boyd,
Brian Masney, Philipp Zabel, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Konrad Dybcio
Cc: Ajit Pandey, Taniya Das, Jagadeesh Kona, linux-arm-msm, linux-clk,
linux-kernel, devicetree
In-Reply-To: <20260605-shikra-audiocorecc-v1-3-7ee6b5f2d928@oss.qualcomm.com>
On 6/5/26 1:26 PM, Imran Shaik wrote:
> Add support for Audio Core Clock Controller (AUDIOCORECC) on Qualcomm
> Shikra SoC. The AUDIOCORECC clocks and resets support differs based on
> Audio subsystem enablement. In the CQM variant, both clocks and resets
> are required as Audio is on APPS, while in the CQS variant only reset
> control is required since Audio is handled on Modem. Handle these
> requirements using variant specific compatibles.
>
> Signed-off-by: Imran Shaik <imran.shaik@oss.qualcomm.com>
> ---
[...]
> +static const struct qcom_reset_map audio_core_cc_shikra_resets[] = {
> + [AUDIO_CORE_CSR_RX_SWR_CGCR] = { 0x1c },
> + [AUDIO_CORE_CSR_TX_SWR_CGCR] = { 0x30 },
So these are not "real resets", but for the sake of existing art, we
can keep pretending they are
bit 1 is HW_CTL (1->hw controlled) and bit 0 is taken into account only
if 1 is cleared
existing drivers toggle the HW_CTRL bit (meaning it's an
maybe-on/surely-on switch rather than off/on).. do we need to rectify
that somehow?
Konrad
^ permalink raw reply
* [PATCH v1 0/2] ASoC: qcom: Shikra machine driver and DT binding support
From: Ajay Kumar Nandam @ 2026-06-11 11:29 UTC (permalink / raw)
To: Srinivas Kandagatla, Liam Girdwood, Mark Brown, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-sound, linux-arm-msm, devicetree, linux-kernel, ajay.nandam
This series adds Qualcomm Shikra sound-card variant support in DT
bindings and in the sc8280xp machine driver.
Shikra EVK variants use different audio components and processing
architectures:
- CQM/CQS use an I2S-based path with WSA885x amplifiers and
PM4125 + Rouleur codec components.
- CQM runs in DSP-bypass mode (CPU-only audio pipeline).
- CQS uses modem-DSP based audio processing.
- IQS uses a third-party MAX98091 codec with modem-DSP support.
The DT binding patch adds explicit Shikra compatibles, and the machine
patch wires variant-specific DAPM/controls and clock behavior so each
board is configured correctly.
Dependencies:
- lore.kernel.org/all/20260605103739.3557573-1-harendra.gautam@oss.qualcomm.com/
- lore.kernel.org/all/20260608023011.942228-1-mohammad.rafi.shaik@oss.qualcomm.com/
- lore.kernel.org/linux-arm-msm/20260610154517.134570-1-prasad.kumpatla@oss.qualcomm.com/
Ajay Kumar Nandam (2):
ASoC: dt-bindings: qcom,sm8250: add Shikra sound card compatibles
ASoC: qcom: sc8280xp: add Shikra EVK machine variants
.../bindings/sound/qcom,sm8250.yaml | 3 +
sound/soc/qcom/common.c | 2 +
sound/soc/qcom/sc8280xp.c | 58 ++++++++++++++++++-
sound/soc/qcom/sdw.c | 3 +
4 files changed, 64 insertions(+), 2 deletions(-)
base-commit: abe651837cb394f76d738a7a747322fca3bf17ba
prerequisite-patch-id: 430488f50f36039338965ab1fc28d83f02dbc9fd
prerequisite-patch-id: 359ddad8a3fb36f171c96da5ce5ffacd7dd63e8f
prerequisite-patch-id: a98885b9d0e0655bb3161dc2c31fd92a844a5e4e
prerequisite-patch-id: 63e6b911ad6700c1039802235c0a6d5870957f8c
prerequisite-patch-id: 6d481dd14afe58a17230318f418ec3fe0d327faf
prerequisite-patch-id: 7675b5ea6f01d7dadb8df43c8532daa4167fc92f
prerequisite-patch-id: 3fcc510f8f38ce63b24f02d48257ce8d7079b61e
prerequisite-patch-id: 037f677639a12a986e024f9a66df2def301925d4
prerequisite-patch-id: 888f7d13f882fdd0a01bc3fcbe008e9e56394bd3
prerequisite-patch-id: 7db9bb6a1d3de3667a0880f8a75c24ce62e52ecd
prerequisite-patch-id: b898d117a21bddc176ae19937b03e733df72f821
prerequisite-patch-id: 3ca81fae4cb388c4970e908fb63cc99bc1cdd008
prerequisite-patch-id: 80aead6484e36f52cb6cc7fd7d9e0326d8296860
prerequisite-patch-id: 2f1bd3efac328030dd8efe28fb95f84603868043
prerequisite-patch-id: 047b4fb1894b92109aa7afcebd7d5c7988ec5379
prerequisite-patch-id: 222630a15afc952683d954a3c66617a223546de0
prerequisite-patch-id: 3c55edb41f1e25920a350ce1c6f31fde67fee45a
prerequisite-patch-id: 0de75678d071f174c865afa2915be4df1aef8c8d
prerequisite-patch-id: 14840d7e2441e2110c1e147941744be637c9595f
prerequisite-patch-id: a7706e25f5951ec41e6b662c1704df8d20662d77
prerequisite-patch-id: 1e1be31d7ddc47eec9193164defa3e5c473b6ef1
prerequisite-patch-id: d13035abab3ff342753f5bf87b53bbf06a02c6a1
prerequisite-patch-id: 79fcbe1428667a6e0059cea9cbfd62d38d114930
--
2.34.1
^ permalink raw reply
* [PATCH v1 1/2] ASoC: dt-bindings: qcom,sm8250: add Shikra sound card compatibles
From: Ajay Kumar Nandam @ 2026-06-11 11:29 UTC (permalink / raw)
To: Srinivas Kandagatla, Liam Girdwood, Mark Brown, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-sound, linux-arm-msm, devicetree, linux-kernel, ajay.nandam,
Mohammad Rafi Shaik
In-Reply-To: <20260611112946.954172-1-ajay.nandam@oss.qualcomm.com>
Add Shikra sound-card compatible strings to the Qualcomm sound card
binding so DT can describe board-specific audio topologies:
- qcom,shikra-cqm-sndcard
- qcom,shikra-cqs-sndcard
- qcom,shikra-iqs-sndcard
Shikra EVK variants use different codec/interface combinations and DSP
processing paths. Describing these variants explicitly in DT allows the
machine driver to select the correct DAPM routes, controls, and clocking
behavior for each board.
Co-developed-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
Signed-off-by: Ajay Kumar Nandam <ajay.nandam@oss.qualcomm.com>
---
Documentation/devicetree/bindings/sound/qcom,sm8250.yaml | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml b/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
index 15f38622b..c5c7284bf 100644
--- a/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
+++ b/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
@@ -45,6 +45,9 @@ properties:
- qcom,sc7180-qdsp6-sndcard
- qcom,sc8280xp-sndcard
- qcom,sdm845-sndcard
+ - qcom,shikra-cqm-sndcard
+ - qcom,shikra-cqs-sndcard
+ - qcom,shikra-iqs-sndcard
- qcom,sm8250-sndcard
- qcom,sm8450-sndcard
- qcom,x1e80100-sndcard
--
2.34.1
^ permalink raw reply related
* [PATCH v1 2/2] ASoC: qcom: sc8280xp: add Shikra EVK machine variants
From: Ajay Kumar Nandam @ 2026-06-11 11:29 UTC (permalink / raw)
To: Srinivas Kandagatla, Liam Girdwood, Mark Brown, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-sound, linux-arm-msm, devicetree, linux-kernel, ajay.nandam,
Mohammad Rafi Shaik
In-Reply-To: <20260611112946.954172-1-ajay.nandam@oss.qualcomm.com>
Add machine-driver support for Qualcomm Shikra EVK variants by matching
dedicated compatible strings and applying board-specific audio behavior.
Shikra platforms are available as CQM, CQS, and IQS variants with
different audio components and processing architectures:
- CQM/CQS use an I2S-based path with WSA885x amplifiers and
PM4125 + Rouleur codec components.
- CQM runs in DSP-bypass mode, where the complete audio pipeline runs
on CPU only and no DSP is involved.
- CQS uses modem-DSP based audio processing.
- IQS uses a third-party MAX98091 codec with modem-DSP support.
Introduce variant-specific handling so codec controls, DAPM widgets, and
clock programming match the board design and avoid invalid clock setup on
variants where it is not applicable.
Co-developed-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
Signed-off-by: Ajay Kumar Nandam <ajay.nandam@oss.qualcomm.com>
---
sound/soc/qcom/common.c | 2 ++
sound/soc/qcom/sc8280xp.c | 58 +++++++++++++++++++++++++++++++++++++--
sound/soc/qcom/sdw.c | 3 ++
3 files changed, 61 insertions(+), 2 deletions(-)
diff --git a/sound/soc/qcom/common.c b/sound/soc/qcom/common.c
index f42c98ded..32d6c09b2 100644
--- a/sound/soc/qcom/common.c
+++ b/sound/soc/qcom/common.c
@@ -3,6 +3,7 @@
// Copyright (c) 2018, The Linux Foundation. All rights reserved.
#include <dt-bindings/sound/qcom,q6afe.h>
+#include <dt-bindings/sound/qcom,qaif.h>
#include <linux/module.h>
#include <sound/jack.h>
#include <linux/input-event-codes.h>
@@ -430,6 +431,7 @@ int qcom_snd_wcd_jack_setup(struct snd_soc_pcm_runtime *rtd,
}
switch (cpu_dai->id) {
+ case QAIF_CDC_DMA_RX0:
case TX_CODEC_DMA_TX_0:
case TX_CODEC_DMA_TX_1:
case TX_CODEC_DMA_TX_2:
diff --git a/sound/soc/qcom/sc8280xp.c b/sound/soc/qcom/sc8280xp.c
index e5d23e244..4c985f81c 100644
--- a/sound/soc/qcom/sc8280xp.c
+++ b/sound/soc/qcom/sc8280xp.c
@@ -37,6 +37,25 @@ static struct snd_soc_dapm_widget sc8280xp_dapm_widgets[] = {
SND_SOC_DAPM_SPK("DP7 Jack", NULL),
};
+static struct snd_soc_dapm_widget shikra_cqm_dapm_widgets[] = {
+ SND_SOC_DAPM_HP("Headphone Jack", NULL),
+ SND_SOC_DAPM_MIC("Mic Jack", NULL),
+};
+
+static const struct snd_soc_dapm_widget shikra_iqs_dapm_widgets[] = {
+ SND_SOC_DAPM_HP("Headphone", NULL),
+ SND_SOC_DAPM_MIC("Headset Mic", NULL),
+ SND_SOC_DAPM_MIC("Int Mic", NULL),
+ SND_SOC_DAPM_SPK("Speaker", NULL),
+};
+
+static const struct snd_kcontrol_new shikra_iqs_controls[] = {
+ SOC_DAPM_PIN_SWITCH("Headset Mic"),
+ SOC_DAPM_PIN_SWITCH("Headphone"),
+ SOC_DAPM_PIN_SWITCH("Int Mic"),
+ SOC_DAPM_PIN_SWITCH("Speaker"),
+};
+
struct snd_soc_common {
const char *driver_name;
const struct snd_soc_dapm_widget *dapm_widgets;
@@ -49,6 +68,7 @@ struct snd_soc_common {
bool codec_sysclk_set;
bool mi2s_mclk_enable;
bool mi2s_bclk_enable;
+ bool dsp_bypass;
};
struct sc8280xp_snd_data {
@@ -219,6 +239,10 @@ static int sc8280xp_snd_hw_params(struct snd_pcm_substream *substream,
int mclk_freq = sc8280xp_get_mclk_freq(params);
int bclk_freq = sc8280xp_get_bclk_freq(params);
+ /* Skip DSP configuration when operating in CPU-only (bypass) mode */
+ if (data->snd_soc_common_priv->dsp_bypass)
+ return 0;
+
switch (cpu_dai->id) {
case PRIMARY_MI2S_RX ... QUATERNARY_MI2S_TX:
case QUINARY_MI2S_RX ... QUINARY_MI2S_TX:
@@ -239,7 +263,7 @@ static int sc8280xp_snd_hw_params(struct snd_pcm_substream *substream,
SND_SOC_CLOCK_IN);
if (data->snd_soc_common_priv->codec_sysclk_set)
- snd_soc_dai_set_sysclk(cpu_dai,
+ snd_soc_dai_set_sysclk(codec_dai,
0, mclk_freq,
SND_SOC_CLOCK_IN);
break;
@@ -284,7 +308,7 @@ static void sc8280xp_add_be_ops(struct snd_soc_card *card)
int i;
for_each_card_prelinks(card, i, link) {
- if (link->no_pcm == 1) {
+ if (link->no_pcm == 1 || link->num_codecs > 0) {
link->init = sc8280xp_snd_init;
link->be_hw_params_fixup = sc8280xp_be_hw_params_fixup;
link->ops = &sc8280xp_be_ops;
@@ -375,6 +399,33 @@ static struct snd_soc_common sc8280xp_priv_data = {
.num_dapm_widgets = ARRAY_SIZE(sc8280xp_dapm_widgets),
};
+static const struct snd_soc_common shikra_cqm_priv_data = {
+ .driver_name = "shikra",
+ .dapm_widgets = shikra_cqm_dapm_widgets,
+ .num_dapm_widgets = ARRAY_SIZE(shikra_cqm_dapm_widgets),
+ .dsp_bypass = true,
+};
+
+static const struct snd_soc_common shikra_cqs_priv_data = {
+ .driver_name = "shikra",
+ .dapm_widgets = shikra_cqm_dapm_widgets,
+ .num_dapm_widgets = ARRAY_SIZE(shikra_cqm_dapm_widgets),
+ .mi2s_bclk_enable = true,
+ .codec_sysclk_set = true,
+};
+
+static const struct snd_soc_common shikra_iqs_priv_data = {
+ .driver_name = "shikra",
+ .dapm_widgets = shikra_iqs_dapm_widgets,
+ .num_dapm_widgets = ARRAY_SIZE(shikra_iqs_dapm_widgets),
+ .controls = shikra_iqs_controls,
+ .num_controls = ARRAY_SIZE(shikra_iqs_controls),
+ .codec_dai_fmt = SND_SOC_DAIFMT_CBP_CFP |
+ SND_SOC_DAIFMT_NB_NF |
+ SND_SOC_DAIFMT_I2S,
+ .codec_sysclk_set = true,
+ .mi2s_bclk_enable = true,
+};
+
static struct snd_soc_common sm8450_priv_data = {
.driver_name = "sm8450",
.dapm_widgets = sc8280xp_dapm_widgets,
@@ -408,6 +459,9 @@ static const struct of_device_id snd_sc8280xp_dt_match[] = {
{.compatible = "qcom,qcs9075-sndcard", .data = &qcs9100_priv_data},
{.compatible = "qcom,qcs9100-sndcard", .data = &qcs9100_priv_data},
{.compatible = "qcom,sc8280xp-sndcard", .data = &sc8280xp_priv_data},
+ {.compatible = "qcom,shikra-cqm-sndcard", .data = &shikra_cqm_priv_data},
+ {.compatible = "qcom,shikra-cqs-sndcard", .data = &shikra_cqs_priv_data},
+ {.compatible = "qcom,shikra-iqs-sndcard", .data = &shikra_iqs_priv_data},
{.compatible = "qcom,sm8450-sndcard", .data = &sm8450_priv_data},
{.compatible = "qcom,sm8550-sndcard", .data = &sm8550_priv_data},
{.compatible = "qcom,sm8650-sndcard", .data = &sm8650_priv_data},
diff --git a/sound/soc/qcom/sdw.c b/sound/soc/qcom/sdw.c
index 6576b47a4..0be743cec 100644
--- a/sound/soc/qcom/sdw.c
+++ b/sound/soc/qcom/sdw.c
@@ -4,6 +4,7 @@
#include <dt-bindings/sound/qcom,lpass.h>
#include <dt-bindings/sound/qcom,q6afe.h>
+#include <dt-bindings/sound/qcom,qaif.h>
#include <linux/module.h>
#include <sound/soc.h>
#include "sdw.h"
@@ -41,6 +42,8 @@ static bool qcom_snd_is_sdw_dai(int id)
switch (id) {
case LPASS_CDC_DMA_TX3:
case LPASS_CDC_DMA_RX0:
+ case QAIF_CDC_DMA_VA_TX0:
+ case QAIF_CDC_DMA_RX0:
return true;
default:
break;
--
2.34.1
^ permalink raw reply related
* RE: [PATCH v1 1/7] ASoC: dt-bindings: ES8389: Add members about HPF and clock
From: Zhang Yi @ 2026-06-11 11:30 UTC (permalink / raw)
To: krzk
Cc: broonie, conor+dt, devicetree, krzk+dt, linux-sound, robh, tiwai,
zhangyi
In-Reply-To: <20260611-mamba-of-legendary-anger-af76a9@quoll>
> So this is like third time you send the same.
>
> You never responded to feedback, I don't see improvements and you keep
> sending the same v1.
>
> Version your patches correctly - read help of git format-patch or just
> use b4.
>
> NAK again because you just ignore us.
Sorry, I was wondering if you received the following two emails
https://lore.kernel.org/all/20260610100637.25568-1-zhangyi@everest-semi.com/
https://lore.kernel.org/all/20260610095820.25386-1-zhangyi@everest-semi.com/
^ permalink raw reply
* Re: [PATCH v8 5/7] leds: Add driver for ASUS Transformer LEDs
From: Lee Jones @ 2026-06-11 11:30 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <20260528053203.9339-6-clamor95@gmail.com>
On Thu, 28 May 2026, Svyatoslav Ryhel wrote:
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>
> ASUS Transformer tablets have a green and an amber LED on both the Pad
> and the Dock. If both LEDs are enabled simultaneously, the emitted light
> will be yellow.
>
> Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> drivers/leds/Kconfig | 11 +++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-asus-transformer-ec.c | 125 ++++++++++++++++++++++++
> 3 files changed, 137 insertions(+)
> create mode 100644 drivers/leds/leds-asus-transformer-ec.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index f4a0a3c8c870..f637d23400a8 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -120,6 +120,17 @@ config LEDS_OSRAM_AMS_AS3668
> To compile this driver as a module, choose M here: the module
> will be called leds-as3668.
>
> +config LEDS_ASUS_TRANSFORMER_EC
> + tristate "LED Support for Asus Transformer charging LED"
> + depends on LEDS_CLASS
> + depends on MFD_ASUS_TRANSFORMER_EC
> + help
> + This option enables support for charging indicator on
> + Asus Transformer's Pad and it's Dock.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called leds-asus-transformer-ec.
> +
> config LEDS_AW200XX
> tristate "LED support for Awinic AW20036/AW20054/AW20072/AW20108"
> depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 8fdb45d5b439..d5395c3f1124 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o
> obj-$(CONFIG_LEDS_APU) += leds-apu.o
> obj-$(CONFIG_LEDS_ARIEL) += leds-ariel.o
> obj-$(CONFIG_LEDS_AS3668) += leds-as3668.o
> +obj-$(CONFIG_LEDS_ASUS_TRANSFORMER_EC) += leds-asus-transformer-ec.o
> obj-$(CONFIG_LEDS_AW200XX) += leds-aw200xx.o
> obj-$(CONFIG_LEDS_AW2013) += leds-aw2013.o
> obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o
> diff --git a/drivers/leds/leds-asus-transformer-ec.c b/drivers/leds/leds-asus-transformer-ec.c
> new file mode 100644
> index 000000000000..09503e76331c
> --- /dev/null
> +++ b/drivers/leds/leds-asus-transformer-ec.c
> @@ -0,0 +1,125 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/err.h>
> +#include <linux/leds.h>
> +#include <linux/mfd/asus-transformer-ec.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +enum {
> + ASUSEC_LED_AMBER,
> + ASUSEC_LED_GREEN,
> + ASUSEC_LED_MAX
> +};
> +
> +struct asus_ec_led_config {
> + const char *name;
> + unsigned int color;
> + unsigned long long ctrl_bit;
Should we use 'u64' here instead of 'unsigned long long' to align with standard
kernel integer types?
> +};
> +
> +struct asus_ec_led {
> + struct asus_ec_leds_data *ddata;
> + struct led_classdev cdev;
> + unsigned long long ctrl_bit;
Should we use 'u64' here as well to keep it consistent?
> +};
> +
> +struct asus_ec_leds_data {
> + const struct asusec_core *ec;
> + struct asus_ec_led leds[ASUSEC_LED_MAX];
> +};
> +
> +static const struct asus_ec_led_config asus_ec_leds[] = {
> + [ASUSEC_LED_AMBER] = {
> + .name = "amber",
> + .color = LED_COLOR_ID_AMBER,
> + .ctrl_bit = ASUSEC_CTL_LED_AMBER,
> + },
> + [ASUSEC_LED_GREEN] = {
> + .name = "green",
> + .color = LED_COLOR_ID_GREEN,
> + .ctrl_bit = ASUSEC_CTL_LED_GREEN,
> + },
> +};
> +
> +static enum led_brightness asus_ec_led_get_brightness(struct led_classdev *cdev)
> +{
> + struct asus_ec_led *led = container_of(cdev, struct asus_ec_led, cdev);
> + const struct asusec_core *ec = led->ddata->ec;
I'm getting confused here.
ddata is what I'd be calling the device data struct passed by the parent?
In fact, ddata is a little known concept in Leds. Any reason to go for
this over the standard nomenclature?
> + u64 ctl;
> + int ret;
> +
> + ret = asus_dockram_access_ctl(ec->dockram, &ctl, 0, 0);
Did we discuss preferring regmap already?
> + if (ret)
> + return LED_OFF;
> +
> + return ctl & led->ctrl_bit ? LED_ON : LED_OFF;
> +}
> +
> +static int asus_ec_led_set_brightness(struct led_classdev *cdev,
> + enum led_brightness brightness)
> +{
> + struct asus_ec_led *led = container_of(cdev, struct asus_ec_led, cdev);
> + const struct asusec_core *ec = led->ddata->ec;
> +
> + if (brightness)
> + return asus_dockram_access_ctl(ec->dockram, NULL,
> + led->ctrl_bit, led->ctrl_bit);
> +
> + return asus_dockram_access_ctl(ec->dockram, NULL, led->ctrl_bit, 0);
> +}
> +
> +static int asus_ec_led_probe(struct platform_device *pdev)
> +{
> + const struct asusec_core *ec = dev_get_drvdata(pdev->dev.parent);
> + struct asus_ec_leds_data *ddata;
> + struct device *dev = &pdev->dev;
> + int i, ret;
Could we declare the loop counter 'i' directly within the 'for' statement's
scope to keep its scope limited? For example, 'for (int i = 0; ...)'.
> +
> + ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> + if (!ddata)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, ddata);
> + ddata->ec = ec;
> +
> + for (i = 0; i < ASUSEC_LED_MAX; i++) {
Nit: for (int i = ...
> + const struct asus_ec_led_config *cfg = &asus_ec_leds[i];
> + struct asus_ec_led *led = &ddata->leds[i];
> +
> + led->cdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s::%s",
> + ddata->ec->name, cfg->name);
> + if (!led->cdev.name)
> + return -ENOMEM;
> +
> + led->cdev.max_brightness = 1;
> + led->cdev.color = cfg->color;
> + led->cdev.flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;
> + led->cdev.brightness_get = asus_ec_led_get_brightness;
> + led->cdev.brightness_set_blocking = asus_ec_led_set_brightness;
> +
> + led->ddata = ddata;
> + led->ctrl_bit = cfg->ctrl_bit;
> +
> + ret = devm_led_classdev_register(dev, &led->cdev);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to register %s LED\n",
> + cfg->name);
Should we capitalise the error message here to match our style guidelines
(e.g. 'Failed to register...')?
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver asus_ec_led_driver = {
> + .driver.name = "asus-transformer-ec-led",
> + .probe = asus_ec_led_probe,
> +};
> +module_platform_driver(asus_ec_led_driver);
> +
> +MODULE_ALIAS("platform:asus-transformer-ec-led");
> +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> +MODULE_DESCRIPTION("ASUS Transformer's charging LED driver");
> +MODULE_LICENSE("GPL");
> --
> 2.51.0
>
>
--
Lee Jones
^ permalink raw reply
* Re: [PATCH V11 7/9] iio: imu: inv_icm42607: Add Accelerometer for icm42607
From: Jonathan Cameron @ 2026-06-11 11:31 UTC (permalink / raw)
To: Chris Morgan
Cc: linux-iio, andy, nuno.sa, dlechner, jean-baptiste.maneyrol,
linux-rockchip, devicetree, heiko, conor+dt, krzk+dt, robh,
andriy.shevchenko, Chris Morgan
In-Reply-To: <20260610175455.19006-8-macroalpha82@gmail.com>
On Wed, 10 Jun 2026 12:54:51 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
>
> Add icm42607 accelerometer sensor for icm42607.
>
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
Minor things inline.
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
> new file mode 100644
> index 000000000000..cb60bb5ecc14
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
> @@ -0,0 +1,379 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2026 InvenSense, Inc.
> + */
> +
> +#include <linux/iio/iio.h>
> +#include <linux/mutex.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +
> +#include "inv_icm42607.h"
> +#include "inv_icm42607_temp.h"
> +
> +#define INV_ICM42607_ACCEL_CHAN(_modifier, _index, _ext_info) \
> +{ \
> + .type = IIO_ACCEL, \
> + .modified = 1, \
> + .channel2 = _modifier, \
> + .info_mask_separate = \
> + BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_shared_by_type_available = \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_shared_by_all = \
> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> + .info_mask_shared_by_all_available = \
> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
See other comments on this is is 'all' and perhaps applies to temperature.
If it doesn't apply to temperature then it may need splitting up.
Little less clear cut for mount matrix, so I don't mind that one just
> + .scan_index = _index, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 16, \
> + .storagebits = 16, \
> + .endianness = IIO_BE, \
> + }, \
> + .ext_info = _ext_info, \
> +}
> +static int inv_icm42607_accel_write_odr(struct iio_dev *indio_dev,
> + int val, int val2)
> +{
> + struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> + struct device *dev = regmap_get_device(st->map);
> + unsigned int idx;
> + struct inv_icm42607_sensor_conf conf = INV_ICM42607_SENSOR_CONF_INIT;
> + int ret;
> +
> + for (idx = 5; idx < ARRAY_SIZE(inv_icm42607_accel_odr); ++idx) {
> + if (val == inv_icm42607_accel_odr[idx][0] &&
> + val2 == inv_icm42607_accel_odr[idx][1])
> + break;
> + }
> + if (idx >= ARRAY_SIZE(inv_icm42607_accel_odr))
> + return -EINVAL;
> +
> + conf.odr = idx;
> +
> + PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
> + ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> + if (ret)
> + return ret;
> +
> + guard(mutex)(&st->lock);
> +
> + ret = inv_icm42607_set_accel_conf(st, &conf, NULL);
> + if (ret)
> + return ret;
> +
> + return 0;
If this isn't getting more complex later,
return inv_...
> +}
> +
> +static int inv_icm42607_accel_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> + s16 data;
> + int ret;
> +
> + switch (chan->type) {
> + case IIO_ACCEL:
> + break;
> + case IIO_TEMP:
> + return inv_icm42607_temp_read_raw(indio_dev, chan, val, val2, mask);
I commented on this in previous patch, but once the shared_by_all is
added to the temp channel, this needs modifying to ensure we only call the
temp handler for cases that aren't shared_by_all.
> + default:
> + return -EINVAL;
> + }
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (!iio_device_claim_direct(indio_dev))
> + return -EBUSY;
As below - drop these mode claims as for now there is only one mode.
> + ret = inv_icm42607_accel_read_sensor(indio_dev, chan, &data);
> + iio_device_release_direct(indio_dev);
> + if (ret)
> + return ret;
> + *val = data;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + return inv_icm42607_accel_read_scale(indio_dev, val, val2);
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + return inv_icm42607_accel_read_odr(st, val, val2);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int inv_icm42607_accel_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals,
> + int *type, int *length, long mask)
> +{
> + if (chan->type != IIO_ACCEL)
> + return -EINVAL;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + *vals = (const int *)inv_icm42607_accel_scale_nano;
> + *type = IIO_VAL_INT_PLUS_NANO;
> + *length = ARRAY_SIZE(inv_icm42607_accel_scale_nano) * 2;
> + return IIO_AVAIL_LIST;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *vals = (const int *)inv_icm42607_accel_odr[5];
> + *type = IIO_VAL_INT_PLUS_MICRO;
> + *length = (ARRAY_SIZE(inv_icm42607_accel_odr) - 5) * 2;
> + return IIO_AVAIL_LIST;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int inv_icm42607_accel_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + int ret;
> +
> + if (chan->type != IIO_ACCEL)
> + return -EINVAL;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + if (!iio_device_claim_direct(indio_dev))
As in previous, this stuff doesn't belong in a driver that is always in direct
mode.
> + return -EBUSY;
> + ret = inv_icm42607_accel_write_scale(indio_dev, val, val2);
> + iio_device_release_direct(indio_dev);
> + return ret;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + return inv_icm42607_accel_write_odr(indio_dev, val, val2);
> + default:
> + return -EINVAL;
> + }
> +struct iio_dev *inv_icm42607_accel_init(struct inv_icm42607_state *st)
> +{
> + struct device *dev = regmap_get_device(st->map);
> + const char *name;
where no other strong reason for ordering, reverse xmas tree.
> + struct inv_icm42607_sensor_state *accel_st;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + name = devm_kasprintf(dev, GFP_KERNEL, "%s-accel", st->hw->name);
> + if (!name)
> + return ERR_PTR(-ENOMEM);
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*accel_st));
> + if (!indio_dev)
> + return ERR_PTR(-ENOMEM);
> + accel_st = iio_priv(indio_dev);
> +
> + accel_st->power_mode = INV_ICM42607_SENSOR_MODE_LOW_NOISE;
> + accel_st->filter = INV_ICM42607_FILTER_BW_73HZ;
> +
> + iio_device_set_drvdata(indio_dev, st);
> + indio_dev->name = name;
> + indio_dev->info = &inv_icm42607_accel_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = inv_icm42607_accel_channels;
> + indio_dev->num_channels = ARRAY_SIZE(inv_icm42607_accel_channels);
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + return indio_dev;
> +}
^ permalink raw reply
* Re: [PATCH v3 3/8] pinctrl: qcom: Register functions before enabling pinctrl
From: Konrad Dybcio @ 2026-06-11 11:32 UTC (permalink / raw)
To: contact, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, MyungJoo Ham, Chanwoo Choi,
Guru Das Srinagesh, Linus Walleij, Rob Clark, Joerg Roedel,
Will Deacon, Robin Murphy, Kees Cook, Tony Luck,
Guilherme G. Piccoli
Cc: linux-arm-msm, devicetree, linux-kernel, linux-gpio, iommu,
phone-devel
In-Reply-To: <20260519-mainline-send-v1-sending-v3-3-3dd7aa125353@alex-min.fr>
On 5/19/26 9:16 AM, Alexandre MINETTE via B4 Relay wrote:
> From: Alexandre MINETTE <contact@alex-min.fr>
>
> pinctrl consumers can request states while the pinctrl core enables the
> controller. On Qualcomm pinctrl drivers this can happen before the SoC
> function list has been registered, which leaves the function table
> incomplete during state lookup.
>
> On APQ8064 this can fail while claiming pinctrl hogs:
>
> apq8064-pinctrl 800000.pinctrl: invalid function ps_hold in map table
> apq8064-pinctrl 800000.pinctrl: error claiming hogs: -22
> apq8064-pinctrl 800000.pinctrl: could not claim hogs: -22
>
> Register Qualcomm pinctrl with devm_pinctrl_register_and_init(), add the
> SoC pin functions, and only then enable the pinctrl device.
>
> Signed-off-by: Alexandre MINETTE <contact@alex-min.fr>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH v3 06/18] arm64: dts: qcom: sm8450: Add minidump SRAM config to SCM node
From: Konrad Dybcio @ 2026-06-11 11:34 UTC (permalink / raw)
To: Mukesh Ojha, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Marko,
Guru Das Srinagesh
Cc: cros-qcom-dts-watchers, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20260522195009.2961022-7-mukesh.ojha@oss.qualcomm.com>
On 5/22/26 9:49 PM, Mukesh Ojha wrote:
> Point the SCM node at the minidump config slot in the always-on SRAM.
> Boot firmware reads this word before DDR is initialised on a warm reset
> to decide where to deliver the minidump.
>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH v3 13/18] arm64: dts: qcom: sm8350: Add minidump SRAM config to SCM node
From: Konrad Dybcio @ 2026-06-11 11:34 UTC (permalink / raw)
To: Mukesh Ojha, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Marko,
Guru Das Srinagesh
Cc: cros-qcom-dts-watchers, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20260522195009.2961022-14-mukesh.ojha@oss.qualcomm.com>
On 5/22/26 9:50 PM, Mukesh Ojha wrote:
> Point the SCM node at the minidump config slot in the always-on SRAM.
> Boot firmware reads this word before DDR is initialised on a warm reset
> to decide where to deliver the minidump.
>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH v3 18/18] arm64: dts: qcom: sdm845: Add minidump SRAM config to SCM node
From: Konrad Dybcio @ 2026-06-11 11:34 UTC (permalink / raw)
To: Mukesh Ojha, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Marko,
Guru Das Srinagesh
Cc: cros-qcom-dts-watchers, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20260522195009.2961022-19-mukesh.ojha@oss.qualcomm.com>
On 5/22/26 9:50 PM, Mukesh Ojha wrote:
> Point the SCM node at the minidump config slot in the always-on SRAM.
> Boot firmware reads this word before DDR is initialised on a warm reset
> to decide where to deliver the minidump.
>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH v3 16/18] arm64: dts: qcom: sm6375: Add minidump SRAM config to SCM node
From: Konrad Dybcio @ 2026-06-11 11:35 UTC (permalink / raw)
To: Mukesh Ojha, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Marko,
Guru Das Srinagesh
Cc: cros-qcom-dts-watchers, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20260522195009.2961022-17-mukesh.ojha@oss.qualcomm.com>
On 5/22/26 9:50 PM, Mukesh Ojha wrote:
> Point the SCM node at the minidump config slot in the always-on SRAM.
> Boot firmware reads this word before DDR is initialised on a warm reset
> to decide where to deliver the minidump.
>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH v3 15/18] arm64: dts: qcom: sm6350: Add minidump SRAM config to SCM node
From: Konrad Dybcio @ 2026-06-11 11:35 UTC (permalink / raw)
To: Mukesh Ojha, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Marko,
Guru Das Srinagesh
Cc: cros-qcom-dts-watchers, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20260522195009.2961022-16-mukesh.ojha@oss.qualcomm.com>
On 5/22/26 9:50 PM, Mukesh Ojha wrote:
> Point the SCM node at the minidump config slot in the always-on SRAM.
> Boot firmware reads this word before DDR is initialised on a warm reset
> to decide where to deliver the minidump.
>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH v3 14/18] arm64: dts: qcom: sc7180: Add minidump SRAM config to SCM node
From: Konrad Dybcio @ 2026-06-11 11:36 UTC (permalink / raw)
To: Mukesh Ojha, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Marko,
Guru Das Srinagesh
Cc: cros-qcom-dts-watchers, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20260522195009.2961022-15-mukesh.ojha@oss.qualcomm.com>
On 5/22/26 9:50 PM, Mukesh Ojha wrote:
> Point the SCM node at the minidump config slot in the always-on SRAM.
> Boot firmware reads this word before DDR is initialised on a warm reset
> to decide where to deliver the minidump.
>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH v3 12/18] arm64: dts: qcom: sc7280: Add minidump SRAM config to SCM node
From: Konrad Dybcio @ 2026-06-11 11:36 UTC (permalink / raw)
To: Mukesh Ojha, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Marko,
Guru Das Srinagesh
Cc: cros-qcom-dts-watchers, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20260522195009.2961022-13-mukesh.ojha@oss.qualcomm.com>
On 5/22/26 9:50 PM, Mukesh Ojha wrote:
> Point the SCM node at the minidump config slot in the always-on SRAM.
> Boot firmware reads this word before DDR is initialised on a warm reset
> to decide where to deliver the minidump.
>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH v3 11/18] arm64: dts: qcom: sm8650: Add minidump SRAM config to SCM node
From: Konrad Dybcio @ 2026-06-11 11:36 UTC (permalink / raw)
To: Mukesh Ojha, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Marko,
Guru Das Srinagesh
Cc: cros-qcom-dts-watchers, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20260522195009.2961022-12-mukesh.ojha@oss.qualcomm.com>
On 5/22/26 9:50 PM, Mukesh Ojha wrote:
> Point the SCM node at the minidump config slot in the always-on SRAM.
> Boot firmware reads this word before DDR is initialised on a warm reset
> to decide where to deliver the minidump.
>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH v3 10/18] arm64: dts: qcom: sm8550: Add minidump SRAM config to SCM node
From: Konrad Dybcio @ 2026-06-11 11:37 UTC (permalink / raw)
To: Mukesh Ojha, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Marko,
Guru Das Srinagesh
Cc: cros-qcom-dts-watchers, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20260522195009.2961022-11-mukesh.ojha@oss.qualcomm.com>
On 5/22/26 9:50 PM, Mukesh Ojha wrote:
> Point the SCM node at the minidump config slot in the always-on SRAM.
> Boot firmware reads this word before DDR is initialised on a warm reset
> to decide where to deliver the minidump.
>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH v3 09/18] arm64: dts: qcom: qdu1000: Add minidump SRAM config to SCM node
From: Konrad Dybcio @ 2026-06-11 11:37 UTC (permalink / raw)
To: Mukesh Ojha, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Marko,
Guru Das Srinagesh
Cc: cros-qcom-dts-watchers, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20260522195009.2961022-10-mukesh.ojha@oss.qualcomm.com>
On 5/22/26 9:50 PM, Mukesh Ojha wrote:
> Point the SCM node at the minidump config slot in the always-on SRAM.
> Boot firmware reads this word before DDR is initialised on a warm reset
> to decide where to deliver the minidump.
>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH v3 04/21] pinctrl: starfive: Add StarFive JHB100 sys0 controller driver
From: Linus Walleij @ 2026-06-11 11:37 UTC (permalink / raw)
To: Changhuang Liang
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Emil Renner Berthing, Paul Walmsley, Albert Ou, Palmer Dabbelt,
Alexandre Ghiti, Philipp Zabel, Bartosz Golaszewski, linux-gpio,
linux-kernel, devicetree, linux-riscv, Lianfeng Ouyang
In-Reply-To: <20260603055347.66845-5-changhuang.liang@starfivetech.com>
Hi Changhuang,
thanks for your patch!
On Wed, Jun 3, 2026 at 7:54 AM Changhuang Liang
<changhuang.liang@starfivetech.com> wrote:
> Add pinctrl driver for StarFive JHB100 SoC System-0(sys0) pinctrl
> controller.
>
> Co-developed-by: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>
> Signed-off-by: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>
> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
This patch adds generic infrastructure "JHB100" that is then used
by several drivers does it not?
Write something about that and some about the design in the
commit message.
> +++ b/drivers/pinctrl/starfive/pinctrl-starfive-jhb100-sys0.c
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Pinctrl / GPIO driver for StarFive JHB100 SoC System-0 domain
> + *
> + * Copyright (C) 2024 StarFive Technology Co., Ltd.
> + * Author: Alex Soo <yuklin.soo@starfivetech.com>
Shouldn't this person be in the Signed-off-by?
I guess it's not legally necessary but feels appropriate.
> +static struct config_reg_layout_desc jhb100_sys0_pinctrl_crl_desc[] = {
> + {
> + .pin_start = 0,
> + .pin_cnt = 4,
> + .drive_strength_2bit = { .shift = 0, .width = 2 },
> + .input_enable = { .shift = 2, .width = 1 },
> + .pull_down = { .shift = 3, .width = 1 },
> + .pull_up = { .shift = 4, .width = 1 },
> + .slew_rate = { .shift = 5, .width = 1 },
> + .schmitt_trigger_select = { .shift = 6, .width = 1 },
> + .reserved = { .shift = 7, .width = 8 },
> + .debounce_width = { .shift = 15, .width = 17 },
> + },
> + {
> + .pin_start = 4,
> + .pin_cnt = 5,
> + .schmitt_trigger_select = { .shift = 0, .width = 1 },
> + .reserved = { .shift = 1, .width = 31 },
> + },
> + {
> + .pin_start = 9,
> + .pin_cnt = 1,
> + .drive_strength_2bit = { .shift = 0, .width = 2 },
> + .slew_rate = { .shift = 2, .width = 1 },
> + .reserved = { .shift = 3, .width = 29 },
> + },
> + {
> + .pin_start = 10,
> + .pin_cnt = 1,
> + .drive_strength_2bit = { .shift = 0, .width = 2 },
> + .input_enable = { .shift = 2, .width = 1 },
> + .pull_down = { .shift = 3, .width = 1 },
> + .pull_up = { .shift = 4, .width = 1 },
> + .slew_rate = { .shift = 5, .width = 1 },
> + .schmitt_trigger_select = { .shift = 6, .width = 1 },
> + .reserved = { .shift = 7, .width = 25 },
> + },
> + { 0xff },
> +};
Would it be appropriate to index the different register variants with
a enum with a good name so it is easy to understand which
variant each entry in the array is?
> +#include <linux/string.h>
> +#include <linux/sort.h>
Hm why... I guess I will see.
> +#define JHB100_DEBOUNCE_WIDTH_STAGES_MAX 0x1FFFFU
Is that a GENMASK(16,0)?
Since it seems to have something to do with bitfield widths.
> +/* i2c open-drain pull-up select */
> +#define JHB100_I2C_OPEN_DRAIN_PU_600_OHMS 0
> +#define JHB100_I2C_OPEN_DRAIN_PU_900_OHMS 1
> +#define JHB100_I2C_OPEN_DRAIN_PU_1200_OHMS 2
> +#define JHB100_I2C_OPEN_DRAIN_PU_2000_OHMS 3
Very nice and to the point! It's easy to read and understand drivers
that are writing things out explicitly like this!
> +#define JHB100_NR_GPIOS_PER_BANK 32
(...)
> +static inline struct jhb100_gpio_bank *jhb100_gc_to_bank(struct gpio_chip *gc)
> +{
> + return container_of(gc, struct jhb100_gpio_bank, gc);
> +}
> +
> +static unsigned int jhb100_gpio_to_pin(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct jhb100_gpio_bank *bank = jhb100_gc_to_bank(gc);
> +
> + return bank->id * JHB100_NR_GPIOS_PER_BANK + gpio;
> +}
This usually tells me that GPIO_GENERIC can be used but maybe
this has been discussed before...
> +static const struct pinctrl_ops jhb100_pinctrl_ops = {
> + .get_groups_count = pinctrl_generic_get_group_count,
> + .get_group_name = pinctrl_generic_get_group_name,
> + .get_group_pins = pinctrl_generic_get_group_pins,
> + .dt_node_to_map = pinctrl_generic_pins_function_dt_node_to_map,
> + .dt_free_map = pinctrl_utils_free_map,
> +};
Nice use of the generic helpers!
> +static void jhb100_set_gpioval(struct jhb100_pinctrl *sfp, unsigned int pin,
> + unsigned int val)
> +{
> + const struct jhb100_pinctrl_domain_info *info = sfp->info;
> + unsigned int offset = 4 * (pin / 32);
> + unsigned int shift = 1 * (pin % 32);
> + unsigned int fs_offset = 4 * (pin / 16);
> + unsigned int fs_shift = 2 * (pin % 16);
> + u32 func_sel_mask;
> + u32 dout, doen, fs;
> + void __iomem *reg_gpio_o;
> + void __iomem *reg_gpio_oen;
> + void __iomem *reg_gpio_func_sel;
> + unsigned long flags;
> +
> + reg_gpio_o = sfp->base + info->regs->output + offset;
> + reg_gpio_oen = sfp->base + info->regs->output_en + offset;
> + reg_gpio_func_sel = sfp->base + info->regs->func_sel.reg + fs_offset;
The part from here:
> + func_sel_mask = GENMASK(info->regs->func_sel.width_per_pin - 1, 0) << fs_shift;
(...)
> +
> + raw_spin_lock_irqsave(&sfp->lock, flags);
> + fs = readl_relaxed(reg_gpio_func_sel);
> + if (fs & func_sel_mask) {
> + fs &= ~func_sel_mask;
> + writel_relaxed(fs, reg_gpio_func_sel);
> + }
..to here seems to reimplement the shortcut
.gpio_request_enable() in struct pinmux_ops.
Then this:
> + dout = val << shift;
> + doen = 0;
> + dout |= readl_relaxed(reg_gpio_o) & ~BIT(shift);
> + writel_relaxed(dout, reg_gpio_o);
> + doen |= readl_relaxed(reg_gpio_oen) & ~BIT(shift);
> + writel_relaxed(doen, reg_gpio_oen);
Seems more like the actual code that should be here.
> + raw_spin_unlock_irqrestore(&sfp->lock, flags);
Please use guards for these spinlocks. They make for less
bugs.
guard(raw_spinlock_irqsave)(&sfp->lock);
> +static const struct pinmux_ops jhb100_pinmux_ops = {
> + .get_functions_count = pinmux_generic_get_function_count,
> + .get_function_name = pinmux_generic_get_function_name,
> + .get_function_groups = pinmux_generic_get_function_groups,
> + .set_mux = jhb100_set_mux,
> +};
Implement .gpio_request_enable() (see above) and
.gpio_set_direction() see below.
Maybe also .gpio_disable_free() if you need to deconfigure
stuff when a pin is release from GPIO.
> +static const struct pinconf_ops jhb100_pinconf_ops = {
> + .pin_config_get = jhb100_pinconf_get,
> + .pin_config_set = jhb100_pinconf_set,
> + .pin_config_group_get = jhb100_pinconf_group_get,
> + .pin_config_group_set = jhb100_pinconf_group_set,
> + .is_generic = true,
> +};
Overall this looks nice, good use of the group config!
> +static int jhb100_gpio_get_direction(struct gpio_chip *gc,
> + unsigned int gpio)
> +{
> + struct jhb100_gpio_bank *bank = jhb100_gc_to_bank(gc);
> + struct jhb100_pinctrl *sfp = gpiochip_get_data(gc);
> + const struct jhb100_pinctrl_domain_info *info = sfp->info;
> + unsigned int offset = 4 * bank->id;
> + u32 doen;
> + void __iomem *reg_gpio_oen;
> +
> + reg_gpio_oen = sfp->base + info->regs->output_en + offset;
> +
> + doen = (readl_relaxed(reg_gpio_oen) & BIT(gpio)) >> gpio;
> +
> + return doen == GPOEN_ENABLE ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
> +}
> +
> +static int jhb100_gpio_direction_input(struct gpio_chip *gc,
> + unsigned int gpio)
> +{
> + struct jhb100_pinctrl *sfp = gpiochip_get_data(gc);
> + struct device *dev = sfp->dev;
> + struct config_reg_layout_desc *crl_desc;
> + unsigned int pin = jhb100_gpio_to_pin(gc, gpio);
> +
> + crl_desc = get_crl_desc_by_pin(sfp, pin);
> + if (!crl_desc) {
> + dev_err(dev, "pin %d can't not found reg layout descriptor\n",
> + pin);
> + return -EINVAL;
> + }
> +
> + jhb100_padcfg_rmw(sfp, pin,
> + RL_DESC_GENMASK(crl_desc, input_enable) |
> + RL_DESC_GENMASK(crl_desc, schmitt_trigger_select),
> + RL_DESC_GENMASK(crl_desc, input_enable) |
> + RL_DESC_GENMASK(crl_desc, schmitt_trigger_select));
Instead of doing these writes directly into the config registers, implement
.gpio_set_direction() in struct pinmux_ops and call the pinmux
generic back-end.
> +static int jhb100_gpio_direction_output(struct gpio_chip *gc,
> + unsigned int gpio, int value)
> +{
> + struct jhb100_pinctrl *sfp = gpiochip_get_data(gc);
> + struct device *dev = sfp->dev;
> + struct config_reg_layout_desc *crl_desc;
> + unsigned int pin = jhb100_gpio_to_pin(gc, gpio);
> +
> + jhb100_set_one_pin_mux(sfp, pin, 0,
> + value ? GPOUT_HIGH : GPOUT_LOW);
> +
> + crl_desc = get_crl_desc_by_pin(sfp, pin);
> + if (!crl_desc) {
> + dev_err(dev, "pin %d can't not found reg layout descriptor\n",
> + pin);
> + return -EINVAL;
> + }
> +
> + jhb100_padcfg_rmw(sfp, pin,
> + RL_DESC_GENMASK(crl_desc, input_enable) |
> + RL_DESC_GENMASK(crl_desc, schmitt_trigger_select) |
> + RL_DESC_GENMASK(crl_desc, pull_down) |
> + RL_DESC_GENMASK(crl_desc, pull_up),
> + 0);
Dito.
> +static int jhb100_gpio_get(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct jhb100_gpio_bank *bank = jhb100_gc_to_bank(gc);
> + struct jhb100_pinctrl *sfp = gpiochip_get_data(gc);
> + const struct jhb100_pinctrl_domain_info *info = sfp->info;
> + unsigned int offset = 4 * bank->id;
> + u32 doen = 0;
> + void __iomem *reg_gpio_oen;
> + void __iomem *reg;
> + unsigned long flags;
> +
> + reg_gpio_oen = sfp->base + info->regs->output_en + offset;
> + reg = sfp->base + info->regs->gpio_status + offset;
> +
> + raw_spin_lock_irqsave(&sfp->lock, flags);
> + doen = readl_relaxed(reg_gpio_oen) | BIT(gpio);
> + writel_relaxed(doen, reg_gpio_oen);
> + raw_spin_unlock_irqrestore(&sfp->lock, flags);
Why *on* *earth* are you read-modify-writing the output enable
register in the *get* function? Is this a copy-on-paste error??
> + return !!(readl_relaxed(reg) & BIT(gpio % 32));
Also you never actuall read reg .... ehhhh this is a glaring bug.
> +static int jhb100_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
> +{
> + struct jhb100_gpio_bank *bank = jhb100_gc_to_bank(gc);
> + struct jhb100_pinctrl *sfp = gpiochip_get_data(gc);
> + const struct jhb100_pinctrl_domain_info *info = sfp->info;
> + unsigned int offset = 4 * bank->id;
> + void __iomem *reg_dout;
> + u32 dout;
> + unsigned long flags;
> +
> + reg_dout = sfp->base + info->regs->output + offset;
> + dout = (value ? GPOUT_HIGH : GPOUT_LOW) << gpio;
> +
> + raw_spin_lock_irqsave(&sfp->lock, flags);
> + dout |= readl_relaxed(reg_dout) & ~BIT(gpio);
> + writel_relaxed(dout, reg_dout);
> + raw_spin_unlock_irqrestore(&sfp->lock, flags);
> +
> + return 0;
> +}
This looks right, did you only test output and not input..?
> +static const struct irq_chip jhb100_irq_chip = {
> + .irq_ack = jhb100_irq_ack,
> + .irq_mask = jhb100_irq_mask,
> + .irq_mask_ack = jhb100_irq_mask_ack,
> + .irq_unmask = jhb100_irq_unmask,
> + .irq_set_type = jhb100_irq_set_type,
> + .irq_set_wake = jhb100_irq_set_wake,
> + .irq_print_chip = jhb100_irq_print_chip,
> + .flags = IRQCHIP_SET_TYPE_MASKED |
> + IRQCHIP_IMMUTABLE |
> + IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND |
> + IRQCHIP_MASK_ON_SUSPEND |
> + IRQCHIP_SKIP_SET_WAKE,
> + GPIOCHIP_IRQ_RESOURCE_HELPERS,
> +};
The irqchip looks good!
> +static int field_compare(const void *a, const void *b)
> +{
> + const struct field_info *fa = (const struct field_info *)a;
> + const struct field_info *fb = (const struct field_info *)b;
> +
> + if (fa->shift < fb->shift)
> + return -1;
> +
> + if (fa->shift > fb->shift)
> + return 1;
> +
> + return 0;
> +}
Are you sure the kernel doesn't already have a helper like this...
> + sfp->num_banks = DIV_ROUND_UP(sfp->ngpios, JHB100_NR_GPIOS_PER_BANK);
> +
> + for (unsigned int i = 0; i < sfp->num_banks; i++) {
> + if (sfp->ngpios > (i + 1) * JHB100_NR_GPIOS_PER_BANK)
> + sfp->banks[i].gc.ngpio = (i + 1) * JHB100_NR_GPIOS_PER_BANK;
> + else
> + sfp->banks[i].gc.ngpio = sfp->ngpios - i * JHB100_NR_GPIOS_PER_BANK;
This looks completely bananas, shouldn't this be simply:
sfp->banks[i].gc.ngpio = JHB100_NR_GPIOS_PER_BANK;
???
What is getting assigned to ngpios looks like a gpiochip base, and have
all the signs of a real bad AI hallucination.
> +
> + sfp->banks[i].id = i;
> +
> + sfp->banks[i].gc.parent = dev;
> + sfp->banks[i].gc.label = dev_name(dev);
> + sfp->banks[i].gc.owner = THIS_MODULE;
> + sfp->banks[i].gc.request = pinctrl_gpio_request;
Use
gpiochip_generic_request
> + sfp->banks[i].gc.free = pinctrl_gpio_free;
Use
gpiochip_generic_free
These calls will do what you want, and also check that the
right gpio ranges are available.
Make sure you add GPIO ranges (the mapping between pin control
pins and corresponding GPIO offsets) for this to work properly.
I'm pretty sure you can have a generic pin config backend as well.
sfp->banks[i].gc.set_config = gpiochip_generic_config;
This will make config calls to the gpiochip call into the pinctrl
backend = what you want.
> + sfp->banks[i].gc.get_direction = jhb100_gpio_get_direction;
> + sfp->banks[i].gc.direction_input = jhb100_gpio_direction_input;
> + sfp->banks[i].gc.direction_output = jhb100_gpio_direction_output;
> + sfp->banks[i].gc.get = jhb100_gpio_get;
> + sfp->banks[i].gc.set = jhb100_gpio_set;
> + sfp->banks[i].gc.set_config = gpiochip_generic_config;
> + sfp->banks[i].gc.base = -1;
> + sfp->banks[i].gc.of_gpio_n_cells = 3;
> + sfp->banks[i].gc.of_node_instance_match = starfive_of_node_instance_match;
Since you have a threecell scheme with 32 gpios
(JHB100_NR_GPIOS_PER_BANK) per instance (right? the ngpios
code above made me really confused....)
you should be able so select GPIO_GENERIC,
#include <linux/gpio/generic.h> and use
the generic GPIO pretty much the same way
drivers/gpio/gpio-spacemit-k1.c does it, check that driver
out (especially spacemit_gpio_add_bank()).
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v3 08/18] arm64: dts: qcom: qcs8300: Add minidump SRAM config to SCM node
From: Konrad Dybcio @ 2026-06-11 11:37 UTC (permalink / raw)
To: Mukesh Ojha, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Marko,
Guru Das Srinagesh
Cc: cros-qcom-dts-watchers, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20260522195009.2961022-9-mukesh.ojha@oss.qualcomm.com>
On 5/22/26 9:49 PM, Mukesh Ojha wrote:
> Point the SCM node at the minidump config slot in the always-on SRAM.
> Boot firmware reads this word before DDR is initialised on a warm reset
> to decide where to deliver the minidump.
>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH v3 07/18] arm64: dts: qcom: sa8775p: Add minidump SRAM config to SCM node
From: Konrad Dybcio @ 2026-06-11 11:37 UTC (permalink / raw)
To: Mukesh Ojha, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Marko,
Guru Das Srinagesh
Cc: cros-qcom-dts-watchers, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20260522195009.2961022-8-mukesh.ojha@oss.qualcomm.com>
On 5/22/26 9:49 PM, Mukesh Ojha wrote:
> Point the SCM node at the minidump config slot in the always-on SRAM.
> Boot firmware reads this word before DDR is initialised on a warm reset
> to decide where to deliver the minidump.
>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH v3 17/18] arm64: dts: qcom: qcs615: Add minidump SRAM config to SCM node
From: Konrad Dybcio @ 2026-06-11 11:37 UTC (permalink / raw)
To: Mukesh Ojha, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Marko,
Guru Das Srinagesh
Cc: cros-qcom-dts-watchers, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20260522195009.2961022-18-mukesh.ojha@oss.qualcomm.com>
On 5/22/26 9:50 PM, Mukesh Ojha wrote:
> Point the SCM node at the minidump config slot in the always-on SRAM.
> Boot firmware reads this word before DDR is initialised on a warm reset
> to decide where to deliver the minidump.
>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH v3 04/18] firmware: qcom: scm: Add minidump SRAM support
From: Konrad Dybcio @ 2026-06-11 11:45 UTC (permalink / raw)
To: Mukesh Ojha, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Marko,
Guru Das Srinagesh
Cc: cros-qcom-dts-watchers, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20260522195009.2961022-5-mukesh.ojha@oss.qualcomm.com>
On 5/22/26 9:49 PM, Mukesh Ojha wrote:
> On most Qualcomm SoCs where minidump is supported, a word in always-on
> SRAM is shared between the kernel and boot firmware. Before DDR is
> initialised on the warm reset following a crash, firmware reads this
> word to decide if minidump is enabled and collect a minidump and where
> to deliver it (USB upload to a host, or save to local storage).
>
> The SRAM region is described by a 'sram' phandle on the SCM DT node.
> If the property is absent the feature is silently disabled, keeping
> existing SoCs unaffected.
>
> Expose a 'minidump_dest' module parameter (default: usb) so the user can
> select the destination. Only the string names "usb" or "storage" are
> acceptable values.
>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
[...]
> + for (i = 0; i < ARRAY_SIZE(minidump_dest_map); i++)
> + if (sysfs_streq(val, minidump_dest_map[i].name))
I'm not sure about sysfs_streq() specifically, but otherwise this lgtm
Konrad
^ permalink raw reply
* Re: [PATCH v9 2/2] leds: ltc3220: Add Support for LTC3220 18 channel LED Driver
From: Lee Jones @ 2026-06-11 11:47 UTC (permalink / raw)
To: Edelweise Escala
Cc: Pavel Machek, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-leds, devicetree, linux-kernel
In-Reply-To: <20260528-ltc3220-driver-v9-2-69450fc213cb@analog.com>
/* Sashiko Automation: Issues Found (10 Findings) */
Would you mind taking care of these before I conduct my next review please?
On Thu, 28 May 2026, Edelweise Escala wrote:
> Add driver for the LTC3220 18-channel LED driver
> with I2C interface, individual brightness control, and hardware-assisted
> blink/gradation features.
>
> Signed-off-by: Edelweise Escala <edelweise.escala@analog.com>
> ---
> MAINTAINERS | 1 +
> drivers/leds/Kconfig | 13 ++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-ltc3220.c | 440 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 455 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c8a242577d2f..0f553ada61d9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15229,6 +15229,7 @@ L: linux-leds@vger.kernel.org
> S: Maintained
> W: https://ez.analog.com/linux-software-drivers
> F: Documentation/devicetree/bindings/leds/adi,ltc3220.yaml
> +F: drivers/leds/leds-ltc3220.c
>
> LTC4282 HARDWARE MONITOR DRIVER
> M: Nuno Sa <nuno.sa@analog.com>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index f4a0a3c8c870..31b1e3ff094c 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -1000,6 +1000,19 @@ config LEDS_ST1202
> Say Y to enable support for LEDs connected to LED1202
> LED driver chips accessed via the I2C bus.
>
> +config LEDS_LTC3220
> + tristate "LED Driver for Analog Devices Inc. LTC3220"
> + depends on I2C && LEDS_CLASS
> + select REGMAP_I2C
> + help
> + Say Y to enable support for the Analog Devices LTC3220
> + 18-channel LED controller with I2C interface.
> + The driver supports individual LED brightness control (64 steps),
> + hardware-assisted blinking and gradation effects.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called leds-ltc3220.
> +
> config LEDS_TPS6105X
> tristate "LED support for TI TPS6105X"
> depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 7db3768912ca..a68244bd50fb 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -61,6 +61,7 @@ obj-$(CONFIG_LEDS_LP8788) += leds-lp8788.o
> obj-$(CONFIG_LEDS_LP8860) += leds-lp8860.o
> obj-$(CONFIG_LEDS_LP8864) += leds-lp8864.o
> obj-$(CONFIG_LEDS_LT3593) += leds-lt3593.o
> +obj-$(CONFIG_LEDS_LTC3220) += leds-ltc3220.o
> obj-$(CONFIG_LEDS_MAX5970) += leds-max5970.o
> obj-$(CONFIG_LEDS_MAX77650) += leds-max77650.o
> obj-$(CONFIG_LEDS_MAX77705) += leds-max77705.o
> diff --git a/drivers/leds/leds-ltc3220.c b/drivers/leds/leds-ltc3220.c
> new file mode 100644
> index 000000000000..a633ac5ca6ae
> --- /dev/null
> +++ b/drivers/leds/leds-ltc3220.c
> @@ -0,0 +1,440 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LTC3220 18-Channel LED Driver
> + *
> + * Copyright 2026 Analog Devices Inc.
> + *
> + * Author: Edelweise Escala <edelweise.escala@analog.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
[Severity: Low]
Is this include needed?
It doesn't appear that any delay functions like msleep() or usleep_range()
are used in the driver implementation.
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/leds.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
[Severity: Low]
Should this file include <linux/property.h>?
The driver uses fwnode property APIs such as device_for_each_child_node_scoped(),
fwnode_property_read_u32(), and fwnode_property_present(), which are defined
in <linux/property.h>.
> +
> +/* LTC3220 Registers */
> +#define LTC3220_COMMAND_REG 0x00
> +#define LTC3220_QUICK_WRITE_MASK BIT(0)
> +#define LTC3220_SHUTDOWN_MASK BIT(3)
> +
> +#define LTC3220_ULED_REG(x) (0x01 + (x))
> +#define LTC3220_LED_CURRENT_MASK GENMASK(5, 0)
> +#define LTC3220_LED_MODE_MASK GENMASK(7, 6)
> +
> +#define LTC3220_GRAD_BLINK_REG 0x13
> +#define LTC3220_GRADATION_MASK GENMASK(2, 0)
> +#define LTC3220_GRADATION_DIRECTION_MASK BIT(0)
> +#define LTC3220_GRADATION_PERIOD_MASK GENMASK(2, 1)
> +#define LTC3220_BLINK_MASK GENMASK(4, 3)
> +
> +#define LTC3220_NUM_LEDS 18
> +
> +#define LTC3220_GRADATION_START_VALUE 128
[Severity: Low]
Is this macro used anywhere in the driver?
It appears to be dead code as it is never referenced.
> +#define LTC3220_GRADATION_RAMP_TIME_240MS 240
> +#define LTC3220_GRADATION_RAMP_TIME_480MS 480
> +
> +#define LTC3220_BLINK_ON_156MS 156
> +#define LTC3220_BLINK_ON_625MS 625
> +#define LTC3220_BLINK_PERIOD_1250MS 1250
> +#define LTC3220_BLINK_PERIOD_2500MS 2500
> +
> +#define LTC3220_BLINK_SHORT_ON_TIME BIT(0)
> +#define LTC3220_BLINK_LONG_PERIOD BIT(1)
> +
> +enum ltc3220_led_mode {
> + LTC3220_NORMAL_MODE,
> + LTC3220_BLINK_MODE,
> + LTC3220_GRADATION_MODE,
> +};
> +
> +enum ltc3220_blink_mode {
> + LTC3220_BLINK_MODE_625MS_1250MS,
> + LTC3220_BLINK_MODE_156MS_1250MS,
> + LTC3220_BLINK_MODE_625MS_2500MS,
> + LTC3220_BLINK_MODE_156MS_2500MS
> +};
> +
> +enum ltc3220_gradation_mode {
> + LTC3220_GRADATION_MODE_DISABLED,
> + LTC3220_GRADATION_MODE_240MS_RAMP_TIME,
> + LTC3220_GRADATION_MODE_480MS_RAMP_TIME,
> + LTC3220_GRADATION_MODE_960MS_RAMP_TIME
> +};
> +
> +static bool ltc3220_volatile_reg(struct device *dev, unsigned int reg)
> +{
> + return reg == LTC3220_GRAD_BLINK_REG;
> +}
[Severity: High]
Does this register need to be marked volatile?
Marking a register as volatile tells regmap to bypass the cache and read from
the physical hardware. If the LTC3220 is a write-only I2C device, the physical
read will fail, causing regmap_update_bits() to fail entirely in
ltc3220_pattern_set() and ltc3220_blink_set().
Even if the device supports reads, this register is exclusively controlled by
software and does not change autonomously, so could it be cached normally to
avoid unnecessary I2C reads?
> +
> +static const struct regmap_config ltc3220_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = LTC3220_GRAD_BLINK_REG,
> + .cache_type = REGCACHE_FLAT_S,
> + .volatile_reg = ltc3220_volatile_reg,
> +};
> +
> +struct ltc3220_uled_cfg {
> + struct led_classdev led_cdev;
> + u8 reg_value;
> + u8 led_index;
> +};
> +
> +struct ltc3220 {
> + struct ltc3220_uled_cfg uled_cfg[LTC3220_NUM_LEDS];
> + struct regmap *regmap;
> + bool is_aggregated;
> +};
> +
> +/*
> + * Set LED brightness. Hardware supports 0-63 brightness levels.
> + * Mode switching (blink/gradation) is handled through dedicated callbacks.
> + */
> +static int ltc3220_set_led_data(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
> + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg,
> + led_cdev);
> + struct ltc3220 *ltc3220 = container_of(uled_cfg, struct ltc3220,
> + uled_cfg[uled_cfg->led_index]);
[Severity: Low]
Will this dynamic variable index inside container_of() cause a build failure?
The driver attempts to retrieve the parent structure using
uled_cfg[uled_cfg->led_index]. The container_of macro relies on offsetof(),
which strictly requires the member argument to be a constant expression.
Since uled_cfg->led_index is a runtime variable, this violates the C standard
and can abort compilation on compilers like Clang with an error about a
non-constant integer expression.
> + int ret;
> +
> + ret = regmap_write(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg->led_index),
> + brightness);
> + if (ret)
> + return ret;
> +
> + uled_cfg->reg_value = brightness;
> +
> + /*
> + * When aggregated LED mode is enabled, writing to LED 1 updates all
> + * LEDs simultaneously via quick-write mode. Update cached values for
> + * all LEDs to reflect the synchronized state.
> + * See Documentation/devicetree/bindings/leds/adi,ltc3220.yaml for how
> + * to configure aggregated LED mode.
> + */
> + if (ltc3220->is_aggregated && uled_cfg->led_index == 0) {
> + for (int i = 0; i < LTC3220_NUM_LEDS; i++)
> + ltc3220->uled_cfg[i].reg_value = brightness;
> + }
> +
> + return 0;
> +}
> +
> +static enum led_brightness ltc3220_get_led_data(struct led_classdev *led_cdev)
> +{
> + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg,
> + led_cdev);
> +
> + return uled_cfg->reg_value;
> +}
> +
> +/*
> + * LTC3220 pattern support for hardware-assisted breathing/gradation.
> + * The hardware supports 3 gradation ramp time 240ms, 480ms, 960ms)
> + * and can ramp up or down.
> + *
> + * Pattern array interpretation:
> + * pattern[0].brightness = start brightness (0-63)
> + * pattern[0].delta_t = ramp time in milliseconds
> + * pattern[1].brightness = end brightness (0-63)
> + * pattern[1].delta_t = (optional, can be 0 or same as pattern[0].delta_t)
> + */
> +static int ltc3220_pattern_set(struct led_classdev *led_cdev,
> + struct led_pattern *pattern,
> + u32 len, int repeat)
> +{
> + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg,
> + led_cdev);
> + struct ltc3220 *ltc3220 = container_of(uled_cfg, struct ltc3220,
> + uled_cfg[uled_cfg->led_index]);
[Severity: Low]
Will this dynamic variable index inside container_of() cause a build failure?
The driver attempts to retrieve the parent structure using
uled_cfg[uled_cfg->led_index]. The container_of macro relies on offsetof(),
which strictly requires the member argument to be a constant expression.
Since uled_cfg->led_index is a runtime variable, this violates the C standard
and can abort compilation on compilers like Clang with an error about a
non-constant integer expression.
> + u8 gradation_period;
> + u8 start_brightness;
> + u8 end_brightness;
> + u8 gradation_val;
> + bool is_increasing;
> + int ret;
> +
> + if (len != 2)
> + return -EINVAL;
> +
> + start_brightness = pattern[0].brightness & LTC3220_LED_CURRENT_MASK;
> + end_brightness = pattern[1].brightness & LTC3220_LED_CURRENT_MASK;
> +
> + is_increasing = end_brightness > start_brightness;
> +
> + if (pattern[0].delta_t == 0)
> + gradation_period = LTC3220_GRADATION_MODE_DISABLED;
> + else if (pattern[0].delta_t <= LTC3220_GRADATION_RAMP_TIME_240MS)
> + gradation_period = LTC3220_GRADATION_MODE_240MS_RAMP_TIME;
> + else if (pattern[0].delta_t <= LTC3220_GRADATION_RAMP_TIME_480MS)
> + gradation_period = LTC3220_GRADATION_MODE_480MS_RAMP_TIME;
> + else
> + gradation_period = LTC3220_GRADATION_MODE_960MS_RAMP_TIME;
> +
> + gradation_val = FIELD_PREP(LTC3220_GRADATION_PERIOD_MASK, gradation_period);
> + gradation_val |= FIELD_PREP(LTC3220_GRADATION_DIRECTION_MASK, is_increasing);
> +
> + ret = regmap_update_bits(ltc3220->regmap, LTC3220_GRAD_BLINK_REG,
> + LTC3220_GRADATION_MASK, gradation_val);
> + if (ret)
> + return ret;
> +
> + ret = ltc3220_set_led_data(led_cdev, start_brightness);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg->led_index),
> + FIELD_PREP(LTC3220_LED_MODE_MASK, LTC3220_GRADATION_MODE) |
> + end_brightness);
> + if (ret)
> + return ret;
> +
> + uled_cfg->reg_value = end_brightness;
> +
> + return 0;
> +}
> +
> +static int ltc3220_pattern_clear(struct led_classdev *led_cdev)
> +{
> + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg,
> + led_cdev);
> + struct ltc3220 *ltc3220 = container_of(uled_cfg, struct ltc3220,
> + uled_cfg[uled_cfg->led_index]);
[Severity: Low]
Will this dynamic variable index inside container_of() cause a build failure?
The driver attempts to retrieve the parent structure using
uled_cfg[uled_cfg->led_index]. The container_of macro relies on offsetof(),
which strictly requires the member argument to be a constant expression.
Since uled_cfg->led_index is a runtime variable, this violates the C standard
and can abort compilation on compilers like Clang with an error about a
non-constant integer expression.
> + int ret;
> +
> + ret = regmap_update_bits(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg->led_index),
> + LTC3220_LED_MODE_MASK, LTC3220_NORMAL_MODE);
> + if (ret)
> + return ret;
> +
> + return ltc3220_set_led_data(led_cdev, LED_OFF);
> +}
> +
> +/*
> + * LTC3220 has a global blink configuration that affects all LEDs.
> + * This implementation allows per-LED blink requests, but the blink timing
> + * will be shared across all LEDs. The delay values are mapped to the
> + * hardware's discrete blink rates.
> + */
> +static int ltc3220_blink_set(struct led_classdev *led_cdev,
> + unsigned long *delay_on,
> + unsigned long *delay_off)
> +{
> + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg,
> + led_cdev);
> + struct ltc3220 *ltc3220 = container_of(uled_cfg, struct ltc3220,
> + uled_cfg[uled_cfg->led_index]);
[Severity: Low]
Will this dynamic variable index inside container_of() cause a build failure?
The driver attempts to retrieve the parent structure using
uled_cfg[uled_cfg->led_index]. The container_of macro relies on offsetof(),
which strictly requires the member argument to be a constant expression.
Since uled_cfg->led_index is a runtime variable, this violates the C standard
and can abort compilation on compilers like Clang with an error about a
non-constant integer expression.
> + u8 blink_mode = 0;
> + int ret;
> +
> + if (*delay_on <= LTC3220_BLINK_ON_156MS)
> + blink_mode = LTC3220_BLINK_SHORT_ON_TIME;
> +
> + if (*delay_on + *delay_off > LTC3220_BLINK_PERIOD_1250MS)
> + blink_mode |= LTC3220_BLINK_LONG_PERIOD;
> +
> + switch (blink_mode) {
> + case LTC3220_BLINK_MODE_625MS_1250MS:
> + *delay_on = LTC3220_BLINK_ON_625MS;
> + *delay_off = LTC3220_BLINK_PERIOD_1250MS - LTC3220_BLINK_ON_625MS;
> + break;
> + case LTC3220_BLINK_MODE_156MS_1250MS:
> + *delay_on = LTC3220_BLINK_ON_156MS;
> + *delay_off = LTC3220_BLINK_PERIOD_1250MS - LTC3220_BLINK_ON_156MS;
> + break;
> + case LTC3220_BLINK_MODE_625MS_2500MS:
> + *delay_on = LTC3220_BLINK_ON_625MS;
> + *delay_off = LTC3220_BLINK_PERIOD_2500MS - LTC3220_BLINK_ON_625MS;
> + break;
> + case LTC3220_BLINK_MODE_156MS_2500MS:
> + *delay_on = LTC3220_BLINK_ON_156MS;
> + *delay_off = LTC3220_BLINK_PERIOD_2500MS - LTC3220_BLINK_ON_156MS;
> + break;
> + }
> +
> + ret = regmap_update_bits(ltc3220->regmap, LTC3220_GRAD_BLINK_REG,
> + LTC3220_BLINK_MASK, FIELD_PREP(LTC3220_BLINK_MASK, blink_mode));
> + if (ret)
> + return ret;
> +
> + return regmap_update_bits(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg->led_index),
> + LTC3220_LED_MODE_MASK,
> + FIELD_PREP(LTC3220_LED_MODE_MASK, LTC3220_BLINK_MODE));
> +}
[Severity: Medium]
Will the hardware blink activate at 0 brightness if the LED was previously off?
When the LED is off, its current bits in LTC3220_ULED_REG are 0. The driver
updates the mode bits to LTC3220_BLINK_MODE using regmap_update_bits() but
does not update the brightness bits.
Because the core expects the driver to handle brightness configuration for
hardware blinking, could this hardware blink execute at 0 brightness, leaving
the LED unlit?
> +
> +static void ltc3220_reset_gpio_action(void *data)
> +{
> + struct gpio_desc *reset_gpio = data;
> +
> + gpiod_set_value_cansleep(reset_gpio, 1);
> +}
> +
> +static int ltc3220_reset(struct ltc3220 *ltc3220, struct i2c_client *client)
> +{
> + struct gpio_desc *reset_gpio;
> + int ret;
> +
> + reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(reset_gpio))
> + return dev_err_probe(&client->dev, PTR_ERR(reset_gpio), "Failed on reset GPIO\n");
> +
> + if (reset_gpio) {
> + gpiod_set_value_cansleep(reset_gpio, 0);
[Severity: High]
Does this sequence lack the necessary delays to ensure a successful reset?
The driver asserts the reset GPIO by requesting it with GPIOD_OUT_HIGH and
immediately de-asserts it with gpiod_set_value_cansleep(reset_gpio, 0). There
is no delay between these calls, creating a near-zero width pulse that may fail
to meet the IC's minimum reset pulse width requirement.
It also lacks a wakeup delay before subsequent I2C commands might be sent.
> +
> + return devm_add_action_or_reset(&client->dev, ltc3220_reset_gpio_action,
> + reset_gpio);
[Severity: High]
Does returning early here skip software initialization and regmap cache setup?
If the reset_gpio is successfully acquired, this function immediately returns
0, entirely skipping the subsequent regmap_write() calls intended to initialize
the hardware registers.
Because the driver does not define a .reg_defaults table, skipping these writes
leaves the regmap cache uninitialized, which can break subsequent
read-modify-write logic via regmap_update_bits().
> + }
> +
> + ret = regmap_write(ltc3220->regmap, LTC3220_COMMAND_REG, 0);
> + if (ret)
> + return ret;
> +
> + for (int i = 0; i < LTC3220_NUM_LEDS; i++) {
> + ret = regmap_write(ltc3220->regmap, LTC3220_ULED_REG(i), 0);
> + if (ret)
> + return ret;
> + }
> +
> + return regmap_write(ltc3220->regmap, LTC3220_GRAD_BLINK_REG, 0);
> +}
> +
> +static int ltc3220_suspend(struct device *dev)
> +{
> + struct ltc3220 *ltc3220 = i2c_get_clientdata(to_i2c_client(dev));
> +
> + return regmap_update_bits(ltc3220->regmap, LTC3220_COMMAND_REG,
> + LTC3220_SHUTDOWN_MASK, LTC3220_SHUTDOWN_MASK);
> +}
> +
> +static int ltc3220_resume(struct device *dev)
> +{
> + struct ltc3220 *ltc3220 = i2c_get_clientdata(to_i2c_client(dev));
> +
> + return regmap_update_bits(ltc3220->regmap, LTC3220_COMMAND_REG,
> + LTC3220_SHUTDOWN_MASK, 0);
> +}
[Severity: High]
Are the hardware registers left permanently off after resume?
In ltc3220_suspend(), the driver sets the LTC3220_SHUTDOWN_MASK bit. On many
such chips, asserting the shutdown bit resets internal registers. Additionally,
power to the IC may be lost during system suspend.
In ltc3220_resume(), the driver clears the shutdown bit but does not call
regcache_mark_dirty() and regcache_sync(). Could this leave the physical LED
outputs permanently off, while the kernel's regmap cache and LED classdev state
incorrectly reflect that the LEDs are still configured and illuminated?
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(ltc3220_pm_ops, ltc3220_suspend, ltc3220_resume);
> +
> +static int ltc3220_probe(struct i2c_client *client)
> +{
> + struct ltc3220 *ltc3220;
> + bool aggregated_led_found = false;
> + int num_leds = 0;
> + u8 led_index = 0;
> + int ret;
> +
> + ltc3220 = devm_kzalloc(&client->dev, sizeof(*ltc3220), GFP_KERNEL);
> + if (!ltc3220)
> + return -ENOMEM;
> +
> + ltc3220->regmap = devm_regmap_init_i2c(client, <c3220_regmap_config);
> + if (IS_ERR(ltc3220->regmap))
> + return dev_err_probe(&client->dev, PTR_ERR(ltc3220->regmap),
> + "Failed to initialize regmap\n");
> +
> + i2c_set_clientdata(client, ltc3220);
> +
> + ret = ltc3220_reset(ltc3220, client);
> + if (ret)
> + return dev_err_probe(&client->dev, ret, "Failed to reset device\n");
> +
> + device_for_each_child_node_scoped(&client->dev, child) {
> + struct led_init_data init_data = {};
> + struct ltc3220_uled_cfg *led;
> + u32 source;
> +
> + ret = fwnode_property_read_u32(child, "reg", &source);
> + if (ret)
> + return dev_err_probe(&client->dev, ret, "Couldn't read LED address\n");
> +
> + if (!source || source > LTC3220_NUM_LEDS)
> + return dev_err_probe(&client->dev, -EINVAL, "LED address out of range\n");
> +
> + init_data.fwnode = child;
> + init_data.devicename = "ltc3220";
> +
> + if (fwnode_property_present(child, "led-sources")) {
> + if (source != 1)
> + return dev_err_probe(&client->dev, -EINVAL,
> + "Aggregated LED out of range\n");
> +
> + if (aggregated_led_found)
> + return dev_err_probe(&client->dev, -EINVAL,
> + "One Aggregated LED only\n");
> +
> + aggregated_led_found = true;
> + ltc3220->is_aggregated = true;
> +
> + ret = regmap_update_bits(ltc3220->regmap,
> + LTC3220_COMMAND_REG,
> + LTC3220_QUICK_WRITE_MASK,
> + LTC3220_QUICK_WRITE_MASK);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "Failed to set quick write mode\n");
> + }
> +
> + num_leds++;
> +
> + /* LED node reg/index/address goes from 1 to 18 */
> + led_index = source - 1;
> + led = <c3220->uled_cfg[led_index];
> +
> + if (led->led_cdev.dev)
> + return dev_err_probe(&client->dev, -EINVAL,
> + "Duplicate LED reg %u found", source);
> +
> + led->led_index = led_index;
> + led->reg_value = 0;
> + led->led_cdev.brightness_set_blocking = ltc3220_set_led_data;
> + led->led_cdev.brightness_get = ltc3220_get_led_data;
> + led->led_cdev.max_brightness = 63;
> + led->led_cdev.blink_set = ltc3220_blink_set;
> + led->led_cdev.pattern_set = ltc3220_pattern_set;
> + led->led_cdev.pattern_clear = ltc3220_pattern_clear;
> +
> + ret = devm_led_classdev_register_ext(&client->dev, &led->led_cdev, &init_data);
> + if (ret)
> + return dev_err_probe(&client->dev, ret, "Failed to register LED class\n");
> + }
> +
> + /*
> + * Aggregated LED mode uses hardware quick-write to control all 18 LEDs
> + * simultaneously. This is mutually exclusive with individual LED control.
> + * See Documentation/devicetree/bindings/leds/adi,ltc3220.yaml for details
> + * on how to configure aggregated LED mode.
> + */
> + if (aggregated_led_found && num_leds > 1)
> + return dev_err_probe(&client->dev, -EINVAL,
> + "Aggregated LED must be the only LED node\n");
[Severity: Medium]
Are the LED sysfs interfaces exposed to userspace before the driver probe is
fully validated?
Inside the loop above, devm_led_classdev_register_ext() is called, which
immediately exposes sysfs interfaces. If the validation check here fails and
returns an error, userspace could have already discovered and interacted with
the LED devices before they are unexpectedly torn down by devres.
Could these validation checks be completed before registering the interfaces?
> +
> + return 0;
> +}
> +
> +static const struct of_device_id ltc3220_of_match[] = {
> + { .compatible = "adi,ltc3220" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ltc3220_of_match);
> +
> +static struct i2c_driver ltc3220_led_driver = {
> + .driver = {
> + .name = "ltc3220",
> + .of_match_table = ltc3220_of_match,
> + .pm = pm_sleep_ptr(<c3220_pm_ops),
> + },
> + .probe = ltc3220_probe,
> +};
> +module_i2c_driver(ltc3220_led_driver);
> +
> +MODULE_AUTHOR("Edelweise Escala <edelweise.escala@analog.com>");
> +MODULE_DESCRIPTION("LED driver for LTC3220 controllers");
> +MODULE_LICENSE("GPL");
>
> --
> 2.43.0
>
--
Lee Jones
^ 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