[PATCH 0/4] Better support for ACPI in smiapp

[PATCH 0/4] Better support for ACPI in smiapp

[Sakari Ailus]

Hi,

The few patches here fix a bug (power sequence error handling) and change
clock handling in a way that allow using the driver in systems without
explicit clock control, e.g. ACPI.

Sakari Ailus (4):
  smiapp: Fix error handling in power on sequence
  smiapp: Verify clock frequency after setting it, prevent changing it
  smiapp: Get clock rate if it's not available through DT
  smiapp: Make clock control optional

 drivers/media/i2c/smiapp/smiapp-core.c | 50 ++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 12 deletions(-)

-- 
2.11.0

[PATCH 1/4] smiapp: Fix error handling in power on sequence

[Sakari Ailus]

The error handling code in smiapp_power_on() returned in case of a failed
I2C write instead of cleaning up the mess. Fix this.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 700f433261d0..d581625d7826 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -1313,7 +1313,7 @@ static int smiapp_power_on(struct device *dev)
 	rval = smiapp_write(sensor, SMIAPP_REG_U8_DPHY_CTRL,
 			    SMIAPP_DPHY_CTRL_UI);
 	if (rval < 0)
-		return rval;
+		goto out_cci_addr_fail;
 
 	rval = smiapp_call_quirk(sensor, post_poweron);
 	if (rval) {
-- 
2.11.0

[PATCH 2/4] smiapp: Verify clock frequency after setting it, prevent changing it

[Sakari Ailus]

The external clock frequency was set by the driver but the obtained
frequency was never verified. Do that.

Being able to obtain the exact frequency is important as the value is used
for PLL calculations which may result in frequencies that violate the PLL
tree limits.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index d581625d7826..55771826b446 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2870,6 +2870,7 @@ static int smiapp_probe(struct i2c_client *client,
 {
 	struct smiapp_sensor *sensor;
 	struct smiapp_hwconfig *hwcfg = smiapp_get_hwconfig(&client->dev);
+	unsigned long rate;
 	unsigned int i;
 	int rval;
 
@@ -2908,6 +2909,14 @@ static int smiapp_probe(struct i2c_client *client,
 		return rval;
 	}
 
+	rate = clk_get_rate(sensor->ext_clk);
+	if (rate != sensor->hwcfg->ext_clk) {
+		dev_err(&client->dev,
+			"can't set clock freq, asked for %u but got %lu\n",
+			sensor->hwcfg->ext_clk, rate);
+		return rval;
+	}
+
 	sensor->xshutdown = devm_gpiod_get_optional(&client->dev, "xshutdown",
 						    GPIOD_OUT_LOW);
 	if (IS_ERR(sensor->xshutdown))
-- 
2.11.0

[PATCH 3/4] smiapp: Get clock rate if it's not available through DT

[Sakari Ailus]

Obtain the clock rate from the clock framework if it's not available
through DT. The assumption is that the parent device (camera module)
defines the rate as clock control is a part of the power on and power off
sequences --- which are camera module specific.

Also use the clock rate from DT if no clock is provided.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c | 52 +++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 19 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 55771826b446..009b5e26204b 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2829,12 +2829,10 @@ static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
 	/* NVM size is not mandatory */
 	fwnode_property_read_u32(fwnode, "nokia,nvm-size", &hwcfg->nvm_size);
 
-	rval = fwnode_property_read_u32(fwnode, "clock-frequency",
+	rval = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
 					&hwcfg->ext_clk);
-	if (rval) {
-		dev_warn(dev, "can't get clock-frequency\n");
-		goto out_err;
-	}
+	if (rval)
+		dev_info(dev, "can't get clock-frequency\n");
 
 	dev_dbg(dev, "nvm %d, clk %d, mode %d\n",
 		hwcfg->nvm_size, hwcfg->ext_clk, hwcfg->csi_signalling_mode);
@@ -2870,7 +2868,6 @@ static int smiapp_probe(struct i2c_client *client,
 {
 	struct smiapp_sensor *sensor;
 	struct smiapp_hwconfig *hwcfg = smiapp_get_hwconfig(&client->dev);
-	unsigned long rate;
 	unsigned int i;
 	int rval;
 
@@ -2901,20 +2898,37 @@ static int smiapp_probe(struct i2c_client *client,
 		return -EPROBE_DEFER;
 	}
 
-	rval = clk_set_rate(sensor->ext_clk, sensor->hwcfg->ext_clk);
-	if (rval < 0) {
-		dev_err(&client->dev,
-			"unable to set clock freq to %u\n",
-			sensor->hwcfg->ext_clk);
-		return rval;
-	}
+	if (sensor->ext_clk) {
+		if (sensor->hwcfg->ext_clk) {
+			unsigned long rate;
 
-	rate = clk_get_rate(sensor->ext_clk);
-	if (rate != sensor->hwcfg->ext_clk) {
-		dev_err(&client->dev,
-			"can't set clock freq, asked for %u but got %lu\n",
-			sensor->hwcfg->ext_clk, rate);
-		return rval;
+			rval = clk_set_rate(sensor->ext_clk,
+					    sensor->hwcfg->ext_clk);
+			if (rval < 0) {
+				dev_err(&client->dev,
+					"unable to set clock freq to %u\n",
+					sensor->hwcfg->ext_clk);
+				return rval;
+			}
+
+			rate = clk_get_rate(sensor->ext_clk);
+			if (rate != sensor->hwcfg->ext_clk) {
+				dev_err(&client->dev,
+					"can't set clock freq, asked for %u but got %lu\n",
+					sensor->hwcfg->ext_clk, rate);
+				return rval;
+			}
+		} else {
+			sensor->hwcfg->ext_clk = clk_get_rate(sensor->ext_clk);
+			dev_dbg(&client->dev, "obtained clock freq %u\n",
+				sensor->hwcfg->ext_clk);
+		}
+	} else if (sensor->hwcfg->ext_clk) {
+		dev_dbg(&client->dev, "assuming clock freq %u\n",
+			sensor->hwcfg->ext_clk);
+	} else {
+		dev_err(&client->dev, "unable to obtain clock freq\n");
+		return -EINVAL;
 	}
 
 	sensor->xshutdown = devm_gpiod_get_optional(&client->dev, "xshutdown",
-- 
2.11.0

[PATCH 4/4] smiapp: Make clock control optional

[Sakari Ailus]

The clock control is not explicitly controlled by the driver in two cases:
ACPI based systems and when the clock is part of the power sequence of the
camera module.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 009b5e26204b..fbd851be51d2 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2892,7 +2892,10 @@ static int smiapp_probe(struct i2c_client *client,
 	}
 
 	sensor->ext_clk = devm_clk_get(&client->dev, NULL);
-	if (IS_ERR(sensor->ext_clk)) {
+	if (PTR_ERR(sensor->ext_clk) == -ENOENT) {
+		dev_info(&client->dev, "no clock defined, continuing...\n");
+		sensor->ext_clk = NULL;
+	} else if (IS_ERR(sensor->ext_clk)) {
 		dev_err(&client->dev, "could not get clock (%ld)\n",
 			PTR_ERR(sensor->ext_clk));
 		return -EPROBE_DEFER;
-- 
2.11.0

Re: [PATCH 3/4] smiapp: Get clock rate if it's not available through DT

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3066 bytes --]

Hi Sakari,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.10-rc8]
[cannot apply to linuxtv-media/master next-20170215]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sakari-Ailus/smiapp-cleanups-clock-control-changes/20170214-010429
config: x86_64-randconfig-it0-02160426 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/media/i2c/smiapp/smiapp-core.c: In function 'smiapp_probe':
>> drivers/media/i2c/smiapp/smiapp-core.c:2909:6: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'uint32_t' [-Wformat=]
         sensor->hwcfg->ext_clk, rate);
         ^

vim +2909 drivers/media/i2c/smiapp/smiapp-core.c

88ea1579 Sakari Ailus 2017-02-13  2893  		if (sensor->hwcfg->ext_clk) {
88ea1579 Sakari Ailus 2017-02-13  2894  			unsigned long rate;
88ea1579 Sakari Ailus 2017-02-13  2895  
88ea1579 Sakari Ailus 2017-02-13  2896  			rval = clk_set_rate(sensor->ext_clk,
88ea1579 Sakari Ailus 2017-02-13  2897  					    sensor->hwcfg->ext_clk);
3ecb8664 Sakari Ailus 2016-09-12  2898  			if (rval < 0) {
3ecb8664 Sakari Ailus 2016-09-12  2899  				dev_err(&client->dev,
3ecb8664 Sakari Ailus 2016-09-12  2900  					"unable to set clock freq to %u\n",
3ecb8664 Sakari Ailus 2016-09-12  2901  					sensor->hwcfg->ext_clk);
3ecb8664 Sakari Ailus 2016-09-12  2902  				return rval;
3ecb8664 Sakari Ailus 2016-09-12  2903  			}
3ecb8664 Sakari Ailus 2016-09-12  2904  
87cb6c6a Sakari Ailus 2017-02-13  2905  			rate = clk_get_rate(sensor->ext_clk);
87cb6c6a Sakari Ailus 2017-02-13  2906  			if (rate != sensor->hwcfg->ext_clk) {
87cb6c6a Sakari Ailus 2017-02-13  2907  				dev_err(&client->dev,
88ea1579 Sakari Ailus 2017-02-13  2908  					"can't set clock freq, asked for %lu but got %lu\n",
87cb6c6a Sakari Ailus 2017-02-13 @2909  					sensor->hwcfg->ext_clk, rate);
87cb6c6a Sakari Ailus 2017-02-13  2910  				return rval;
87cb6c6a Sakari Ailus 2017-02-13  2911  			}
88ea1579 Sakari Ailus 2017-02-13  2912  		} else {
88ea1579 Sakari Ailus 2017-02-13  2913  			sensor->hwcfg->ext_clk = clk_get_rate(sensor->ext_clk);
88ea1579 Sakari Ailus 2017-02-13  2914  			dev_dbg(&client->dev, "obtained clock freq %u\n",
88ea1579 Sakari Ailus 2017-02-13  2915  				sensor->hwcfg->ext_clk);
88ea1579 Sakari Ailus 2017-02-13  2916  		}
88ea1579 Sakari Ailus 2017-02-13  2917  	} else if (sensor->hwcfg->ext_clk) {

:::::: The code at line 2909 was first introduced by commit
:::::: 87cb6c6a8fdcfc1d0327e6c826165f0ba1b5eff0 smiapp: Verify clock frequency after setting it, prevent changing it

:::::: TO: Sakari Ailus <sakari.ailus@linux.intel.com>
:::::: CC: 0day robot <fengguang.wu@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31202 bytes --]

Re: [PATCH 3/4] smiapp: Get clock rate if it's not available through DT

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2015 bytes --]

Hi Sakari,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.10-rc8]
[cannot apply to linuxtv-media/master next-20170213]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sakari-Ailus/smiapp-cleanups-clock-control-changes/20170214-010429
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/media/i2c/smiapp/smiapp-core.c: In function 'smiapp_probe':
>> drivers/media/i2c/smiapp/smiapp-core.c:2908:41: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'uint32_t {aka unsigned int}' [-Wformat=]
         "can't set clock freq, asked for %lu but got %lu\n",
                                            ^

vim +2908 drivers/media/i2c/smiapp/smiapp-core.c

  2892		if (sensor->ext_clk) {
  2893			if (sensor->hwcfg->ext_clk) {
  2894				unsigned long rate;
  2895	
  2896				rval = clk_set_rate(sensor->ext_clk,
  2897						    sensor->hwcfg->ext_clk);
  2898				if (rval < 0) {
  2899					dev_err(&client->dev,
  2900						"unable to set clock freq to %u\n",
  2901						sensor->hwcfg->ext_clk);
  2902					return rval;
  2903				}
  2904	
  2905				rate = clk_get_rate(sensor->ext_clk);
  2906				if (rate != sensor->hwcfg->ext_clk) {
  2907					dev_err(&client->dev,
> 2908						"can't set clock freq, asked for %lu but got %lu\n",
  2909						sensor->hwcfg->ext_clk, rate);
  2910					return rval;
  2911				}
  2912			} else {
  2913				sensor->hwcfg->ext_clk = clk_get_rate(sensor->ext_clk);
  2914				dev_dbg(&client->dev, "obtained clock freq %u\n",
  2915					sensor->hwcfg->ext_clk);
  2916			}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 57940 bytes --]

[PATCH 3/4] smiapp: Get clock rate if it's not available through DT

[Sakari Ailus]

Obtain the clock rate from the clock framework if it's not available
through DT. The assumption is that the parent device (camera module)
defines the rate as clock control is a part of the power on and power off
sequences --- which are camera module specific.

Also use the clock rate from DT if no clock is provided.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c | 50 ++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 64ee215..caf376c 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2822,10 +2822,8 @@ static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
 
 	rval = of_property_read_u32(dev->of_node, "clock-frequency",
 				    &hwcfg->ext_clk);
-	if (rval) {
-		dev_warn(dev, "can't get clock-frequency\n");
-		goto out_err;
-	}
+	if (rval)
+		dev_info(dev, "can't get clock-frequency\n");
 
 	dev_dbg(dev, "nvm %d, clk %d, csi %d\n", hwcfg->nvm_size,
 		hwcfg->ext_clk, hwcfg->csi_signalling_mode);
@@ -2861,7 +2859,6 @@ static int smiapp_probe(struct i2c_client *client,
 {
 	struct smiapp_sensor *sensor;
 	struct smiapp_hwconfig *hwcfg = smiapp_get_hwconfig(&client->dev);
-	unsigned long rate;
 	unsigned int i;
 	int rval;
 
@@ -2892,20 +2889,37 @@ static int smiapp_probe(struct i2c_client *client,
 		return -EPROBE_DEFER;
 	}
 
-	rval = clk_set_rate(sensor->ext_clk, sensor->hwcfg->ext_clk);
-	if (rval < 0) {
-		dev_err(&client->dev,
-			"unable to set clock freq to %u\n",
-			sensor->hwcfg->ext_clk);
-		return rval;
-	}
+	if (sensor->ext_clk) {
+		if (sensor->hwcfg->ext_clk) {
+			unsigned long rate;
 
-	rate = clk_get_rate(sensor->ext_clk);
-	if (rate != sensor->hwcfg->ext_clk) {
-		dev_err(&client->dev,
-			"can't set clock freq, asked for %u but got %lu\n",
-			sensor->hwcfg->ext_clk, rate);
-		return rval;
+			rval = clk_set_rate(sensor->ext_clk,
+					    sensor->hwcfg->ext_clk);
+			if (rval < 0) {
+				dev_err(&client->dev,
+					"unable to set clock freq to %u\n",
+					sensor->hwcfg->ext_clk);
+				return rval;
+			}
+
+			rate = clk_get_rate(sensor->ext_clk);
+			if (rate != sensor->hwcfg->ext_clk) {
+				dev_err(&client->dev,
+					"can't set clock freq, asked for %lu but got %lu\n",
+					sensor->hwcfg->ext_clk, rate);
+				return rval;
+			}
+		} else {
+			sensor->hwcfg->ext_clk = clk_get_rate(sensor->ext_clk);
+			dev_dbg(&client->dev, "obtained clock freq %u\n",
+				sensor->hwcfg->ext_clk);
+		}
+	} else if (sensor->hwcfg->ext_clk) {
+		dev_dbg(&client->dev, "assuming clock freq %u\n",
+			sensor->hwcfg->ext_clk);
+	} else {
+		dev_err(&client->dev, "unable to obtain clock freq\n");
+		return -EINVAL;
 	}
 
 	sensor->xshutdown = devm_gpiod_get_optional(&client->dev, "xshutdown",
-- 
2.1.4