Re: [PATCH 6/8] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Krzysztof and Jacopo,

On Mon, 12 Feb 2024 at 08:12, Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 09/02/2024 17:48, Jacopo Mondi wrote:
> > Add bindings for the Raspberry Pi PiSP Back End memory-to-memory image
> > signal processor.
> >
> > Datasheet:
> > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> >
>
>
> > +---
> > +$id: http://devicetree.org/schemas/media/raspberrypi,pispbe.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Raspberry Pi PiSP Image Signal Processor (ISP) Back End
> > +
> > +maintainers:
> > +  - Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > +  - David Plowman <david.plowman@xxxxxxxxxxxxxxx>
> > +  - Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> > +  - Naushir Patuck <naush@xxxxxxxxxxxxxxx>
> > +  - Nick Hollinghurst <nick.hollinghurst@xxxxxxxxxxxxxxx>
>
> I assume all folks are fine being here?

Although I am fine with my name above, I think it might be easier to use:

Raspberry Pi Kernel Maintenance <kernel-list@xxxxxxxxxxxxxxx>

to follow other Raspberry Pi drivers.

>
> > +
> > +description: |
> > +  The Raspberry Pi PiSP Image Signal Processor (ISP) Back End is an image
> > +  processor that fetches images in Bayer or Grayscale format from DRAM memory
> > +  in tiles and produces images consumable by applications.
> > +
> > +  The full ISP documentation is available at:
> > +  https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> > +
> > +properties:
> > +  compatible:
> > +    const: raspberrypi,pispbe
>
> Nothing more specific? No model name, no version? It's quite generic
> compatible which in general should not be allowed. I would assume that
> at least version of Pi could denote some sort of a model... unless
> version is detectable?

There is a version register that is tested for different HW variants.  The
expectation is variant handling in the driver would happen based on this version
register.  Do you think that is sufficient to keep the compatible string as-is?

Thanks,
Naush

>
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    const: isp_be
>
> You can skip clock-names if they have only one entry and it matches
> block name. Quite useless.
>
> > +
> > +  iommus:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - clock-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    rpi1 {
>
> soc {
>
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        isp: pisp_be@880000  {
>
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> so: isp@
>
> and drop unused label
>
> > +             compatible = "raspberrypi,pispbe";
> ds,
> Krzysztof
>




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux