Re: [PATCH 2/6] firmware: tegra: bpmp: Move channel, resource init to helper

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5982 bytes --]

On Mon, Jun 15, 2026 at 08:37:27AM +0000, Aniruddha Rao wrote:
> Refactor the BPMP driver by moving channel initialization and Device Tree
> resource parsing into separate helper functions.
> 
> This prepares the driver for ACPI support, where these helpers will be
> skipped because channel initialization is handled by ACPI AML methods on
> ACPI-based systems.
> 
> Signed-off-by: Aniruddha Rao <anrao@nvidia.com>
> ---
>  drivers/firmware/tegra/bpmp.c | 94 +++++++++++++++++++++--------------
>  1 file changed, 57 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
> index 753472b53bd8..16ca0d104c1d 100644
> --- a/drivers/firmware/tegra/bpmp.c
> +++ b/drivers/firmware/tegra/bpmp.c
> @@ -733,19 +733,9 @@ void tegra_bpmp_handle_rx(struct tegra_bpmp *bpmp)
>  	spin_unlock(&bpmp->lock);
>  }
>  
> -static int tegra_bpmp_probe(struct platform_device *pdev)
> +static int tegra_bpmp_init_channels(struct tegra_bpmp *bpmp)
>  {
> -	struct tegra_bpmp *bpmp;
> -	char tag[TAG_SZ];
>  	size_t size;
> -	int err;
> -
> -	bpmp = devm_kzalloc(&pdev->dev, sizeof(*bpmp), GFP_KERNEL);
> -	if (!bpmp)
> -		return -ENOMEM;
> -
> -	bpmp->soc = of_device_get_match_data(&pdev->dev);
> -	bpmp->dev = &pdev->dev;
>  
>  	INIT_LIST_HEAD(&bpmp->mrqs);
>  	spin_lock_init(&bpmp->lock);
> @@ -755,37 +745,85 @@ static int tegra_bpmp_probe(struct platform_device *pdev)
>  
>  	size = BITS_TO_LONGS(bpmp->threaded.count) * sizeof(long);
>  
> -	bpmp->threaded.allocated = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
> +	bpmp->threaded.allocated = devm_kzalloc(bpmp->dev, size, GFP_KERNEL);
>  	if (!bpmp->threaded.allocated)
>  		return -ENOMEM;
>  
> -	bpmp->threaded.busy = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
> +	bpmp->threaded.busy = devm_kzalloc(bpmp->dev, size, GFP_KERNEL);
>  	if (!bpmp->threaded.busy)
>  		return -ENOMEM;
>  
>  	spin_lock_init(&bpmp->atomic_tx_lock);
> -	bpmp->tx_channel = devm_kzalloc(&pdev->dev, sizeof(*bpmp->tx_channel),
> +	bpmp->tx_channel = devm_kzalloc(bpmp->dev, sizeof(*bpmp->tx_channel),
>  					GFP_KERNEL);
>  	if (!bpmp->tx_channel)
>  		return -ENOMEM;
>  
> -	bpmp->rx_channel = devm_kzalloc(&pdev->dev, sizeof(*bpmp->rx_channel),
> +	bpmp->rx_channel = devm_kzalloc(bpmp->dev, sizeof(*bpmp->rx_channel),
>  	                                GFP_KERNEL);
>  	if (!bpmp->rx_channel)
>  		return -ENOMEM;
>  
> -	bpmp->threaded_channels = devm_kcalloc(&pdev->dev, bpmp->threaded.count,
> +	bpmp->threaded_channels = devm_kcalloc(bpmp->dev, bpmp->threaded.count,
>  					       sizeof(*bpmp->threaded_channels),
>  					       GFP_KERNEL);
>  	if (!bpmp->threaded_channels)
>  		return -ENOMEM;
>  
> -	platform_set_drvdata(pdev, bpmp);
> +	return 0;
> +}
> +
> +static int tegra_bpmp_init_resources(struct tegra_bpmp *bpmp)
> +{
> +	int err;
> +
> +	err = of_platform_default_populate(bpmp->dev->of_node, NULL, bpmp->dev);
> +	if (err < 0)
> +		return err;

This doesn't really belong in tegra_bpmp_init_resources(). Instead I
think we should keep this separate and wrap the calls to the OF-specific
functions in an OF conditional.

Come to think of it, looking at subsequent patches the OF conditional
seems to have been the prime motivator for moving this into a separate
function. And if we're moving of_platform_default_populate() out of this
there's very little reason left to do so. I think we should just keep
these calls in tegra_bpmp_probe() and wrap them in the conditional
instead. Should make the patch a bit easier to understand, too.

Thierry

> +
> +	if (of_property_present(bpmp->dev->of_node, "#clock-cells")) {
> +		err = tegra_bpmp_init_clocks(bpmp);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	if (of_property_present(bpmp->dev->of_node, "#reset-cells")) {
> +		err = tegra_bpmp_init_resets(bpmp);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	if (of_property_present(bpmp->dev->of_node, "#power-domain-cells"))
> +		err = tegra_bpmp_init_powergates(bpmp);
> +
> +	return err;
> +}
> +
> +static int tegra_bpmp_probe(struct platform_device *pdev)
> +{
> +	struct tegra_bpmp *bpmp;
> +	char tag[TAG_SZ];
> +	int err;
>  
> -	err = bpmp->soc->ops->init(bpmp);
> +	bpmp = devm_kzalloc(&pdev->dev, sizeof(*bpmp), GFP_KERNEL);
> +	if (!bpmp)
> +		return -ENOMEM;
> +
> +	bpmp->soc = device_get_match_data(&pdev->dev);
> +	bpmp->dev = &pdev->dev;
> +
> +	err = tegra_bpmp_init_channels(bpmp);
>  	if (err < 0)
>  		return err;
>  
> +	platform_set_drvdata(pdev, bpmp);
> +
> +	if (bpmp->soc->ops->init) {

Maybe add in a check for bpmp->soc->ops in here, so that we don't have
to assign an empty ops structure for ACPI if it isn't needed.

> +		err = bpmp->soc->ops->init(bpmp);
> +		if (err < 0)
> +			return err;
> +	}
> +
>  	err = tegra_bpmp_request_mrq(bpmp, MRQ_PING,
>  				     tegra_bpmp_mrq_handle_ping, bpmp);
>  	if (err < 0)
> @@ -805,28 +843,10 @@ static int tegra_bpmp_probe(struct platform_device *pdev)
>  
>  	dev_info(&pdev->dev, "firmware: %.*s\n", (int)sizeof(tag), tag);
>  
> -	err = of_platform_default_populate(pdev->dev.of_node, NULL, &pdev->dev);
> +	err = tegra_bpmp_init_resources(bpmp);
>  	if (err < 0)
>  		goto free_mrq;
>  
> -	if (of_property_present(pdev->dev.of_node, "#clock-cells")) {
> -		err = tegra_bpmp_init_clocks(bpmp);
> -		if (err < 0)
> -			goto free_mrq;
> -	}
> -
> -	if (of_property_present(pdev->dev.of_node, "#reset-cells")) {
> -		err = tegra_bpmp_init_resets(bpmp);
> -		if (err < 0)
> -			goto free_mrq;
> -	}
> -
> -	if (of_property_present(pdev->dev.of_node, "#power-domain-cells")) {
> -		err = tegra_bpmp_init_powergates(bpmp);
> -		if (err < 0)
> -			goto free_mrq;
> -	}
> -
>  	err = tegra_bpmp_init_debugfs(bpmp);
>  	if (err < 0)
>  		dev_err(&pdev->dev, "debugfs initialization failed: %d\n", err);
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]