Re: [PATCH 1/2] backlight: Add RAVE SP backlight driver

[Daniel Thompson]

On 08/03/18 17:29, Andrey Smirnov wrote:
> On Thu, Mar 8, 2018 at 8:58 AM, Daniel Thompson
> <daniel.thompson@xxxxxxxxxx> wrote:
> > On 07/03/18 02:40, Andrey Smirnov wrote:
> > > This driver provides access to RAVE SP backlight control
> > > functionality.
> > >
> > > Cc: Lee Jones <lee.jones@xxxxxxxxxx>
> > > Cc: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> > > Cc: Jingoo Han <jingoohan1@xxxxxxxxx>
> > > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > > Cc: Chris Healy <cphealy@xxxxxxxxx>
> > > Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> > > Cc: Aleksander Morgado <aleksander@xxxxxxxxxxxxx>
> > > Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
> > > ---
> > >    drivers/video/backlight/Kconfig             |  6 +++
> > >    drivers/video/backlight/Makefile            |  1 +
> > >    drivers/video/backlight/rave-sp-backlight.c | 82
> > > +++++++++++++++++++++++++++++
> > >    include/linux/mfd/rave-sp.h                 |  1 +
> > >    4 files changed, 90 insertions(+)
> > >    create mode 100644 drivers/video/backlight/rave-sp-backlight.c
> > >
> > > diff --git a/drivers/video/backlight/Kconfig
> > > b/drivers/video/backlight/Kconfig
> > > index 4e1d2ad50ba1..5d2d0d7e8100 100644
> > > --- a/drivers/video/backlight/Kconfig
> > > +++ b/drivers/video/backlight/Kconfig
> > > @@ -467,6 +467,12 @@ config BACKLIGHT_ARCXCNN
> > >            If you have an ARCxCnnnn family backlight say Y to enable
> > >            the backlight driver.
> > >    +config BACKLIGHT_RAVE_SP
> > > +       tristate "RAVE SP Backlight driver"
> > > +       depends on RAVE_SP_CORE
> > > +       help
> > > +         Support for backlight control on RAVE SP device.
> > > +
> > >    endif # BACKLIGHT_CLASS_DEVICE
> > >      endif # BACKLIGHT_LCD_SUPPORT
> > > diff --git a/drivers/video/backlight/Makefile
> > > b/drivers/video/backlight/Makefile
> > > index 5e28f01c8391..19da71d518bf 100644
> > > --- a/drivers/video/backlight/Makefile
> > > +++ b/drivers/video/backlight/Makefile
> > > @@ -57,3 +57,4 @@ obj-$(CONFIG_BACKLIGHT_TOSA)          += tosa_bl.o
> > >    obj-$(CONFIG_BACKLIGHT_TPS65217)      += tps65217_bl.o
> > >    obj-$(CONFIG_BACKLIGHT_WM831X)                += wm831x_bl.o
> > >    obj-$(CONFIG_BACKLIGHT_ARCXCNN)       += arcxcnn_bl.o
> > > +obj-$(CONFIG_BACKLIGHT_RAVE_SP)                += rave-sp-backlight.o
> > > diff --git a/drivers/video/backlight/rave-sp-backlight.c
> > > b/drivers/video/backlight/rave-sp-backlight.c
> > > new file mode 100644
> > > index 000000000000..62836ba561db
> > > --- /dev/null
> > > +++ b/drivers/video/backlight/rave-sp-backlight.c
> > > @@ -0,0 +1,82 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +
> > > +/*
> > > + * LCD Backlight driver for RAVE SP
> > > + *
> > > + * Copyright (C) 2018 Zodiac Inflight Innovations
> > > + *
> > > + */
> > > +
> > > +#include <linux/backlight.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mfd/rave-sp.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#define        RAVE_SP_BACKLIGHT_LCD_EN        BIT(7)
> > > +
> > > +static int rave_sp_backlight_update_status(struct backlight_device *bd)
> > > +{
> > > +       const struct backlight_properties *p = &bd->props;
> > > +       const u8 intensity =
> > > +               (p->power == FB_BLANK_UNBLANK) ? p->brightness : 0;
> > > +       struct rave_sp *sp = dev_get_drvdata(&bd->dev);
> > > +       u8 cmd[] = {
> > > +               [0] = RAVE_SP_CMD_SET_BACKLIGHT,
> > > +               [1] = 0,
> > > +               [2] = intensity ? RAVE_SP_BACKLIGHT_LCD_EN | intensity :
> > > 0,
> > > +               [3] = 0,
> > > +               [4] = 0,
> > > +       };
> > > +
> > > +       return rave_sp_exec(sp, cmd, sizeof(cmd), NULL, 0);
> > > +}
> > > +
> > > +static const struct backlight_ops rave_sp_backlight_ops = {
> > > +       .options        = BL_CORE_SUSPENDRESUME,
> > > +       .update_status  = rave_sp_backlight_update_status,
> > > +};
> > > +
> > > +static struct backlight_properties rave_sp_backlight_props = {
> > > +       .type           = BACKLIGHT_FIRMWARE,
> >
> >
> > I would have thought BACKLIGHT_PLATFORM would be more suitable here.
> > BACKLIGHT_FIRMWARE is used for things like the ACPI backlight driver.
> >
> > Why did you select this one?
>
> Good point and I don't think I have any justification to choose
> FIRMWARE over PLATFORM, will fix in v2.

Thanks. With that fixed:

Acked-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>


Daniel.