Re: [Letux-kernel] [PATCH 5/5] rtc: rtc-rc5t583: add ricoh rc5t619 RTC driver
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, _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
Re: [Letux-kernel] [PATCH 5/5] rtc: rtc-rc5t583: add ricoh rc5t619 RTC driver
Just a meta-comment...
> Am 21.10.2019 um 12:15 schrieb Alexandre Belloni
> :
>
> Hi,
>
> The subject line is weird, how is it related to rc5t583?
>
> On 21/10/2019 07:41:04+0200, Andreas Kemnade wrote:
>> config RTC_DRV_S35390A
>> tristate "Seiko Instruments S-35390A"
>> select BITREVERSE
>> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
>> index 6b09c21dc1b6..1d0673fd0954 100644
>> --- a/drivers/rtc/Makefile
>> +++ b/drivers/rtc/Makefile
>> @@ -136,6 +136,7 @@ obj-$(CONFIG_RTC_DRV_PXA)+= rtc-pxa.o
>> obj-$(CONFIG_RTC_DRV_R7301) += rtc-r7301.o
>> obj-$(CONFIG_RTC_DRV_R9701) += rtc-r9701.o
>> obj-$(CONFIG_RTC_DRV_RC5T583)+= rtc-rc5t583.o
>> +obj-$(CONFIG_RTC_DRV_RC5T619) += rtc-rc5t619.o
>> obj-$(CONFIG_RTC_DRV_RK808) += rtc-rk808.o
>> obj-$(CONFIG_RTC_DRV_RP5C01) += rtc-rp5c01.o
>> obj-$(CONFIG_RTC_DRV_RS5C313)+= rtc-rs5c313.o
>> diff --git a/drivers/rtc/rtc-rc5t619.c b/drivers/rtc/rtc-rc5t619.c
>> new file mode 100644
>> index ..311788ff0723
>> --- /dev/null
>> +++ b/drivers/rtc/rtc-rc5t619.c
>> @@ -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?
>
>> + */
>> +
>> +/* #define debug1 */
>> +/* #define verbose_debug1 */
>> +
>
> No dead code please.
>
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +struct rc5t619_rtc {
>> +int irq;
>> +struct rtc_device *rtc;
>> +struct rn5t618 *rn5t618;
>> +};
>> +
>> +#define CTRL1_ALARM_ENABLED 0x40
>> +#define CTRL1_24HR 0x20
>> +#define CTRL1_PERIODIC_MASK 0xf
>> +
>> +#define CTRL2_PON 0x10
>> +#define CTRL2_ALARM_STATUS 0x80
>> +#define CTRL2_CTFG 0x4
>> +#define CTRL2_CTC 0x1
>> +
>> +static int rc5t619_rtc_periodic_disable(struct device *dev)
>> +{
>> +struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
>> +int err;
>> +
>> +/* disable function */
>> +err = regmap_update_bits(rtc->rn5t618->regmap,
>> +RN5T618_RTC_CTRL1, CTRL1_PERIODIC_MASK, 0);
>> +if (err < 0)
>> +return err;
>> +
>> +/* clear alarm flag and CTFG */
>> +err = regmap_update_bits(rtc->rn5t618->regmap, RN5T618_RTC_CTRL2,
>> +CTRL2_ALARM_STATUS | CTRL2_CTFG | CTRL2_CTC, 0);
>> +if (err < 0)
>> +return err;
>> +
>> +return 0;
>> +}
>> +
>> +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.
>
> Also what is that adjusting? If this is adding/removing clock cycles,
> you need to use .set_offset and .read_offset.
>
>> +}
>> +
>> +static int rc5t619_rtc_pon_get_clr(struct device *dev, uint8_t *pon_f)
>> +{
>> +struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
>> +int err;
>> +unsigned int reg_data;
>> +
>> +err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL2, _data);
>> +if (err < 0)
>> +return err;
>> +
>> +if (reg_data & CTRL2_PON) {
>> +*pon_f = 1;
>> +/* clear VDET PON */
>> +reg_data &= ~(CTRL2_PON | CTRL2_CTC | 0x4a);/* 0101-1011 */
>> +reg_data |= 0x20; /* 0010- */
>
> Is it possible to have more defines for those magic values?
>
>> +err = regmap_write(rtc->rn5t618->regmap, RN5T618_RTC_CTRL2,
>> +reg_data);
>> +} else {
>> +*pon_f = 0;
>> +}
>> +
>> +return err;
>> +}
>> +
>> +/* 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);
