Messages in this thread | | | Date | Thu, 2 May 2024 11:40:44 +0300 | Subject | Re: [PATCH 1/2] staging: pi433: Use class_create instead of class_register. | From | Shahar Avidar <> |
| |
On 01/05/2024 17:12, Greg KH wrote: > On Wed, May 01, 2024 at 08:58:19AM +0300, Shahar Avidar wrote: >> Make use of a higher level API. > > What does this mean? > By "higher level" I meant a wrapper function that includes the "class_register" call.
>> Reduce global memory allocation from struct class to pointer size. > > No, you increased memory allocation here, why do you think you reduced > it? > Reducing *global* memory allocation. I understand the tradeoff would be allocating in run time the class struct anyway, but than, it could also be freed.
Since the Pi433 is a RasPi expansion board and can be attached\removed in an asynchronous matter by the user, and only one can be attached at a time, I thought it is best not to statically allocate memory which won't be freed even if the hat is removed.
By using the class_create & class_destroy I thought of reducing memory allocated by the RasPi if the pi433 is removed.
But following your response I now actually see that the class struct will have the same lifespan anyway if allocated statically or dynamically if its alive between the init\exit calls.
> Also, this looks like a revert of commit f267da65bb6b ("staging: pi433: > make pi433_class constant"), accepted a few months ago, why not just > call it out as an explicit revert if that's what you want to do? > I actually saw this commit, but for some reason did not connect the dots when I wrote this patch. My bad.
> class_create is going away "soon", why add this back when people are > working so hard to remove its usage? What tutorial did you read that > made you want to make this change? > It's true, I got it the wrong way I guess. I thought class_create is the preferred API (but now that you mentioned commit f267da65bb6b, I see it's not). I did notice it in many other drivers though, and took them as an example (e.g. gnss).
> thanks, > > greg k-h
I actually initially thought that the pi433 class should be removed since it doesn't bring any new attributes with it, and that spi_slave_class is more appropriate, but then I saw no other driver using it. Any thoughts about that? -- Regards,
Shahar
| |