[PATCH 0/7] Some atomisp fixes and improvements

[PATCH 0/7] Some atomisp fixes and improvements

[Mauro Carvalho Chehab]

Those patches are meant to improve device detection by the atomisp driver,
relying on ACPI bios when possible.

It also adds a basis for using ACPI PM, but only if the DSDT tables have
a description about how to turn on the resources needed by the cameras.

At least on the device I'm using for tests, this is not the case.

Mauro Carvalho Chehab (7):
  media: atomisp: reorganize the code under gmin_subdev_add()
  media: atomisp: Prepare sensor support for ACPI PM
  media: atomisp: properly parse CLK PMIC on newer devices
  media: atomisp: fix call to g_frame_interval
  media: atomisp: print info if gpio0 and gpio2 were detected
  media: atomisp: split add from find subdev
  media: atomisp: place all gpio parsing together

 .../staging/media/atomisp/pci/atomisp_cmd.c   |   2 +-
 .../media/atomisp/pci/atomisp_gmin_platform.c | 393 ++++++++++++------
 2 files changed, 267 insertions(+), 128 deletions(-)

-- 
2.26.2

[PATCH 1/7] media: atomisp: reorganize the code under gmin_subdev_add()

[Mauro Carvalho Chehab]

The gmin_subdev_add() currently doesn't use ACPI device
power management. In order to prepare for adding support
for it, let's shift some things, placing the PM-related
stuff at the end of the probing logic.

Let's also store the current gs on a temporary var, in
order to simplify the source code.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../media/atomisp/pci/atomisp_gmin_platform.c | 146 +++++++++---------
 1 file changed, 73 insertions(+), 73 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index 4aebc7cd22d9..74331c4467bd 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -447,6 +447,7 @@ static struct gmin_subdev *gmin_subdev_add(struct v4l2_subdev *subdev)
 {
 	struct i2c_client *power = NULL, *client = v4l2_get_subdevdata(subdev);
 	struct acpi_device *adev;
+	struct gmin_subdev *gs;
 	acpi_handle handle;
 	struct device *dev;
 	int i, ret, clock_num;
@@ -457,16 +458,52 @@ static struct gmin_subdev *gmin_subdev_add(struct v4l2_subdev *subdev)
 	dev = &client->dev;
 
 	handle = ACPI_HANDLE(dev);
-
-	// FIXME: may need to release resources allocated by acpi_bus_get_device()
-	if (!handle || acpi_bus_get_device(handle, &adev)) {
-		dev_err(dev, "Error could not get ACPI device\n");
-		return NULL;
-	}
+	adev = ACPI_COMPANION(&client->dev);
 
 	dev_info(&client->dev, "%s: ACPI detected it on bus ID=%s, HID=%s\n",
 		__func__, acpi_device_bid(adev), acpi_device_hid(adev));
 
+	for (i = 0; i < MAX_SUBDEVS && gmin_subdevs[i].subdev; i++)
+		;
+	if (i >= MAX_SUBDEVS)
+		return NULL;
+
+	gs = &gmin_subdevs[i];
+	gs->subdev = subdev;
+
+	/*
+	 * FIXME:
+	 * 	WA:CHT requires XTAL clock as PLL is not stable.
+	 *
+	 * However, such data doesn't seem to be present at the _DSM
+	 * table under the GUID dc2f6c4f-045b-4f1d-97b9-882a6860a4be.
+	 * So, let's change the default according with the ISP version,
+	 * but allowing it to be overridden by BIOS or by DMI match tables.
+	 */
+	if (IS_ISP2401)
+		gs->clock_src = VLV2_CLK_XTAL_25_0MHz;
+	else
+		gs->clock_src = VLV2_CLK_PLL_19P2MHZ;
+
+	gs->clock_src = gmin_get_var_int(dev, false, "ClkSrc",
+					 gs->clock_src);
+
+	gs->csi_port = gmin_get_var_int(dev, false, "CsiPort", 0);
+	gs->csi_lanes = gmin_get_var_int(dev, false, "CsiLanes", 1);
+
+	gs->gpio0 = gpiod_get_index(dev, NULL, 0, GPIOD_OUT_LOW);
+	if (IS_ERR(gs->gpio0))
+		gs->gpio0 = NULL;
+
+	gs->gpio1 = gpiod_get_index(dev, NULL, 1, GPIOD_OUT_LOW);
+	if (IS_ERR(gs->gpio1))
+		gs->gpio1 = NULL;
+
+	/*
+	 * FIXME: the code below doesn't rely on ACPI device_pm.c code to
+	 * set clocks and do power management.
+	 */
+
 	if (!pmic_id) {
 		if (gmin_i2c_dev_exists(dev, PMIC_ACPI_TI, &power))
 			pmic_id = PMIC_TI;
@@ -478,13 +515,8 @@ static struct gmin_subdev *gmin_subdev_add(struct v4l2_subdev *subdev)
 			pmic_id = PMIC_REGULATOR;
 	}
 
-	for (i = 0; i < MAX_SUBDEVS && gmin_subdevs[i].subdev; i++)
-		;
-	if (i >= MAX_SUBDEVS)
-		return NULL;
-
 	if (power) {
-		gmin_subdevs[i].pwm_i2c_addr = power->addr;
+		gs->pwm_i2c_addr = power->addr;
 		dev_info(dev,
 			 "gmin: power management provided via %s (i2c addr 0x%02x)\n",
 			 pmic_name[pmic_id], power->addr);
@@ -493,31 +525,7 @@ static struct gmin_subdev *gmin_subdev_add(struct v4l2_subdev *subdev)
 			 pmic_name[pmic_id]);
 	}
 
-	gmin_subdevs[i].subdev = subdev;
-
 	/*
-	 * FIXME:
-	 * 	WA:CHT requires XTAL clock as PLL is not stable.
-	 *
-	 * However, such data doesn't seem to be present at the _DSM
-	 * table under the GUID dc2f6c4f-045b-4f1d-97b9-882a6860a4be.
-	 * So, let's change the default according with the ISP version,
-	 * but allowing it to be overridden by BIOS or by DMI match tables.
-	 */
-	if (IS_ISP2401)
-		gmin_subdevs[i].clock_src = VLV2_CLK_XTAL_25_0MHz;
-	else
-		gmin_subdevs[i].clock_src = VLV2_CLK_PLL_19P2MHZ;
-
-	gmin_subdevs[i].clock_src = gmin_get_var_int(dev, false, "ClkSrc",
-						     gmin_subdevs[i].clock_src);
-
-	gmin_subdevs[i].csi_port = gmin_get_var_int(dev, false, "CsiPort", 0);
-	gmin_subdevs[i].csi_lanes = gmin_get_var_int(dev, false, "CsiLanes", 1);
-
-	/*
-	 * FIXME:
-	 *
 	 * According with :
 	 *   https://github.com/projectceladon/hardware-intel-kernelflinger/blob/master/doc/fastboot.md
 	 *
@@ -540,9 +548,9 @@ static struct gmin_subdev *gmin_subdev_add(struct v4l2_subdev *subdev)
 	snprintf(gmin_pmc_clk_name, sizeof(gmin_pmc_clk_name),
 		 "%s_%d", "pmc_plt_clk", clock_num);
 
-	gmin_subdevs[i].pmc_clk = devm_clk_get(dev, gmin_pmc_clk_name);
-	if (IS_ERR(gmin_subdevs[i].pmc_clk)) {
-		ret = PTR_ERR(gmin_subdevs[i].pmc_clk);
+	gs->pmc_clk = devm_clk_get(dev, gmin_pmc_clk_name);
+	if (IS_ERR(gs->pmc_clk)) {
+		ret = PTR_ERR(gs->pmc_clk);
 
 		dev_err(dev,
 			"Failed to get clk from %s : %d\n",
@@ -563,25 +571,17 @@ static struct gmin_subdev *gmin_subdev_add(struct v4l2_subdev *subdev)
 	 * to disable a clock that has not been enabled,
 	 * we need to enable the clock first.
 	 */
-	ret = clk_prepare_enable(gmin_subdevs[i].pmc_clk);
+	ret = clk_prepare_enable(gs->pmc_clk);
 	if (!ret)
-		clk_disable_unprepare(gmin_subdevs[i].pmc_clk);
-
-	gmin_subdevs[i].gpio0 = gpiod_get_index(dev, NULL, 0, GPIOD_OUT_LOW);
-	if (IS_ERR(gmin_subdevs[i].gpio0))
-		gmin_subdevs[i].gpio0 = NULL;
-
-	gmin_subdevs[i].gpio1 = gpiod_get_index(dev, NULL, 1, GPIOD_OUT_LOW);
-	if (IS_ERR(gmin_subdevs[i].gpio1))
-		gmin_subdevs[i].gpio1 = NULL;
+		clk_disable_unprepare(gs->pmc_clk);
 
 	switch (pmic_id) {
 	case PMIC_REGULATOR:
-		gmin_subdevs[i].v1p8_reg = regulator_get(dev, "V1P8SX");
-		gmin_subdevs[i].v2p8_reg = regulator_get(dev, "V2P8SX");
+		gs->v1p8_reg = regulator_get(dev, "V1P8SX");
+		gs->v2p8_reg = regulator_get(dev, "V2P8SX");
 
-		gmin_subdevs[i].v1p2_reg = regulator_get(dev, "V1P2A");
-		gmin_subdevs[i].v2p8_vcm_reg = regulator_get(dev, "VPROG4B");
+		gs->v1p2_reg = regulator_get(dev, "V1P2A");
+		gs->v2p8_vcm_reg = regulator_get(dev, "VPROG4B");
 
 		/* Note: ideally we would initialize v[12]p8_on to the
 		 * output of regulator_is_enabled(), but sadly that
@@ -593,32 +593,32 @@ static struct gmin_subdev *gmin_subdev_add(struct v4l2_subdev *subdev)
 		break;
 
 	case PMIC_AXP:
-		gmin_subdevs[i].eldo1_1p8v = gmin_get_var_int(dev, false,
-							      "eldo1_1p8v",
-							      ELDO1_1P8V);
-		gmin_subdevs[i].eldo1_sel_reg = gmin_get_var_int(dev, false,
-								 "eldo1_sel_reg",
-								 ELDO1_SEL_REG);
-		gmin_subdevs[i].eldo1_ctrl_shift = gmin_get_var_int(dev, false,
-								    "eldo1_ctrl_shift",
-								    ELDO1_CTRL_SHIFT);
-		gmin_subdevs[i].eldo2_1p8v = gmin_get_var_int(dev, false,
-							      "eldo2_1p8v",
-							      ELDO2_1P8V);
-		gmin_subdevs[i].eldo2_sel_reg = gmin_get_var_int(dev, false,
-								 "eldo2_sel_reg",
-								 ELDO2_SEL_REG);
-		gmin_subdevs[i].eldo2_ctrl_shift = gmin_get_var_int(dev, false,
-								    "eldo2_ctrl_shift",
-								    ELDO2_CTRL_SHIFT);
-		gmin_subdevs[i].pwm_i2c_addr = power->addr;
+		gs->eldo1_1p8v = gmin_get_var_int(dev, false,
+						  "eldo1_1p8v",
+						  ELDO1_1P8V);
+		gs->eldo1_sel_reg = gmin_get_var_int(dev, false,
+						     "eldo1_sel_reg",
+						     ELDO1_SEL_REG);
+		gs->eldo1_ctrl_shift = gmin_get_var_int(dev, false,
+							"eldo1_ctrl_shift",
+							ELDO1_CTRL_SHIFT);
+		gs->eldo2_1p8v = gmin_get_var_int(dev, false,
+						  "eldo2_1p8v",
+						  ELDO2_1P8V);
+		gs->eldo2_sel_reg = gmin_get_var_int(dev, false,
+						     "eldo2_sel_reg",
+						     ELDO2_SEL_REG);
+		gs->eldo2_ctrl_shift = gmin_get_var_int(dev, false,
+							"eldo2_ctrl_shift",
+							ELDO2_CTRL_SHIFT);
+		gs->pwm_i2c_addr = power->addr;
 		break;
 
 	default:
 		break;
 	}
 
-	return &gmin_subdevs[i];
+	return gs;
 }
 
 static struct gmin_subdev *find_gmin_subdev(struct v4l2_subdev *subdev)
-- 
2.26.2

[PATCH 2/7] media: atomisp: Prepare sensor support for ACPI PM

[Mauro Carvalho Chehab]

Add support code for this driver to use ACPI power management.

Yet, at least with known devices, this won't work without
further changes.

The rationale is that the ACPI handling code checks for the _PR? tables
in order to know what is required to switch the device from power state
D0 (_PR0) up to D3COLD (_PR3).

The adev->flags.power_manageable is set to true if the device has a _PR0
table, which can be checked by calling acpi_device_power_manageable(adev).

However, this only says that the device can be set to power off mode.

At least on the DSDT tables we've seen so far, there's no _PR3 nor _PS3
(which would have a somewhat similar effect).

So, using ACPI for power management won't work, except if adding
an ACPI override logic somewhere.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../media/atomisp/pci/atomisp_gmin_platform.c | 89 +++++++++++++++++--
 1 file changed, 83 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index 74331c4467bd..2903aa52115b 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -65,7 +65,6 @@ enum clock_rate {
 struct gmin_subdev {
 	struct v4l2_subdev *subdev;
 	enum clock_rate clock_src;
-	bool clock_on;
 	struct clk *pmc_clk;
 	struct gpio_desc *gpio0;
 	struct gpio_desc *gpio1;
@@ -77,6 +76,8 @@ struct gmin_subdev {
 	unsigned int csi_lanes;
 	enum atomisp_input_format csi_fmt;
 	enum atomisp_bayer_order csi_bayer;
+
+	bool clock_on;
 	bool v1p8_on;
 	bool v2p8_on;
 	bool v1p2_on;
@@ -107,7 +108,7 @@ static enum {
 } pmic_id;
 
 static const char *pmic_name[] = {
-	[PMIC_UNSET]		= "unset",
+	[PMIC_UNSET]		= "ACPI device PM",
 	[PMIC_REGULATOR]	= "regulator driver",
 	[PMIC_AXP]		= "XPower AXP288 PMIC",
 	[PMIC_TI]		= "Dollar Cove TI PMIC",
@@ -500,8 +501,39 @@ static struct gmin_subdev *gmin_subdev_add(struct v4l2_subdev *subdev)
 		gs->gpio1 = NULL;
 
 	/*
-	 * FIXME: the code below doesn't rely on ACPI device_pm.c code to
-	 * set clocks and do power management.
+	 * FIXME:
+	 *
+	 * The ACPI handling code checks for the _PR? tables in order to
+	 * know what is required to switch the device from power state
+	 * D0 (_PR0) up to D3COLD (_PR3).
+	 *
+	 * The adev->flags.power_manageable is set to true if the device
+	 * has a _PR0 table, which can be checked by calling
+	 * acpi_device_power_manageable(adev).
+	 *
+	 * However, this only says that the device can be set to power off
+	 * mode.
+	 *
+	 * At least on the DSDT tables we've seen so far, there's no _PR3,
+	 * nor _PS3 (which would have a somewhat similar effect).
+	 * So, using ACPI for power management won't work, except if adding
+	 * an ACPI override logic somewhere.
+	 *
+	 * So, at least for the existing devices we know, the check below
+	 * will always be false.
+	 */
+	if (acpi_device_can_wakeup(adev) &&
+	    acpi_device_can_poweroff(adev)) {
+		dev_info(dev,
+			 "gmin: power management provided via device PM\n");
+
+		return gs;
+	}
+
+	/*
+	 * The code below is here due to backward compatibility with devices
+	 * whose ACPI BIOS may not contain everything that would be needed
+	 * in order to set clocks and do power management.
 	 */
 
 	if (!pmic_id) {
@@ -856,6 +888,37 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
 	return -EINVAL;
 }
 
+static int gmin_acpi_pm_ctrl(struct v4l2_subdev *subdev, int on)
+{
+	int ret = 0;
+	struct gmin_subdev *gs = find_gmin_subdev(subdev);
+	struct i2c_client *client = v4l2_get_subdevdata(subdev);
+	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
+
+	/* Use the ACPI power management to control it */
+	on = !!on;
+	if (gs->clock_on == on)
+		return 0;
+
+	dev_dbg(subdev->dev, "Setting power state to %s\n",
+		on ? "on" : "off");
+
+	if (on)
+		ret = acpi_device_set_power(adev,
+					    ACPI_STATE_D0);
+	else
+		ret = acpi_device_set_power(adev,
+					    ACPI_STATE_D3_COLD);
+
+	if (!ret)
+		gs->clock_on = on;
+	else
+		dev_err(subdev->dev, "Couldn't set power state to %s\n",
+		        on ? "on" : "off");
+
+	return ret;
+}
+
 static int gmin_flisclk_ctrl(struct v4l2_subdev *subdev, int on)
 {
 	int ret = 0;
@@ -921,7 +984,7 @@ static struct camera_vcm_control *gmin_get_vcm_ctrl(struct v4l2_subdev *subdev,
 	return NULL;
 }
 
-static struct camera_sensor_platform_data gmin_plat = {
+static struct camera_sensor_platform_data pmic_gmin_plat = {
 	.gpio0_ctrl = gmin_gpio0_ctrl,
 	.gpio1_ctrl = gmin_gpio1_ctrl,
 	.v1p8_ctrl = gmin_v1p8_ctrl,
@@ -932,6 +995,17 @@ static struct camera_sensor_platform_data gmin_plat = {
 	.get_vcm_ctrl = gmin_get_vcm_ctrl,
 };
 
+static struct camera_sensor_platform_data acpi_gmin_plat = {
+	.gpio0_ctrl = gmin_gpio0_ctrl,
+	.gpio1_ctrl = gmin_gpio1_ctrl,
+	.v1p8_ctrl = gmin_acpi_pm_ctrl,
+	.v2p8_ctrl = gmin_acpi_pm_ctrl,
+	.v1p2_ctrl = gmin_acpi_pm_ctrl,
+	.flisclk_ctrl = gmin_acpi_pm_ctrl,
+	.csi_cfg = gmin_csi_cfg,
+	.get_vcm_ctrl = gmin_get_vcm_ctrl,
+};
+
 struct camera_sensor_platform_data *gmin_camera_platform_data(
     struct v4l2_subdev *subdev,
     enum atomisp_input_format csi_format,
@@ -942,7 +1016,10 @@ struct camera_sensor_platform_data *gmin_camera_platform_data(
 	gs->csi_fmt = csi_format;
 	gs->csi_bayer = csi_bayer;
 
-	return &gmin_plat;
+	if (gs->pmc_clk)
+		return &pmic_gmin_plat;
+	else
+		return &acpi_gmin_plat;
 }
 EXPORT_SYMBOL_GPL(gmin_camera_platform_data);
 
-- 
2.26.2

[PATCH 3/7] media: atomisp: properly parse CLK PMIC on newer devices

[Mauro Carvalho Chehab]

Newer devices don't place the PMIC CLK line inside an EFI
var. Instead, those are found at the _PR0 table.

Add a parser for it, using info stored via the DSDT table.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../media/atomisp/pci/atomisp_gmin_platform.c | 66 ++++++++++++++++++-
 1 file changed, 64 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index 2903aa52115b..e476cf1f3294 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -444,6 +444,61 @@ static int gmin_i2c_write(struct device *dev, u16 i2c_addr, u8 reg,
 	return ret;
 }
 
+static int atomisp_get_acpi_power(struct device *dev, acpi_handle handle)
+{
+	char name[5];
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct acpi_buffer b_name = { sizeof(name), name };
+	union acpi_object *package, *element;
+	acpi_handle rhandle;
+	acpi_status status;
+	int clock_num = -1;
+	int i;
+
+	status = acpi_evaluate_object(handle, "_PR0", NULL, &buffer);
+	if (!ACPI_SUCCESS(status))
+		return -1;
+
+	package = buffer.pointer;
+
+	if (!buffer.length || !package
+	    || package->type != ACPI_TYPE_PACKAGE
+	    || !package->package.count)
+		goto fail;
+
+	for (i = 0; i < package->package.count; i++) {
+		element = &package->package.elements[i];
+
+		if (element->type != ACPI_TYPE_LOCAL_REFERENCE)
+			continue;
+
+		rhandle = element->reference.handle;
+		if (!rhandle)
+			goto fail;
+
+		acpi_get_name(rhandle, ACPI_SINGLE_NAME, &b_name);
+
+		dev_dbg(dev, "Found PM resource '%s'\n", name);
+		if (strlen(name) == 4 && !strncmp(name, "CLK", 3)) {
+		        if (name[3] >= '0' && name[3] <= '4')
+				clock_num = name[3] - '0';
+#if 0
+			/*
+			 * We could abort here, but let's parse all resources,
+			 * as this is helpful for debugging purposes
+			 */
+			if (clock_num >= 0)
+			    break;
+#endif
+		}
+	}
+
+fail:
+	ACPI_FREE(buffer.pointer);
+
+	return clock_num;
+}
+
 static struct gmin_subdev *gmin_subdev_add(struct v4l2_subdev *subdev)
 {
 	struct i2c_client *power = NULL, *client = v4l2_get_subdevdata(subdev);
@@ -451,7 +506,7 @@ static struct gmin_subdev *gmin_subdev_add(struct v4l2_subdev *subdev)
 	struct gmin_subdev *gs;
 	acpi_handle handle;
 	struct device *dev;
-	int i, ret, clock_num;
+	int i, ret, clock_num = -1;
 
 	if (!client)
 		return NULL;
@@ -570,7 +625,14 @@ static struct gmin_subdev *gmin_subdev_add(struct v4l2_subdev *subdev)
 	 * is a power resource already, falling back to the EFI vars detection
 	 * otherwise.
 	 */
-	clock_num = gmin_get_var_int(dev, false, "CamClk", -1);
+
+	/* Try first to use ACPI to get the clock resource */
+	if (acpi_device_power_manageable(adev))
+		clock_num = atomisp_get_acpi_power(dev, handle);
+
+	/* Fall-back use EFI and/or DMI match */
+	if (clock_num < 0)
+		clock_num = gmin_get_var_int(dev, false, "CamClk", 0);
 
 	if (clock_num < 0 || clock_num > MAX_CLK_COUNT) {
 		dev_err(dev, "Invalid clock number\n");
-- 
2.26.2

[PATCH 4/7] media: atomisp: fix call to g_frame_interval

[Mauro Carvalho Chehab]

The media core has now a check if fi->pad is bigger than zero
or bigger than sd->entity.num_pads, if the media controller
is defined.

This causes a call to g_frame_interval to return -EINVAL.

Fix it by first cleaning up the struct.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/staging/media/atomisp/pci/atomisp_cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 7b936e5a5f03..388b57455204 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -109,7 +109,7 @@ struct atomisp_acc_pipe *atomisp_to_acc_pipe(struct video_device *dev)
 
 static unsigned short atomisp_get_sensor_fps(struct atomisp_sub_device *asd)
 {
-	struct v4l2_subdev_frame_interval fi;
+	struct v4l2_subdev_frame_interval fi = { 0 };
 	struct atomisp_device *isp = asd->isp;
 
 	unsigned short fps = 0;
-- 
2.26.2

[PATCH 5/7] media: atomisp: print info if gpio0 and gpio2 were detected

[Mauro Carvalho Chehab]

If the ACPI DSDT tables provide _CRS for the camera, the
GPIO core code should be able to handle them automatically.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index e476cf1f3294..31ec21a3ba84 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -550,10 +550,14 @@ static struct gmin_subdev *gmin_subdev_add(struct v4l2_subdev *subdev)
 	gs->gpio0 = gpiod_get_index(dev, NULL, 0, GPIOD_OUT_LOW);
 	if (IS_ERR(gs->gpio0))
 		gs->gpio0 = NULL;
+	else
+		dev_info(dev, "will handle gpio0 via ACPI\n");
 
 	gs->gpio1 = gpiod_get_index(dev, NULL, 1, GPIOD_OUT_LOW);
 	if (IS_ERR(gs->gpio1))
 		gs->gpio1 = NULL;
+	else
+		dev_info(dev, "will handle gpio1 via ACPI\n");
 
 	/*
 	 * FIXME:
-- 
2.26.2

[PATCH 6/7] media: atomisp: split add from find subdev

[Mauro Carvalho Chehab]

There's only one place where a subdev can be added: when the
sensor driver registers it. Trying to do it elsewhere will
cause problems, as the detection code needs to access the
I2C bus in order to probe some things.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index 31ec21a3ba84..3136759bf96f 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -202,6 +202,8 @@ int atomisp_register_i2c_module(struct v4l2_subdev *subdev,
 	 * gmin_subdev struct is already initialized for us.
 	 */
 	gs = find_gmin_subdev(subdev);
+	if (!gs)
+		return -ENODEV;
 
 	pdata.subdevs[i].type = type;
 	pdata.subdevs[i].port = gs->csi_port;
@@ -726,7 +728,7 @@ static struct gmin_subdev *find_gmin_subdev(struct v4l2_subdev *subdev)
 	for (i = 0; i < MAX_SUBDEVS; i++)
 		if (gmin_subdevs[i].subdev == subdev)
 			return &gmin_subdevs[i];
-	return gmin_subdev_add(subdev);
+	return NULL;
 }
 
 static int axp_regulator_set(struct device *dev, struct gmin_subdev *gs,
@@ -1077,7 +1079,7 @@ struct camera_sensor_platform_data *gmin_camera_platform_data(
     enum atomisp_input_format csi_format,
     enum atomisp_bayer_order csi_bayer)
 {
-	struct gmin_subdev *gs = find_gmin_subdev(subdev);
+	struct gmin_subdev *gs = gmin_subdev_add(subdev);
 
 	gs->csi_fmt = csi_format;
 	gs->csi_bayer = csi_bayer;
-- 
2.26.2

[PATCH 7/7] media: atomisp: place all gpio parsing together

[Mauro Carvalho Chehab]

Instead of parsing GPIO for two exceptions inside the logic
which would be trying to use the GPIO, move the init code
to the routine which adds a new gmin device.

Move the notes to it too, as this helps to identify where
those two GPIO settings are used, explaining why they're
defaulting to -1 when not found.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../media/atomisp/pci/atomisp_gmin_platform.c | 86 +++++++++----------
 1 file changed, 40 insertions(+), 46 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index 3136759bf96f..1a1a6a9ee8a3 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -83,6 +83,9 @@ struct gmin_subdev {
 	bool v1p2_on;
 	bool v2p8_vcm_on;
 
+	int v1p8_gpio;
+	int v2p8_gpio;
+
 	u8 pwm_i2c_addr;
 
 	/* For PMIC AXP */
@@ -122,24 +125,6 @@ static const struct atomisp_platform_data pdata = {
 	.subdevs = pdata_subdevs,
 };
 
-/*
- * Something of a hack.  The ECS E7 board drives camera 2.8v from an
- * external regulator instead of the PMIC.  There's a gmin_CamV2P8
- * config variable that specifies the GPIO to handle this particular
- * case, but this needs a broader architecture for handling camera
- * power.
- */
-enum { V2P8_GPIO_UNSET = -2, V2P8_GPIO_NONE = -1 };
-static int v2p8_gpio = V2P8_GPIO_UNSET;
-
-/*
- * Something of a hack. The CHT RVP board drives camera 1.8v from an
- * external regulator instead of the PMIC just like ECS E7 board, see the
- * comments above.
- */
-enum { V1P8_GPIO_UNSET = -2, V1P8_GPIO_NONE = -1 };
-static int v1p8_gpio = V1P8_GPIO_UNSET;
-
 static LIST_HEAD(vcm_devices);
 static DEFINE_MUTEX(vcm_lock);
 
@@ -561,6 +546,23 @@ static struct gmin_subdev *gmin_subdev_add(struct v4l2_subdev *subdev)
 	else
 		dev_info(dev, "will handle gpio1 via ACPI\n");
 
+	/*
+	 * Those are used only when there is an external regulator apart
+	 * from the PMIC that would be providing power supply, like on the
+	 * two cases below:
+	 *
+	 * The ECS E7 board drives camera 2.8v from an external regulator
+	 * instead of the PMIC.  There's a gmin_CamV2P8 config variable
+	 * that specifies the GPIO to handle this particular case,
+	 * but this needs a broader architecture for handling camera power.
+	 *
+	 * The CHT RVP board drives camera 1.8v from an* external regulator
+	 * instead of the PMIC just like ECS E7 board.
+	 */
+
+	gs->v1p8_gpio = gmin_get_var_int(dev, true, "V1P8GPIO", -1);
+	gs->v2p8_gpio = gmin_get_var_int(dev, true, "V2P8GPIO", -1);
+
 	/*
 	 * FIXME:
 	 *
@@ -843,26 +845,22 @@ static int gmin_v1p8_ctrl(struct v4l2_subdev *subdev, int on)
 
 	dev = &client->dev;
 
-	if (v1p8_gpio == V1P8_GPIO_UNSET) {
-		v1p8_gpio = gmin_get_var_int(dev, true,
-					     "V1P8GPIO", V1P8_GPIO_NONE);
-		if (v1p8_gpio != V1P8_GPIO_NONE) {
-			pr_info("atomisp_gmin_platform: 1.8v power on GPIO %d\n",
-				v1p8_gpio);
-			ret = gpio_request(v1p8_gpio, "camera_v1p8_en");
-			if (!ret)
-				ret = gpio_direction_output(v1p8_gpio, 0);
-			if (ret)
-				pr_err("V1P8 GPIO initialization failed\n");
-		}
+	if (gs->v1p8_gpio >= 0) {
+		pr_info("atomisp_gmin_platform: 1.8v power on GPIO %d\n",
+			gs->v1p8_gpio);
+		ret = gpio_request(gs->v1p8_gpio, "camera_v1p8_en");
+		if (!ret)
+			ret = gpio_direction_output(gs->v1p8_gpio, 0);
+		if (ret)
+			pr_err("V1P8 GPIO initialization failed\n");
 	}
 
 	if (!gs || gs->v1p8_on == on)
 		return 0;
 	gs->v1p8_on = on;
 
-	if (v1p8_gpio >= 0)
-		gpio_set_value(v1p8_gpio, on);
+	if (gs->v1p8_gpio >= 0)
+		gpio_set_value(gs->v1p8_gpio, on);
 
 	if (gs->v1p8_reg) {
 		regulator_set_voltage(gs->v1p8_reg, 1800000, 1800000);
@@ -905,26 +903,22 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
 
 	dev = &client->dev;
 
-	if (v2p8_gpio == V2P8_GPIO_UNSET) {
-		v2p8_gpio = gmin_get_var_int(dev, true,
-					     "V2P8GPIO", V2P8_GPIO_NONE);
-		if (v2p8_gpio != V2P8_GPIO_NONE) {
-			pr_info("atomisp_gmin_platform: 2.8v power on GPIO %d\n",
-				v2p8_gpio);
-			ret = gpio_request(v2p8_gpio, "camera_v2p8");
-			if (!ret)
-				ret = gpio_direction_output(v2p8_gpio, 0);
-			if (ret)
-				pr_err("V2P8 GPIO initialization failed\n");
-		}
+	if (gs->v2p8_gpio >= 0) {
+		pr_info("atomisp_gmin_platform: 2.8v power on GPIO %d\n",
+			gs->v2p8_gpio);
+		ret = gpio_request(gs->v2p8_gpio, "camera_v2p8");
+		if (!ret)
+			ret = gpio_direction_output(gs->v2p8_gpio, 0);
+		if (ret)
+			pr_err("V2P8 GPIO initialization failed\n");
 	}
 
 	if (!gs || gs->v2p8_on == on)
 		return 0;
 	gs->v2p8_on = on;
 
-	if (v2p8_gpio >= 0)
-		gpio_set_value(v2p8_gpio, on);
+	if (gs->v2p8_gpio >= 0)
+		gpio_set_value(gs->v2p8_gpio, on);
 
 	if (gs->v2p8_reg) {
 		regulator_set_voltage(gs->v2p8_reg, 2900000, 2900000);
-- 
2.26.2

Re: [PATCH 0/7] Some atomisp fixes and improvements

[Andy Shevchenko]

On Fri, Jun 26, 2020 at 04:04:52PM +0200, Mauro Carvalho Chehab wrote:
> Those patches are meant to improve device detection by the atomisp driver,
> relying on ACPI bios when possible.
> 
> It also adds a basis for using ACPI PM, but only if the DSDT tables have
> a description about how to turn on the resources needed by the cameras.
> 
> At least on the device I'm using for tests, this is not the case.

Is this in your experimental tree? I'll rebase mine on top and test.
After I will send the rest from my series and give a tag to this.

> 
> Mauro Carvalho Chehab (7):
>   media: atomisp: reorganize the code under gmin_subdev_add()
>   media: atomisp: Prepare sensor support for ACPI PM
>   media: atomisp: properly parse CLK PMIC on newer devices
>   media: atomisp: fix call to g_frame_interval
>   media: atomisp: print info if gpio0 and gpio2 were detected
>   media: atomisp: split add from find subdev
>   media: atomisp: place all gpio parsing together
> 
>  .../staging/media/atomisp/pci/atomisp_cmd.c   |   2 +-
>  .../media/atomisp/pci/atomisp_gmin_platform.c | 393 ++++++++++++------
>  2 files changed, 267 insertions(+), 128 deletions(-)
> 
> -- 
> 2.26.2
> 
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/7] Some atomisp fixes and improvements

[Mauro Carvalho Chehab]

Em Fri, 26 Jun 2020 18:00:21 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> escreveu:

> On Fri, Jun 26, 2020 at 04:04:52PM +0200, Mauro Carvalho Chehab wrote:
> > Those patches are meant to improve device detection by the atomisp driver,
> > relying on ACPI bios when possible.
> > 
> > It also adds a basis for using ACPI PM, but only if the DSDT tables have
> > a description about how to turn on the resources needed by the cameras.
> > 
> > At least on the device I'm using for tests, this is not the case.  
> 
> Is this in your experimental tree? 

Yes. 

> I'll rebase mine on top and test.
> After I will send the rest from my series and give a tag to this.

It would be helpful if you could test removing the DMI match table from
your board. If your device has a DSDT table close to the one I have, the
new code may be able to get everything from DSDT.

> 
> > 
> > Mauro Carvalho Chehab (7):
> >   media: atomisp: reorganize the code under gmin_subdev_add()
> >   media: atomisp: Prepare sensor support for ACPI PM
> >   media: atomisp: properly parse CLK PMIC on newer devices
> >   media: atomisp: fix call to g_frame_interval
> >   media: atomisp: print info if gpio0 and gpio2 were detected
> >   media: atomisp: split add from find subdev
> >   media: atomisp: place all gpio parsing together
> > 
> >  .../staging/media/atomisp/pci/atomisp_cmd.c   |   2 +-
> >  .../media/atomisp/pci/atomisp_gmin_platform.c | 393 ++++++++++++------
> >  2 files changed, 267 insertions(+), 128 deletions(-)
> > 
> > -- 
> > 2.26.2
> > 
> >   
> 



Thanks,
Mauro

Re: [PATCH 0/7] Some atomisp fixes and improvements

[Mauro Carvalho Chehab]

Em Fri, 26 Jun 2020 17:52:16 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Fri, 26 Jun 2020 18:00:21 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> escreveu:
> 
> > On Fri, Jun 26, 2020 at 04:04:52PM +0200, Mauro Carvalho Chehab wrote:
> > > Those patches are meant to improve device detection by the atomisp driver,
> > > relying on ACPI bios when possible.
> > > 
> > > It also adds a basis for using ACPI PM, but only if the DSDT tables have
> > > a description about how to turn on the resources needed by the cameras.
> > > 
> > > At least on the device I'm using for tests, this is not the case.  
> > 
> > Is this in your experimental tree? 
> 
> Yes. 
> 
> > I'll rebase mine on top and test.
> > After I will send the rest from my series and give a tag to this.
> 
> It would be helpful if you could test removing the DMI match table from
> your board. If your device has a DSDT table close to the one I have, the
> new code may be able to get everything from DSDT.

Err... I spoke too soon... looking on the logs from your 00/15, it 
seems that your device's DSDT is indeed different:

	[  116.401267] ACPI: \: failed to evaluate _DSM (0x1001)

If possible, could you please send me (could be in priv) a copy of your
DSDT?

Thanks,
Mauro

Re: [PATCH 0/7] Some atomisp fixes and improvements

[Andy Shevchenko]

On Fri, Jun 26, 2020 at 05:52:16PM +0200, Mauro Carvalho Chehab wrote:
> Em Fri, 26 Jun 2020 18:00:21 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> escreveu:
> 
> > On Fri, Jun 26, 2020 at 04:04:52PM +0200, Mauro Carvalho Chehab wrote:
> > > Those patches are meant to improve device detection by the atomisp driver,
> > > relying on ACPI bios when possible.
> > > 
> > > It also adds a basis for using ACPI PM, but only if the DSDT tables have
> > > a description about how to turn on the resources needed by the cameras.
> > > 
> > > At least on the device I'm using for tests, this is not the case.  
> > 
> > Is this in your experimental tree? 
> 
> Yes. 
> 
> > I'll rebase mine on top and test.
> > After I will send the rest from my series and give a tag to this.
> 
> It would be helpful if you could test removing the DMI match table from
> your board. If your device has a DSDT table close to the one I have, the
> new code may be able to get everything from DSDT.

I have checked the atomisp_v5 branch and it doesn't bring any regression to my
case. So, feel free to add
Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I'll send rebased patches soon.

-- 
With Best Regards,
Andy Shevchenko