Hi,
sorry it took me some time but this is the v3 for my previously send
patchstack:
https://lore.kernel.org/linux-media/[email protected]/
Changes in v3:
- Dropped 1/5 media: mt9p031: Add support for 8 bit and 10 bit formats
- Dropped 3/5 media: mt9p031: Implement [gs]_register debug calls
- Added reviewed-by from Laurent Pinchart to
media: mt9p031: Read back the real clock rate
- Dropped unnecessary register reads in
media: mt9p031: Fix corrupted frame after restarting
- Changed sorting of register bits from MSB to LSB
- Added patch to switch to BIT macro
- Added two additional dt-bindings patches to add missing properties
documentation
Christian Hemp (1):
media: mt9p031: Make pixel clock polarity configurable by DT
Dirk Bender (1):
media: mt9p031: Fix corrupted frame after restarting stream
Enrico Scholz (1):
media: mt9p031: Read back the real clock rate
Stefan Riedmueller (3):
dt-bindings: media: mt9p031: Add missing required properties
dt-bindings: media: mt9p031: add pclk-sample property
media: mt9p031: Use BIT macro
.../devicetree/bindings/media/i2c/mt9p031.txt | 17 +++++
drivers/media/i2c/Kconfig | 1 +
drivers/media/i2c/mt9p031.c | 71 +++++++++++++++----
include/media/i2c/mt9p031.h | 1 +
4 files changed, 78 insertions(+), 12 deletions(-)
--
2.25.1
From: Christian Hemp <[email protected]>
Evaluate the desired pixel clock polarity from the device tree.
Signed-off-by: Christian Hemp <[email protected]>
Signed-off-by: Stefan Riedmueller <[email protected]>
---
drivers/media/i2c/Kconfig | 1 +
drivers/media/i2c/mt9p031.c | 20 +++++++++++++++++++-
include/media/i2c/mt9p031.h | 1 +
3 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 588f8eb95984..1f9e98be8066 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -1187,6 +1187,7 @@ config VIDEO_MT9P031
select MEDIA_CONTROLLER
select VIDEO_V4L2_SUBDEV_API
select VIDEO_APTINA_PLL
+ select V4L2_FWNODE
help
This is a Video4Linux2 sensor driver for the Aptina
(Micron) mt9p031 5 Mpixel camera.
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 3eaaa8d44523..6a6f16df3f4a 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -27,6 +27,7 @@
#include <media/v4l2-async.h>
#include <media/v4l2-ctrls.h>
#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
#include <media/v4l2-subdev.h>
#include "aptina-pll.h"
@@ -398,6 +399,14 @@ static int __mt9p031_set_power(struct mt9p031 *mt9p031, bool on)
return ret;
}
+ /* Configure the pixel clock polarity */
+ if (mt9p031->pdata && mt9p031->pdata->pixclk_pol) {
+ ret = mt9p031_write(client, MT9P031_PIXEL_CLOCK_CONTROL,
+ MT9P031_PIXEL_CLOCK_INVERT);
+ if (ret < 0)
+ return ret;
+ }
+
return v4l2_ctrl_handler_setup(&mt9p031->ctrls);
}
@@ -1040,8 +1049,11 @@ static const struct v4l2_subdev_internal_ops mt9p031_subdev_internal_ops = {
static struct mt9p031_platform_data *
mt9p031_get_pdata(struct i2c_client *client)
{
- struct mt9p031_platform_data *pdata;
+ struct mt9p031_platform_data *pdata = NULL;
struct device_node *np;
+ struct v4l2_fwnode_endpoint endpoint = {
+ .bus_type = V4L2_MBUS_PARALLEL
+ };
if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
return client->dev.platform_data;
@@ -1050,6 +1062,9 @@ mt9p031_get_pdata(struct i2c_client *client)
if (!np)
return NULL;
+ if (v4l2_fwnode_endpoint_parse(of_fwnode_handle(np), &endpoint) < 0)
+ goto done;
+
pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata)
goto done;
@@ -1057,6 +1072,9 @@ mt9p031_get_pdata(struct i2c_client *client)
of_property_read_u32(np, "input-clock-frequency", &pdata->ext_freq);
of_property_read_u32(np, "pixel-clock-frequency", &pdata->target_freq);
+ pdata->pixclk_pol = !!(endpoint.bus.parallel.flags &
+ V4L2_MBUS_PCLK_SAMPLE_RISING);
+
done:
of_node_put(np);
return pdata;
diff --git a/include/media/i2c/mt9p031.h b/include/media/i2c/mt9p031.h
index 7c29c53aa988..f933cd0be8e5 100644
--- a/include/media/i2c/mt9p031.h
+++ b/include/media/i2c/mt9p031.h
@@ -10,6 +10,7 @@ struct v4l2_subdev;
* @target_freq: Pixel clock frequency
*/
struct mt9p031_platform_data {
+ unsigned int pixclk_pol:1;
int ext_freq;
int target_freq;
};
--
2.25.1
From: Enrico Scholz <[email protected]>
The real and requested clock can differ and because it is used to
calculate PLL values, the real clock rate should be read.
Signed-off-by: Enrico Scholz <[email protected]>
Signed-off-by: Stefan Riedmueller <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
drivers/media/i2c/mt9p031.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 77567341ec98..3eaaa8d44523 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -255,6 +255,7 @@ static int mt9p031_clk_setup(struct mt9p031 *mt9p031)
struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
struct mt9p031_platform_data *pdata = mt9p031->pdata;
+ unsigned long ext_freq;
int ret;
mt9p031->clk = devm_clk_get(&client->dev, NULL);
@@ -265,13 +266,15 @@ static int mt9p031_clk_setup(struct mt9p031 *mt9p031)
if (ret < 0)
return ret;
+ ext_freq = clk_get_rate(mt9p031->clk);
+
/* If the external clock frequency is out of bounds for the PLL use the
* pixel clock divider only and disable the PLL.
*/
- if (pdata->ext_freq > limits.ext_clock_max) {
+ if (ext_freq > limits.ext_clock_max) {
unsigned int div;
- div = DIV_ROUND_UP(pdata->ext_freq, pdata->target_freq);
+ div = DIV_ROUND_UP(ext_freq, pdata->target_freq);
div = roundup_pow_of_two(div) / 2;
mt9p031->clk_div = min_t(unsigned int, div, 64);
@@ -280,7 +283,7 @@ static int mt9p031_clk_setup(struct mt9p031 *mt9p031)
return 0;
}
- mt9p031->pll.ext_clock = pdata->ext_freq;
+ mt9p031->pll.ext_clock = ext_freq;
mt9p031->pll.pix_clock = pdata->target_freq;
mt9p031->use_pll = true;
--
2.25.1
Add missing required clocks and supply regulator properties for the
sensor input clock and vdd, vdd_io and vaa supply regulators.
Signed-off-by: Stefan Riedmueller <[email protected]>
---
.../devicetree/bindings/media/i2c/mt9p031.txt | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
index cb60443ff78f..4437d0e3147d 100644
--- a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
+++ b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
@@ -9,6 +9,12 @@ Required Properties:
(a) "aptina,mt9p031" for mt9p031 sensor
(b) "aptina,mt9p031m" for mt9p031m sensor
+- clocks: Reference to the sensor input clock
+
+- vdd-supply: VDD supply regulator
+- vdd_io-supply: VDD_IO supply regulator
+- vaa-supply: VAA supply regulator
+
- input-clock-frequency: Input clock frequency.
- pixel-clock-frequency: Pixel clock frequency.
@@ -29,6 +35,12 @@ Example:
reg = <0x5d>;
reset-gpios = <&gpio3 30 0>;
+ clocks = <&sensor_clk>;
+
+ vdd-supply = <®_vdd>;
+ vdd_io-supply = <®_vdd_io>;
+ vaa-supply = <®_vaa>;
+
port {
mt9p031_1: endpoint {
input-clock-frequency = <6000000>;
--
2.25.1
Add the pclk-sample property to the list of optional endpoint
properties for the mt9p031 camera sensor.
Signed-off-by: Stefan Riedmueller <[email protected]>
---
Documentation/devicetree/bindings/media/i2c/mt9p031.txt | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
index 4437d0e3147d..17e44fc6dc66 100644
--- a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
+++ b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
@@ -22,6 +22,10 @@ Required Properties:
Optional Properties:
- reset-gpios: Chip reset GPIO
+Optional endpoint properties:
+- pclk-sample: For information see ../video-interfaces.txt. The value is set to
+ 0 if it isn't specified.
+
For further reading on port node refer to
Documentation/devicetree/bindings/media/video-interfaces.txt.
@@ -45,6 +49,7 @@ Example:
mt9p031_1: endpoint {
input-clock-frequency = <6000000>;
pixel-clock-frequency = <96000000>;
+ pclk-sample = <1>;
};
};
};
--
2.25.1
Make use of the BIT macro for setting individual bits. This improves
readability and safety with respect to shifts.
Signed-off-by: Stefan Riedmueller <[email protected]>
---
drivers/media/i2c/mt9p031.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 3511c4ff350d..0a5bcbebe55f 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -76,39 +76,39 @@
#define MT9P031_PLL_CONFIG_1 0x11
#define MT9P031_PLL_CONFIG_2 0x12
#define MT9P031_PIXEL_CLOCK_CONTROL 0x0a
-#define MT9P031_PIXEL_CLOCK_INVERT (1 << 15)
+#define MT9P031_PIXEL_CLOCK_INVERT BIT(15)
#define MT9P031_PIXEL_CLOCK_SHIFT(n) ((n) << 8)
#define MT9P031_PIXEL_CLOCK_DIVIDE(n) ((n) << 0)
#define MT9P031_RESTART 0x0b
-#define MT9P031_FRAME_PAUSE_RESTART (1 << 1)
-#define MT9P031_FRAME_RESTART (1 << 0)
+#define MT9P031_FRAME_PAUSE_RESTART BIT(1)
+#define MT9P031_FRAME_RESTART BIT(0)
#define MT9P031_SHUTTER_DELAY 0x0c
#define MT9P031_RST 0x0d
#define MT9P031_RST_ENABLE 1
#define MT9P031_RST_DISABLE 0
#define MT9P031_READ_MODE_1 0x1e
#define MT9P031_READ_MODE_2 0x20
-#define MT9P031_READ_MODE_2_ROW_MIR (1 << 15)
-#define MT9P031_READ_MODE_2_COL_MIR (1 << 14)
-#define MT9P031_READ_MODE_2_ROW_BLC (1 << 6)
+#define MT9P031_READ_MODE_2_ROW_MIR BIT(15)
+#define MT9P031_READ_MODE_2_COL_MIR BIT(14)
+#define MT9P031_READ_MODE_2_ROW_BLC BIT(6)
#define MT9P031_ROW_ADDRESS_MODE 0x22
#define MT9P031_COLUMN_ADDRESS_MODE 0x23
#define MT9P031_GLOBAL_GAIN 0x35
#define MT9P031_GLOBAL_GAIN_MIN 8
#define MT9P031_GLOBAL_GAIN_MAX 1024
#define MT9P031_GLOBAL_GAIN_DEF 8
-#define MT9P031_GLOBAL_GAIN_MULT (1 << 6)
+#define MT9P031_GLOBAL_GAIN_MULT BIT(6)
#define MT9P031_ROW_BLACK_TARGET 0x49
#define MT9P031_ROW_BLACK_DEF_OFFSET 0x4b
#define MT9P031_GREEN1_OFFSET 0x60
#define MT9P031_GREEN2_OFFSET 0x61
#define MT9P031_BLACK_LEVEL_CALIBRATION 0x62
-#define MT9P031_BLC_MANUAL_BLC (1 << 0)
+#define MT9P031_BLC_MANUAL_BLC BIT(0)
#define MT9P031_RED_OFFSET 0x63
#define MT9P031_BLUE_OFFSET 0x64
#define MT9P031_TEST_PATTERN 0xa0
#define MT9P031_TEST_PATTERN_SHIFT 3
-#define MT9P031_TEST_PATTERN_ENABLE (1 << 0)
+#define MT9P031_TEST_PATTERN_ENABLE BIT(0)
#define MT9P031_TEST_PATTERN_DISABLE (0 << 0)
#define MT9P031_TEST_PATTERN_GREEN 0xa1
#define MT9P031_TEST_PATTERN_RED 0xa2
--
2.25.1
From: Dirk Bender <[email protected]>
To prevent corrupted frames after starting and stopping the sensor its
datasheet specifies a specific pause sequence to follow:
Stopping:
Set Pause_Restart Bit -> Set Restart Bit -> Set Chip_Enable Off
Restarting:
Set Chip_Enable On -> Clear Pause_Restart Bit
The Restart Bit is cleared automatically and must not be cleared
manually as this would cause undefined behavior.
Signed-off-by: Dirk Bender <[email protected]>
Signed-off-by: Stefan Riedmueller <[email protected]>
---
drivers/media/i2c/mt9p031.c | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 6a6f16df3f4a..3511c4ff350d 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -79,7 +79,9 @@
#define MT9P031_PIXEL_CLOCK_INVERT (1 << 15)
#define MT9P031_PIXEL_CLOCK_SHIFT(n) ((n) << 8)
#define MT9P031_PIXEL_CLOCK_DIVIDE(n) ((n) << 0)
-#define MT9P031_FRAME_RESTART 0x0b
+#define MT9P031_RESTART 0x0b
+#define MT9P031_FRAME_PAUSE_RESTART (1 << 1)
+#define MT9P031_FRAME_RESTART (1 << 0)
#define MT9P031_SHUTTER_DELAY 0x0c
#define MT9P031_RST 0x0d
#define MT9P031_RST_ENABLE 1
@@ -482,9 +484,23 @@ static int mt9p031_set_params(struct mt9p031 *mt9p031)
static int mt9p031_s_stream(struct v4l2_subdev *subdev, int enable)
{
struct mt9p031 *mt9p031 = to_mt9p031(subdev);
+ struct i2c_client *client = v4l2_get_subdevdata(subdev);
+ int val;
int ret;
if (!enable) {
+ /* enable pause restart */
+ val = MT9P031_FRAME_PAUSE_RESTART;
+ ret = mt9p031_write(client, MT9P031_RESTART, val);
+ if (ret < 0)
+ return ret;
+
+ /* enable restart + keep pause restart set */
+ val |= MT9P031_FRAME_RESTART;
+ ret = mt9p031_write(client, MT9P031_RESTART, val);
+ if (ret < 0)
+ return ret;
+
/* Stop sensor readout */
ret = mt9p031_set_output_control(mt9p031,
MT9P031_OUTPUT_CONTROL_CEN, 0);
@@ -504,6 +520,16 @@ static int mt9p031_s_stream(struct v4l2_subdev *subdev, int enable)
if (ret < 0)
return ret;
+ /*
+ * - clear pause restart
+ * - don't clear restart as clearing restart manually can cause
+ * undefined behavior
+ */
+ val = MT9P031_FRAME_RESTART;
+ ret = mt9p031_write(client, MT9P031_RESTART, val);
+ if (ret < 0)
+ return ret;
+
return mt9p031_pll_enable(mt9p031);
}
--
2.25.1
Hi Stefan,
Thank you for the patch.
On Fri, Jul 02, 2021 at 11:59:22AM +0200, Stefan Riedmueller wrote:
> Make use of the BIT macro for setting individual bits. This improves
> readability and safety with respect to shifts.
>
> Signed-off-by: Stefan Riedmueller <[email protected]>
> ---
> drivers/media/i2c/mt9p031.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index 3511c4ff350d..0a5bcbebe55f 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -76,39 +76,39 @@
> #define MT9P031_PLL_CONFIG_1 0x11
> #define MT9P031_PLL_CONFIG_2 0x12
> #define MT9P031_PIXEL_CLOCK_CONTROL 0x0a
> -#define MT9P031_PIXEL_CLOCK_INVERT (1 << 15)
> +#define MT9P031_PIXEL_CLOCK_INVERT BIT(15)
> #define MT9P031_PIXEL_CLOCK_SHIFT(n) ((n) << 8)
> #define MT9P031_PIXEL_CLOCK_DIVIDE(n) ((n) << 0)
> #define MT9P031_RESTART 0x0b
> -#define MT9P031_FRAME_PAUSE_RESTART (1 << 1)
> -#define MT9P031_FRAME_RESTART (1 << 0)
> +#define MT9P031_FRAME_PAUSE_RESTART BIT(1)
> +#define MT9P031_FRAME_RESTART BIT(0)
> #define MT9P031_SHUTTER_DELAY 0x0c
> #define MT9P031_RST 0x0d
> #define MT9P031_RST_ENABLE 1
This could also be turned into BIT(0).
> #define MT9P031_RST_DISABLE 0
This should then be dropped.
> #define MT9P031_READ_MODE_1 0x1e
> #define MT9P031_READ_MODE_2 0x20
> -#define MT9P031_READ_MODE_2_ROW_MIR (1 << 15)
> -#define MT9P031_READ_MODE_2_COL_MIR (1 << 14)
> -#define MT9P031_READ_MODE_2_ROW_BLC (1 << 6)
> +#define MT9P031_READ_MODE_2_ROW_MIR BIT(15)
> +#define MT9P031_READ_MODE_2_COL_MIR BIT(14)
> +#define MT9P031_READ_MODE_2_ROW_BLC BIT(6)
> #define MT9P031_ROW_ADDRESS_MODE 0x22
> #define MT9P031_COLUMN_ADDRESS_MODE 0x23
> #define MT9P031_GLOBAL_GAIN 0x35
> #define MT9P031_GLOBAL_GAIN_MIN 8
> #define MT9P031_GLOBAL_GAIN_MAX 1024
> #define MT9P031_GLOBAL_GAIN_DEF 8
> -#define MT9P031_GLOBAL_GAIN_MULT (1 << 6)
> +#define MT9P031_GLOBAL_GAIN_MULT BIT(6)
> #define MT9P031_ROW_BLACK_TARGET 0x49
> #define MT9P031_ROW_BLACK_DEF_OFFSET 0x4b
> #define MT9P031_GREEN1_OFFSET 0x60
> #define MT9P031_GREEN2_OFFSET 0x61
> #define MT9P031_BLACK_LEVEL_CALIBRATION 0x62
> -#define MT9P031_BLC_MANUAL_BLC (1 << 0)
> +#define MT9P031_BLC_MANUAL_BLC BIT(0)
> #define MT9P031_RED_OFFSET 0x63
> #define MT9P031_BLUE_OFFSET 0x64
> #define MT9P031_TEST_PATTERN 0xa0
> #define MT9P031_TEST_PATTERN_SHIFT 3
> -#define MT9P031_TEST_PATTERN_ENABLE (1 << 0)
> +#define MT9P031_TEST_PATTERN_ENABLE BIT(0)
> #define MT9P031_TEST_PATTERN_DISABLE (0 << 0)
Similarly here, MT9P031_TEST_PATTERN_DISABLE should be dropped.
> #define MT9P031_TEST_PATTERN_GREEN 0xa1
> #define MT9P031_TEST_PATTERN_RED 0xa2
--
Regards,
Laurent Pinchart
Hi Stefan,
Thank you for the patch.
On Fri, Jul 02, 2021 at 11:59:19AM +0200, Stefan Riedmueller wrote:
> Add missing required clocks and supply regulator properties for the
> sensor input clock and vdd, vdd_io and vaa supply regulators.
Can I volunteer you to convert these bindings to YAML first, and add the
properties on top ? :-)
> Signed-off-by: Stefan Riedmueller <[email protected]>
> ---
> .../devicetree/bindings/media/i2c/mt9p031.txt | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> index cb60443ff78f..4437d0e3147d 100644
> --- a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> @@ -9,6 +9,12 @@ Required Properties:
> (a) "aptina,mt9p031" for mt9p031 sensor
> (b) "aptina,mt9p031m" for mt9p031m sensor
>
> +- clocks: Reference to the sensor input clock
> +
> +- vdd-supply: VDD supply regulator
> +- vdd_io-supply: VDD_IO supply regulator
> +- vaa-supply: VAA supply regulator
> +
> - input-clock-frequency: Input clock frequency.
>
> - pixel-clock-frequency: Pixel clock frequency.
> @@ -29,6 +35,12 @@ Example:
> reg = <0x5d>;
> reset-gpios = <&gpio3 30 0>;
>
> + clocks = <&sensor_clk>;
> +
> + vdd-supply = <®_vdd>;
> + vdd_io-supply = <®_vdd_io>;
> + vaa-supply = <®_vaa>;
> +
> port {
> mt9p031_1: endpoint {
> input-clock-frequency = <6000000>;
--
Regards,
Laurent Pinchart
Hi Enrico,
On Fri, Jul 02, 2021 at 11:59:17AM +0200, Stefan Riedmueller wrote:
> From: Enrico Scholz <[email protected]>
>
> The real and requested clock can differ and because it is used to
> calculate PLL values, the real clock rate should be read.
Do you have a system where this happens? That suggests there's a wrong
value in DT.
The preference nowadays is to rely on assigned-clock-rates, even though
it's inherently somewhat unreliable, just as clk_set_rate(). This is an
existing driver though. The old ones could be kept for compatibility with
older DT binaries.
>
> Signed-off-by: Enrico Scholz <[email protected]>
> Signed-off-by: Stefan Riedmueller <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> drivers/media/i2c/mt9p031.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index 77567341ec98..3eaaa8d44523 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -255,6 +255,7 @@ static int mt9p031_clk_setup(struct mt9p031 *mt9p031)
>
> struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
> struct mt9p031_platform_data *pdata = mt9p031->pdata;
> + unsigned long ext_freq;
> int ret;
>
> mt9p031->clk = devm_clk_get(&client->dev, NULL);
> @@ -265,13 +266,15 @@ static int mt9p031_clk_setup(struct mt9p031 *mt9p031)
> if (ret < 0)
> return ret;
>
> + ext_freq = clk_get_rate(mt9p031->clk);
> +
> /* If the external clock frequency is out of bounds for the PLL use the
> * pixel clock divider only and disable the PLL.
> */
> - if (pdata->ext_freq > limits.ext_clock_max) {
> + if (ext_freq > limits.ext_clock_max) {
> unsigned int div;
>
> - div = DIV_ROUND_UP(pdata->ext_freq, pdata->target_freq);
> + div = DIV_ROUND_UP(ext_freq, pdata->target_freq);
> div = roundup_pow_of_two(div) / 2;
>
> mt9p031->clk_div = min_t(unsigned int, div, 64);
> @@ -280,7 +283,7 @@ static int mt9p031_clk_setup(struct mt9p031 *mt9p031)
> return 0;
> }
>
> - mt9p031->pll.ext_clock = pdata->ext_freq;
> + mt9p031->pll.ext_clock = ext_freq;
> mt9p031->pll.pix_clock = pdata->target_freq;
> mt9p031->use_pll = true;
>
--
Kind regards,
Sakari Ailus
Hi Sakari,
On Mon, 2021-07-05 at 10:13 +0300, Sakari Ailus wrote:
> Hi Enrico,
>
> On Fri, Jul 02, 2021 at 11:59:17AM +0200, Stefan Riedmueller wrote:
> > From: Enrico Scholz <[email protected]>
> >
> > The real and requested clock can differ and because it is used to
> > calculate PLL values, the real clock rate should be read.
>
> Do you have a system where this happens? That suggests there's a wrong
> value in DT.
The use case here is when the clock is supplied by one of the clock outputs of
a SOC which might not hit the requested frequency exactly due to internal PLL
configuration. So to get a better pixel clock the actual clock rate is read to
calculate the PLL parameters on the sensor. At least that's the idea.
Regards,
Stefan
>
> The preference nowadays is to rely on assigned-clock-rates, even though
> it's inherently somewhat unreliable, just as clk_set_rate(). This is an
> existing driver though. The old ones could be kept for compatibility with
> older DT binaries.
>
> > Signed-off-by: Enrico Scholz <[email protected]>
> > Signed-off-by: Stefan Riedmueller <[email protected]>
> > Reviewed-by: Laurent Pinchart <[email protected]>
> > ---
> > drivers/media/i2c/mt9p031.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> > index 77567341ec98..3eaaa8d44523 100644
> > --- a/drivers/media/i2c/mt9p031.c
> > +++ b/drivers/media/i2c/mt9p031.c
> > @@ -255,6 +255,7 @@ static int mt9p031_clk_setup(struct mt9p031 *mt9p031)
> >
> > struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
> > struct mt9p031_platform_data *pdata = mt9p031->pdata;
> > + unsigned long ext_freq;
> > int ret;
> >
> > mt9p031->clk = devm_clk_get(&client->dev, NULL);
> > @@ -265,13 +266,15 @@ static int mt9p031_clk_setup(struct mt9p031
> > *mt9p031)
> > if (ret < 0)
> > return ret;
> >
> > + ext_freq = clk_get_rate(mt9p031->clk);
> > +
> > /* If the external clock frequency is out of bounds for the PLL use
> > the
> > * pixel clock divider only and disable the PLL.
> > */
> > - if (pdata->ext_freq > limits.ext_clock_max) {
> > + if (ext_freq > limits.ext_clock_max) {
> > unsigned int div;
> >
> > - div = DIV_ROUND_UP(pdata->ext_freq, pdata->target_freq);
> > + div = DIV_ROUND_UP(ext_freq, pdata->target_freq);
> > div = roundup_pow_of_two(div) / 2;
> >
> > mt9p031->clk_div = min_t(unsigned int, div, 64);
> > @@ -280,7 +283,7 @@ static int mt9p031_clk_setup(struct mt9p031 *mt9p031)
> > return 0;
> > }
> >
> > - mt9p031->pll.ext_clock = pdata->ext_freq;
> > + mt9p031->pll.ext_clock = ext_freq;
> > mt9p031->pll.pix_clock = pdata->target_freq;
> > mt9p031->use_pll = true;
> >
Hi Laurent,
On Fri, 2021-07-02 at 16:15 +0300, Laurent Pinchart wrote:
> Hi Stefan,
>
> Thank you for the patch.
>
> On Fri, Jul 02, 2021 at 11:59:19AM +0200, Stefan Riedmueller wrote:
> > Add missing required clocks and supply regulator properties for the
> > sensor input clock and vdd, vdd_io and vaa supply regulators.
>
> Can I volunteer you to convert these bindings to YAML first, and add the
> properties on top ? :-)
Sure, I can give it a try :-)
Regards,
Stefan
>
> > Signed-off-by: Stefan Riedmueller <[email protected]>
> > ---
> > .../devicetree/bindings/media/i2c/mt9p031.txt | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> > b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> > index cb60443ff78f..4437d0e3147d 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
> > @@ -9,6 +9,12 @@ Required Properties:
> > (a) "aptina,mt9p031" for mt9p031 sensor
> > (b) "aptina,mt9p031m" for mt9p031m sensor
> >
> > +- clocks: Reference to the sensor input clock
> > +
> > +- vdd-supply: VDD supply regulator
> > +- vdd_io-supply: VDD_IO supply regulator
> > +- vaa-supply: VAA supply regulator
> > +
> > - input-clock-frequency: Input clock frequency.
> >
> > - pixel-clock-frequency: Pixel clock frequency.
> > @@ -29,6 +35,12 @@ Example:
> > reg = <0x5d>;
> > reset-gpios = <&gpio3 30 0>;
> >
> > + clocks = <&sensor_clk>;
> > +
> > + vdd-supply = <®_vdd>;
> > + vdd_io-supply = <®_vdd_io>;
> > + vaa-supply = <®_vaa>;
> > +
> > port {
> > mt9p031_1: endpoint {
> > input-clock-frequency = <6000000>;