* [PATCH v2 2/5] media: mt9p031: Read back the real clock rate
2020-09-30 10:51 [PATCH v2 1/5] media: mt9p031: Add support for 8 bit and 10 bit formats Stefan Riedmueller
@ 2020-09-30 10:51 ` Stefan Riedmueller
2020-10-01 23:56 ` Laurent Pinchart
2020-09-30 10:51 ` [PATCH v2 3/5] media: mt9p031: Implement [gs]_register debug calls Stefan Riedmueller
` (3 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Stefan Riedmueller @ 2020-09-30 10:51 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel,
Enrico Scholz, Stefan Riedmueller
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.
Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
---
No changes in v2
---
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 2e6671ef877c..b4c042f418c1 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/5] media: mt9p031: Read back the real clock rate
2020-09-30 10:51 ` [PATCH v2 2/5] media: mt9p031: Read back the real clock rate Stefan Riedmueller
@ 2020-10-01 23:56 ` Laurent Pinchart
0 siblings, 0 replies; 21+ messages in thread
From: Laurent Pinchart @ 2020-10-01 23:56 UTC (permalink / raw)
To: Stefan Riedmueller
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel,
Enrico Scholz
Hi Stefan,
Thank you for the patch.
On Wed, Sep 30, 2020 at 12:51:30PM +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.
>
> 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>
> ---
> No changes in v2
> ---
> 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 2e6671ef877c..b4c042f418c1 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;
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 3/5] media: mt9p031: Implement [gs]_register debug calls
2020-09-30 10:51 [PATCH v2 1/5] media: mt9p031: Add support for 8 bit and 10 bit formats Stefan Riedmueller
2020-09-30 10:51 ` [PATCH v2 2/5] media: mt9p031: Read back the real clock rate Stefan Riedmueller
@ 2020-09-30 10:51 ` Stefan Riedmueller
2020-09-30 11:38 ` Laurent Pinchart
2020-09-30 10:51 ` [PATCH v2 4/5] media: mt9p031: Make pixel clock polarity configurable by DT Stefan Riedmueller
` (2 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Stefan Riedmueller @ 2020-09-30 10:51 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel,
Enrico Scholz, Stefan Riedmueller
From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
Implement g_register and s_register v4l2_subdev_core_ops to access
camera register directly from userspace for debug purposes.
Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
---
No changes in v2
---
drivers/media/i2c/mt9p031.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index b4c042f418c1..de36025260a8 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -703,6 +703,30 @@ static int mt9p031_restore_blc(struct mt9p031 *mt9p031)
return 0;
}
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+static int mt9p031_g_register(struct v4l2_subdev *sd,
+ struct v4l2_dbg_register *reg)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ int ret;
+
+ ret = mt9p031_read(client, reg->reg);
+ if (ret < 0)
+ return ret;
+
+ reg->val = ret;
+ return 0;
+}
+
+static int mt9p031_s_register(struct v4l2_subdev *sd,
+ struct v4l2_dbg_register const *reg)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+ return mt9p031_write(client, reg->reg, reg->val);
+}
+#endif
+
static int mt9p031_s_ctrl(struct v4l2_ctrl *ctrl)
{
struct mt9p031 *mt9p031 =
@@ -1000,6 +1024,10 @@ static int mt9p031_close(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
static const struct v4l2_subdev_core_ops mt9p031_subdev_core_ops = {
.s_power = mt9p031_set_power,
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+ .s_register = mt9p031_s_register,
+ .g_register = mt9p031_g_register,
+#endif
};
static const struct v4l2_subdev_video_ops mt9p031_subdev_video_ops = {
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 3/5] media: mt9p031: Implement [gs]_register debug calls
2020-09-30 10:51 ` [PATCH v2 3/5] media: mt9p031: Implement [gs]_register debug calls Stefan Riedmueller
@ 2020-09-30 11:38 ` Laurent Pinchart
2020-10-01 8:56 ` Stefan Riedmüller
0 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2020-09-30 11:38 UTC (permalink / raw)
To: Stefan Riedmueller
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel,
Enrico Scholz
Hi Stefan,
Thank you for the patch.
On Wed, Sep 30, 2020 at 12:51:31PM +0200, Stefan Riedmueller wrote:
> From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
>
> Implement g_register and s_register v4l2_subdev_core_ops to access
> camera register directly from userspace for debug purposes.
As the name of the operations imply, this is meant for debug purpose
only. They are however prone to be abused to configure the sensor from
userspace in production, which isn't a direction we want to take.
What's your use case for this ? I'd rather drop this patch and see the
driver extended with support for more controls if needed
> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> ---
> No changes in v2
> ---
> drivers/media/i2c/mt9p031.c | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index b4c042f418c1..de36025260a8 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -703,6 +703,30 @@ static int mt9p031_restore_blc(struct mt9p031 *mt9p031)
> return 0;
> }
>
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +static int mt9p031_g_register(struct v4l2_subdev *sd,
> + struct v4l2_dbg_register *reg)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> + int ret;
> +
> + ret = mt9p031_read(client, reg->reg);
> + if (ret < 0)
> + return ret;
> +
> + reg->val = ret;
> + return 0;
> +}
> +
> +static int mt9p031_s_register(struct v4l2_subdev *sd,
> + struct v4l2_dbg_register const *reg)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + return mt9p031_write(client, reg->reg, reg->val);
> +}
> +#endif
> +
> static int mt9p031_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct mt9p031 *mt9p031 =
> @@ -1000,6 +1024,10 @@ static int mt9p031_close(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
>
> static const struct v4l2_subdev_core_ops mt9p031_subdev_core_ops = {
> .s_power = mt9p031_set_power,
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> + .s_register = mt9p031_s_register,
> + .g_register = mt9p031_g_register,
> +#endif
> };
>
> static const struct v4l2_subdev_video_ops mt9p031_subdev_video_ops = {
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 3/5] media: mt9p031: Implement [gs]_register debug calls
2020-09-30 11:38 ` Laurent Pinchart
@ 2020-10-01 8:56 ` Stefan Riedmüller
2020-10-02 0:06 ` Laurent Pinchart
0 siblings, 1 reply; 21+ messages in thread
From: Stefan Riedmüller @ 2020-10-01 8:56 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel,
Enrico Scholz
Hi Laurent,
On 30.09.20 13:38, Laurent Pinchart wrote:
> Hi Stefan,
>
> Thank you for the patch.
>
> On Wed, Sep 30, 2020 at 12:51:31PM +0200, Stefan Riedmueller wrote:
>> From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
>>
>> Implement g_register and s_register v4l2_subdev_core_ops to access
>> camera register directly from userspace for debug purposes.
>
> As the name of the operations imply, this is meant for debug purpose
> only. They are however prone to be abused to configure the sensor from
> userspace in production, which isn't a direction we want to take.
> What's your use case for this ? I'd rather drop this patch and see the
> driver extended with support for more controls if needed
thanks for your feedback.
I get your point. I myself solely use these operations for debugging
purposes but I'm aware that others like to abuse them.
I thought I send it anyway since for me the DEBUG config is enough to
signalize that these operations are not to be used with a productive system.
But I'm OK with dropping this patch if you think it might send the wrong signal.
Regards,
Stefan
>
>> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
>> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
>> ---
>> No changes in v2
>> ---
>> drivers/media/i2c/mt9p031.c | 28 ++++++++++++++++++++++++++++
>> 1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
>> index b4c042f418c1..de36025260a8 100644
>> --- a/drivers/media/i2c/mt9p031.c
>> +++ b/drivers/media/i2c/mt9p031.c
>> @@ -703,6 +703,30 @@ static int mt9p031_restore_blc(struct mt9p031 *mt9p031)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>> +static int mt9p031_g_register(struct v4l2_subdev *sd,
>> + struct v4l2_dbg_register *reg)
>> +{
>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>> + int ret;
>> +
>> + ret = mt9p031_read(client, reg->reg);
>> + if (ret < 0)
>> + return ret;
>> +
>> + reg->val = ret;
>> + return 0;
>> +}
>> +
>> +static int mt9p031_s_register(struct v4l2_subdev *sd,
>> + struct v4l2_dbg_register const *reg)
>> +{
>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> + return mt9p031_write(client, reg->reg, reg->val);
>> +}
>> +#endif
>> +
>> static int mt9p031_s_ctrl(struct v4l2_ctrl *ctrl)
>> {
>> struct mt9p031 *mt9p031 =
>> @@ -1000,6 +1024,10 @@ static int mt9p031_close(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
>>
>> static const struct v4l2_subdev_core_ops mt9p031_subdev_core_ops = {
>> .s_power = mt9p031_set_power,
>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>> + .s_register = mt9p031_s_register,
>> + .g_register = mt9p031_g_register,
>> +#endif
>> };
>>
>> static const struct v4l2_subdev_video_ops mt9p031_subdev_video_ops = {
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 3/5] media: mt9p031: Implement [gs]_register debug calls
2020-10-01 8:56 ` Stefan Riedmüller
@ 2020-10-02 0:06 ` Laurent Pinchart
2020-10-05 9:33 ` Stefan Riedmüller
0 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2020-10-02 0:06 UTC (permalink / raw)
To: Stefan Riedmüller
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel,
Enrico Scholz
Hi Stefan,
On Thu, Oct 01, 2020 at 10:56:24AM +0200, Stefan Riedmüller wrote:
> On 30.09.20 13:38, Laurent Pinchart wrote:
> > On Wed, Sep 30, 2020 at 12:51:31PM +0200, Stefan Riedmueller wrote:
> >> From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> >>
> >> Implement g_register and s_register v4l2_subdev_core_ops to access
> >> camera register directly from userspace for debug purposes.
> >
> > As the name of the operations imply, this is meant for debug purpose
> > only. They are however prone to be abused to configure the sensor from
> > userspace in production, which isn't a direction we want to take.
> > What's your use case for this ? I'd rather drop this patch and see the
> > driver extended with support for more controls if needed
>
> thanks for your feedback.
>
> I get your point. I myself solely use these operations for debugging
> purposes but I'm aware that others like to abuse them.
>
> I thought I send it anyway since for me the DEBUG config is enough to
> signalize that these operations are not to be used with a productive system.
> But I'm OK with dropping this patch if you think it might send the wrong signal.
I'd rather avoid this patch due to the risk of abuse if it's OK with
you.
> >> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> >> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> >> ---
> >> No changes in v2
> >> ---
> >> drivers/media/i2c/mt9p031.c | 28 ++++++++++++++++++++++++++++
> >> 1 file changed, 28 insertions(+)
> >>
> >> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> >> index b4c042f418c1..de36025260a8 100644
> >> --- a/drivers/media/i2c/mt9p031.c
> >> +++ b/drivers/media/i2c/mt9p031.c
> >> @@ -703,6 +703,30 @@ static int mt9p031_restore_blc(struct mt9p031 *mt9p031)
> >> return 0;
> >> }
> >>
> >> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> >> +static int mt9p031_g_register(struct v4l2_subdev *sd,
> >> + struct v4l2_dbg_register *reg)
> >> +{
> >> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> >> + int ret;
> >> +
> >> + ret = mt9p031_read(client, reg->reg);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + reg->val = ret;
> >> + return 0;
> >> +}
> >> +
> >> +static int mt9p031_s_register(struct v4l2_subdev *sd,
> >> + struct v4l2_dbg_register const *reg)
> >> +{
> >> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> >> +
> >> + return mt9p031_write(client, reg->reg, reg->val);
> >> +}
> >> +#endif
> >> +
> >> static int mt9p031_s_ctrl(struct v4l2_ctrl *ctrl)
> >> {
> >> struct mt9p031 *mt9p031 =
> >> @@ -1000,6 +1024,10 @@ static int mt9p031_close(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
> >>
> >> static const struct v4l2_subdev_core_ops mt9p031_subdev_core_ops = {
> >> .s_power = mt9p031_set_power,
> >> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> >> + .s_register = mt9p031_s_register,
> >> + .g_register = mt9p031_g_register,
> >> +#endif
> >> };
> >>
> >> static const struct v4l2_subdev_video_ops mt9p031_subdev_video_ops = {
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 3/5] media: mt9p031: Implement [gs]_register debug calls
2020-10-02 0:06 ` Laurent Pinchart
@ 2020-10-05 9:33 ` Stefan Riedmüller
0 siblings, 0 replies; 21+ messages in thread
From: Stefan Riedmüller @ 2020-10-05 9:33 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel,
Enrico Scholz
Hi Laurent,
On 02.10.20 02:06, Laurent Pinchart wrote:
> Hi Stefan,
>
> On Thu, Oct 01, 2020 at 10:56:24AM +0200, Stefan Riedmüller wrote:
>> On 30.09.20 13:38, Laurent Pinchart wrote:
>>> On Wed, Sep 30, 2020 at 12:51:31PM +0200, Stefan Riedmueller wrote:
>>>> From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
>>>>
>>>> Implement g_register and s_register v4l2_subdev_core_ops to access
>>>> camera register directly from userspace for debug purposes.
>>>
>>> As the name of the operations imply, this is meant for debug purpose
>>> only. They are however prone to be abused to configure the sensor from
>>> userspace in production, which isn't a direction we want to take.
>>> What's your use case for this ? I'd rather drop this patch and see the
>>> driver extended with support for more controls if needed
>>
>> thanks for your feedback.
>>
>> I get your point. I myself solely use these operations for debugging
>> purposes but I'm aware that others like to abuse them.
>>
>> I thought I send it anyway since for me the DEBUG config is enough to
>> signalize that these operations are not to be used with a productive system.
>> But I'm OK with dropping this patch if you think it might send the wrong signal.
>
> I'd rather avoid this patch due to the risk of abuse if it's OK with
> you.
Yes, that's fine. I will drop it in v3.
Regards,
Stefan
>
>>>> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
>>>> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
>>>> ---
>>>> No changes in v2
>>>> ---
>>>> drivers/media/i2c/mt9p031.c | 28 ++++++++++++++++++++++++++++
>>>> 1 file changed, 28 insertions(+)
>>>>
>>>> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
>>>> index b4c042f418c1..de36025260a8 100644
>>>> --- a/drivers/media/i2c/mt9p031.c
>>>> +++ b/drivers/media/i2c/mt9p031.c
>>>> @@ -703,6 +703,30 @@ static int mt9p031_restore_blc(struct mt9p031 *mt9p031)
>>>> return 0;
>>>> }
>>>>
>>>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>>>> +static int mt9p031_g_register(struct v4l2_subdev *sd,
>>>> + struct v4l2_dbg_register *reg)
>>>> +{
>>>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>> + int ret;
>>>> +
>>>> + ret = mt9p031_read(client, reg->reg);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + reg->val = ret;
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int mt9p031_s_register(struct v4l2_subdev *sd,
>>>> + struct v4l2_dbg_register const *reg)
>>>> +{
>>>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>> +
>>>> + return mt9p031_write(client, reg->reg, reg->val);
>>>> +}
>>>> +#endif
>>>> +
>>>> static int mt9p031_s_ctrl(struct v4l2_ctrl *ctrl)
>>>> {
>>>> struct mt9p031 *mt9p031 =
>>>> @@ -1000,6 +1024,10 @@ static int mt9p031_close(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
>>>>
>>>> static const struct v4l2_subdev_core_ops mt9p031_subdev_core_ops = {
>>>> .s_power = mt9p031_set_power,
>>>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>>>> + .s_register = mt9p031_s_register,
>>>> + .g_register = mt9p031_g_register,
>>>> +#endif
>>>> };
>>>>
>>>> static const struct v4l2_subdev_video_ops mt9p031_subdev_video_ops = {
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 4/5] media: mt9p031: Make pixel clock polarity configurable by DT
2020-09-30 10:51 [PATCH v2 1/5] media: mt9p031: Add support for 8 bit and 10 bit formats Stefan Riedmueller
2020-09-30 10:51 ` [PATCH v2 2/5] media: mt9p031: Read back the real clock rate Stefan Riedmueller
2020-09-30 10:51 ` [PATCH v2 3/5] media: mt9p031: Implement [gs]_register debug calls Stefan Riedmueller
@ 2020-09-30 10:51 ` Stefan Riedmueller
2020-10-01 16:10 ` Sakari Ailus
2020-09-30 10:51 ` [PATCH v2 5/5] media: mt9p031: Fix corrupted frame after restarting stream Stefan Riedmueller
2020-09-30 11:42 ` [PATCH v2 1/5] media: mt9p031: Add support for 8 bit and 10 bit formats Laurent Pinchart
4 siblings, 1 reply; 21+ messages in thread
From: Stefan Riedmueller @ 2020-09-30 10:51 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel,
Christian Hemp, Stefan Riedmueller
From: Christian Hemp <c.hemp@phytec.de>
Evaluate the desired pixel clock polarity from the device tree.
Signed-off-by: Christian Hemp <c.hemp@phytec.de>
Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
---
Changes in v2:
- Initialise endpoint bus_type field to V4L2_MBUS_PARALLEL since the
sensor only supports a parallel interface
---
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 c7ba76fee599..7c026daeacf0 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -1103,6 +1103,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 de36025260a8..d10457361e6c 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"
@@ -399,6 +400,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);
}
@@ -1062,8 +1071,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;
@@ -1072,6 +1084,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;
@@ -1079,6 +1094,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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 4/5] media: mt9p031: Make pixel clock polarity configurable by DT
2020-09-30 10:51 ` [PATCH v2 4/5] media: mt9p031: Make pixel clock polarity configurable by DT Stefan Riedmueller
@ 2020-10-01 16:10 ` Sakari Ailus
2020-10-01 16:11 ` Sakari Ailus
0 siblings, 1 reply; 21+ messages in thread
From: Sakari Ailus @ 2020-10-01 16:10 UTC (permalink / raw)
To: Stefan Riedmueller
Cc: Laurent Pinchart, Mauro Carvalho Chehab, linux-media,
linux-kernel, Christian Hemp
Hi Stefan,
On Wed, Sep 30, 2020 at 12:51:32PM +0200, Stefan Riedmueller wrote:
> From: Christian Hemp <c.hemp@phytec.de>
>
> Evaluate the desired pixel clock polarity from the device tree.
>
> Signed-off-by: Christian Hemp <c.hemp@phytec.de>
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> ---
> Changes in v2:
> - Initialise endpoint bus_type field to V4L2_MBUS_PARALLEL since the
> sensor only supports a parallel interface
> ---
> 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 c7ba76fee599..7c026daeacf0 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1103,6 +1103,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 de36025260a8..d10457361e6c 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"
> @@ -399,6 +400,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);
> }
>
> @@ -1062,8 +1071,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;
> @@ -1072,6 +1084,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;
> @@ -1079,6 +1094,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);
Could you document this in DT bindings? And the default, too.
Ideally other properties relevant to the device would be there, too.
--
Sakari Ailus
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 4/5] media: mt9p031: Make pixel clock polarity configurable by DT
2020-10-01 16:10 ` Sakari Ailus
@ 2020-10-01 16:11 ` Sakari Ailus
2020-10-05 8:52 ` Stefan Riedmüller
0 siblings, 1 reply; 21+ messages in thread
From: Sakari Ailus @ 2020-10-01 16:11 UTC (permalink / raw)
To: Stefan Riedmueller
Cc: Laurent Pinchart, Mauro Carvalho Chehab, linux-media,
linux-kernel, Christian Hemp
On Thu, Oct 01, 2020 at 07:10:31PM +0300, Sakari Ailus wrote:
> > @@ -1079,6 +1094,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);
>
> Could you document this in DT bindings? And the default, too.
Please make it a separate patch.
--
Sakari Ailus
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/5] media: mt9p031: Make pixel clock polarity configurable by DT
2020-10-01 16:11 ` Sakari Ailus
@ 2020-10-05 8:52 ` Stefan Riedmüller
0 siblings, 0 replies; 21+ messages in thread
From: Stefan Riedmüller @ 2020-10-05 8:52 UTC (permalink / raw)
To: Sakari Ailus
Cc: Laurent Pinchart, Mauro Carvalho Chehab, linux-media,
linux-kernel, Christian Hemp
Hi Sakari,
On 01.10.20 18:11, Sakari Ailus wrote:
> On Thu, Oct 01, 2020 at 07:10:31PM +0300, Sakari Ailus wrote:
>>> @@ -1079,6 +1094,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);
>>
>> Could you document this in DT bindings? And the default, too.
>
> Please make it a separate patch.
Sure, I'll send a separate patch for the DT bindings.
Regards,
Stefan
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 5/5] media: mt9p031: Fix corrupted frame after restarting stream
2020-09-30 10:51 [PATCH v2 1/5] media: mt9p031: Add support for 8 bit and 10 bit formats Stefan Riedmueller
` (2 preceding siblings ...)
2020-09-30 10:51 ` [PATCH v2 4/5] media: mt9p031: Make pixel clock polarity configurable by DT Stefan Riedmueller
@ 2020-09-30 10:51 ` Stefan Riedmueller
2020-10-02 0:05 ` Laurent Pinchart
2020-09-30 11:42 ` [PATCH v2 1/5] media: mt9p031: Add support for 8 bit and 10 bit formats Laurent Pinchart
4 siblings, 1 reply; 21+ messages in thread
From: Stefan Riedmueller @ 2020-09-30 10:51 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel,
Dirk Bender, Stefan Riedmueller
From: Dirk Bender <d.bender@phytec.de>
To prevent corrupted frames after starting and stopping the sensor it's
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 <d.bender@phytec.de>
Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
---
No changes in v2
---
drivers/media/i2c/mt9p031.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index d10457361e6c..d59f66e3dcf3 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -80,6 +80,8 @@
#define MT9P031_PIXEL_CLOCK_SHIFT(n) ((n) << 8)
#define MT9P031_PIXEL_CLOCK_DIVIDE(n) ((n) << 0)
#define MT9P031_FRAME_RESTART 0x0b
+#define MT9P031_FRAME_RESTART_SET (1 << 0)
+#define MT9P031_FRAME_PAUSE_RESTART_SET (1 << 1)
#define MT9P031_SHUTTER_DELAY 0x0c
#define MT9P031_RST 0x0d
#define MT9P031_RST_ENABLE 1
@@ -483,9 +485,25 @@ 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) {
+ val = mt9p031_read(client, MT9P031_FRAME_RESTART);
+
+ /* enable pause restart */
+ val |= MT9P031_FRAME_PAUSE_RESTART_SET;
+ ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
+ if (ret < 0)
+ return ret;
+
+ /* enable restart + keep pause restart set */
+ val |= MT9P031_FRAME_RESTART_SET;
+ ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
+ if (ret < 0)
+ return ret;
+
/* Stop sensor readout */
ret = mt9p031_set_output_control(mt9p031,
MT9P031_OUTPUT_CONTROL_CEN, 0);
@@ -505,6 +523,13 @@ static int mt9p031_s_stream(struct v4l2_subdev *subdev, int enable)
if (ret < 0)
return ret;
+ val = mt9p031_read(client, MT9P031_FRAME_RESTART);
+ /* disable reset + pause restart */
+ val &= ~MT9P031_FRAME_PAUSE_RESTART_SET;
+ ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
+ if (ret < 0)
+ return ret;
+
return mt9p031_pll_enable(mt9p031);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 5/5] media: mt9p031: Fix corrupted frame after restarting stream
2020-09-30 10:51 ` [PATCH v2 5/5] media: mt9p031: Fix corrupted frame after restarting stream Stefan Riedmueller
@ 2020-10-02 0:05 ` Laurent Pinchart
2020-10-05 9:28 ` Stefan Riedmüller
0 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2020-10-02 0:05 UTC (permalink / raw)
To: Stefan Riedmueller
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel,
Dirk Bender
Hi Stefan,
Thank you for the patch.
On Wed, Sep 30, 2020 at 12:51:33PM +0200, Stefan Riedmueller wrote:
> From: Dirk Bender <d.bender@phytec.de>
>
> To prevent corrupted frames after starting and stopping the sensor it's
s/it's/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 <d.bender@phytec.de>
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> ---
> No changes in v2
> ---
> drivers/media/i2c/mt9p031.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index d10457361e6c..d59f66e3dcf3 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -80,6 +80,8 @@
> #define MT9P031_PIXEL_CLOCK_SHIFT(n) ((n) << 8)
> #define MT9P031_PIXEL_CLOCK_DIVIDE(n) ((n) << 0)
> #define MT9P031_FRAME_RESTART 0x0b
> +#define MT9P031_FRAME_RESTART_SET (1 << 0)
> +#define MT9P031_FRAME_PAUSE_RESTART_SET (1 << 1)
The fields are named Restart and Pause_Restart, I would drop _SET. Could
you also sort them from MSB to LSB as for the other registers ? Using
BIT() would be good too, although this could be done as an additional
patch to convert all the existing macros.
> #define MT9P031_SHUTTER_DELAY 0x0c
> #define MT9P031_RST 0x0d
> #define MT9P031_RST_ENABLE 1
> @@ -483,9 +485,25 @@ 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) {
> + val = mt9p031_read(client, MT9P031_FRAME_RESTART);
Do you need to read the register ? Can't you write
MT9P031_FRAME_PAUSE_RESTART_SET and then MT9P031_FRAME_PAUSE_RESTART_SET
| MT9P031_FRAME_RESTART_SET ? And actually, can't we just write both
bits in one go, do we need two writes ?
> +
> + /* enable pause restart */
> + val |= MT9P031_FRAME_PAUSE_RESTART_SET;
> + ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
> + if (ret < 0)
> + return ret;
> +
> + /* enable restart + keep pause restart set */
> + val |= MT9P031_FRAME_RESTART_SET;
> + ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
> + if (ret < 0)
> + return ret;
> +
> /* Stop sensor readout */
> ret = mt9p031_set_output_control(mt9p031,
> MT9P031_OUTPUT_CONTROL_CEN, 0);
> @@ -505,6 +523,13 @@ static int mt9p031_s_stream(struct v4l2_subdev *subdev, int enable)
> if (ret < 0)
> return ret;
>
> + val = mt9p031_read(client, MT9P031_FRAME_RESTART);
> + /* disable reset + pause restart */
> + val &= ~MT9P031_FRAME_PAUSE_RESTART_SET;
Same here, I think you can simply write MT9P031_FRAME_PAUSE_RESTART_SET.
> + ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
> + if (ret < 0)
> + return ret;
> +
> return mt9p031_pll_enable(mt9p031);
> }
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 5/5] media: mt9p031: Fix corrupted frame after restarting stream
2020-10-02 0:05 ` Laurent Pinchart
@ 2020-10-05 9:28 ` Stefan Riedmüller
2020-10-05 13:08 ` Laurent Pinchart
0 siblings, 1 reply; 21+ messages in thread
From: Stefan Riedmüller @ 2020-10-05 9:28 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel,
Dirk Bender
Hi Laurent,
On 02.10.20 02:05, Laurent Pinchart wrote:
> Hi Stefan,
>
> Thank you for the patch.
>
> On Wed, Sep 30, 2020 at 12:51:33PM +0200, Stefan Riedmueller wrote:
>> From: Dirk Bender <d.bender@phytec.de>
>>
>> To prevent corrupted frames after starting and stopping the sensor it's
>
> s/it's/its/
thanks, I'll fix that.
>
>> 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 <d.bender@phytec.de>
>> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
>> ---
>> No changes in v2
>> ---
>> drivers/media/i2c/mt9p031.c | 25 +++++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
>> index d10457361e6c..d59f66e3dcf3 100644
>> --- a/drivers/media/i2c/mt9p031.c
>> +++ b/drivers/media/i2c/mt9p031.c
>> @@ -80,6 +80,8 @@
>> #define MT9P031_PIXEL_CLOCK_SHIFT(n) ((n) << 8)
>> #define MT9P031_PIXEL_CLOCK_DIVIDE(n) ((n) << 0)
>> #define MT9P031_FRAME_RESTART 0x0b
>> +#define MT9P031_FRAME_RESTART_SET (1 << 0)
>> +#define MT9P031_FRAME_PAUSE_RESTART_SET (1 << 1)
>
> The fields are named Restart and Pause_Restart, I would drop _SET. Could
> you also sort them from MSB to LSB as for the other registers ? Using
> BIT() would be good too, although this could be done as an additional
> patch to convert all the existing macros.
I'll do that. Also I will rename the register to MT9P031_RESTART and the
bits to MT9P031_FRAME_RESTART and MT9P031_FRAME_PAUSE_RESTART.
>
>> #define MT9P031_SHUTTER_DELAY 0x0c
>> #define MT9P031_RST 0x0d
>> #define MT9P031_RST_ENABLE 1
>> @@ -483,9 +485,25 @@ 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) {
>> + val = mt9p031_read(client, MT9P031_FRAME_RESTART);
>
> Do you need to read the register ? Can't you write
> MT9P031_FRAME_PAUSE_RESTART_SET and then MT9P031_FRAME_PAUSE_RESTART_SET
> | MT9P031_FRAME_RESTART_SET ? And actually, can't we just write both
> bits in one go, do we need two writes ?
I think you're right we don't necessarily need to read the registers. The
only other bit is not used by the driver.
But I think we do need two separate writes, at least that is what the
datasheet states.
So I would drop the read but keep both write, ok?
>
>> +
>> + /* enable pause restart */
>> + val |= MT9P031_FRAME_PAUSE_RESTART_SET;
>> + ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* enable restart + keep pause restart set */
>> + val |= MT9P031_FRAME_RESTART_SET;
>> + ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
>> + if (ret < 0)
>> + return ret;
>> +
>> /* Stop sensor readout */
>> ret = mt9p031_set_output_control(mt9p031,
>> MT9P031_OUTPUT_CONTROL_CEN, 0);
>> @@ -505,6 +523,13 @@ static int mt9p031_s_stream(struct v4l2_subdev *subdev, int enable)
>> if (ret < 0)
>> return ret;
>>
>> + val = mt9p031_read(client, MT9P031_FRAME_RESTART);
>> + /* disable reset + pause restart */
>> + val &= ~MT9P031_FRAME_PAUSE_RESTART_SET;
>
> Same here, I think you can simply write MT9P031_FRAME_PAUSE_RESTART_SET.
I'll drop the read here as well. But I need to make sure, that the Restart
Bit is not cleared manually here.
Regards,
Stefan
>
>> + ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
>> + if (ret < 0)
>> + return ret;
>> +
>> return mt9p031_pll_enable(mt9p031);
>> }
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 5/5] media: mt9p031: Fix corrupted frame after restarting stream
2020-10-05 9:28 ` Stefan Riedmüller
@ 2020-10-05 13:08 ` Laurent Pinchart
2020-10-14 7:14 ` Stefan Riedmüller
0 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2020-10-05 13:08 UTC (permalink / raw)
To: Stefan Riedmüller
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel,
Dirk Bender
Hi Stefan,
On Mon, Oct 05, 2020 at 11:28:21AM +0200, Stefan Riedmüller wrote:
> On 02.10.20 02:05, Laurent Pinchart wrote:
> > On Wed, Sep 30, 2020 at 12:51:33PM +0200, Stefan Riedmueller wrote:
> >> From: Dirk Bender <d.bender@phytec.de>
> >>
> >> To prevent corrupted frames after starting and stopping the sensor it's
> >
> > s/it's/its/
>
> thanks, I'll fix that.
>
> >> 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 <d.bender@phytec.de>
> >> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> >> ---
> >> No changes in v2
> >> ---
> >> drivers/media/i2c/mt9p031.c | 25 +++++++++++++++++++++++++
> >> 1 file changed, 25 insertions(+)
> >>
> >> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> >> index d10457361e6c..d59f66e3dcf3 100644
> >> --- a/drivers/media/i2c/mt9p031.c
> >> +++ b/drivers/media/i2c/mt9p031.c
> >> @@ -80,6 +80,8 @@
> >> #define MT9P031_PIXEL_CLOCK_SHIFT(n) ((n) << 8)
> >> #define MT9P031_PIXEL_CLOCK_DIVIDE(n) ((n) << 0)
> >> #define MT9P031_FRAME_RESTART 0x0b
> >> +#define MT9P031_FRAME_RESTART_SET (1 << 0)
> >> +#define MT9P031_FRAME_PAUSE_RESTART_SET (1 << 1)
> >
> > The fields are named Restart and Pause_Restart, I would drop _SET. Could
> > you also sort them from MSB to LSB as for the other registers ? Using
> > BIT() would be good too, although this could be done as an additional
> > patch to convert all the existing macros.
>
> I'll do that. Also I will rename the register to MT9P031_RESTART and the
> bits to MT9P031_FRAME_RESTART and MT9P031_FRAME_PAUSE_RESTART.
>
> >> #define MT9P031_SHUTTER_DELAY 0x0c
> >> #define MT9P031_RST 0x0d
> >> #define MT9P031_RST_ENABLE 1
> >> @@ -483,9 +485,25 @@ 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) {
> >> + val = mt9p031_read(client, MT9P031_FRAME_RESTART);
> >
> > Do you need to read the register ? Can't you write
> > MT9P031_FRAME_PAUSE_RESTART_SET and then MT9P031_FRAME_PAUSE_RESTART_SET
> > | MT9P031_FRAME_RESTART_SET ? And actually, can't we just write both
> > bits in one go, do we need two writes ?
>
> I think you're right we don't necessarily need to read the registers. The
> only other bit is not used by the driver.
>
> But I think we do need two separate writes, at least that is what the
> datasheet states.
>
> So I would drop the read but keep both write, ok?
That's fine with me if required, although I don't see where this is
indicated in the datasheet, but I may have missed it.
> >> +
> >> + /* enable pause restart */
> >> + val |= MT9P031_FRAME_PAUSE_RESTART_SET;
> >> + ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + /* enable restart + keep pause restart set */
> >> + val |= MT9P031_FRAME_RESTART_SET;
> >> + ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> /* Stop sensor readout */
> >> ret = mt9p031_set_output_control(mt9p031,
> >> MT9P031_OUTPUT_CONTROL_CEN, 0);
> >> @@ -505,6 +523,13 @@ static int mt9p031_s_stream(struct v4l2_subdev *subdev, int enable)
> >> if (ret < 0)
> >> return ret;
> >>
> >> + val = mt9p031_read(client, MT9P031_FRAME_RESTART);
> >> + /* disable reset + pause restart */
> >> + val &= ~MT9P031_FRAME_PAUSE_RESTART_SET;
> >
> > Same here, I think you can simply write MT9P031_FRAME_PAUSE_RESTART_SET.
>
> I'll drop the read here as well. But I need to make sure, that the Restart
> Bit is not cleared manually here.
>
> >> + ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> return mt9p031_pll_enable(mt9p031);
> >> }
> >>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 5/5] media: mt9p031: Fix corrupted frame after restarting stream
2020-10-05 13:08 ` Laurent Pinchart
@ 2020-10-14 7:14 ` Stefan Riedmüller
0 siblings, 0 replies; 21+ messages in thread
From: Stefan Riedmüller @ 2020-10-14 7:14 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel,
Dirk Bender
Hi Laurent,
On 05.10.20 15:08, Laurent Pinchart wrote:
> Hi Stefan,
>
> On Mon, Oct 05, 2020 at 11:28:21AM +0200, Stefan Riedmüller wrote:
>> On 02.10.20 02:05, Laurent Pinchart wrote:
>>> On Wed, Sep 30, 2020 at 12:51:33PM +0200, Stefan Riedmueller wrote:
>>>> From: Dirk Bender <d.bender@phytec.de>
>>>>
>>>> To prevent corrupted frames after starting and stopping the sensor it's
>>>
>>> s/it's/its/
>>
>> thanks, I'll fix that.
>>
>>>> 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 <d.bender@phytec.de>
>>>> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
>>>> ---
>>>> No changes in v2
>>>> ---
>>>> drivers/media/i2c/mt9p031.c | 25 +++++++++++++++++++++++++
>>>> 1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
>>>> index d10457361e6c..d59f66e3dcf3 100644
>>>> --- a/drivers/media/i2c/mt9p031.c
>>>> +++ b/drivers/media/i2c/mt9p031.c
>>>> @@ -80,6 +80,8 @@
>>>> #define MT9P031_PIXEL_CLOCK_SHIFT(n) ((n) << 8)
>>>> #define MT9P031_PIXEL_CLOCK_DIVIDE(n) ((n) << 0)
>>>> #define MT9P031_FRAME_RESTART 0x0b
>>>> +#define MT9P031_FRAME_RESTART_SET (1 << 0)
>>>> +#define MT9P031_FRAME_PAUSE_RESTART_SET (1 << 1)
>>>
>>> The fields are named Restart and Pause_Restart, I would drop _SET. Could
>>> you also sort them from MSB to LSB as for the other registers ? Using
>>> BIT() would be good too, although this could be done as an additional
>>> patch to convert all the existing macros.
>>
>> I'll do that. Also I will rename the register to MT9P031_RESTART and the
>> bits to MT9P031_FRAME_RESTART and MT9P031_FRAME_PAUSE_RESTART.
>>
>>>> #define MT9P031_SHUTTER_DELAY 0x0c
>>>> #define MT9P031_RST 0x0d
>>>> #define MT9P031_RST_ENABLE 1
>>>> @@ -483,9 +485,25 @@ 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) {
>>>> + val = mt9p031_read(client, MT9P031_FRAME_RESTART);
>>>
>>> Do you need to read the register ? Can't you write
>>> MT9P031_FRAME_PAUSE_RESTART_SET and then MT9P031_FRAME_PAUSE_RESTART_SET
>>> | MT9P031_FRAME_RESTART_SET ? And actually, can't we just write both
>>> bits in one go, do we need two writes ?
>>
>> I think you're right we don't necessarily need to read the registers. The
>> only other bit is not used by the driver.
>>
>> But I think we do need two separate writes, at least that is what the
>> datasheet states.
>>
>> So I would drop the read but keep both write, ok?
>
> That's fine with me if required, although I don't see where this is
> indicated in the datasheet, but I may have missed it.
It's in "Standby and Chip Enable". There is a Sequence for entering soft
standby with two separate writes:
REG = 0x0B, 0x0002
REG = 0x0B, 0x0003
Regards,
Stefan
>
>>>> +
>>>> + /* enable pause restart */
>>>> + val |= MT9P031_FRAME_PAUSE_RESTART_SET;
>>>> + ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + /* enable restart + keep pause restart set */
>>>> + val |= MT9P031_FRAME_RESTART_SET;
>>>> + ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> /* Stop sensor readout */
>>>> ret = mt9p031_set_output_control(mt9p031,
>>>> MT9P031_OUTPUT_CONTROL_CEN, 0);
>>>> @@ -505,6 +523,13 @@ static int mt9p031_s_stream(struct v4l2_subdev *subdev, int enable)
>>>> if (ret < 0)
>>>> return ret;
>>>>
>>>> + val = mt9p031_read(client, MT9P031_FRAME_RESTART);
>>>> + /* disable reset + pause restart */
>>>> + val &= ~MT9P031_FRAME_PAUSE_RESTART_SET;
>>>
>>> Same here, I think you can simply write MT9P031_FRAME_PAUSE_RESTART_SET.
>>
>> I'll drop the read here as well. But I need to make sure, that the Restart
>> Bit is not cleared manually here.
>>
>>>> + ret = mt9p031_write(client, MT9P031_FRAME_RESTART, val);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> return mt9p031_pll_enable(mt9p031);
>>>> }
>>>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/5] media: mt9p031: Add support for 8 bit and 10 bit formats
2020-09-30 10:51 [PATCH v2 1/5] media: mt9p031: Add support for 8 bit and 10 bit formats Stefan Riedmueller
` (3 preceding siblings ...)
2020-09-30 10:51 ` [PATCH v2 5/5] media: mt9p031: Fix corrupted frame after restarting stream Stefan Riedmueller
@ 2020-09-30 11:42 ` Laurent Pinchart
2020-10-01 9:07 ` Stefan Riedmüller
4 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2020-09-30 11:42 UTC (permalink / raw)
To: Stefan Riedmueller
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel,
Christian Hemp, Jan Luebbe
Hi Stefan,
Thank you for the patch.
On Wed, Sep 30, 2020 at 12:51:29PM +0200, Stefan Riedmueller wrote:
> From: Christian Hemp <c.hemp@phytec.de>
>
> Aside from 12 bit monochrome or color format the sensor implicitly
> supports 10 and 8 bit formats as well by simply dropping the
> corresponding LSBs.
That's not how it should work though. If you set the format on
MEDIA_BUS_FMT_SGRBG8_1X8 through the pipeline for instance, you will end
up capturing the 8 LSB, not the 8 MSB.
What's your use case for this ?
> Signed-off-by: Christian Hemp <c.hemp@phytec.de>
> [jlu@pengutronix.de: simplified by dropping v4l2_colorspace handling]
> Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> ---
> Changes in v2:
> - Use unsigned int for num_fmts and loop variable in find_datafmt
> - Remove superfluous const qualifier from find_datafmt
> ---
> drivers/media/i2c/mt9p031.c | 50 +++++++++++++++++++++++++++++--------
> 1 file changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index dc23b9ed510a..2e6671ef877c 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -116,6 +116,18 @@ enum mt9p031_model {
> MT9P031_MODEL_MONOCHROME,
> };
>
> +static const u32 mt9p031_color_fmts[] = {
> + MEDIA_BUS_FMT_SGRBG8_1X8,
> + MEDIA_BUS_FMT_SGRBG10_1X10,
> + MEDIA_BUS_FMT_SGRBG12_1X12,
> +};
> +
> +static const u32 mt9p031_monochrome_fmts[] = {
> + MEDIA_BUS_FMT_Y8_1X8,
> + MEDIA_BUS_FMT_Y10_1X10,
> + MEDIA_BUS_FMT_Y12_1X12,
> +};
> +
> struct mt9p031 {
> struct v4l2_subdev subdev;
> struct media_pad pad;
> @@ -138,6 +150,9 @@ struct mt9p031 {
> struct v4l2_ctrl *blc_auto;
> struct v4l2_ctrl *blc_offset;
>
> + const u32 *fmts;
> + unsigned int num_fmts;
> +
> /* Registers cache */
> u16 output_control;
> u16 mode2;
> @@ -148,6 +163,17 @@ static struct mt9p031 *to_mt9p031(struct v4l2_subdev *sd)
> return container_of(sd, struct mt9p031, subdev);
> }
>
> +static u32 mt9p031_find_datafmt(struct mt9p031 *mt9p031, u32 code)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < mt9p031->num_fmts; i++)
> + if (mt9p031->fmts[i] == code)
> + return mt9p031->fmts[i];
> +
> + return mt9p031->fmts[mt9p031->num_fmts-1];
> +}
> +
> static int mt9p031_read(struct i2c_client *client, u8 reg)
> {
> return i2c_smbus_read_word_swapped(client, reg);
> @@ -476,10 +502,11 @@ static int mt9p031_enum_mbus_code(struct v4l2_subdev *subdev,
> {
> struct mt9p031 *mt9p031 = to_mt9p031(subdev);
>
> - if (code->pad || code->index)
> + if (code->pad || code->index >= mt9p031->num_fmts)
> return -EINVAL;
>
> - code->code = mt9p031->format.code;
> + code->code = mt9p031->fmts[code->index];
> +
> return 0;
> }
>
> @@ -573,6 +600,8 @@ static int mt9p031_set_format(struct v4l2_subdev *subdev,
> __format->width = __crop->width / hratio;
> __format->height = __crop->height / vratio;
>
> + __format->code = mt9p031_find_datafmt(mt9p031, format->format.code);
> +
> format->format = *__format;
>
> return 0;
> @@ -951,10 +980,7 @@ static int mt9p031_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
>
> format = v4l2_subdev_get_try_format(subdev, fh->pad, 0);
>
> - if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
> - format->code = MEDIA_BUS_FMT_Y12_1X12;
> - else
> - format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
> + format->code = mt9p031_find_datafmt(mt9p031, 0);
>
> format->width = MT9P031_WINDOW_WIDTH_DEF;
> format->height = MT9P031_WINDOW_HEIGHT_DEF;
> @@ -1121,10 +1147,14 @@ static int mt9p031_probe(struct i2c_client *client,
> mt9p031->crop.left = MT9P031_COLUMN_START_DEF;
> mt9p031->crop.top = MT9P031_ROW_START_DEF;
>
> - if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
> - mt9p031->format.code = MEDIA_BUS_FMT_Y12_1X12;
> - else
> - mt9p031->format.code = MEDIA_BUS_FMT_SGRBG12_1X12;
> + if (mt9p031->model == MT9P031_MODEL_MONOCHROME) {
> + mt9p031->fmts = mt9p031_monochrome_fmts;
> + mt9p031->num_fmts = ARRAY_SIZE(mt9p031_monochrome_fmts);
> + } else {
> + mt9p031->fmts = mt9p031_color_fmts;
> + mt9p031->num_fmts = ARRAY_SIZE(mt9p031_color_fmts);
> + }
> + mt9p031->format.code = mt9p031_find_datafmt(mt9p031, 0);
>
> mt9p031->format.width = MT9P031_WINDOW_WIDTH_DEF;
> mt9p031->format.height = MT9P031_WINDOW_HEIGHT_DEF;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 1/5] media: mt9p031: Add support for 8 bit and 10 bit formats
2020-09-30 11:42 ` [PATCH v2 1/5] media: mt9p031: Add support for 8 bit and 10 bit formats Laurent Pinchart
@ 2020-10-01 9:07 ` Stefan Riedmüller
2020-10-01 23:53 ` Laurent Pinchart
0 siblings, 1 reply; 21+ messages in thread
From: Stefan Riedmüller @ 2020-10-01 9:07 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel,
Christian Hemp, Jan Luebbe
Hi Laurent,
On 30.09.20 13:42, Laurent Pinchart wrote:
> Hi Stefan,
>
> Thank you for the patch.
>
> On Wed, Sep 30, 2020 at 12:51:29PM +0200, Stefan Riedmueller wrote:
>> From: Christian Hemp <c.hemp@phytec.de>
>>
>> Aside from 12 bit monochrome or color format the sensor implicitly
>> supports 10 and 8 bit formats as well by simply dropping the
>> corresponding LSBs.
>
> That's not how it should work though. If you set the format on
> MEDIA_BUS_FMT_SGRBG8_1X8 through the pipeline for instance, you will end
> up capturing the 8 LSB, not the 8 MSB.
>
> What's your use case for this ?
I use this sensor in combination with an i.MX 6 and i.MX 6UL. When the
sensor is connected with 12 bit (or 10 bit on the i.MX 6UL) and I set
MEDIA_BUS_FMT_SGRBG8_1X8 through the pipeline the CSI interface drops the
unused 4 LSB (or 2 LSB on the i.MX 6UL) so I get the 8 MSB from my 12 bit
sensor.
Does this clarify things? Maybe the description in the commit message is not
accurate enough or did I get something wrong?
Thanks,
Stefan
>
>> Signed-off-by: Christian Hemp <c.hemp@phytec.de>
>> [jlu@pengutronix.de: simplified by dropping v4l2_colorspace handling]
>> Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
>> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
>> ---
>> Changes in v2:
>> - Use unsigned int for num_fmts and loop variable in find_datafmt
>> - Remove superfluous const qualifier from find_datafmt
>> ---
>> drivers/media/i2c/mt9p031.c | 50 +++++++++++++++++++++++++++++--------
>> 1 file changed, 40 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
>> index dc23b9ed510a..2e6671ef877c 100644
>> --- a/drivers/media/i2c/mt9p031.c
>> +++ b/drivers/media/i2c/mt9p031.c
>> @@ -116,6 +116,18 @@ enum mt9p031_model {
>> MT9P031_MODEL_MONOCHROME,
>> };
>>
>> +static const u32 mt9p031_color_fmts[] = {
>> + MEDIA_BUS_FMT_SGRBG8_1X8,
>> + MEDIA_BUS_FMT_SGRBG10_1X10,
>> + MEDIA_BUS_FMT_SGRBG12_1X12,
>> +};
>> +
>> +static const u32 mt9p031_monochrome_fmts[] = {
>> + MEDIA_BUS_FMT_Y8_1X8,
>> + MEDIA_BUS_FMT_Y10_1X10,
>> + MEDIA_BUS_FMT_Y12_1X12,
>> +};
>> +
>> struct mt9p031 {
>> struct v4l2_subdev subdev;
>> struct media_pad pad;
>> @@ -138,6 +150,9 @@ struct mt9p031 {
>> struct v4l2_ctrl *blc_auto;
>> struct v4l2_ctrl *blc_offset;
>>
>> + const u32 *fmts;
>> + unsigned int num_fmts;
>> +
>> /* Registers cache */
>> u16 output_control;
>> u16 mode2;
>> @@ -148,6 +163,17 @@ static struct mt9p031 *to_mt9p031(struct v4l2_subdev *sd)
>> return container_of(sd, struct mt9p031, subdev);
>> }
>>
>> +static u32 mt9p031_find_datafmt(struct mt9p031 *mt9p031, u32 code)
>> +{
>> + unsigned int i;
>> +
>> + for (i = 0; i < mt9p031->num_fmts; i++)
>> + if (mt9p031->fmts[i] == code)
>> + return mt9p031->fmts[i];
>> +
>> + return mt9p031->fmts[mt9p031->num_fmts-1];
>> +}
>> +
>> static int mt9p031_read(struct i2c_client *client, u8 reg)
>> {
>> return i2c_smbus_read_word_swapped(client, reg);
>> @@ -476,10 +502,11 @@ static int mt9p031_enum_mbus_code(struct v4l2_subdev *subdev,
>> {
>> struct mt9p031 *mt9p031 = to_mt9p031(subdev);
>>
>> - if (code->pad || code->index)
>> + if (code->pad || code->index >= mt9p031->num_fmts)
>> return -EINVAL;
>>
>> - code->code = mt9p031->format.code;
>> + code->code = mt9p031->fmts[code->index];
>> +
>> return 0;
>> }
>>
>> @@ -573,6 +600,8 @@ static int mt9p031_set_format(struct v4l2_subdev *subdev,
>> __format->width = __crop->width / hratio;
>> __format->height = __crop->height / vratio;
>>
>> + __format->code = mt9p031_find_datafmt(mt9p031, format->format.code);
>> +
>> format->format = *__format;
>>
>> return 0;
>> @@ -951,10 +980,7 @@ static int mt9p031_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
>>
>> format = v4l2_subdev_get_try_format(subdev, fh->pad, 0);
>>
>> - if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
>> - format->code = MEDIA_BUS_FMT_Y12_1X12;
>> - else
>> - format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
>> + format->code = mt9p031_find_datafmt(mt9p031, 0);
>>
>> format->width = MT9P031_WINDOW_WIDTH_DEF;
>> format->height = MT9P031_WINDOW_HEIGHT_DEF;
>> @@ -1121,10 +1147,14 @@ static int mt9p031_probe(struct i2c_client *client,
>> mt9p031->crop.left = MT9P031_COLUMN_START_DEF;
>> mt9p031->crop.top = MT9P031_ROW_START_DEF;
>>
>> - if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
>> - mt9p031->format.code = MEDIA_BUS_FMT_Y12_1X12;
>> - else
>> - mt9p031->format.code = MEDIA_BUS_FMT_SGRBG12_1X12;
>> + if (mt9p031->model == MT9P031_MODEL_MONOCHROME) {
>> + mt9p031->fmts = mt9p031_monochrome_fmts;
>> + mt9p031->num_fmts = ARRAY_SIZE(mt9p031_monochrome_fmts);
>> + } else {
>> + mt9p031->fmts = mt9p031_color_fmts;
>> + mt9p031->num_fmts = ARRAY_SIZE(mt9p031_color_fmts);
>> + }
>> + mt9p031->format.code = mt9p031_find_datafmt(mt9p031, 0);
>>
>> mt9p031->format.width = MT9P031_WINDOW_WIDTH_DEF;
>> mt9p031->format.height = MT9P031_WINDOW_HEIGHT_DEF;
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 1/5] media: mt9p031: Add support for 8 bit and 10 bit formats
2020-10-01 9:07 ` Stefan Riedmüller
@ 2020-10-01 23:53 ` Laurent Pinchart
2020-10-05 9:31 ` Stefan Riedmüller
0 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2020-10-01 23:53 UTC (permalink / raw)
To: Stefan Riedmüller
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel,
Christian Hemp, Jan Luebbe
Hi Stefan,
On Thu, Oct 01, 2020 at 11:07:00AM +0200, Stefan Riedmüller wrote:
> On 30.09.20 13:42, Laurent Pinchart wrote:
> > On Wed, Sep 30, 2020 at 12:51:29PM +0200, Stefan Riedmueller wrote:
> >> From: Christian Hemp <c.hemp@phytec.de>
> >>
> >> Aside from 12 bit monochrome or color format the sensor implicitly
> >> supports 10 and 8 bit formats as well by simply dropping the
> >> corresponding LSBs.
> >
> > That's not how it should work though. If you set the format on
> > MEDIA_BUS_FMT_SGRBG8_1X8 through the pipeline for instance, you will end
> > up capturing the 8 LSB, not the 8 MSB.
> >
> > What's your use case for this ?
>
> I use this sensor in combination with an i.MX 6 and i.MX 6UL. When the
> sensor is connected with 12 bit (or 10 bit on the i.MX 6UL) and I set
> MEDIA_BUS_FMT_SGRBG8_1X8 through the pipeline the CSI interface drops the
> unused 4 LSB (or 2 LSB on the i.MX 6UL) so I get the 8 MSB from my 12 bit
> sensor.
Is that the PIXEL_BIT bit in CSI_CSICR1 for the i.MX6UL ? If so I think
this should be handled in the imx7-media-csi driver. You could set the
format to MEDIA_BUS_FMT_SGRBG10_1X10 on the sink pad of the CSI and to
MEDIA_BUS_FMT_SGRBG8_1X8 on the source pad to configure this. I don't
think the sensor driver should be involved, otherwise we'd have to patch
all sensor drivers. From a sensor point of view, it outputs 12-bit
Bayer, not 8-bit.
Now there's a caveat. When used with the i.MX6UL, I assume you connected
D[11:2] of the sensor to D[9:0] of the i.MX6UL, right ? The i.MX6UL
doesn't support 12-bit inputs, so it should accept
MEDIA_BUS_FMT_SGRBG12_1X12 on its sink pad. In this case, as D[1:0] of
the sensor are left unconnected, I think you should set data-shift to 2
and bus-width to 10 in DT on the sensor side. The MT9P031 driver should
parse that, and output MEDIA_BUS_FMT_SGRBG10_1X10 instead of
MEDIA_BUS_FMT_SGRBG12_1X12 in that case.
> Does this clarify things? Maybe the description in the commit message is not
> accurate enough or did I get something wrong?
>
> >> Signed-off-by: Christian Hemp <c.hemp@phytec.de>
> >> [jlu@pengutronix.de: simplified by dropping v4l2_colorspace handling]
> >> Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
> >> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> >> ---
> >> Changes in v2:
> >> - Use unsigned int for num_fmts and loop variable in find_datafmt
> >> - Remove superfluous const qualifier from find_datafmt
> >> ---
> >> drivers/media/i2c/mt9p031.c | 50 +++++++++++++++++++++++++++++--------
> >> 1 file changed, 40 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> >> index dc23b9ed510a..2e6671ef877c 100644
> >> --- a/drivers/media/i2c/mt9p031.c
> >> +++ b/drivers/media/i2c/mt9p031.c
> >> @@ -116,6 +116,18 @@ enum mt9p031_model {
> >> MT9P031_MODEL_MONOCHROME,
> >> };
> >>
> >> +static const u32 mt9p031_color_fmts[] = {
> >> + MEDIA_BUS_FMT_SGRBG8_1X8,
> >> + MEDIA_BUS_FMT_SGRBG10_1X10,
> >> + MEDIA_BUS_FMT_SGRBG12_1X12,
> >> +};
> >> +
> >> +static const u32 mt9p031_monochrome_fmts[] = {
> >> + MEDIA_BUS_FMT_Y8_1X8,
> >> + MEDIA_BUS_FMT_Y10_1X10,
> >> + MEDIA_BUS_FMT_Y12_1X12,
> >> +};
> >> +
> >> struct mt9p031 {
> >> struct v4l2_subdev subdev;
> >> struct media_pad pad;
> >> @@ -138,6 +150,9 @@ struct mt9p031 {
> >> struct v4l2_ctrl *blc_auto;
> >> struct v4l2_ctrl *blc_offset;
> >>
> >> + const u32 *fmts;
> >> + unsigned int num_fmts;
> >> +
> >> /* Registers cache */
> >> u16 output_control;
> >> u16 mode2;
> >> @@ -148,6 +163,17 @@ static struct mt9p031 *to_mt9p031(struct v4l2_subdev *sd)
> >> return container_of(sd, struct mt9p031, subdev);
> >> }
> >>
> >> +static u32 mt9p031_find_datafmt(struct mt9p031 *mt9p031, u32 code)
> >> +{
> >> + unsigned int i;
> >> +
> >> + for (i = 0; i < mt9p031->num_fmts; i++)
> >> + if (mt9p031->fmts[i] == code)
> >> + return mt9p031->fmts[i];
> >> +
> >> + return mt9p031->fmts[mt9p031->num_fmts-1];
> >> +}
> >> +
> >> static int mt9p031_read(struct i2c_client *client, u8 reg)
> >> {
> >> return i2c_smbus_read_word_swapped(client, reg);
> >> @@ -476,10 +502,11 @@ static int mt9p031_enum_mbus_code(struct v4l2_subdev *subdev,
> >> {
> >> struct mt9p031 *mt9p031 = to_mt9p031(subdev);
> >>
> >> - if (code->pad || code->index)
> >> + if (code->pad || code->index >= mt9p031->num_fmts)
> >> return -EINVAL;
> >>
> >> - code->code = mt9p031->format.code;
> >> + code->code = mt9p031->fmts[code->index];
> >> +
> >> return 0;
> >> }
> >>
> >> @@ -573,6 +600,8 @@ static int mt9p031_set_format(struct v4l2_subdev *subdev,
> >> __format->width = __crop->width / hratio;
> >> __format->height = __crop->height / vratio;
> >>
> >> + __format->code = mt9p031_find_datafmt(mt9p031, format->format.code);
> >> +
> >> format->format = *__format;
> >>
> >> return 0;
> >> @@ -951,10 +980,7 @@ static int mt9p031_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
> >>
> >> format = v4l2_subdev_get_try_format(subdev, fh->pad, 0);
> >>
> >> - if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
> >> - format->code = MEDIA_BUS_FMT_Y12_1X12;
> >> - else
> >> - format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
> >> + format->code = mt9p031_find_datafmt(mt9p031, 0);
> >>
> >> format->width = MT9P031_WINDOW_WIDTH_DEF;
> >> format->height = MT9P031_WINDOW_HEIGHT_DEF;
> >> @@ -1121,10 +1147,14 @@ static int mt9p031_probe(struct i2c_client *client,
> >> mt9p031->crop.left = MT9P031_COLUMN_START_DEF;
> >> mt9p031->crop.top = MT9P031_ROW_START_DEF;
> >>
> >> - if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
> >> - mt9p031->format.code = MEDIA_BUS_FMT_Y12_1X12;
> >> - else
> >> - mt9p031->format.code = MEDIA_BUS_FMT_SGRBG12_1X12;
> >> + if (mt9p031->model == MT9P031_MODEL_MONOCHROME) {
> >> + mt9p031->fmts = mt9p031_monochrome_fmts;
> >> + mt9p031->num_fmts = ARRAY_SIZE(mt9p031_monochrome_fmts);
> >> + } else {
> >> + mt9p031->fmts = mt9p031_color_fmts;
> >> + mt9p031->num_fmts = ARRAY_SIZE(mt9p031_color_fmts);
> >> + }
> >> + mt9p031->format.code = mt9p031_find_datafmt(mt9p031, 0);
> >>
> >> mt9p031->format.width = MT9P031_WINDOW_WIDTH_DEF;
> >> mt9p031->format.height = MT9P031_WINDOW_HEIGHT_DEF;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 1/5] media: mt9p031: Add support for 8 bit and 10 bit formats
2020-10-01 23:53 ` Laurent Pinchart
@ 2020-10-05 9:31 ` Stefan Riedmüller
0 siblings, 0 replies; 21+ messages in thread
From: Stefan Riedmüller @ 2020-10-05 9:31 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, linux-kernel,
Christian Hemp, Jan Luebbe
Hi Laurent,
On 02.10.20 01:53, Laurent Pinchart wrote:
> Hi Stefan,
>
> On Thu, Oct 01, 2020 at 11:07:00AM +0200, Stefan Riedmüller wrote:
>> On 30.09.20 13:42, Laurent Pinchart wrote:
>>> On Wed, Sep 30, 2020 at 12:51:29PM +0200, Stefan Riedmueller wrote:
>>>> From: Christian Hemp <c.hemp@phytec.de>
>>>>
>>>> Aside from 12 bit monochrome or color format the sensor implicitly
>>>> supports 10 and 8 bit formats as well by simply dropping the
>>>> corresponding LSBs.
>>>
>>> That's not how it should work though. If you set the format on
>>> MEDIA_BUS_FMT_SGRBG8_1X8 through the pipeline for instance, you will end
>>> up capturing the 8 LSB, not the 8 MSB.
>>>
>>> What's your use case for this ?
>>
>> I use this sensor in combination with an i.MX 6 and i.MX 6UL. When the
>> sensor is connected with 12 bit (or 10 bit on the i.MX 6UL) and I set
>> MEDIA_BUS_FMT_SGRBG8_1X8 through the pipeline the CSI interface drops the
>> unused 4 LSB (or 2 LSB on the i.MX 6UL) so I get the 8 MSB from my 12 bit
>> sensor.
>
> Is that the PIXEL_BIT bit in CSI_CSICR1 for the i.MX6UL ? If so I think
> this should be handled in the imx7-media-csi driver. You could set the
> format to MEDIA_BUS_FMT_SGRBG10_1X10 on the sink pad of the CSI and to
> MEDIA_BUS_FMT_SGRBG8_1X8 on the source pad to configure this. I don't
> think the sensor driver should be involved, otherwise we'd have to patch
> all sensor drivers. From a sensor point of view, it outputs 12-bit
> Bayer, not 8-bit.
Ah, I had it wrong. What you say makes total sense. I will take another look
at it and also try to work your suggestion from below in.
Thanks,
Stefan
>
> Now there's a caveat. When used with the i.MX6UL, I assume you connected
> D[11:2] of the sensor to D[9:0] of the i.MX6UL, right ? The i.MX6UL
> doesn't support 12-bit inputs, so it should accept
> MEDIA_BUS_FMT_SGRBG12_1X12 on its sink pad. In this case, as D[1:0] of
> the sensor are left unconnected, I think you should set data-shift to 2
> and bus-width to 10 in DT on the sensor side. The MT9P031 driver should
> parse that, and output MEDIA_BUS_FMT_SGRBG10_1X10 instead of
> MEDIA_BUS_FMT_SGRBG12_1X12 in that case.
>
>> Does this clarify things? Maybe the description in the commit message is not
>> accurate enough or did I get something wrong?
>>
>>>> Signed-off-by: Christian Hemp <c.hemp@phytec.de>
>>>> [jlu@pengutronix.de: simplified by dropping v4l2_colorspace handling]
>>>> Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
>>>> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
>>>> ---
>>>> Changes in v2:
>>>> - Use unsigned int for num_fmts and loop variable in find_datafmt
>>>> - Remove superfluous const qualifier from find_datafmt
>>>> ---
>>>> drivers/media/i2c/mt9p031.c | 50 +++++++++++++++++++++++++++++--------
>>>> 1 file changed, 40 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
>>>> index dc23b9ed510a..2e6671ef877c 100644
>>>> --- a/drivers/media/i2c/mt9p031.c
>>>> +++ b/drivers/media/i2c/mt9p031.c
>>>> @@ -116,6 +116,18 @@ enum mt9p031_model {
>>>> MT9P031_MODEL_MONOCHROME,
>>>> };
>>>>
>>>> +static const u32 mt9p031_color_fmts[] = {
>>>> + MEDIA_BUS_FMT_SGRBG8_1X8,
>>>> + MEDIA_BUS_FMT_SGRBG10_1X10,
>>>> + MEDIA_BUS_FMT_SGRBG12_1X12,
>>>> +};
>>>> +
>>>> +static const u32 mt9p031_monochrome_fmts[] = {
>>>> + MEDIA_BUS_FMT_Y8_1X8,
>>>> + MEDIA_BUS_FMT_Y10_1X10,
>>>> + MEDIA_BUS_FMT_Y12_1X12,
>>>> +};
>>>> +
>>>> struct mt9p031 {
>>>> struct v4l2_subdev subdev;
>>>> struct media_pad pad;
>>>> @@ -138,6 +150,9 @@ struct mt9p031 {
>>>> struct v4l2_ctrl *blc_auto;
>>>> struct v4l2_ctrl *blc_offset;
>>>>
>>>> + const u32 *fmts;
>>>> + unsigned int num_fmts;
>>>> +
>>>> /* Registers cache */
>>>> u16 output_control;
>>>> u16 mode2;
>>>> @@ -148,6 +163,17 @@ static struct mt9p031 *to_mt9p031(struct v4l2_subdev *sd)
>>>> return container_of(sd, struct mt9p031, subdev);
>>>> }
>>>>
>>>> +static u32 mt9p031_find_datafmt(struct mt9p031 *mt9p031, u32 code)
>>>> +{
>>>> + unsigned int i;
>>>> +
>>>> + for (i = 0; i < mt9p031->num_fmts; i++)
>>>> + if (mt9p031->fmts[i] == code)
>>>> + return mt9p031->fmts[i];
>>>> +
>>>> + return mt9p031->fmts[mt9p031->num_fmts-1];
>>>> +}
>>>> +
>>>> static int mt9p031_read(struct i2c_client *client, u8 reg)
>>>> {
>>>> return i2c_smbus_read_word_swapped(client, reg);
>>>> @@ -476,10 +502,11 @@ static int mt9p031_enum_mbus_code(struct v4l2_subdev *subdev,
>>>> {
>>>> struct mt9p031 *mt9p031 = to_mt9p031(subdev);
>>>>
>>>> - if (code->pad || code->index)
>>>> + if (code->pad || code->index >= mt9p031->num_fmts)
>>>> return -EINVAL;
>>>>
>>>> - code->code = mt9p031->format.code;
>>>> + code->code = mt9p031->fmts[code->index];
>>>> +
>>>> return 0;
>>>> }
>>>>
>>>> @@ -573,6 +600,8 @@ static int mt9p031_set_format(struct v4l2_subdev *subdev,
>>>> __format->width = __crop->width / hratio;
>>>> __format->height = __crop->height / vratio;
>>>>
>>>> + __format->code = mt9p031_find_datafmt(mt9p031, format->format.code);
>>>> +
>>>> format->format = *__format;
>>>>
>>>> return 0;
>>>> @@ -951,10 +980,7 @@ static int mt9p031_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
>>>>
>>>> format = v4l2_subdev_get_try_format(subdev, fh->pad, 0);
>>>>
>>>> - if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
>>>> - format->code = MEDIA_BUS_FMT_Y12_1X12;
>>>> - else
>>>> - format->code = MEDIA_BUS_FMT_SGRBG12_1X12;
>>>> + format->code = mt9p031_find_datafmt(mt9p031, 0);
>>>>
>>>> format->width = MT9P031_WINDOW_WIDTH_DEF;
>>>> format->height = MT9P031_WINDOW_HEIGHT_DEF;
>>>> @@ -1121,10 +1147,14 @@ static int mt9p031_probe(struct i2c_client *client,
>>>> mt9p031->crop.left = MT9P031_COLUMN_START_DEF;
>>>> mt9p031->crop.top = MT9P031_ROW_START_DEF;
>>>>
>>>> - if (mt9p031->model == MT9P031_MODEL_MONOCHROME)
>>>> - mt9p031->format.code = MEDIA_BUS_FMT_Y12_1X12;
>>>> - else
>>>> - mt9p031->format.code = MEDIA_BUS_FMT_SGRBG12_1X12;
>>>> + if (mt9p031->model == MT9P031_MODEL_MONOCHROME) {
>>>> + mt9p031->fmts = mt9p031_monochrome_fmts;
>>>> + mt9p031->num_fmts = ARRAY_SIZE(mt9p031_monochrome_fmts);
>>>> + } else {
>>>> + mt9p031->fmts = mt9p031_color_fmts;
>>>> + mt9p031->num_fmts = ARRAY_SIZE(mt9p031_color_fmts);
>>>> + }
>>>> + mt9p031->format.code = mt9p031_find_datafmt(mt9p031, 0);
>>>>
>>>> mt9p031->format.width = MT9P031_WINDOW_WIDTH_DEF;
>>>> mt9p031->format.height = MT9P031_WINDOW_HEIGHT_DEF;
>
^ permalink raw reply [flat|nested] 21+ messages in thread