Re: [PATCH 2/3] soc: ti: k3-socinfo: Export SoC registration helper

[Andrew Davis <afd@ti.com>]

On 10/19/26 3:07 AM, Moteen Shah wrote:
> The k3-socinfo driver reads the JTAG ID through a dedicated MMIO region and
> then decodes it and registers the SoC device.
> 
> Newer TI SoCs like TDA54 use a different path for accessing the JTAG ID
> register, but the decoding of the JTAG ID into the SoC information remains
> the same.
> 
> Split the decode and registration part out of k3_chipinfo_probe() into
> k3_socinfo_register() and export it, so that other drivers can register the
> SoC device once they have read the JTAG ID through their own access path.
> 
> Since the helper can now be called from modular drivers, it must not leak
> the SoC device when the caller goes away. Tie the registration to the
> calling device with a devm action, which unregisters the SoC device and
> frees its attributes on unbind. Callers need no unregister counterpart and
> the helper keeps returning an int. If the soc bus is not up yet, the
> attributes are only parked for later registration and there is nothing to
> undo, so no action is added in that case.
> 
> Signed-off-by: Moteen Shah <m-shah@ti.com>
> ---
>   drivers/soc/ti/k3-socinfo.c       | 101 +++++++++++++++++++++++-------
>   include/linux/soc/ti/k3-socinfo.h |  11 ++++
>   2 files changed, 90 insertions(+), 22 deletions(-)
>   create mode 100644 include/linux/soc/ti/k3-socinfo.h
> 
> diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
> index 48dbef3acceb4..5fae03004e350 100644
> --- a/drivers/soc/ti/k3-socinfo.c
> +++ b/drivers/soc/ti/k3-socinfo.c
> @@ -5,6 +5,8 @@
>    * Copyright (C) 2020 Texas Instruments Incorporated - http://www.ti.com
>    */
>   
> +#include <linux/device.h>
> +#include <linux/export.h>
>   #include <linux/mfd/syscon.h>
>   #include <linux/nvmem-consumer.h>
>   #include <linux/of.h>
> @@ -12,6 +14,7 @@
>   #include <linux/regmap.h>
>   #include <linux/platform_device.h>
>   #include <linux/slab.h>
> +#include <linux/soc/ti/k3-socinfo.h>
>   #include <linux/string.h>
>   #include <linux/sys_soc.h>
>   
> @@ -152,38 +155,44 @@ k3_chipinfo_variant_to_sr(struct device *dev, unsigned int partno,
>   	return -ENODEV;
>   }
>   
> -static const struct regmap_config k3_chipinfo_regmap_cfg = {
> -	.reg_bits = 32,
> -	.val_bits = 32,
> -	.reg_stride = 4,
> +struct k3_socinfo {
> +	struct soc_device *soc_dev;
> +	struct soc_device_attribute *soc_dev_attr;
>   };
>   
> -static int k3_chipinfo_probe(struct platform_device *pdev)
> +static void k3_socinfo_unregister(void *data)
> +{
> +	struct k3_socinfo *info = data;
> +
> +	soc_device_unregister(info->soc_dev);
> +	/* The attributes must only be freed after the unregister. */
> +	kfree(info->soc_dev_attr->revision);
> +	kfree(info->soc_dev_attr);
> +}
> +
> +/**
> + * k3_socinfo_register() - Decode a JTAG ID and register the SoC device
> + * @dev: device on whose behalf the SoC device is registered
> + * @jtag_id: raw JTAG ID register value read by the caller
> + *
> + * The registration is managed by @dev: the SoC device and its attributes are
> + * released automatically when @dev is unbound, so callers need no matching
> + * unregister call. If the soc bus is not yet available, the registration is
> + * deferred until it is and cannot be released by @dev.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int k3_socinfo_register(struct device *dev, u32 jtag_id)
>   {
> -	struct device_node *node = pdev->dev.of_node;
> +	struct device_node *node;
> +	struct k3_socinfo *info;
>   	struct soc_device_attribute *soc_dev_attr;
> -	struct device *dev = &pdev->dev;
>   	struct soc_device *soc_dev;
> -	struct regmap *regmap;
> -	void __iomem *base;
>   	u32 partno_id;
>   	u32 variant;
> -	u32 jtag_id;
>   	u32 mfg;
>   	int ret;
>   
> -	base = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(base))
> -		return PTR_ERR(base);
> -
> -	regmap = devm_regmap_init_mmio(dev, base, &k3_chipinfo_regmap_cfg);
> -	if (IS_ERR(regmap))
> -		return PTR_ERR(regmap);
> -
> -	ret = regmap_read(regmap, CTRLMMR_WKUP_JTAGID_REG, &jtag_id);
> -	if (ret < 0)
> -		return ret;
> -
>   	mfg = (jtag_id & CTRLMMR_WKUP_JTAGID_MFG_MASK) >>
>   	       CTRLMMR_WKUP_JTAGID_MFG_SHIFT;
>   
> @@ -198,6 +207,10 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
>   	partno_id = (jtag_id & CTRLMMR_WKUP_JTAGID_PARTNO_MASK) >>
>   		 CTRLMMR_WKUP_JTAGID_PARTNO_SHIFT;
>   
> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
>   	soc_dev_attr = kzalloc_obj(*soc_dev_attr);
>   	if (!soc_dev_attr)
>   		return -ENOMEM;
> @@ -224,6 +237,20 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
>   		goto err_free_rev;
>   	}
>   
> +	/*
> +	 * A NULL handle means the soc bus is not up yet and the attributes
> +	 * were parked for registration at soc bus init. They must stay
> +	 * allocated and cannot be unregistered, so there is nothing to undo.

This seems really messy, the only time drivers need to deal with this
early_soc_dev_attr hackyness is if they can register before the soc bus,
which only happens for drivers using early_initcall(), but that is
not the case for either driver using k3_socinfo_register().

Can you explain why you added all this?

Otherwise just allocate soc_dev_attr using devm, k3_socinfo_unregister()
action can be added unconditionally, and only needs to call
soc_device_unregister().

Andrew

> +	 */
> +	if (soc_dev) {
> +		info->soc_dev = soc_dev;
> +		info->soc_dev_attr = soc_dev_attr;
> +		ret = devm_add_action_or_reset(dev, k3_socinfo_unregister,
> +					       info);
> +		if (ret)
> +			return ret;
> +	}
> +
>   	dev_info(dev, "Family:%s rev:%s JTAGID[0x%08x] Detected\n",
>   		 soc_dev_attr->family,
>   		 soc_dev_attr->revision, jtag_id);
> @@ -236,6 +263,36 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
>   	kfree(soc_dev_attr);
>   	return ret;
>   }
> +EXPORT_SYMBOL_GPL(k3_socinfo_register);
> +
> +static const struct regmap_config k3_chipinfo_regmap_cfg = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +};
> +
> +static int k3_chipinfo_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct regmap *regmap;
> +	void __iomem *base;
> +	u32 jtag_id;
> +	int ret;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	regmap = devm_regmap_init_mmio(dev, base, &k3_chipinfo_regmap_cfg);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	ret = regmap_read(regmap, CTRLMMR_WKUP_JTAGID_REG, &jtag_id);
> +	if (ret < 0)
> +		return ret;
> +
> +	return k3_socinfo_register(dev, jtag_id);
> +}
>   
>   static const struct of_device_id k3_chipinfo_of_match[] = {
>   	{ .compatible = "ti,am654-chipid", },
> diff --git a/include/linux/soc/ti/k3-socinfo.h b/include/linux/soc/ti/k3-socinfo.h
> new file mode 100644
> index 0000000000000..487e69d0069d1
> --- /dev/null
> +++ b/include/linux/soc/ti/k3-socinfo.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_SOC_TI_K3_SOCINFO_H__
> +#define __LINUX_SOC_TI_K3_SOCINFO_H__
> +
> +#include <linux/types.h>
> +
> +struct device;
> +
> +int k3_socinfo_register(struct device *dev, u32 jtag_id);
> +
> +#endif /* __LINUX_SOC_TI_K3_SOCINFO_H__ */