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

[Faiyaz Mohammed <faiyaz.mohammed@oss.qualcomm.com>]

On 16-10-2026 22:50, Jonathan Cameron wrote:
> 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()
> 
> 
Hi Jonathan,

Done.
>>  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.
> 
Good point — changed lcp_irq to use return dev_err_probe() to be
consistent with the adjacent ecc_irq 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.
> 
> 	
Agreed, restructured accordingly. Will repost as v2.
	
>> +	}
>>  	drv_data->edac_reg_offset = cfg->edac_reg_offset;
>>  	drv_data->ecc_irq_configured = cfg->irq_configured;
>>  	drv_data->dev = dev;
> 

Thanks & regards,
Mohammed Faiyaz