lkml.org 
[lkml]   [2017]   [Jun]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2] rtc: ds3232: add temperature support
2017-06-27 16:00 GMT+03:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 27/06/2017 at 15:24:57 +0300, Kirill Esipov wrote:
>> 2017-06-25 19:39 GMT+03:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
>> > On Thu, Jun 22, 2017 at 7:58 PM, Kirill Esipov <yesipov@gmail.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.
>

Why? "RTC_DRV_DS3232_HWMON" is bool, not tristate. So it can't be
defined as "m".

>> >> +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



--
Kirill Esipov

\
 
 \ /
  Last update: 2017-06-27 17:28    [W:0.061 / U:0.800 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site