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
