All of lore.kernel.org
 help / color / mirror / Atom feed
From: u.kleine-koenig@pengutronix.de (Uwe Kleine-König)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 2/4] mfd: mc13783: When probing, unlock the mc13783 before subsystems initialisation.
Date: Sun, 20 Dec 2009 19:49:42 +0100	[thread overview]
Message-ID: <20091220184941.GA13294@pengutronix.de> (raw)
In-Reply-To: <1261316918.5224.15.camel@climbing-alby>

Hi Alberto,

On Sun, Dec 20, 2009 at 02:48:38PM +0100, Alberto Panizzo wrote:
> > > PATCH 1 & 2 are fixes that can go to .33
> > I don't like patch 1.  I'd prefer that drivers touching
> > MC13783_REG_POWER_MISCELLANEOUS are aware of the bit in question and
> > wouldn't rely on mc13783-core.
> 
> Yes, but MC13783_REG_POWER_MISCELLANEOUS contains bit that control
> different aspect of mc13783 chip.
> GPO are regulator related, but those two bits in question maybe 
> apply to a power management driver, so this problem is a matter
> of mc13783-core.
maybe?
 
> Another possible solution, is to trace the writings to those two bits
> in mc13783_reg_rmw storing the value written an reproducing it in next
> mc13783_reg_rmw calls.
> But the problem for this is that we don't know if the bootloader 
> had initialised those with another non default value.
> Another problem is that if another driver make use of 
> mc13783_reg_write for modifying those bits, the state stored will
> be inconsistent.
The next best thing I would consider acceptable are dedicated functions
for MC13783_REG_POWER_MISCELLANEOUS.  I havn't checked what the register
in question is used for, but I think the bootloader isn't generally a
problem as Linux shouldn't rely on things setup by the bootloader (apart
from the things described in
http://www.arm.linux.org.uk/developer/booting.php of course).  And I
don't see a problem for in-kernel users of mc13783_reg_write to modify
the register.  If the mc13783-API is changed that
MC13783_REG_POWER_MISCELLANEOUS must only be modified by using (say)
mc13783_powermisc_rmw() it's a (probably uncatched) bug to use
mc13783_reg_write.

And patch 1 is definitly *not* material for .33, as there is currently
no user of MC13783_REG_POWER_MISCELLANEOUS in .33-rc1, so there is
nothing to fix.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

WARNING: multiple messages have this Message-ID (diff)
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Alberto Panizzo <maramaopercheseimorto@gmail.com>
Cc: Samuel Ortiz <sameo@linux.intel.com>,
	Sascha linux-arm <s.hauer@pengutronix.de>,
	Mark Brown <broonie@opensource.wolfsonmicro.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	linux-arm-kernel-infradead <linux-arm-kernel@lists.infradead.org>,
	Liam Girdwood <lrg@slimlogic.co.uk>
Subject: Re: [PATCH 2/4] mfd: mc13783: When probing, unlock the mc13783 before subsystems initialisation.
Date: Sun, 20 Dec 2009 19:49:42 +0100	[thread overview]
Message-ID: <20091220184941.GA13294@pengutronix.de> (raw)
In-Reply-To: <1261316918.5224.15.camel@climbing-alby>

Hi Alberto,

On Sun, Dec 20, 2009 at 02:48:38PM +0100, Alberto Panizzo wrote:
> > > PATCH 1 & 2 are fixes that can go to .33
> > I don't like patch 1.  I'd prefer that drivers touching
> > MC13783_REG_POWER_MISCELLANEOUS are aware of the bit in question and
> > wouldn't rely on mc13783-core.
> 
> Yes, but MC13783_REG_POWER_MISCELLANEOUS contains bit that control
> different aspect of mc13783 chip.
> GPO are regulator related, but those two bits in question maybe 
> apply to a power management driver, so this problem is a matter
> of mc13783-core.
maybe?
 
> Another possible solution, is to trace the writings to those two bits
> in mc13783_reg_rmw storing the value written an reproducing it in next
> mc13783_reg_rmw calls.
> But the problem for this is that we don't know if the bootloader 
> had initialised those with another non default value.
> Another problem is that if another driver make use of 
> mc13783_reg_write for modifying those bits, the state stored will
> be inconsistent.
The next best thing I would consider acceptable are dedicated functions
for MC13783_REG_POWER_MISCELLANEOUS.  I havn't checked what the register
in question is used for, but I think the bootloader isn't generally a
problem as Linux shouldn't rely on things setup by the bootloader (apart
from the things described in
http://www.arm.linux.org.uk/developer/booting.php of course).  And I
don't see a problem for in-kernel users of mc13783_reg_write to modify
the register.  If the mc13783-API is changed that
MC13783_REG_POWER_MISCELLANEOUS must only be modified by using (say)
mc13783_powermisc_rmw() it's a (probably uncatched) bug to use
mc13783_reg_write.

And patch 1 is definitly *not* material for .33, as there is currently
no user of MC13783_REG_POWER_MISCELLANEOUS in .33-rc1, so there is
nothing to fix.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

  reply	other threads:[~2009-12-20 18:49 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-12-14 16:41 [PATCH 0/4 v2] Patch series for introduce voltage selecting for mc13783 regulators Alberto Panizzo
2009-12-14 16:41 ` Alberto Panizzo
2009-12-14 17:12 ` [PATCH 1/4] mfd: mc13783: Take care of semantic inversion between read and write value of two bits in POWER_MISCELLANEUS register Alberto Panizzo
2009-12-14 17:12   ` Alberto Panizzo
2009-12-14 17:18   ` [PATCH 2/4] mfd: mc13783: When probing, unlock the mc13783 before subsystems initialisation Alberto Panizzo
2009-12-14 17:18     ` Alberto Panizzo
2009-12-14 17:26     ` [PATCH 3/4] regulator: add voltage selection capability to mc13783 regulators v2 Alberto Panizzo
2009-12-14 17:26       ` Alberto Panizzo
2009-12-14 17:53       ` [PATCH 4/4] regulator: mc13783-regulator: correct the probing time Alberto Panizzo
2009-12-14 17:53         ` Alberto Panizzo
2009-12-15 14:50         ` Liam Girdwood
2009-12-15 14:50           ` Liam Girdwood
2009-12-15 14:52         ` Mark Brown
2009-12-15 14:52           ` Mark Brown
2009-12-15 14:54       ` [PATCH 3/4] regulator: add voltage selection capability to mc13783 regulators v2 Liam Girdwood
2009-12-15 14:54         ` Liam Girdwood
2009-12-18 16:12     ` [PATCH 2/4] mfd: mc13783: When probing, unlock the mc13783 before subsystems initialisation Alberto Panizzo
2009-12-18 16:12       ` Alberto Panizzo
2009-12-19 20:18       ` Uwe Kleine-König
2009-12-19 20:18         ` Uwe Kleine-König
2009-12-20 13:48         ` Alberto Panizzo
2009-12-20 13:48           ` Alberto Panizzo
2009-12-20 18:49           ` Uwe Kleine-König [this message]
2009-12-20 18:49             ` Uwe Kleine-König
2009-12-20 22:50             ` Alberto Panizzo
2009-12-20 22:50               ` Alberto Panizzo
2010-01-08 11:13               ` Alberto Panizzo
2010-01-08 11:13                 ` Alberto Panizzo
2010-01-08 11:44                 ` Samuel Ortiz
2010-01-08 11:44                   ` Samuel Ortiz
2009-12-14 17:45   ` [PATCH 1/4] mfd: mc13783: Take care of semantic inversion between read and write value of two bits in POWER_MISCELLANEUS register Sergei Shtylyov
2009-12-14 17:59     ` Alberto Panizzo
2009-12-14 17:59       ` Alberto Panizzo
2010-01-05 18:15       ` Samuel Ortiz
2010-01-05 18:15         ` Samuel Ortiz
2010-01-05 19:55         ` Uwe Kleine-König
2010-01-05 19:55           ` Uwe Kleine-König
2010-01-08 10:53           ` Alberto Panizzo
2010-01-08 10:53             ` Alberto Panizzo
  -- strict thread matches above, loose matches on Subject: below --
2009-12-12 16:37 [PATCH 0/4] Patch series for introduce voltage selecting for mc13783 regulators Alberto Panizzo
2009-12-12 16:48 ` [PATCH 1/4] mfd: mc13783: Take care of semantic inversion between read and write value of two bits in POWER_MISCELLANEUS register Alberto Panizzo
2009-12-12 16:53   ` [PATCH 2/4] mfd: mc13783: When probing, unlock the mc13783 before subsystems initialisation Alberto Panizzo
2009-12-12 16:53     ` Alberto Panizzo
2009-12-13 19:57     ` Uwe Kleine-König
2009-12-13 19:57       ` Uwe Kleine-König

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=20091220184941.GA13294@pengutronix.de \
    --to=u.kleine-koenig@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    /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.