On Thu, Mar 18, 2021 at 08:15:06PM -0300, Jonas Malaco wrote:

[ ... ]

quoted

Either case, the spinlocks are overkill. It would be much easier to
convert raw readings here into temperature and fan speed and store
the resulting values in struct kraken2_priv_data, and then to
just report it in the read functions. That would be much less costly
because the spinlock would not be needed at all, and calculations
would be done only once per event.

Oddly enough, this is very close to how the code read last week.

But I was storing the values in kraken2_priv_data as longs, and I'm not
sure that storing and loading longs is atomic on all architectures.

Was that safe, or should I use something else instead of longs?

Hard to say, but do you see any code in the kernel that assumes
that loading or storing a long is not atomic, for any architecture ?

Also, I don't see how u16 * 1000 could ever require a long
for storage. int would be good enough.

quoted

quoted

+	memcpy(priv->status, data, STATUS_USEFUL_SIZE);
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return 0;
+}

For my education: What triggers those events ? Are they reported
by the hardware autonomously whenever something changes ?
A comment at the top of the driver explaining how this works
might be useful.

The device autonomously sends these status reports twice a second.

I'll add the comment for v2.

That would be great, thanks.

quoted

Also, is there a way to initialize values during probe ? Otherwise
the driver would report values of 0 until the hardware reports
something.

The device doesn't respond to HID Get_Report, so we can't get valid
initial values.

The best we can do is identify the situation and report it to
user-space.  Am I right that ENODATA should be used for this?

Yes, I think that would be a good idea, and ENODATA sounds good.

Thanks,
Guenter