Re: [PATCH v2] drm/tinydrm: Move backlight helpers to a separate file

[Noralf Trønnes <noralf@xxxxxxxxxxx>]

Den 25.09.2017 16.56, skrev Noralf Trønnes:
> Hi Meghana,
>
>
> Den 22.09.2017 17.09, skrev Meghana Madhyastha:
> > Move backlight helpers from tinydrm-helpers.c to
> > tinydrm-backlight.c. This is because it is organizationally
> > simpler to understand and advantageous to group functions
> > performing a similar function to a separate file as opposed to
> > having one helper file with heteregenous helper functions.
> >
> > Signed-off-by: Meghana Madhyastha <meghana.madhyastha@xxxxxxxxx>
> > ---
>
> I don't think there is much gain in just moving the code like this.
>
> The idea is to add a drm_backlight helper that can be useful for all
> DRM drivers that use the backlight subsystem.

The full path to that helper would be:
drivers/gpu/drm/drm_backlight.c

> This is what the TODO says:
> https://dri.freedesktop.org/docs/drm/gpu/todo.html#tinydrm
>
> - backlight helpers, probably best to put them into a new 
> drm_backlight.c.
>   This is because drivers/video is de-facto unmaintained. We could also
>   move drivers/video/backlight to drivers/gpu/backlight and take it all
>   over within drm-misc, but that’s more work.
>
> There is also this discussion to take into account:
> KMS backlight ABI proposition
> https://lists.freedesktop.org/archives/dri-devel/2017-February/133206.html 
>
>
> I don't remember what came out of that discussion.
>
> Noralf.
>
> > Changes in v2:
> >   -Improved commit message by explaining why the changes were made.
> >
> >   drivers/gpu/drm/tinydrm/core/Makefile            |   2 +-
> >   drivers/gpu/drm/tinydrm/core/tinydrm-backlight.c | 103 
> > +++++++++++++++++++++++
> >   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c   |  94 
> > ---------------------
> >   drivers/gpu/drm/tinydrm/mi0283qt.c               |   1 +
> >   drivers/gpu/drm/tinydrm/mipi-dbi.c               |   1 +
> >   include/drm/tinydrm/tinydrm-backlight.h          |  18 ++++
> >   6 files changed, 124 insertions(+), 95 deletions(-)
> >   create mode 100644 drivers/gpu/drm/tinydrm/core/tinydrm-backlight.c
> >   create mode 100644 include/drm/tinydrm/tinydrm-backlight.h
> >
> > diff --git a/drivers/gpu/drm/tinydrm/core/Makefile 
> > b/drivers/gpu/drm/tinydrm/core/Makefile
> > index fb221e6..389ca7a 100644
> > --- a/drivers/gpu/drm/tinydrm/core/Makefile
> > +++ b/drivers/gpu/drm/tinydrm/core/Makefile
> > @@ -1,3 +1,3 @@
> > -tinydrm-y := tinydrm-core.o tinydrm-pipe.o tinydrm-helpers.o
> > +tinydrm-y := tinydrm-core.o tinydrm-pipe.o tinydrm-backlight.o 
> > tinydrm-helpers.o
> >     obj-$(CONFIG_DRM_TINYDRM) += tinydrm.o
> > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-backlight.c 
> > b/drivers/gpu/drm/tinydrm/core/tinydrm-backlight.c
> > new file mode 100644
> > index 0000000..dc6f17d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-backlight.c
> > @@ -0,0 +1,103 @@
> > +#include <linux/backlight.h>
> > +#include <linux/dma-buf.h>
> > +#include <linux/pm.h>
> > +#include <linux/swab.h>
> > +
> > +#include <drm/tinydrm/tinydrm.h>
> > +#include <drm/tinydrm/tinydrm-backlight.h>
> > +
> > +/**
> > + * tinydrm_of_find_backlight - Find backlight device in device-tree
> > + * @dev: Device
> > + *
> > + * This function looks for a DT node pointed to by a property named 
> > 'backlight'
> > + * and uses of_find_backlight_by_node() to get the backlight device.
> > + * Additionally if the brightness property is zero, it is set to
> > + * max_brightness.
> > + *
> > + * Returns:
> > + * NULL if there's no backlight property.
> > + * Error pointer -EPROBE_DEFER if the DT node is found, but no 
> > backlight device
> > + * is found.
> > + * If the backlight device is found, a pointer to the structure is 
> > returned.
> > + */
> > +
> > +struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
> > +{
> > +    struct backlight_device *backlight;
> > +    struct device_node *np;
> > +
> > +    np = of_parse_phandle(dev->of_node, "backlight", 0);
> > +    if (!np)
> > +        return NULL;
> > +
> > +    backlight = of_find_backlight_by_node(np);
> > +    of_node_put(np);
> > +
> > +    if (!backlight)
> > +        return ERR_PTR(-EPROBE_DEFER);
> > +
> > +    if (!backlight->props.brightness) {
> > +        backlight->props.brightness = backlight->props.max_brightness;
> > +        DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> > +                  backlight->props.brightness);
> > +    }
> > +
> > +    return backlight;
> > +}
> > +EXPORT_SYMBOL(tinydrm_of_find_backlight);
> > +
> > +/**
> > + * tinydrm_enable_backlight - Enable backlight helper
> > + * @backlight: Backlight device
> > + *
> > + * Returns:
> > + * Zero on success, negative error code on failure.
> > + */
> > +int tinydrm_enable_backlight(struct backlight_device *backlight)
> > +{
> > +    unsigned int old_state;
> > +    int ret;
> > +
> > +    if (!backlight)
> > +        return 0;
> > +
> > +    old_state = backlight->props.state;
> > +    backlight->props.state &= ~BL_CORE_FBBLANK;
> > +    DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
> > +              backlight->props.state);
> > +
> > +    ret = backlight_update_status(backlight);
> > +    if (ret)
> > +        DRM_ERROR("Failed to enable backlight %d\n", ret);
> > +
> > +    return ret;
> > +}
> > +EXPORT_SYMBOL(tinydrm_enable_backlight);
> > +
> > +/**
> > + * tinydrm_disable_backlight - Disable backlight helper
> > + * @backlight: Backlight device
> > + *
> > + * Returns:
> > + * Zero on success, negative error code on failure.
> > + */
> > +int tinydrm_disable_backlight(struct backlight_device *backlight)
> > +{
> > +    unsigned int old_state;
> > +    int ret;
> > +
> > +    if (!backlight)
> > +        return 0;
> > +
> > +    old_state = backlight->props.state;
> > +    backlight->props.state |= BL_CORE_FBBLANK;
> > +    DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
> > +              backlight->props.state);
> > +    ret = backlight_update_status(backlight);
> > +    if (ret)
> > +        DRM_ERROR("Failed to disable backlight %d\n", ret);
> > +
> > +    return ret;
> > +}
> > +EXPORT_SYMBOL(tinydrm_disable_backlight);
> > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c 
> > b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > index bd6cce0..ee8ad8c 100644
> > --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > @@ -236,100 +236,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void 
> > *vaddr, struct drm_framebuffer *fb,
> >   }
> >   EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
> >   -/**
> > - * tinydrm_of_find_backlight - Find backlight device in device-tree
> > - * @dev: Device
> > - *
> > - * This function looks for a DT node pointed to by a property named 
> > 'backlight'
> > - * and uses of_find_backlight_by_node() to get the backlight device.
> > - * Additionally if the brightness property is zero, it is set to
> > - * max_brightness.
> > - *
> > - * Returns:
> > - * NULL if there's no backlight property.
> > - * Error pointer -EPROBE_DEFER if the DT node is found, but no 
> > backlight device
> > - * is found.
> > - * If the backlight device is found, a pointer to the structure is 
> > returned.
> > - */
> > -struct backlight_device *tinydrm_of_find_backlight(struct device *dev)
> > -{
> > -    struct backlight_device *backlight;
> > -    struct device_node *np;
> > -
> > -    np = of_parse_phandle(dev->of_node, "backlight", 0);
> > -    if (!np)
> > -        return NULL;
> > -
> > -    backlight = of_find_backlight_by_node(np);
> > -    of_node_put(np);
> > -
> > -    if (!backlight)
> > -        return ERR_PTR(-EPROBE_DEFER);
> > -
> > -    if (!backlight->props.brightness) {
> > -        backlight->props.brightness = backlight->props.max_brightness;
> > -        DRM_DEBUG_KMS("Backlight brightness set to %d\n",
> > -                  backlight->props.brightness);
> > -    }
> > -
> > -    return backlight;
> > -}
> > -EXPORT_SYMBOL(tinydrm_of_find_backlight);
> > -
> > -/**
> > - * tinydrm_enable_backlight - Enable backlight helper
> > - * @backlight: Backlight device
> > - *
> > - * Returns:
> > - * Zero on success, negative error code on failure.
> > - */
> > -int tinydrm_enable_backlight(struct backlight_device *backlight)
> > -{
> > -    unsigned int old_state;
> > -    int ret;
> > -
> > -    if (!backlight)
> > -        return 0;
> > -
> > -    old_state = backlight->props.state;
> > -    backlight->props.state &= ~BL_CORE_FBBLANK;
> > -    DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
> > -              backlight->props.state);
> > -
> > -    ret = backlight_update_status(backlight);
> > -    if (ret)
> > -        DRM_ERROR("Failed to enable backlight %d\n", ret);
> > -
> > -    return ret;
> > -}
> > -EXPORT_SYMBOL(tinydrm_enable_backlight);
> > -
> > -/**
> > - * tinydrm_disable_backlight - Disable backlight helper
> > - * @backlight: Backlight device
> > - *
> > - * Returns:
> > - * Zero on success, negative error code on failure.
> > - */
> > -int tinydrm_disable_backlight(struct backlight_device *backlight)
> > -{
> > -    unsigned int old_state;
> > -    int ret;
> > -
> > -    if (!backlight)
> > -        return 0;
> > -
> > -    old_state = backlight->props.state;
> > -    backlight->props.state |= BL_CORE_FBBLANK;
> > -    DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,
> > -              backlight->props.state);
> > -    ret = backlight_update_status(backlight);
> > -    if (ret)
> > -        DRM_ERROR("Failed to disable backlight %d\n", ret);
> > -
> > -    return ret;
> > -}
> > -EXPORT_SYMBOL(tinydrm_disable_backlight);
> >     #if IS_ENABLED(CONFIG_SPI)
> >   diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c 
> > b/drivers/gpu/drm/tinydrm/mi0283qt.c
> > index 7e5bb7d..c161d45 100644
> > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> > @@ -12,6 +12,7 @@
> >   #include <drm/tinydrm/ili9341.h>
> >   #include <drm/tinydrm/mipi-dbi.h>
> >   #include <drm/tinydrm/tinydrm-helpers.h>
> > +#include <drm/tinydrm/tinydrm-backlight.h>
> >   #include <linux/delay.h>
> >   #include <linux/gpio/consumer.h>
> >   #include <linux/module.h>
> > diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c 
> > b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> > index 2caeabc..dc84f26 100644
> > --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> > +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> > @@ -11,6 +11,7 @@
> >     #include <drm/tinydrm/mipi-dbi.h>
> >   #include <drm/tinydrm/tinydrm-helpers.h>
> > +#include <drm/tinydrm/tinydrm-backlight.h>
> >   #include <linux/debugfs.h>
> >   #include <linux/dma-buf.h>
> >   #include <linux/gpio/consumer.h>
> > diff --git a/include/drm/tinydrm/tinydrm-backlight.h 
> > b/include/drm/tinydrm/tinydrm-backlight.h
> > new file mode 100644
> > index 0000000..6a7b6d5
> > --- /dev/null
> > +++ b/include/drm/tinydrm/tinydrm-backlight.h
> > @@ -0,0 +1,18 @@
> > +/*
> > + * Copyright (C) 2016 Noralf Trønnes
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#ifndef __LINUX_TINYDRM_BACKLIGHT_H
> > +#define __LINUX_TINYDRM_BACKLIGHT_H
> > +
> > +struct backlight_device;
> > +struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
> > +int tinydrm_enable_backlight(struct backlight_device *backlight);
> > +int tinydrm_disable_backlight(struct backlight_device *backlight);
> > +
> > +#endif /* __LINUX_TINYDRM_BACKLIGHT_H */
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel