Re: [PATCH 2/4] video: backlight: lp855x: get PWM for PWM mode during probe
From: Uwe Kleine-König <hidden>
Date: 2023-06-14 08:40:22
Also in:
dri-devel, linux-arm-msm, linux-devicetree, linux-leds, linux-pwm, linux-tegra, lkml
On Sat, Apr 29, 2023 at 12:45:32PM +0200, Artur Weber wrote:
quoted hunk ↗ jump to hunk
Also deprecate the pwm-period DT property, as it is now redundant (pwms property already contains period value). Signed-off-by: Artur Weber <aweber.kernel@gmail.com> --- drivers/video/backlight/lp855x_bl.c | 48 ++++++++++++++++------------- 1 file changed, 26 insertions(+), 22 deletions(-)diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c index 81012bf29baf..21eb4943ed56 100644 --- a/drivers/video/backlight/lp855x_bl.c +++ b/drivers/video/backlight/lp855x_bl.c@@ -218,23 +218,10 @@ static int lp855x_configure(struct lp855x *lp) static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br) { - struct pwm_device *pwm; struct pwm_state state; - /* request pwm device with the consumer name */ - if (!lp->pwm) { - pwm = devm_pwm_get(lp->dev, lp->chipname); - if (IS_ERR(pwm)) - return; - - lp->pwm = pwm; - - pwm_init_state(lp->pwm, &state); - } else { - pwm_get_state(lp->pwm, &state); - } + pwm_get_state(lp->pwm, &state);
pwm_get_state returns an error code. Do you care if it fails? (You probably should.)
quoted hunk ↗ jump to hunk
- state.period = lp->pdata->period_ns; state.duty_cycle = div_u64(br * state.period, max_br); state.enabled = state.duty_cycle;@@ -339,6 +326,7 @@ static int lp855x_parse_dt(struct lp855x *lp) of_property_read_string(node, "bl-name", &pdata->name); of_property_read_u8(node, "dev-ctrl", &pdata->device_control); of_property_read_u8(node, "init-brt", &pdata->initial_brightness); + /* Deprecated, specify period in pwms property instead */ of_property_read_u32(node, "pwm-period", &pdata->period_ns); /* Fill ROM platform data if defined */@@ -399,6 +387,7 @@ static int lp855x_probe(struct i2c_client *cl) const struct i2c_device_id *id = i2c_client_get_device_id(cl); const struct acpi_device_id *acpi_id = NULL; struct device *dev = &cl->dev; + struct pwm_state pwmstate; struct lp855x *lp; int ret;@@ -457,11 +446,6 @@ static int lp855x_probe(struct i2c_client *cl) } } - if (lp->pdata->period_ns > 0) - lp->mode = PWM_BASED; - else - lp->mode = REGISTER_BASED; - lp->supply = devm_regulator_get(dev, "power"); if (IS_ERR(lp->supply)) { if (PTR_ERR(lp->supply) == -EPROBE_DEFER)@@ -472,11 +456,31 @@ static int lp855x_probe(struct i2c_client *cl) lp->enable = devm_regulator_get_optional(dev, "enable"); if (IS_ERR(lp->enable)) { ret = PTR_ERR(lp->enable); - if (ret == -ENODEV) { + if (ret == -ENODEV) lp->enable = NULL; - } else { + else return dev_err_probe(dev, ret, "getting enable regulator\n"); - } + } + + lp->pwm = devm_pwm_get(lp->dev, lp->chipname); + if (IS_ERR(lp->pwm)) { + ret = PTR_ERR(lp->pwm); + if (ret == -ENODEV || ret == -EINVAL)
Why would you ignore EINVAL?
+ lp->pwm = NULL;
+ else
+ return dev_err_probe(dev, ret, "getting PWM\n");
+
+ lp->mode = REGISTER_BASED;
+ dev_dbg(dev, "mode: register based\n");
+ } else {pwmstate could be declared here.
+ pwm_init_state(lp->pwm, &pwmstate); + /* Legacy platform data compatibility */ + if (lp->pdata->period_ns > 0) + pwmstate.period = lp->pdata->period_ns; + pwm_apply_state(lp->pwm, &pwmstate);
This is a change in behaviour. Before lp855x_probe() didn't modify the state the bootloader left the backlight in. Now you're disabling it (I think). Is this intended? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachments
- signature.asc [application/pgp-signature] 488 bytes