All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lee Jones <lee.jones@linaro.org>
To: Dave Gerlach <d-gerlach@ti.com>
Cc: linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-omap@vger.kernel.org,
	Tony Lindgren <tony@atomide.com>, Mark Brown <broonie@kernel.org>,
	Keerthy <j-keerthy@ti.com>
Subject: Re: [PATCH] mfd: tps65218: Reorder tps65218_regulator_id enum
Date: Wed, 7 Mar 2018 13:00:17 +0000	[thread overview]
Message-ID: <20180307130017.m6swph3mlupu6g76@dell> (raw)
In-Reply-To: <20180216041705.6340-1-d-gerlach@ti.com>

On Thu, 15 Feb 2018, Dave Gerlach wrote:

> Commit 2dc4940360d4 ("regulator: tps65218: Remove all the compatibles")
> changes the probe function of drivers/regulator/tps65218-regulator.c so
> that it iterates through all available regulators and assumes that the
> regulator IDs are sequential and match the order present in the enum
> tps65218_regulator_id. However, for some reason the much older commit
> c0ea88b890d6 ("regulator: tps65218: add support for LS3 current
> regulator") updated all arrays with LS3 at the end but added it second
> to last for the enum.
> 
> Because of this long standing mismatch in order between the
> tps65218_regulator_id enum and the regulator_desc array in the tps65218
> regulator driver, the new probe function causes the strobe values to be
> associated with the wrong regulator ID. This causes LDO1 to fail to
> suspend in tps65218_pmic_set_suspend_disable due to not having anything
> probes for its strobe value. Fix the order in the enum so the probe
> function works as the update intended.

This sounds super fragile.

I'm willing to accept this patch if it 'makes stuff work', but we
should look into why a dynamically assigned enum is sensitive to
ordering.  At the very least you should consider providing a comment
with working to that effect.

> Fixes: 2dc4940360d4 ("regulator: tps65218: Remove all the compatibles")
> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> ---
>  include/linux/mfd/tps65218.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied, thanks.

-- 
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

WARNING: multiple messages have this Message-ID (diff)
From: lee.jones@linaro.org (Lee Jones)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] mfd: tps65218: Reorder tps65218_regulator_id enum
Date: Wed, 7 Mar 2018 13:00:17 +0000	[thread overview]
Message-ID: <20180307130017.m6swph3mlupu6g76@dell> (raw)
In-Reply-To: <20180216041705.6340-1-d-gerlach@ti.com>

On Thu, 15 Feb 2018, Dave Gerlach wrote:

> Commit 2dc4940360d4 ("regulator: tps65218: Remove all the compatibles")
> changes the probe function of drivers/regulator/tps65218-regulator.c so
> that it iterates through all available regulators and assumes that the
> regulator IDs are sequential and match the order present in the enum
> tps65218_regulator_id. However, for some reason the much older commit
> c0ea88b890d6 ("regulator: tps65218: add support for LS3 current
> regulator") updated all arrays with LS3 at the end but added it second
> to last for the enum.
> 
> Because of this long standing mismatch in order between the
> tps65218_regulator_id enum and the regulator_desc array in the tps65218
> regulator driver, the new probe function causes the strobe values to be
> associated with the wrong regulator ID. This causes LDO1 to fail to
> suspend in tps65218_pmic_set_suspend_disable due to not having anything
> probes for its strobe value. Fix the order in the enum so the probe
> function works as the update intended.

This sounds super fragile.

I'm willing to accept this patch if it 'makes stuff work', but we
should look into why a dynamically assigned enum is sensitive to
ordering.  At the very least you should consider providing a comment
with working to that effect.

> Fixes: 2dc4940360d4 ("regulator: tps65218: Remove all the compatibles")
> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> ---
>  include/linux/mfd/tps65218.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied, thanks.

-- 
Lee Jones
Linaro Services Technical Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

  parent reply	other threads:[~2018-03-07 13:00 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-16  4:17 [PATCH] mfd: tps65218: Reorder tps65218_regulator_id enum Dave Gerlach
2018-02-16  4:17 ` Dave Gerlach
2018-02-16  4:17 ` Dave Gerlach
2018-02-16  4:40 ` Keerthy
2018-02-16  4:40   ` Keerthy
2018-02-16  4:40   ` Keerthy
2018-03-07 13:00 ` Lee Jones [this message]
2018-03-07 13:00   ` Lee Jones

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=20180307130017.m6swph3mlupu6g76@dell \
    --to=lee.jones@linaro.org \
    --cc=broonie@kernel.org \
    --cc=d-gerlach@ti.com \
    --cc=j-keerthy@ti.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=tony@atomide.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.