[v2,9/9] rtc: isl12022: add support for temperature sensor

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/rtc/rtc-isl12022.c | 104 +++++++++++++++++++++++++++++++++++++
 1 file changed, 104 insertions(+)

On 9/21/22 04:46, Rasmus Villemoes wrote:
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>   drivers/rtc/rtc-isl12022.c | 104 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 104 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
> index ca677c4265e6..f3efe61c81e5 100644
> --- a/drivers/rtc/rtc-isl12022.c
> +++ b/drivers/rtc/rtc-isl12022.c
> @@ -17,6 +17,7 @@
>   #include <linux/of.h>
>   #include <linux/of_device.h>
>   #include <linux/regmap.h>
> +#include <linux/hwmon.h>
>   
>   /* ISL register offsets */
>   #define ISL12022_REG_SC		0x00
> @@ -30,6 +31,9 @@
>   #define ISL12022_REG_SR		0x07
>   #define ISL12022_REG_INT	0x08
>   
> +#define ISL12022_REG_BETA	0x0d
> +#define ISL12022_REG_TEMP_L	0x28
> +
>   /* ISL register bits */
>   #define ISL12022_HR_MIL		(1 << 7)	/* military or 24 hour time */
>   
> @@ -38,6 +42,7 @@
>   
>   #define ISL12022_INT_WRTC	(1 << 6)
>   
> +#define ISL12022_BETA_TSE	(1 << 7)
>   
>   static struct i2c_driver isl12022_driver;
>   
> @@ -46,6 +51,103 @@ struct isl12022 {
>   	struct regmap *regmap;
>   };
>   
> +static umode_t isl12022_hwmon_is_visible(const void *data,
> +					 enum hwmon_sensor_types type,
> +					 u32 attr, int channel)
> +{
> +	if (type == hwmon_chip && attr == hwmon_chip_update_interval)
> +		return 0444;
> +
> +	if (type == hwmon_temp && attr == hwmon_temp_input)
> +		return 0444;
> +
> +	return 0;
> +}
> +
> +/*
> + * A user-initiated temperature conversion is not started by this function,
> + * so the temperature is updated once every ~60 seconds.
> + */
> +static int isl12022_hwmon_read_temp(struct device *dev, long *mC)
> +{
> +	struct isl12022 *isl12022 = dev_get_drvdata(dev);
> +	struct regmap *regmap = isl12022->regmap;
> +	u8 temp_buf[2];
> +	int temp, ret;
> +
> +	ret = regmap_bulk_read(regmap, ISL12022_REG_TEMP_L,
> +			       temp_buf, sizeof(temp_buf));
> +	if (ret)
> +		return ret;
> +	/*
> +	 * Temperature is represented as a 10-bit number, unit half-Kelvins.
> +	 */
> +	temp = (temp_buf[1] << 8) | temp_buf[0];
> +	temp *= 500;
> +	temp -= 273000;
> +
> +	*mC = temp;
> +
> +	return 0;
> +}
> +
> +static int isl12022_hwmon_read(struct device *dev,
> +			       enum hwmon_sensor_types type,
> +			       u32 attr, int channel, long *val)
> +{
> +	if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
> +		*val = 60000;
> +		return 0;
> +	}

It is not the purpose of the update_interval attribute to inform the
user what the update interval of this chip happens to be. The purpose
of the attribute is to inform the chip what update interval it should use.

 From the documentation:
	Some devices have a variable update rate or interval.
	This attribute can be used to change it to the desired value.

The patch is not in the hwmon subsystem, to it is up to the maintainers
to decide if they want to accept the patch anyway. Please don't expect
a reviewed tag, though.

> +
> +	if (type == hwmon_temp && attr == hwmon_temp_input) {
> +		return isl12022_hwmon_read_temp(dev, val);
> +	}

Nit: { } is unnecessary.

Guenter

> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct hwmon_channel_info *isl12022_hwmon_info[] = {
> +	HWMON_CHANNEL_INFO(chip, HWMON_C_UPDATE_INTERVAL),
> +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> +	NULL
> +};
> +
> +static const struct hwmon_ops isl12022_hwmon_ops = {
> +	.is_visible = isl12022_hwmon_is_visible,
> +	.read = isl12022_hwmon_read,
> +};
> +
> +static const struct hwmon_chip_info isl12022_hwmon_chip_info = {
> +	.ops = &isl12022_hwmon_ops,
> +	.info = isl12022_hwmon_info,
> +};
> +
> +static void isl12022_hwmon_register(struct device *dev)
> +{
> +	struct isl12022 *isl12022;
> +	struct device *hwmon;
> +	int ret;
> +
> +	if (!IS_REACHABLE(CONFIG_HWMON))
> +		return;
> +
> +	isl12022 = dev_get_drvdata(dev);
> +
> +	ret = regmap_update_bits(isl12022->regmap, ISL12022_REG_BETA,
> +				 ISL12022_BETA_TSE, ISL12022_BETA_TSE);
> +	if (ret) {
> +		dev_warn(dev, "unable to enable temperature sensor\n");
> +		return;
> +	}
> +
> +	hwmon = devm_hwmon_device_register_with_info(dev, "isl12022", isl12022,
> +						     &isl12022_hwmon_chip_info,
> +						     NULL);
> +	if (IS_ERR(hwmon))
> +		dev_warn(dev, "unable to register hwmon device: %pe\n", hwmon);
> +}
> +
>   /*
>    * In the routines that deal directly with the isl12022 hardware, we use
>    * rtc_time -- month 0-11, hour 0-23, yr = calendar year-epoch.
> @@ -160,6 +262,8 @@ static int isl12022_probe(struct i2c_client *client)
>   		return PTR_ERR(isl12022->regmap);
>   	}
>   
> +	isl12022_hwmon_register(&client->dev);
> +
>   	isl12022->rtc = devm_rtc_allocate_device(&client->dev);
>   	if (IS_ERR(isl12022->rtc))
>   		return PTR_ERR(isl12022->rtc);

On 21/09/2022 16.13, Guenter Roeck wrote:
> On 9/21/22 04:46, Rasmus Villemoes wrote:

>> +static int isl12022_hwmon_read(struct device *dev,
>> +                   enum hwmon_sensor_types type,
>> +                   u32 attr, int channel, long *val)
>> +{
>> +    if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
>> +        *val = 60000;
>> +        return 0;
>> +    }
> 
> It is not the purpose of the update_interval attribute to inform the
> user what the update interval of this chip happens to be. The purpose
> of the attribute is to inform the chip what update interval it should use.

Well, I think it's a completely natural thing to expose a fixed and
known update_interval as a 0444 property, and it might even be useful to
userspace to know that there's no point reading the sensor any more
often than that. And I didn't come up with this by myself, there's
already at least a couple of instances of a 0444 update_interval.

I'll leave it to the RTC maintainers to decide, it's easy enough to remove.

Rasmus

On 9/23/22 01:40, Rasmus Villemoes wrote:
> On 21/09/2022 16.13, Guenter Roeck wrote:
>> On 9/21/22 04:46, Rasmus Villemoes wrote:
> 
>>> +static int isl12022_hwmon_read(struct device *dev,
>>> +                   enum hwmon_sensor_types type,
>>> +                   u32 attr, int channel, long *val)
>>> +{
>>> +    if (type == hwmon_chip && attr == hwmon_chip_update_interval) {
>>> +        *val = 60000;
>>> +        return 0;
>>> +    }
>>
>> It is not the purpose of the update_interval attribute to inform the
>> user what the update interval of this chip happens to be. The purpose
>> of the attribute is to inform the chip what update interval it should use.
> 
> Well, I think it's a completely natural thing to expose a fixed and
> known update_interval as a 0444 property, and it might even be useful to
> userspace to know that there's no point reading the sensor any more
> often than that. And I didn't come up with this by myself, there's
> already at least a couple of instances of a 0444 update_interval.
> 

That doesn't make it better. It is still an abuse of the ABI.

Guenter