Message-ID: <52elmnb77a4kvlnmy5bhheypfyyw6x5qn4k45u22mvzybax7ti@wmpi2ww5uqsg>
Date: Tue, 17 Jun 2025 09:24:57 -0500
From: Bjorn Andersson <andersson@...nel.org>
To: Praveen Talari <quic_ptalari@...cinc.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Jiri Slaby <jirislaby@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Konrad Dybcio <konradybcio@...nel.org>, linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-serial@...r.kernel.org, devicetree@...r.kernel.org, psodagud@...cinc.com, 
	djaggi@...cinc.com, quic_msavaliy@...cinc.com, quic_vtanuku@...cinc.com, 
	quic_arandive@...cinc.com, quic_mnaresh@...cinc.com, quic_shazhuss@...cinc.com
Subject: Re: [PATCH v6 3/8] soc: qcom: geni-se: Enable QUPs on SA8255p
 Qualcomm platforms

On Fri, Jun 06, 2025 at 10:51:09PM +0530, Praveen Talari wrote:
> On the sa8255p platform, resources such as clocks,interconnects
> and TLMM (GPIO) configurations are managed by firmware.
> 
> Introduce a platform data function callback to distinguish whether
> resource control is performed by firmware or directly by the driver
> in linux.
> 
> The refactor ensures clear differentiation of resource
> management mechanisms, improving maintainability and flexibility
> in handling platform-specific configurations.
> 
> Signed-off-by: Praveen Talari <quic_ptalari@...cinc.com>
> ---
> v5 -> v6
> - replaced dev_err with dev_err_probe
> - added a check for desc->num_clks with MAX_CLKS, an error if
>   the specified num_clks in descriptor exceeds defined MAX_CLKS.
> - removed min_t which is not necessary.
> - renamed callback function names to resources_init.
> - resolved kernel bot warning error by documenting function
>   pointer in geni_se_desc structure.
> 
> v3 -> v4
> - declared an empty struct for sa8255p and added check as num clks.
> - Added version log after ---
> 
> v1 -> v2
> - changed datatype of i from int to unsigned int as per comment.
> ---
>  drivers/soc/qcom/qcom-geni-se.c | 77 +++++++++++++++++++++------------
>  1 file changed, 49 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 4cb959106efa..5c727b9a17e9 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -101,10 +101,13 @@ struct geni_wrapper {
>   * struct geni_se_desc - Data structure to represent the QUP Wrapper resources
>   * @clks:		Name of the primary & optional secondary AHB clocks
>   * @num_clks:		Count of clock names
> + * @resources_init:	Function pointer for initializing QUP Wrapper resources
>   */
>  struct geni_se_desc {
>  	unsigned int num_clks;
>  	const char * const *clks;
> +	int (*resources_init)(struct geni_wrapper *wrapper,
> +			      const struct geni_se_desc *desc);
>  };
>  
>  static const char * const icc_path_names[] = {"qup-core", "qup-config",
> @@ -891,10 +894,47 @@ int geni_icc_disable(struct geni_se *se)
>  }
>  EXPORT_SYMBOL_GPL(geni_icc_disable);
>  
> +static int geni_se_resource_init(struct geni_wrapper *wrapper,
> +				 const struct geni_se_desc *desc)
> +{
> +	struct device *dev = wrapper->dev;
> +	int ret;
> +	unsigned int i;
> +
> +	if (desc->num_clks > MAX_CLKS)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Too many clocks specified in descriptor:%u (max allowed: %u)\n",
> +				     desc->num_clks, MAX_CLKS);
> +
> +	wrapper->num_clks = desc->num_clks;
> +
> +	for (i = 0; i < wrapper->num_clks; ++i)
> +		wrapper->clks[i].id = desc->clks[i];
> +
> +	ret = of_count_phandle_with_args(dev->of_node, "clocks", "#clock-cells");
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "invalid clocks property at %pOF\n", dev->of_node);
> +
> +	if (ret < wrapper->num_clks) {
> +		dev_err(dev, "invalid clocks count at %pOF, expected %d entries\n",
> +			dev->of_node, wrapper->num_clks);
> +		return -EINVAL;
> +	}
> +
> +	ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks);
> +	if (ret) {
> +		dev_err(dev, "Err getting clks %d\n", ret);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
>  static int geni_se_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct geni_wrapper *wrapper;
> +	const struct geni_se_desc *desc;
>  	int ret;
>  
>  	wrapper = devm_kzalloc(dev, sizeof(*wrapper), GFP_KERNEL);
> @@ -906,36 +946,12 @@ static int geni_se_probe(struct platform_device *pdev)
>  	if (IS_ERR(wrapper->base))
>  		return PTR_ERR(wrapper->base);
>  
> -	if (!has_acpi_companion(&pdev->dev)) {
> -		const struct geni_se_desc *desc;
> -		int i;
> -
> -		desc = device_get_match_data(&pdev->dev);
> -		if (!desc)
> -			return -EINVAL;
> -
> -		wrapper->num_clks = min_t(unsigned int, desc->num_clks, MAX_CLKS);
> -
> -		for (i = 0; i < wrapper->num_clks; ++i)
> -			wrapper->clks[i].id = desc->clks[i];
> -
> -		ret = of_count_phandle_with_args(dev->of_node, "clocks", "#clock-cells");
> -		if (ret < 0) {
> -			dev_err(dev, "invalid clocks property at %pOF\n", dev->of_node);
> -			return ret;
> -		}
> +	desc = device_get_match_data(&pdev->dev);
>  
> -		if (ret < wrapper->num_clks) {
> -			dev_err(dev, "invalid clocks count at %pOF, expected %d entries\n",
> -				dev->of_node, wrapper->num_clks);
> +	if (!has_acpi_companion(&pdev->dev) && desc->num_clks) {

Reading this again, the only functional change I can spot is the
addition of this desc->num_clks check. (And the addition of the new
compatible)

The rest of the patch is just moving things out of this if statement
body, introducing the flexibility of a function pointer with a single
possible value etc.


As I've said before, function pointers are useful to create
abstractions, but they make it harder to follow the code (both for me
and the CPU) so you need to provide some value in return - and I'm
failing to see what that value is.

> +		ret = desc->resources_init(wrapper, desc);

In other words, you can replace this line with:

	ret = geni_se_resource_init();

Or, if this is all we get, then you can do nothing and just add the
additional expression to the condition and be done with it.

> +		if (ret)
>  			return -EINVAL;
> -		}
> -
> -		ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks);
> -		if (ret) {
> -			dev_err(dev, "Err getting clks %d\n", ret);
> -			return ret;
> -		}
>  	}
>  
>  	dev_set_drvdata(dev, wrapper);
> @@ -951,8 +967,11 @@ static const char * const qup_clks[] = {
>  static const struct geni_se_desc qup_desc = {
>  	.clks = qup_clks,
>  	.num_clks = ARRAY_SIZE(qup_clks),
> +	.resources_init = geni_se_resource_init,
>  };
>  
> +static const struct geni_se_desc sa8255p_qup_desc;

This looks like a forward declaration, it took me a while to realize
that this is giving you the actual all-zero geni_se_desc.

Add a = {}; to make it clear that this is where you declare the
variable.

Thanks,
Bjorn

> +
>  static const char * const i2c_master_hub_clks[] = {
>  	"s-ahb",
>  };
> @@ -960,11 +979,13 @@ static const char * const i2c_master_hub_clks[] = {
>  static const struct geni_se_desc i2c_master_hub_desc = {
>  	.clks = i2c_master_hub_clks,
>  	.num_clks = ARRAY_SIZE(i2c_master_hub_clks),
> +	.resources_init = geni_se_resource_init,
>  };
>  
>  static const struct of_device_id geni_se_dt_match[] = {
>  	{ .compatible = "qcom,geni-se-qup", .data = &qup_desc },
>  	{ .compatible = "qcom,geni-se-i2c-master-hub", .data = &i2c_master_hub_desc },
> +	{ .compatible = "qcom,sa8255p-geni-se-qup", .data = &sa8255p_qup_desc },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, geni_se_dt_match);
> -- 
> 2.17.1
> 

Powered by blists - more mailing lists