[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 8 Aug 2018 13:01:21 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Peter Rosin <peda@...ntia.se>
Cc: Hans de Goede <hdegoede@...hat.com>,
"Rafael J . Wysocki" <rjw@...ysocki.net>,
Len Brown <lenb@...nel.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
Darren Hart <dvhart@...radead.org>,
Wolfram Sang <wsa@...-dreams.de>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
Platform Driver <platform-driver-x86@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
John Garry <john.garry@...wei.com>,
linux-i2c <linux-i2c@...r.kernel.org>
Subject: Re: [PATCH v4 4/4] i2c: Add multi-instantiate pseudo driver
On Wed, Aug 8, 2018 at 12:47 PM, Peter Rosin <peda@...ntia.se> wrote:
> On 2018-08-08 11:08, Andy Shevchenko wrote:
>> On Wed, Aug 8, 2018 at 11:30 AM, Hans de Goede <hdegoede@...hat.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
Powered by blists - more mailing lists