All of lore.kernel.org
 help / color / mirror / Atom feed
From: Rob Herring <robh@kernel.org>
To: Rayyan Ansari <rayyan@ansari.sh>
Cc: dri-devel@lists.freedesktop.org,
	~postmarketos/upstreaming@lists.sr.ht, asahi@lists.linux.dev,
	janne@jannau.net, Daniel Vetter <daniel@ffwll.ch>,
	David Airlie <airlied@gmail.com>,
	devicetree@vger.kernel.org, Hans de Goede <hdegoede@redhat.com>,
	Javier Martinez Canillas <javierm@redhat.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	Thomas Zimmermann <tzimmermann@suse.de>
Subject: Re: [PATCH v2 2/2] dt-bindings: display: simple-framebuffer: Document physical width and height properties
Date: Mon, 23 Jan 2023 11:53:39 -0600	[thread overview]
Message-ID: <20230123175339.GA2019900-robh@kernel.org> (raw)
In-Reply-To: <cdf32cb0-4529-6bbd-fdda-ae641d141ee5@ansari.sh>

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

WARNING: multiple messages have this Message-ID (diff)
From: Rob Herring <robh@kernel.org>
To: Rayyan Ansari <rayyan@ansari.sh>
Cc: devicetree@vger.kernel.org, linux-fbdev@vger.kernel.org,
	janne@jannau.net,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Javier Martinez Canillas <javierm@redhat.com>,
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	Hans de Goede <hdegoede@redhat.com>,
	~postmarketos/upstreaming@lists.sr.ht, asahi@lists.linux.dev
Subject: Re: [PATCH v2 2/2] dt-bindings: display: simple-framebuffer: Document physical width and height properties
Date: Mon, 23 Jan 2023 11:53:39 -0600	[thread overview]
Message-ID: <20230123175339.GA2019900-robh@kernel.org> (raw)
In-Reply-To: <cdf32cb0-4529-6bbd-fdda-ae641d141ee5@ansari.sh>

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

  reply	other threads:[~2023-01-23 17:53 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-21 15:35 [PATCH v2 0/2] SimpleDRM: allow configuring physical width and height Rayyan Ansari
2023-01-21 15:35 ` Rayyan Ansari
2023-01-21 15:35 ` [PATCH v2 1/2] drm/simpledrm: Allow physical width and height configuration via DT Rayyan Ansari
2023-01-21 15:35   ` Rayyan Ansari
2023-01-26 13:05   ` Thomas Zimmermann
2023-01-26 13:05     ` Thomas Zimmermann
2023-01-21 15:35 ` [PATCH v2 2/2] dt-bindings: display: simple-framebuffer: Document physical width and height properties Rayyan Ansari
2023-01-21 15:35   ` Rayyan Ansari
2023-01-22 15:31   ` Rob Herring
2023-01-22 15:31     ` Rob Herring
2023-01-22 17:28     ` Rayyan Ansari
2023-01-22 17:28       ` Rayyan Ansari
2023-01-22 15:36   ` Rob Herring
2023-01-22 15:36     ` Rob Herring
2023-01-22 15:42     ` Hans de Goede
2023-01-22 15:42       ` Hans de Goede
2023-01-22 17:25     ` Rayyan Ansari
2023-01-22 17:25       ` Rayyan Ansari
2023-01-23 17:53       ` Rob Herring [this message]
2023-01-23 17:53         ` Rob Herring
2023-01-24 22:19         ` Rayyan Ansari
2023-01-24 22:19           ` Rayyan Ansari
2023-01-24 22:40           ` Mark Kettenis
2023-01-24 22:40             ` Mark Kettenis
2023-01-25  7:28           ` Krzysztof Kozlowski
2023-01-25  7:28             ` Krzysztof Kozlowski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230123175339.GA2019900-robh@kernel.org \
    --to=robh@kernel.org \
    --cc=airlied@gmail.com \
    --cc=asahi@lists.linux.dev \
    --cc=daniel@ffwll.ch \
    --cc=devicetree@vger.kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=hdegoede@redhat.com \
    --cc=janne@jannau.net \
    --cc=javierm@redhat.com \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=linux-fbdev@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rayyan@ansari.sh \
    --cc=tzimmermann@suse.de \
    --cc=~postmarketos/upstreaming@lists.sr.ht \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.