On Wed, Jun 12, 2019 at 11:26:15AM +0100, Daniel Thompson wrote: > Hi Shobhit > > Thanks for the patch. Feedback below...
Hi Daneil, You provided some valuable feedback. Thank you for your time and effort. > > > On Tue, Jun 11, 2019 at 09:32:32PM -0700, Shobhit Kukreti wrote: > > Port the sky81452-backlight driver to adhere to new gpio descriptor based > > APIs. Modified the file sky81452-backlight.c and sky81452-backlight.h. > > The gpio descriptor property in device tree should be "sky81452-en-gpios" > > That is contradicted by the device tree bindings. The property should > remain "gpios" as it was before this conversion. You are correct. > > > > Removed unnecessary header files "linux/gpio.h" and "linux/of_gpio.h". > > > > Signed-off-by: Shobhit Kukreti <[email protected]> > > What level of testing have you done? Is this a fix for hardware you own > or a cleanup after searching the sources? > I did not the test on actual hardware. I was trying to do a clean up and boot on QEMU/rock960 board. I will drop the patch until I can find the sk81452 hardware to test it. > > > --- > > drivers/video/backlight/sky81452-backlight.c | 24 > > ++++++++++++------------ > > include/linux/platform_data/sky81452-backlight.h | 4 +++- > > 2 files changed, 15 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/video/backlight/sky81452-backlight.c > > b/drivers/video/backlight/sky81452-backlight.c > > index d414c7a..12ef628 100644 > > --- a/drivers/video/backlight/sky81452-backlight.c > > +++ b/drivers/video/backlight/sky81452-backlight.c > > @@ -19,12 +19,10 @@ > > > > #include <linux/backlight.h> > > #include <linux/err.h> > > -#include <linux/gpio.h> > > #include <linux/init.h> > > #include <linux/kernel.h> > > #include <linux/module.h> > > #include <linux/of.h> > > -#include <linux/of_gpio.h> > > #include <linux/platform_device.h> > > #include <linux/regmap.h> > > #include <linux/platform_data/sky81452-backlight.h> > > @@ -193,7 +191,6 @@ static struct sky81452_bl_platform_data > > *sky81452_bl_parse_dt( > > pdata->ignore_pwm = of_property_read_bool(np, "skyworks,ignore-pwm"); > > pdata->dpwm_mode = of_property_read_bool(np, "skyworks,dpwm-mode"); > > pdata->phase_shift = of_property_read_bool(np, "skyworks,phase-shift"); > > - pdata->gpio_enable = of_get_gpio(np, 0); > > > > ret = of_property_count_u32_elems(np, "led-sources"); > > if (ret < 0) { > > @@ -274,13 +271,17 @@ static int sky81452_bl_probe(struct platform_device > > *pdev) > > if (IS_ERR(pdata)) > > return PTR_ERR(pdata); > > } > > - > > - if (gpio_is_valid(pdata->gpio_enable)) { > > - ret = devm_gpio_request_one(dev, pdata->gpio_enable, > > - GPIOF_OUT_INIT_HIGH, "sky81452-en"); > > - if (ret < 0) { > > - dev_err(dev, "failed to request GPIO. err=%d\n", ret); > > - return ret; > > + pdata->gpiod_enable = devm_gpiod_get(dev, "sk81452-en", GPIOD_OUT_HIGH); > > As above... I think the second argument here needs to be NULL in order > to preserve the current DT bindings. You are correct. I was testing this driver with a custom dtb in rock960 board and named the property as above in the dts. It should be NULL. > > > + if (IS_ERR(pdata->gpiod_enable)) { > > + long ret = PTR_ERR(pdata->gpiod_enable); > > + > > + /** > > Nitpicking... but no second star here. That's a trigger symbold for > documentation processors. That is a great feedback. I will keep that in mind. > > > + * gpiod_enable is optional in device tree. > > + * Return error only if gpio was assigned in device tree > > Also nitpicking but I had to read this a few times because > gpiod_enable is not in device tree, gpios is. You are correct. I will make necessary changes. > > This is a common pattern so the comment can be very short. Something > like: > > This DT property is optional so no need to propagate ENOENT Noted. > > > > + */ > > + if (ret != -ENOENT) { > > + dev_err(dev, "failed to request GPIO. err=%ld\n", ret); > > + return PTR_ERR(pdata->gpiod_enable); > > } > > } > > > > @@ -323,8 +324,7 @@ static int sky81452_bl_remove(struct platform_device > > *pdev) > > bd->props.brightness = 0; > > backlight_update_status(bd); > > > > - if (gpio_is_valid(pdata->gpio_enable)) > > - gpio_set_value_cansleep(pdata->gpio_enable, 0); > > + gpiod_set_value_cansleep(pdata->gpiod_enable, 0); > > > > return 0; > > } > > diff --git a/include/linux/platform_data/sky81452-backlight.h > > b/include/linux/platform_data/sky81452-backlight.h > > index 1231e9b..dc4cb85 100644 > > --- a/include/linux/platform_data/sky81452-backlight.h > > +++ b/include/linux/platform_data/sky81452-backlight.h > > @@ -20,6 +20,8 @@ > > #ifndef _SKY81452_BACKLIGHT_H > > #define _SKY81452_BACKLIGHT_H > > > > +#include <linux/gpio/consumer.h> > > + > > This heaer file should be included from the C file... it is not required > to parse the header. I appreciate the feedback. I will keep that in mind to avoid future errors. > > > Daniel. > > > > /** > > * struct sky81452_platform_data > > * @name: backlight driver name. > > @@ -34,7 +36,7 @@ > > */ > > struct sky81452_bl_platform_data { > > const char *name; > > - int gpio_enable; > > + struct gpio_desc *gpiod_enable; > > unsigned int enable; > > bool ignore_pwm; > > bool dpwm_mode; > > -- > > 2.7.4 > >
