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: <20220527084647.30835-1-ctcchien@nuvoton.com> <20220527084647.30835-4-ctcchien@nuvoton.com>
 <YrYd+FkiFPz84twJ@mail.local> <CAHpyw9cdmCSZEE4ZbpnehpyvFhpPWA+_E5zXzJerNX9xqYet5Q@mail.gmail.com>
 <CAL3ZnpzSm7f1L2MMuDr9_vg3TQuk3PSr46fwmJTMz4sA2sdCJg@mail.gmail.com> <YsaQrksj8jBhJYGP@mail.local>
In-Reply-To: <YsaQrksj8jBhJYGP@mail.local>
From:   Mining Lin <mimi05633@gmail.com>
Date:   Thu, 7 Jul 2022 17:36:26 +0800
Message-ID: <CAL3ZnpxwEhH4bu8pmCCCkVG=bUHGvoTKSa1NgVJaEeEtcbGCBQ@mail.gmail.com>
Subject: Re: [PATCH v3 3/3] RTC: nuvoton: Add NCT3018Y real time clock driver
To:     Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc:     Benjamin Fair <benjaminfair@google.com>,
        Nancy Yuen <yuenn@google.com>,
        Patrick Venture <venture@google.com>,
        Tali Perry <tali.perry1@gmail.com>,
        Tomer Maimon <tmaimon77@gmail.com>,
        Avi Fishman <avifishman70@gmail.com>,
        Rob Herring <robh+dt@kernel.org>, a.zummo@towertech.it,
        KWLIU@nuvoton.com, YSCHU@nuvoton.com, JJLIU0@nuvoton.com,
        KFTING <KFTING@nuvoton.com>, ctcchien@nuvoton.com,
        Medad Young <medadyoung@gmail.com>,
        CS20 MYLin1 <mylin1@nuvoton.com>,
        OpenBMC Maillist <openbmc@lists.ozlabs.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        linux-rtc@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Dear Alexandre,

Thank you for your comment and response.
I will refine again and reply below.

Thanks.
Best Regards,
Mia

Alexandre Belloni <alexandre.belloni@bootlin.com> =E6=96=BC 2022=E5=B9=B47=
=E6=9C=887=E6=97=A5 =E9=80=B1=E5=9B=9B =E4=B8=8B=E5=8D=883:52=E5=AF=AB=E9=
=81=93=EF=BC=9A
>
> On 07/07/2022 15:17:28+0800, Mining Lin wrote:
> > Dear Alexandre,
> >
> > Thank you for your comments.
> > I will refine and reply below.
> >
> > Thanks.
> > Best Regards,
> > Mia
> >
> > Medad Young <medadyoung@gmail.com> =E6=96=BC 2022=E5=B9=B47=E6=9C=887=
=E6=97=A5 =E9=80=B1=E5=9B=9B =E4=B8=8B=E5=8D=881:31=E5=AF=AB=E9=81=93=EF=BC=
=9A
> > >
> > > Hello Alexandre,
> > >
> > > Thanks for your comments.
> > > I add Mining Lin <mimi05633@gmail.com> into this mail thread,
> > > and she is going to follow up this RTC driver.
> > > She will be in charge of maintaining this driver.
> > >
> > > Alexandre Belloni <alexandre.belloni@bootlin.com> =E6=96=BC 2022=E5=
=B9=B46=E6=9C=8825=E6=97=A5 =E9=80=B1=E5=85=AD =E5=87=8C=E6=99=A84:26=E5=AF=
=AB=E9=81=93=EF=BC=9A
> > > >
> > > > Hello,
> > > >
> > > > Please run ./scripts/checkpatch.pl --strict on your patch, there ar=
e a
> > > > bunch of issues.
> > > >
> > [Mia] I will run ./scripts/checkpatch.pl --strict on my patch to fix is=
sues.
> >
> > > > On 27/05/2022 16:46:47+0800, medadyoung@gmail.com wrote:
> > > > > +static int nct3018y_set_alarm_mode(struct i2c_client *client, bo=
ol on)
> > > > > +{
> > > > > +     int err, flags;
> > > > > +
> > > > > +     dev_dbg(&client->dev, "%s:on:%d\n", __func__, on);
> > > > > +
> > > > > +     flags =3D  i2c_smbus_read_byte_data(client, NCT3018Y_REG_CT=
RL);
> > > > > +     if (flags < 0) {
> > > > > +             dev_err(&client->dev,
> > > > > +                     "Failed to read NCT3018Y_REG_CTRL\n");
> > > >
> > > > You should cut down on the number of error messages, they are usual=
ly
> > > > not useful as the user doesn't have any specific action after getti=
ng
> > > > one of them apart from trying the action once again. Also, this wil=
l
> > > > make your code shorter. dev_dbg is fine.
> > > >
> > [Mia] I will modify dev_err to dev_dbg if there is an error and nothing=
 to do.
> >
> > > > > +/*
> > > > > + * In the routines that deal directly with the nct3018y hardware=
, we use
> > > > > + * rtc_time -- month 0-11, hour 0-23, yr =3D calendar year-epoch=
.
> > > > > + */
> > > > > +static int nct3018y_rtc_read_time(struct device *dev, struct rtc=
_time *tm)
> > > > > +{
> > > > > +     struct i2c_client *client =3D to_i2c_client(dev);
> > > > > +     unsigned char buf[10];
> > > > > +     int err;
> > > > > +
> > > >
> > > > You should still return an error if the time is invalid there but w=
ithout
> > > > an error message.
> > > >
> > [Mia] I will verify the time by rtc_valid_tm(tm).
> >
>
> No, I meant checking NCT3018Y_REG_ST as was done in the previous
> revisions of the series
>
[Mia] I will check NCT3018Y_REG_ST as was done in the previous revisions.

> > > > > +static struct clk *nct3018y_clkout_register_clk(struct nct3018y =
*nct3018y)
> > > > > +{
> > > > > +     struct i2c_client *client =3D nct3018y->client;
> > > > > +     struct device_node *node =3D client->dev.of_node;
> > > > > +     struct clk *clk;
> > > > > +     struct clk_init_data init;
> > > > > +     int flags, err;
> > > > > +
> > > > > +     /* disable the clkout output */
> > > > > +     flags =3D 0;
> > > > > +     err =3D i2c_smbus_write_byte_data(client, NCT3018Y_REG_CLKO=
, flags);
> > > >
> > > > BTW, this introduces a glitch in the clock output if the clock is
> > > > actually used. Maybe you could just rely on the CCF core to disable=
 this
> > > > clock when there are no users.
> > > >
> > [Mia] Do you mean there is no need to disable the clock output here?
> >
>
> The CCF will disable the clock at boot time if there are no users
>
[Mia] I will remove disable clock output. Thank you so much for your reply.

> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com