Re: [PATCH v3 3/3] nvmem: stm32: add TAMP nvmem driver

[Patrice CHOTARD <patrice.chotard@foss.st.com>]

On 10/17/26 19:57, Krzysztof Kozlowski wrote:
> On Fri, Oct 16, 2026 at 05:26:05PM +0200, Patrice Chotard wrote:
>> +static int stm32_tamp_nvram_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct stm32_tamp_nvram_priv *priv;
>> +	struct resource *res;
>> +	struct device_node *of_node;
>> +	const struct of_device_id *of_id;
>> +	void __iomem *base_addr;
>> +	int *zones_end;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->dev = &pdev->dev;
>> +
>> +	of_node = dev_of_node(dev);
>> +	if (!of_node) {
>> +		dev_err(dev, "Can't get of node of the device\n");
>> +		return -ENODEV;
> 
> Entire dev_of_node and if() look not necessary.

Ok

> 
>> +	}
>> +
>> +	of_id = of_match_node(stm32_tamp_nvram_of_match, of_node);
>> +	if (!of_id) {
>> +		dev_err(dev, "Can't get of_id of the device\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	priv->data = (struct stm32_tamp_nvram_plat *)of_id->data;
> 
> Use wrapper - device_get_match_data or something similar (don't remember
> exactly).

Ok> 
>> +	if (!priv->data) {
>> +		dev_err(dev, "Can't get platform data of the node\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	priv->config_regmap = syscon_node_to_regmap(dev->parent->of_node);
>> +	if (IS_ERR(priv->config_regmap))
>> +		return dev_err_probe(dev, PTR_ERR(priv->config_regmap),
>> +				     "Can't get TAMP syscon regmap\n");
>> +
>> +	priv->idx_bkpreg_zones_end = stm32_tamp_nvram_get_backup_zones(priv);
>> +	if (IS_ERR(priv->idx_bkpreg_zones_end))
>> +		return dev_err_probe(dev, PTR_ERR(priv->idx_bkpreg_zones_end),
>> +				     "Can't determine protection zone\n");
>> +
>> +	zones_end = priv->idx_bkpreg_zones_end;
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
> 
> Drop blank line

Ok> 
>> +	if (!res) {
>> +		dev_err(dev, "Can't get resource\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	base_addr = devm_ioremap_resource(dev, res);
> 
> You cannot use wrapper over these two? And I am a bit confused - device
> takes syscon from the parent, so how does it map also another memory?

This part will be reworked. On patch 2 of DT series [1], sashiko made a similar 
remark regarding the overlapping parent syscon's memory region.
I will address this point in next revision.

[1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20260930-upstream_tamp_nvram_dts_part-v2-2-bd73158ce46b@foss.st.com/

> 
>> +	if (IS_ERR(base_addr)) {
>> +		dev_err(dev, "Can't remap resource\n");
>> +		return PTR_ERR(base_addr);
>> +	}
>> +
>> +	priv->cfg.name = "stm32-tamp-nvram";
>> +	priv->cfg.word_size = 1;
>> +	priv->cfg.stride = 1;
>> +	priv->cfg.dev = dev;
>> +	priv->cfg.priv = priv;
>> +	priv->cfg.owner = THIS_MODULE;
>> +	priv->cfg.type = NVMEM_TYPE_BATTERY_BACKED;
>> +	priv->cfg.size = resource_size(res);
>> +	priv->cfg.reg_read = stm32_tamp_nvram_read;
>> +	priv->cfg.reg_write = stm32_tamp_nvram_write;
>> +
>> +	if (priv->data->nb_zones == NB_ZONES_STM32MP2) {
>> +		priv->bkpreg_access = stm32_tamp_get_access_rights_mp2x(priv);
>> +
>> +		if (IS_ERR(priv->bkpreg_access))
>> +			return PTR_ERR(priv->bkpreg_access);
>> +
>> +		dev_dbg(dev, "\n"
>> +			"Zone 1-RIF1 %3d - %3d %c%c\n"
>> +			"Zone 1-RIF2 %3d - %3d %c%c\n"
>> +			"Zone 2-RIF1 %3d - %3d %c%c\n"
>> +			"Zone 2-RIF2 %3d - %3d %c%c\n"
>> +			"Zone 3-RIF1 %3d - %3d %c%c\n"
>> +			"Zone 3-RIF0 %3d - %3d %c%c\n"
>> +			"Zone 3-RIF2 %3d - %3d %c%c\n",
>> +			0,
>> +			zones_end[BKPREG_PROTECTION_ZONE_1_RIF1],
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_1_RIF1],
>> +						true) ? 'R' : '-',
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_1_RIF1],
>> +						false) ? 'W' : '-',
>> +			zones_end[BKPREG_PROTECTION_ZONE_1_RIF1] + 1,
>> +			zones_end[BKPREG_PROTECTION_ZONE_1_RIF2],
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_1_RIF2],
>> +						true) ? 'R' : '-',
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_1_RIF2],
>> +						false) ? 'W' : '-',
>> +			zones_end[BKPREG_PROTECTION_ZONE_1_RIF2] + 1,
>> +			zones_end[BKPREG_PROTECTION_ZONE_2_RIF1],
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_2_RIF1],
>> +						true) ? 'R' : '-',
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_2_RIF1],
>> +						false) ? 'W' : '-',
>> +			zones_end[BKPREG_PROTECTION_ZONE_2_RIF1] + 1,
>> +			zones_end[BKPREG_PROTECTION_ZONE_2_RIF2],
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_2_RIF2],
>> +						true) ? 'R' : '-',
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_2_RIF2],
>> +						false) ? 'W' : '-',
>> +			zones_end[BKPREG_PROTECTION_ZONE_2_RIF2] + 1,
>> +			zones_end[BKPREG_PROTECTION_ZONE_3_RIF1],
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_3_RIF1],
>> +						true) ? 'R' : '-',
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_3_RIF1],
>> +						false) ? 'W' : '-',
>> +			zones_end[BKPREG_PROTECTION_ZONE_3_RIF1] + 1,
>> +			zones_end[BKPREG_PROTECTION_ZONE_3_RIF0],
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_3_RIF0],
>> +						true) ? 'R' : '-',
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_3_RIF0],
>> +						false) ? 'W' : '-',
>> +			zones_end[BKPREG_PROTECTION_ZONE_3_RIF0] + 1,
>> +			zones_end[BKPREG_PROTECTION_ZONE_3_RIF2],
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_3_RIF2],
>> +						true) ? 'R' : '-',
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_3_RIF2],
>> +						false) ? 'W' : '-'
>> +				);
>> +	} else if (priv->data->nb_zones == NB_ZONES_STM32MP1) {
>> +		priv->bkpreg_access = stm32mp1_tamp_bkpreg_access;
>> +
>> +		dev_dbg(dev, "\n"
>> +			"Zone 1 %3d - %3d %c%c\n"
>> +			"Zone 2 %3d - %3d %c%c\n"
>> +			"Zone 3 %3d - %3d %c%c\n",
>> +			0,
>> +			zones_end[BKPREG_PROTECTION_ZONE_1],
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_1],
>> +						true) ? 'R' : '-',
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_1],
>> +						false) ? 'W' : '-',
>> +			zones_end[BKPREG_PROTECTION_ZONE_1] + 1,
>> +			zones_end[BKPREG_PROTECTION_ZONE_2],
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_2],
>> +						true) ? 'R' : '-',
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_2],
>> +						false) ? 'W' : '-',
>> +			zones_end[BKPREG_PROTECTION_ZONE_2] + 1,
>> +			zones_end[BKPREG_PROTECTION_ZONE_3],
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_3],
>> +						true) ? 'R' : '-',
>> +			stm32_tamp_nvram_rights(priv,
>> +						zones_end[BKPREG_PROTECTION_ZONE_3],
>> +						false) ? 'W' : '-');
>> +	}
>> +
>> +	platform_set_drvdata(pdev, priv);
>> +	priv->bkpregs_regmap = devm_regmap_init_mmio(dev, base_addr,
>> +						     priv->data->bkpregs_regmap_cfg);
>> +	if (IS_ERR(priv->bkpregs_regmap)) {
>> +		dev_err(dev, "failed to init regmap: %ld\n", PTR_ERR(priv->bkpregs_regmap));
>> +		return PTR_ERR(priv->bkpregs_regmap);
> 
> return dev_err_probe

Ok

Thanks
Patrice

> 
> 
> Best regards,
> Krzysztof
>