Re: [PATCH 4/5] hwmon: (pmbus/core) improve handling of write protected regulators

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 23, 2024 at 06:53:07PM +0200, Jerome Brunet wrote:
> On Mon 23 Sep 2024 at 14:21, Mark Brown <broonie@xxxxxxxxxx> wrote:

> > So, I know we talked about this a bit on IRC but I didn't register the
> > specific use case.  Now I see that it's coming down to the fact that the
> > chip is simply write protected I'm wondering if it might not be simpler
> > to handle this at the ops level rather than the constraints level.  When
> > registering the driver could check for write protection and then instead
> > of registering the normal operations register an alternative set with
> > the relevant write operations removed.  That would have the same effect
> > that you're going for AFAICT.  Sorry for not thinking of it earlier.

> Actually I thaught about it, that was my first idea.

> There is tiny difference between the 2 approach:
> * A regulator that does not provide enable()/disable() is considered
> always-on, so it is not really checked if it is enabled or not
> * A regulator that does provide enable()/disable() but disallow status
> change will be checked with is_enabled()

> In the second case, we will pick up on regulator that is disabled and we
> can't enable. In the first case, I don't think we do. I don't know if it
> is a bug of not but that makes the 2nd case more correct for what we do
> with pmbus regs I think

I think for that we should just always use the is_enabled() operation if
it's available.  This is simply an oversight caused by not imagining a
case where a regulator could have an enable control which is locked out
like this, the normal case is that either you can control enable or
the regulator is always on.

> The other thing, although is more a pmbus implemantion consideration,
> is that the type regulator is opaque in
> pmbus_regulator_register. Different types could be registered so
> manipulating the ops is tricky. That's where a callback is helpful 

> If building the ops dynamically is the preferred way, I'll find a way to
> make it happen. I'll need to way to identify which one need it, loose
> the 'const' qualifier in a lot of place, etc ... that will be a bit
> hackish I'm afraid.

With only a limited set of options it might be better to just have a set
of static structs and pick one to register (some other drivers do this
based on hardware options), but the number of combinations with this is
getting a bit big for that.

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux