Devicetree
 help / color / mirror / Atom feed
* Re: [PATCH v5 10/16] media: iris: add minimal GET_PROPERTY implementation
From: sashiko-bot @ 2026-06-16  0:20 UTC (permalink / raw)
  To: Dmitry Baryshkov; +Cc: conor+dt, devicetree, robh
In-Reply-To: <20260616-iris-ar50lt-v5-10-583b42770b6a@oss.qualcomm.com>

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Use-After-Free due to missing reference counting in instance lookup.
- [High] Unsafe release of instance lock during firmware wait allows concurrent IOCTLs to clobber completion state.
--

commit d766418e0dabcbcbb8dc9a3f16b8a80a51ec61f4
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

media: iris: add minimal GET_PROPERTY implementation

This commit adds minimal support for querying properties from the firmware
to satisfy Gen1 firmware requirements for AR50Lt. It implements the
session_get_property operation and handles the resulting property info
messages.

> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> index 7674b47ad6c49..99e82e5510abe 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> @@ -1117,10 +1117,31 @@ static int iris_hfi_gen1_session_set_config_params(struct iris_inst *inst, u32 p
>  	return 0;
>  }
>  
> +static int iris_hfi_gen1_session_get_property(struct iris_inst *inst, u32 packet_type)
> +{
> +	struct hfi_session_get_property_pkt pkt;
> +	int ret;
> +
> +	pkt.shdr.hdr.size = sizeof(pkt);
> +	pkt.shdr.hdr.pkt_type = HFI_CMD_SESSION_GET_PROPERTY;
> +	pkt.shdr.session_id = inst->session_id;
> +	pkt.num_properties = 1;
> +	pkt.data = packet_type;
> +
> +	reinit_completion(&inst->completion);
> +
> +	ret = iris_hfi_queue_cmd_write(inst->core, &pkt, pkt.shdr.hdr.size);
> +	if (ret)
> +		return ret;
> +
> +	return iris_wait_for_session_response(inst, false);
> +}

[Severity: High]
This is a pre-existing issue, but the newly added function utilizes a pattern
that might be unsafe.

In iris_wait_for_session_response(), inst->lock is explicitly dropped while
waiting for inst->completion. Because the driver does not appear to use
vdev->lock to serialize V4L2 ioctls, dropping inst->lock could expose the
driver to concurrent ioctls.

Could a second thread acquire inst->lock, issue another firmware command,
and call reinit_completion(&inst->completion), erasing the state the first
thread is waiting on? 

If so, when the firmware finally responds, complete(&inst->completion) might
arbitrarily wake up the wrong thread and desynchronize the driver state.

> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
> index bfd7495bf44f0..23fc7194b1e3a 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
[ ... ]
> @@ -652,6 +656,8 @@ static void iris_hfi_gen1_handle_response(struct iris_core *core, void *response
>  			iris_hfi_gen1_session_etb_done(inst, hdr);
>  		} else if (hdr->pkt_type == HFI_MSG_SESSION_FILL_BUFFER) {
>  			iris_hfi_gen1_session_ftb_done(inst, hdr);
> +		} else if (hdr->pkt_type == HFI_MSG_SESSION_PROPERTY_INFO) {
> +			complete(&inst->completion);

[Severity: Critical]
This isn't a bug introduced by this patch, but there appears to be a
critical structural flaw with how the instance is handled here.

Earlier in iris_hfi_gen1_handle_response(), iris_get_instance() looks up an
iris_inst from the core->instances list while holding core->lock, but returns
the raw pointer without incrementing any reference count, and then drops
core->lock.

The response handler then subsequently attempts to acquire
mutex_lock(&inst->lock). If a concurrent thread executing iris_close()
acquires core->lock via iris_remove_session(), removes the instance from
the list, and then calls kfree(inst), could the IRQ handler end up
executing a locking operation on freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-iris-ar50lt-v5-0-583b42770b6a@oss.qualcomm.com?part=10

^ permalink raw reply

* Re: [PATCH v2 2/6] iommu/arm-smmu: Add interconnect bandwidth voting support
From: Dmitry Baryshkov @ 2026-06-16  0:21 UTC (permalink / raw)
  To: Bibek Kumar Patro
  Cc: Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
	linux-arm-kernel, iommu, devicetree, linux-kernel, linux-arm-msm
In-Reply-To: <d25f24ca-5bb3-4276-ac8f-8340e8fb4ce8@oss.qualcomm.com>

On Mon, Jun 15, 2026 at 06:36:51PM +0530, Bibek Kumar Patro wrote:
> 
> 
> On 6/8/2026 7:25 PM, Dmitry Baryshkov wrote:
> > On Tue, May 26, 2026 at 08:12:03PM +0530, Bibek Kumar Patro wrote:
> > > On some SoCs the SMMU registers require an active interconnect
> > > bandwidth vote to be accessible. While other clients typically
> > > satisfy this requirement implicitly, certain corner cases (e.g.
> > > during sleep/wakeup transitions) can leave the SMMU without a
> > > vote, causing intermittent register access failures.
> > > 
> > > Add support for an optional interconnect path to the arm-smmu
> > > driver and vote for bandwidth while the SMMU is active. The path
> > > is acquired from DT if present and ignored otherwise.
> > > 
> > > The bandwidth vote is enabled before accessing SMMU registers
> > > during probe and runtime resume, and released during runtime
> > > suspend and on error paths.
> > > 
> > > Generally, from an architectural perspective, GEM_NOC and DDR are
> > > expected to have an active vote whenever the adreno_smmu block is
> > > powered on. In most common use cases, this requirement is implicitly
> > > satisfied because other GPU-related clients (for example, the GMU
> > > device) already hold a GEM_NOC vote when adreno_smmu is enabled.
> > > 
> > > However, there are certain corner cases, such as during sleep/wakeup
> > > transitions, where the GEM_NOC vote can be removed before adreno_smmu
> > > is powered down. If adreno_smmu is then accessed while the interconnect
> > > vote is missing, it can lead to the observed failures. Because of the
> > > precise ordering involved, this scenario is difficult to reproduce
> > > consistently.
> > > (also GDSC is involved in adreno usecases can have an independent vote)
> > > 
> > > Signed-off-by: Bibek Kumar Patro <bibek.patro@oss.qualcomm.com>
> > > ---
> > >   drivers/iommu/arm/arm-smmu/arm-smmu.c | 57 +++++++++++++++++++++++++++++++++--
> > >   drivers/iommu/arm/arm-smmu/arm-smmu.h |  2 ++
> > >   2 files changed, 57 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > index 0bd21d206eb3e75c3b9fb1364cdc92e82c5aa499..07c7e44ec6a5bd1488f00f87d859a20495e46601 100644
> > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > @@ -53,6 +53,11 @@
> > >   #define MSI_IOVA_BASE			0x8000000
> > >   #define MSI_IOVA_LENGTH			0x100000
> > > +/* Interconnect bandwidth vote values for the SMMU register access path */
> > > +#define ARM_SMMU_ICC_AVG_BW		0
> > > +#define ARM_SMMU_ICC_PEAK_BW_HIGH	1000
> > 
> > totally random numbers, which might be different for non-Qualcomm platform.
> > 
> 
> Ideally, any non-zero value would be enough to keep the path active.

This is true for Qualcomm devices. However, you are adding this to a
generic code.

> Here 1 Would be enough to keep the path active, but might be too small to
> reliably keep the bus active.
> Other is UINT_MAX, which will reliably keep the bus active but might cause a
> power penalty.
> 
> #define ARM_SMMU_ICC_PEAK_BW_HIGH	UINT_MAX
> 
> seems to be suitable here to reliably keep the bus active by BCM
> for both Qualcomm and non-Qualcomm platforms (with some power penalty).
> 
> LMK, if you feel otherwise.

Shift it to the qcom instance or provide platform-specific values? (My
preference would be towards the first solution).

> 
> 
> > > +#define ARM_SMMU_ICC_PEAK_BW_LOW	0
> > > +
> > >   static int force_stage;
> > >   module_param(force_stage, int, S_IRUGO);
> > >   MODULE_PARM_DESC(force_stage,

-- 
With best wishes
Dmitry

^ permalink raw reply

* Re: [PATCH v5 13/16] media: iris: Introduce buffer size calculations for AR50LT
From: sashiko-bot @ 2026-06-16  0:21 UTC (permalink / raw)
  To: Dmitry Baryshkov; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260616-iris-ar50lt-v5-13-583b42770b6a@oss.qualcomm.com>

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The AR50LT decoder line buffer size calculations accept an `is_opb` parameter but completely ignore it, failing to allocate memory for the VPSS line buffer when OPB (split mode) is enabled.
- [High] The AR50LT BIN buffer size calculations for H.264 and HEVC ignore `num_vpp_pipes`, preventing proper buffer partitioning and alignment across hardware pipelines.
- [High] The VP9 AR50LT line buffer calculation omits critical component sizes for DMA metadata and QP maps.
- [High] Missing SEI user data allocation in the H.264 AR50LT persist buffer calculation.
--

commit 41b73900b6c3face3194c0a45272d5dc37396dab
Author: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>

media: iris: Introduce buffer size calculations for AR50LT
    
Introduces AR50LT  buffer size calculation for both encoder and
decoder. Reuse the buffer size calculation which are common, while
adding the AR50LT specific ones separately.

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_buffer.c b/drivers/media/platform/qcom/iris/iris_vpu_buffer.c
> index 4a39b8fef52b0..ca03d65705136 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_buffer.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_buffer.c
> @@ -50,6 +50,32 @@ static u32 hfi_buffer_bin_h264d(u32 frame_width, u32 frame_height, u32 num_vpp_p
>  	return size_h264d_hw_bin_buffer(n_aligned_w, n_aligned_h, num_vpp_pipes);
>  }
>  
> +static u32 size_h264d_hw_bin_buffer_ar50lt(u32 frame_width, u32 frame_height, u32 num_vpp_pipes)
> +{
> +	u32 size_yuv, size_bin_hdr, size_bin_res;
> +
> +	size_yuv = ((frame_width * frame_height * 3) >> 1);
> +	if (size_yuv <= 1920 * 1088 * 3 / 2) {
> +		size_bin_hdr = size_yuv * H264_CABAC_HDR_RATIO_SM_TOT;
> +		size_bin_res = size_yuv * H264_CABAC_RES_RATIO_SM_TOT;
> +	} else {
> +		size_bin_hdr = (size_yuv * 3) / 5;
> +		size_bin_res = (size_yuv * 3) / 2;
> +	}
> +	size_bin_hdr = ALIGN(size_bin_hdr, DMA_ALIGNMENT);
> +	size_bin_res = ALIGN(size_bin_res, DMA_ALIGNMENT);
> +
> +	return size_bin_hdr + size_bin_res;
> +}

[Severity: High]
Will this cause incorrect buffer sizing when multiple VPP pipelines are active?
The num_vpp_pipes argument is accepted but doesn't seem to be used to
partition and align the buffer components like it is in the generic
size_h264d_hw_bin_buffer() calculation.

[ ... ]

> @@ -111,6 +152,32 @@ static u32 hfi_buffer_bin_h265d(u32 frame_width, u32 frame_height, u32 num_vpp_p
>  	return size_h265d_hw_bin_buffer(n_aligned_w, n_aligned_h, num_vpp_pipes);
>  }
>  
> +static u32 size_h265d_hw_bin_buffer_ar50lt(u32 frame_width, u32 frame_height, u32 num_vpp_pipes)
> +{
> +	u32 size_yuv, size_bin_hdr, size_bin_res;
> +
> +	size_yuv = ((frame_width * frame_height * 3) >> 1);
> +	if (size_yuv <= ((BIN_BUFFER_THRESHOLD * 3) >> 1)) {
> +		size_bin_hdr = size_yuv * H265_CABAC_HDR_RATIO_SM_TOT;
> +		size_bin_res = size_yuv * H265_CABAC_RES_RATIO_SM_TOT;
> +	} else {
> +		size_bin_hdr = (size_yuv * 41) / 50;
> +		size_bin_res = (size_yuv * 59) / 50;
> +	}
> +	size_bin_hdr = ALIGN(size_bin_hdr, DMA_ALIGNMENT);
> +	size_bin_res = ALIGN(size_bin_res, DMA_ALIGNMENT);
> +
> +	return size_bin_hdr + size_bin_res;
> +}

[Severity: High]
Similar to the H.264 function above, does this missing num_vpp_pipes
partitioning logic lead to internal misalignment for secondary pipes?

[ ... ]

> @@ -230,6 +341,11 @@ static u32 hfi_buffer_persist_h264d(void)
>  		    DMA_ALIGNMENT);
>  }
>  
> +static u32 hfi_buffer_persist_h264d_ar50lt(void)
> +{
> +	return ALIGN((SIZE_SLIST_BUF_H264 * NUM_SLIST_BUF_H264), DMA_ALIGNMENT);
> +}

[Severity: High]
Does this calculation inadvertently omit the SEI user data allocation?

The original hfi_buffer_persist_h264d() and the new AR50LT HEVC function both
include NUM_HW_PIC_BUF * SIZE_SEI_USERDATA, so leaving it out here might
cause the hardware to write out of bounds during SEI metadata extraction.

[ ... ]

> @@ -404,6 +584,29 @@ u32 hfi_buffer_line_h265d(u32 frame_width, u32 frame_height, bool is_opb, u32 nu
>  	return ALIGN((_size + vpss_lb_size), DMA_ALIGNMENT);
>  }
>  
> +static inline
> +u32 hfi_buffer_line_h265d_ar50lt(u32 frame_width, u32 frame_height, bool is_opb, u32 num_vpp_pipes)
> +{
> +	u32 size;
> +
> +	size = ALIGN(size_h265d_lb_fe_top_data_ar50lt(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_h265d_lb_fe_top_ctrl(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_h265d_lb_fe_left_ctrl(frame_width, frame_height),
> +		      DMA_ALIGNMENT) * num_vpp_pipes +
> +		ALIGN(size_h265d_lb_se_left_ctrl_ar50lt(frame_width, frame_height),
> +		      DMA_ALIGNMENT) * num_vpp_pipes +
> +		ALIGN(size_h265d_lb_se_top_ctrl(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_h265d_lb_pe_top_data_ar50lt(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_h265d_lb_vsp_top(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_h265d_lb_vsp_left(frame_width, frame_height),
> +		      DMA_ALIGNMENT) * num_vpp_pipes +
> +		ALIGN(size_h265d_lb_recon_dma_metadata_wr(frame_width, frame_height),
> +		      DMA_ALIGNMENT) * 4 +
> +		ALIGN(size_h265d_qp(frame_width, frame_height), DMA_ALIGNMENT);
> +
> +	return ALIGN(size, DMA_ALIGNMENT);
> +}

[Severity: High]
Is the is_opb parameter intentionally ignored here?

Without conditionally adding size_vpss_lb() when is_opb is true, could the
returned line buffer size be significantly undersized during split mode
operations, potentially leading to memory corruption?

[ ... ]

> @@ -492,6 +706,19 @@ u32 hfi_iris3_vp9d_lb_size(u32 frame_width, u32 frame_height, u32 num_vpp_pipes)
>  		ALIGN(size_vp9d_qp(frame_width, frame_height), DMA_ALIGNMENT);
>  }
>  
> +static inline
> +u32 hfi_ar50lt_vp9d_lb_size(u32 frame_width, u32 frame_height, u32 num_vpp_pipes)
> +{
> +	return ALIGN(size_vpxd_lb_fe_left_ctrl(frame_width, frame_height), DMA_ALIGNMENT) *
> +		num_vpp_pipes +
> +		ALIGN(size_vpxd_lb_se_left_ctrl_ar50lt(frame_width, frame_height), DMA_ALIGNMENT) *
> +		num_vpp_pipes +
> +		ALIGN(size_vp9d_lb_vsp_top(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_vpxd_lb_se_top_ctrl(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_vp9d_lb_pe_top_data(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_vp9d_lb_fe_top_data(frame_width, frame_height), DMA_ALIGNMENT);
> +}

[Severity: High]
Are we missing the DMA metadata, top control, and QP map allocations here?

The generic hfi_iris3_vp9d_lb_size() explicitly adds
size_vpxd_lb_fe_top_ctrl(), size_vpxd_lb_recon_dma_metadata_wr(), and
size_vp9d_qp(). Omitting these could result in an undersized buffer.

> +
>  static inline
>  u32 hfi_buffer_line_vp9d(u32 frame_width, u32 frame_height, u32 _yuv_bufcount_min, bool is_opb,
>  			 u32 num_vpp_pipes)
> @@ -507,6 +734,13 @@ u32 hfi_buffer_line_vp9d(u32 frame_width, u32 frame_height, u32 _yuv_bufcount_mi
>  	return _lb_size + vpss_lb_size + 4096;
>  }
>  
> +static inline
> +u32 hfi_buffer_line_vp9d_ar50lt(u32 frame_width, u32 frame_height, u32 _yuv_bufcount_min,
> +				bool is_opb, u32 num_vpp_pipes)
> +{
> +	return hfi_ar50lt_vp9d_lb_size(frame_width, frame_height, num_vpp_pipes);
> +}

[Severity: High]
Does this function also need to check is_opb and add the VPSS line buffer
size, similarly to hfi_buffer_line_vp9d()?

[ ... ]

> @@ -529,6 +763,25 @@ static u32 hfi_buffer_line_h264d(u32 frame_width, u32 frame_height,
>  	return ALIGN((size + vpss_lb_size), DMA_ALIGNMENT);
>  }
>  
> +static u32 hfi_buffer_line_h264d_ar50lt(u32 frame_width, u32 frame_height,
> +					bool is_opb, u32 num_vpp_pipes)
> +{
> +	u32 size;
> +
> +	size = ALIGN(size_h264d_lb_fe_top_data_ar50lt(frame_width), DMA_ALIGNMENT) +
> +		ALIGN(size_h264d_lb_fe_top_ctrl_ar50lt(frame_width), DMA_ALIGNMENT) +
> +		ALIGN(size_h264d_lb_fe_left_ctrl(frame_height), DMA_ALIGNMENT) * num_vpp_pipes +
> +		ALIGN(size_h264d_lb_se_top_ctrl_ar50lt(frame_width), DMA_ALIGNMENT) +
> +		ALIGN(size_h264d_lb_se_left_ctrl_ar50lt(frame_height), DMA_ALIGNMENT) *
> +		num_vpp_pipes +
> +		ALIGN(size_h264d_lb_pe_top_data_ar50lt(frame_width), DMA_ALIGNMENT) +
> +		ALIGN(size_h264d_lb_vsp_top(frame_width), DMA_ALIGNMENT) +
> +		ALIGN(size_h264d_lb_recon_dma_metadata_wr(frame_height), DMA_ALIGNMENT) * 2 +
> +		ALIGN(size_h264d_qp(frame_width, frame_height), DMA_ALIGNMENT);
> +
> +	return ALIGN(size, DMA_ALIGNMENT);
> +}

[Severity: High]
As with the HEVC and VP9 functions above, the is_opb argument is entirely
ignored. Could this lead to out-of-bounds DMA writes when split mode is
enabled?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-iris-ar50lt-v5-0-583b42770b6a@oss.qualcomm.com?part=13

^ permalink raw reply

* Re: [PATCH v2 2/6] iommu/arm-smmu: Add interconnect bandwidth voting support
From: Dmitry Baryshkov @ 2026-06-16  0:22 UTC (permalink / raw)
  To: Bibek Kumar Patro
  Cc: Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
	linux-arm-kernel, iommu, devicetree, linux-kernel, linux-arm-msm
In-Reply-To: <8518a085-b8b7-4ee6-b08c-8dde3971a6f1@oss.qualcomm.com>

On Mon, Jun 15, 2026 at 06:55:45PM +0530, Bibek Kumar Patro wrote:
> 
> 
> On 6/8/2026 7:25 PM, Dmitry Baryshkov wrote:
> > On Tue, May 26, 2026 at 08:12:03PM +0530, Bibek Kumar Patro wrote:
> > > On some SoCs the SMMU registers require an active interconnect
> > > bandwidth vote to be accessible. While other clients typically
> > > satisfy this requirement implicitly, certain corner cases (e.g.
> > > during sleep/wakeup transitions) can leave the SMMU without a
> > > vote, causing intermittent register access failures.
> > > 
> > > Add support for an optional interconnect path to the arm-smmu
> > > driver and vote for bandwidth while the SMMU is active. The path
> > > is acquired from DT if present and ignored otherwise.
> > > 
> > > The bandwidth vote is enabled before accessing SMMU registers
> > > during probe and runtime resume, and released during runtime
> > > suspend and on error paths.
> > > 
> > > Generally, from an architectural perspective, GEM_NOC and DDR are
> > > expected to have an active vote whenever the adreno_smmu block is
> > > powered on. In most common use cases, this requirement is implicitly
> > > satisfied because other GPU-related clients (for example, the GMU
> > > device) already hold a GEM_NOC vote when adreno_smmu is enabled.
> > > 
> > > However, there are certain corner cases, such as during sleep/wakeup
> > > transitions, where the GEM_NOC vote can be removed before adreno_smmu
> > > is powered down. If adreno_smmu is then accessed while the interconnect
> > > vote is missing, it can lead to the observed failures. Because of the
> > > precise ordering involved, this scenario is difficult to reproduce
> > > consistently.
> > > (also GDSC is involved in adreno usecases can have an independent vote)
> > > 
> > > Signed-off-by: Bibek Kumar Patro <bibek.patro@oss.qualcomm.com>
> > > ---
> > >   drivers/iommu/arm/arm-smmu/arm-smmu.c | 57 +++++++++++++++++++++++++++++++++--
> > >   drivers/iommu/arm/arm-smmu/arm-smmu.h |  2 ++
> > >   2 files changed, 57 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > index 0bd21d206eb3e75c3b9fb1364cdc92e82c5aa499..07c7e44ec6a5bd1488f00f87d859a20495e46601 100644
> > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > @@ -53,6 +53,11 @@
> > >   #define MSI_IOVA_BASE			0x8000000
> > >   #define MSI_IOVA_LENGTH			0x100000
> > > +/* Interconnect bandwidth vote values for the SMMU register access path */
> > > +#define ARM_SMMU_ICC_AVG_BW		0
> > > +#define ARM_SMMU_ICC_PEAK_BW_HIGH	1000
> > 
> > totally random numbers, which might be different for non-Qualcomm platform.
> > 
> > > +#define ARM_SMMU_ICC_PEAK_BW_LOW	0
> > > +
> > >   static int force_stage;
> > >   module_param(force_stage, int, S_IRUGO);
> > >   MODULE_PARM_DESC(force_stage,
> > > @@ -86,6 +91,36 @@ static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
> > >   	}
> > >   }
> > > +static int arm_smmu_icc_get(struct arm_smmu_device *smmu)
> > > +{
> > > +	smmu->icc_path = devm_of_icc_get(smmu->dev, NULL);
> > 
> > Is there always only one bus / path in question?
> > 
> 
> <Apologies, missed to respond to this query>
> Yes for TCU, it needs to only have a vote on GEM_NOC interconnect
> while accessing the DDR in downstream path (client->TCU->DDR), which we are
> addressing here.
> Hence it's only one icc path in question here.

Again, you are describing Qualcomm platform, while the code part is
generic.


-- 
With best wishes
Dmitry

^ permalink raw reply

* Re: [PATCH 1/2] dt-bindings: phy: qcom,usb-hs-phy: add qcom,vendor-init-seq
From: Dmitry Baryshkov @ 2026-06-16  0:24 UTC (permalink / raw)
  To: github.com
  Cc: Konrad Dybcio, linux-phy, devicetree, linux-arm-msm, Vinod Koul,
	Neil Armstrong, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Bjorn Andersson
In-Reply-To: <0298e09a5b4497910c226269daa2586b@herrie.org>

On Mon, Jun 15, 2026 at 02:17:08PM +0200, me@herrie.org wrote:
> On 2026-06-12 02:25, Dmitry Baryshkov wrote:
> > On Thu, Jun 11, 2026 at 12:39:45PM +0200, Konrad Dybcio wrote:
> > > On 6/4/26 1:02 AM, Dmitry Baryshkov wrote:
> > > > On Wed, Jun 03, 2026 at 06:09:18PM +0200, me@herrie.org wrote:
> > > >> On 2026-06-03 15:57, Dmitry Baryshkov wrote:
> > > >>> On Wed, Jun 03, 2026 at 07:48:08AM +0200, Herman van Hazendonk wrote:
> > > >>>> Add an optional "qcom,vendor-init-seq" property carrying raw ULPI
> > > >>>> (address, value) pairs that are written after PHY reset.
> > > >>>>
> > > >>>> Unlike the existing "qcom,init-seq" property, the address field is
> > > >>>> NOT offset by ULPI_EXT_VENDOR_SPECIFIC, so the new property can
> > > >>>> reach the standard ULPI vendor register range (0x30-0x3f). MSM8x60-
> > > >>>> class hardware needs this range to programme pre-emphasis, HS driver
> > > >>>> slope and CDR auto-reset bits the legacy msm_otg driver used to set
> > > >>>> via platform data.
> > > >>>
> > > >>> Are those register writes specific to the device or to the whole
> > > >>> platform? In the latter case please extend the driver to write them.
> > > >>
> > > >> Looking at every MSM8x60 reference kernel I could find (Qualcomm's own
> > > >> msm8x60 board, HP TouchPad / APQ8060, and some HTC/Saumsung MSM8660
> > > >> devices), the writes split into two groups:
> > > >>
> > > >> Platform-level (same across all MSM8x60 hardware):
> > > >>  - reg 0x36 bits 1+2: CDR auto-reset disabled, SE1 gating disabled
> > > >>  - reg 0x32 bits [5:4]: pre-emphasis at 20%
> > > >>
> > > >> Board-specific:
> > > >>  - reg 0x32 bits [3:0]: HS driver slope — HP TouchPad uses 5, HTC
> > > >>    devices use 1. This clearly depends on board layout (trace length,
> > > >>    connector loading, etc.).
> > > >>
> > > >> So the platform-level writes should move unconditionally into the driver
> > > >> behind a match-data flag for the MSM8x60-class compatible, and only the
> > > >> HS driver slope value belongs in DT.
> > > >
> > > > Looks like it. Please hardcode the value for your platform in the driver
> > > > (with the comment), meanwhile we can try looking up the actual values.
> > > 
> > > Do we have the values for a MTP/QRD (or whatever they used to be
> > > called
> > > back then..), like we would usually put in there?
> > 
> > As far as I can understand msm-3.0 and msm-3.4 most of the boards were
> > writing 0 here (although it might have been unexpected). None of the
> > board files set the hsdrvslope value (which means 0).
> > 
> > Please correct me if I'm wrong. I see that for tenderloin kernels change
> > that to 0x5, but I can't find a sensible commit message.
> > 
> > I could not find the documentation for vendor ULPI registers for those
> > chips, so I don't think we can identify, how to make sense of those
> > values. In such a case and having different board-specific values, we
> > don't have a better option than having a qcom,hsdrvslope (or similarly
> > named) property in DT.
> Dmitry/Konrad,
> 
> The bit-level meaning *is* documented byte-for-byte across
> every MSM8x60 downstream tree I could find: Code Aurora is the
> canonical source:
> 
>     arch/arm/mach-msm/include/mach/msm_hsusb_hw.h
> 
>     #define ULPI_CONFIG_REG3        0x32
>     #define ULPI_DIGOUT_CTRL        0x36          /* on MSM7x30 / MSM8x60 */
>     #define ULPI_CDR_AUTORESET      (1 << 1)
>     #define ULPI_SE1_GATE           (1 << 2)
>     #define ULPI_PRE_EMPHASIS_MASK  (3 << 4)
>     #define ULPI_HSDRVSLOPE_MASK    (0x0F)
> 
>   reg 0x32 [5:4]: pre-emphasis level (00 disabled, 11 = 20%, 10 = 10%)
>   reg 0x32 [3:0]: HS driver slope
>   reg 0x36 bit 1: CDR auto-reset enable
>   reg 0x36 bit 2: SE1 gating enable
> 
> The legacy `msm72k_otg.c` semantics for the 0x36 bits are inverted:
> setting the bit disables the function (see set_cdr_auto_reset() and
> set_se1_gating()).  Every MSM8x60 reference board sets both bits,
> i.e. CDR auto-reset disabled + SE1 gating disabled.
> 
> Surveying every MSM8x60-class board file I can reach: Qualcomm's own
> reference, Samsung, Sony, HTC, and HP: the platform-level values
> are unanimous:
> 
>   pre-emphasis     = 20%
>   CDR auto-reset   = disabled
>   SE1 gating       = disabled
> 
> |  Vendor / board                                 | hsdrvslope |
> |  -----------------------------------------------|------------|
> |  Qualcomm reference (SURF / FFA / Fluid /       |            |
> |    Dragon / Fusion -- board-msm8x60.c)          | unset (0)  |
> |  Samsung Galaxy S2 family (Q1 / Celox / Dali /  |            |
> |    generic 8x60 MTP)                            | unset (0)  |
> |  Sony MSM8660 (sony-kernel-msm8660)             | unset (0)  |
> |  HTC MSM8660 (shooter / holiday / pyramid /     |            |
> |    doubleshot / shooter_u / ruby)               | 1          |
> |  HP TouchPad (board-tenderloin.c)               | 5          |
> 
> So Qualcomm's own MTP, the closest thing to a reference platform we
> have, leaves hsdrvslope at the silicon default (0).  Two OEMs (HTC,
> HP) override it to non-zero values that match their board layout.
> Neither override has a commit message. HTC's value is buried in an
> init_seq array, HP's appears as `.hsdrvslope = 0x05` in
> board-tenderloin.c with no comment.  I've kept TouchPad's 5 because
> that's the value the device shipped with and our HS link is happy
> with it; changing it risks regressing eye margin on the silicon we
> still have to test against.
> 
> So the plan is exactly what you both already converged on:
> 
>   1. Drop qcom,vendor-init-seq from the binding (this patch goes away
>      entirely).
>   2. In the driver, hardcode the three platform-level writes behind
>      the qcom,usb-hs-phy-msm8660 compatible match-data, with a
>      comment citing msm_hsusb_hw.h as the source for the bit names.
>   3. Add a single qcom,hs-drv-slope property (u8, 0..15) for the
>      board-varying value.  TouchPad DT sets 5.  Absent ⇒ leave silicon
>      default in place, matching Qualcomm/Samsung/Sony reference
>      behaviour.
> 
> I'll send the new v in that shape: single dt-bindings patch
> (qcom,hs-drv-slope) plus the driver patch that hardcodes the platform
> writes and consumes the new property.

Sounds good. Please explain in the commit messages that you can't decode
how the values correspond to the actual slope values (I couldn't find
corresponding doc inside Qualcomm).

-- 
With best wishes
Dmitry

^ permalink raw reply

* Re: [PATCH v5 14/16] media: iris: add Gen2 firmware support on the Agatti platform
From: sashiko-bot @ 2026-06-16  0:26 UTC (permalink / raw)
  To: Dmitry Baryshkov; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260616-iris-ar50lt-v5-14-583b42770b6a@oss.qualcomm.com>

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incomplete dual firmware refactoring leads to build breakage and NULL pointer dereference during device probe.
--

commit 56abb89242d476eadcfa7c399ffc9ee1dfc1fca4
Author: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>

media: iris: add Gen2 firmware support on the Agatti platform

This patch extends the iris driver to support both HFI Gen1 and Gen2
firmwares for the Agatti platform.

> diff --git a/drivers/media/platform/qcom/iris/iris_platform_vpu_ar50lt.c b/drivers/media/platform/qcom/iris/iris_platform_vpu_ar50lt.c
> index 393256f39112b..d9de7dcb59e3a 100644
> --- a/drivers/media/platform/qcom/iris/iris_platform_vpu_ar50lt.c
> +++ b/drivers/media/platform/qcom/iris/iris_platform_vpu_ar50lt.c

[ ... ]

> @@ -78,7 +84,8 @@ static struct platform_inst_caps platform_inst_cap_ar50lt = {
>  };
>  
>  const struct iris_platform_data qcm2290_data = {
> -	.firmware_desc_gen1 = &iris_vpu_ar50lt_p1_gen1_s6_desc,
> +	.firmware_desc_gen1 = &iris_vpu_ar50lt_p1_gen1_desc,
> +	.firmware_desc_gen2 = &iris_vpu_ar50lt_p1_gen2_s6_desc,
>  	.vpu_ops = &iris_vpu_ar50lt_ops,

[Severity: High]
Does struct iris_platform_data actually contain the firmware_desc_gen2 field?

It looks like this might cause a build failure if struct iris_platform_data
hasn't been updated to include these specific gen1/gen2 fields.

Additionally, does this introduce a NULL pointer dereference during device
probe? If iris_probe() in drivers/media/platform/qcom/iris/iris_probe.c
still unconditionally reads the legacy firmware_desc field:

iris_probe() {
    ...
    core->iris_firmware_desc = core->iris_platform_data->firmware_desc;
    core->iris_firmware_data = core->iris_firmware_desc->firmware_data;
    ...
}

Won't this dereference a NULL pointer since the legacy firmware_desc is left
uninitialized for this platform? It seems a prerequisite patch refactoring
struct iris_platform_data and the probe function might be missing from this
series.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-iris-ar50lt-v5-0-583b42770b6a@oss.qualcomm.com?part=14

^ permalink raw reply

* Re: [PATCH 1/2] power: sequencing: pcie-m2: Add PCI ID 0x1103 for WCN6855 Bluetooth
From: Dmitry Baryshkov @ 2026-06-16  0:26 UTC (permalink / raw)
  To: Wei Deng
  Cc: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Manivannan Sadhasivam, Bartosz Golaszewski,
	linux-arm-msm, devicetree, linux-kernel, linux-pci, linux-pm,
	quic_chezhou, cheng.jiang, shuai.zhang, jinwang.li, xiuzhuo.shang,
	mengshi.wu
In-Reply-To: <20260608091702.3797437-2-wei.deng@oss.qualcomm.com>

On Mon, Jun 08, 2026 at 02:47:01PM +0530, Wei Deng wrote:
> WCN6855 is a Qualcomm Wi-Fi/BT combo chip that uses PCI device ID
> 0x1103. Add it to pwrseq_m2_pci_ids[] alongside the existing 0x1107
> (WCN7850) entry, so that the pwrseq-pcie-m2 driver creates a Bluetooth
> serdev device for WCN6855 cards inserted into PCIe M.2 Key E connectors.
> 
> Signed-off-by: Wei Deng <wei.deng@oss.qualcomm.com>
> ---
>  drivers/power/sequencing/pwrseq-pcie-m2.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
> index efeb25ba9c79..b3af14464314 100644
> --- a/drivers/power/sequencing/pwrseq-pcie-m2.c
> +++ b/drivers/power/sequencing/pwrseq-pcie-m2.c
> @@ -188,6 +188,8 @@ static int pwrseq_pcie_m2_match(struct pwrseq_device *pwrseq,
>  static const struct pci_device_id pwrseq_m2_pci_ids[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_QCOM, 0x1107),
>  	  .driver_data = (kernel_ulong_t)"qcom,wcn7850-bt" },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_QCOM, 0x1103),
> +	  .driver_data = (kernel_ulong_t)"qcom,wcn6855-bt" },
>  	{ } /* Sentinel */

Please keep the list sorted. I saw that Bartosz has applied the patch.
Would you please send a followup?

>  };
>  
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

^ permalink raw reply

* Re: [PATCH 1/4] dt-bindings: iio: adc: add ti,ads122c14
From: Kurt Borja @ 2026-06-16  0:26 UTC (permalink / raw)
  To: David Lechner (TI), Jonathan Cameron, Nuno Sá,
	Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley
  Cc: Kurt Borja, Nguyen Minh Tien, linux-iio, devicetree, linux-kernel
In-Reply-To: <20260615-iio-adc-ti-ads122c14-v1-1-e6bdadf7cb2b@baylibre.com>

On Mon Jun 15, 2026 at 4:59 PM -05, David Lechner (TI) wrote:
> Add new bindings for ti,ads122c14 and similar devices.
>
> This is an ADC that is primarily intended for use with temperature
> sensors. There are a few unusual properties because of this. In
> particular, the reference voltage source and current output requirements
> can be different for each measurement, so these are included in the
> channel bindings.
>
> The REFP/REFN reference voltage is usually just connected to a resistor
> that is being driven by the ADC's current outputs, so there is special
> property for this case rather than requiring a regulator to be defined
> to represent that.
>
> ti,vref-source is reused from ti,tlv320adcx140.yaml (otherwise might
> have preferred an enum of strings).
>
> Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>
> ---
>  .../devicetree/bindings/iio/adc/ti,ads112c14.yaml  | 224 +++++++++++++++++++++
>  MAINTAINERS                                        |   7 +
>  include/dt-bindings/iio/adc/ti,ads112c14.h         |  11 +
>  3 files changed, 242 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads112c14.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads112c14.yaml
> new file mode 100644
> index 000000000000..dc7f37cad772
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads112c14.yaml
> @@ -0,0 +1,224 @@

[...]

> +patternProperties:
> +  ^channel@[0-7]$:
> +    $ref: adc.yaml
> +
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        maximum: 16 # arbitrary limit, channel@ can be any combination of AIN0-AIN7
> +
> +      single-channel:
> +        maximum: 7
> +
> +      diff-channels:
> +        items:
> +          maximum: 7
> +
> +      bipolar:
> +        description:
> +          Set this flag if the differential input can be negative.
> +
> +      excitation-channels:

I noticed this doesn't have the "ti," prefix. Is your plan to add this
to adc.yaml?

Also, do you think excitation-pins might be a better name?

> +        description: AINx pins used as current output.
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        minItems: 1
> +        maxItems: 2
> +        items:
> +          maximum: 7
> +
> +      excitation-current-microamp:
> +        description: The current output of the excitation channels in microamps.
> +        minimum: 1
> +        maximum: 1000
> +
> +      current-chopping:

If you agree with the above comment, I think this too should be added.
In that case, can we call this something like
excitation-current-rotation for less ambiguity?

I say this because my device has an "IDAC rotation mode", but it also has
a "Chop Mode" which rotates analog inputs and averages consecutive
conversions.

> +        $ref: /schemas/types.yaml#/definitions/flag
> +        description:
> +          If provided, the two excitation channels are to be used with current
> +          chopping enabled.
> +
> +      ti,vref-source:
> +        description: |
> +          Indicates the source for the reference voltage for this channel.
> +          0 - Internal 2.5V reference
> +          1 - Internal 1.25V reference
> +          2 - External reference (REFP-REFN)
> +          3 - AVDD as reference
> +
> +          For convenience, macros for these values are available in
> +          dt-bindings/iio/adc/ti,ads112c14.h.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        maximum: 3
> +        default: 0
> +
> +    dependencies:
> +      excitation-channels: [ excitation-current-microamp ]
> +      excitation-current-microamp: [ excitation-channels ]
> +      current-chopping: [ excitation-channels ]
> +
> +    oneOf:
> +      - required: [ single-channel ]
> +      - required: [ diff-channels ]

[...]

-- 
Thanks,
 ~ Kurt

^ permalink raw reply

* Re: [PATCH v2] arm64: dts: qcom: lemans-evk: Describe the PCIe M.2 Key E connector
From: Dmitry Baryshkov @ 2026-06-16  0:28 UTC (permalink / raw)
  To: Wei Deng
  Cc: andersson, konradybcio, robh, krzk+dt, conor+dt, linux-arm-msm,
	devicetree, linux-kernel, manivannan.sadhasivam,
	bartosz.golaszewski, quic_chezhou, cheng.jiang, shuai.zhang,
	jinwang.li, xiuzhuo.shang, mengshi.wu
In-Reply-To: <20260615103228.3104083-1-wei.deng@oss.qualcomm.com>

On Mon, Jun 15, 2026 at 04:02:28PM +0530, Wei Deng wrote:
> The lemans EVK has the PCIe M.2 Mechanical Key E connector to connect
> wireless connectivity cards over PCIe and UART interfaces. Hence,
> describe the connector node and link it with the PCIe 0 Root Port and
> UART17 nodes through graph port/endpoint.
> 
> Also add 'compatible = "pciclass,0604"' to the pcieport0 node in
> lemans.dtsi to allow the PCI subsystem to associate the DT node with
> the PCI-to-PCI bridge device.
> 
> The M.2 Key E connector is powered by a 3.3V fixed regulator
> (vreg_wcn_3p3) which is sourced from the board's 12V DC input rail
> (vreg_dcin_12v). Both regulators are always-on and are required by the
> pcie-m2-e-connector binding.
> 
> Also add the serial1 = &uart17 alias, which is required for the
> Bluetooth serdev device to be enumerated on the UART17 interface.
> 
> Signed-off-by: Wei Deng <wei.deng@oss.qualcomm.com>
> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> Changes in v2:
> - Collected Reviewed-by tag and reorganized the patch

Please don't send the patches as replies to the previous iterations.
Each new revision should be in a separate thread.

> 
> Link: https://lore.kernel.org/linux-arm-msm/20260608091702.3797437-2-wei.deng@oss.qualcomm.com/ [v1]
> 
>  arch/arm64/boot/dts/qcom/lemans-evk.dts | 75 +++++++++++++++++++++++++
>  arch/arm64/boot/dts/qcom/lemans.dtsi    |  1 +
>  2 files changed, 76 insertions(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

^ permalink raw reply

* Re: [PATCH v5 13/16] media: iris: Introduce buffer size calculations for AR50LT
From: Dmitry Baryshkov @ 2026-06-16  0:30 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260616002148.EE7CB1F000E9@smtp.kernel.org>

On Tue, Jun 16, 2026 at 12:21:48AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] The AR50LT decoder line buffer size calculations accept an `is_opb` parameter but completely ignore it, failing to allocate memory for the VPSS line buffer when OPB (split mode) is enabled.
> - [High] The AR50LT BIN buffer size calculations for H.264 and HEVC ignore `num_vpp_pipes`, preventing proper buffer partitioning and alignment across hardware pipelines.
> - [High] The VP9 AR50LT line buffer calculation omits critical component sizes for DMA metadata and QP maps.
> - [High] Missing SEI user data allocation in the H.264 AR50LT persist buffer calculation.

Vikash, Visnu, could you pleae check whether buffer sizes for HFI Gen2
are calculated correctly or not?

-- 
With best wishes
Dmitry

^ permalink raw reply

* Re: [PATCH v5 14/16] media: iris: add Gen2 firmware support on the Agatti platform
From: Dmitry Baryshkov @ 2026-06-16  0:31 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260616002607.0BABA1F000E9@smtp.kernel.org>

On Tue, Jun 16, 2026 at 12:26:06AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Incomplete dual firmware refactoring leads to build breakage and NULL pointer dereference during device probe.
> --
> 
> commit 56abb89242d476eadcfa7c399ffc9ee1dfc1fca4
> Author: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
> 
> media: iris: add Gen2 firmware support on the Agatti platform
> 
> This patch extends the iris driver to support both HFI Gen1 and Gen2
> firmwares for the Agatti platform.
> 
> > diff --git a/drivers/media/platform/qcom/iris/iris_platform_vpu_ar50lt.c b/drivers/media/platform/qcom/iris/iris_platform_vpu_ar50lt.c
> > index 393256f39112b..d9de7dcb59e3a 100644
> > --- a/drivers/media/platform/qcom/iris/iris_platform_vpu_ar50lt.c
> > +++ b/drivers/media/platform/qcom/iris/iris_platform_vpu_ar50lt.c
> 
> [ ... ]
> 
> > @@ -78,7 +84,8 @@ static struct platform_inst_caps platform_inst_cap_ar50lt = {
> >  };
> >  
> >  const struct iris_platform_data qcm2290_data = {
> > -	.firmware_desc_gen1 = &iris_vpu_ar50lt_p1_gen1_s6_desc,
> > +	.firmware_desc_gen1 = &iris_vpu_ar50lt_p1_gen1_desc,
> > +	.firmware_desc_gen2 = &iris_vpu_ar50lt_p1_gen2_s6_desc,
> >  	.vpu_ops = &iris_vpu_ar50lt_ops,
> 
> [Severity: High]
> Does struct iris_platform_data actually contain the firmware_desc_gen2 field?
> 
> It looks like this might cause a build failure if struct iris_platform_data
> hasn't been updated to include these specific gen1/gen2 fields.

Huh? It's listed as a dependency patch.

> 
> Additionally, does this introduce a NULL pointer dereference during device
> probe? If iris_probe() in drivers/media/platform/qcom/iris/iris_probe.c
> still unconditionally reads the legacy firmware_desc field:
> 
> iris_probe() {
>     ...
>     core->iris_firmware_desc = core->iris_platform_data->firmware_desc;
>     core->iris_firmware_data = core->iris_firmware_desc->firmware_data;
>     ...
> }
> 
> Won't this dereference a NULL pointer since the legacy firmware_desc is left
> uninitialized for this platform? It seems a prerequisite patch refactoring
> struct iris_platform_data and the probe function might be missing from this
> series.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260616-iris-ar50lt-v5-0-583b42770b6a@oss.qualcomm.com?part=14

-- 
With best wishes
Dmitry

^ permalink raw reply

* Re: [PATCH v5 02/16] media: iris: Filter UBWC raw formats based on hardware capabilities
From: Dmitry Baryshkov @ 2026-06-16  0:32 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260616001733.5FC251F000E9@smtp.kernel.org>

On Tue, Jun 16, 2026 at 12:17:32AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The `iris_vdec_formats_noubwc_cap` array contains a zero-initialized gap at index 1, which prematurely terminates V4L2 format enumeration and improperly validates 0 as a valid pixel format.

I will check later and respond or fix the issues.

> --
> 
> commit 1ff6b6e464175bc1ccf6878e0aa89ffec126608b
> Author: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
> 

-- 
With best wishes
Dmitry

^ permalink raw reply

* Re: [PATCH net-next 1/8] dt-bindings: net: realtek,rtl9301-mdio: Add RTL83xx series
From: Jakub Kicinski @ 2026-06-16  0:37 UTC (permalink / raw)
  To: markus.stockhausen
  Cc: Jakub Kicinski, andrew, hkallweit1, linux, davem, edumazet,
	pabeni, netdev, chris.packham, daniel, robh, krzk+dt, conor+dt,
	devicetree
In-Reply-To: <20260613112946.1071411-2-markus.stockhausen@gmx.de>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: dt-bindings: net: realtek,rtl9301-mdio: Add RTL83xx series

This commit extends the realtek,rtl9301-mdio binding to cover the
lower-end Realtek Otto switch families: Maple (RTL8380/8381/8382) and
Cypress (RTL8391/8392/8393). New compatible strings are added for these
SoCs alongside the existing RTL9300/RTL9311 entries.

> diff --git a/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml b/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
> index 271e05bae9c5..de33364b67ef 100644
> --- a/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
> +++ b/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
> @@ -12,6 +12,16 @@ maintainers:
>  properties:
>    compatible:
>      oneOf:
> +      - items:
> +          - enum:
> +              - realtek,rtl8381-mdio
> +              - realtek,rtl8382-mdio
> +          - const: realtek,rtl8380-mdio
> +      - items:
> +          - enum:
> +              - realtek,rtl8392-mdio
> +              - realtek,rtl8393-mdio
> +          - const: realtek,rtl8391-mdio

[Low]
With this change the binding now describes three distinct SoC families
(RTL83xx Maple, RTL839x Cypress, and the existing RTL93xx), but the
file still carries:

  title: Realtek RTL9300 MDIO Controller

Should the title (and the top-level description, if applicable) be
updated to reflect the broader scope now that RTL8380/8381/8382 and
RTL8391/8392/8393 are covered by the same schema?

> @@ -24,6 +34,8 @@ properties:
>                - realtek,rtl9313-mdio
>            - const: realtek,rtl9311-mdio
>        - enum:
> +          - realtek,rtl8380-mdio
> +          - realtek,rtl8391-mdio
>            - realtek,rtl9301-mdio
>            - realtek,rtl9311-mdio
>
-- 
pw-bot: cr

^ permalink raw reply

* Re: [PATCH net-next 2/8] net: mdio: realtek-rtl9300: Add polling documentation
From: Jakub Kicinski @ 2026-06-16  0:37 UTC (permalink / raw)
  To: markus.stockhausen
  Cc: Jakub Kicinski, andrew, hkallweit1, linux, davem, edumazet,
	pabeni, netdev, chris.packham, daniel, robh, krzk+dt, conor+dt,
	devicetree
In-Reply-To: <20260613112946.1071411-3-markus.stockhausen@gmx.de>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mdio: realtek-rtl9300: Add polling documentation

This commit adds a comment block to the rtl9300 MDIO driver describing
how the Realtek Otto switches' hardware polling unit interacts with the
MDIO bus and what this implies for kernel-initiated PHY accesses.

> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> index 892ed3780a65..da2864c94d2c 100644
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
> @@ -35,6 +35,72 @@

[ ... ]

> + *	phy_read(phy, 5);		phy_read(phy, 14);
> + *	phy_read(phy, 6);
> + *
> + * The c22 over c45 register 13/14 sequences read MDIO_AN_EEE_ADV and MDIO_AN_EEE_LPABLE. As soon
> + * as one PHY status is read, the polling engine goes over to the next PHY. Basically the bus is
> + * always busy and the MAC status is updated in realtime.

[ ... ]

> + * - A c45 over c22 kernel access sequence is most likely to fail because chances are high that
> + *   the polling engine overwrites registers 13/14 in between.

[Low]
The two paragraphs above describe the same mechanism — using Clause 22
transactions on registers 13 (MII_MMD_CTRL) and 14 (MII_MMD_DATA) to
indirectly access Clause 45 MMD registers — but use opposite wordings:
"c22 over c45" in the first paragraph and "c45 over c22" in the second.

The example writes shown earlier in the same comment (phy_write(phy, 13,
0x7); phy_write(phy, 14, 0x3c); ...) are the standard C22 indirect access
to C45 MMD registers, conventionally referred to in the kernel as
"C45 over C22".

Since the stated goal of this comment is to simplify future development
and review, would it make sense to use the same phrasing in both places
(for instance "C45 over C22" in both)?

^ permalink raw reply

* Re: [PATCH net-next 5/8] net: mdio: realtek-rtl9300: Add c45 over c22 mitigation
From: Jakub Kicinski @ 2026-06-16  0:37 UTC (permalink / raw)
  To: markus.stockhausen
  Cc: Jakub Kicinski, andrew, hkallweit1, linux, davem, edumazet,
	pabeni, netdev, chris.packham, daniel, robh, krzk+dt, conor+dt,
	devicetree
In-Reply-To: <20260613112946.1071411-6-markus.stockhausen@gmx.de>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mdio: realtek-rtl9300: Add c45 over c22 mitigation

This patch adds a per-port state machine that disables hardware MAC polling
around a register 13/14/13/14 c45-over-c22 MMD access sequence so the
sequence cannot be interrupted by a parallel polling read.  Polling is
disabled when the first MII_MMD_CTRL access is seen and re-enabled when
the four-step sequence completes or unexpected input is detected.

A few questions on the new state-machine corner cases below.

> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> index a7fd075947b6..e206ee3e2b1c 100644
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c

[ ... ]

> @@ -254,6 +257,43 @@ static int otto_emdio_set_port_polling(struct otto_emdio_priv *priv, int port, b
>  				  BIT(port % 32), active);
>  }
>  
> +static int otto_emdio_mmd_prefix(struct otto_emdio_priv *priv, int port, int regnum)
> +{
> +	u8 newstate, *state = &priv->mmd_state[port];
> +	int expected, ret = 0;
> +
> +	if (!priv->init_done)
> +		return 0;
> +	/*
> +	 * Disabled polling might produce link flapping and false notification interrupts on the
> +	 * MAC layer. In this case disable c45 over c22 MMD access because chances are high that
> +	 * the register 13/14/13/14 sequence is intercepted by a parallel hardware access. As
> +	 * a workaround the PHY must provide its own mmd read/write() callbacks and redirect to
> +	 * normal c22 registers. See rtlgen_read_mmd().
> +	 */
> +	if (priv->info->link_flap)
> +		return (regnum == MII_MMD_DATA || regnum == MII_MMD_CTRL) ? -EIO : 0;
> +
> +	expected = (*state & 1) ? MII_MMD_DATA : MII_MMD_CTRL;
> +	newstate = regnum == expected ? *state + 1 : 0;
> +
> +	if (newstate == 1 || newstate < *state)
> +		ret = otto_emdio_set_port_polling(priv, port, !newstate);
> +	*state = newstate;
> +
> +	return ret;
> +}
> +
> +static int otto_emdio_mmd_postfix(struct otto_emdio_priv *priv, int port, int regnum)
> +{
> +	if (priv->mmd_state[port] != 4)
> +		return 0;
> +
> +	priv->mmd_state[port] = 0;
> +
> +	return otto_emdio_set_port_polling(priv, port, true);
> +}

[High]
Can otto_emdio_mmd_prefix() and otto_emdio_mmd_postfix() get out of sync
on a transient MDIO failure?

The prefix transitions state 0->1 on the first MII_MMD_CTRL access and
calls otto_emdio_set_port_polling(port, false).  In the c22 read/write
helpers below, if priv->info->read_c22() / write_c22() then returns an
error (otto_emdio_run_cmd() can return -ETIMEDOUT from
regmap_read_poll_timeout(), -ENXIO via cmd_fail, or any regmap_write()
error), the function returns immediately and never calls
otto_emdio_mmd_postfix().

Polling is left disabled, and *state stays non-zero.

On subsequent retries with regnums that still match the parity-based
expected value (the natural retry shape for an EEE 13/14/13/14
sequence), mmd_prefix keeps doing *state + 1 unbounded (state walks
through 5, 6, 7, ...).  mmd_postfix only re-enables polling when
mmd_state == 4 exactly:

	if (priv->mmd_state[port] != 4)
		return 0;

so once the counter overshoots 4 it stays disabled until either an
unrelated regnum forces newstate < *state in mmd_prefix, or the u8
wraps at 255->0.

The commit message says polling is re-enabled "as soon as the sequence
is finished or any other unexpected input is detected", but a transient
MDIO failure mid-sequence is neither.

Should the error paths in otto_emdio_read_c22() / otto_emdio_write_c22()
reset mmd_state and re-enable polling on a failed transfer?

> @@ -463,7 +503,15 @@ static int otto_emdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
>  		if (regnum == 31)
>  			return priv->page[port];
>  
> +		ret = otto_emdio_mmd_prefix(priv, port, regnum);
> +		if (ret)
> +			return ret;
> +
>  		ret = priv->info->read_c22(bus, port, regnum, &value);
> +		if (ret)
> +			return ret;
> +
> +		ret = otto_emdio_mmd_postfix(priv, port, regnum);
>  	}
>  
>  	return ret ? ret : value;

[Medium]
The regnum == 31 page-select path returns from inside the scoped_guard
before reaching mmd_prefix() / mmd_postfix():

		if (regnum == 31)
			return priv->page[port];

If a page-register access occurs while mmd_state is mid-sequence (1, 2,
or 3), the counter is not reset and the polling-disable bit is not
restored, even though a page write between MMD steps is exactly the
"unexpected input" the commit message describes.

Recovery only happens on the next non-13/14 access that falls through
mmd_prefix and triggers the newstate < *state branch.

Should the regnum == 31 path also reconcile mmd_state and re-enable
polling when a sequence is in progress?  The same question applies in
otto_emdio_write_c22().

[Low]
A separate observation about the read return path:

>  	return ret ? ret : value;

After a successful priv->info->read_c22() populates value, ret is
overwritten by mmd_postfix()'s return.  If
otto_emdio_set_port_polling(priv, port, true) inside mmd_postfix()
fails, the function returns that error even though the data transfer
succeeded, so the PHY layer discards correctly read data.  For
read-clear / latched status registers the hardware-side bits have
already been consumed.

Would it be preferable to treat a postfix failure as a polling-
bookkeeping issue (log and proceed) and still return the read value?

> @@ -487,7 +535,15 @@ static int otto_emdio_write_c22(struct mii_bus *bus, int phy_id, int regnum, u16
>  			return 0;
>  		}
>  
> -		return priv->info->write_c22(bus, port, regnum, value);
> +		ret = otto_emdio_mmd_prefix(priv, port, regnum);
> +		if (ret)
> +			return ret;
> +
> +		ret = priv->info->write_c22(bus, port, regnum, value);
> +		if (ret)
> +			return ret;
> +
> +		return otto_emdio_mmd_postfix(priv, port, regnum);
>  	}
>  }

The same shape exists for the write path: a postfix failure here turns
a successful write into a reported error and may cause an upper-layer
retry of an already-applied write.

^ permalink raw reply

* Re: [PATCH net-next 8/8] net: mdio: realtek-rtl9300: Add support for RTL839x
From: Jakub Kicinski @ 2026-06-16  0:37 UTC (permalink / raw)
  To: markus.stockhausen
  Cc: Jakub Kicinski, andrew, hkallweit1, linux, davem, edumazet,
	pabeni, netdev, chris.packham, daniel, robh, krzk+dt, conor+dt,
	devicetree
In-Reply-To: <20260613112946.1071411-9-markus.stockhausen@gmx.de>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: mdio: realtek-rtl9300: Add support for RTL839x

This patch adds RTL839x (cypress) support to the realtek-rtl9300 MDIO
driver, reusing the multi-device infrastructure that was prepared for
this purpose. It introduces a new RTL8390 register set, four c22/c45
read/write helpers, and a new otto_emdio_8390_info compatible table
entry.

> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> index d9ff0b0aecbb..2ab1aeb85eed 100644
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
> @@ -139,6 +139,29 @@
>  #define RTL8380_SMI_POLL_CTRL			0xa17c
>  #define RTL8380_SMI_PORT0_5_ADDR_CTRL		0xa1c8
>  
> +#define RTL8390_NUM_BUSES			2
> +#define RTL8390_NUM_PAGES			8192
> +#define RTL8390_NUM_PORTS			52
> +#define RTL8390_BCAST_PHYID_CTRL		0x03ec
> +#define RTL8390_PHYREG_ACCESS_CTRL		0x03dc
> +#define   RTL8390_PHY_CTRL_REG_ADDR		GENMASK(9, 5)
> +#define   RTL8390_PHY_CTRL_PARK_PAGE		GENMASK(27, 23)

[Low]
Is RTL8390_PHY_CTRL_PARK_PAGE used anywhere?  It does not appear to
be referenced in the rest of the patch, while the analogous masks for
the 8380 and 9300 variants are populated with 0x1f in their c22 helpers.

For example, otto_emdio_9300_read_c22() does:

	.c22_data	= FIELD_PREP(RTL9300_PHY_CTRL_REG_ADDR, regnum) |
			  FIELD_PREP(RTL9300_PHY_CTRL_PARK_PAGE, 0x1f) |
			  FIELD_PREP(RTL9300_PHY_CTRL_MAIN_PAGE, priv->page[port]),

while otto_emdio_8390_read_c22() below only sets REG_ADDR and MAIN_PAGE,
leaving bits 27..23 of c22_data at 0:

> +#define   RTL8390_PHY_CTRL_MAIN_PAGE		GENMASK(22, 10)
> +#define   RTL8390_PHY_CTRL_FAIL			BIT(1)
> +#define   RTL8390_PHY_CTRL_WRITE		BIT(3)
> +#define   RTL8390_PHY_CTRL_READ			0
> +#define   RTL8390_PHY_CTRL_TYPE_C45		BIT(2)
> +#define   RTL8390_PHY_CTRL_TYPE_C22		0

[ ... ]

> @@ -457,6 +480,62 @@ static int otto_emdio_8380_write_c45(struct mii_bus *bus, int port,
>  	return otto_emdio_write_cmd(bus, RTL8380_PHY_CTRL_TYPE_C45, &cmd_data);
>  }
>  
> +static int otto_emdio_8390_read_c22(struct mii_bus *bus, int port, int regnum, u32 *value)
> +{
> +	struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
> +	struct otto_emdio_cmd_regs cmd_data = {
> +		.c22_data	= FIELD_PREP(RTL8390_PHY_CTRL_REG_ADDR, regnum) |
> +				  FIELD_PREP(RTL8390_PHY_CTRL_MAIN_PAGE, priv->page[port]),
> +		.ext_page	= FIELD_PREP(RTL8390_PHY_CTRL_EXT_PAGE, 0x1ff),
> +		.io_data	= FIELD_PREP(RTL8390_PHY_CTRL_INDATA, port),
> +	};
> +
> +	return otto_emdio_read_cmd(bus, RTL8390_PHY_CTRL_TYPE_C22, &cmd_data,
> +				   RTL8390_PHY_CTRL_DATA, value);
> +}
> +
> +static int otto_emdio_8390_write_c22(struct mii_bus *bus, int port, int regnum, u16 value)
> +{
> +	struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
> +	struct otto_emdio_cmd_regs cmd_data = {
> +		.c22_data	= FIELD_PREP(RTL8390_PHY_CTRL_REG_ADDR, regnum) |
> +				  FIELD_PREP(RTL8390_PHY_CTRL_MAIN_PAGE, priv->page[port]),

Should these helpers also set RTL8390_PHY_CTRL_PARK_PAGE to 0x1f, matching
the 8380/9300 convention, or is the macro itself intended to be removed
because the 8390 hardware does not need a parking page?

> +		.ext_page	= FIELD_PREP(RTL8390_PHY_CTRL_EXT_PAGE, 0x1ff),
> +		.io_data	= FIELD_PREP(RTL8390_PHY_CTRL_INDATA, value),
> +		.port_mask_high	= (u32)(BIT_ULL(port) >> 32),
> +		.port_mask_low	= (u32)(BIT_ULL(port)),
> +	};
> +
> +	return otto_emdio_write_cmd(bus, RTL8390_PHY_CTRL_TYPE_C22, &cmd_data);
> +}

[ ... ]

^ permalink raw reply

* Re: [PATCH v5 12/16] media: iris: implement support for the Agatti platform
From: sashiko-bot @ 2026-06-16  0:40 UTC (permalink / raw)
  To: Dmitry Baryshkov; +Cc: conor+dt, devicetree, robh
In-Reply-To: <20260616-iris-ar50lt-v5-12-583b42770b6a@oss.qualcomm.com>

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Logic Error: Invalid hardware frequency calculation due to missing `mb_cycles_vsp` parameter
- [High] Hardware Configuration: The watchdog interrupt is defined but never unmasked, breaking watchdog recovery
- [Low] Build Failure: Initialization of non-existent struct members in qcm2290_data
--

commit eabd242734902d122adf5442b6b1946c0a61380b
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

media: iris: implement support for the Agatti platform
    
Port support for the AR50Lt video codec core (present for example on the
Agatti platform) to the Iris driver. Unlike more recent cores this
generation doesn't have the PIPE property (as it always has only one
pipe). Also, unlike newer platforms, buffer sizes are requested from the
firmware instead of being calculated by the driver.

> diff --git a/drivers/media/platform/qcom/iris/iris_platform_vpu_ar50lt.c b/drivers/media/platform/qcom/iris/iris_platform_vpu_ar50lt.c
> new file mode 100644
> index 0000000000000..393256f39112b
> --- /dev/null
> +++ b/drivers/media/platform/qcom/iris/iris_platform_vpu_ar50lt.c

[ ... ]

> +static struct platform_inst_caps platform_inst_cap_ar50lt = {
> +	.min_frame_width = 128,
> +	.max_frame_width = 1920,
> +	.min_frame_height = 128,
> +	.max_frame_height = 1920,
> +	.max_mbpf = (1920 * 1088) / 256,
> +	.mb_cycles_vpp = 440,
> +	.mb_cycles_fw = 733003,
> +	.mb_cycles_fw_vpp = 225975,

[Severity: High]
Does this code need to initialize .mb_cycles_vsp?

It looks like .mb_cycles_vsp is omitted here, so it will default to 0. If
the .calc_freq callback in iris_vpu_ar50lt_ops uses
iris_vpu2_calculate_frequency(), that function calculates vsp_freq by
multiplying the macroblocks-per-second by caps->mb_cycles_vsp. Because
mb_cycles_vsp is 0, the resulting hardware clock frequency might be severely
undercalculated, potentially causing performance degradation or hardware
timeouts.

> +	.max_frame_rate = 120,
> +	.max_operating_rate = 120,
> +};
> +
> +const struct iris_platform_data qcm2290_data = {
> +	.firmware_desc_gen1 = &iris_vpu_ar50lt_p1_gen1_s6_desc,

[Severity: Low]
Does struct iris_platform_data have a member named firmware_desc_gen1?

It appears that struct iris_platform_data (defined in
iris_platform_common.h) only contains a generic firmware_desc member. Using
.firmware_desc_gen1 here might result in a compilation error.

> +	.vpu_ops = &iris_vpu_ar50lt_ops,
> +	.icc_tbl = iris_icc_info_ar50lt,
> +	.icc_tbl_size = ARRAY_SIZE(iris_icc_info_ar50lt),

[ ... ]

> +	.tz_cp_config_data_size = ARRAY_SIZE(tz_cp_config_ar50lt),
> +	.num_vpp_pipe = 1,
> +	.no_rpmh = true,
> +	.wd_intr_mask = WRAPPER_INTR_STATUS_A2HWD_BMSK,

[Severity: High]
Will the watchdog interrupt actually be unmasked by the hardware?

This defines the platform's watchdog status bit mask as
WRAPPER_INTR_STATUS_A2HWD_BMSK (0x10). However, if
iris_vpu_ar50lt_interrupt_init() in iris_vpu_ar50lt.c overwrites
WRAPPER_INTR_MASK with a hardcoded mask that omits the watchdog bit, the
watchdog interrupt will not be unmasked, preventing the driver from
detecting and recovering from VPU firmware hangs.

> +	.icc_ib_multiplier = 2,
> +	.max_session_count = 8,
> +	.max_core_mbpf = ((1920 * 1088) / 256) * 4,
> +	/* Concurrency: 1080p@30 decode + 1080p@30 encode */
> +	/* Concurrency: 3 * 1080p@30 decode */
> +	.max_core_mbps = (((1920 * 1088) / 256) * 90),
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-iris-ar50lt-v5-0-583b42770b6a@oss.qualcomm.com?part=12

^ permalink raw reply

* Re: [PATCH V1 0/2] arm64: dts: qcom: Shikra SD Card support
From: Dmitry Baryshkov @ 2026-06-16  0:54 UTC (permalink / raw)
  To: Monish Chunara
  Cc: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Vinod Koul, Neil Armstrong, Wesley Cheng,
	Ulf Hansson, Kernel Team, linux-arm-msm, devicetree, linux-kernel,
	linux-phy, linux-mmc, Nitin Rawat, Pradeep Pragallapati,
	Komal Bajaj, Konrad Dybcio
In-Reply-To: <20260604122045.494712-1-monish.chunara@oss.qualcomm.com>

On Thu, Jun 04, 2026 at 05:50:43PM +0530, Monish Chunara wrote:
> This series adds SD card support for the Shikra platform.
> 
> The first patch adds the SDHC2 controller node and the necessary pinctrl
> configurations to the base Shikra SoC dtsi. The second patch enables 
> this support on the Shikra EVK (CQS, CQM, and IQS variants) by defining
> the regulator supplies and the card detection GPIO.
> 
> Testing:
> - Validated on Shikra EVK variants.
> 
> This series depends on:
> - https://lore.kernel.org/all/20260527-shikra-dt-v4-0-b5ca1fa0b392@oss.qualcomm.com/
> - https://lore.kernel.org/all/20260521-shikra-rproc-v3-0-2fca0bbe1ad7@oss.qualcomm.com/
> - https://lore.kernel.org/linux-devicetree/20260513-tsens_binding-v1-1-1780c6a6caf2@oss.qualcomm.com/

If the SD card depends on remote proc, tsens or cpufreq, then something
is wrong. Maybe, the way the serieas are organized and sent.

> - https://lore.kernel.org/all/20260524-shikra_epss_l3-v1-0-b1528a436134@oss.qualcomm.com/
> - https://lore.kernel.org/all/20260522-shikra-cpufreq-scaling-v4-0-f042a25896c5@oss.qualcomm.com/
> - https://lore.kernel.org/all/20260530-shikra-dt-m1-v2-0-6bb581035d13@oss.qualcomm.com/
> 
> Monish Chunara (2):
>   arm64: dts: qcom: Add SD Card support for Shikra SoC
>   arm64: dts: qcom: Enable SD card for Shikra EVK
> 
>  arch/arm64/boot/dts/qcom/shikra-cqm-evk.dts | 18 ++++
>  arch/arm64/boot/dts/qcom/shikra-cqs-evk.dts | 18 ++++
>  arch/arm64/boot/dts/qcom/shikra-iqs-evk.dts | 18 ++++
>  arch/arm64/boot/dts/qcom/shikra.dtsi        | 93 +++++++++++++++++++++
>  4 files changed, 147 insertions(+)
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

^ permalink raw reply

* Re: [PATCH v2 2/2] pinctrl: qcom: Add the tlmm driver for Maili platform
From: Dmitry Baryshkov @ 2026-06-16  0:55 UTC (permalink / raw)
  To: Jingyi Wang, g
  Cc: Bjorn Andersson, Linus Walleij, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, aiqun.yu, tingwei.zhang, trilok.soni, yijie.yang,
	linux-arm-msm, linux-gpio, devicetree, linux-kernel
In-Reply-To: <20260614-maili-pinctrl-v2-2-0db5bfc23d64@oss.qualcomm.com>

On Sun, Jun 14, 2026 at 11:55:04PM -0700, Jingyi Wang wrote:
> Add support for Maili TLMM configuration and control via the pinctrl
> framework.
> 
> Signed-off-by: Jingyi Wang <jingyi.wang@oss.qualcomm.com>
> ---
>  drivers/pinctrl/qcom/Kconfig.msm     |   10 +
>  drivers/pinctrl/qcom/Makefile        |    1 +
>  drivers/pinctrl/qcom/pinctrl-maili.c | 1630 ++++++++++++++++++++++++++++++++++
>  3 files changed, 1641 insertions(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

^ permalink raw reply

* Re: [PATCH v2 2/3] drm/msm/adreno: Add support for A704 GPU
From: Dmitry Baryshkov @ 2026-06-16  0:56 UTC (permalink / raw)
  To: Akhil P Oommen
  Cc: Rob Clark, Sean Paul, Konrad Dybcio, Dmitry Baryshkov,
	Abhinav Kumar, Jessica Zhang, Marijn Suijten, David Airlie,
	Simona Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Will Deacon, Robin Murphy, Joerg Roedel (AMD), Bibek Kumar Patro,
	linux-arm-msm, dri-devel, freedreno, devicetree, linux-kernel,
	linux-arm-kernel, iommu, Aditya Sherawat, Konrad Dybcio
In-Reply-To: <20260615-shikra-gpu-v2-2-2f2d1347c3fb@oss.qualcomm.com>

On Mon, Jun 15, 2026 at 03:02:58AM +0530, Akhil P Oommen wrote:
> From: Aditya Sherawat <asherawa@qti.qualcomm.com>
> 
> Adreno A704 GPU found in Shikra is an IP reuse of A702 GPU with very
> minimal changes. The only KMD facing difference is the chipid and the
> zap firmware which is specified via devicetree.
> 
> Just add the new chipid to enable support for A704 GPU in Shikra.
> 
> Signed-off-by: Aditya Sherawat <asherawa@qti.qualcomm.com>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Akhil P Oommen <akhilpo@oss.qualcomm.com>
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

^ permalink raw reply

* Re: [PATCH v2 1/2] dt-bindings: media: qcom,sm8550-iris: Allow IOVA reservation memory-region
From: Dmitry Baryshkov @ 2026-06-16  0:57 UTC (permalink / raw)
  To: Daniel J Blueman
  Cc: Bryan O'Donoghue, Vikash Garodia, Abhinav Kumar,
	Bjorn Andersson, Konrad Dybcio, Mauro Carvalho Chehab,
	Stephan Gerhold, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	linux-arm-msm, linux-media, devicetree, linux-kernel, stable
In-Reply-To: <20260614145113.84243-1-daniel@quora.org>

On Sun, Jun 14, 2026 at 10:51:11PM +0800, Daniel J Blueman wrote:
> In addition to the firmware-loaded codec carveout, some Iris platforms
> need to declare an IOMMU IOVA reservation (a reserved-memory node with
> iommu-addresses) to keep DMA away from IOVA ranges that earlier
> firmware stages have already mapped through the SMMU.
> 
> Permit a second memory-region phandle for this purpose, and describe
> the meaning of each entry so the ordering is unambiguous.

With no driver changes?

> 
> Fixes: 9065340ac04d ("arm64: dts: qcom: x1e80100: Add IRIS video codec")
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel J Blueman <daniel@quora.org>
> ---
> v2:
> - drop redundant maxItems, keeping the items descriptions (Rob)
> - add Fixes tag and Cc stable for the backport dependency
> v1: https://lore.kernel.org/lkml/20260601041336.9497-1-daniel@quora.org/
> 
>  .../devicetree/bindings/media/qcom,sm8550-iris.yaml          | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> index 9c4b760508b5..5abcaee4101c 100644
> --- a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> @@ -80,7 +80,10 @@ properties:
>    dma-coherent: true
>  
>    memory-region:
> -    maxItems: 1
> +    minItems: 1
> +    items:
> +      - description: Firmware-loaded codec carveout
> +      - description: IOMMU IOVA reservation region
>  
>    operating-points-v2: true
>  
> -- 
> 2.53.0
> 

-- 
With best wishes
Dmitry

^ permalink raw reply

* [PATCH v11 0/2] Add Support for LTC3220 18 Channel LED Driver
From: Edelweise Escala @ 2026-06-16  1:36 UTC (permalink / raw)
  To: Lee Jones, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: linux-leds, devicetree, linux-kernel, Edelweise Escala,
	Conor Dooley

The LTC3220/LTC3220-1 is a multi-display LED driver, which contains a
high-efficiency, low-noise charge pump to provide power to up to
18 LED current sources. The LEDs are individually configurable to
64-step linear brightness control, blinking and gradation control
via 2-wire I2C interface. The blinking and gradation configuration
is shared across all LED.

LTC3220 has a quick write function which allows changing the brightness
on all LEDS simultaneously when the brightness is changed on led 1.
For this leds are aggregated in the device tree and on probe we check
if led-sources exist to enable quick write. 
We would like to know if this approach is alright?
Another way we might want to know is, is it alright to just make a
virtual led for the quick write function. Changing brightness on 
the virtual led will change the brightness for all.

Signed-off-by: Edelweise Escala <edelweise.escala@analog.com>
---
Changes in v11:
- Add guards to check for double reg
- Add led_classdev_suspend/resume
- Return -EOPNOTSUPP for repeat request not 1
- Single write instead of write+read for blink
- Link to v10: https://lore.kernel.org/r/20260615-ltc3220-driver-v10-0-916562c88598@analog.com

Changes in v10:
- Remove volatile reg
- Add regacache_sync on resume
- Add delays on reset
- Remove early return and let software rset even if there is reset
- Set to max brightness before blink
- Two pass validation approach to avoid premature sysfs exposure
- Use pointer arithmetic for container_of
- Remove unused macros
- Add missing property header
- Link to v9: https://lore.kernel.org/r/20260528-ltc3220-driver-v9-0-69450fc213cb@analog.com

Changes in v9:
- Add address-cells and size-cells in required
- Restrict to reg 1 if led-sources is present
- Make set brightness only change brighntess for nomal mode,
  blink and gradation is now handled only in its respective triggers.
- Fix blink to change mode on blink set
- Make pattern clear only change brightness and not change gradation
  mode so it won't affect other leds.
- Add checks if ever there is duplicate reg in the bindings.
- Add REGMAP_I2C on Kconfig
- Rebase on Latest next
- Link to v8: https://lore.kernel.org/r/20260519-ltc3220-driver-v8-0-e1771c1733f7@analog.com

Changes in v8:
- Add '>' on descriptions with multiple paragraph
- Change led registers to hex
- Change cache type to REGCACHE_FLAT_S
- Link to v7: https://lore.kernel.org/r/20260508-ltc3220-driver-v7-0-0f092ba54f23@analog.com

Changes in v7:
- Add cahcetype on regmap
- Dropped struct ltc3220_state *ltc3220_state and use container_of()
- Rename ltc3220_state to ltc3220
- Dropped functions only used once and used inline instead.
- Loop variable changes
- Consistent decalaration for container_of
- Tabbing fixes
- Link to v6: https://lore.kernel.org/r/20260417-ltc3220-driver-v6-0-18157871eddd@analog.com

Changes in v6:
- Fix commit message
- Add manufacturer on Kconfig and improve description
- Rearrange register map and bitmask and improve naming
- Use regmap, also use update bits of regmap to avoid unnecessary
  structs
- Alignment and spacing fixes
- Use Define for magic naumbers
- Fix blink calculation
- Add comments on aggregated LED
- Fix variable name to something more understandable like i to led_index
- Link to v5: https://lore.kernel.org/r/20260126-ltc3220-driver-v5-0-152a30e98ab7@analog.com

Changes in v5:
- Missed rename on bindings filename in MAINTAINERS file
- Link to v4: https://lore.kernel.org/linux-leds/20260126-ltc3220-driver-v4-0-c59517206c24@analog.com

Changes in v4:
- Rename leds-ltc3220.yaml to adi,ltc3220.yaml
- Add Reviewed-by: Conor Dooley <conor.dooley@microchip.com> on
  adi,ltc3220.yaml
Other V1 comments I think already addressed
- Subject commit message was already changed to match hardware
- Fixed wrapping after description
- Dropped "Bindings for" in descriptions and improved description to match hardware
- Dropped adi,ltc3220-1
- Dropped redundant description on reset-gpios
- Dropped adi,force-cpo-level
- Dropped adi,quick-write in favor of aggregated LED
- Used consistent quotes ^led@([1-9]|1[0-8])$
- Fixed wrapping on error messages

- Link to v3: https://lore.kernel.org/r/20260120-ltc3220-driver-v3-0-fef612ec4faa@analog.com

Changes in v3:
- Dropped quick-write on bindings and added aggregated led instead.
- Add aggregated led example.
- Modify quick write to check if there is aggregated led, if there is
  aggregated led enable quick write.
- Use DEFINE_SIMPLE_DEV_PM_OPS instead of SIMPLE_DEV_PM_OPS.
- Link to v2: https://lore.kernel.org/r/20260112-ltc3220-driver-v2-0-d043058fc4df@analog.com

Changes in v2:
leds-ltc3220.yaml changes
- Fix wrapping on description
- Improve description and commit messge to describe hardware
- Drop ltc3220-1
- Drop charge pump
ltc3220.c changes
- Fix wrapping
- Drop ltc3220-1
- Drop devname_mandatory
- Link to v1: https://lore.kernel.org/r/20260106-ltc3220-driver-v1-0-73601d6f1649@analog.com

---
Edelweise Escala (2):
      dt-bindings: leds: Add LTC3220 18 channel LED Driver
      leds: ltc3220: Add Support for LTC3220 18 channel LED Driver

 .../devicetree/bindings/leds/adi,ltc3220.yaml      | 131 ++++++
 MAINTAINERS                                        |   8 +
 drivers/leds/Kconfig                               |  13 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-ltc3220.c                        | 480 +++++++++++++++++++++
 5 files changed, 633 insertions(+)
---
base-commit: 25025253476a64c186592d952c27f24bc3490e42
change-id: 20260106-ltc3220-driver-f9ab6cc9d1e4

Best regards,
-- 
Edelweise Escala <edelweise.escala@analog.com>


^ permalink raw reply

* [PATCH v11 1/2] dt-bindings: leds: Add LTC3220 18 channel LED Driver
From: Edelweise Escala @ 2026-06-16  1:36 UTC (permalink / raw)
  To: Lee Jones, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: linux-leds, devicetree, linux-kernel, Edelweise Escala,
	Conor Dooley
In-Reply-To: <20260616-ltc3220-driver-v11-0-6e51dd97ced6@analog.com>

LTC3220 is a multi-display LED driver with I2C interface.
The LTC3220 provides individual brightness control (64-step),
blinking, and gradation features for up to 18 LED outputs.

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Edelweise Escala <edelweise.escala@analog.com>
---
 .../devicetree/bindings/leds/adi,ltc3220.yaml      | 131 +++++++++++++++++++++
 MAINTAINERS                                        |   7 ++
 2 files changed, 138 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/adi,ltc3220.yaml b/Documentation/devicetree/bindings/leds/adi,ltc3220.yaml
new file mode 100644
index 000000000000..c859490e9108
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/adi,ltc3220.yaml
@@ -0,0 +1,131 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/adi,ltc3220.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices LTC3220 LED Driver
+
+maintainers:
+  - Edelweise Escala <edelweise.escala@analog.com>
+
+description: >
+  The LTC3220 is a multi-display LED driver, which contains a high-efficiency,
+  low-noise charge pump to provide power to up to 18 LED current sources.
+  The LEDs are individually configurable to 64-step linear brightness control,
+  blinking and gradation control via 2-wire I2C interface.
+
+  For more product information please see the link below
+    https://www.analog.com/en/products/ltc3220.html
+
+properties:
+  compatible:
+    const: adi,ltc3220
+
+  reg:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  reset-gpios:
+    maxItems: 1
+
+patternProperties:
+  '^led@([1-9a-f]|1[0-2])$':
+    type: object
+    $ref: /schemas/leds/common.yaml#
+    unevaluatedProperties: false
+    properties:
+      reg:
+        description:
+          Output channel for the LED (1-18 maps to LED outputs D1-D18).
+          Unit-address must be in hexadecimal (1-12 hex = 1-18 decimal).
+          For aggregated LED control, define only one LED node with reg = <0x1>
+          and use led-sources to list all controlled outputs. Only reg 1 should
+          be present when using led-sources.
+        minimum: 1
+        maximum: 18
+
+    required:
+      - reg
+
+    if:
+      required:
+        - led-sources
+    then:
+      properties:
+        reg:
+          const: 1
+
+required:
+  - compatible
+  - reg
+  - '#address-cells'
+  - '#size-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    // Independent LEDs
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/leds/common.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led-controller@1c {
+            compatible = "adi,ltc3220";
+            reg = <0x1c>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            reset-gpios = <&gpio 17 GPIO_ACTIVE_LOW>;
+
+            led@1 {
+                reg = <0x1>;
+                function = LED_FUNCTION_INDICATOR;
+                function-enumerator = <1>;
+            };
+
+            led@2 {
+                reg = <0x2>;
+                function = LED_FUNCTION_INDICATOR;
+                function-enumerator = <2>;
+            };
+
+            led@3 {
+                reg = <0x3>;
+                function = LED_FUNCTION_INDICATOR;
+                function-enumerator = <3>;
+            };
+        };
+    };
+
+  - |
+    // Aggregated LED
+    #include <dt-bindings/leds/common.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led-controller@1c {
+            compatible = "adi,ltc3220";
+            reg = <0x1c>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            led@1 {
+                reg = <0x1>;
+                led-sources = <0x1 0x2 0x3 0x4 0x5 0x6 0x7 0x8 0x9 0xa 0xb 0xc 0xd 0xe 0xf 0x10 0x11 0x12>;
+                function = LED_FUNCTION_BACKLIGHT;
+            };
+        };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 2fb1c75afd16..c8a242577d2f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15223,6 +15223,13 @@ W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
 F:	drivers/iio/temperature/ltc2983.c
 
+LTC3220 LED DRIVER
+M:	Edelweise Escala <edelweise.escala@analog.com>
+L:	linux-leds@vger.kernel.org
+S:	Maintained
+W:	https://ez.analog.com/linux-software-drivers
+F:	Documentation/devicetree/bindings/leds/adi,ltc3220.yaml
+
 LTC4282 HARDWARE MONITOR DRIVER
 M:	Nuno Sa <nuno.sa@analog.com>
 L:	linux-hwmon@vger.kernel.org

-- 
2.43.0


^ permalink raw reply related

* [PATCH v11 2/2] leds: ltc3220: Add Support for LTC3220 18 channel LED Driver
From: Edelweise Escala @ 2026-06-16  1:36 UTC (permalink / raw)
  To: Lee Jones, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: linux-leds, devicetree, linux-kernel, Edelweise Escala
In-Reply-To: <20260616-ltc3220-driver-v11-0-6e51dd97ced6@analog.com>

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 | 480 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 495 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..8bb06e50d3c6
--- /dev/null
+++ b/drivers/leds/leds-ltc3220.c
@@ -0,0 +1,480 @@
+// 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>
+#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/property.h>
+#include <linux/regmap.h>
+#include <linux/types.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_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 const struct regmap_config ltc3220_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = LTC3220_GRAD_BLINK_REG,
+	.cache_type = REGCACHE_FLAT_S,
+};
+
+struct ltc3220_uled_cfg {
+	struct led_classdev led_cdev;
+	u8 reg_value;
+	u8 led_index;
+	bool registered;
+};
+
+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 - uled_cfg->led_index, struct ltc3220,
+					       uled_cfg[0]);
+	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 - uled_cfg->led_index, struct ltc3220,
+					       uled_cfg[0]);
+	u8 gradation_period;
+	u8 start_brightness;
+	u8 end_brightness;
+	u8 gradation_val;
+	bool is_increasing;
+	int ret;
+
+	if (len != 2)
+		return -EINVAL;
+
+	if (repeat != 1)
+		return -EOPNOTSUPP;
+
+	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 - uled_cfg->led_index, struct ltc3220,
+					       uled_cfg[0]);
+	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 - uled_cfg->led_index, struct ltc3220,
+					       uled_cfg[0]);
+	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;
+
+	/* Ensure LED has non-zero brightness before enabling blink mode */
+	if (uled_cfg->reg_value == 0)
+		uled_cfg->reg_value = led_cdev->max_brightness;
+
+	return regmap_write(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg->led_index),
+			    FIELD_PREP(LTC3220_LED_MODE_MASK, LTC3220_BLINK_MODE) |
+			    uled_cfg->reg_value);
+}
+
+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) {
+		usleep_range(10000, 12000);
+		gpiod_set_value_cansleep(reset_gpio, 0);
+		usleep_range(10000, 12000);
+
+		ret = devm_add_action_or_reset(&client->dev, ltc3220_reset_gpio_action,
+						reset_gpio);
+		if (ret)
+			return ret;
+	}
+
+	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));
+
+	for (int i = 0; i < LTC3220_NUM_LEDS; i++) {
+		if (ltc3220->uled_cfg[i].registered)
+			led_classdev_suspend(&ltc3220->uled_cfg[i].led_cdev);
+	}
+
+	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));
+	int ret;
+
+	ret = regmap_update_bits(ltc3220->regmap, LTC3220_COMMAND_REG,
+				 LTC3220_SHUTDOWN_MASK, 0);
+	if (ret)
+		return ret;
+
+	ret = regcache_sync(ltc3220->regmap);
+	if (ret)
+		return ret;
+
+	for (int i = 0; i < LTC3220_NUM_LEDS; i++) {
+		if (ltc3220->uled_cfg[i].registered)
+			led_classdev_resume(&ltc3220->uled_cfg[i].led_cdev);
+	}
+
+	return 0;
+}
+
+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, &ltc3220_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");
+
+	/* First pass: validate configuration and set up LED structures */
+	device_for_each_child_node_scoped(&client->dev, child) {
+		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");
+
+		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 = &ltc3220->uled_cfg[led_index];
+
+		if (led->registered)
+			return dev_err_probe(&client->dev, -EINVAL,
+					     "Duplicate LED reg %u found", source);
+
+		led->registered = true;
+		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;
+	}
+
+	/*
+	 * 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");
+
+	/* Second pass: register LEDs after validation */
+	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 ret;
+
+		init_data.fwnode = child;
+		init_data.devicename = "ltc3220";
+
+		led_index = source - 1;
+		led = &ltc3220->uled_cfg[led_index];
+
+		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");
+	}
+
+	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(&ltc3220_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


^ permalink raw reply related

* Re: [PATCH v11 2/2] leds: ltc3220: Add Support for LTC3220 18 channel LED Driver
From: sashiko-bot @ 2026-06-16  1:48 UTC (permalink / raw)
  To: Edelweise Escala; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260616-ltc3220-driver-v11-2-6e51dd97ced6@analog.com>

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Typographical error in regmap_config uses an undefined enum value `REGCACHE_FLAT_S`, causing a build failure.
- [High] Aggregated mode (QUICK_WRITE) causes the regmap cache to desynchronize, breaking LEDs during regcache_sync().
- [Medium] regcache_sync() in the resume handler is a no-op because the regmap cache is never marked dirty.
- [Medium] User-provided brightness values in hardware patterns are incorrectly masked instead of clamped, causing wrap-around behavior.
- [Medium] The driver modifies the cached LED state (`uled_cfg->reg_value`) and hardware registers without driver-level synchronization, allowing concurrent operations to permanently desync the software cache from the hardware.
--

commit 531f73073c031952f85d8c75169d6129f992543c
Author: Edelweise Escala <edelweise.escala@analog.com>

leds: ltc3220: Add Support for LTC3220 18 channel LED Driver
    
Add driver for the LTC3220 18-channel LED driver with I2C interface,
individual brightness control, and hardware-assisted blink/gradation
features.

> diff --git a/drivers/leds/leds-ltc3220.c b/drivers/leds/leds-ltc3220.c
> new file mode 100644
> index 0000000000000..8bb06e50d3c6a
> --- /dev/null
> +++ b/drivers/leds/leds-ltc3220.c
[ ... ]
> +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,

[Severity: High]
Does this cause a build failure?

The correct enum value defined by the kernel is REGCACHE_FLAT, but this uses
an undefined REGCACHE_FLAT_S.

> +};
[ ... ]
> +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 - uled_cfg->led_index, struct ltc3220,
> +					       uled_cfg[0]);
> +	int ret;
> +
> +	ret = regmap_write(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg->led_index),
> +			   brightness);

[Severity: High]
When configured in aggregated mode, the hardware's QUICK_WRITE feature is
enabled, and a single write to LED 0 implicitly updates LEDs 1-17 in
hardware.

However, the driver doesn't update the regmap cache for registers 0x02
through 0x12, leaving them at 0.

When regcache_sync() runs during resume, will it write out the cached 0s and
inadvertently turn off LEDs 1-17?

Should the driver manually sync the cache for these implicitly updated
registers?

> +	if (ret)
> +		return ret;
> +
> +	uled_cfg->reg_value = brightness;

[Severity: Medium]
Since the LED core does not strictly serialize asynchronous triggers against
all callback paths, is this unprotected assignment susceptible to a race
condition?

A concurrent call to ltc3220_set_led_data() or ltc3220_blink_set() could
cause the software reg_value to become permanently desynchronized from the
hardware register state.

Does this need a per-device or per-LED mutex to protect read-modify-write
operations?

> +
> +	/*
> +	 * When aggregated LED mode is enabled, writing to LED 1 updates all
> +	 * LEDs simultaneously via quick-write mode. Update cached values for
[ ... ]
> +static int ltc3220_pattern_set(struct led_classdev *led_cdev,
> +			       struct led_pattern *pattern,
> +			       u32 len, int repeat)
> +{
[ ... ]
> +	if (repeat != 1)
> +		return -EOPNOTSUPP;
> +
> +	start_brightness = pattern[0].brightness & LTC3220_LED_CURRENT_MASK;
> +	end_brightness = pattern[1].brightness & LTC3220_LED_CURRENT_MASK;

[Severity: Medium]
Will this bitwise masking cause wrap-around behavior if a user provides a
brightness value greater than 63 via the sysfs hw_pattern interface?

For instance, a value of 64 would wrap to 0, unexpectedly turning the LED
off.

Would it be safer to use min_t() to clamp the input instead of a
bitwise mask?

> +
> +	is_increasing = end_brightness > start_brightness;
[ ... ]
> +static int ltc3220_blink_set(struct led_classdev *led_cdev,
> +			     unsigned long *delay_on,
> +			     unsigned long *delay_off)
> +{
[ ... ]
> +	ret = regmap_update_bits(ltc3220->regmap, LTC3220_GRAD_BLINK_REG,
> +				 LTC3220_BLINK_MASK, FIELD_PREP(LTC3220_BLINK_MASK, blink_mode));
> +	if (ret)
> +		return ret;
> +
> +	/* Ensure LED has non-zero brightness before enabling blink mode */
> +	if (uled_cfg->reg_value == 0)
> +		uled_cfg->reg_value = led_cdev->max_brightness;

[Severity: Medium]
Could this rely on a stale reg_value if concurrent operations are
modifying the state without synchronization, as noted in
ltc3220_set_led_data() above?

> +
> +	return regmap_write(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg->led_index),
> +			    FIELD_PREP(LTC3220_LED_MODE_MASK, LTC3220_BLINK_MODE) |
> +			    uled_cfg->reg_value);
> +}
[ ... ]
> +static int ltc3220_suspend(struct device *dev)
> +{
> +	struct ltc3220 *ltc3220 = i2c_get_clientdata(to_i2c_client(dev));
> +
> +	for (int i = 0; i < LTC3220_NUM_LEDS; i++) {
> +		if (ltc3220->uled_cfg[i].registered)
> +			led_classdev_suspend(&ltc3220->uled_cfg[i].led_cdev);
> +	}
> +
> +	return regmap_update_bits(ltc3220->regmap, LTC3220_COMMAND_REG,
> +				 LTC3220_SHUTDOWN_MASK, LTC3220_SHUTDOWN_MASK);
> +}

[Severity: Medium]
Should regcache_mark_dirty(ltc3220->regmap) be called here or in resume?

Without marking the cache dirty, the regcache_sync() in ltc3220_resume()
will silently return without restoring any registers, resulting in the
permanent loss of the blink and gradation states upon resume.

> +
> +static int ltc3220_resume(struct device *dev)
> +{
> +	struct ltc3220 *ltc3220 = i2c_get_clientdata(to_i2c_client(dev));
> +	int ret;
> +
> +	ret = regmap_update_bits(ltc3220->regmap, LTC3220_COMMAND_REG,
> +				 LTC3220_SHUTDOWN_MASK, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = regcache_sync(ltc3220->regmap);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-ltc3220-driver-v11-0-6e51dd97ced6@analog.com?part=2

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox