[v5,0/7] backlight: gpio: simplify the driver
mbox series

Message ID 20191007033200.13443-1-brgl@bgdev.pl
Headers show
Series
  • backlight: gpio: simplify the driver
Related show

Message

Bartosz Golaszewski Oct. 7, 2019, 3:31 a.m. UTC
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

While working on my other series related to gpio-backlight[1] I noticed
that we could simplify the driver if we made the only user of platform
data use GPIO lookups and device properties. This series tries to do
that.

The first patch adds all necessary data structures to ecovec24. Patch
2/7 unifies much of the code for both pdata and non-pdata cases. Patches
3-4/7 remove unused platform data fields. Last three patches contain
additional improvements for the GPIO backlight driver while we're already
modifying it.

I don't have access to this HW but hopefully this works. Only compile
tested.

[1] https://lkml.org/lkml/2019/6/25/900

v1 -> v2:
- rebased on top of v5.3-rc1 and adjusted to the recent changes from Andy
- added additional two patches with minor improvements

v2 -> v3:
- in patch 7/7: used initializers to set values for pdata and dev local vars

v3 -> v4:
- rebased on top of v5.4-rc1
- removed changes that are no longer relevant after commit ec665b756e6f
  ("backlight: gpio-backlight: Correct initial power state handling")
- added patch 7/7

v4 ->V5:
- in patch 7/7: added a comment replacing the name of the function being
  pulled into probe()

Bartosz Golaszewski (7):
  backlight: gpio: remove unneeded include
  sh: ecovec24: add additional properties to the backlight device
  backlight: gpio: simplify the platform data handling
  sh: ecovec24: don't set unused fields in platform data
  backlight: gpio: remove unused fields from platform data
  backlight: gpio: use a helper variable for &pdev->dev
  backlight: gpio: pull gpio_backlight_initial_power_state() into probe

 arch/sh/boards/mach-ecovec24/setup.c         |  33 ++++--
 drivers/video/backlight/gpio_backlight.c     | 108 +++++--------------
 include/linux/platform_data/gpio_backlight.h |   3 -
 3 files changed, 53 insertions(+), 91 deletions(-)

Comments

Lee Jones Oct. 14, 2019, 8:12 a.m. UTC | #1
On Mon, 07 Oct 2019, Bartosz Golaszewski wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> While working on my other series related to gpio-backlight[1] I noticed
> that we could simplify the driver if we made the only user of platform
> data use GPIO lookups and device properties. This series tries to do
> that.
> 
> The first patch adds all necessary data structures to ecovec24. Patch
> 2/7 unifies much of the code for both pdata and non-pdata cases. Patches
> 3-4/7 remove unused platform data fields. Last three patches contain
> additional improvements for the GPIO backlight driver while we're already
> modifying it.
> 
> I don't have access to this HW but hopefully this works. Only compile
> tested.
> 
> [1] https://lkml.org/lkml/2019/6/25/900
> 
> v1 -> v2:
> - rebased on top of v5.3-rc1 and adjusted to the recent changes from Andy
> - added additional two patches with minor improvements
> 
> v2 -> v3:
> - in patch 7/7: used initializers to set values for pdata and dev local vars
> 
> v3 -> v4:
> - rebased on top of v5.4-rc1
> - removed changes that are no longer relevant after commit ec665b756e6f
>   ("backlight: gpio-backlight: Correct initial power state handling")
> - added patch 7/7
> 
> v4 ->V5:
> - in patch 7/7: added a comment replacing the name of the function being
>   pulled into probe()
> 
> Bartosz Golaszewski (7):
>   backlight: gpio: remove unneeded include
>   sh: ecovec24: add additional properties to the backlight device
>   backlight: gpio: simplify the platform data handling
>   sh: ecovec24: don't set unused fields in platform data
>   backlight: gpio: remove unused fields from platform data
>   backlight: gpio: use a helper variable for &pdev->dev
>   backlight: gpio: pull gpio_backlight_initial_power_state() into probe
> 
>  arch/sh/boards/mach-ecovec24/setup.c         |  33 ++++--

I guess we're just waiting for the SH Acks now?

>  drivers/video/backlight/gpio_backlight.c     | 108 +++++--------------
>  include/linux/platform_data/gpio_backlight.h |   3 -
>  3 files changed, 53 insertions(+), 91 deletions(-)
>
Bartosz Golaszewski Oct. 14, 2019, 8:15 a.m. UTC | #2
pon., 14 paź 2019 o 10:12 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Mon, 07 Oct 2019, Bartosz Golaszewski wrote:
>
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > While working on my other series related to gpio-backlight[1] I noticed
> > that we could simplify the driver if we made the only user of platform
> > data use GPIO lookups and device properties. This series tries to do
> > that.
> >
> > The first patch adds all necessary data structures to ecovec24. Patch
> > 2/7 unifies much of the code for both pdata and non-pdata cases. Patches
> > 3-4/7 remove unused platform data fields. Last three patches contain
> > additional improvements for the GPIO backlight driver while we're already
> > modifying it.
> >
> > I don't have access to this HW but hopefully this works. Only compile
> > tested.
> >
> > [1] https://lkml.org/lkml/2019/6/25/900
> >
> > v1 -> v2:
> > - rebased on top of v5.3-rc1 and adjusted to the recent changes from Andy
> > - added additional two patches with minor improvements
> >
> > v2 -> v3:
> > - in patch 7/7: used initializers to set values for pdata and dev local vars
> >
> > v3 -> v4:
> > - rebased on top of v5.4-rc1
> > - removed changes that are no longer relevant after commit ec665b756e6f
> >   ("backlight: gpio-backlight: Correct initial power state handling")
> > - added patch 7/7
> >
> > v4 ->V5:
> > - in patch 7/7: added a comment replacing the name of the function being
> >   pulled into probe()
> >
> > Bartosz Golaszewski (7):
> >   backlight: gpio: remove unneeded include
> >   sh: ecovec24: add additional properties to the backlight device
> >   backlight: gpio: simplify the platform data handling
> >   sh: ecovec24: don't set unused fields in platform data
> >   backlight: gpio: remove unused fields from platform data
> >   backlight: gpio: use a helper variable for &pdev->dev
> >   backlight: gpio: pull gpio_backlight_initial_power_state() into probe
> >
> >  arch/sh/boards/mach-ecovec24/setup.c         |  33 ++++--
>
> I guess we're just waiting for the SH Acks now?
>

We've been waiting for them for a couple months now - the sh patches
haven't changed since v1...

Rich, Yoshinori - could you ack this so that it can go in for v5.5?

Thanks,
Bartosz

> >  drivers/video/backlight/gpio_backlight.c     | 108 +++++--------------
> >  include/linux/platform_data/gpio_backlight.h |   3 -
> >  3 files changed, 53 insertions(+), 91 deletions(-)
> >
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
Linus Walleij Oct. 16, 2019, 12:56 p.m. UTC | #3
On Mon, Oct 14, 2019 at 10:12 AM Lee Jones <lee.jones@linaro.org> wrote:

> >  arch/sh/boards/mach-ecovec24/setup.c         |  33 ++++--
>
> I guess we're just waiting for the SH Acks now?

The one maintainer with this board is probably overloaded.

I would say just apply it, it can't hold back the entire series.

Yours,
Linus Walleij
Jacopo Mondi Oct. 16, 2019, 1:05 p.m. UTC | #4
Hi, sorry for not having replied earlier

On Wed, Oct 16, 2019 at 02:56:57PM +0200, Linus Walleij wrote:
> On Mon, Oct 14, 2019 at 10:12 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> > >  arch/sh/boards/mach-ecovec24/setup.c         |  33 ++++--
> >
> > I guess we're just waiting for the SH Acks now?
>
> The one maintainer with this board is probably overloaded.
>
> I would say just apply it, it can't hold back the entire series.

I've been able to resurect the Ecovec, and I've also been given a copy
of its schematics file a few weeks ago.

It's in my TODO list to test this series but I didn't manage to find
time. If I pinky promise I get back to you before end of the week,
could you wait for me ? :)

Thanks and sorry again!

>
> Yours,
> Linus Walleij
Lee Jones Oct. 17, 2019, 7:25 a.m. UTC | #5
On Wed, 16 Oct 2019, Jacopo Mondi wrote:

> Hi, sorry for not having replied earlier
> 
> On Wed, Oct 16, 2019 at 02:56:57PM +0200, Linus Walleij wrote:
> > On Mon, Oct 14, 2019 at 10:12 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > > >  arch/sh/boards/mach-ecovec24/setup.c         |  33 ++++--
> > >
> > > I guess we're just waiting for the SH Acks now?
> >
> > The one maintainer with this board is probably overloaded.
> >
> > I would say just apply it, it can't hold back the entire series.
> 
> I've been able to resurect the Ecovec, and I've also been given a copy
> of its schematics file a few weeks ago.
> 
> It's in my TODO list to test this series but I didn't manage to find
> time. If I pinky promise I get back to you before end of the week,
> could you wait for me ? :)

Yes, no problem.
Jacopo Mondi Oct. 18, 2019, 3:04 p.m. UTC | #6
Hi,

On Thu, Oct 17, 2019 at 08:25:50AM +0100, Lee Jones wrote:
> On Wed, 16 Oct 2019, Jacopo Mondi wrote:
>
> > Hi, sorry for not having replied earlier
> >
> > On Wed, Oct 16, 2019 at 02:56:57PM +0200, Linus Walleij wrote:
> > > On Mon, Oct 14, 2019 at 10:12 AM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > > >  arch/sh/boards/mach-ecovec24/setup.c         |  33 ++++--
> > > >
> > > > I guess we're just waiting for the SH Acks now?
> > >
> > > The one maintainer with this board is probably overloaded.
> > >
> > > I would say just apply it, it can't hold back the entire series.
> >
> > I've been able to resurect the Ecovec, and I've also been given a copy
> > of its schematics file a few weeks ago.
> >
> > It's in my TODO list to test this series but I didn't manage to find
> > time. If I pinky promise I get back to you before end of the week,
> > could you wait for me ? :)

Finally had some time to spend on this.

As I've reported to Bartosz, this version does not work on Ecovec out
of the box, as the GPIO line connected to the backlight needs to be
configured to work in output mode before registering the backlight
device.

With this simple change:

$ git diff
diff --git a/arch/sh/boards/mach-ecovec24/setup.c b/arch/sh/boards/mach-ecovec24/setup.c
index dd427bac5cde..eec6e805c3ed 100644
--- a/arch/sh/boards/mach-ecovec24/setup.c
+++ b/arch/sh/boards/mach-ecovec24/setup.c
@@ -1473,6 +1473,7 @@ static int __init arch_setup(void)
 #endif
 #endif

+       gpio_direction_output(GPIO_PTR1, 1);
        gpiod_add_lookup_table(&gpio_backlight_lookup);
        gpio_backlight_device = platform_device_register_full(
                                        &gpio_backlight_device_info);

I can now control the gpio through the backlight interface.

So please add this bit on top of next iteration and add my:
Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks and sorry for the long time it took!

>
> Yes, no problem.
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
Bartosz Golaszewski Oct. 18, 2019, 4:20 p.m. UTC | #7
pt., 18 paź 2019 o 17:02 Jacopo Mondi <jacopo@jmondi.org> napisał(a):
>
> Hi,
>
> On Thu, Oct 17, 2019 at 08:25:50AM +0100, Lee Jones wrote:
> > On Wed, 16 Oct 2019, Jacopo Mondi wrote:
> >
> > > Hi, sorry for not having replied earlier
> > >
> > > On Wed, Oct 16, 2019 at 02:56:57PM +0200, Linus Walleij wrote:
> > > > On Mon, Oct 14, 2019 at 10:12 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > > >  arch/sh/boards/mach-ecovec24/setup.c         |  33 ++++--
> > > > >
> > > > > I guess we're just waiting for the SH Acks now?
> > > >
> > > > The one maintainer with this board is probably overloaded.
> > > >
> > > > I would say just apply it, it can't hold back the entire series.
> > >
> > > I've been able to resurect the Ecovec, and I've also been given a copy
> > > of its schematics file a few weeks ago.
> > >
> > > It's in my TODO list to test this series but I didn't manage to find
> > > time. If I pinky promise I get back to you before end of the week,
> > > could you wait for me ? :)
>
> Finally had some time to spend on this.
>
> As I've reported to Bartosz, this version does not work on Ecovec out
> of the box, as the GPIO line connected to the backlight needs to be
> configured to work in output mode before registering the backlight
> device.
>
> With this simple change:
>
> $ git diff
> diff --git a/arch/sh/boards/mach-ecovec24/setup.c b/arch/sh/boards/mach-ecovec24/setup.c
> index dd427bac5cde..eec6e805c3ed 100644
> --- a/arch/sh/boards/mach-ecovec24/setup.c
> +++ b/arch/sh/boards/mach-ecovec24/setup.c
> @@ -1473,6 +1473,7 @@ static int __init arch_setup(void)
>  #endif
>  #endif
>
> +       gpio_direction_output(GPIO_PTR1, 1);

This is a hack actually. The problem lies with the gpio backlight
driver - it should really set the mode to output, not leave it as is.
If there's no good reason to keep it as it is now, I'll add another
patch to the series that moves the call to devm_gpiod_get() to where
we've already determined the initial value in probe() and pass an
appropriate GPIOD_OUT_HIGH/LOW flag.

Bart

>         gpiod_add_lookup_table(&gpio_backlight_lookup);
>         gpio_backlight_device = platform_device_register_full(
>                                         &gpio_backlight_device_info);
>
> I can now control the gpio through the backlight interface.
>
> So please add this bit on top of next iteration and add my:
> Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> Thanks and sorry for the long time it took!
>
> >
> > Yes, no problem.
> >
> > --
> > Lee Jones [李琼斯]
> > Linaro Services Technical Lead
> > Linaro.org │ Open source software for ARM SoCs
> > Follow Linaro: Facebook | Twitter | Blog