On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:
> Certain regulators and clocks need voting by rproc on behalf of hexagon
> only during restart operation but certain clocks and voltage need to be
> voted till hexagon is up, these regulators and clocks are identified as
> proxy and active resource whose handle is being obtained by supplying
> private proxy and active regulator and clock string.
>
Please split this patch out into the regulator and clock patches
respectively, making each a clear functional change.
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@xxxxxxxxxxxxxx>
> ---
> drivers/remoteproc/qcom_q6v5_pil.c | 148 +++++++++++++++++++++++++++----------
> 1 file changed, 107 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 3360312..b0f0fcf 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -37,7 +37,6 @@
>
> #include <linux/qcom_scm.h>
>
> -#define MBA_FIRMWARE_NAME "mba.b00"
Please move this to patch 1.
> #define MPSS_FIRMWARE_NAME "modem.mdt"
>
> #define MPSS_CRASH_REASON_SMEM 421
> @@ -132,6 +131,14 @@ struct q6v5 {
> struct clk *ahb_clk;
> struct clk *axi_clk;
> struct clk *rom_clk;
> + struct clk *active_clks[8];
> + struct clk *proxy_clks[4];
> + struct reg_info active_regs[1];
> + struct reg_info proxy_regs[3];
> + int active_reg_count;
> + int proxy_reg_count;
> + int active_clk_count;
> + int proxy_clk_count;
Group clocks members together and regulators together.
>
> struct completion start_done;
> struct completion stop_done;
> @@ -160,27 +167,43 @@ enum {
> Q6V5_SUPPLY_PLL,
> };
>
> -static int q6v5_regulator_init(struct q6v5 *qproc)
> +static int q6v5_regulator_init(struct device *dev,
> + struct reg_info *regs, char **reg_str, int volatage_load[][2])
> {
> - int ret;
> + int reg_count = 0, i;
Please, one variable per line, sorted (descending) by line length.
>
> - qproc->supply[Q6V5_SUPPLY_CX].supply = "cx";
> - qproc->supply[Q6V5_SUPPLY_MX].supply = "mx";
> - qproc->supply[Q6V5_SUPPLY_MSS].supply = "mss";
> - qproc->supply[Q6V5_SUPPLY_PLL].supply = "pll";
> + if (!reg_str)
> + return 0;
>
> - ret = devm_regulator_bulk_get(qproc->dev,
> - ARRAY_SIZE(qproc->supply), qproc->supply);
> - if (ret < 0) {
> - dev_err(qproc->dev, "failed to get supplies\n");
> - return ret;
> - }
> + while (reg_str[reg_count] != NULL)
Drop "!= NULL" from your condition.
> + reg_count++;
>
> - regulator_set_load(qproc->supply[Q6V5_SUPPLY_CX].consumer, 100000);
> - regulator_set_load(qproc->supply[Q6V5_SUPPLY_MSS].consumer, 100000);
> - regulator_set_load(qproc->supply[Q6V5_SUPPLY_PLL].consumer, 10000);
> + if (!reg_count)
> + return reg_count;
You can omit this check, as the for loop below will iterate 0 times and
we will then return 0.
>
> - return 0;
> + if (!regs)
> + return -ENOMEM;
This will not happen, please drop.
> +
> + for (i = 0; i < reg_count; i++) {
> + const char *reg_name;
Please keep local variables at the top of the function.
And generally try to use short but clear variable names; in line with my
suggestion in patch 1 name it "supply" (or in this case where the
original variable is quite short just use it directly).
> +
> + reg_name = reg_str[i];
> + regs[i].reg = devm_regulator_get(dev, reg_name);
> + if (IS_ERR(regs[i].reg)) {
> +
> + int rc = PTR_ERR(regs[i].reg);
> +
> + if (rc != -EPROBE_DEFER)
> + dev_err(dev, "Failed to get %s\n regulator",
> + reg_name);
> + return rc;
> + }
> +
> + regs[i].uV = volatage_load[i][0];
> + regs[i].uA = volatage_load[i][1];
> + }
> +
> + return reg_count;
> }
>
> static int q6v5_regulator_enable(struct q6v5 *qproc)
> @@ -725,27 +748,41 @@ static int q6v5_init_mem(struct q6v5 *qproc, struct platform_device *pdev)
> return 0;
> }
>
> -static int q6v5_init_clocks(struct q6v5 *qproc)
> +static int q6v5_init_clocks(struct device *dev, struct clk **clks,
> + char **clk_str)
> {
> - qproc->ahb_clk = devm_clk_get(qproc->dev, "iface");
> - if (IS_ERR(qproc->ahb_clk)) {
> - dev_err(qproc->dev, "failed to get iface clock\n");
> - return PTR_ERR(qproc->ahb_clk);
> - }
> + int clk_count = 0, i;
One variable per line, please. And "count" is unambiguous enough.
>
> - qproc->axi_clk = devm_clk_get(qproc->dev, "bus");
> - if (IS_ERR(qproc->axi_clk)) {
> - dev_err(qproc->dev, "failed to get bus clock\n");
> - return PTR_ERR(qproc->axi_clk);
> - }
> + if (!clk_str)
> + return 0;
> +
> + while (clk_str[clk_count] != NULL)
Drop "!= NULL" part
> + clk_count++;
> +
> + if (!clk_count)
> + return clk_count;
This is not needed.
> +
> + if (!clks)
> + return -ENOMEM;
This can't happen.
> +
> + for (i = 0; i < clk_count; i++) {
> + const char *clock_name;
> +
> + clock_name = clk_str[i];
> + clks[i] = devm_clk_get(dev, clock_name);
> + if (IS_ERR(clks[i])) {
> +
> + int rc = PTR_ERR(clks[i]);
> +
> + if (rc != -EPROBE_DEFER)
> + dev_err(dev, "Failed to get %s clock\n",
> + clock_name);
> + return rc;
> + }
>
> - qproc->rom_clk = devm_clk_get(qproc->dev, "mem");
> - if (IS_ERR(qproc->rom_clk)) {
> - dev_err(qproc->dev, "failed to get mem clock\n");
> - return PTR_ERR(qproc->rom_clk);
> }
>
> - return 0;
> + return clk_count;
> }
>
> static int q6v5_init_reset(struct q6v5 *qproc)
> @@ -830,10 +867,15 @@ static int q6v5_probe(struct platform_device *pdev)
> {
> struct q6v5 *qproc;
> struct rproc *rproc;
> - int ret;
> + const struct rproc_hexagon_res *desc;
> + int ret, count;
One variable per line please, and sort (descending) on line length.
> +
> + desc = of_device_get_match_data(&pdev->dev);
> + if (!desc)
> + return -EINVAL;
>
> rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops,
> - MBA_FIRMWARE_NAME, sizeof(*qproc));
> + desc->hexagon_mba_image, sizeof(*qproc));
Please move this to patch 1.
> if (!rproc) {
> dev_err(&pdev->dev, "failed to allocate rproc\n");
> return -ENOMEM;
> @@ -857,18 +899,42 @@ static int q6v5_probe(struct platform_device *pdev)
> if (ret)
> goto free_rproc;
>
> - if (ret)
> - goto free_rproc;
> + count = q6v5_init_clocks(&pdev->dev, qproc->proxy_clks,
> + desc->proxy_clk_string);
Your "count" serves the same purpose as "ret", so just use "ret".
And align broken lines with the parenthesis.
> + if (count < 0) {
> + dev_err(&pdev->dev, "Failed to setup proxy clocks.\n");
> + return count;
And using "ret" instead of "count" makes it easy to goto free_rproc,
which you must do after calling rproc_alloc().
> + }
> + qproc->proxy_clk_count = count;
>
> - ret = q6v5_regulator_init(qproc);
> - if (ret)
> - goto free_rproc;
> + count = q6v5_init_clocks(&pdev->dev, qproc->active_clks,
> + desc->active_clk_string);
> + if (count < 0) {
> + dev_err(&pdev->dev, "Failed to setup active clocks.\n");
> + return count;
> + }
> + qproc->active_clk_count = count;
>
> ret = q6v5_init_reset(qproc);
> if (ret)
> goto free_rproc;
>
> + count = q6v5_regulator_init(&pdev->dev, qproc->proxy_regs,
> + desc->proxy_reg_string, (int (*)[2])desc->proxy_uV_uA);
Moving proxy_reg_string and proxy_uV_uA into a struct (as suggested in
patch 1) will make this cleaner and we get rid of that typecast.
> + if (count < 0) {
> + dev_err(&pdev->dev, "Failed to setup active regulators.\n");
> + return count;
> + }
> + qproc->proxy_reg_count = count;
> +
> + count = q6v5_regulator_init(&pdev->dev, qproc->active_regs,
> + desc->active_reg_string, (int (*)[2])desc->active_uV_uA);
> + if (count < 0) {
> + dev_err(&pdev->dev, "Failed to setup proxy regulators.\n");
> + return count;
> + }
> + qproc->active_reg_count = count;
> +
> ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
> if (ret < 0)
> goto free_rproc;
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
