Re: [PATCH v1 4/7] rtc-rv8803: add tamper function to sysfs for rv8901

[Krzysztof Kozlowski]

On Fri, Jan 10, 2025 at 07:13:58AM +0100, Markus Burri wrote:
> +
> +static ssize_t enable_store(struct device *dev, struct device_attribute *attr, const char *buf,
> +			    size_t count)
> +{
> +	int ret;
> +	int i;
> +	unsigned long tmo;
> +	u8 reg;
> +	u8 reg_mask;
> +	struct rv8803_data *rv8803 = dev_get_drvdata(dev->parent);
> +	struct i2c_client *client = rv8803->client;
> +
> +	/* EVINxCPEN | EVINxEN */;
> +	const u8 reg_mask_evin_en = GENMASK(5, 3) | GENMASK(2, 0);
> +
> +	bool enable = (strstr(buf, "1") == buf) ? true : false;
> +
> +	guard(mutex)(&rv8803->flags_lock);


That's absolutely huge guard. Isn't this supposed to protect only flags?
Not all register writes?

> +
> +	/* check if event detection status match requested mode */
> +	ret = rv8803_read_reg(client, RX8901_EVIN_EN);
> +	if (ret < 0)
> +		return ret;

...

> +	/* re-enable interrupts */
> +	ret = rv8803_read_reg(client, RV8803_CTRL);
> +	if (ret < 0)
> +		return ret;
> +	ret = rv8803_write_reg(client, RV8803_CTRL, ret | RV8803_CTRL_EIE);
> +	if (ret < 0)
> +		return ret;
> +
> +	return offset;
> +}
> +
> +static DEVICE_ATTR_WO(enable);

You need to document the ABI.

Best regards,
Krzysztof