Linux-RTC Archive on lore.kernel.org
 help / color / Atom feed
From: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: "H. Nikolaus Schaller" <hns@goldelico.com>
Cc: Andreas Kemnade <andreas@kemnade.info>,
	linux-rtc@vger.kernel.org, a.zummo@towertech.it,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	stefan@agner.ch, b.galvani@gmail.com, phh@phh.me,
	lee.jones@linaro.org,
	Discussions about the Letux Kernel 
	<letux-kernel@openphoenux.org>
Subject: Re: [Letux-kernel] [PATCH 5/5] rtc: rtc-rc5t583: add ricoh rc5t619 RTC driver
Date: Mon, 21 Oct 2019 17:09:59 +0200
Message-ID: <20191021150959.GX3125@piout.net> (raw)
In-Reply-To: <F16BC3B8-5497-4A7D-AC88-4DB221038519@goldelico.com>

On 21/10/2019 12:31:30+0200, H. Nikolaus Schaller wrote:
> >> @@ -0,0 +1,476 @@
> >> +// SPDX-License-Identifier: GPL-2.0+
> >> +/*
> >> + * drivers/rtc/rtc-ricoh619.c
> >> + *
> >> + * Real time clock driver for RICOH R5T619 power management chip.
> >> + *
> >> + * Copyright (C) 2019 Andreas Kemnade
> >> + *
> >> + * Based on code
> >> + *  Copyright (C) 2012-2014 RICOH COMPANY,LTD
> >> + *
> >> + * Based on code
> >> + *  Copyright (C) 2011 NVIDIA Corporation
> > 
> > Based on is not useful.
> 
> Yes, it is difficult to track what the real contribution was
> and what is left over.
> 
> On the other hand it is IMHO fair to attribute those who have
> spent time to save ours.
> 
> What would be a better way for attribution?
> 

I don't think this require attribution

> >> +static int rc5t619_rtc_clk_adjust(struct device *dev, uint8_t clk)
> >> +{
> >> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> >> +
> >> +	return regmap_write(rtc->rn5t618->regmap, RN5T618_RTC_ADJUST, clk);
> > 
> > Is it useful to have a function for a single regmap_write?
> 
> I'd say yes. It is wrapping all regmap accesses in getter/setter functions
> whose name describes what it is setting. And it may do type conversion.
> IMHO this makes code better readable and maintainable.
> And a good compiler may even decide to inline this.
> 

But this takes up a lot of space in the file which makes it harder to
read while adding no information, how is rc5t619_rtc_clk_adjust more
informative than regmap_write(rtc->rn5t618->regmap, RN5T618_RTC_ADJUST,
clk)?

in both cases, I can easily deduce that the RTC adjust register is
changed.


> >> +/* 0-12hour, 1-24hour */
> >> +static int rc5t619_rtc_24hour_mode_set(struct device *dev, int mode)
> >> +{
> >> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> >> +
> >> +	return regmap_update_bits(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1,
> >> +			CTRL1_24HR, mode ? CTRL1_24HR : 0);
> > 
> > Is it useful to have a function for a single regmap_update_bits?
> 
> Same as above. I can immediately understand
> 
> 	r = rc5t619_rtc_24hour_mode_set(dev, MODE_SOMETHING);
> 
> somewhere else in code but deciphering 
> 
> 	r = regmap_update_bits(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1,
> 				CTRL1_24HR, mode ? CTRL1_24HR : 0);
> 
> spread over several places is probably difficult.
> 

I can immediately understand updating CTRL1_24HR in RN5T618_RTC_CTRL1
And it is used exactly once...

If this is all about naming and understanding, then why that driver
still had so many magic values?


> > 
> >> +}
> >> +
> >> +
> >> +static int rc5t619_rtc_read_time(struct device *dev, struct rtc_time *tm)
> >> +{
> >> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> >> +	u8 buff[7];
> >> +	int err;
> >> +	int cent_flag;
> >> +
> >> +	err = regmap_bulk_read(rtc->rn5t618->regmap, RN5T618_RTC_SECONDS,
> >> +				buff, sizeof(buff));
> >> +	if (err < 0) {
> >> +		dev_err(dev, "failed to read time: %d\n", err);
> > 
> > Please reconsider adding so many strings in the driver, they add almost
> > no value but will increase the kernel memory footprint.
> 
> You mean removing error messages is better than taking some footprint?
> 

Definitively yes, what is the value of the error message on the device?
What should the end user do about it? Is there even an end user reading
that message?

> >> +static int rc5t619_rtc_alarm_is_enabled(struct device *dev,  uint8_t *enabled)
> >> +{
> >> +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> >> +	int err;
> >> +	unsigned int reg_data;
> >> +
> >> +	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL1, &reg_data);
> >> +	if (err) {
> >> +		dev_err(dev, "read RTC_CTRL1 error %d\n", err);
> >> +		*enabled = 0;
> > 
> > Is it necessary to set enabled here?
> 
> Well, in case of error it is probably more safe to assume it is *not* enabled
> that keeping the random value passed by the caller of this function.
> 

I would certainly hope that the caller is smart enough to not use a
value when the function calling it returns an error. This has to be
removed, it is useless.


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

  reply index

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-21  5:40 [PATCH 0/5] Add rtc support for rn5t618 mfd Andreas Kemnade
2019-10-21  5:41 ` [PATCH 1/5] mfd: rn5t618: prepare for irq handling Andreas Kemnade
2019-10-21  5:41 ` [PATCH 2/5] mfd: rn5t618: add irq support Andreas Kemnade
2019-10-21  9:07   ` Alexandre Belloni
2019-10-21 15:06     ` Andreas Kemnade
2019-10-21  5:41 ` [PATCH 3/5] mfd: rn5t618: add rtc related registers Andreas Kemnade
2019-10-21  5:41 ` [PATCH 4/5] mfd: rn5t618: add more subdevices Andreas Kemnade
2019-10-21  5:41 ` [PATCH 5/5] rtc: rtc-rc5t583: add ricoh rc5t619 RTC driver Andreas Kemnade
2019-10-21 10:15   ` Alexandre Belloni
2019-10-21 10:31     ` [Letux-kernel] " H. Nikolaus Schaller
2019-10-21 15:09       ` Alexandre Belloni [this message]
2019-10-21 21:13     ` Andreas Kemnade
2019-10-23 23:17       ` Alexandre Belloni
2019-10-24 20:25         ` Andreas Kemnade
2019-10-24 22:06           ` Alexandre Belloni
2019-10-29  6:46         ` Andreas Kemnade
2019-10-21 13:19   ` Stefan Agner
2019-10-21 13:50     ` Alexandre Belloni
2019-10-21 15:20       ` Andreas Kemnade

Reply instructions:

You may reply publically 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=20191021150959.GX3125@piout.net \
    --to=alexandre.belloni@bootlin.com \
    --cc=a.zummo@towertech.it \
    --cc=andreas@kemnade.info \
    --cc=b.galvani@gmail.com \
    --cc=hns@goldelico.com \
    --cc=lee.jones@linaro.org \
    --cc=letux-kernel@openphoenux.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rtc@vger.kernel.org \
    --cc=phh@phh.me \
    --cc=stefan@agner.ch \
    /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

Linux-RTC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rtc/0 linux-rtc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rtc linux-rtc/ https://lore.kernel.org/linux-rtc \
		linux-rtc@vger.kernel.org
	public-inbox-index linux-rtc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rtc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git