On Fri, Sep 4, 2020 at 5:41 PM Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx> wrote: > > On 04/09/2020 17:21:15+0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx> > > > > Align the arguments passed to devm_rtc_device_register() with the upper > > line. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx> > > --- > > drivers/rtc/rtc-rx8010.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/rtc/rtc-rx8010.c b/drivers/rtc/rtc-rx8010.c > > index 181fc21cefa8..ed8ba38b4991 100644 > > --- a/drivers/rtc/rtc-rx8010.c > > +++ b/drivers/rtc/rtc-rx8010.c > > @@ -450,7 +450,7 @@ static int rx8010_probe(struct i2c_client *client, > > } > > > > rx8010->rtc = devm_rtc_device_register(&client->dev, client->name, > > - &rx8010_rtc_ops, THIS_MODULE); > > + &rx8010_rtc_ops, THIS_MODULE); > > > > You have bonus points if you replace that patch by switching from > devm_rtc_device_register to devm_rtc_allocate_device and > rtc_register_device. > > More bonus points if you also set range_min and range_max and then get > rid of the range checking in set_time. > Hi Alexandre! I've just looked at the code and wondered why there's no devm counterpart for rtc_register_device(). Then I noticed that the release callback for devm_rtc_allocate_device() takes care of unregistering the device. This looks like serious devres abuse to me. In general the idea is for the release callback to only undo whatever the devres function did and this should be opaque to the concerned resources. In this case I believe there's no need for the 'registered' field in struct rtc_device - this structure should *not* care about this - and there should be devm_rtc_register_device() whose release callback would take care of the unregistering. Since this function would be called after devm_rtc_allocate_device(), it would be released before so the ordering should be fine. Let me know your thoughts. Best regards, Bartosz Golaszewski
