Re: [PATCH] leds: pca955x: kzalloc + kcalloc to single kzalloc

[Rosen Penev]

On Thu, Apr 9, 2026 at 5:44 AM Lee Jones <lee@xxxxxxxxxx> wrote:
>
> On Thu, 26 Mar 2026, Rosen Penev wrote:
>
> > On Thu, Mar 26, 2026 at 4:33 AM Lee Jones <lee@xxxxxxxxxx> wrote:
> > >
> > > On Thu, 19 Mar 2026, Rosen Penev wrote:
> > >
> > > > Two fewer allocations as a result.
> > > >
> > > > Required placing some structs before others as flexible array members
> > > > require a complete definition. Declaration is not enough.
> > > >
> > > > Added __counted_by support for one of the structs for extra runtime
> > > > analysis.
> > > >
> > > > Signed-off-by: Rosen Penev <rosenp@xxxxxxxxx>
> > > > ---
> > > >  drivers/leds/leds-pca955x.c | 45 ++++++++++++++-----------------------
> > > >  1 file changed, 17 insertions(+), 28 deletions(-)
> > > >
> > > > diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
> > > > index 2007fe6217ec..ee5f02eaa3c9 100644
> > > > --- a/drivers/leds/leds-pca955x.c
> > > > +++ b/drivers/leds/leds-pca955x.c
> > > > @@ -112,19 +112,6 @@ static const struct pca955x_chipdef pca955x_chipdefs[] = {
> > > >       },
> > > >  };
> > > >
> > > > -struct pca955x {
> > > > -     struct mutex lock;
> > > > -     struct pca955x_led *leds;
> > > > -     const struct pca955x_chipdef    *chipdef;
> > > > -     struct i2c_client       *client;
> > > > -     unsigned long active_blink;
> > > > -     unsigned long active_pins;
> > > > -     unsigned long blink_period;
> > > > -#ifdef CONFIG_LEDS_PCA955X_GPIO
> > > > -     struct gpio_chip gpio;
> > > > -#endif
> > > > -};
> > > > -
> > > >  struct pca955x_led {
> > > >       struct pca955x  *pca955x;
> > > >       struct led_classdev     led_cdev;
> > > > @@ -137,8 +124,21 @@ struct pca955x_led {
> > > >  #define led_to_pca955x(l)    container_of(l, struct pca955x_led, led_cdev)
> > > >
> > > >  struct pca955x_platform_data {
> > > > -     struct pca955x_led      *leds;
> > > >       int                     num_leds;
> > > > +     struct pca955x_led      leds[] __counted_by(num_leds);
>
> I'm not sure we should we be embedding the full 'struct pca955x_led'
> (which contains runtime framework structures like 'struct led_classdev')
> directly inside the platform data?  Besides platform data should ideally
> be restricted to static configuration properties rather than runtime
> state.
>
> > > Where is the memory allocated to this now?
> > The kzalloc call was updated to use struct_size to allocate it
> > together with the struct.
> > >
> > > Why do we need this in both structs?
> > kzalloc and kcalloc can be combined in both locations.
>
> I believe my original question might have been misunderstood.  I was
> asking about the architectural design choice.  Why does the same 'leds'
> array need to exist in both the 'pca955x' runtime structure and the
> 'pca955x_platform_data' configuration structure?  Could we look at
> eliminating this redundancy to avoid storing duplicate data?
>From what I see, this bit of code handles missing pdata.
>
> > >
> > > > +};
> > > > +
> > > > +struct pca955x {
> > > > +     struct mutex lock;
> > > > +     const struct pca955x_chipdef    *chipdef;
> > > > +     struct i2c_client       *client;
> > > > +     unsigned long active_blink;
> > > > +     unsigned long active_pins;
> > > > +     unsigned long blink_period;
> > > > +#ifdef CONFIG_LEDS_PCA955X_GPIO
> > > > +     struct gpio_chip gpio;
> > > > +#endif
>
> #ifdef statements are to be avoided in C files.
>
> Please find another way to do this (if it's required).
This is not my code.
>
> > > > +     struct pca955x_led leds[];
> > > >  };
> > > >
> > > >  /* 8 bits per input register */
> > > > @@ -542,15 +542,11 @@ pca955x_get_pdata(struct i2c_client *client, const struct pca955x_chipdef *chip)
> > > >       if (count > chip->bits)
> > > >               return ERR_PTR(-ENODEV);
> > > >
> > > > -     pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> > > > +     pdata = devm_kzalloc(&client->dev, struct_size(pdata, leds, chip->bits), GFP_KERNEL);
> > > >       if (!pdata)
> > > >               return ERR_PTR(-ENOMEM);
> > > >
> > > > -     pdata->leds = devm_kcalloc(&client->dev,
> > > > -                                chip->bits, sizeof(struct pca955x_led),
> > > > -                                GFP_KERNEL);
> > > > -     if (!pdata->leds)
> > > > -             return ERR_PTR(-ENOMEM);
> > > > +     pdata->num_leds = chip->bits;
> > > >
> > > >       device_for_each_child_node(&client->dev, child) {
> > > >               u32 reg;
> > > > @@ -568,8 +564,6 @@ pca955x_get_pdata(struct i2c_client *client, const struct pca955x_chipdef *chip)
> > > >               fwnode_property_read_u32(child, "type", &led->type);
> > > >       }
> > > >
> > > > -     pdata->num_leds = chip->bits;
> > > > -
> > > >       return pdata;
> > > >  }
> > > >
> > > > @@ -623,15 +617,10 @@ static int pca955x_probe(struct i2c_client *client)
> > > >               return -ENODEV;
> > > >       }
> > > >
> > > > -     pca955x = devm_kzalloc(&client->dev, sizeof(*pca955x), GFP_KERNEL);
> > > > +     pca955x = devm_kzalloc(&client->dev, struct_size(pca955x, leds, chip->bits), GFP_KERNEL);
> > > >       if (!pca955x)
> > > >               return -ENOMEM;
> > > >
> > > > -     pca955x->leds = devm_kcalloc(&client->dev, chip->bits,
> > > > -                                  sizeof(*pca955x_led), GFP_KERNEL);
> > > > -     if (!pca955x->leds)
> > > > -             return -ENOMEM;
> > > > -
> > > >       i2c_set_clientdata(client, pca955x);
> > > >
> > > >       mutex_init(&pca955x->lock);
> > > > --
> > > > 2.53.0
> > > >
> > >
> > > --
> > > Lee Jones [李琼斯]
> >
>
> --
> Lee Jones [李琼斯]