| Message ID | 20230703214715.623447-3-paul@crapouillou.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Galaxy S2 (i9100) panel updates | expand |
Hi, > -----Original Message----- > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of > Paul Cercueil > Sent: Tuesday, July 4, 2023 6:47 AM > To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Rob Herring > <robh+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Alim Akhtar > <alim.akhtar@samsung.com>; Neil Armstrong <neil.armstrong@linaro.org>; Sam > Ravnborg <sam@ravnborg.org> > Cc: devicetree@vger.kernel.org; linux-samsung-soc@vger.kernel.org; linux- > kernel@vger.kernel.org; dri-devel@lists.freedesktop.org; Paul Cercueil > <paul@crapouillou.net>; linux-arm-kernel@lists.infradead.org > Subject: [PATCH 2/3] drm/panel: ld9040: Register a backlight device > > Register a backlight device to be able to switch between all the gamma > levels. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > drivers/gpu/drm/panel/panel-samsung-ld9040.c | 40 ++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/drivers/gpu/drm/panel/panel-samsung-ld9040.c > b/drivers/gpu/drm/panel/panel-samsung-ld9040.c > index 7fd9444b42c5..b4f87d6244cb 100644 > --- a/drivers/gpu/drm/panel/panel-samsung-ld9040.c > +++ b/drivers/gpu/drm/panel/panel-samsung-ld9040.c > @@ -8,6 +8,7 @@ > * Andrzej Hajda <a.hajda@samsung.com> > */ > > +#include <linux/backlight.h> > #include <linux/delay.h> > #include <linux/gpio/consumer.h> > #include <linux/module.h> > @@ -311,8 +312,40 @@ static int ld9040_parse_dt(struct ld9040 *ctx) > return 0; > } > > +static int ld9040_bl_update_status(struct backlight_device *dev) > +{ > + struct ld9040 *ctx = dev_get_drvdata(&dev->dev); > + > + ctx->brightness = dev->props.brightness; > + ld9040_brightness_set(ctx); > + > + return 0; > +} > + > +static int ld9040_bl_get_intensity(struct backlight_device *dev) > +{ > + if (dev->props.fb_blank == FB_BLANK_UNBLANK && fb_blank member is deprecated according to the description of backlight.h file so you could drop above condition I think. Thanks, Inki Dae
Hi Inki, Le mardi 04 juillet 2023 à 08:49 +0900, 대인기/Tizen Platform Lab(SR)/삼성전자 a écrit : > Hi, > > > -----Original Message----- > > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf > > Of > > Paul Cercueil > > Sent: Tuesday, July 4, 2023 6:47 AM > > To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Rob > > Herring > > <robh+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Alim > > Akhtar > > <alim.akhtar@samsung.com>; Neil Armstrong > > <neil.armstrong@linaro.org>; Sam > > Ravnborg <sam@ravnborg.org> > > Cc: devicetree@vger.kernel.org; linux-samsung-soc@vger.kernel.org; > > linux- > > kernel@vger.kernel.org; dri-devel@lists.freedesktop.org; Paul > > Cercueil > > <paul@crapouillou.net>; linux-arm-kernel@lists.infradead.org > > Subject: [PATCH 2/3] drm/panel: ld9040: Register a backlight device > > > > Register a backlight device to be able to switch between all the > > gamma > > levels. > > > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > > --- > > drivers/gpu/drm/panel/panel-samsung-ld9040.c | 40 > > ++++++++++++++++++++ > > 1 file changed, 40 insertions(+) > > > > diff --git a/drivers/gpu/drm/panel/panel-samsung-ld9040.c > > b/drivers/gpu/drm/panel/panel-samsung-ld9040.c > > index 7fd9444b42c5..b4f87d6244cb 100644 > > --- a/drivers/gpu/drm/panel/panel-samsung-ld9040.c > > +++ b/drivers/gpu/drm/panel/panel-samsung-ld9040.c > > @@ -8,6 +8,7 @@ > > * Andrzej Hajda <a.hajda@samsung.com> > > */ > > > > +#include <linux/backlight.h> > > #include <linux/delay.h> > > #include <linux/gpio/consumer.h> > > #include <linux/module.h> > > @@ -311,8 +312,40 @@ static int ld9040_parse_dt(struct ld9040 *ctx) > > return 0; > > } > > > > +static int ld9040_bl_update_status(struct backlight_device *dev) > > +{ > > + struct ld9040 *ctx = dev_get_drvdata(&dev->dev); > > + > > + ctx->brightness = dev->props.brightness; > > + ld9040_brightness_set(ctx); > > + > > + return 0; > > +} > > + > > +static int ld9040_bl_get_intensity(struct backlight_device *dev) > > +{ > > + if (dev->props.fb_blank == FB_BLANK_UNBLANK && > > fb_blank member is deprecated according to the description of > backlight.h > file so you could drop above condition I think. Thanks. I'll send a V2. Cheers, -Paul
On 03/07/2023 23:47, Paul Cercueil wrote: > Register a backlight device to be able to switch between all the gamma > levels. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > drivers/gpu/drm/panel/panel-samsung-ld9040.c | 40 ++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/drivers/gpu/drm/panel/panel-samsung-ld9040.c b/drivers/gpu/drm/panel/panel-samsung-ld9040.c > index 7fd9444b42c5..b4f87d6244cb 100644 > --- a/drivers/gpu/drm/panel/panel-samsung-ld9040.c > +++ b/drivers/gpu/drm/panel/panel-samsung-ld9040.c > @@ -8,6 +8,7 @@ > * Andrzej Hajda <a.hajda@samsung.com> > */ > > +#include <linux/backlight.h> > #include <linux/delay.h> > #include <linux/gpio/consumer.h> > #include <linux/module.h> > @@ -311,8 +312,40 @@ static int ld9040_parse_dt(struct ld9040 *ctx) > return 0; > } > > +static int ld9040_bl_update_status(struct backlight_device *dev) > +{ > + struct ld9040 *ctx = dev_get_drvdata(&dev->dev); > + > + ctx->brightness = dev->props.brightness; > + ld9040_brightness_set(ctx); > + > + return 0; > +} > + > +static int ld9040_bl_get_intensity(struct backlight_device *dev) > +{ > + if (dev->props.fb_blank == FB_BLANK_UNBLANK && > + dev->props.power == FB_BLANK_UNBLANK) > + return dev->props.brightness; > + > + return 0; > +} You can totally drop the _get_brightness. Neil > + > +static const struct backlight_ops ld9040_bl_ops = { > + .get_brightness = ld9040_bl_get_intensity, > + .update_status = ld9040_bl_update_status, > +}; > + > +static const struct backlight_properties ld9040_bl_props = { > + .type = BACKLIGHT_RAW, > + .scale = BACKLIGHT_SCALE_NON_LINEAR, > + .max_brightness = ARRAY_SIZE(ld9040_gammas) - 1, > + .brightness = ARRAY_SIZE(ld9040_gammas) - 1, > +}; > + > static int ld9040_probe(struct spi_device *spi) > { > + struct backlight_device *bldev; > struct device *dev = &spi->dev; > struct ld9040 *ctx; > int ret; > @@ -354,6 +387,13 @@ static int ld9040_probe(struct spi_device *spi) > drm_panel_init(&ctx->panel, dev, &ld9040_drm_funcs, > DRM_MODE_CONNECTOR_DPI); > > + > + bldev = devm_backlight_device_register(dev, dev_name(dev), dev, > + ctx, &ld9040_bl_ops, > + &ld9040_bl_props); > + if (IS_ERR(bldev)) > + return PTR_ERR(bldev); > + > drm_panel_add(&ctx->panel); > > return 0;
Hi Neil, Le mercredi 05 juillet 2023 à 15:57 +0200, Neil Armstrong a écrit : > On 03/07/2023 23:47, Paul Cercueil wrote: > > Register a backlight device to be able to switch between all the > > gamma > > levels. > > > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > > --- > > drivers/gpu/drm/panel/panel-samsung-ld9040.c | 40 > > ++++++++++++++++++++ > > 1 file changed, 40 insertions(+) > > > > diff --git a/drivers/gpu/drm/panel/panel-samsung-ld9040.c > > b/drivers/gpu/drm/panel/panel-samsung-ld9040.c > > index 7fd9444b42c5..b4f87d6244cb 100644 > > --- a/drivers/gpu/drm/panel/panel-samsung-ld9040.c > > +++ b/drivers/gpu/drm/panel/panel-samsung-ld9040.c > > @@ -8,6 +8,7 @@ > > * Andrzej Hajda <a.hajda@samsung.com> > > */ > > > > +#include <linux/backlight.h> > > #include <linux/delay.h> > > #include <linux/gpio/consumer.h> > > #include <linux/module.h> > > @@ -311,8 +312,40 @@ static int ld9040_parse_dt(struct ld9040 *ctx) > > return 0; > > } > > > > +static int ld9040_bl_update_status(struct backlight_device *dev) > > +{ > > + struct ld9040 *ctx = dev_get_drvdata(&dev->dev); > > + > > + ctx->brightness = dev->props.brightness; > > + ld9040_brightness_set(ctx); > > + > > + return 0; > > +} > > + > > +static int ld9040_bl_get_intensity(struct backlight_device *dev) > > +{ > > + if (dev->props.fb_blank == FB_BLANK_UNBLANK && > > + dev->props.power == FB_BLANK_UNBLANK) > > + return dev->props.brightness; > > + > > + return 0; > > +} > > You can totally drop the _get_brightness. The current behaviour is to return 0 when the framebuffer is blanked. A few drivers do that so I thought it was the norm; and the backlight core doesn't do that by default (and just uses dev->props.brightness). It is not clear to me if that's the preferred behaviour. The "backlight_get_brightness" function in backlight.h seems to suggest that the current behaviour is correct, unless it is not supposed to be used in the backlight_ops.get_brightness() callback. Then in that case some other drivers get it wrong too. Cheers, -Paul > Neil > > > + > > +static const struct backlight_ops ld9040_bl_ops = { > > + .get_brightness = ld9040_bl_get_intensity, > > + .update_status = ld9040_bl_update_status, > > +}; > > + > > +static const struct backlight_properties ld9040_bl_props = { > > + .type = BACKLIGHT_RAW, > > + .scale = BACKLIGHT_SCALE_NON_LINEAR, > > + .max_brightness = ARRAY_SIZE(ld9040_gammas) - 1, > > + .brightness = ARRAY_SIZE(ld9040_gammas) - 1, > > +}; > > + > > static int ld9040_probe(struct spi_device *spi) > > { > > + struct backlight_device *bldev; > > struct device *dev = &spi->dev; > > struct ld9040 *ctx; > > int ret; > > @@ -354,6 +387,13 @@ static int ld9040_probe(struct spi_device > > *spi) > > drm_panel_init(&ctx->panel, dev, &ld9040_drm_funcs, > > DRM_MODE_CONNECTOR_DPI); > > > > + > > + bldev = devm_backlight_device_register(dev, dev_name(dev), > > dev, > > + ctx, &ld9040_bl_ops, > > + &ld9040_bl_props); > > + if (IS_ERR(bldev)) > > + return PTR_ERR(bldev); > > + > > drm_panel_add(&ctx->panel); > > > > return 0; >
Hi Paul, On Wed, Jul 05, 2023 at 04:38:05PM +0200, Paul Cercueil wrote: > Hi Neil, > > Le mercredi 05 juillet 2023 à 15:57 +0200, Neil Armstrong a écrit : > > On 03/07/2023 23:47, Paul Cercueil wrote: > > > Register a backlight device to be able to switch between all the > > > gamma > > > levels. > > > > > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > > > --- > > > drivers/gpu/drm/panel/panel-samsung-ld9040.c | 40 > > > ++++++++++++++++++++ > > > 1 file changed, 40 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/panel/panel-samsung-ld9040.c > > > b/drivers/gpu/drm/panel/panel-samsung-ld9040.c > > > index 7fd9444b42c5..b4f87d6244cb 100644 > > > --- a/drivers/gpu/drm/panel/panel-samsung-ld9040.c > > > +++ b/drivers/gpu/drm/panel/panel-samsung-ld9040.c > > > @@ -8,6 +8,7 @@ > > > * Andrzej Hajda <a.hajda@samsung.com> > > > */ > > > > > > +#include <linux/backlight.h> > > > #include <linux/delay.h> > > > #include <linux/gpio/consumer.h> > > > #include <linux/module.h> > > > @@ -311,8 +312,40 @@ static int ld9040_parse_dt(struct ld9040 *ctx) > > > return 0; > > > } > > > > > > +static int ld9040_bl_update_status(struct backlight_device *dev) > > > +{ > > > + struct ld9040 *ctx = dev_get_drvdata(&dev->dev); > > > + > > > + ctx->brightness = dev->props.brightness; Use backlight_get_brightness(dev); > > > + ld9040_brightness_set(ctx); > > > + > > > + return 0; > > > +} > > > + > > > +static int ld9040_bl_get_intensity(struct backlight_device *dev) > > > +{ > > > + if (dev->props.fb_blank == FB_BLANK_UNBLANK && > > > + dev->props.power == FB_BLANK_UNBLANK) > > > + return dev->props.brightness; > > > + > > > + return 0; > > > +} > > > > You can totally drop the _get_brightness. > > The current behaviour is to return 0 when the framebuffer is blanked. A > few drivers do that so I thought it was the norm; and the backlight > core doesn't do that by default (and just uses dev->props.brightness). > > It is not clear to me if that's the preferred behaviour. The > "backlight_get_brightness" function in backlight.h seems to suggest > that the current behaviour is correct, unless it is not supposed to be > used in the backlight_ops.get_brightness() callback. Then in that case > some other drivers get it wrong too. Several drivers get it wrong. You are supposed to provide get_brightness only when you read back a value from the HW, which is not the case here so just drop it is the right choice. Sam
Hi Paul, On Mon, Jul 03, 2023 at 11:47:14PM +0200, Paul Cercueil wrote: > Register a backlight device to be able to switch between all the gamma > levels. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > drivers/gpu/drm/panel/panel-samsung-ld9040.c | 40 ++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/drivers/gpu/drm/panel/panel-samsung-ld9040.c b/drivers/gpu/drm/panel/panel-samsung-ld9040.c > index 7fd9444b42c5..b4f87d6244cb 100644 > --- a/drivers/gpu/drm/panel/panel-samsung-ld9040.c > +++ b/drivers/gpu/drm/panel/panel-samsung-ld9040.c > @@ -8,6 +8,7 @@ > * Andrzej Hajda <a.hajda@samsung.com> > */ > > +#include <linux/backlight.h> > #include <linux/delay.h> > #include <linux/gpio/consumer.h> > #include <linux/module.h> > @@ -311,8 +312,40 @@ static int ld9040_parse_dt(struct ld9040 *ctx) > return 0; > } > > +static int ld9040_bl_update_status(struct backlight_device *dev) > +{ > + struct ld9040 *ctx = dev_get_drvdata(&dev->dev); There is also the helper bl_get_data() - that do the same. Sam
diff --git a/drivers/gpu/drm/panel/panel-samsung-ld9040.c b/drivers/gpu/drm/panel/panel-samsung-ld9040.c index 7fd9444b42c5..b4f87d6244cb 100644 --- a/drivers/gpu/drm/panel/panel-samsung-ld9040.c +++ b/drivers/gpu/drm/panel/panel-samsung-ld9040.c @@ -8,6 +8,7 @@ * Andrzej Hajda <a.hajda@samsung.com> */ +#include <linux/backlight.h> #include <linux/delay.h> #include <linux/gpio/consumer.h> #include <linux/module.h> @@ -311,8 +312,40 @@ static int ld9040_parse_dt(struct ld9040 *ctx) return 0; } +static int ld9040_bl_update_status(struct backlight_device *dev) +{ + struct ld9040 *ctx = dev_get_drvdata(&dev->dev); + + ctx->brightness = dev->props.brightness; + ld9040_brightness_set(ctx); + + return 0; +} + +static int ld9040_bl_get_intensity(struct backlight_device *dev) +{ + if (dev->props.fb_blank == FB_BLANK_UNBLANK && + dev->props.power == FB_BLANK_UNBLANK) + return dev->props.brightness; + + return 0; +} + +static const struct backlight_ops ld9040_bl_ops = { + .get_brightness = ld9040_bl_get_intensity, + .update_status = ld9040_bl_update_status, +}; + +static const struct backlight_properties ld9040_bl_props = { + .type = BACKLIGHT_RAW, + .scale = BACKLIGHT_SCALE_NON_LINEAR, + .max_brightness = ARRAY_SIZE(ld9040_gammas) - 1, + .brightness = ARRAY_SIZE(ld9040_gammas) - 1, +}; + static int ld9040_probe(struct spi_device *spi) { + struct backlight_device *bldev; struct device *dev = &spi->dev; struct ld9040 *ctx; int ret; @@ -354,6 +387,13 @@ static int ld9040_probe(struct spi_device *spi) drm_panel_init(&ctx->panel, dev, &ld9040_drm_funcs, DRM_MODE_CONNECTOR_DPI); + + bldev = devm_backlight_device_register(dev, dev_name(dev), dev, + ctx, &ld9040_bl_ops, + &ld9040_bl_props); + if (IS_ERR(bldev)) + return PTR_ERR(bldev); + drm_panel_add(&ctx->panel); return 0;
Register a backlight device to be able to switch between all the gamma levels. Signed-off-by: Paul Cercueil <paul@crapouillou.net> --- drivers/gpu/drm/panel/panel-samsung-ld9040.c | 40 ++++++++++++++++++++ 1 file changed, 40 insertions(+)