Date: Wed, 21 Jun 2017 15:24:21 -0700
From: Guenter Roeck <linux@roeck-us.net>
To: Kirill Esipov <yesipov@gmail.com>
Cc: a.zummo@towertech.it, alexandre.belloni@free-electrons.com,
        linux-rtc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: rtc: ds3232: add temperature support
Message-ID: <20170621222421.GA23791@roeck-us.net>
References: <1498056583-16551-1-git-send-email-yesipov@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1498056583-16551-1-git-send-email-yesipov@gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4236
Lines: 147

On Wed, Jun 21, 2017 at 05:49:43PM +0300, Kirill Esipov 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
> 
> Signed-off-by: Kirill Esipov <yesipov@gmail.com>
> ---
>  drivers/rtc/Kconfig      |  9 ++++++
>  drivers/rtc/rtc-ds3232.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 93 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..f94ff0685942 100644
> --- a/drivers/rtc/rtc-ds3232.c
> +++ b/drivers/rtc/rtc-ds3232.c
> @@ -22,6 +22,8 @@
>  #include <linux/bcd.h>
>  #include <linux/slab.h>
>  #include <linux/regmap.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
>  
>  #define DS3232_REG_SECONDS      0x00
>  #define DS3232_REG_MINUTES      0x01
> @@ -275,6 +277,86 @@ 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, s32 *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 ssize_t ds3232_hwmon_show_temp(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	int ret;
> +	s32 temp;
> +
> +	ret = ds3232_hwmon_read_temp(dev, &temp);
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n", temp);
> +}
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, ds3232_hwmon_show_temp,
> +			NULL, 0);
> +
> +static struct attribute *ds3232_hwmon_attrs[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(ds3232_hwmon);
> +
> +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_groups(dev, name, ds3232,
> +							   ds3232_hwmon_groups);

Any reason for not using devm_hwmon_device_register_with_info() ?

Guenter

> +	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 ds3232 *ds3232)
> +{
> +}
> +
> +#endif
> +
>  static int ds3232_alarm_irq_enable(struct device *dev, unsigned int enabled)
>  {
>  	struct ds3232 *ds3232 = dev_get_drvdata(dev);
> @@ -366,6 +448,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))