Re: [PATCH v9 1/3] pps: generators: Add PPS Generator TIO Driver

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, Jun 05 2024 at 21:05, lakshmi.sowjanya.d@intel.com wrote:
> +static ssize_t enable_store(struct device *dev, struct device_attribute *attr, const char *buf,
> +			    size_t count)
> +{
> +	struct pps_tio *tio = dev_get_drvdata(dev);
> +	bool enable;
> +	int err;
> +
> +	if (!timekeeping_clocksource_has_base(CSID_X86_ART)) {
> +		dev_err(dev, "PPS cannot be used as clock is not related to ART");

                dev_err_once() if at all

> +		return -EPERM;

Why -EPERM? This has nothing to do with permissions.

ENODEV or ENOTSUPPORTED perhaps.

> +static ssize_t enable_show(struct device *dev, struct device_attribute *devattr, char *buf)
> +{
> +	struct pps_tio *tio = dev_get_drvdata(dev);
> +	u32 ctrl;
> +
> +	ctrl = pps_tio_read(tio, TIOCTL);
> +	ctrl &= TIOCTL_EN;

Why reading the hardware instead of simply using tio->enabled?

> +
> +	return sysfs_emit(buf, "%u\n", ctrl);
> +}

Thanks,

        tglx