On Sat, 2021-12-04 at 12:48 +0100, Krzysztof Kozlowski wrote:
> On 03/12/2021 07:40, Yong Wu wrote:
> > sleep control means that when the larb go to sleep, we should wait
> > a bit
>
> s/go/goes/
>
> > until all the current commands are finished. thus, when the larb
> > runtime
>
> Please start every sentence with a capital letter.
>
> > suspend, we need enable this function to wait until all the existed
>
> s/suspend/suspends/
> s/we need enable/we need to enable/
>
> > command are finished. when the larb resume, just disable this
> > function.
>
> s/command/commands/
> s/resume/resumes/
>
> > This function only improve the safe of bus. Add a new flag for this
>
> s/improve/improves/
> s/the safe/the safety/
>
> > function. Prepare for mt8186.
>
> In total it is hard to parse, really.
Will fix them in next version.
Thanks for reviewing so detailedly. Sorry. I didn't pay attention to
the grammar before.
>
> >
> > Signed-off-by: Anan Sun <anan.sun@xxxxxxxxxxxx>
> > Signed-off-by: Yong Wu <yong.wu@xxxxxxxxxxxx>
> > ---
> > drivers/memory/mtk-smi.c | 39 +++++++++++++++++++++++++++++++++++-
> > ---
> > 1 file changed, 35 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> > index b883dcc0bbfa..4b59b28e4d73 100644
> > --- a/drivers/memory/mtk-smi.c
> > +++ b/drivers/memory/mtk-smi.c
> > @@ -8,6 +8,7 @@
> > #include <linux/device.h>
> > #include <linux/err.h>
> > #include <linux/io.h>
> > +#include <linux/iopoll.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/of_platform.h>
> > @@ -32,6 +33,10 @@
> > #define SMI_DUMMY 0x444
> >
> > /* SMI LARB */
> > +#define SMI_LARB_SLP_CON 0x00c
> > +#define SLP_PROT_EN BIT(0)
> > +#define SLP_PROT_RDY BIT(16)
> > +
> > #define SMI_LARB_CMD_THRT_CON 0x24
> > #define SMI_LARB_THRT_RD_NU_LMT_MSK GENMASK(7, 4)
> > #define SMI_LARB_THRT_RD_NU_LMT (5 << 4)
> > @@ -81,6 +86,7 @@
> >
> > #define MTK_SMI_FLAG_THRT_UPDATE BIT(0)
> > #define MTK_SMI_FLAG_SW_FLAG BIT(1)
> > +#define MTK_SMI_FLAG_SLEEP_CTL BIT(2)
> > #define MTK_SMI_CAPS(flags, _x) (!!((flags) & (_x)))
> >
> > struct mtk_smi_reg_pair {
> > @@ -371,6 +377,24 @@ static const struct of_device_id
> > mtk_smi_larb_of_ids[] = {
> > {}
> > };
> >
> > +static int mtk_smi_larb_sleep_ctrl(struct device *dev, bool
> > to_sleep)
> > +{
>
> Make two functions instead. There is no single code reuse (shared)
> between sleep and resume. In the same time bool arguments are
> confusing
> when looking at caller and one never knows whether true means to
> resume
> or to sleep. Having two functions is obvious. Obvious code is easier
> to
> read and maintain.
Make sense. Thanks for this suggestion.
>
> > + struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> > + int ret = 0;
> > + u32 tmp;
> > +
> > + if (to_sleep) {
> > + writel_relaxed(SLP_PROT_EN, larb->base +
> > SMI_LARB_SLP_CON);
> > + ret = readl_poll_timeout_atomic(larb->base +
> > SMI_LARB_SLP_CON,
> > + tmp, !!(tmp &
> > SLP_PROT_RDY), 10, 1000);
> > + if (ret)
> > + dev_warn(dev, "sleep ctrl is not
> > ready(0x%x).\n", tmp);
> > + } else {
> > + writel_relaxed(0, larb->base + SMI_LARB_SLP_CON);
> > + }
> > + return ret;
> > +}
> > +
> > static int mtk_smi_device_link_common(struct device *dev, struct
> > device **com_dev)
> > {
> > struct platform_device *smi_com_pdev;
> > @@ -477,24 +501,31 @@ static int __maybe_unused
> > mtk_smi_larb_resume(struct device *dev)
> > {
> > struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> > const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen;
> > - int ret;
> > + int ret = 0;
>
> This line does not have a sense.
Yes. This is unhelpful. Will remove this.
>
> >
> > ret = clk_bulk_prepare_enable(larb->smi.clk_num, larb-
> > >smi.clks);
> > - if (ret < 0)
> > + if (ret)
>
> Why changing this?
The successful return value should be 0. I will use a independent patch
for this.
>
> Best regards,
> Krzysztof
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
