linux-kernel - Re: [PATCH v3 3/3] RTC: nuvoton: Add NCT3018Y real time clock driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 7 Jul 2022 09:52:14 +0200
From:   Alexandre Belloni <alexandre.belloni@...tlin.com>
To:     Mining Lin <mimi05633@...il.com>
Cc:     Benjamin Fair <benjaminfair@...gle.com>,
        Nancy Yuen <yuenn@...gle.com>,
        Patrick Venture <venture@...gle.com>,
        Tali Perry <tali.perry1@...il.com>,
        Tomer Maimon <tmaimon77@...il.com>,
        Avi Fishman <avifishman70@...il.com>,
        Rob Herring <robh+dt@...nel.org>, a.zummo@...ertech.it,
        KWLIU@...oton.com, YSCHU@...oton.com, JJLIU0@...oton.com,
        KFTING <KFTING@...oton.com>, ctcchien@...oton.com,
        Medad Young <medadyoung@...il.com>,
        CS20 MYLin1 <mylin1@...oton.com>,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        linux-rtc@...r.kernel.org
Subject: Re: [PATCH v3 3/3] RTC: nuvoton: Add NCT3018Y real time clock driver

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@...il.com> 於 2022年7月7日 週四 下午1:31寫道：
> >
> > Hello Alexandre,
> >
> > Thanks for your comments.
> > I add Mining Lin <mimi05633@...il.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@...tlin.com> 於 2022年6月25日 週六 凌晨4:26寫道：
> > >
> > > Hello,
> > >
> > > Please run ./scripts/checkpatch.pl --strict on your patch, there are a
> > > bunch of issues.
> > >
> [Mia] I will run ./scripts/checkpatch.pl --strict on my patch to fix issues.
> 
> > > On 27/05/2022 16:46:47+0800, medadyoung@...il.com wrote:
> > > > +static int nct3018y_set_alarm_mode(struct i2c_client *client, bool on)
> > > > +{
> > > > +     int err, flags;
> > > > +
> > > > +     dev_dbg(&client->dev, "%s:on:%d\n", __func__, on);
> > > > +
> > > > +     flags =  i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
> > > > +     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 usually
> > > not useful as the user doesn't have any specific action after getting
> > > one of them apart from trying the action once again. Also, this will
> > > 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 = calendar year-epoch.
> > > > + */
> > > > +static int nct3018y_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > > > +{
> > > > +     struct i2c_client *client = to_i2c_client(dev);
> > > > +     unsigned char buf[10];
> > > > +     int err;
> > > > +
> > >
> > > You should still return an error if the time is invalid there but without
> > > 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

> > > > +static struct clk *nct3018y_clkout_register_clk(struct nct3018y *nct3018y)
> > > > +{
> > > > +     struct i2c_client *client = nct3018y->client;
> > > > +     struct device_node *node = client->dev.of_node;
> > > > +     struct clk *clk;
> > > > +     struct clk_init_data init;
> > > > +     int flags, err;
> > > > +
> > > > +     /* disable the clkout output */
> > > > +     flags = 0;
> > > > +     err = 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

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