Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <1680076012-10785-1-git-send-email-quic_mojha@quicinc.com>
 <1680076012-10785-5-git-send-email-quic_mojha@quicinc.com>
 <ZHEt2mrYpSMKBuIX@surfacebook> <6d9f251e-2c1a-ed50-638e-a052404ffc64@quicinc.com>
In-Reply-To: <6d9f251e-2c1a-ed50-638e-a052404ffc64@quicinc.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Mon, 26 Jun 2023 20:12:52 +0300
Message-ID: <CAHp75VdU+ee8_UveZp3SD2UKYYg4Jm0cYUpcXfVwMEVC1d0Bdw@mail.gmail.com>
Subject: Re: [PATCH v6 4/5] firmware: qcom_scm: Refactor code to support
 multiple download mode
To:     Mukesh Ojha <quic_mojha@quicinc.com>
Cc:     agross@kernel.org, andersson@kernel.org, konrad.dybcio@linaro.org,
        linus.walleij@linaro.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jun 26, 2023 at 7:28=E2=80=AFPM Mukesh Ojha <quic_mojha@quicinc.com=
> wrote:
> On 5/27/2023 3:38 AM, andy.shevchenko@gmail.com wrote:
> > Wed, Mar 29, 2023 at 01:16:51PM +0530, Mukesh Ojha kirjoitti:

...

> >>              ret =3D qcom_scm_io_update_field(__scm->dload_mode_addr,
> >> -                            QCOM_DOWNLOAD_MODE_MASK,
> >> -                            enable ? QCOM_DOWNLOAD_FULLDUMP : 0);
> >> +                            QCOM_DOWNLOAD_MODE_MASK, download_mode);
> >
> > Can ping-pong style be avoided? I.e. do the right thing in the previous=
 patch,
> > so you won't change lines that were introduced just before.
>
> If you notice, I have just converted download mode data type from bool
> to int in this patch and hence the changing the line here. Last patch
> was about just using the exported API, so i hope you would be fine here.

Thank you for elaboration. I'm fine with that.

...

> >  > Also, what about download_mode that doesn't fit to the above two?
>
> return sysfs_emit(buffer, "unknown\n"); ?

For example, yes.

...

> >> +static int set_download_mode(const char *val, const struct kernel_par=
am *kp)
> >> +{
> >> +    u32 old =3D download_mode;
> >> +
> >> +    if (sysfs_streq(val, "full")) {
> >> +            download_mode =3D QCOM_DOWNLOAD_FULLDUMP;
> >> +    } else if (sysfs_streq(val, "off")) {
> >> +            download_mode =3D QCOM_DOWNLOAD_NODUMP;
> >
> > NIH sysfs_match_string().
>
> NIH ?

Not Invented Here

> My apology, if i did not get this..
> Do you want me to use sysfs_match_string()

Yes.

> and how would that help compare to what is present now ?

This will make your ABi gathered in one place (all strings and all
values) and less code will be used esp. if it's going to be expanded
in the future (isn't it in the next patches?).


> >> +    }
> >> +
> >> +    if (__scm)
> >> +            qcom_scm_set_download_mode(download_mode);
> >> +
> >> +    return 0;
> >> +}

...

> > Have you updated corresponding documentation about this parameter?
> > Or there is none?
>
> There is none as of yet outside this file; should that be good what i
> have added in 5/5..

It was a long time ago when I reviewed this. Just a note that all ABI
must be documented, debugfs is highly recommended to be documented.

--=20
With Best Regards,
Andy Shevchenko