Re: [PATCH 2/4] soc: qcom: llcc: add LCP/SRP DDR DRAM ECC register offsets

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 14 Oct 2026 21:26:17 +0530
Faiyaz Mohammed <faiyaz.mohammed@oss.qualcomm.com> wrote:

> Add LCP/SRP ECC register offsets to struct llcc_edac_reg_offset and
> create llcc_lcp_srp_edac_reg_offset for QCM2390, leaving the shared
> llcc_v2_1_edac_reg_offset table unchanged. Switch shikra_cfg to the
> new table so other platforms with the same block can reuse it.
> 
> Add lcp_irq to struct llcc_drv_data and retrieve the optional second
> interrupt in qcom_llcc_probe(), propagating non-absence errors so the
> driver defers correctly.
> 
> Signed-off-by: Faiyaz Mohammed <faiyaz.mohammed@oss.qualcomm.com>
Hi Faiyaz,

One comment on existing code + a suggestion to tidy the new stuff
up a little.

That global for drv_data is less than ideal, but given you are touching
the driver please tidy up initial assignment to use INIT_ERR_PTR()


>  static const struct llcc_edac_reg_offset llcc_v6_edac_reg_offset = {
>  	.trp_ecc_error_status0 = 0x47448,
>  	.trp_ecc_error_status1 = 0x47450,
> @@ -4767,7 +4802,7 @@ static const struct qcom_llcc_config shikra_cfg[] = {
>  		.sct_data	= shikra_data,
>  		.size		= ARRAY_SIZE(shikra_data),
>  		.reg_offset	= llcc_v2_1_reg_offset,
> -		.edac_reg_offset = &llcc_v2_1_edac_reg_offset,
> +		.edac_reg_offset = &llcc_lcp_srp_edac_reg_offset,
>  		.irq_configured = true,
>  	},
>  };
> @@ -5798,7 +5833,13 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>  	if (drv_data->ecc_irq < 0 && drv_data->ecc_irq != -ENXIO)
>  		return dev_err_probe(&pdev->dev, drv_data->ecc_irq,
>  				     "failed to get IRQ resource\n");

Unrelated to your patch, but why don't we need that tiny bit of error cleanup under
goto err for this path?  That use of a global is not the best anyway, but none
the less I'd expect either some consistency or a comment on why not in the above
path.

> -
> +	if (cfg->edac_reg_offset && cfg->edac_reg_offset->lcp_srp_interrupt_enable) {
> +		drv_data->lcp_irq = platform_get_irq_optional(pdev, 1);
> +		if (drv_data->lcp_irq < 0 && drv_data->lcp_irq != -ENXIO) {
> +			ret = drv_data->lcp_irq;
> +			goto err;
> +		}

Maybe it's local style, but generally I'd put the value in ret until you've checked validty
then copy it to the good location.

		ret = platform_get_irq_optional(pdev, 1);
		if (ret < 0 && ret != -ENXIO)
			goto err;
		drv_data->lcp_irq = ret;
Both more compact and obvious when we trust the value to be good.

		
> +	}
>  	drv_data->edac_reg_offset = cfg->edac_reg_offset;
>  	drv_data->ecc_irq_configured = cfg->irq_configured;
>  	drv_data->dev = dev;