| Message ID | 20190722150302.29526-8-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | backlight: gpio: simplify the driver | expand |
pon., 22 lip 2019 o 18:09 Andy Shevchenko <andriy.shevchenko@linux.intel.com> napisał(a): > > On Mon, Jul 22, 2019 at 05:03:02PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Instead of dereferencing pdev each time, use a helper variable for > > the associated device pointer. > > > static int gpio_backlight_probe(struct platform_device *pdev) > > { > > - struct gpio_backlight_platform_data *pdata = > > - dev_get_platdata(&pdev->dev); > > + struct gpio_backlight_platform_data *pdata; > > struct backlight_properties props; > > struct backlight_device *bl; > > struct gpio_backlight *gbl; > > enum gpiod_flags flags; > > + struct device *dev; > > Can't we do > > struct device dev = &pdev->dev; > struct gpio_backlight_platform_data *pdata = dev_get_platdata(dev); > > ? It fits 80 nicely. > IMO it's more readable like that with the reverse christmas tree layout. Bart
On Tue, Jul 23, 2019 at 08:29:52AM +0200, Bartosz Golaszewski wrote: > pon., 22 lip 2019 o 18:09 Andy Shevchenko > <andriy.shevchenko@linux.intel.com> napisał(a): > > > > On Mon, Jul 22, 2019 at 05:03:02PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > Instead of dereferencing pdev each time, use a helper variable for > > > the associated device pointer. > > > > > static int gpio_backlight_probe(struct platform_device *pdev) > > > { > > > - struct gpio_backlight_platform_data *pdata = > > > - dev_get_platdata(&pdev->dev); > > > + struct gpio_backlight_platform_data *pdata; > > > struct backlight_properties props; > > > struct backlight_device *bl; > > > struct gpio_backlight *gbl; > > > enum gpiod_flags flags; > > > + struct device *dev; > > > > Can't we do > > > > struct device dev = &pdev->dev; > > struct gpio_backlight_platform_data *pdata = dev_get_platdata(dev); > > > > ? It fits 80 nicely. > > > > IMO it's more readable like that with the reverse christmas tree layout. There is no requirement for reverse christmas tree layout for this area of the kernel (and especially not where RCTL is used as a justification to avoid initializers). I have a weak personal preference for initializers although it is sufficiently weak I was happy to put a reviewed by on the original patch without comment. Daniel.
On Tue, Jul 23, 2019 at 08:29:52AM +0200, Bartosz Golaszewski wrote: > pon., 22 lip 2019 o 18:09 Andy Shevchenko > <andriy.shevchenko@linux.intel.com> napisał(a): > > > > On Mon, Jul 22, 2019 at 05:03:02PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > Instead of dereferencing pdev each time, use a helper variable for > > > the associated device pointer. > > > > > static int gpio_backlight_probe(struct platform_device *pdev) > > > { > > > - struct gpio_backlight_platform_data *pdata = > > > - dev_get_platdata(&pdev->dev); > > > + struct gpio_backlight_platform_data *pdata; > > > struct backlight_properties props; > > > struct backlight_device *bl; > > > struct gpio_backlight *gbl; > > > enum gpiod_flags flags; > > > + struct device *dev; > > > > Can't we do > > > > struct device dev = &pdev->dev; > > struct gpio_backlight_platform_data *pdata = dev_get_platdata(dev); > > > > ? It fits 80 nicely. > > > > IMO it's more readable like that with the reverse christmas tree layout. It makes more churn in the original code and for initializers the order is defined by its nature.
wt., 23 lip 2019 o 17:34 Andy Shevchenko <andriy.shevchenko@linux.intel.com> napisał(a): > > On Tue, Jul 23, 2019 at 08:29:52AM +0200, Bartosz Golaszewski wrote: > > pon., 22 lip 2019 o 18:09 Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> napisał(a): > > > > > > On Mon, Jul 22, 2019 at 05:03:02PM +0200, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > > > Instead of dereferencing pdev each time, use a helper variable for > > > > the associated device pointer. > > > > > > > static int gpio_backlight_probe(struct platform_device *pdev) > > > > { > > > > - struct gpio_backlight_platform_data *pdata = > > > > - dev_get_platdata(&pdev->dev); > > > > + struct gpio_backlight_platform_data *pdata; > > > > struct backlight_properties props; > > > > struct backlight_device *bl; > > > > struct gpio_backlight *gbl; > > > > enum gpiod_flags flags; > > > > + struct device *dev; > > > > > > Can't we do > > > > > > struct device dev = &pdev->dev; > > > struct gpio_backlight_platform_data *pdata = dev_get_platdata(dev); > > > > > > ? It fits 80 nicely. > > > > > > > IMO it's more readable like that with the reverse christmas tree layout. > > It makes more churn in the original code and for initializers the order is > defined by its nature. > > -- > With Best Regards, > Andy Shevchenko > > Fair enough, I changed it in v3. Bart
diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c index cd6a75bca9cc..091ff799659a 100644 --- a/drivers/video/backlight/gpio_backlight.c +++ b/drivers/video/backlight/gpio_backlight.c @@ -54,29 +54,32 @@ static const struct backlight_ops gpio_backlight_ops = { static int gpio_backlight_probe(struct platform_device *pdev) { - struct gpio_backlight_platform_data *pdata = - dev_get_platdata(&pdev->dev); + struct gpio_backlight_platform_data *pdata; struct backlight_properties props; struct backlight_device *bl; struct gpio_backlight *gbl; enum gpiod_flags flags; + struct device *dev; int ret, def_value; - gbl = devm_kzalloc(&pdev->dev, sizeof(*gbl), GFP_KERNEL); + dev = &pdev->dev; + pdata = dev_get_platdata(dev); + + gbl = devm_kzalloc(dev, sizeof(*gbl), GFP_KERNEL); if (gbl == NULL) return -ENOMEM; if (pdata) gbl->fbdev = pdata->fbdev; - def_value = device_property_read_bool(&pdev->dev, "default-on"); + def_value = device_property_read_bool(dev, "default-on"); flags = def_value ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW; - gbl->gpiod = devm_gpiod_get(&pdev->dev, NULL, flags); + gbl->gpiod = devm_gpiod_get(dev, NULL, flags); if (IS_ERR(gbl->gpiod)) { ret = PTR_ERR(gbl->gpiod); if (ret != -EPROBE_DEFER) { - dev_err(&pdev->dev, + dev_err(dev, "Error: The gpios parameter is missing or invalid.\n"); } return ret; @@ -85,11 +88,10 @@ static int gpio_backlight_probe(struct platform_device *pdev) memset(&props, 0, sizeof(props)); props.type = BACKLIGHT_RAW; props.max_brightness = 1; - bl = devm_backlight_device_register(&pdev->dev, dev_name(&pdev->dev), - &pdev->dev, gbl, &gpio_backlight_ops, - &props); + bl = devm_backlight_device_register(dev, dev_name(dev), dev, gbl, + &gpio_backlight_ops, &props); if (IS_ERR(bl)) { - dev_err(&pdev->dev, "failed to register backlight\n"); + dev_err(dev, "failed to register backlight\n"); return PTR_ERR(bl); }