From: Guenter Roeck <linux@roeck-us.net>
To: Jonas Malaco <jonas@protocubo.io>
Cc: linux-hwmon@vger.kernel.org, Jean Delvare <jdelvare@suse.com>,
linux-input@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] hwmon: add driver for NZXT Kraken X42/X52/X62/X72
Date: Thu, 18 Mar 2021 16:36:08 -0700 [thread overview]
Message-ID: <20210318233608.GA140894@roeck-us.net> (raw)
In-Reply-To: <20210318231506.v4dvz4jtvqazdoo2@calvin.localdomain>
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
next prev parent reply other threads:[~2021-03-18 23:37 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-18 16:48 [PATCH] hwmon: add driver for NZXT Kraken X42/X52/X62/X72 Jonas Malaco
2021-03-18 18:55 ` Guenter Roeck
2021-03-18 23:15 ` Jonas Malaco
2021-03-18 23:36 ` Guenter Roeck [this message]
2021-03-19 0:16 ` Jonas Malaco
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210318233608.GA140894@roeck-us.net \
--to=linux@roeck-us.net \
--cc=jdelvare@suse.com \
--cc=jonas@protocubo.io \
--cc=linux-doc@vger.kernel.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).