Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 8 Aug 2018 13:01:21 +0300 | Subject | Re: [PATCH v4 4/4] i2c: Add multi-instantiate pseudo driver |
| |
On Wed, Aug 8, 2018 at 12:47 PM, Peter Rosin <peda@axentia.se> wrote: > On 2018-08-08 11:08, Andy Shevchenko wrote: >> On Wed, Aug 8, 2018 at 11:30 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> + /* Count number of clients to instantiate */ >>> + for (i = 0; inst_data[i].type; i++) {} >>> + >>> + multi = devm_kmalloc(dev, >>> + offsetof(struct i2c_multi_inst_data, clients[i]), >>> + GFP_KERNEL); >>> + if (!multi) >>> + return -ENOMEM; >> >> Here I see the following: >> - it's kinda unusual use of offsetof(), perhaps i*sizeof() + sizeof() >> would be more understandable >> - there is no guard against i == 0 > > I don't see why a guard is needed?
Because there is no point to have a module loaded when there is none client to serve.
> *Your* code below needs it, but that > issue is not a concern for the original code.
I can admit that's not a big deal, just making logic slightly more robust.
> It might however be a > good idea to fail the probe if there are no clients to instantiate, but > that's a different issue...
That's what I have in mind.
>> multi = devm_kmalloc(sizeof(*multi), GFP_KERNEL); >> if (!multi) >> return -ENOMEM; >> >> multi->clients = devm_kcalloc(i, sizeof(*multi->clients), GFP_KERNEL); >> if (ZERO_PTR_OR_NULL(multi->clients)) >> return -ENOMEM; >> >> But I would like to hear your (other's) opinion(s). > > I think using two allocations is a waste in this case.
On the other hand it makes code more readable. With offsetof() it is a bit hard to get it on the first glance.
>>> + if (inst_data[i].irq_idx != -1) { >> >>> = 0 sounds more robust > > But not as flexible/future-proof. Why should 0 be the only valid IRQ index?
Ah, because > is used usually is a quoting character in email you missed the point. It was written as >= 0.
-- With Best Regards, Andy Shevchenko
| |