Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Mon, 10 Apr 2023 10:47:18 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Matti Vaittinen <mazziesaccount@gmail.com>
Cc:     Mark Brown <broonie@kernel.org>,
        Naresh Solanki <naresh.solanki@9elements.com>,
        linux-hwmon@vger.kernel.org, Jean Delvare <jdelvare@suse.com>,
        Patrick Rudolph <patrick.rudolph@9elements.com>,
        linux-kernel@vger.kernel.org, Sascha Hauer <sha@pengutronix.de>,
        jerome Neanne <jneanne@baylibre.com>,
        "Mutanen, Mikko" <Mikko.Mutanen@fi.rohmeurope.com>
Subject: Re: [PATCH v2 2/3] hwmon: (pmbus/core): Add regulator event support
Message-ID: <b49cc518-634e-4812-ae4a-ee6ec45b42f2@roeck-us.net>
References: <c88d3cdd-fb2f-c3ac-a9e8-e49f8e98b811@gmail.com>
 <17934bff-f728-d57a-c3c8-956634bd48c8@roeck-us.net>
 <3be67394-6082-1aeb-8a8d-90149217bdc7@gmail.com>
 <aea044ab-3a83-2369-aff7-5ef153618619@roeck-us.net>
 <0672fe4d-7293-4374-9186-29b008e5f8a2@sirena.org.uk>
 <CANhJrGO3X7pSsMBg6Gtf-q3=_JiCX4Qs=pGudL=etooM2F676g@mail.gmail.com>
 <d6a3ca82-7245-45e1-b8ff-a9970671b04f@sirena.org.uk>
 <CANhJrGMkwi1TVW_wGw=Boj1vRO_wGrd9=atOxKfbbdM4cwPGsw@mail.gmail.com>
 <7c71d182-2ac5-b01e-2875-5060fb509056@roeck-us.net>
 <65e06627-c368-d081-e18f-a45f245c37ab@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <65e06627-c368-d081-e18f-a45f245c37ab@gmail.com>
Precedence: bulk

On Mon, Apr 10, 2023 at 07:53:46PM +0300, Matti Vaittinen wrote:
> On 4/10/23 17:40, Guenter Roeck wrote:
> > On 4/10/23 01:19, Matti Vaittinen wrote:
> > > to 6. huhtik. 2023 klo 16.43 Mark Brown (broonie@kernel.org) kirjoitti:
> > > > 
> > > > On Thu, Apr 06, 2023 at 11:00:02AM +0300, Matti Vaittinen wrote:
> > > > > ke 5. huhtik. 2023 klo 18.19 Mark Brown (broonie@kernel.org) kirjoitti:
> > > > > > On Wed, Apr 05, 2023 at 07:18:32AM -0700, Guenter Roeck wrote:
> > > 
> > > > > > It can also try to avoid
> > > > > > interacting with hardware if that might not work.
> > > > 
> > > > > It'd be great to have documentation / specification for sending and/or
> > > > > handling the regulator events. I don't think we currently have such.
> > > > > As far as I understand, the notifications can be picked up by all
> > > > > consumers of a regulator. I am a bit worried about:
> > > > > a) Situations where notification handlers 'collide' by doing 'actions'
> > > > > which are unexpected by other handlers
> > > > 
> > > > I'm not sure what you're expecting there?? A device working with itself
> > > > shouldn't disrupt any other users.
> > > 
> > > I have no concrete idea, just a vague uneasy feeling knowing that
> > > devices tend to interact with each other. I guess it is more about the
> > > amount of uncertainty caused by my lack of knowledge regarding what
> > > could be done by these handlers. So, as I already said - if no one
> > > else is bothered by this then I definitely don't want to block the
> > > series. Still, if the error handling should be kept internal to PMBus
> > > - then we should probably either say that consumer drivers must not
> > > (forcibly) turn off the supply when receiving these notifications - or
> > > not send these notifications from PMBus and allow PMBus to decide
> > > error handling internally. (Again, I don't know if any in-tree
> > > consumer drivers do turn off the supply regulator in error handlers -
> > > but I don't think it is actually forbidden). Or am I just making? a
> > > problem that does not exist?
> > > 
> > 
> > For my part I (still) don't understand why this is considered a problem
> > for this driver but not for all the other drivers reporting various
> > error conditions to the regulator subsystem. At least some of them
> > also have programmable reaction to such error conditions.
> 
> I may not know the drivers you're referring to. And, as I said, maybe there
> is no problem.
> 
> To explain why I asked this question for this driver:
> 
> What caught my attention was use of the REGULATOR_EVENT_*_WARN flags, which
> were originally added so regulators could be flagging 'not yet
> critical'-errors Vs. the other, older REGULATOR_EVENT_* flags. From the
> discussions I have understood the older flags were informing severe hardware
> failures where system is typically thought to be no longer usable. I have
> understood that the most likely handling for such notification is shutting
> off the regulator. I have further understood that there are not many
> consumers doing handling of these events. (This is all just my understanding
> based on discussions - take it with grain of salt).
> 
> I was the one who added these warning level notifications. Thus, I have been
> following (only) use of those warnings. I have no proper insight to existing
> drivers using all these flags.
> 
> When grepping for the WARNING level regulator events I can find following
> drivers:
> drivers/regulator/bd9576-regulator.c
> drivers/regulator/max597x-regulator.c
> drivers/regulator/tps65219-regulator.c
> 
> The difference (in my head) between these and PMBus-core is that these are
> very specific PMIC ICs. The board designer should have a good understanding
> which of the power-rails may have 'warning level' failures, and which errors
> are 'critical'. They should select and set the IRQ limits and error
> severities in the device-tree accordingly.
> 
> PMBus core (again, in my head) is more generic purpose system. This is why I
> originally asked if the 'error severity' in PMBus specifies also the error
> handling - and if these regulator notifications map to intended handling.
> Now, after this discussion I think that:
> 
> PMBus has it's own error-handling which is implemented independently from
> these notifications. This handling should not be messed-up by regulator
> consumers, for example by touching the regulator state.
> 
> This is what made me think sending regulator notifications might not be the
> best approach - (but as I said, I may be wrong. I am no longer sure what
> kind of handling there is expected for these events. Furthermore, as we see
> below, I did not find in-tree consumers taking "radical" actions when
> receiving the notifications).
> 
> Yep, I didn't find other in-tree consumers for these notifications except:
> drivers/usb/host/ohci-da8xx.c
> 
> (I was not thorough so may have missed some, but seems there is not many
> in-tree consumers.)
> 
> I did only a very very brief shallow peek but it seems to me that even there
> driver only sets a flag - which is used in debug message. (I may have missed
> something here as well).
> 
> Judging this it seems to me that, at the moment, these notifications are
> mostly ignored by consumers - and they are sent by only a handful of
> devices. There probably exist some downstream users for those, but I have
> not heard of them. Maybe they are only used on very specific systems. This
> could explain why there has been no problems.
> 
> I know, I know. Lot's of guessing, assuming and hand waving. Sorry :/
> 

Oh, now the problem (though I still don't understand what the problem
actually is) is extended to warnings. I thought you were concerned
about errors.

Personally I think you are concerned about a non-issue, but without
explicit guidance from regulator maintainers (and no clear definition if
and when regulator notifications should or may be sent) I won't be able
to apply this series.

Having said this, I find the whole discussion kind of surreal,
especially since the PMBus drivers already report error states to the
regulator subsystem using the get_error_flags callback, but it is what
it is. Also, no, I won't revert that code without a clear explanation
of the _actual_ (not perceived) problem that such a revert is supposed
to solve.

Guenter