Re: [PATCH 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

On 12/10/2021 19:16, Sam Protsenko wrote:
> Old Exynos SoCs have both Product ID and Revision ID in one single
> register, while new SoCs tend to have two separate registers for those
> IDs. Implement handling of both cases by passing Revision ID register
> offsets in driver data.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/soc/samsung/exynos-chipid.c       | 67 +++++++++++++++++++----
>  include/linux/soc/samsung/exynos-chipid.h |  6 +-
>  2 files changed, 58 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c
> index 5c1d0f97f766..1264a18aef97 100644
> --- a/drivers/soc/samsung/exynos-chipid.c
> +++ b/drivers/soc/samsung/exynos-chipid.c
> @@ -16,6 +16,7 @@
>  #include <linux/errno.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
> @@ -24,6 +25,17 @@
>  
>  #include "exynos-asv.h"
>  
> +struct exynos_chipid_variant {
> +	unsigned int rev_reg;		/* revision register offset */
> +	unsigned int main_rev_bit;	/* main revision offset */

I understand it is offset of a bit within register, so how about:

unsigned int main_rev_shift;	/* main revision offset within rev_reg
unsigned int sub_rev_shift;	/* sub revision offset within rev_reg */

> +	unsigned int sub_rev_bit;	/* sub revision offset */
> +};
> +
> +struct exynos_chipid_info {
> +	u32 product_id;
> +	u32 revision;
> +};
> +
>  static const struct exynos_soc_id {
>  	const char *name;
>  	unsigned int id;
> @@ -49,31 +61,55 @@ static const char *product_id_to_soc_id(unsigned int product_id)
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(soc_ids); i++)
> -		if ((product_id & EXYNOS_MASK) == soc_ids[i].id)
> +		if (product_id == soc_ids[i].id)
>  			return soc_ids[i].name;
>  	return NULL;
>  }
>  
> +static int exynos_chipid_get_chipid_info(struct regmap *regmap,
> +		const struct exynos_chipid_variant *data,
> +		struct exynos_chipid_info *soc_info)
> +{
> +	int ret;
> +	unsigned int val, main_rev, sub_rev;
> +
> +	ret = regmap_read(regmap, EXYNOS_CHIPID_REG_PRO_ID, &val);
> +	if (ret < 0)
> +		return ret;
> +	soc_info->product_id = val & EXYNOS_MASK;
> +
> +	ret = regmap_read(regmap, data->rev_reg, &val);
> +	if (ret < 0)
> +		return ret;
> +	main_rev = (val >> data->main_rev_bit) & EXYNOS_REV_PART_LEN;
> +	sub_rev = (val >> data->sub_rev_bit) & EXYNOS_REV_PART_LEN;
> +	soc_info->revision = (main_rev << EXYNOS_REV_PART_OFF) | sub_rev;
> +
> +	return 0;
> +}
> +
>  static int exynos_chipid_probe(struct platform_device *pdev)
>  {
> +	const struct exynos_chipid_variant *drv_data;
> +	struct exynos_chipid_info soc_info;
>  	struct soc_device_attribute *soc_dev_attr;
>  	struct soc_device *soc_dev;
>  	struct device_node *root;
>  	struct regmap *regmap;
> -	u32 product_id;
> -	u32 revision;
>  	int ret;
>  
> +	drv_data = of_device_get_match_data(&pdev->dev);
> +	if (!drv_data)
> +		return -EINVAL;
> +
>  	regmap = device_node_to_regmap(pdev->dev.of_node);
>  	if (IS_ERR(regmap))
>  		return PTR_ERR(regmap);
>  
> -	ret = regmap_read(regmap, EXYNOS_CHIPID_REG_PRO_ID, &product_id);
> +	ret = exynos_chipid_get_chipid_info(regmap, drv_data, &soc_info);
>  	if (ret < 0)
>  		return ret;
>  
> -	revision = product_id & EXYNOS_REV_MASK;
> -
>  	soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
>  				    GFP_KERNEL);
>  	if (!soc_dev_attr)
> @@ -86,8 +122,8 @@ static int exynos_chipid_probe(struct platform_device *pdev)
>  	of_node_put(root);
>  
>  	soc_dev_attr->revision = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> -						"%x", revision);
> -	soc_dev_attr->soc_id = product_id_to_soc_id(product_id);
> +						"%x", soc_info.revision);
> +	soc_dev_attr->soc_id = product_id_to_soc_id(soc_info.product_id);
>  	if (!soc_dev_attr->soc_id) {
>  		pr_err("Unknown SoC\n");
>  		return -ENODEV;
> @@ -106,7 +142,7 @@ static int exynos_chipid_probe(struct platform_device *pdev)
>  
>  	dev_info(soc_device_to_device(soc_dev),
>  		 "Exynos: CPU[%s] PRO_ID[0x%x] REV[0x%x] Detected\n",
> -		 soc_dev_attr->soc_id, product_id, revision);
> +		 soc_dev_attr->soc_id, soc_info.product_id, soc_info.revision);
>  
>  	return 0;
>  
> @@ -125,9 +161,18 @@ static int exynos_chipid_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct exynos_chipid_variant exynos4210_chipid_drv_data = {
> +	.rev_reg	= 0x0,
> +	.main_rev_bit	= 0,
> +	.sub_rev_bit	= 4,
> +};
> +
>  static const struct of_device_id exynos_chipid_of_device_ids[] = {
> -	{ .compatible = "samsung,exynos4210-chipid" },
> -	{}
> +	{
> +		.compatible	= "samsung,exynos4210-chipid",
> +		.data		= &exynos4210_chipid_drv_data,
> +	},
> +	{ }
>  };
>  
>  static struct platform_driver exynos_chipid_driver = {
> diff --git a/include/linux/soc/samsung/exynos-chipid.h b/include/linux/soc/samsung/exynos-chipid.h
> index 8bca6763f99c..5270725ba408 100644
> --- a/include/linux/soc/samsung/exynos-chipid.h
> +++ b/include/linux/soc/samsung/exynos-chipid.h
> @@ -9,10 +9,8 @@
>  #define __LINUX_SOC_EXYNOS_CHIPID_H
>  
>  #define EXYNOS_CHIPID_REG_PRO_ID	0x00
> -#define EXYNOS_SUBREV_MASK		(0xf << 4)
> -#define EXYNOS_MAINREV_MASK		(0xf << 0)
> -#define EXYNOS_REV_MASK			(EXYNOS_SUBREV_MASK | \
> -					 EXYNOS_MAINREV_MASK)
> +#define EXYNOS_REV_PART_LEN		0xf

EXYNOS_REV_PART_MASK

> +#define EXYNOS_REV_PART_OFF		4

define EXYNOS_REV_PART_SHIFT

>  #define EXYNOS_MASK			0xfffff000
>  
>  #define EXYNOS_CHIPID_REG_PKG_ID	0x04
> 


Best regards,
Krzysztof