[PATCH v2 0/2] SimpleDRM: allow configuring physical width and height

[PATCH v2 0/2] SimpleDRM: allow configuring physical width and height

[Rayyan Ansari]

Hello,

The following patches:
- Add support for configuring the width-mm and height-mm DRM mode
  properties in the SimpleDRM driver via Device Tree
- Document these two new Device Tree properties

This is useful for allowing interfaces such as Phosh to calculate       
proper scaling values.

Changes since RFC:
- Switch to using 32-bit DT property
- Report errors for return values of of_property_read_u32 except -EINVAL
- Calculate default value during probe
- Add documentation

Rayyan Ansari (2):
  drm/simpledrm: Allow physical width and height configuration via DT
  dt-bindings: display: simple-framebuffer: Document physical width and
    height properties

 .../bindings/display/simple-framebuffer.yaml  |  8 +++
 drivers/gpu/drm/tiny/simpledrm.c              | 60 ++++++++++++++++---
 2 files changed, 59 insertions(+), 9 deletions(-)

-- 
2.39.0

[PATCH v2 1/2] drm/simpledrm: Allow physical width and height configuration via DT

[Rayyan Ansari]

Signed-off-by: Rayyan Ansari <rayyan@ansari.sh>
---
 drivers/gpu/drm/tiny/simpledrm.c | 60 +++++++++++++++++++++++++++-----
 1 file changed, 51 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 162eb44dcba8..7aab7fa572f0 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -128,6 +128,23 @@ simplefb_read_u32_of(struct drm_device *dev, struct device_node *of_node,
 	return ret;
 }
 
+static int
+simplefb_read_u32_of_opt(struct drm_device *dev, struct device_node *of_node,
+			 const char *name, u32 *value)
+{
+	int ret = of_property_read_u32(of_node, name, value);
+
+	if (ret == -EINVAL) {
+		*value = 0;
+		ret = 0;
+	} else if (ret) {
+		drm_err(dev, "simplefb: cannot parse framebuffer %s: error %d\n",
+			name, ret);
+	}
+
+	return ret;
+}
+
 static int
 simplefb_read_string_of(struct drm_device *dev, struct device_node *of_node,
 			const char *name, const char **value)
@@ -184,6 +201,19 @@ simplefb_get_format_of(struct drm_device *dev, struct device_node *of_node)
 	return simplefb_get_validated_format(dev, format);
 }
 
+static int
+simplefb_get_mm_of(struct drm_device *dev, struct device_node *of_node,
+		   const char *name)
+{
+	int mm;
+	int ret = simplefb_read_u32_of_opt(dev, of_node, name, &mm);
+
+	if (ret)
+		return ret;
+	return simplefb_get_validated_int(dev, name, mm);
+}
+
+
 /*
  * Simple Framebuffer device
  */
@@ -599,16 +629,12 @@ static const struct drm_mode_config_funcs simpledrm_mode_config_funcs = {
  */
 
 static struct drm_display_mode simpledrm_mode(unsigned int width,
-					      unsigned int height)
+					      unsigned int height,
+					      unsigned int width_mm,
+					      unsigned int height_mm)
 {
-	/*
-	 * Assume a monitor resolution of 96 dpi to
-	 * get a somewhat reasonable screen size.
-	 */
 	const struct drm_display_mode mode = {
-		DRM_MODE_INIT(60, width, height,
-			      DRM_MODE_RES_MM(width, 96ul),
-			      DRM_MODE_RES_MM(height, 96ul))
+		DRM_MODE_INIT(60, width, height, width_mm, height_mm)
 	};
 
 	return mode;
@@ -622,6 +648,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	struct simpledrm_device *sdev;
 	struct drm_device *dev;
 	int width, height, stride;
+	int width_mm = 0, height_mm = 0;
 	const struct drm_format_info *format;
 	struct resource *res, *mem;
 	void __iomem *screen_base;
@@ -676,6 +703,12 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 		format = simplefb_get_format_of(dev, of_node);
 		if (IS_ERR(format))
 			return ERR_CAST(format);
+		width_mm = simplefb_get_mm_of(dev, of_node, "width-mm");
+		if (width_mm < 0)
+			return ERR_PTR(width_mm);
+		height_mm = simplefb_get_mm_of(dev, of_node, "height-mm");
+		if (height_mm < 0)
+			return ERR_PTR(height_mm);
 	} else {
 		drm_err(dev, "no simplefb configuration found\n");
 		return ERR_PTR(-ENODEV);
@@ -686,7 +719,16 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 			return ERR_PTR(-EINVAL);
 	}
 
-	sdev->mode = simpledrm_mode(width, height);
+	/*
+	 * Assume a monitor resolution of 96 dpi if physical dimensions
+	 * are not specified to get a somewhat reasonable screen size.
+	 */
+	if (!width_mm)
+		width_mm = DRM_MODE_RES_MM(width, 96ul);
+	if (!height_mm)
+		height_mm = DRM_MODE_RES_MM(height, 96ul);
+
+	sdev->mode = simpledrm_mode(width, height, width_mm, height_mm);
 	sdev->format = format;
 	sdev->pitch = stride;
 
-- 
2.39.0

Re: [PATCH v2 1/2] drm/simpledrm: Allow physical width and height configuration via DT

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4408 bytes --]

Hi,

I just want to comment that the code in this patch is fine, but I'm not 
going to merge it until the other discussion about using the panel's DT 
nodes has been resolved. IMHO, the panel-based solution seems preferable 
to the new properties.

Best regards
Thomas

Am 21.01.23 um 16:35 schrieb Rayyan Ansari:
> Signed-off-by: Rayyan Ansari <rayyan@ansari.sh>
> ---
>   drivers/gpu/drm/tiny/simpledrm.c | 60 +++++++++++++++++++++++++++-----
>   1 file changed, 51 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 162eb44dcba8..7aab7fa572f0 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -128,6 +128,23 @@ simplefb_read_u32_of(struct drm_device *dev, struct device_node *of_node,
>   	return ret;
>   }
>   
> +static int
> +simplefb_read_u32_of_opt(struct drm_device *dev, struct device_node *of_node,
> +			 const char *name, u32 *value)
> +{
> +	int ret = of_property_read_u32(of_node, name, value);
> +
> +	if (ret == -EINVAL) {
> +		*value = 0;
> +		ret = 0;
> +	} else if (ret) {
> +		drm_err(dev, "simplefb: cannot parse framebuffer %s: error %d\n",
> +			name, ret);
> +	}
> +
> +	return ret;
> +}
> +
>   static int
>   simplefb_read_string_of(struct drm_device *dev, struct device_node *of_node,
>   			const char *name, const char **value)
> @@ -184,6 +201,19 @@ simplefb_get_format_of(struct drm_device *dev, struct device_node *of_node)
>   	return simplefb_get_validated_format(dev, format);
>   }
>   
> +static int
> +simplefb_get_mm_of(struct drm_device *dev, struct device_node *of_node,
> +		   const char *name)
> +{
> +	int mm;
> +	int ret = simplefb_read_u32_of_opt(dev, of_node, name, &mm);
> +
> +	if (ret)
> +		return ret;
> +	return simplefb_get_validated_int(dev, name, mm);
> +}
> +
> +
>   /*
>    * Simple Framebuffer device
>    */
> @@ -599,16 +629,12 @@ static const struct drm_mode_config_funcs simpledrm_mode_config_funcs = {
>    */
>   
>   static struct drm_display_mode simpledrm_mode(unsigned int width,
> -					      unsigned int height)
> +					      unsigned int height,
> +					      unsigned int width_mm,
> +					      unsigned int height_mm)
>   {
> -	/*
> -	 * Assume a monitor resolution of 96 dpi to
> -	 * get a somewhat reasonable screen size.
> -	 */
>   	const struct drm_display_mode mode = {
> -		DRM_MODE_INIT(60, width, height,
> -			      DRM_MODE_RES_MM(width, 96ul),
> -			      DRM_MODE_RES_MM(height, 96ul))
> +		DRM_MODE_INIT(60, width, height, width_mm, height_mm)
>   	};
>   
>   	return mode;
> @@ -622,6 +648,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>   	struct simpledrm_device *sdev;
>   	struct drm_device *dev;
>   	int width, height, stride;
> +	int width_mm = 0, height_mm = 0;
>   	const struct drm_format_info *format;
>   	struct resource *res, *mem;
>   	void __iomem *screen_base;
> @@ -676,6 +703,12 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>   		format = simplefb_get_format_of(dev, of_node);
>   		if (IS_ERR(format))
>   			return ERR_CAST(format);
> +		width_mm = simplefb_get_mm_of(dev, of_node, "width-mm");
> +		if (width_mm < 0)
> +			return ERR_PTR(width_mm);
> +		height_mm = simplefb_get_mm_of(dev, of_node, "height-mm");
> +		if (height_mm < 0)
> +			return ERR_PTR(height_mm);
>   	} else {
>   		drm_err(dev, "no simplefb configuration found\n");
>   		return ERR_PTR(-ENODEV);
> @@ -686,7 +719,16 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>   			return ERR_PTR(-EINVAL);
>   	}
>   
> -	sdev->mode = simpledrm_mode(width, height);
> +	/*
> +	 * Assume a monitor resolution of 96 dpi if physical dimensions
> +	 * are not specified to get a somewhat reasonable screen size.
> +	 */
> +	if (!width_mm)
> +		width_mm = DRM_MODE_RES_MM(width, 96ul);
> +	if (!height_mm)
> +		height_mm = DRM_MODE_RES_MM(height, 96ul);
> +
> +	sdev->mode = simpledrm_mode(width, height, width_mm, height_mm);
>   	sdev->format = format;
>   	sdev->pitch = stride;
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[PATCH v2 2/2] dt-bindings: display: simple-framebuffer: Document physical width and height properties

[Rayyan Ansari]

Signed-off-by: Rayyan Ansari <rayyan@ansari.sh>
---
 .../devicetree/bindings/display/simple-framebuffer.yaml   | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
index dd64f70b5014..eb33bfd805db 100644
--- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
+++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
@@ -106,6 +106,14 @@ properties:
       - x2r10g10b10
       - x8r8g8b8
 
+  width-mm:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Physical width of the display in millimetres
+
+  height-mm:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Physical height of the display in millimetres
+
   display:
     $ref: /schemas/types.yaml#/definitions/phandle
     description: Primary display hardware node
-- 
2.39.0

Re: [PATCH v2 2/2] dt-bindings: display: simple-framebuffer: Document physical width and height properties

[Rob Herring]

On Sat, 21 Jan 2023 15:35:44 +0000, Rayyan Ansari wrote:
> Signed-off-by: Rayyan Ansari <rayyan@ansari.sh>
> ---
>  .../devicetree/bindings/display/simple-framebuffer.yaml   | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/display/simple-framebuffer.yaml: properties:width-mm: '$ref' should not be valid under {'const': '$ref'}
	hint: Standard unit suffix properties don't need a type $ref
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/display/simple-framebuffer.yaml: properties:height-mm: '$ref' should not be valid under {'const': '$ref'}
	hint: Standard unit suffix properties don't need a type $ref
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230121153544.467126-3-rayyan@ansari.sh

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 2/2] dt-bindings: display: simple-framebuffer: Document physical width and height properties

[Rayyan Ansari]

On 22/01/2023 15:31, Rob Herring wrote:
> 
> On Sat, 21 Jan 2023 15:35:44 +0000, Rayyan Ansari wrote:
>> Signed-off-by: Rayyan Ansari <rayyan@ansari.sh>
>> ---
>>   .../devicetree/bindings/display/simple-framebuffer.yaml   | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/display/simple-framebuffer.yaml: properties:width-mm: '$ref' should not be valid under {'const': '$ref'}
> 	hint: Standard unit suffix properties don't need a type $ref
> 	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/display/simple-framebuffer.yaml: properties:height-mm: '$ref' should not be valid under {'const': '$ref'}
> 	hint: Standard unit suffix properties don't need a type $ref
> 	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230121153544.467126-3-rayyan@ansari.sh
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
> 

I will remove the $ref property in v2, but I will also wait if there is 
any other feedback to address.

-- 
Rayyan Ansari
https://ansari.sh

Re: [PATCH v2 2/2] dt-bindings: display: simple-framebuffer: Document physical width and height properties

[Rob Herring]

On Sat, Jan 21, 2023 at 9:36 AM Rayyan Ansari <rayyan@ansari.sh> wrote:
>

Why do you need this change?

The 'simple-framebuffer' contains data on how the bootloader
configured the display. The bootloader doesn't configure the display
size, so this information doesn't belong here. The information should
already be in the panel node, so also no point in duplicating it here.

> Signed-off-by: Rayyan Ansari <rayyan@ansari.sh>
> ---
>  .../devicetree/bindings/display/simple-framebuffer.yaml   | 8 ++++++++
>  1 file changed, 8 insertions(+)

Re: [PATCH v2 2/2] dt-bindings: display: simple-framebuffer: Document physical width and height properties

[Hans de Goede]

Hi,

On 1/22/23 16:36, Rob Herring wrote:
> On Sat, Jan 21, 2023 at 9:36 AM Rayyan Ansari <rayyan@ansari.sh> wrote:
>>
> 
> Why do you need this change?
> 
> The 'simple-framebuffer' contains data on how the bootloader
> configured the display. The bootloader doesn't configure the display
> size, so this information doesn't belong here. The information should
> already be in the panel node, so also no point in duplicating it here.

The idea is that early boot code which uses the simplefb node (no more
complex display driver loaded yet) knows the panel's DPI so that it can
decide if hi-dpi rendering / scaling is necessary or not.

This definitely is a useful feature to have.

I guess that for dt systems an alternative approach could be to
add a link to the panel node to the simplefb dt-node.

Regards,

Hans

Re: [PATCH v2 2/2] dt-bindings: display: simple-framebuffer: Document physical width and height properties

[Rayyan Ansari]

On 22/01/2023 15:36, Rob Herring wrote:
> On Sat, Jan 21, 2023 at 9:36 AM Rayyan Ansari <rayyan@ansari.sh> wrote:
>>
> 
> Why do you need this change?
> 
> The 'simple-framebuffer' contains data on how the bootloader
> configured the display. The bootloader doesn't configure the display
> size, so this information doesn't belong here. The information should
> already be in the panel node, so also no point in duplicating it here.
> 
>> Signed-off-by: Rayyan Ansari <rayyan@ansari.sh>
>> ---
>>   .../devicetree/bindings/display/simple-framebuffer.yaml   | 8 ++++++++
>>   1 file changed, 8 insertions(+)

Hi Rob,

There is the usecase that Hans has mentioned, but I have also mentioned 
another usecase previously.

Adding the width-mm and height-mm properties allows user interfaces such 
as Phosh (https://puri.sm/posts/phosh-overview/) to scale correctly to 
the screen. In my case, a panel node is not available and the 
aforementioned interface is in fact running on the SimpleDRM driver 
(which binds to the simple-framebuffer device).

Here is the device I have tested this patch on, the Lumia 735 phone: 
https://wiki.postmarketos.org/images/c/c3/Lumia_735_Phosh.png
Without this patch, this would appear quite small on the screen.

See https://patchwork.freedesktop.org/patch/519107/?series=113053&rev=1 
for some background info about this patch.

Regards,
-- 
Rayyan Ansari
https://ansari.sh

Re: [PATCH v2 2/2] dt-bindings: display: simple-framebuffer: Document physical width and height properties

[Rob Herring]

On Sun, Jan 22, 2023 at 05:25:38PM +0000, Rayyan Ansari wrote:
> On 22/01/2023 15:36, Rob Herring wrote:
> > On Sat, Jan 21, 2023 at 9:36 AM Rayyan Ansari <rayyan@ansari.sh> wrote:
> > > 
> > 
> > Why do you need this change?
> > 
> > The 'simple-framebuffer' contains data on how the bootloader
> > configured the display. The bootloader doesn't configure the display
> > size, so this information doesn't belong here. The information should
> > already be in the panel node, so also no point in duplicating it here.
> > 
> > > Signed-off-by: Rayyan Ansari <rayyan@ansari.sh>
> > > ---
> > >   .../devicetree/bindings/display/simple-framebuffer.yaml   | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> 
> Hi Rob,
> 
> There is the usecase that Hans has mentioned, but I have also mentioned
> another usecase previously.
> 
> Adding the width-mm and height-mm properties allows user interfaces such as
> Phosh (https://puri.sm/posts/phosh-overview/) to scale correctly to the
> screen. In my case, a panel node is not available and the aforementioned
> interface is in fact running on the SimpleDRM driver (which binds to the
> simple-framebuffer device).

Why is the panel node not available? Why not add it? Presumably it is 
not there because you aren't (yet) using the simple-panel driver (and 
others that would need). But presumably you will eventually as I'd 
imagine turning the screen off and back on might be a desired feature.

So why add a temporary DT property that's tied to your *current* kernel? 
The DT should not be tightly coupled to the kernel.

Rob

Re: [PATCH v2 2/2] dt-bindings: display: simple-framebuffer: Document physical width and height properties

[Rayyan Ansari]

On 23/01/2023 17:53, Rob Herring wrote:
> On Sun, Jan 22, 2023 at 05:25:38PM +0000, Rayyan Ansari wrote:
>> On 22/01/2023 15:36, Rob Herring wrote:
>>> On Sat, Jan 21, 2023 at 9:36 AM Rayyan Ansari <rayyan@ansari.sh> wrote:
>>>>
>>>
>>> Why do you need this change?
>>>
>>> The 'simple-framebuffer' contains data on how the bootloader
>>> configured the display. The bootloader doesn't configure the display
>>> size, so this information doesn't belong here. The information should
>>> already be in the panel node, so also no point in duplicating it here.
>>>
>>>> Signed-off-by: Rayyan Ansari <rayyan@ansari.sh>
>>>> ---
>>>>    .../devicetree/bindings/display/simple-framebuffer.yaml   | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>
>> Hi Rob,
>>
>> There is the usecase that Hans has mentioned, but I have also mentioned
>> another usecase previously.
>>
>> Adding the width-mm and height-mm properties allows user interfaces such as
>> Phosh (https://puri.sm/posts/phosh-overview/) to scale correctly to the
>> screen. In my case, a panel node is not available and the aforementioned
>> interface is in fact running on the SimpleDRM driver (which binds to the
>> simple-framebuffer device).
> 
> Why is the panel node not available? Why not add it? Presumably it is
> not there because you aren't (yet) using the simple-panel driver (and
> others that would need). But presumably you will eventually as I'd
> imagine turning the screen off and back on might be a desired feature.

It requires more than using the simple-panel driver: first the SoC side 
display hardware needs to be brought up, then a panel driver that 
implements the proper DCS initialisation sequence needs to be written 
(which is currently not fully known).

> 
> So why add a temporary DT property that's tied to your *current* kernel? > The DT should not be tightly coupled to the kernel.

I'm not sure what you mean by it being "tightly coupled" to the kernel.

> 
> Rob

-- 
Rayyan Ansari
https://ansari.sh

Re: [PATCH v2 2/2] dt-bindings: display: simple-framebuffer: Document physical width and height properties

[Mark Kettenis]

> Date: Tue, 24 Jan 2023 22:19:09 +0000
> From: Rayyan Ansari <rayyan@ansari.sh>
> 
> On 23/01/2023 17:53, Rob Herring wrote:
> > On Sun, Jan 22, 2023 at 05:25:38PM +0000, Rayyan Ansari wrote:
> >> On 22/01/2023 15:36, Rob Herring wrote:
> >>> On Sat, Jan 21, 2023 at 9:36 AM Rayyan Ansari <rayyan@ansari.sh> wrote:
> >>>>
> >>>
> >>> Why do you need this change?
> >>>
> >>> The 'simple-framebuffer' contains data on how the bootloader
> >>> configured the display. The bootloader doesn't configure the display
> >>> size, so this information doesn't belong here. The information should
> >>> already be in the panel node, so also no point in duplicating it here.
> >>>
> >>>> Signed-off-by: Rayyan Ansari <rayyan@ansari.sh>
> >>>> ---
> >>>>    .../devicetree/bindings/display/simple-framebuffer.yaml   | 8 ++++++++
> >>>>    1 file changed, 8 insertions(+)
> >>
> >> Hi Rob,
> >>
> >> There is the usecase that Hans has mentioned, but I have also mentioned
> >> another usecase previously.
> >>
> >> Adding the width-mm and height-mm properties allows user interfaces such as
> >> Phosh (https://puri.sm/posts/phosh-overview/) to scale correctly to the
> >> screen. In my case, a panel node is not available and the aforementioned
> >> interface is in fact running on the SimpleDRM driver (which binds to the
> >> simple-framebuffer device).
> > 
> > Why is the panel node not available? Why not add it? Presumably it is
> > not there because you aren't (yet) using the simple-panel driver (and
> > others that would need). But presumably you will eventually as I'd
> > imagine turning the screen off and back on might be a desired feature.
> 
> It requires more than using the simple-panel driver: first the SoC side 
> display hardware needs to be brought up, then a panel driver that 
> implements the proper DCS initialisation sequence needs to be written 
> (which is currently not fully known).

You don't really need a driver.  You can just lookup the panel node
from your simple-framebuffer driver and get the values of the
properties there.

Re: [PATCH v2 2/2] dt-bindings: display: simple-framebuffer: Document physical width and height properties

[Krzysztof Kozlowski]

On 24/01/2023 23:19, Rayyan Ansari wrote:
> On 23/01/2023 17:53, Rob Herring wrote:
>> On Sun, Jan 22, 2023 at 05:25:38PM +0000, Rayyan Ansari wrote:
>>> On 22/01/2023 15:36, Rob Herring wrote:
>>>> On Sat, Jan 21, 2023 at 9:36 AM Rayyan Ansari <rayyan@ansari.sh> wrote:
>>>>>
>>>>
>>>> Why do you need this change?
>>>>
>>>> The 'simple-framebuffer' contains data on how the bootloader
>>>> configured the display. The bootloader doesn't configure the display
>>>> size, so this information doesn't belong here. The information should
>>>> already be in the panel node, so also no point in duplicating it here.
>>>>
>>>>> Signed-off-by: Rayyan Ansari <rayyan@ansari.sh>
>>>>> ---
>>>>>    .../devicetree/bindings/display/simple-framebuffer.yaml   | 8 ++++++++
>>>>>    1 file changed, 8 insertions(+)
>>>
>>> Hi Rob,
>>>
>>> There is the usecase that Hans has mentioned, but I have also mentioned
>>> another usecase previously.
>>>
>>> Adding the width-mm and height-mm properties allows user interfaces such as
>>> Phosh (https://puri.sm/posts/phosh-overview/) to scale correctly to the
>>> screen. In my case, a panel node is not available and the aforementioned
>>> interface is in fact running on the SimpleDRM driver (which binds to the
>>> simple-framebuffer device).
>>
>> Why is the panel node not available? Why not add it? Presumably it is
>> not there because you aren't (yet) using the simple-panel driver (and
>> others that would need). But presumably you will eventually as I'd
>> imagine turning the screen off and back on might be a desired feature.
> 
> It requires more than using the simple-panel driver: first the SoC side 
> display hardware needs to be brought up, then a panel driver that 
> implements the proper DCS initialisation sequence needs to be written 
> (which is currently not fully known).
> 
>>
>> So why add a temporary DT property that's tied to your *current* kernel? > The DT should not be tightly coupled to the kernel.
> 
> I'm not sure what you mean by it being "tightly coupled" to the kernel.

It means that you used current Linux driver support (or lack) for some
hardware as an argument for bindings. If you add later the driver, the
bindings should be changed? Answer is: not. Bindings should be
independent of Linux drivers, thus whatever kernel is missing now, is
not an argument in favor of this property.

Best regards,
Krzysztof