Received-SPF: pass (google.com: domain of linux-kernel+bounces-166128-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <fede8589-dd11-4b0c-aa70-7ec23aed64b1@gmail.com>
Date: Thu, 2 May 2024 11:40:44 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/2] staging: pi433: Use class_create instead of
 class_register.
To: Greg KH <gregkh@linuxfoundation.org>
Cc: hverkuil-cisco@xs4all.nl, andriy.shevchenko@linux.intel.com,
 robh@kernel.org, felixkimbu1@gmail.com, dan.carpenter@linaro.org,
 linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
References: <20240501055820.603272-1-ikobh7@gmail.com>
 <20240501055820.603272-2-ikobh7@gmail.com>
 <2024050109-reward-vision-58e9@gregkh>
Content-Language: en-US
From: Shahar Avidar <ikobh7@gmail.com>
In-Reply-To: <2024050109-reward-vision-58e9@gregkh>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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