Re: [PATCH v3] mfd: twl-core: Allocate modules with twl_private

[Lee Jones <lee@kernel.org>]

--- checkpatch.pl (3 issues) ---
Line 4: ERROR: Invalid commit separator - some tools may have problems applying this
Line 32: WARNING: please, no spaces at the start of a line
Line 41: WARNING: please, no spaces at the start of a line
----------------------------------

--- checkpatch.pl (2 issues) ---
Line 27: WARNING: please, no spaces at the start of a line
Line 36: WARNING: please, no spaces at the start of a line
----------------------------------

On Thu, 15 Oct 2026, Rosen Penev wrote:

> On Thu, Oct 15, 2026 at 5:05 AM Lee Jones <lee@kernel.org> wrote:
> >
> > On Fri, 25 Sep 2026, Rosen Penev wrote:
> >
> > > Use a flexible array member to allocate the twl_client array together
> > > with struct twl_private. This removes a separate devm_kcalloc() and
> > > keeps the module data tied to the main private structure.
> > >
> > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > > ---
> > >  v3: Capitalize Allocate.
> > >  v2: don't use twl_get_num_slaves().
> > >  drivers/mfd/twl-core.c | 16 ++++------------
> > >  1 file changed, 4 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> > > index f86df356d658..0b78fe30e047 100644
> > > --- a/drivers/mfd/twl-core.c
> > > +++ b/drivers/mfd/twl-core.c
> > > @@ -159,7 +159,7 @@ struct twl_private {
> > >       unsigned int twl_id;
> > >
> > >       struct twl_mapping *twl_map;
> > > -     struct twl_client *twl_modules;
> > > +     struct twl_client twl_modules[];
> > >  };
> > >
> > >  static struct twl_private *twl_priv;
> > > @@ -770,7 +770,9 @@ twl_probe(struct i2c_client *client)
> > >               goto out;
> > >       }
> > >
> > > -     twl_priv = devm_kzalloc(&client->dev, sizeof(struct twl_private),
> > > +     num_slaves = (id->driver_data & TWL6030_CLASS) ? 3 : 4;
> >
> > What's wrong with twl_get_num_slaves()?
> It relies on twl_priv already being allocated.

IMHO, this is too much hoop jumping to save on one pointer.  Code
cleanliness remains much more important here.  And don't even get me
started on that mess in include/linux/mfd/twl.h !

Just define TWL_MAX_SLAVES as 4 and have done:

        struct twl_client twl_modules[TWL_MAX_SLAVES];

> > > +     twl_priv = devm_kzalloc(&client->dev,
> > > +                             struct_size(twl_priv, twl_modules, num_slaves),
> > >                               GFP_KERNEL);
> > >       if (!twl_priv) {
> > >               status = -ENOMEM;
> > > @@ -787,16 +789,6 @@ twl_probe(struct i2c_client *client)
> > >               twl_regmap_config = twl4030_regmap_config;
> > >       }
> > >
> > > -     num_slaves = twl_get_num_slaves();
> > > -     twl_priv->twl_modules = devm_kcalloc(&client->dev,
> > > -                                      num_slaves,
> > > -                                      sizeof(struct twl_client),
> > > -                                      GFP_KERNEL);
> > > -     if (!twl_priv->twl_modules) {
> > > -             status = -ENOMEM;
> > > -             goto out;
> > > -     }
> > > -
> > >       for (i = 0; i < num_slaves; i++) {
> > >               struct twl_client *twl = &twl_priv->twl_modules[i];
> > >
> > > --
> > > 2.55.0
> > >
> >
> > --
> > Lee Jones

-- 
Lee Jones