On Thu, Mar 18, 2021 at 04:36:08PM -0700, Guenter Roeck wrote: > 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 ? No, I don't think so. > > Also, I don't see how u16 * 1000 could ever require a long > for storage. int would be good enough. Oh, that's true. I'll submit a v2 shortly. Thanks again, Jonas > > > > > > > > + 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
