diff mbox series

drm/bridge: ti-sn65dsi86: Associate PWM device to auxiliary device

Message ID 20231127101547.734061-2-u.kleine-koenig@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series drm/bridge: ti-sn65dsi86: Associate PWM device to auxiliary device | expand

Commit Message

Uwe Kleine-König Nov. 27, 2023, 10:15 a.m. UTC 
It's the ti_sn65dsi86.pwm auxiliary driver that creates the pwmchip, so
let the auxiliary device be the parent of the pwm device.

Note that getting a reference to the ti-sn65dsi86's pwm using pwm_get()
isn't affected by this change as ti_sn65dsi86_add_aux_device() sets the
auxiliary device's of_node to that of the main device.

Also change PM runtime tracking and diagnostic messages to use that one.
As the PM runtime functions also handle parent devices this should work
fine, too.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

this patch has an (easy to resolve) conflict with a patch I sent earlier
"drm/bridge: ti-sn65dsi86: Simplify using pm_runtime_resume_and_get()"
(https://lore.kernel.org/dri-devel/20231123175425.496956-2-u.kleine-koenig@pengutronix.de).
I was unsure if I should base this new patch on that older one.

While I think the patch is fine, I'd have a better feeling about it if
someone could give feedback that the PWM still works as intended with
this change.

Best regards
Uwe

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)


base-commit: 48bbaf8b793e0770798519f8ee1ea2908ff0943a

Comments

Doug Anderson Nov. 28, 2023, 12:46 a.m. UTC | #1 
Hi,

On Mon, Nov 27, 2023 at 2:15 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> It's the ti_sn65dsi86.pwm auxiliary driver that creates the pwmchip, so
> let the auxiliary device be the parent of the pwm device.
>
> Note that getting a reference to the ti-sn65dsi86's pwm using pwm_get()
> isn't affected by this change as ti_sn65dsi86_add_aux_device() sets the
> auxiliary device's of_node to that of the main device.
>
> Also change PM runtime tracking and diagnostic messages to use that one.
> As the PM runtime functions also handle parent devices this should work
> fine, too.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
>
> this patch has an (easy to resolve) conflict with a patch I sent earlier
> "drm/bridge: ti-sn65dsi86: Simplify using pm_runtime_resume_and_get()"
> (https://lore.kernel.org/dri-devel/20231123175425.496956-2-u.kleine-koenig@pengutronix.de).
> I was unsure if I should base this new patch on that older one.
>
> While I think the patch is fine, I'd have a better feeling about it if
> someone could give feedback that the PWM still works as intended with
> this change.
>
> Best regards
> Uwe
>
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

This looks right to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

For now, I'll wait for Bjorn (or someone else who uses this PWM) to
give a Tested-by before applying.

-Doug
Nikita Travkin Dec. 8, 2023, 5:29 p.m. UTC | #2 
Uwe Kleine-König писал(а) 27.11.2023 15:15:
> It's the ti_sn65dsi86.pwm auxiliary driver that creates the pwmchip, so
> let the auxiliary device be the parent of the pwm device.
> 
> Note that getting a reference to the ti-sn65dsi86's pwm using pwm_get()
> isn't affected by this change as ti_sn65dsi86_add_aux_device() sets the
> auxiliary device's of_node to that of the main device.
> 
> Also change PM runtime tracking and diagnostic messages to use that one.
> As the PM runtime functions also handle parent devices this should work
> fine, too.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> this patch has an (easy to resolve) conflict with a patch I sent earlier
> "drm/bridge: ti-sn65dsi86: Simplify using pm_runtime_resume_and_get()"
> (https://lore.kernel.org/dri-devel/20231123175425.496956-2-u.kleine-koenig@pengutronix.de).
> I was unsure if I should base this new patch on that older one.
> 
> While I think the patch is fine, I'd have a better feeling about it if
> someone could give feedback that the PWM still works as intended with
> this change.
> 

Hi, with this patch applied, pwm backlight fails to
probe on sc7180-acer-aspire1:

[    0.377853] pwm-backlight backlight: failed to apply initial PWM state: -13
[    0.378349] pwm-backlight: probe of backlight failed with error -13

Seems like the pwmchip device should get runtime_pm enabled?
The following patch seem to fix it.

Nikita


diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 254f0039dad2..b6813f9f6a8f 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1593,6 +1593,8 @@ static int ti_sn_pwm_probe(struct auxiliary_device *adev,
 	pdata->pchip.of_xlate = of_pwm_single_xlate;
 	pdata->pchip.of_pwm_n_cells = 1;
 
+	pm_runtime_enable(&adev->dev);
+
 	return pwmchip_add(&pdata->pchip);
 }
Doug Anderson Dec. 8, 2023, 6:02 p.m. UTC | #3 
Hi,

On Fri, Dec 8, 2023 at 9:30 AM Nikita Travkin <nikita@trvn.ru> wrote:
>
> Uwe Kleine-König писал(а) 27.11.2023 15:15:
> > It's the ti_sn65dsi86.pwm auxiliary driver that creates the pwmchip, so
> > let the auxiliary device be the parent of the pwm device.
> >
> > Note that getting a reference to the ti-sn65dsi86's pwm using pwm_get()
> > isn't affected by this change as ti_sn65dsi86_add_aux_device() sets the
> > auxiliary device's of_node to that of the main device.
> >
> > Also change PM runtime tracking and diagnostic messages to use that one.
> > As the PM runtime functions also handle parent devices this should work
> > fine, too.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> > Hello,
> >
> > this patch has an (easy to resolve) conflict with a patch I sent earlier
> > "drm/bridge: ti-sn65dsi86: Simplify using pm_runtime_resume_and_get()"
> > (https://lore.kernel.org/dri-devel/20231123175425.496956-2-u.kleine-koenig@pengutronix.de).
> > I was unsure if I should base this new patch on that older one.
> >
> > While I think the patch is fine, I'd have a better feeling about it if
> > someone could give feedback that the PWM still works as intended with
> > this change.
> >
>
> Hi, with this patch applied, pwm backlight fails to
> probe on sc7180-acer-aspire1:
>
> [    0.377853] pwm-backlight backlight: failed to apply initial PWM state: -13
> [    0.378349] pwm-backlight: probe of backlight failed with error -13
>
> Seems like the pwmchip device should get runtime_pm enabled?

Glad you checked. Thanks!


> The following patch seem to fix it.
>
> Nikita
>
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 254f0039dad2..b6813f9f6a8f 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1593,6 +1593,8 @@ static int ti_sn_pwm_probe(struct auxiliary_device *adev,
>         pdata->pchip.of_xlate = of_pwm_single_xlate;
>         pdata->pchip.of_pwm_n_cells = 1;
>
> +       pm_runtime_enable(&adev->dev);
> +

I think you'd want devm_pm_runtime_enable(), which looks safe in this case.

-Doug
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index c45c07840f64..254f0039dad2 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1413,9 +1413,9 @@  static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	int ret;
 
 	if (!pdata->pwm_enabled) {
-		ret = pm_runtime_get_sync(pdata->dev);
+		ret = pm_runtime_get_sync(chip->dev);
 		if (ret < 0) {
-			pm_runtime_put_sync(pdata->dev);
+			pm_runtime_put_sync(chip->dev);
 			return ret;
 		}
 	}
@@ -1431,7 +1431,7 @@  static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 						 SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),
 						 SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));
 			if (ret) {
-				dev_err(pdata->dev, "failed to mux in PWM function\n");
+				dev_err(chip->dev, "failed to mux in PWM function\n");
 				goto out;
 			}
 		}
@@ -1507,7 +1507,7 @@  static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 		ret = regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div);
 		if (ret) {
-			dev_err(pdata->dev, "failed to update PWM_PRE_DIV\n");
+			dev_err(chip->dev, "failed to update PWM_PRE_DIV\n");
 			goto out;
 		}
 
@@ -1519,7 +1519,7 @@  static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		     FIELD_PREP(SN_PWM_INV_MASK, state->polarity == PWM_POLARITY_INVERSED);
 	ret = regmap_write(pdata->regmap, SN_PWM_EN_INV_REG, pwm_en_inv);
 	if (ret) {
-		dev_err(pdata->dev, "failed to update PWM_EN/PWM_INV\n");
+		dev_err(chip->dev, "failed to update PWM_EN/PWM_INV\n");
 		goto out;
 	}
 
@@ -1527,7 +1527,7 @@  static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 out:
 
 	if (!pdata->pwm_enabled)
-		pm_runtime_put_sync(pdata->dev);
+		pm_runtime_put_sync(chip->dev);
 
 	return ret;
 }
@@ -1587,7 +1587,7 @@  static int ti_sn_pwm_probe(struct auxiliary_device *adev,
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
 
-	pdata->pchip.dev = pdata->dev;
+	pdata->pchip.dev = &adev->dev;
 	pdata->pchip.ops = &ti_sn_pwm_ops;
 	pdata->pchip.npwm = 1;
 	pdata->pchip.of_xlate = of_pwm_single_xlate;
@@ -1603,7 +1603,7 @@  static void ti_sn_pwm_remove(struct auxiliary_device *adev)
 	pwmchip_remove(&pdata->pchip);
 
 	if (pdata->pwm_enabled)
-		pm_runtime_put_sync(pdata->dev);
+		pm_runtime_put_sync(&adev->dev);
 }
 
 static const struct auxiliary_device_id ti_sn_pwm_id_table[] = {