From: Andreas Kemnade <andreas@kemnade.info>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Pablo Mazzini <pmazzini@gmail.com>,
Mark Brown <broonie@kernel.org>,
Liam Girdwood <lgirdwood@gmail.com>, Lee Jones <lee@kernel.org>,
Alistair Francis <alistair@alistair23.me>,
linux-hwmon@vger.kernel.org, mfd@lists.linux.dev,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 5/7] regulator: sy7636a: report the temperature
Date: Tue, 27 Oct 2026 00:26:35 +0100 [thread overview]
Message-ID: <20261027002635.6b831c0b@kemnade.info> (raw)
In-Reply-To: <f68279bc-20f1-41a9-ba5c-92b974785cd8@roeck-us.net>
On Mon, 26 Oct 2026 07:35:24 -0700
Guenter Roeck <linux@roeck-us.net> wrote:
> On Mon, Oct 26, 2026 at 12:39:16AM +0000, Pablo Mazzini wrote:
> > The separate hwmon cell enables VCOM at probe. Report the temperature
> > from here, as the TPS65185 and FP9931 drivers do, and under the chip
> > name as they do: read it while the rails are on and return -ENODATA
> > otherwise. Boards that need the rails on mark VCOM always-on. Drop the
> > hwmon driver.
>
> This is going the wrong way. The correct solution would be to convert
> the hwmon driver to an auxiliary driver. I am not going to approve
> this patch. I am not giving an official NACK (yet).
>
First we need the problem defined, then the solution, and then we can
talk about what is the correct solution. The commit message does not
name a problem.
So what we have:
one bit to turn on a bunch of things,
after one time to sample (< 1ms), fresh values are provided on the ADC
register periodically.
after some more time (max. 50ms), several voltage outputs are ready.
Powering on those things is expensive, so you would not use this as an
ordinary temperature sensor.
The problems I see in the sensor driver this patch might want to solve:
- problems if the regulator driver is registered earlier.
- not having powered on the regulator all the time (that is described
in the cover letter)
The way the patch handles it does not require any structural change,
availabitily of the regmap is given. So it would be a separate cleanup
patch.
But it introduces another race condition.
The temperature might be read too early.
1ms delay should suffice.
The SY7636A_OPERATION_MODE_CRL_ONOFF flag might be reset before
reading the temperature. If that register does not reset to zero
but just keeps the temperature last read, then we have no problem.
If not, there needs to be some synchronisation between the regulator
part and the sensor part, probably a mutex saved in a common structure.
And in that case, structural change of the drivers is needed which
I see the following possibilities:
a) non-simple mfd driver
b) aux device as a child of the regulator driver.
c) regulator driver containing everything.
No strong personal opinion here.
Regards,
Andreas
next prev parent reply other threads:[~2026-10-26 23:26 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-26 0:39 [PATCH 0/7] sy7636a: power the panel only during updates Pablo Mazzini
2026-10-26 0:39 ` [PATCH 1/7] ARM: dts: imx: keep the SY7636A VCOM on Pablo Mazzini
2026-10-26 7:30 ` Andreas Kemnade
2026-10-26 8:21 ` Pablo Mazzini
2026-10-26 0:39 ` [PATCH 2/7] regulator: sy7636a: fix the chip name Pablo Mazzini
2026-10-26 7:39 ` Andreas Kemnade
2026-10-26 0:39 ` [PATCH 3/7] regulator: sy7636a: add VCOM voltage control Pablo Mazzini
2026-10-26 7:41 ` Andreas Kemnade
2026-10-26 0:39 ` [PATCH 4/7] regulator: sy7636a: wait for power good, then switch VCOM_EN Pablo Mazzini
2026-10-26 8:10 ` Andreas Kemnade
2026-10-26 8:44 ` Pablo Mazzini
2026-10-26 0:39 ` [PATCH 5/7] regulator: sy7636a: report the temperature Pablo Mazzini
2026-10-26 8:42 ` Andreas Kemnade
2026-10-26 14:35 ` Guenter Roeck
2026-10-26 23:26 ` Andreas Kemnade [this message]
2026-10-27 7:32 ` Pablo Mazzini
2026-10-27 9:18 ` Andreas Kemnade
2026-10-26 0:39 ` [PATCH 6/7] regulator: sy7636a: bind to the I2C device directly Pablo Mazzini
2026-10-26 0:39 ` [PATCH 7/7] ARM: imx_v6_v7_defconfig: drop the SY7636A MFD and hwmon options Pablo Mazzini
2026-10-26 9:37 ` [PATCH 0/7] sy7636a: power the panel only during updates Andreas Kemnade
2026-10-26 11:28 ` Pablo Mazzini
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=20261027002635.6b831c0b@kemnade.info \
--to=andreas@kemnade.info \
--cc=alistair@alistair23.me \
--cc=broonie@kernel.org \
--cc=lee@kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=mfd@lists.linux.dev \
--cc=pmazzini@gmail.com \
/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
all inboxes | Powered by JetHome®