From: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: Carlos Menin <menin@carlosaurelio.net>
Cc: linux-rtc@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org,
Alessandro Zummo <a.zummo@towertech.it>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
Jean Delvare <jdelvare@suse.com>,
Guenter Roeck <linux@roeck-us.net>,
Sergio Prado <sergio.prado@e-labworks.com>
Subject: Re: [PATCH v2 1/2] rtc: add pcf85053a
Date: Fri, 17 Nov 2023 23:21:44 +0100 [thread overview]
Message-ID: <2023111722214402006513@mail.local> (raw)
In-Reply-To: <ZUf_3TAZh1bpVOVt@arch-bow>
On 05/11/2023 17:49:33-0300, Carlos Menin wrote:
> > > +static int pcf85053a_rtc_check_reliability(struct device *dev, u8 status_reg)
> > > +{
> > > + int ret = 0;
> > > +
> > > + if (status_reg & REG_STATUS_CIF) {
> > > + dev_warn(dev, "tamper detected,"
> > > + " date/time is not reliable\n");
> > You should not split strings. Also, I don't think most of the messages
> > are actually useful as the end user doesn't have any specific action
> > after seeing it. You should probably drop them.
> >
>
> About the usefullness of the message, do this apply to this CIF related
> message or also to the other two flags?
This actually applies to all the messages of the driver, they will add
to the size of the kernel then slow the boot time so please have a
careful look at the usefulness of messages. You may consider swtching
them to dev_dbg.
> > > + tm->tm_year = buf[REG_YEARS];
> > > + /* adjust for 1900 base of rtc_time */
> > > + tm->tm_year += 100;
> > > +
> > > + tm->tm_wday = (buf[REG_WEEKDAYS] - 1) & 7; /* 1 - 7 */
> > > + tm->tm_sec = buf[REG_SECS];
> > > + tm->tm_min = buf[REG_MINUTES];
> > > + tm->tm_hour = buf[REG_HOURS];
> > > + tm->tm_mday = buf[REG_DAYS];
> > > + tm->tm_mon = buf[REG_MONTHS] - 1; /* 1 - 12 */
> >
> > Those comments are not useful.
> >
>
> I Will improve them to explain why I am offsetting the register value.
I don't think this is necessary, most of the drivers are doing it so
this is expected.
> > > +static struct attribute *pcf85053a_attrs_flags[] = {
> > > + &dev_attr_rtc_fail.attr,
> > > + &dev_attr_oscillator_fail.attr,
> > > + &dev_attr_rtc_clear.attr,
> > > + 0,
> > > +};
> >
> > Don't add undocumented sysfs files. Also, You must not allow userspace
> > to clear those flags without setting the time properly.
> >
>
> Should the flags be cleared only by setting the time, or is there
> another acceptable method? What would be the correct way to let
> userspace read those flags?
The RTC_VL_READ ioctl will allow reading the flags from userspace. For
the flags that monitor the validity of the time and date, they must only
be cleared when the time and date is known to be good s only when
setting the time.
> > > +}
> > > +
> > > +static void pcf85053a_set_low_jitter(struct device *dev, u8 *reg_ctrl)
> > > +{
> > > + bool val;
> > > + u8 regval;
> > > +
> > > + val = of_property_read_bool(dev->of_node, "nxp,low-jitter-mode");
> >
> > Bool properties don't work well with RTC because with this, there is now
> > way to enable the normal mode.
> >
>
> Wouldn't the absence of the property enable normal mode? Or I am missing
> something?
RTC have a greater lifetime than the linux system so you must have a way
to indicate that you don't want to change the configuration for example
if it has been set from the bootloader or at the factory.
Regards,
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
prev parent reply other threads:[~2023-11-17 22:21 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-03 12:51 [PATCH v2 1/2] rtc: add pcf85053a Carlos Menin
2023-11-03 12:51 ` [PATCH v2 2/2] dt-bindings: " Carlos Menin
2023-11-05 13:39 ` Krzysztof Kozlowski
2023-11-03 14:09 ` [PATCH v2 1/2] " Guenter Roeck
2023-11-05 20:17 ` Carlos Menin
2023-11-05 22:31 ` Guenter Roeck
2023-11-03 14:28 ` Alexandre Belloni
2023-11-05 20:49 ` Carlos Menin
2023-11-17 22:21 ` Alexandre Belloni [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2023111722214402006513@mail.local \
--to=alexandre.belloni@bootlin.com \
--cc=a.zummo@towertech.it \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=jdelvare@suse.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rtc@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=menin@carlosaurelio.net \
--cc=robh+dt@kernel.org \
--cc=sergio.prado@e-labworks.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).