mbox series

[v3,0/6] media: mt9p031: Read back the real clock rate

Message ID 20210702095922.118614-1-s.riedmueller@phytec.de
Headers show
Series media: mt9p031: Read back the real clock rate | expand

Message

Stefan Riedmüller July 2, 2021, 9:59 a.m. UTC 
Hi,

sorry it took me some time but this is the v3 for my previously send
patchstack:
https://lore.kernel.org/linux-media/20200930105133.139981-1-s.riedmueller@phytec.de/

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

Comments

Laurent Pinchart July 2, 2021, 1:13 p.m. UTC | #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 <s.riedmueller@phytec.de>
> ---
>  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
Sakari Ailus July 5, 2021, 7:13 a.m. UTC | #2 
Hi Enrico,

On Fri, Jul 02, 2021 at 11:59:17AM +0200, Stefan Riedmueller wrote:
> From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> 
> 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 <enrico.scholz@sigma-chemnitz.de>
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  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;
>
Stefan Riedmüller July 5, 2021, 7:42 a.m. UTC | #3 
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 <enrico.scholz@sigma-chemnitz.de>
> > 
> > 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 <enrico.scholz@sigma-chemnitz.de>
> > Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  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;
> >