linux-kernel - Re: [PATCH v2] rtc: ds3232: add temperature support

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:   Tue, 27 Jun 2017 15:00:57 +0200
From:   Alexandre Belloni <alexandre.belloni@...e-electrons.com>
To:     Kirill Esipov <yesipov@...il.com>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Alessandro Zummo <a.zummo@...ertech.it>,
        linux-rtc@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] rtc: ds3232: add temperature support

On 27/06/2017 at 15:24:57 +0300, Kirill Esipov wrote:
> 2017-06-25 19:39 GMT+03:00 Andy Shevchenko <andy.shevchenko@...il.com>:
> > On Thu, Jun 22, 2017 at 7:58 PM, Kirill Esipov <yesipov@...il.com> wrote:
> >> DS3232/DS3234 has the temperature registers with a resolution of 0.25
> >> degree celsius. This enables to get the value through hwmon.
> >>
> >>         # cat /sys/class/hwmon/hwmon0/temp1_input
> >>         37250
> >
> >> +config RTC_DRV_DS3232_HWMON
> >> +       bool "HWMON support for Dallas/Maxim DS3232/DS3234"
> >
> >> +       depends on RTC_DRV_DS3232 && HWMON
> >> +       depends on !(RTC_DRV_DS3232=y && HWMON=m)
> >
> > Perhaps it might be squeezed into one line (something like that logic
> > has been required by I2C related PMIC IIRC)
> >
> >> +       default y
> >
> > Is it really sane default?
> >
> 
> At first sight i thought that yes it is sane default (and others RTC with
> hwmon set it "default y" (ds1307, rv3029c2)).
> But if it's not sane, then we should turn it off by default in others drivers?
> 

It is definitively sane.

> 
> >> +#ifdef CONFIG_RTC_DRV_DS3232_HWMON
> >
> > IS_BUILTIN() ?
> >

I'd use IS_ENABLED in that case.

> >> +static int ds3232_hwmon_read_temp(struct device *dev, long int *mC)
> >> +{
> >> +       struct ds3232 *ds3232 = dev_get_drvdata(dev);
> >> +       u8 temp_buf[2];
> >> +       s16 temp;
> >> +       int ret;
> >> +
> >> +       ret = regmap_bulk_read(ds3232->regmap, DS3232_REG_TEMPERATURE, temp_buf,
> >> +                               sizeof(temp_buf));
> >
> >> +
> >
> > Remove.

I'd recommend running checkpatch.pl --strict to remove the remaining
whitespace issues too (a few alignments are off).

> >
> > I dunno which style is preferred, though you may use
> > if (IS_BUILTIN(...))
> >  return;
> >
> > at the beginning of the function and allow gcc optimizer to take care
> > of everything else.
> >

I don't have a strong opinion there.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com