Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
Cc: linux-remoteproc@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	spjoshi@codeaurora.org, kaushalk@codeaurora.org
Subject: Re: [PATCH 5/5] remoteproc: Modifying probe for initializing q6v55 specific resources
Date: Tue, 25 Oct 2016 12:35:54 -0700
Message-ID: <20161025193554.GQ7509@tuxbot> (raw)
In-Reply-To: <1477324559-24752-6-git-send-email-akdwived@codeaurora.org>

On Mon 24 Oct 08:55 PDT 2016, Avaneesh Kumar Dwivedi wrote:

> Probe is being modified to save q6 version and invoke appropriate
> init functions to accommodate q6v55 remoteproc driver.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 43 ++++++++++++++++++++++++++++++--------
>  1 file changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index dd19d41..c65c904 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -1370,6 +1370,9 @@ static int q6v5_probe(struct platform_device *pdev)
>  	init_completion(&qproc->start_done);
>  	init_completion(&qproc->stop_done);
>  
> +	if (of_device_is_compatible(pdev->dev.of_node, "qcom,q6v55-pil"))
> +		qproc->is_q6v55 = true;
> +

With the changes I've suggested in the other patches I would recommend
that you describe the differences as a set of "features" in a struct
that you put as .data in the match table and then use
of_device_get_match_data() to acquire the matching data struct.

>  	ret = q6v5_init_mem(qproc, pdev);
>  	if (ret)
>  		goto free_rproc;
> @@ -1378,17 +1381,39 @@ static int q6v5_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto free_rproc;
>  
> -	ret = q6v5_init_clocks(qproc);
> -	if (ret)
> -		goto free_rproc;
> +	if (qproc->is_q6v55) {
> +		ret = q6v55_init_clocks(qproc);
> +		if (ret)
> +			goto free_rproc;
> +	} else {
> +		ret = q6v5_init_clocks(qproc);
> +		if (ret)
> +			goto free_rproc;
> +	}

Based on the fact that I don't think this is a q6v55, but rather a q6v6,
we will now end up with:

if (is_q6v55) {
} else if (is_q6v6) {
} else if (is_q6v5) {
} else {
	fail
};

And this function will turn into an (even worse) mess.


I would suggest that you instead define each resource as a flag and
provide a struct with these flags as .data with the compatible. Then
pass that to the clock and regulator init and based on the flags they
can acquire the individual resources. That way adding a new version is a
matter of listing which resources that needs to grab.

And in that struct you can also have a function pointer to an
appropriate reset function, completely removing the need for checking
which version we have after initialization.

>  
> -	ret = q6v5_regulator_init(qproc);
> -	if (ret)
> -		goto free_rproc;
> +	if (qproc->is_q6v55) {
> +		ret = q6v55_init_reset(qproc, pdev);
> +		if (ret)
> +			goto free_rproc;
> +	} else {
> +		ret = q6v5_init_reset(qproc);
> +		if (ret)
> +			goto free_rproc;
> +	}
>  
> -	ret = q6v5_init_reset(qproc);
> -	if (ret)
> -		goto free_rproc;
> +	if (qproc->is_q6v55) {
> +		ret = q6v55_regulator_init(qproc);
> +		if (ret)
> +			goto free_rproc;
> +	} else {
> +		ret = q6v5_regulator_init(qproc);
> +		if (ret)
> +			goto free_rproc;
> +	}
> +
> +	qproc->ahb_clk_vote = of_property_read_bool(pdev->dev.of_node,
> +			"qcom,ahb-clk-vote");
> +	mutex_init(&qproc->q6_lock);
>  
>  	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
>  	if (ret < 0)

Regards,
Bjorn

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-10-24 15:55 [PATCH 0/5] Self authenticating hexagon driver for q6v55 Avaneesh Kumar Dwivedi
2016-10-24 15:55 ` [PATCH 1/5] remoteproc: Add q6v55 specific parameters and enable probing " Avaneesh Kumar Dwivedi
2016-10-25 18:47   ` Bjorn Andersson
2016-11-04 13:27     ` Avaneesh Kumar Dwivedi
2016-11-08  5:28       ` Bjorn Andersson
2016-10-24 15:55 ` [PATCH 2/5] remoteproc: Adding q6v55 specific regulator, clk, reset interface Avaneesh Kumar Dwivedi
2016-10-25 19:05   ` Bjorn Andersson
2016-11-04 13:41     ` Avaneesh Kumar Dwivedi
2016-10-24 15:55 ` [PATCH 3/5] remoteproc: Adding reset sequence and halt seq changes for q6v55 Avaneesh Kumar Dwivedi
2016-10-25 19:15   ` Bjorn Andersson
2016-11-04 13:42     ` Avaneesh Kumar Dwivedi
2016-10-24 15:55 ` [PATCH 4/5] remoteproc: Add start and shutdown interface " Avaneesh Kumar Dwivedi
2016-10-25 19:27   ` Bjorn Andersson
2016-11-04 13:46     ` Avaneesh Kumar Dwivedi
2016-10-24 15:55 ` [PATCH 5/5] remoteproc: Modifying probe for initializing q6v55 specific resources Avaneesh Kumar Dwivedi
2016-10-25 19:35   ` Bjorn Andersson [this message]
2016-11-04 13:47     ` Avaneesh Kumar Dwivedi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161025193554.GQ7509@tuxbot \
    --to=bjorn.andersson@linaro.org \
    --cc=akdwived@codeaurora.org \
    --cc=kaushalk@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=spjoshi@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git