Hi Weijie,
On Sun, 7 Aug 2022 at 20:36, Weijie Gao <weijie....@mediatek.com> wrote:
>
> On Thu, 2022-08-04 at 07:56 -0600, Simon Glass wrote:
> > Hi Weijie,
> >
> > On Wed, 3 Aug 2022 at 21:36, Weijie Gao <weijie....@mediatek.com>
> > wrote:
> > >
> > > The baud clock on some platform may change due to assigned-clock-
> > > parent
> > > set in DT. In current flow the baud clock is only retrieved during
> > > probe
> > > stage. If the parent of the source clock changes after probe stage,
> > > the
> > > setbrg will set wrong baudrate.
> > >
> > > To get the right clock rate, this patch records the baud clk struct
> > > to the
> > > driver's priv, and changes the driver's flow to get the clock rate
> > > before
> > > calling _mtk_serial_setbrg().
> > >
> > > Signed-off-by: Weijie Gao <weijie....@mediatek.com>
> > > ---
> > > drivers/serial/serial_mtk.c | 72 ++++++++++++++++++++-------------
> > > ----
> > > 1 file changed, 39 insertions(+), 33 deletions(-)
> >
> > Reviewed-by: Simon Glass <s...@chromium.org>
> >
> > please see below
> >
> > >
> > > diff --git a/drivers/serial/serial_mtk.c
> > > b/drivers/serial/serial_mtk.c
> > > index a84f39b3fa..99cf62b8d9 100644
> > > --- a/drivers/serial/serial_mtk.c
> > > +++ b/drivers/serial/serial_mtk.c
> > > @@ -10,6 +10,7 @@
> > > #include <common.h>
> > > #include <div64.h>
> > > #include <dm.h>
> > > +#include <dm/device_compat.h>
> > > #include <errno.h>
> > > #include <log.h>
> > > #include <serial.h>
> > > @@ -72,25 +73,27 @@ struct mtk_serial_regs {
> > >
> > > struct mtk_serial_priv {
> >
> > please add a full comment for this struct
>
> OK.
>
> >
> > > struct mtk_serial_regs __iomem *regs;
> > > - u32 clock;
> > > + struct clk clk;
> > > + u32 fixed_clk_rate;
> > > bool force_highspeed;
> > > };
> > >
> > > -static void _mtk_serial_setbrg(struct mtk_serial_priv *priv, int
> > > baud)
> > > +static void _mtk_serial_setbrg(struct mtk_serial_priv *priv, int
> > > baud,
> > > + u32 clk_rate)
> >
> > Why u32? Can you use uint? Generally it is better for parameters to
> > use natural types unless there is a good reason.
>
> In fact u32 is identical to uint.
>
> <asm-generic/int-ll64.h>: typedef __u32 u32;
> <asm-generic/int-ll64.h>: typedef unsigned int __u32;
>
> <linux/types.h>: typedef unsigned int uint;
It's not about the eventual type...if u32 is the same as uint, why not
just use uint? It is simpler and does the right thing on 64-bit, where
u32 is different. The clock interface uses long. In some cases the
compiler must mask the values so it adds to code size.
Regards,
SImon
