If the bridge cannot get powered on, there's no reason to try to
communicate with it: change the ps8640_bridge_poweron function to
return an error value to the caller, so that we can avoid calling
ps8640_bridge_vdo_control() in ps8640_pre_enable() if the poweron
sequence fails.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/bridge/parade-ps8640.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 4b36e4dc78f1..8c5402947b3c 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -293,19 +293,19 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
return 0;
}
-static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
+static int ps8640_bridge_poweron(struct ps8640 *ps_bridge)
{
struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
int ret, status;
if (ps_bridge->powered)
- return;
+ return 0;
ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
ps_bridge->supplies);
if (ret < 0) {
DRM_ERROR("cannot enable regulators %d\n", ret);
- return;
+ return ret;
}
gpiod_set_value(ps_bridge->gpio_powerdown, 0);
@@ -352,11 +352,13 @@ static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
ps_bridge->powered = true;
- return;
+ return 0;
err_regulators_disable:
regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
ps_bridge->supplies);
+
+ return ret;
}
static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
@@ -381,7 +383,9 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
int ret;
- ps8640_bridge_poweron(ps_bridge);
+ ret = ps8640_bridge_poweron(ps_bridge);
+ if (ret)
+ return;
ret = ps8640_bridge_vdo_control(ps_bridge, ENABLE);
if (ret < 0)
--
2.33.1
In preparation for varying the poweron error handling in function
ps8640_bridge_poweron(), move function ps8640_bridge_poweroff() up
and also move the actual logic to power off the chip to a new
__ps8640_bridge_poweroff() function.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/bridge/parade-ps8640.c | 37 ++++++++++++++------------
1 file changed, 20 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 8c5402947b3c..41f5d511d516 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -293,6 +293,26 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
return 0;
}
+static void __ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
+{
+ gpiod_set_value(ps_bridge->gpio_reset, 1);
+ gpiod_set_value(ps_bridge->gpio_powerdown, 1);
+ if (regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
+ ps_bridge->supplies)) {
+ DRM_ERROR("cannot disable regulators\n");
+ }
+}
+
+static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
+{
+ if (!ps_bridge->powered)
+ return;
+
+ __ps8640_bridge_poweroff(ps_bridge);
+
+ ps_bridge->powered = false;
+}
+
static int ps8640_bridge_poweron(struct ps8640 *ps_bridge)
{
struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
@@ -361,23 +381,6 @@ static int ps8640_bridge_poweron(struct ps8640 *ps_bridge)
return ret;
}
-static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
-{
- int ret;
-
- if (!ps_bridge->powered)
- return;
-
- gpiod_set_value(ps_bridge->gpio_reset, 1);
- gpiod_set_value(ps_bridge->gpio_powerdown, 1);
- ret = regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
- ps_bridge->supplies);
- if (ret < 0)
- DRM_ERROR("cannot disable regulators %d\n", ret);
-
- ps_bridge->powered = false;
-}
-
static void ps8640_pre_enable(struct drm_bridge *bridge)
{
struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
--
2.33.1
In function ps8640_bridge_poweron(), in case of a failure not relative
to the regulators enablement, the code was disabling the regulators but
the gpio changes that happened during the poweron sequence were not
being reverted back to a clean poweroff state.
Since it is expected that, when we enter ps8640_bridge_poweron(), the
powerdown and reset GPIOs are both in active state exactly as they were
left in the poweroff function before, we can simply call function
__ps8640_bridge_poweroff() in the failure case, reverting every change
that was done during the power on sequence.
Of course it was chosen to call the poweroff function instead of adding
code to revert the GPIO changes to the poweron one to avoid duplicating
code, as we would be doing exactly what the poweroff function does.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/bridge/parade-ps8640.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 41f5d511d516..ef1b51d8b676 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -344,7 +344,7 @@ static int ps8640_bridge_poweron(struct ps8640 *ps_bridge)
if (ret < 0) {
DRM_ERROR("failed read PAGE2_GPIO_H: %d\n", ret);
- goto err_regulators_disable;
+ goto err_poweroff;
}
msleep(50);
@@ -360,23 +360,22 @@ static int ps8640_bridge_poweron(struct ps8640 *ps_bridge)
ret = regmap_update_bits(map, PAGE2_MCS_EN, MCS_EN, 0);
if (ret < 0) {
DRM_ERROR("failed write PAGE2_MCS_EN: %d\n", ret);
- goto err_regulators_disable;
+ goto err_poweroff;
}
/* Switch access edp panel's edid through i2c */
ret = regmap_write(map, PAGE2_I2C_BYPASS, I2C_BYPASS_EN);
if (ret < 0) {
DRM_ERROR("failed write PAGE2_I2C_BYPASS: %d\n", ret);
- goto err_regulators_disable;
+ goto err_poweroff;
}
ps_bridge->powered = true;
return 0;
-err_regulators_disable:
- regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
- ps_bridge->supplies);
+err_poweroff:
+ __ps8640_bridge_poweroff(ps_bridge);
return ret;
}
--
2.33.1
On 02.11.21 11:36, AngeloGioacchino Del Regno wrote:
> In preparation for varying the poweron error handling in function
> ps8640_bridge_poweron(), move function ps8640_bridge_poweroff() up
> and also move the actual logic to power off the chip to a new
> __ps8640_bridge_poweroff() function.
>
> Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
> ---
> drivers/gpu/drm/bridge/parade-ps8640.c | 37 ++++++++++++++------------
> 1 file changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 8c5402947b3c..41f5d511d516 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -293,6 +293,26 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
> return 0;
> }
>
> +static void __ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
> +{
> + gpiod_set_value(ps_bridge->gpio_reset, 1);
> + gpiod_set_value(ps_bridge->gpio_powerdown, 1);
> + if (regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
> + ps_bridge->supplies)) {
> + DRM_ERROR("cannot disable regulators\n");
> + }
That '{' is redundant
Thanks,
Danfa
> +}
> +
> +static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
> +{
> + if (!ps_bridge->powered)
> + return;
> +
> + __ps8640_bridge_poweroff(ps_bridge);
> +
> + ps_bridge->powered = false;
> +}
> +
> static int ps8640_bridge_poweron(struct ps8640 *ps_bridge)
> {
> struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
> @@ -361,23 +381,6 @@ static int ps8640_bridge_poweron(struct ps8640 *ps_bridge)
> return ret;
> }
>
> -static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
> -{
> - int ret;
> -
> - if (!ps_bridge->powered)
> - return;
> -
> - gpiod_set_value(ps_bridge->gpio_reset, 1);
> - gpiod_set_value(ps_bridge->gpio_powerdown, 1);
> - ret = regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
> - ps_bridge->supplies);
> - if (ret < 0)
> - DRM_ERROR("cannot disable regulators %d\n", ret);
> -
> - ps_bridge->powered = false;
> -}
> -
> static void ps8640_pre_enable(struct drm_bridge *bridge)
> {
> struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>
On 02.11.21 11:36, AngeloGioacchino Del Regno wrote:
> If the bridge cannot get powered on, there's no reason to try to
> communicate with it: change the ps8640_bridge_poweron function to
> return an error value to the caller, so that we can avoid calling
> ps8640_bridge_vdo_control() in ps8640_pre_enable() if the poweron
> sequence fails.
>
> Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
Reviewed-by: Dafna Hirschfeld <[email protected]>
> ---
> drivers/gpu/drm/bridge/parade-ps8640.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 4b36e4dc78f1..8c5402947b3c 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -293,19 +293,19 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
> return 0;
> }
>
> -static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
> +static int ps8640_bridge_poweron(struct ps8640 *ps_bridge)
> {
> struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
> int ret, status;
>
> if (ps_bridge->powered)
> - return;
> + return 0;
>
> ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
> ps_bridge->supplies);
> if (ret < 0) {
> DRM_ERROR("cannot enable regulators %d\n", ret);
> - return;
> + return ret;
> }
>
> gpiod_set_value(ps_bridge->gpio_powerdown, 0);
> @@ -352,11 +352,13 @@ static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
>
> ps_bridge->powered = true;
>
> - return;
> + return 0;
>
> err_regulators_disable:
> regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
> ps_bridge->supplies);
> +
> + return ret;
> }
>
> static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
> @@ -381,7 +383,9 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
> struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> int ret;
>
> - ps8640_bridge_poweron(ps_bridge);
> + ret = ps8640_bridge_poweron(ps_bridge);
> + if (ret)
> + return;
>
> ret = ps8640_bridge_vdo_control(ps_bridge, ENABLE);
> if (ret < 0)
>
Il 10/11/21 13:44, Dafna Hirschfeld ha scritto:
>
>
> On 02.11.21 11:36, AngeloGioacchino Del Regno wrote:
>> In preparation for varying the poweron error handling in function
>> ps8640_bridge_poweron(), move function ps8640_bridge_poweroff() up
>> and also move the actual logic to power off the chip to a new
>> __ps8640_bridge_poweroff() function.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
>> ---
>> drivers/gpu/drm/bridge/parade-ps8640.c | 37 ++++++++++++++------------
>> 1 file changed, 20 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c
>> b/drivers/gpu/drm/bridge/parade-ps8640.c
>> index 8c5402947b3c..41f5d511d516 100644
>> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
>> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
>> @@ -293,6 +293,26 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
>> return 0;
>> }
>> +static void __ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
>> +{
>> + gpiod_set_value(ps_bridge->gpio_reset, 1);
>> + gpiod_set_value(ps_bridge->gpio_powerdown, 1);
>> + if (regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
>> + ps_bridge->supplies)) {
>> + DRM_ERROR("cannot disable regulators\n");
>> + }
>
> That '{' is redundant
>
> Thanks,
> Danfa
>
Hi Dafna,
the braces were added as a way to increase human readability.
Cheers,
- Angelo
On 02.11.21 11:36, AngeloGioacchino Del Regno wrote:
> In function ps8640_bridge_poweron(), in case of a failure not relative
> to the regulators enablement, the code was disabling the regulators but
> the gpio changes that happened during the poweron sequence were not
> being reverted back to a clean poweroff state.
>
> Since it is expected that, when we enter ps8640_bridge_poweron(), the
> powerdown and reset GPIOs are both in active state exactly as they were
> left in the poweroff function before, we can simply call function
> __ps8640_bridge_poweroff() in the failure case, reverting every change
> that was done during the power on sequence.
>
> Of course it was chosen to call the poweroff function instead of adding
> code to revert the GPIO changes to the poweron one to avoid duplicating
> code, as we would be doing exactly what the poweroff function does.
>
> Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
Reviewed-by: Dafna Hirschfeld <[email protected]>
> ---
> drivers/gpu/drm/bridge/parade-ps8640.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 41f5d511d516..ef1b51d8b676 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -344,7 +344,7 @@ static int ps8640_bridge_poweron(struct ps8640 *ps_bridge)
>
> if (ret < 0) {
> DRM_ERROR("failed read PAGE2_GPIO_H: %d\n", ret);
> - goto err_regulators_disable;
> + goto err_poweroff;
> }
>
> msleep(50);
> @@ -360,23 +360,22 @@ static int ps8640_bridge_poweron(struct ps8640 *ps_bridge)
> ret = regmap_update_bits(map, PAGE2_MCS_EN, MCS_EN, 0);
> if (ret < 0) {
> DRM_ERROR("failed write PAGE2_MCS_EN: %d\n", ret);
> - goto err_regulators_disable;
> + goto err_poweroff;
> }
>
> /* Switch access edp panel's edid through i2c */
> ret = regmap_write(map, PAGE2_I2C_BYPASS, I2C_BYPASS_EN);
> if (ret < 0) {
> DRM_ERROR("failed write PAGE2_I2C_BYPASS: %d\n", ret);
> - goto err_regulators_disable;
> + goto err_poweroff;
> }
>
> ps_bridge->powered = true;
>
> return 0;
>
> -err_regulators_disable:
> - regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
> - ps_bridge->supplies);
> +err_poweroff:
> + __ps8640_bridge_poweroff(ps_bridge);
>
> return ret;
> }
>
Hey Angelo,
On Wed, 10 Nov 2021 at 13:46, AngeloGioacchino Del Regno
<[email protected]> wrote:
>
> Il 10/11/21 13:44, Dafna Hirschfeld ha scritto:
> >
> >
> > On 02.11.21 11:36, AngeloGioacchino Del Regno wrote:
> >> In preparation for varying the poweron error handling in function
> >> ps8640_bridge_poweron(), move function ps8640_bridge_poweroff() up
> >> and also move the actual logic to power off the chip to a new
> >> __ps8640_bridge_poweroff() function.
> >>
> >> Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
> >> ---
> >> drivers/gpu/drm/bridge/parade-ps8640.c | 37 ++++++++++++++------------
> >> 1 file changed, 20 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c
> >> b/drivers/gpu/drm/bridge/parade-ps8640.c
> >> index 8c5402947b3c..41f5d511d516 100644
> >> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> >> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> >> @@ -293,6 +293,26 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
> >> return 0;
> >> }
> >> +static void __ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
> >> +{
> >> + gpiod_set_value(ps_bridge->gpio_reset, 1);
> >> + gpiod_set_value(ps_bridge->gpio_powerdown, 1);
> >> + if (regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
> >> + ps_bridge->supplies)) {
> >> + DRM_ERROR("cannot disable regulators\n");
> >> + }
> >
> > That '{' is redundant
> >
> > Thanks,
> > Danfa
> >
>
> Hi Dafna,
> the braces were added as a way to increase human readability.
Not to bikeshed this, but the kernel style guide is clear about this.
No unneeded braces should be used where a single statement will do.
Il 19/11/21 14:19, Robert Foss ha scritto:
> Hey Angelo,
>
> On Wed, 10 Nov 2021 at 13:46, AngeloGioacchino Del Regno
> <[email protected]> wrote:
>>
>> Il 10/11/21 13:44, Dafna Hirschfeld ha scritto:
>>>
>>>
>>> On 02.11.21 11:36, AngeloGioacchino Del Regno wrote:
>>>> In preparation for varying the poweron error handling in function
>>>> ps8640_bridge_poweron(), move function ps8640_bridge_poweroff() up
>>>> and also move the actual logic to power off the chip to a new
>>>> __ps8640_bridge_poweroff() function.
>>>>
>>>> Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
>>>> ---
>>>> drivers/gpu/drm/bridge/parade-ps8640.c | 37 ++++++++++++++------------
>>>> 1 file changed, 20 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c
>>>> b/drivers/gpu/drm/bridge/parade-ps8640.c
>>>> index 8c5402947b3c..41f5d511d516 100644
>>>> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
>>>> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
>>>> @@ -293,6 +293,26 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
>>>> return 0;
>>>> }
>>>> +static void __ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
>>>> +{
>>>> + gpiod_set_value(ps_bridge->gpio_reset, 1);
>>>> + gpiod_set_value(ps_bridge->gpio_powerdown, 1);
>>>> + if (regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
>>>> + ps_bridge->supplies)) {
>>>> + DRM_ERROR("cannot disable regulators\n");
>>>> + }
>>>
>>> That '{' is redundant
>>>
>>> Thanks,
>>> Danfa
>>>
>>
>> Hi Dafna,
>> the braces were added as a way to increase human readability.
>
> Not to bikeshed this, but the kernel style guide is clear about this.
> No unneeded braces should be used where a single statement will do.
>
Hey Robert!
That's right, style rules come before personal preferences.
I'll send a v3 without the braces as soon as possible!
Cheers,
- Angelo