Messages in this thread | | | Date | Wed, 18 Jan 2023 16:29:24 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v2 1/1] iio: adc: qcom-spmi-adc5: Fix the channel name |
| |
On Wed, Jan 18, 2023 at 03:04:23PM +0100, Marijn Suijten wrote: > On 2023-01-18 15:22:42, Andy Shevchenko wrote:
...
> > > > + name = devm_kasprintf(dev, GFP_KERNEL, "%pfwP", fwnode); > > > > > > Is this better/cleaner than copying the string from fwnode_get_name? > > > > Coying to where? And what would be the lifetime of that string? > > > > With devm_kasprintf(): > > - we don't care how long the string is > > - we don't care about corner cases of lifetime as it's the same as > > device itself (i.o.w. the same as the IIO device container) > > Curious if there isn't a devm_strdup(name) or similar? Main point is > that %pfwP seems like magic when fwnode_get_name is not (but returns a > const string that we cannot modify).
The devm_kstrdup(fwnode_get_name()) is an open coded variant of the above. I don't think we need to open code and produce NIH even a single API. And no, there is no magic behind that. At least from the fwnode point of view.
You may very well say that > 1500 instances of "%pOF" is a magic...
> If there is not, let's stick with > devm_kasprintf().
There is, but I'm against it. See above why.
> > > > + name[strchrnul(name, '@') - name] = '\0'; > > > > > > This is the same as *strchrnul(name, '@') = '\0'; if I'm not mistaken. > > > > Yes, But it's harder to read and understand. I believe the compiler has > > enough power to optimize this to the same assembly code. > > I find the latter clearer as it doesn't require the reader to figure out > that name - name cancels itself out. Alternatively we can write > strchrnul(name, '@')[0].
I don't like to have Pythonisms in the C code, really.
P.S. I guess this little patch already emptied my bandwidth, so I leave any further discussion to you and IIO maintainers. Thank you for the review!
-- With Best Regards, Andy Shevchenko
| |