Re: [PATCH v3 2/3] rtc: implement a sysfs interface for clock offset

[Alexandre Belloni]

A really small nitpick below

On 03/02/2016 at 09:16:43 -0800, Joshua Clayton wrote :
> clock offset may be set and read in decimal parts per billion
> attribute is /sys/class/rtc/rtcN/offset
> The attribute is only visible for rtcs that have set_offset implemented.
> 
> Signed-off-by: Joshua Clayton <stillcompiling@xxxxxxxxx>
> ---
>  Documentation/rtc.txt   |  6 ++++++
>  drivers/rtc/rtc-sysfs.c | 35 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/rtc.txt b/Documentation/rtc.txt
> index 8446f1e..ddc3660 100644
> --- a/Documentation/rtc.txt
> +++ b/Documentation/rtc.txt
> @@ -157,6 +157,12 @@ wakealarm:	 The time at which the clock will generate a system wakeup
>  		 the epoch by default, or if there's a leading +, seconds in the
>  		 future, or if there is a leading +=, seconds ahead of the current
>  		 alarm.
> +offset:		 The amount which the rtc clock has been adjusted in firmware.
> +		 Visible only if the driver supports clock offset adjustment.
> +		 The unit is parts per billion, i.e. The number of clock ticks
> +		 which are added to or removed from the rtc's base clock per
> +		 billion ticks. A positive value makes a day pass more slowly,
> +		 longer, and a negative value makes a day pass more quickly.
>  
>  IOCTL INTERFACE
>  ---------------
> diff --git a/drivers/rtc/rtc-sysfs.c b/drivers/rtc/rtc-sysfs.c
> index 463e286..f8d7f8d 100644
> --- a/drivers/rtc/rtc-sysfs.c
> +++ b/drivers/rtc/rtc-sysfs.c
> @@ -218,6 +218,34 @@ wakealarm_store(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RW(wakealarm);
>  
> +static ssize_t
> +offset_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	ssize_t retval;
> +	long offset;
> +
> +	retval = rtc_read_offset(to_rtc_device(dev), &offset);
> +	if (retval == 0)
> +		retval = sprintf(buf, "%ld\n", offset);
> +
> +	return retval;
> +}
> +
> +static ssize_t
> +offset_store(struct device *dev, struct device_attribute *attr,
> +		const char *buf, size_t n)

This is not properly aligned.

> +{
> +	ssize_t retval;
> +	long offset;
> +
> +	retval = kstrtol(buf, 10, &offset);
> +	if (retval == 0)
> +		retval = rtc_set_offset(to_rtc_device(dev), offset);
> +
> +	return (retval < 0) ? retval : n;
> +}
> +static DEVICE_ATTR_RW(offset);
> +
>  static struct attribute *rtc_attrs[] = {
>  	&dev_attr_name.attr,
>  	&dev_attr_date.attr,
> @@ -226,6 +254,7 @@ static struct attribute *rtc_attrs[] = {
>  	&dev_attr_max_user_freq.attr,
>  	&dev_attr_hctosys.attr,
>  	&dev_attr_wakealarm.attr,
> +	&dev_attr_offset.attr,
>  	NULL,
>  };
>  
> @@ -249,9 +278,13 @@ static umode_t rtc_attr_is_visible(struct kobject *kobj,
>  	struct rtc_device *rtc = to_rtc_device(dev);
>  	umode_t mode = attr->mode;
>  
> -	if (attr == &dev_attr_wakealarm.attr)
> +	if (attr == &dev_attr_wakealarm.attr) {
>  		if (!rtc_does_wakealarm(rtc))
>  			mode = 0;
> +	} else if (attr == &dev_attr_offset.attr) {
> +		if (!rtc->ops->set_offset)
> +			mode = 0;
> +	}
>  
>  	return mode;
>  }
> -- 
> 2.5.0
> 

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