[PATCH 0/2] media: hi556: Fixes for x86 support

[PATCH 0/2] media: hi556: Fixes for x86 support

[Hans de Goede]

Hi All,

Here are 2 hi556 fixes which together fix hi556 camera sensors not working
on various x86 laptop models. This has been tested and confirmed to fix
the camera not working on a Dell Latitude 7450:
https://bugzilla.redhat.com/show_bug.cgi?id=2368506

It would be nice if these can be queued up as fixes for 6.16-rc#.

Regards,

Hans


Hans de Goede (2):
  media: hi556: Fix reset GPIO timings
  media: hi556: Support full range of power rails

 drivers/media/i2c/hi556.c | 47 +++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 19 deletions(-)


base-commit: 5e1ff2314797bf53636468a97719a8222deca9ae
-- 
2.49.0

[PATCH 1/2] media: hi556: Fix reset GPIO timings

[Hans de Goede]

From: Hans de Goede <hdegoede@redhat.com>

probe() requests the reset GPIO to be set to high when getting it.
Immeditately after this hi556_resume() is called and sets the GPIO low.

If the GPIO was low before requesting it this will result in the GPIO
only very briefly spiking high and the sensor not being properly reset.
The same problem also happens on back to back runtime suspend + resume.

Fix this by adding a sleep of 2 ms in hi556_resume() before setting
the GPIO low (if there is a reset GPIO).

The final sleep is kept unconditional, because if there is e.g. no reset
GPIO but a controllable clock then the sensor also needs some time after
enabling the clock.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/hi556.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/hi556.c b/drivers/media/i2c/hi556.c
index aed258211b8a..d3cc65b67855 100644
--- a/drivers/media/i2c/hi556.c
+++ b/drivers/media/i2c/hi556.c
@@ -1321,7 +1321,12 @@ static int hi556_resume(struct device *dev)
 		return ret;
 	}
 
-	gpiod_set_value_cansleep(hi556->reset_gpio, 0);
+	if (hi556->reset_gpio) {
+		/* Assert reset for at least 2ms on back to back off-on */
+		usleep_range(2000, 2200);
+		gpiod_set_value_cansleep(hi556->reset_gpio, 0);
+	}
+
 	usleep_range(5000, 5500);
 	return 0;
 }
-- 
2.49.0

[PATCH 2/2] media: hi556: Support full range of power rails

[Hans de Goede]

From: Hans de Goede <hdegoede@redhat.com>

Use regulator_bulk_* to get the array of potential power rails for
the hi556.

Previously the driver only supported avdd as only avdd is used on IPU6
designs. But other designs may also need the driver to control the other
power rails and the new INT3472 handshake support also makes use of
dvdd on IPU6 designs.

Link: https://bugzilla.redhat.com/show_bug.cgi?id=2368506
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/hi556.c | 40 +++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/hi556.c b/drivers/media/i2c/hi556.c
index d3cc65b67855..110dd00c51ae 100644
--- a/drivers/media/i2c/hi556.c
+++ b/drivers/media/i2c/hi556.c
@@ -624,6 +624,12 @@ static const struct hi556_mode supported_modes[] = {
 	},
 };
 
+static const char * const hi556_supply_names[] = {
+	"dovdd",	/* Digital I/O power */
+	"avdd",		/* Analog power */
+	"dvdd",		/* Digital core power */
+};
+
 struct hi556 {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
@@ -639,7 +645,7 @@ struct hi556 {
 	/* GPIOs, clocks, etc. */
 	struct gpio_desc *reset_gpio;
 	struct clk *clk;
-	struct regulator *avdd;
+	struct regulator_bulk_data supplies[ARRAY_SIZE(hi556_supply_names)];
 
 	/* Current mode */
 	const struct hi556_mode *cur_mode;
@@ -1289,17 +1295,10 @@ static int hi556_suspend(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct hi556 *hi556 = to_hi556(sd);
-	int ret;
 
 	gpiod_set_value_cansleep(hi556->reset_gpio, 1);
-
-	ret = regulator_disable(hi556->avdd);
-	if (ret) {
-		dev_err(dev, "failed to disable avdd: %d\n", ret);
-		gpiod_set_value_cansleep(hi556->reset_gpio, 0);
-		return ret;
-	}
-
+	regulator_bulk_disable(ARRAY_SIZE(hi556_supply_names),
+			       hi556->supplies);
 	clk_disable_unprepare(hi556->clk);
 	return 0;
 }
@@ -1314,9 +1313,10 @@ static int hi556_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	ret = regulator_enable(hi556->avdd);
+	ret = regulator_bulk_enable(ARRAY_SIZE(hi556_supply_names),
+				    hi556->supplies);
 	if (ret) {
-		dev_err(dev, "failed to enable avdd: %d\n", ret);
+		dev_err(dev, "failed to enable regulators: %d", ret);
 		clk_disable_unprepare(hi556->clk);
 		return ret;
 	}
@@ -1335,7 +1335,7 @@ static int hi556_probe(struct i2c_client *client)
 {
 	struct hi556 *hi556;
 	bool full_power;
-	int ret;
+	int i, ret;
 
 	ret = hi556_check_hwcfg(&client->dev);
 	if (ret)
@@ -1358,11 +1358,15 @@ static int hi556_probe(struct i2c_client *client)
 		return dev_err_probe(&client->dev, PTR_ERR(hi556->clk),
 				     "failed to get clock\n");
 
-	/* The regulator core will provide a "dummy" regulator if necessary */
-	hi556->avdd = devm_regulator_get(&client->dev, "avdd");
-	if (IS_ERR(hi556->avdd))
-		return dev_err_probe(&client->dev, PTR_ERR(hi556->avdd),
-				     "failed to get avdd regulator\n");
+	for (i = 0; i < ARRAY_SIZE(hi556_supply_names); i++)
+		hi556->supplies[i].supply = hi556_supply_names[i];
+
+	ret = devm_regulator_bulk_get(&client->dev,
+				      ARRAY_SIZE(hi556_supply_names),
+				      hi556->supplies);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "failed to get regulators\n");
 
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
-- 
2.49.0

Re: [PATCH 2/2] media: hi556: Support full range of power rails

[Sakari Ailus]

Hi Hans,

On Sat, May 31, 2025 at 09:05:34PM +0200, Hans de Goede wrote:
> From: Hans de Goede <hdegoede@redhat.com>
> 
> Use regulator_bulk_* to get the array of potential power rails for
> the hi556.
> 
> Previously the driver only supported avdd as only avdd is used on IPU6
> designs. But other designs may also need the driver to control the other
> power rails and the new INT3472 handshake support also makes use of
> dvdd on IPU6 designs.
> 
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2368506
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/i2c/hi556.c | 40 +++++++++++++++++++++------------------
>  1 file changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/i2c/hi556.c b/drivers/media/i2c/hi556.c
> index d3cc65b67855..110dd00c51ae 100644
> --- a/drivers/media/i2c/hi556.c
> +++ b/drivers/media/i2c/hi556.c
> @@ -624,6 +624,12 @@ static const struct hi556_mode supported_modes[] = {
>  	},
>  };
>  
> +static const char * const hi556_supply_names[] = {
> +	"dovdd",	/* Digital I/O power */
> +	"avdd",		/* Analog power */
> +	"dvdd",		/* Digital core power */
> +};

As the need to have these regulators is not related to a proper firmware
API, I think we should have at least DT bindings that document them.

Another option would be the GPIO-controlled multi-GPIO driver, I can post
one at some point but I can't promise a schedule at the moment so this is
definitely something for later.

On the other hand, virtually all sensors need at least two regulators so we
should be safe for now...

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/2] media: hi556: Support full range of power rails

[Hans de Goede]

Hi Sakari,

On 6-Jun-25 9:17 AM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Sat, May 31, 2025 at 09:05:34PM +0200, Hans de Goede wrote:
>> From: Hans de Goede <hdegoede@redhat.com>
>>
>> Use regulator_bulk_* to get the array of potential power rails for
>> the hi556.
>>
>> Previously the driver only supported avdd as only avdd is used on IPU6
>> designs. But other designs may also need the driver to control the other
>> power rails and the new INT3472 handshake support also makes use of
>> dvdd on IPU6 designs.
>>
>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2368506
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/media/i2c/hi556.c | 40 +++++++++++++++++++++------------------
>>  1 file changed, 22 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/media/i2c/hi556.c b/drivers/media/i2c/hi556.c
>> index d3cc65b67855..110dd00c51ae 100644
>> --- a/drivers/media/i2c/hi556.c
>> +++ b/drivers/media/i2c/hi556.c
>> @@ -624,6 +624,12 @@ static const struct hi556_mode supported_modes[] = {
>>  	},
>>  };
>>  
>> +static const char * const hi556_supply_names[] = {
>> +	"dovdd",	/* Digital I/O power */
>> +	"avdd",		/* Analog power */
>> +	"dvdd",		/* Digital core power */
>> +};
> 
> As the need to have these regulators is not related to a proper firmware
> API, I think we should have at least DT bindings that document them.

The hi556 driver does not support DT binding atm, only ACPI binding
and the DT maintainers have been very clear in the past that they
do NOT want any DT bindings for things which are not actually used
by DT platforms.

I've checked a HI556 datasheet I found on the net and the HI556
has the usual 3 power rails, called VDDD + VDDIO + VDDA in the datasheet,
so I believe that this patch is correct.

Regards,

Hans

Re: [PATCH 0/2] media: hi556: Fixes for x86 support

[Hans de Goede]

Hi Sakari,

On 31-May-25 21:05, Hans de Goede wrote:
> Hi All,
> 
> Here are 2 hi556 fixes which together fix hi556 camera sensors not working
> on various x86 laptop models. This has been tested and confirmed to fix
> the camera not working on a Dell Latitude 7450:
> https://bugzilla.redhat.com/show_bug.cgi?id=2368506
> 
> It would be nice if these can be queued up as fixes for 6.16-rc#.

What is the status if this series, you had one review-remark
on patch 2/2 which I've answered, so from my pov this is still
ready and it would be good to get this upstream since it fixes
the cameras on some HP laptops.

Regards,
 
Hans








> Hans de Goede (2):
>   media: hi556: Fix reset GPIO timings
>   media: hi556: Support full range of power rails
> 
>  drivers/media/i2c/hi556.c | 47 +++++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 19 deletions(-)
> 
> 
> base-commit: 5e1ff2314797bf53636468a97719a8222deca9ae

Re: [PATCH 0/2] media: hi556: Fixes for x86 support

[Sakari Ailus]

Hi Hans,

On Thu, Jun 26, 2025 at 01:42:30PM +0200, Hans de Goede wrote:
> Hi Sakari,
> 
> On 31-May-25 21:05, Hans de Goede wrote:
> > Hi All,
> > 
> > Here are 2 hi556 fixes which together fix hi556 camera sensors not working
> > on various x86 laptop models. This has been tested and confirmed to fix
> > the camera not working on a Dell Latitude 7450:
> > https://bugzilla.redhat.com/show_bug.cgi?id=2368506
> > 
> > It would be nice if these can be queued up as fixes for 6.16-rc#.
> 
> What is the status if this series, you had one review-remark
> on patch 2/2 which I've answered, so from my pov this is still
> ready and it would be good to get this upstream since it fixes
> the cameras on some HP laptops.

Thanks for the ping.

I've picked these, to be included in the next PR for 6.17.

-- 
Regards,

Sakari Ailus