linux-hwmon.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Logan Shaw <Logan.Shaw@alliedtelesis.co.nz>
To: "linux@roeck-us.net" <linux@roeck-us.net>,
	"jdelvare@suse.com" <jdelvare@suse.com>
Cc: Joshua Scott <Joshua.Scott@alliedtelesis.co.nz>,
	"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>
Subject: Re: [PATCH v2 2/2] hwmon: (adt7475) Added attenuator bypass support
Date: Fri, 27 Dec 2019 02:53:16 +0000	[thread overview]
Message-ID: <804d10c6a459b9d7fa02d1e7ba08369c1c95ec87.camel@alliedtelesis.co.nz> (raw)
In-Reply-To: <e88b029b-dbb3-e423-5c37-0917d7716b66@roeck-us.net>

On Wed, 2019-12-18 at 19:53 -0800, Guenter Roeck wrote:
> On 12/18/19 7:32 PM, Logan Shaw wrote:
> > Added a new file documenting the adt7475 devicetree and added the
> > five
> > new properties to it.
> > 
> > Signed-off-by: Logan Shaw <logan.shaw@alliedtelesis.co.nz>
> > ---
> > ---
> >   .../devicetree/bindings/hwmon/adt7475.txt     | 35
> > +++++++++++++++++++
> >   1 file changed, 35 insertions(+)
> >   create mode 100644
> > Documentation/devicetree/bindings/hwmon/adt7475.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.txt
> > b/Documentation/devicetree/bindings/hwmon/adt7475.txt
> > new file mode 100644
> > index 000000000000..388dd718a246
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/adt7475.txt
> > @@ -0,0 +1,35 @@
> > +*ADT7475 hwmon sensor.
> > +
> > +Required properties:
> > +- compatible: One of
> > +	"adi,adt7473"
> > +	"adi,adt7475"
> > +	"adi,adt7476"
> > +	"adi,adt7490"
> > +
> > +- reg: I2C address
> > +
> > +optional properties:
> > +
> > +- bypass-attenuator-all: Configures bypassing all voltage input
> > attenuators.
> > +	This is only supported on the ADT7476 and ADT7490, this
> > property does
> > +	nothing on other chips.
> 
> Both adt7473 and adt7475 do support configuring an attenuator on VCCP
> 

That is true, but as the function of register 0x73 bit 5 differs
between the two set of hardware ({adt7473, adt7475} and {adt7476,
adt7490}) a solution which allows bypassing VCCP on both gets more
complicated which I was trying to avoid.

Is it acceptable to split the function
load_individual_bypass_attenuators into two, one for each set of
chips, and call the appropriate function for the chip? That way adding
"bypass-attenuator-in1" to any of the four adt chips DTS will result in
the attenuator for VCCP being bypassed (albeit by setting different
bits depending on the specific bit).

> > +		property present: Bit set to bypass all voltage input
> > attenuators.
> > +		property absent: Registers left unchanged.
> > +
> > +- bypass-attenuator-inx: Configures bypassing individual voltage
> > input
> > +	attenuators, where x is an integer from the set {0, 1, 3, 4}.
> > This
> > +	is only supported on the ADT7476 and ADT7490, this property
> > does nothing
> > +	on other chips.
> > +		property present: Bit set to bypass specific voltage
> > input attenuator
> > +			for voltage input x.
> > +		property absent: Registers left unchanged.
> > +
> 
> This is interesting. It essentially means "retain configuration from
> ROM
> Monitor", but leaves no means to _disable_ the bypass.
> 

Only a power cycle (after removing the properties from the DTS) will
set the affected bits back to 0. Removing the properties, but only
softly restarting the system (no interrupted power supply to the adt
chip), will not set the bits back to 0.

I decided against setting the bits to 0 by default (if no property was
present) as doing so might break compatibility with systems that expect
the bits to remain unchanged. 

A compromise would be to change the "bypass-attenuator-inx" property to
"bypass-attenuator-inx = <y>", where y = 1 bypasses the attenuator and
y = 0 does not. If the property is not present then the register is
left unchanged. It would make sense to do the same to the "bypass-
attenuator-all" property. Would these changes be acceptable?

> > +Example:
> > +
> > +hwmon@2e {
> > +	compatible = "adi,adt7475";
> > +	reg = <0x2e>;
> > +	bypass-attenuator-all;
> > +	bypass-attenuator-in1;
> 
> What would be the purpose of specifying both all and in1 ?

There would be no practical purpose, it was to keep the example
compact. Instead "bypass-attenuator-in1" could be removed and added to 
second device hwmon@2d. This would show off the syntax for each set of
properties in a more practical way.

> 
> > +};
> > \ No newline at end of file
> > 
> 
> 

  reply	other threads:[~2019-12-27  2:53 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-19  3:32 [PATCH v2 0/2] " Logan Shaw
2019-12-19  3:32 ` [PATCH v2 1/2] " Logan Shaw
2019-12-19  3:32 ` [PATCH v2 2/2] " Logan Shaw
2019-12-19  3:53   ` Guenter Roeck
2019-12-27  2:53     ` Logan Shaw [this message]
2020-01-09  3:07       ` Logan Shaw
2020-01-09 23:03       ` Guenter Roeck
2020-01-13  0:08     ` Chris Packham

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=804d10c6a459b9d7fa02d1e7ba08369c1c95ec87.camel@alliedtelesis.co.nz \
    --to=logan.shaw@alliedtelesis.co.nz \
    --cc=Joshua.Scott@alliedtelesis.co.nz \
    --cc=jdelvare@suse.com \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --subject='Re: [PATCH v2 2/2] hwmon: (adt7475) Added attenuator bypass support' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).