lkml.org 
[lkml]   [2015]   [Jan]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
    On Thu 29 Jan 04:48 PST 2015, Ivan T. Ivanov wrote:

    >
    > Hi Bjorn,
    >
    > Just few nitpick comments.
    >

    Thanks.

    > On Fri, 2015-01-23 at 16:54 -0800, Bjorn Andersson wrote:
    > > From: Courtney Cavin cavin@sonymobile.com>
    > >
    > > This adds support for the WLED ('White' LED) block on Qualcomm's
    > > PM8941 PMICs.
    > >
    > > Signed-off-by: Courtney Cavin cavin@sonymobile.com>
    > > Signed-off-by: Bjorn Andersson andersson@sonymobile.com>
    > > ---
    > > drivers/leds/Kconfig | 8 +
    > > drivers/leds/Makefile | 1 +
    > > drivers/leds/leds-pm8941-wled.c | 459 ++++++++++++++++++++++++++++++++++++++++
    > > 3 files changed, 468 insertions(+)
    > > create mode 100644 drivers/leds/leds-pm8941-wled.c
    > >
    >
    > <snip>
    >
    > > +
    > > +#define PM8941_WLED_REG_VAL_BASE 0x40
    > > +#define PM8941_WLED_REG_VAL_MAX 0xFFF
    > > +
    > > +#define PM8941_WLED_REG_MOD_EN 0x46
    > > +#define PM8941_WLED_REG_MOD_EN_BIT BIT(7)
    > > +#define PM8941_WLED_REG_MOD_EN_MASK BIT(7)
    >
    > Is it possible bit definitions to have same indentation like registers offsets?
    >

    Well, yes ;)

    But I like the separation, so unless Bryan would like me to change it I prefer
    to leave it as is.

    > >
    > > +struct pm8941_wled_config {
    > > + u32 i_boost_limit;
    > > + u32 ovp;
    > > + u32 switch_freq;
    > > + u32 num_strings;
    > > + u32 i_limit;
    > > + bool cs_out_en;
    > > + bool ext_gen;
    > > + bool cabc_en;
    > > +};
    > > +
    >
    > Could this be further squashed to bellow structure?
    >

    It could, but I see that it would simplify things.

    > > +struct pm8941_wled {
    > > + struct regmap *regmap;
    > > + u16 addr;
    > > +
    > > + struct led_classdev cdev;
    > > +
    > > + struct pm8941_wled_config cfg;
    > > +};
    > > +
    > >
    >
    > <snip>
    >
    > > +static void pm8941_wled_set_brightness(struct led_classdev *cdev,
    > > + enum
    > > led_brightness value)
    > > +{
    > > + if (pm8941_wled_set(cdev, value)) {
    >
    > pm8941_wled_set() is used only here, could it be merged into this function?
    >

    It could, but that would just mean that we move these lines into the tail of
    pm8941_wled_set and replace all the returns with a bunch of gotos. So I don't
    think it's cleaner.

    > > + dev_err(cdev->dev, "Unable to set brightness\n");
    > > + return;
    > > + }
    > > + cdev->brightness = value;
    > > +}
    > > +
    > >
    >
    > Otherwise it looks good. Driver is loaded and device is detected
    > properly (i have added readings for type and subtype registers).
    > Do you know where I can measure result from changing brightness
    > sysfs entry. I am using 8074 dragonboard?
    >

    I'm not sure how this is exposed on the dragonboard. I've tested it on Xperia
    Z1 (honami) and the display lights up nicely.

    Regards,
    Bjorn


    \
     
     \ /
      Last update: 2015-01-29 20:21    [W:5.078 / U:0.916 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site