Re: [PATCH v2 1/3] dt-bindings: display: st7701: Add Anbernic RG28XX panel

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3394 bytes --]

On Tue, Jul 02, 2024 at 02:28:27AM +0900, Hironori KIKUCHI wrote:
> On Sun, Jun 30, 2024 at 11:17 PM Conor Dooley <conor@kernel.org> wrote:
> > On Sat, Jun 29, 2024 at 05:26:56PM +0900, Hironori KIKUCHI wrote:
> > > On Sat, Jun 29, 2024 at 1:27 AM Conor Dooley <conor@kernel.org> wrote:
> > > > On Fri, Jun 28, 2024 at 02:10:15PM +0900, Hironori KIKUCHI wrote:

> > > > >            - densitron,dmt028vghmcmi-1a
> > > > >            - elida,kd50t048a
> > > > >            - techstar,ts8550b
> > > > >        - const: sitronix,st7701
> > > > >
> > > > >    reg:
> > > > > -    description: DSI virtual channel used by that screen
> > > > > +    description: DSI / SPI channel used by that screen
> > > > >      maxItems: 1
> > > > >
> > > > >    VCC-supply:
> > > > > @@ -43,6 +41,13 @@ properties:
> > > > >    IOVCC-supply:
> > > > >      description: I/O system regulator
> > > > >
> > > > > +  dc-gpios:
> > > > > +    maxItems: 1
> > > > > +    description:
> > > > > +      Controller data/command selection (D/CX) in 4-line SPI mode.
> > > > > +      If not set, the controller is in 3-line SPI mode.
> > > > > +      Disallowed for DSI.
> > > > > +
> > > > >    port: true
> > > > >    reset-gpios: true
> > > > >    rotation: true
> > > > > @@ -57,7 +62,38 @@ required:
> > > > >    - port
> > > > >    - reset-gpios
> > > > >
> > > > > -additionalProperties: false
> > > > > +allOf:
> > > > > +  - $ref: panel-common.yaml#
> > > > > +  - if:
> > > > > +      properties:
> > > > > +        compatible:
> > > > > +          contains:
> > > > > +            # SPI connected panels
> > > > > +            enum:
> > > > > +              - anbernic,rg28xx-panel
> > > > > +    then:
> > > > > +      $ref: /schemas/spi/spi-peripheral-props.yaml
> > > >
> > > > I'm not really keen on this. I'd rather see a different binding for the
> > > > SPI version compared to the MIPI ones. Is doing things like this common
> > > > for panels? If it is, I'll turn a blind eye..
> > >
> > > This might be the first case that a driver supports both DSI and SPI
> > > for a panel.
> > > The panel can be either a DSI device or an SPI device.
> >
> > The commit message sounded like the panel was capable of doing SPI
> > instead of DSI, is that not the case and the panel is actually capable
> > of doing both, just happens to be connected as SPI in this particular
> > device?
> 
> Sorry for the confusion.
> I think it depends on what the "panel" refers to...
> Although the "panel driver IC" (ST7701 variant) is capable of doing
> both, the "panel assy" (including its cable) of the RG28XX only has an
> SPI interface in its pinout.
> If the compatible string "rg28xx-panel" represents the assy, then I
> could say the "panel" only supports SPI, and the other panels only
> support DSI.
> But if it only represents a specific LCD panel and its driver IC with
> control parameters, then it theoretically supports both, and it might
> be sufficient to just include spi-peripheral-props, as in v1.
> v1: https://lore.kernel.org/linux-kernel/20240618081515.1215552-2-kikuchan98@gmail.com/

I thought about it some more, and I think what you've got for this in
the binding at present is fine. Splitting the binding I think would
require a select and wouldn't really be any "cleaner" of an
implementation.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]