On Thu, Mar 18, 2021 at 08:15:06PM -0300, Jonas Malaco wrote: [ ... ] > > 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. > > > > > + 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. > > > > 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
