Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230608075651.3214540-1-wenst@chromium.org> <ce04685b7c4840b683add0bdd4404da6@AcuMS.aculab.com>
 <CAGXv+5EtdhkMq916LLOfpz8ok_xump5tv+oeNBvPq-_3-93oqQ@mail.gmail.com> <0929093384864ff99c78800029ff5d76@AcuMS.aculab.com>
In-Reply-To: <0929093384864ff99c78800029ff5d76@AcuMS.aculab.com>
From:   Chen-Yu Tsai <wenst@chromium.org>
Date:   Mon, 12 Jun 2023 17:27:12 +0800
Message-ID: <CAGXv+5Hnrpv8kTU+0B9S4NyqALEz+DQOLfO3U7GwMZp9AoqLEw@mail.gmail.com>
Subject: Re: [PATCH] regulator: Use bitfield values for range selectors
To:     David Laight <David.Laight@aculab.com>
Cc:     Mark Brown <broonie@kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Manivannan Sadhasivam <mani@kernel.org>,
        Cristian Ciocaltea <cristian.ciocaltea@gmail.com>,
        Matti Vaittinen <mazziesaccount@gmail.com>,
        "linux-actions@lists.infradead.org" 
        <linux-actions@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jun 12, 2023 at 3:48=E2=80=AFPM David Laight <David.Laight@aculab.c=
om> wrote:
>
> From: Chen-Yu Tsai
> > Sent: 12 June 2023 04:39
> >
> > On Sun, Jun 11, 2023 at 1:10=E2=80=AFAM David Laight <David.Laight@acul=
ab.com> wrote:
> > >
> > > From: Chen-Yu Tsai
> > > > Sent: 08 June 2023 08:57
> > > >
> > > > Right now the regulator helpers expect raw register values for the =
range
> > > > selectors. This is different from the voltage selectors, which are
> > > > normalized as bitfield values. This leads to a bit of confusion. Al=
so,
> > > > raw values are harder to copy from datasheets or match up with them=
,
> > > > as datasheets will typically have bitfield values.
> > > >
> > > > Make the helpers expect bitfield values, and convert existing users=
.
> > > > Include bitops.h explicitly for ffs(), and reorder the header inclu=
de
> > > > statements. While at it, also replace module.h with export.h, since=
 the
> > > > only use is EXPORT_SYMBOL_GPL.
> > > >
> > > ...
> > > >  static const unsigned int atc260x_ldo_voltage_range_sel[] =3D {
> > > > -     0x0, 0x20,
> > > > +     0x0, 0x1,
> > > >  };
> > >
> > > Is there any way the change can be done so that un-edited
> > > modules fail to compile?
> > > Otherwise the whole thing is an accident waiting to happen.
> >
> > I think we could change the field name in the regulator description?
> > But unsuspecting end users / developers might just edit the name and no=
t
> > see that the scheme has changed.
> >
> > Or we could add a sanity check at runtime that checks the values during
> > regulator registration. How does that sound?
> >
> > Mark, is this something you'd like?
>
> Can you change the name of the function the values are passed to?

    .set_voltage_sel =3D regulator_set_voltage_sel_pickable_regmap,

This one, and

    .get_voltage_sel =3D regulator_get_voltage_sel_pickable_regmap,

which calls the helper regulator_range_selector_to_index() that actually
uses the values.

Not sure how changing the helper names would help, or how it is different
from changing the field name? The names are already quite long but spot
on.

> Or maybe change the type to 'unsigned char' (assuming bit numbers
> are small).

That doesn't quite work if the range selector is in the upper four bits.
The values would fit nevertheless.

ChenYu