Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1672842 > unrolled thread
| Started by | Kirill Esipov <yesipov@gmail.com> |
|---|---|
| First post | 2017-06-22 19:00 +0200 |
| Last post | 2017-06-27 20:10 +0200 |
| Articles | 6 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH v2] rtc: ds3232: add temperature support Kirill Esipov <yesipov@gmail.com> - 2017-06-22 19:00 +0200
Re: [PATCH v2] rtc: ds3232: add temperature support Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-25 18:50 +0200
Re: [PATCH v2] rtc: ds3232: add temperature support Kirill Esipov <yesipov@gmail.com> - 2017-06-27 14:30 +0200
Re: [PATCH v2] rtc: ds3232: add temperature support Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-06-27 15:10 +0200
Re: [PATCH v2] rtc: ds3232: add temperature support Kirill Esipov <yesipov@gmail.com> - 2017-06-27 17:30 +0200
Re: [PATCH v2] rtc: ds3232: add temperature support Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-06-27 20:10 +0200
| From | Kirill Esipov <yesipov@gmail.com> |
|---|---|
| Date | 2017-06-22 19:00 +0200 |
| Subject | [PATCH v2] rtc: ds3232: add temperature support |
| Message-ID | <tVdCz-7lD-53@gated-at.bofh.it> |
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
Signed-off-by: Kirill Esipov <yesipov@gmail.com>
---
drivers/rtc/Kconfig | 9 ++++
drivers/rtc/rtc-ds3232.c | 134 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 143 insertions(+)
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 8d3b95728326..b4a6a916d4df 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -791,6 +791,15 @@ config RTC_DRV_DS3232
This driver can also be built as a module. If so, the module
will be called rtc-ds3232.
+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)
+ default y
+ help
+ Say Y here if you want to expose temperature sensor data on
+ rtc-ds3232
+
config RTC_DRV_PCF2127
tristate "NXP PCF2127"
depends on RTC_I2C_AND_SPI
diff --git a/drivers/rtc/rtc-ds3232.c b/drivers/rtc/rtc-ds3232.c
index deff431a37c4..4e7913c2ed79 100644
--- a/drivers/rtc/rtc-ds3232.c
+++ b/drivers/rtc/rtc-ds3232.c
@@ -22,6 +22,7 @@
#include <linux/bcd.h>
#include <linux/slab.h>
#include <linux/regmap.h>
+#include <linux/hwmon.h>
#define DS3232_REG_SECONDS 0x00
#define DS3232_REG_MINUTES 0x01
@@ -275,6 +276,137 @@ static int ds3232_update_alarm(struct device *dev, unsigned int enabled)
return ret;
}
+/*----------------------------------------------------------------------*/
+
+#ifdef CONFIG_RTC_DRV_DS3232_HWMON
+
+/*
+ * Temperature sensor support for ds3232/ds3234 devices.
+ */
+
+#define DS3232_REG_TEMPERATURE 0x11
+
+/*
+ * A user-initiated temperature conversion is not started by this function,
+ * so the temperature is updated once every 64 seconds.
+ */
+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));
+
+ if (ret < 0)
+ return ret;
+
+ /*
+ * Temperature is represented as a 10-bit code with a resolution of
+ * 0.25 degree celsius and encoded in two's complement format.
+ */
+ temp = (temp_buf[0] << 8) | temp_buf[1];
+ temp >>= 6;
+ *mC = temp * 250;
+
+ return 0;
+}
+
+static umode_t ds3232_hwmon_is_visible(const void *data,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ if (type != hwmon_temp)
+ return 0;
+
+ switch (attr) {
+ case hwmon_temp_input:
+ return 0444;
+ default:
+ return 0;
+ }
+}
+
+static int ds3232_hwmon_read(struct device *dev,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel, long *temp)
+{
+ int err;
+
+ switch (attr) {
+ case hwmon_temp_input:
+ ds3232_hwmon_read_temp(dev, temp);
+ err = 0;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return err;
+}
+
+
+static u32 ds3232_hwmon_chip_config[] = {
+ HWMON_C_REGISTER_TZ,
+ 0
+};
+
+static const struct hwmon_channel_info ds3232_hwmon_chip = {
+ .type = hwmon_chip,
+ .config = ds3232_hwmon_chip_config,
+};
+
+static u32 ds3232_hwmon_temp_config[] = {
+ HWMON_T_INPUT,
+ 0
+};
+
+static const struct hwmon_channel_info ds3232_hwmon_temp = {
+ .type = hwmon_temp,
+ .config = ds3232_hwmon_temp_config,
+};
+
+static const struct hwmon_channel_info *ds3232_hwmon_info[] = {
+ &ds3232_hwmon_chip,
+ &ds3232_hwmon_temp,
+ NULL
+};
+
+static const struct hwmon_ops ds3232_hwmon_hwmon_ops = {
+ .is_visible = ds3232_hwmon_is_visible,
+ .read = ds3232_hwmon_read,
+};
+
+static const struct hwmon_chip_info ds3232_hwmon_chip_info = {
+ .ops = &ds3232_hwmon_hwmon_ops,
+ .info = ds3232_hwmon_info,
+};
+
+static void ds3232_hwmon_register(struct device *dev, const char *name)
+{
+ struct ds3232 *ds3232 = dev_get_drvdata(dev);
+ struct device *hwmon_dev;
+
+ hwmon_dev = devm_hwmon_device_register_with_info(dev, name, ds3232,
+ &ds3232_hwmon_chip_info,
+ NULL);
+
+ if (IS_ERR(hwmon_dev)) {
+ dev_warn(dev, "unable to register hwmon device %ld\n",
+ PTR_ERR(hwmon_dev));
+ }
+}
+
+#else
+
+static void ds3232_hwmon_register(struct device *dev, const char *name)
+{
+}
+
+#endif
+
static int ds3232_alarm_irq_enable(struct device *dev, unsigned int enabled)
{
struct ds3232 *ds3232 = dev_get_drvdata(dev);
@@ -366,6 +498,8 @@ static int ds3232_probe(struct device *dev, struct regmap *regmap, int irq,
if (ds3232->irq > 0)
device_init_wakeup(dev, 1);
+ ds3232_hwmon_register(dev, name);
+
ds3232->rtc = devm_rtc_device_register(dev, name, &ds3232_rtc_ops,
THIS_MODULE);
if (IS_ERR(ds3232->rtc))
--
2.7.4
[toc] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-06-25 18:50 +0200 |
| Message-ID | <tWiTw-7sJ-7@gated-at.bofh.it> |
| In reply to | #1672842 |
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?
> +#ifdef CONFIG_RTC_DRV_DS3232_HWMON
IS_BUILTIN() ?
> +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.
> + if (ret < 0)
> + return ret;
> +static umode_t ds3232_hwmon_is_visible(const void *data,
> + enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + if (type != hwmon_temp)
> + return 0;
> +
> + switch (attr) {
> + case hwmon_temp_input:
> + return 0444;
> + default:
> + return 0;
> + }
> +}
> +
> +static int ds3232_hwmon_read(struct device *dev,
> + enum hwmon_sensor_types type,
> + u32 attr, int channel, long *temp)
> +{
> + int err;
> +
> + switch (attr) {
> + case hwmon_temp_input:
> + ds3232_hwmon_read_temp(dev, temp);
> + err = 0;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return err;
You may do as in previous function. Or what did you mean here by
introducing an err variable?
> +}
> +
> +
Remove one of them.
> +static void ds3232_hwmon_register(struct device *dev, const char *name)
> +{
> + struct ds3232 *ds3232 = dev_get_drvdata(dev);
> + struct device *hwmon_dev;
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(dev, name, ds3232,
> + &ds3232_hwmon_chip_info,
> + NULL);
> +
Remove.
> + if (IS_ERR(hwmon_dev)) {
> + dev_warn(dev, "unable to register hwmon device %ld\n",
> + PTR_ERR(hwmon_dev));
> + }
> +}
> +
> +#else
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.
> +
> +static void ds3232_hwmon_register(struct device *dev, const char *name)
> +{
> +}
> +
> +#endif
--
With Best Regards,
Andy Shevchenko
[toc] | [prev] | [next] | [standalone]
| From | Kirill Esipov <yesipov@gmail.com> |
|---|---|
| Date | 2017-06-27 14:30 +0200 |
| Message-ID | <tWXN0-pJ-21@gated-at.bofh.it> |
| In reply to | #1674250 |
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?
>> +#ifdef CONFIG_RTC_DRV_DS3232_HWMON
>
> IS_BUILTIN() ?
>
>> +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.
>
>> + if (ret < 0)
>> + return ret;
>
>> +static umode_t ds3232_hwmon_is_visible(const void *data,
>> + enum hwmon_sensor_types type,
>> + u32 attr, int channel)
>> +{
>> + if (type != hwmon_temp)
>> + return 0;
>> +
>> + switch (attr) {
>> + case hwmon_temp_input:
>> + return 0444;
>> + default:
>> + return 0;
>> + }
>> +}
>> +
>> +static int ds3232_hwmon_read(struct device *dev,
>> + enum hwmon_sensor_types type,
>> + u32 attr, int channel, long *temp)
>> +{
>> + int err;
>> +
>> + switch (attr) {
>> + case hwmon_temp_input:
>> + ds3232_hwmon_read_temp(dev, temp);
>> + err = 0;
>> + break;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + return err;
>
> You may do as in previous function. Or what did you mean here by
> introducing an err variable?
>
>> +}
>
>> +
>> +
>
> Remove one of them.
>
>> +static void ds3232_hwmon_register(struct device *dev, const char *name)
>> +{
>> + struct ds3232 *ds3232 = dev_get_drvdata(dev);
>> + struct device *hwmon_dev;
>> +
>> + hwmon_dev = devm_hwmon_device_register_with_info(dev, name, ds3232,
>> + &ds3232_hwmon_chip_info,
>> + NULL);
>
>> +
>
> Remove.
>
>> + if (IS_ERR(hwmon_dev)) {
>> + dev_warn(dev, "unable to register hwmon device %ld\n",
>> + PTR_ERR(hwmon_dev));
>> + }
>> +}
>> +
>
>> +#else
>
> 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.
>
>> +
>> +static void ds3232_hwmon_register(struct device *dev, const char *name)
>> +{
>> +}
>> +
>> +#endif
>
> --
> With Best Regards,
> Andy Shevchenko
--
Kirill Esipov
[toc] | [prev] | [next] | [standalone]
| From | Alexandre Belloni <alexandre.belloni@free-electrons.com> |
|---|---|
| Date | 2017-06-27 15:10 +0200 |
| Message-ID | <tWYpH-WT-11@gated-at.bofh.it> |
| In reply to | #1675619 |
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.
> >> +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
[toc] | [prev] | [next] | [standalone]
| From | Kirill Esipov <yesipov@gmail.com> |
|---|---|
| Date | 2017-06-27 17:30 +0200 |
| Message-ID | <tX0Bb-2nz-1@gated-at.bofh.it> |
| In reply to | #1675682 |
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
[toc] | [prev] | [next] | [standalone]
| From | Alexandre Belloni <alexandre.belloni@free-electrons.com> |
|---|---|
| Date | 2017-06-27 20:10 +0200 |
| Message-ID | <tX362-4be-31@gated-at.bofh.it> |
| In reply to | #1675885 |
On 27/06/2017 at 18:27:42 +0300, Kirill Esipov wrote:
> 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".
>
It's clearer and doesn't hurt but really, #ifdef
CONFIG_RTC_DRV_DS3232_HWMON is just fine.
> >> >> +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
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web