<pre>
Dear Matthias,

Thanks for the review.

On Thu, 2022-11-10 at 14:12 +0100, Matthias Brugger wrote:
>
> On 08/11/2022 20:43, Nícolas F. R. A. Prado wrote:
> > On Tue, Nov 08, 2022 at 06:37:19PM +0100, Matthias Brugger wrote:
> > >
> > >
> > > On 07/11/2022 08:22, Nancy.Lin wrote:
> > > > Simplify code for update mmsys reg.
> > > >
> > > > Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> > > > Reviewed-by: AngeloGioacchino Del Regno <
> > > > angelogioacchino.delregno@collabora.com>
> > > > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > > > Tested-by: AngeloGioacchino Del Regno <
> > > > angelogioacchino.delregno@collabora.com>
> > > > Tested-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
> > > > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > > > ---
> > > > drivers/soc/mediatek/mtk-mmsys.c | 45 ++++++++++++--------
> > > > ------------
> > > > 1 file changed, 16 insertions(+), 29 deletions(-)
> > > >
> > > > diff --git a/drivers/soc/mediatek/mtk-mmsys.c
> > > > b/drivers/soc/mediatek/mtk-mmsys.c
> > > > index 9a327eb5d9d7..73c8bd27e6ae 100644
> > > > --- a/drivers/soc/mediatek/mtk-mmsys.c
> > > > +++ b/drivers/soc/mediatek/mtk-mmsys.c
> > > > @@ -99,22 +99,27 @@ struct mtk_mmsys {
> > > > struct reset_controller_dev rcdev;
> > > > };
> > > > +static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32
> > > > offset, u32 mask, u32 val)
> > > > +{
> > > > +u32 tmp;
> > > > +
> > > > +tmp = readl_relaxed(mmsys->regs + offset);
> > > > +tmp = (tmp & ~mask) | (val & mask);
> > >
> > > I'm not sure about the change in the implementation of
> > > mtk_mmsys_update_bits(). Nicolas tried to explain it to me on IRC
> > > but I
> > > wasn't totally convincing. As we have to go for at least another
> > > round of
> > > this patches, I'd like to get a clear understanding while it is
> > > needed that
> > > val bits are set to 1 in the mask.
> >
> > The point here was to make sure that mtk_mmsys_update_bits() didn't
> > allow
> > setting bits outside of the mask, since that's never what you want:
> > the entire
> > point of having a mask is to specify the bits that should be
> > updated (and the
> > ones that should be kept unchanged). So for example if you had
> >
> > mask = 0x0ff0
> > val = 0x00ff
> >
> > the previous implementation would happily overwrite the 4 least
> > significant bits
> > on the destination register, despite them not being present in the
> > mask, which
> > is wrong.
> >
> > This wrong behavior could easily lead to hard to trace bugs as soon
> > as a badly
> > formatted/wrong val is passed and an unrelated bit updated due to
> > the mask being
> > ignored.
> >
> > For reference, _regmap_update_bits() does the same masking of the
> > value [1].
> >
> > That said, given that this function already existed and was just
> > being moved,
> > it would've been cleaner to make this change in a separate commit.
> >
>
> Would have been better, but we can leave it as it.
>
> Regards,
> Matthias
>

Do you mean to keep original one (1), or keep this (2) ?

1. tmp = (tmp & ~mask) | val;
2. tmp = (tmp & ~mask) | (val & mask);


Regards,
Nancy

> > [1]
> > https://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap.c*L3122__;Iw!!CTRNKA9wMg0ARbw!xSv5xbY6cv-Mg-1xDGOf3oVZ93uyrcv4tt87DKlx5emjmwufjf2DjION7GiNAaJB$
> >
> >
> > Thanks,
> > Nícolas
> >
> > >
> > > Regards,
> > > Matthias
> > >
> > > > +writel_relaxed(tmp, mmsys->regs + offset);
> > > > +}
> >
> > [..]
> > > > -static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32
> > > > offset, u32 mask, u32 val)
> > > > -{
> > > > -u32 tmp;
> > > > -
> > > > -tmp = readl_relaxed(mmsys->regs + offset);
> > > > -tmp = (tmp & ~mask) | val;
> > > > -writel_relaxed(tmp, mmsys->regs + offset);
> > > > -}
> > > > -
> >
> > [..]
>
>

</pre><!--type:text--><!--{--><pre>************* MEDIATEK Confidentiality Notice ********************
The information contained in this e-mail message (including any 
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be 
conveyed only to the designated recipient(s). Any use, dissemination, 
distribution, printing, retaining or copying of this e-mail (including its 
attachments) by unintended recipient(s) is strictly prohibited and may 
be unlawful. If you are not an intended recipient of this e-mail, or believe 
that you have received this e-mail in error, please notify the sender 
immediately (by replying to this e-mail), delete any and all copies of 
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
</pre><!--}-->