Re: [PATCH 1/2] dt-bindings: display: panel: Add WL-355608-A8 panel

["Ryan Walklin" <ryan@xxxxxxxxxxxxx>]

On Sat, 25 May 2024, at 7:10 AM, Conor Dooley wrote:

Thanks for the review!

>> +
>> +properties:
>> +  compatible:
>> +    const: wl-355608-a8
>
> You're missing a vendor prefix here. And when you add it, update the
> filename to match.

Thanks, I don't actually know the vendor, would it be acceptable to just use "wl"?

>> +
>> +        sck-gpios = <&pio 8 9 GPIO_ACTIVE_HIGH>; // PI9
>> +        mosi-gpios = <&pio 8 10 GPIO_ACTIVE_HIGH>; // PI10
>> +        cs-gpios = <&pio 8 8 GPIO_ACTIVE_HIGH>; // PI8
>> +        num-chipselects = <1>;
>
> All of this is not needed in the example, all you need to have here is:
>
> spi {
>     #address-cells = <1>;
>     #size-cells = <0>;
>
Thanks, will clean it up.
>> +
>> +        panel: panel@0 {
>
> This "panel" label is not used, you should drop it.
>
Noted, ta.
>> +            compatible = "wl_355608_a8";
>
> This doesn't match what you documented, be sure to run dt_binding_check.

Thanks, changed underscore to dash mid-patch and neglected to fix all the examples (and the subsequent code patch it seems. Will correct. Is there a preference one way or another? 

>
>> +            reg = <0>;
>> +
>> +            spi-3wire;
>> +            spi-max-frequency = <3125000>;
>> +
>> +            reset-gpios = <&pio 8 14 GPIO_ACTIVE_LOW>; // PI14
>> +
>> +            backlight = <&backlight>;
>> +            power-supply = <&reg_lcd>;
>> +            pinctrl-0 = <&lcd0_rgb888_pins>;
>> +            pinctrl-names = "default";
>> +
>> +            port {
>> +            	panel_in_rgb: endpoint {
>
> Neither is this label afaict.
>
> Thanks,
> Conor.


Regards,

Ryan