diff mbox series

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

Message ID 20250531190534.94684-3-hansg@kernel.org
State New
Headers show
Series media: hi556: Fixes for x86 support | expand

Commit Message

Hans de Goede May 31, 2025, 7:05 p.m. UTC 
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(-)

Comments

Hans de Goede June 7, 2025, 4:05 p.m. UTC | #1 
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
diff mbox series

Patch

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) {