lkml.org 
[lkml]   [2021]   [Mar]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] hwmon: add driver for NZXT Kraken X42/X52/X62/X72
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

\
 
 \ /
  Last update: 2021-03-19 01:18    [W:0.044 / U:1.148 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site