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@xxxxxxxxxx) kirjoitti: > > > > > > > > On Thu, Apr 06, 2023 at 11:00:02AM +0300, Matti Vaittinen wrote: > > > > > ke 5. huhtik. 2023 klo 18.19 Mark Brown (broonie@xxxxxxxxxx) 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
