All of lore.kernel.org
 help / color / mirror / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: Mark Brown <broonie@kernel.org>, atull@opensource.altera.com
Cc: jdelvare@suse.de, lm-sensors@lm-sensors.org, lgirdwood@gmail.com,
	linux-kernel@vger.kernel.org, delicious.quinoa@gmail.com,
	dinguyen@opensource.altera.com, yvanderv@opensource.altera.com
Subject: Re: [lm-sensors] [PATCH v2 2/2] pmbus: ltc2978: add regulator gating
Date: Sat, 23 Aug 2014 15:10:16 +0000	[thread overview]
Message-ID: <53F8AED8.8010908@roeck-us.net> (raw)
In-Reply-To: <20140822214546.GD24407@sirena.org.uk>

On 08/22/2014 02:45 PM, Mark Brown wrote:
> On Fri, Aug 22, 2014 at 04:11:34PM -0500, atull@opensource.altera.com wrote:
>> From: Alan Tull <atull@opensource.altera.com>
>>
>> Add regulator with support for enabling or disabling all
>> supplies.
>
> Reviwed-by: Mark Brown <broonie@linaro.org>
>
> though it still looks like you should be able to create generic
> functions for the operations.
>
Sorry I didn't have time to review the code myself. I'll have
to check the datasheet about turning regulators on and off.
Using page 0xff for the lm2978 looks wrong, as the chip supports
up to 8 channels which should be controlled separately
(I would assume) instead of turning them all on and off in
one go. Maybe I am missing something, but my assumption would
have been to have a separate regulator for each channel, and
that each channel would have its own regulator which would be
turned on and off separately. So I don't really understand the
change between v1 and v2 of the core patch, which dropped the
per-channel regulators. Someone will have to explain to me
why that makes sense, especially since it means that I won't
be able to use the regulator expansion in my system (which
would require per-channel regulators, and which does not always
have all channels enabled on a given chip).

In respect to generic functions, that really depends on the scope
of the regulators. As written, where all regulators are turned on
in a single operation, per-chip functions are needed. I thought
we would only need per-chip configuration values, but that only
applies if regulators are turned on one by one, not all in one go.

Either case, a wrapper for ltc2978_write_pmbus_operation needs to
be added to pmbus_core.c as pmbus_write_byte_data and exported
(as a separate patch). For paged reads, the existing
pmbus_read_byte_data should be used. In general, avoid direct
accesses to paged registers and use API functions instead
if possible.

Thanks,
Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

WARNING: multiple messages have this Message-ID (diff)
From: Guenter Roeck <linux@roeck-us.net>
To: Mark Brown <broonie@kernel.org>, atull@opensource.altera.com
Cc: jdelvare@suse.de, lm-sensors@lm-sensors.org, lgirdwood@gmail.com,
	linux-kernel@vger.kernel.org, delicious.quinoa@gmail.com,
	dinguyen@opensource.altera.com, yvanderv@opensource.altera.com
Subject: Re: [PATCH v2 2/2] pmbus: ltc2978: add regulator gating
Date: Sat, 23 Aug 2014 08:10:16 -0700	[thread overview]
Message-ID: <53F8AED8.8010908@roeck-us.net> (raw)
In-Reply-To: <20140822214546.GD24407@sirena.org.uk>

On 08/22/2014 02:45 PM, Mark Brown wrote:
> On Fri, Aug 22, 2014 at 04:11:34PM -0500, atull@opensource.altera.com wrote:
>> From: Alan Tull <atull@opensource.altera.com>
>>
>> Add regulator with support for enabling or disabling all
>> supplies.
>
> Reviwed-by: Mark Brown <broonie@linaro.org>
>
> though it still looks like you should be able to create generic
> functions for the operations.
>
Sorry I didn't have time to review the code myself. I'll have
to check the datasheet about turning regulators on and off.
Using page 0xff for the lm2978 looks wrong, as the chip supports
up to 8 channels which should be controlled separately
(I would assume) instead of turning them all on and off in
one go. Maybe I am missing something, but my assumption would
have been to have a separate regulator for each channel, and
that each channel would have its own regulator which would be
turned on and off separately. So I don't really understand the
change between v1 and v2 of the core patch, which dropped the
per-channel regulators. Someone will have to explain to me
why that makes sense, especially since it means that I won't
be able to use the regulator expansion in my system (which
would require per-channel regulators, and which does not always
have all channels enabled on a given chip).

In respect to generic functions, that really depends on the scope
of the regulators. As written, where all regulators are turned on
in a single operation, per-chip functions are needed. I thought
we would only need per-chip configuration values, but that only
applies if regulators are turned on one by one, not all in one go.

Either case, a wrapper for ltc2978_write_pmbus_operation needs to
be added to pmbus_core.c as pmbus_write_byte_data and exported
(as a separate patch). For paged reads, the existing
pmbus_read_byte_data should be used. In general, avoid direct
accesses to paged registers and use API functions instead
if possible.

Thanks,
Guenter


  reply	other threads:[~2014-08-23 15:10 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-08-22 21:11 [lm-sensors] [PATCH v2 0/2] dd regulator support for pmbus and ltc2978 atull
2014-08-22 21:11 ` atull
2014-08-22 21:11 ` [lm-sensors] [PATCH v2 1/2] pmbus: add regulator support atull
2014-08-22 21:11   ` atull
2014-08-22 21:44   ` [lm-sensors] " Mark Brown
2014-08-22 21:44     ` Mark Brown
2014-08-23  0:31     ` [lm-sensors] " atull
2014-08-23  0:31       ` atull
2014-08-23 14:00       ` [lm-sensors] " Guenter Roeck
2014-08-23 14:00         ` Guenter Roeck
2014-08-23 14:54         ` [lm-sensors] " Mark Brown
2014-08-23 14:54           ` Mark Brown
2014-08-24  0:48           ` [lm-sensors] " Alan Tull
2014-08-24  0:48             ` Alan Tull
2014-08-22 21:11 ` [lm-sensors] [PATCH v2 2/2] pmbus: ltc2978: add regulator gating atull
2014-08-22 21:11   ` atull
2014-08-22 21:45   ` [lm-sensors] " Mark Brown
2014-08-22 21:45     ` Mark Brown
2014-08-23 15:10     ` Guenter Roeck [this message]
2014-08-23 15:10       ` Guenter Roeck
2014-08-23 15:53       ` [lm-sensors] " Mark Brown
2014-08-23 15:53         ` Mark Brown
2014-08-24 13:30       ` [lm-sensors] " Alan Tull
2014-08-24 13:30         ` Alan Tull
2014-08-25 17:02         ` [lm-sensors] " Guenter Roeck
2014-08-25 17:02           ` Guenter Roeck

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=53F8AED8.8010908@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=atull@opensource.altera.com \
    --cc=broonie@kernel.org \
    --cc=delicious.quinoa@gmail.com \
    --cc=dinguyen@opensource.altera.com \
    --cc=jdelvare@suse.de \
    --cc=lgirdwood@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lm-sensors@lm-sensors.org \
    --cc=yvanderv@opensource.altera.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.