Re: [PATCH 5/7] regulator: sy7636a: report the temperature

[Pablo Mazzini]

Hi Guenter, Andreas,

On Mon, Oct 26, 2026 at 07:35:24AM -0700, Guenter Roeck wrote:
> This is going the wrong way. The correct solution would be to convert
> the hwmon driver to an auxiliary driver.

Understood. In v2 the hwmon driver stays in drivers/hwmon and becomes an
auxiliary driver of the regulator, which binds the I2C device directly.

On Tue, 27 Oct 2026 00:26:35 +0100, Andreas Kemnade wrote:
> 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.

Good points. For v2 I read power good (0x07) and the temperature (0x08)
in one I2C transfer, and return -ENODATA unless power good is set, so no
locking is needed.

Thanks,
Pablo

On Mon, Oct 26, 2026 at 11:26 PM Andreas Kemnade <andreas@xxxxxxxxxxxx> wrote:
>
> On Mon, 26 Oct 2026 07:35:24 -0700
> Guenter Roeck <linux@xxxxxxxxxxxx> 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
>