lkml.org 
[lkml]   [2024]   [Feb]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2 20/20] media: venus: pm_helpers: Use reset_bulk API
    From
    Date
    Hi Konrad,

    On Fr, 2024-02-09 at 22:10 +0100, Konrad Dybcio wrote:
    > All of the resets are toggled together. Use the bulk api to save on some
    > code complexity.
    >
    > The delay between resets is now correctly determined by the reset
    > framework.

    If this is a recent change, could you reference the commit?

    > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
    > ---
    > drivers/media/platform/qcom/venus/core.c | 15 ++++++++++-----
    > drivers/media/platform/qcom/venus/core.h | 4 ++--
    > drivers/media/platform/qcom/venus/pm_helpers.c | 15 +++------------
    > 3 files changed, 15 insertions(+), 19 deletions(-)
    >
    > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
    > index 873affe17537..ff5601a5ce77 100644
    > --- a/drivers/media/platform/qcom/venus/core.c
    > +++ b/drivers/media/platform/qcom/venus/core.c
    > @@ -328,11 +328,16 @@ static int venus_probe(struct platform_device *pdev)
    > if (ret)
    > return ret;
    >
    > - for (i = 0; i < res->resets_num; i++) {
    > - core->resets[i] = devm_reset_control_get_exclusive(dev, res->resets[i]);
    > - if (IS_ERR(core->resets[i]))
    > - return PTR_ERR(core->resets[i]);
    > - }
    > + core->resets = devm_kcalloc(dev, res->resets_num, sizeof(*core->resets), GFP_KERNEL);

    Since VIDC_RESETS_NUM_MAX is only 2, I don't think a separate
    allocation is worth it.

    > + if (res->resets_num && !core->resets)
    > + return -ENOMEM;
    > +
    > + for (i = 0; i < res->resets_num; i++)
    > + core->resets[i].id = res->resets[i];
    > +
    > + ret = devm_reset_control_bulk_get_exclusive(dev, res->resets_num, core->resets);
    > + if (ret)
    > + return dev_err_probe(dev, ret, "Failed to get resets\n");
    >
    > ret = venus_get_resources(core);
    > if (ret)
    > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
    > index 6ecaa3e38cac..2376b9cbdf2c 100644
    > --- a/drivers/media/platform/qcom/venus/core.h
    > +++ b/drivers/media/platform/qcom/venus/core.h
    > @@ -130,7 +130,7 @@ struct venus_format {
    > * @pmdomains: a pointer to a list of pmdomains
    > * @opp_dl_venus: an device-link for device OPP
    > * @opp_pmdomain: an OPP power-domain
    > - * @resets: an array of reset signals
    > + * @resets: a reset_control_bulk_data array of hardware reset signals
    > * @vdev_dec: a reference to video device structure for decoder instances
    > * @vdev_enc: a reference to video device structure for encoder instances
    > * @v4l2_dev: a holder for v4l2 device structure
    > @@ -183,7 +183,7 @@ struct venus_core {
    > struct dev_pm_domain_list *pmdomains;
    > struct device_link *opp_dl_venus;
    > struct device *opp_pmdomain;
    > - struct reset_control *resets[VIDC_RESETS_NUM_MAX];
    > + struct reset_control_bulk_data *resets;

    Any reason not to just keep this as an array[VIDC_RESETS_NUM_MAX]?

    > struct video_device *vdev_dec;
    > struct video_device *vdev_enc;
    > struct v4l2_device v4l2_dev;
    > diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
    > index 9df8f2292c17..170fb131cb1e 100644
    > --- a/drivers/media/platform/qcom/venus/pm_helpers.c
    > +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
    > @@ -865,21 +865,12 @@ void vcodec_domains_put(struct venus_core *core)
    > static int core_resets_reset(struct venus_core *core)
    > {
    > const struct venus_resources *res = core->res;
    > - unsigned int i;
    > int ret;
    >
    > - for (i = 0; i < res->resets_num; i++) {
    > - ret = reset_control_assert(core->resets[i]);
    > - if (ret)
    > - goto err;
    > -
    > - usleep_range(150, 250);
    > - ret = reset_control_deassert(core->resets[i]);
    > - if (ret)
    > - goto err;
    > - }
    > + ret = reset_control_bulk_reset(res->resets_num, core->resets);
    > + if (ret)
    > + dev_err(core->dev, "Failed to toggle resets: %d\n", ret);
    >
    > -err:
    > return ret;

    Could be simplified to:

    return reset_control_bulk_reset(res->resets_num, core-
    >resets);

    regards
    Philipp

    \
     
     \ /
      Last update: 2024-05-27 15:02    [W:4.190 / U:0.136 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site