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
