[PATCH 0/4] media: hi556: Add reset / clk / regulator support

[PATCH 0/4] media: hi556: Add reset / clk / regulator support

[Hans de Goede]

Hi All,

This series makes the hi556 driver work on x86_64 devices where
the power-up / down sequence is not fully handled in ACPI (as
is done on chromebooks) but where instead the driver is expected
to do it itself

This has been successfully tested on a "HP Spectre x360 13.5 (2023)":
https://github.com/intel/ipu6-drivers/issues/202

Regards,

Hans


Hans de Goede (4):
  media: hi556: Return -EPROBE_DEFER if no endpoint is found
  media: hi556: Add support for reset GPIO
  media: hi556: Add support for external clock
  media: hi556: Add support for avdd regulator

 drivers/media/i2c/hi556.c | 95 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 88 insertions(+), 7 deletions(-)

-- 
2.43.0

[PATCH 1/4] media: hi556: Return -EPROBE_DEFER if no endpoint is found

[Hans de Goede]

With ipu bridge, endpoints may only be created when ipu bridge has
initialised. This may happen after the sensor driver has first probed.

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

diff --git a/drivers/media/i2c/hi556.c b/drivers/media/i2c/hi556.c
index f6ea9b7b9700..258614b33f51 100644
--- a/drivers/media/i2c/hi556.c
+++ b/drivers/media/i2c/hi556.c
@@ -1207,8 +1207,13 @@ static int hi556_check_hwcfg(struct device *dev)
 	int ret = 0;
 	unsigned int i, j;
 
-	if (!fwnode)
-		return -ENXIO;
+	/*
+	 * Sometimes the fwnode graph is initialized by the bridge driver,
+	 * wait for this.
+	 */
+	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
+	if (!ep)
+		return -EPROBE_DEFER;
 
 	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
 	if (ret) {
@@ -1221,10 +1226,6 @@ static int hi556_check_hwcfg(struct device *dev)
 		return -EINVAL;
 	}
 
-	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
-	if (!ep)
-		return -ENXIO;
-
 	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
 	fwnode_handle_put(ep);
 	if (ret)
-- 
2.43.0

[PATCH 2/4] media: hi556: Add support for reset GPIO

[Hans de Goede]

On some ACPI platforms, such as Chromebooks the ACPI methods to
change the power-state (_PS0 and _PS3) fully take care of powering
on/off the sensor.

On other ACPI platforms, such as e.g. various HP models with IPU6 +
hi556 sensor, the sensor driver must control the reset GPIO itself.

Add support for having the driver control an optional reset GPIO.

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

diff --git a/drivers/media/i2c/hi556.c b/drivers/media/i2c/hi556.c
index 258614b33f51..7398391989ea 100644
--- a/drivers/media/i2c/hi556.c
+++ b/drivers/media/i2c/hi556.c
@@ -4,6 +4,7 @@
 #include <asm/unaligned.h>
 #include <linux/acpi.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
@@ -633,6 +634,9 @@ struct hi556 {
 	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *exposure;
 
+	/* GPIOs, clocks, etc. */
+	struct gpio_desc *reset_gpio;
+
 	/* Current mode */
 	const struct hi556_mode *cur_mode;
 
@@ -1277,6 +1281,25 @@ static void hi556_remove(struct i2c_client *client)
 	mutex_destroy(&hi556->mutex);
 }
 
+static int hi556_suspend(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct hi556 *hi556 = to_hi556(sd);
+
+	gpiod_set_value_cansleep(hi556->reset_gpio, 1);
+	return 0;
+}
+
+static int hi556_resume(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct hi556 *hi556 = to_hi556(sd);
+
+	gpiod_set_value_cansleep(hi556->reset_gpio, 0);
+	usleep_range(5000, 5500);
+	return 0;
+}
+
 static int hi556_probe(struct i2c_client *client)
 {
 	struct hi556 *hi556;
@@ -1296,12 +1319,24 @@ static int hi556_probe(struct i2c_client *client)
 
 	v4l2_i2c_subdev_init(&hi556->sd, client, &hi556_subdev_ops);
 
+	hi556->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
+						    GPIOD_OUT_HIGH);
+	if (IS_ERR(hi556->reset_gpio))
+		return dev_err_probe(&client->dev, PTR_ERR(hi556->reset_gpio),
+				     "failed to get reset GPIO\n");
+
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
+		/* Ensure non ACPI managed resources are enabled */
+		ret = hi556_resume(&client->dev);
+		if (ret)
+			return dev_err_probe(&client->dev, ret,
+					     "failed to power on sensor\n");
+
 		ret = hi556_identify_module(hi556);
 		if (ret) {
 			dev_err(&client->dev, "failed to find sensor: %d", ret);
-			return ret;
+			goto probe_error_power_off;
 		}
 	}
 
@@ -1346,9 +1381,16 @@ static int hi556_probe(struct i2c_client *client)
 	v4l2_ctrl_handler_free(hi556->sd.ctrl_handler);
 	mutex_destroy(&hi556->mutex);
 
+probe_error_power_off:
+	if (full_power)
+		hi556_suspend(&client->dev);
+
 	return ret;
 }
 
+static DEFINE_RUNTIME_DEV_PM_OPS(hi556_pm_ops, hi556_suspend, hi556_resume,
+				 NULL);
+
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id hi556_acpi_ids[] = {
 	{"INT3537"},
@@ -1362,6 +1404,7 @@ static struct i2c_driver hi556_i2c_driver = {
 	.driver = {
 		.name = "hi556",
 		.acpi_match_table = ACPI_PTR(hi556_acpi_ids),
+		.pm = pm_sleep_ptr(&hi556_pm_ops),
 	},
 	.probe = hi556_probe,
 	.remove = hi556_remove,
-- 
2.43.0

[PATCH 3/4] media: hi556: Add support for external clock

[Hans de Goede]

On some ACPI platforms, such as Chromebooks the ACPI methods to
change the power-state (_PS0 and _PS3) fully take care of powering
on/off the sensor.

On other ACPI platforms, such as e.g. various HP models with IPU6 +
hi556 sensor, the sensor driver must control the sensor's clock itself.

Add support for having the driver control an optional clock.

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

diff --git a/drivers/media/i2c/hi556.c b/drivers/media/i2c/hi556.c
index 7398391989ea..fb6ba6984e38 100644
--- a/drivers/media/i2c/hi556.c
+++ b/drivers/media/i2c/hi556.c
@@ -3,6 +3,7 @@
 
 #include <asm/unaligned.h>
 #include <linux/acpi.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
@@ -636,6 +637,7 @@ struct hi556 {
 
 	/* GPIOs, clocks, etc. */
 	struct gpio_desc *reset_gpio;
+	struct clk *clk;
 
 	/* Current mode */
 	const struct hi556_mode *cur_mode;
@@ -1287,6 +1289,7 @@ static int hi556_suspend(struct device *dev)
 	struct hi556 *hi556 = to_hi556(sd);
 
 	gpiod_set_value_cansleep(hi556->reset_gpio, 1);
+	clk_disable_unprepare(hi556->clk);
 	return 0;
 }
 
@@ -1294,6 +1297,11 @@ static int hi556_resume(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct hi556 *hi556 = to_hi556(sd);
+	int ret;
+
+	ret = clk_prepare_enable(hi556->clk);
+	if (ret)
+		return ret;
 
 	gpiod_set_value_cansleep(hi556->reset_gpio, 0);
 	usleep_range(5000, 5500);
@@ -1325,6 +1333,11 @@ static int hi556_probe(struct i2c_client *client)
 		return dev_err_probe(&client->dev, PTR_ERR(hi556->reset_gpio),
 				     "failed to get reset GPIO\n");
 
+	hi556->clk = devm_clk_get_optional(&client->dev, "clk");
+	if (IS_ERR(hi556->clk))
+		return dev_err_probe(&client->dev, PTR_ERR(hi556->clk),
+				     "failed to get clock\n");
+
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
 		/* Ensure non ACPI managed resources are enabled */
-- 
2.43.0

[PATCH 4/4] media: hi556: Add support for avdd regulator

[Hans de Goede]

On some ACPI platforms, such as Chromebooks the ACPI methods to
change the power-state (_PS0 and _PS3) fully take care of powering
on/off the sensor.

On other ACPI platforms, such as e.g. various HP models with IPU6 +
hi556 sensor, the sensor driver must control the avdd regulator itself.

Add support for having the driver control the sensor's avdd regulator.
Note this relies on the regulator-core providing a dummy regulator
(which it does by default) on platforms where Linux is not aware of
the avdd regulator.

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

diff --git a/drivers/media/i2c/hi556.c b/drivers/media/i2c/hi556.c
index fb6ba6984e38..90eff282a6e2 100644
--- a/drivers/media/i2c/hi556.c
+++ b/drivers/media/i2c/hi556.c
@@ -9,6 +9,7 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-fwnode.h>
@@ -638,6 +639,7 @@ struct hi556 {
 	/* GPIOs, clocks, etc. */
 	struct gpio_desc *reset_gpio;
 	struct clk *clk;
+	struct regulator *avdd;
 
 	/* Current mode */
 	const struct hi556_mode *cur_mode;
@@ -1287,8 +1289,17 @@ 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;
+	}
+
 	clk_disable_unprepare(hi556->clk);
 	return 0;
 }
@@ -1303,6 +1314,13 @@ static int hi556_resume(struct device *dev)
 	if (ret)
 		return ret;
 
+	ret = regulator_enable(hi556->avdd);
+	if (ret) {
+		dev_err(dev, "failed to enable avdd: %d\n", ret);
+		clk_disable_unprepare(hi556->clk);
+		return ret;
+	}
+
 	gpiod_set_value_cansleep(hi556->reset_gpio, 0);
 	usleep_range(5000, 5500);
 	return 0;
@@ -1338,6 +1356,12 @@ 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");
+
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
 		/* Ensure non ACPI managed resources are enabled */
-- 
2.43.0

Re: [PATCH 4/4] media: hi556: Add support for avdd regulator

[Jacopo Mondi]

Hi Hans

On Thu, Feb 01, 2024 at 10:58:41PM +0100, Hans de Goede wrote:
> On some ACPI platforms, such as Chromebooks the ACPI methods to
> change the power-state (_PS0 and _PS3) fully take care of powering
> on/off the sensor.
>
> On other ACPI platforms, such as e.g. various HP models with IPU6 +
> hi556 sensor, the sensor driver must control the avdd regulator itself.
>
> Add support for having the driver control the sensor's avdd regulator.
> Note this relies on the regulator-core providing a dummy regulator
> (which it does by default) on platforms where Linux is not aware of
> the avdd regulator.
>

Please excuse the question from an ACPI illiterate, but does it mean
that, in example:
1) Chromebooks: you get a dummy
2) HP: you get an actual regulator reference

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/i2c/hi556.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/drivers/media/i2c/hi556.c b/drivers/media/i2c/hi556.c
> index fb6ba6984e38..90eff282a6e2 100644
> --- a/drivers/media/i2c/hi556.c
> +++ b/drivers/media/i2c/hi556.c
> @@ -9,6 +9,7 @@
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-fwnode.h>
> @@ -638,6 +639,7 @@ struct hi556 {
>  	/* GPIOs, clocks, etc. */
>  	struct gpio_desc *reset_gpio;
>  	struct clk *clk;
> +	struct regulator *avdd;
>
>  	/* Current mode */
>  	const struct hi556_mode *cur_mode;
> @@ -1287,8 +1289,17 @@ 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);

I understand in error paths you're supposed to reverse the previously
done operations, but, as this is a reset, isn't it better to keep the
reset enabled since we're suspending anyway ?

> +		return ret;
> +	}
> +
>  	clk_disable_unprepare(hi556->clk);
>  	return 0;
>  }
> @@ -1303,6 +1314,13 @@ static int hi556_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>
> +	ret = regulator_enable(hi556->avdd);
> +	if (ret) {
> +		dev_err(dev, "failed to enable avdd: %d\n", ret);
> +		clk_disable_unprepare(hi556->clk);
> +		return ret;
> +	}
> +
>  	gpiod_set_value_cansleep(hi556->reset_gpio, 0);
>  	usleep_range(5000, 5500);
>  	return 0;
> @@ -1338,6 +1356,12 @@ 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");
> +
>  	full_power = acpi_dev_state_d0(&client->dev);
>  	if (full_power) {
>  		/* Ensure non ACPI managed resources are enabled */
> --
> 2.43.0
>
>

Re: [PATCH 4/4] media: hi556: Add support for avdd regulator

[Hans de Goede]

Hi,

On 2/3/24 11:15, Jacopo Mondi wrote:
> Hi Hans
> 
> On Thu, Feb 01, 2024 at 10:58:41PM +0100, Hans de Goede wrote:
>> On some ACPI platforms, such as Chromebooks the ACPI methods to
>> change the power-state (_PS0 and _PS3) fully take care of powering
>> on/off the sensor.
>>
>> On other ACPI platforms, such as e.g. various HP models with IPU6 +
>> hi556 sensor, the sensor driver must control the avdd regulator itself.
>>
>> Add support for having the driver control the sensor's avdd regulator.
>> Note this relies on the regulator-core providing a dummy regulator
>> (which it does by default) on platforms where Linux is not aware of
>> the avdd regulator.
>>
> 
> Please excuse the question from an ACPI illiterate, but does it mean
> that, in example:
> 1) Chromebooks: you get a dummy
> 2) HP: you get an actual regulator reference

Yes that is correct.

Regards,

Hans




> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/media/i2c/hi556.c | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/media/i2c/hi556.c b/drivers/media/i2c/hi556.c
>> index fb6ba6984e38..90eff282a6e2 100644
>> --- a/drivers/media/i2c/hi556.c
>> +++ b/drivers/media/i2c/hi556.c
>> @@ -9,6 +9,7 @@
>>  #include <linux/i2c.h>
>>  #include <linux/module.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/regulator/consumer.h>
>>  #include <media/v4l2-ctrls.h>
>>  #include <media/v4l2-device.h>
>>  #include <media/v4l2-fwnode.h>
>> @@ -638,6 +639,7 @@ struct hi556 {
>>  	/* GPIOs, clocks, etc. */
>>  	struct gpio_desc *reset_gpio;
>>  	struct clk *clk;
>> +	struct regulator *avdd;
>>
>>  	/* Current mode */
>>  	const struct hi556_mode *cur_mode;
>> @@ -1287,8 +1289,17 @@ 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);
> 
> I understand in error paths you're supposed to reverse the previously
> done operations, but, as this is a reset, isn't it better to keep the
> reset enabled since we're suspending anyway ?
> 
>> +		return ret;
>> +	}
>> +
>>  	clk_disable_unprepare(hi556->clk);
>>  	return 0;
>>  }
>> @@ -1303,6 +1314,13 @@ static int hi556_resume(struct device *dev)
>>  	if (ret)
>>  		return ret;
>>
>> +	ret = regulator_enable(hi556->avdd);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable avdd: %d\n", ret);
>> +		clk_disable_unprepare(hi556->clk);
>> +		return ret;
>> +	}
>> +
>>  	gpiod_set_value_cansleep(hi556->reset_gpio, 0);
>>  	usleep_range(5000, 5500);
>>  	return 0;
>> @@ -1338,6 +1356,12 @@ 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");
>> +
>>  	full_power = acpi_dev_state_d0(&client->dev);
>>  	if (full_power) {
>>  		/* Ensure non ACPI managed resources are enabled */
>> --
>> 2.43.0
>>
>>
>