HI Sean, On Wed, 16 Mar 2022 at 10:18, Sean Anderson <sean...@gmail.com> wrote: > > On 3/1/22 9:58 AM, Simon Glass wrote: > > Hi Sean, > > > > On Sun, 27 Feb 2022 at 12:38, Sean Anderson <sean...@gmail.com> wrote: > >> > >> On 2/26/22 1:36 PM, Simon Glass wrote: > >>> Hi Sean, > >>> > >>> On Tue, 1 Feb 2022 at 21:24, Sean Anderson <sean...@gmail.com> wrote: > >>>> > >>>> On 2/1/22 10:59 PM, Simon Glass wrote: > >>>>> Hi Sean, > >>>>> > >>>>> On Tue, 1 Feb 2022 at 07:49, Sean Anderson <sean...@gmail.com> wrote: > >>>>>> > >>>>>> On 1/27/22 4:35 PM, Simon Glass wrote: > >>>>>>> Hi Sean, > >>>>>>> > >>>>>>> On Thu, 27 Jan 2022 at 08:43, Sean Anderson <sean...@gmail.com> wrote: > >>>>>>>> > >>>>>>>> On 1/27/22 10:05 AM, Simon Glass wrote: > >>>>>>>>> Hi Sean, > >>>>>>>>> > >>>>>>>>> On Sat, 15 Jan 2022 at 15:25, Sean Anderson <sean...@gmail.com> > >>>>>>>>> wrote: > >>>>>>>>>> > >>>>>>>>>> When freeing a clock there is not much we can do if there is an > >>>>>>>>>> error, and > >>>>>>>>>> most callers do not actually check the return value. Even e.g. > >>>>>>>>>> checking to > >>>>>>>>>> make sure that clk->id is valid should have been done in request() > >>>>>>>>>> in the > >>>>>>>>>> first place (unless someone is messing with the driver behind our > >>>>>>>>>> back). > >>>>>>>>>> Just return void and don't bother returning an error. > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Sean Anderson <sean...@gmail.com> > >>>>>>>>>> --- > >>>>>>>>>> > >>>>>>>>>> drivers/clk/clk-uclass.c | 7 +++---- > >>>>>>>>>> drivers/clk/clk_sandbox.c | 6 +++--- > >>>>>>>>>> include/clk-uclass.h | 8 +++----- > >>>>>>>>>> 3 files changed, 9 insertions(+), 12 deletions(-) > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> We have the same thing in other places too, but I am a little > >>>>>>>>> worried > >>>>>>>>> about removing error checking. We try to avoid checking arguments > >>>>>>>>> too > >>>>>>>>> much in U-Boot, due to code-size concerns, so I suppose I agree that > >>>>>>>>> an invalid clk should be caught by a debug assertion rather than a > >>>>>>>>> full check. But with driver model we have generally added an error > >>>>>>>>> return to every uclass method, for consistency and to permit > >>>>>>>>> returning > >>>>>>>>> error information if needed. > >>>>>>>>> > >>>>>>>>> Regards, > >>>>>>>>> Simon > >>>>>>>>> > >>>>>>>> > >>>>>>>> So there are a few reasons why I don't think a return value is useful > >>>>>>>> here. To illustrate this, consider a typical user of the clock API: > >>>>>>>> > >>>>>>>> struct clk a, b; > >>>>>>>> > >>>>>>>> ret = clk_get_by_name(dev, "a", &a); > >>>>>>>> if (ret) > >>>>>>>> return ret; > >>>>>>>> > >>>>>>>> ret = clk_get_by_name(dev, "b", &b); > >>>>>>>> if (ret) > >>>>>>>> goto free_a; > >>>>>>>> > >>>>>>>> ret = clk_set_rate(&a, 5000000); > >>>>>>>> if (ret) > >>>>>>>> goto free_b; > >>>>>>>> > >>>>>>>> ret = clk_enable(&b); > >>>>>>>> > >>>>>>>> free_b: > >>>>>>>> clk_free(&b); > >>>>>>>> free_a: > >>>>>>>> clk_free(&a); > >>>>>>>> return ret; > >>>>>>>> > >>>>>>>> - Because a and b are "thick pointers" they do not need any cleanup > >>>>>>>> to > >>>>>>>> free their own resources. The only cleanup might be if the > >>>>>>>> clock > >>>>>>>> driver has allocated something in clk_request (more on this > >>>>>>>> below) > >>>>>>>> - By the time we call clk_free, the mutable portions of the function > >>>>>>>> have already completed. In effect, the function has succeeded, > >>>>>>>> regardless of whether clk_free fails. Additionally, we cannot > >>>>>>>> take any > >>>>>>>> action if it fails, since we still have to free both clocks. > >>>>>>>> - clk_free occurs during the error path of the function. Even if it > >>>>>>>> errored, we do not want to override the existing error from > >>>>>>>> one of the > >>>>>>>> functions doing "real" work. > >>>>>>>> > >>>>>>>> The last thing is that no clock driver actually does anything in > >>>>>>>> rfree. > >>>>>>>> The only driver with this function is the sandbox driver. I would > >>>>>>>> like > >>>>>>>> to remove the function altogether. As I understand it, the existing > >>>>>>>> API > >>>>>>>> is inspired by the reset drivers, so I would like to review its > >>>>>>>> usage in > >>>>>>>> the reset subsystem before removing it for the clock subsystem. I > >>>>>>>> also > >>>>>>>> want to make some changes to how rates and enables/disables are > >>>>>>>> calculated which might provide a case for rfree. But once that is > >>>>>>>> complete I think there will be no users still. > >>>>>>> > >>>>>>> What does this all look like in Linux? > >>>>>> > >>>>>> Their equivalent (clk_put) returns void, and generally so do most other > >>>>>> cleanup functions, since .device_remove also returns void. > >>>>> > >>>>> We really cannot ignore errors from device_remove(). > >>>> > >>>> Once you are at device_remove, all the users are gone and it's up to the > >>>> device to clean up after itself. And often there is nothing we can do > >>>> once remove is called. As you yourself say in device_remove, > >>>> > >>>> /* We can't put the children back */ > >>> > >>> Well this assumes that device_remove() is actually removing the > >>> device, not just disabling DMA, etc. > >>> > >>>> > >>>> Really the only sensible thing is to print an error and continue booting > >>>> if possible. > >>>> > >>>> And of course no clock drivers actually use this function anyway, nor do > >>>> (all but 5) users check it. > >>>> > >>>>> Anyway I think what you say about the 'thick pointer' makes sense. But > >>>>> my expectation was that removing a clock might turn off a clock above > >>>>> it in the tree, for example. > >>>> > >>>> No, this just frees resources (as is documented). If you want to turn > >>>> off a clock, you have to call clk_disable. In fact, a very common use > >>>> case is just like the example above, where the consmer frees the clock > >>>> after enabling it. > >>>> > >>>> (This is also why clk->enable_count/rate are basically useless for > >>>> anything other than CCF clocks) > >>> > >>> How about a clock provided by an audio codec on an I2C bus? Should > >>> clk_free() do anything in that case? I assume not. I think the > >>> compelling part of your argument is that it is a 'think pointer' and > >>> disable does nothing. So can you update clk_rfree() etc. to document > >>> what is allowed to be done in that function? > >> > >> The ideal case would be if you wanted to allocate some per-struct-clk > >> data. Then, the correct place to free it would be rfree. But no one > >> does this, and if they did it would probably be better to free things > >> in remove. > >> > >> Actually... no one in clk, reset, or power-domain does anything with > >> rfree. So I am inclined to just remove it altogether. > > > > Well, I suppose it is easy enough to add later, if needed. What does > > Linux use this for? > > As far as I can tell, Linux doesn't have request/free. There's > of_clk_provider->get(), but that corresponds more to of_xlate. > Similarly, there's reset_controller_dev->of_xlate(), but no request/free.
OK, sounds good then. Regards, Simon