diff mbox series

[7/8] rtc: rx8010: fix indentation in probe()

Message ID 20200904152116.2157-8-brgl@bgdev.pl
State Superseded
Headers show
Series rtc: rx8010: use regmap instead of i2c smbus API | expand

Commit Message

Bartosz Golaszewski Sept. 4, 2020, 3:21 p.m. UTC
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Align the arguments passed to devm_rtc_device_register() with the upper
line.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/rtc/rtc-rx8010.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alexandre Belloni Sept. 4, 2020, 3:41 p.m. UTC | #1
On 04/09/2020 17:21:15+0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Align the arguments passed to devm_rtc_device_register() with the upper
> line.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  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.
Bartosz Golaszewski Sept. 7, 2020, 9:34 a.m. UTC | #2
On Fri, Sep 4, 2020 at 5:41 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 04/09/2020 17:21:15+0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Align the arguments passed to devm_rtc_device_register() with the upper
> > line.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > ---
> >  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
Alexandre Belloni Sept. 11, 2020, 12:28 p.m. UTC | #3
On 07/09/2020 11:34:59+0200, Bartosz Golaszewski wrote:
> On Fri, Sep 4, 2020 at 5:41 PM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> >
> > On 04/09/2020 17:21:15+0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > Align the arguments passed to devm_rtc_device_register() with the upper
> > > line.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > ---
> > >  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.
> 

Note that the input subsystem is also doing it that way which is
probably not a good reason alone to do it like that. But, IIRC, there
was an actual reason this was done this way and it was the ordering of
the rtc_nvmem_register/rtc_nvmem_unregister with rtc_device_unregister.
I'm not sure this is still necessary though.
Bartosz Golaszewski Sept. 11, 2020, 12:33 p.m. UTC | #4
On Fri, Sep 11, 2020 at 2:28 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 07/09/2020 11:34:59+0200, Bartosz Golaszewski wrote:
> > On Fri, Sep 4, 2020 at 5:41 PM Alexandre Belloni
> > <alexandre.belloni@bootlin.com> wrote:
> > >
> > > On 04/09/2020 17:21:15+0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > Align the arguments passed to devm_rtc_device_register() with the upper
> > > > line.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > ---
> > > >  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.
> >
>
> Note that the input subsystem is also doing it that way which is
> probably not a good reason alone to do it like that. But, IIRC, there

I'm seeing this pattern elsewhere in the kernel too and I just
recently fixed this for MDIO. I think it's just a matter of people
copy-pasting a bad implementation.

> was an actual reason this was done this way and it was the ordering of
> the rtc_nvmem_register/rtc_nvmem_unregister with rtc_device_unregister.
> I'm not sure this is still necessary though.
>

To me - each of these should have their own 'unregister' function and
appropriate devres helpers *OR* RTC-related nvmem structures could be
set up and assigned to struct rtc_device after
devm_rtc_allocate_device() and picked up by the registration function
(and also undone by rtc_unregister_device()).

I'll try to allocate some time to look into this but it's not like
it's urgent or anything - it's just a potential improvement.

Bartosz
Alexandre Belloni Sept. 11, 2020, 12:44 p.m. UTC | #5
On 11/09/2020 14:33:46+0200, Bartosz Golaszewski wrote:
> I'm seeing this pattern elsewhere in the kernel too and I just
> recently fixed this for MDIO. I think it's just a matter of people
> copy-pasting a bad implementation.
> 
> > was an actual reason this was done this way and it was the ordering of
> > the rtc_nvmem_register/rtc_nvmem_unregister with rtc_device_unregister.
> > I'm not sure this is still necessary though.
> >
> 
> To me - each of these should have their own 'unregister' function and
> appropriate devres helpers *OR* RTC-related nvmem structures could be
> set up and assigned to struct rtc_device after
> devm_rtc_allocate_device() and picked up by the registration function
> (and also undone by rtc_unregister_device()).
> 
> I'll try to allocate some time to look into this but it's not like
> it's urgent or anything - it's just a potential improvement.
> 

Well, this could simply be done by adding a devres_add in
__rtc_register_device. I'm planning to remove rtc_nvmem_unregister after
the next LTS which will make that even easier.
diff mbox series

Patch

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);
 
 	if (IS_ERR(rx8010->rtc)) {
 		dev_err(&client->dev, "unable to register the class device\n");