| Message ID | 20240209164825.166800-7-jacopo.mondi@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: raspberrypi: Add support for PiSP Back End | expand |
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@raspberrypi.com> > + - David Plowman <david.plowman@raspberrypi.com> > + - Jacopo Mondi <jacopo.mondi@ideasonboard.com> > + - Naushir Patuck <naush@raspberrypi.com> > + - Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> I assume all folks are fine being here? > + > +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? > + > + 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
Hi Krzysztof On Mon, Feb 12, 2024 at 09:12:11AM +0100, Krzysztof Kozlowski 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@raspberrypi.com> > > + - David Plowman <david.plowman@raspberrypi.com> > > + - Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > + - Naushir Patuck <naush@raspberrypi.com> > > + - Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > I assume all folks are fine being here? > I admint I haven't ask any single one of them, and I listed all of them thinking of "maintainers of PiSP", but when it comes to bindings only we can shorten the list if preferred ? > > + > > +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? > The driver matches on a version register and that should be enough to handle quirks which are specific to an IP revision in the driver itself. Considering how minimal the integration with the SoC is (one clock, one interrupt and one optional iommu reference) even if we'll get future revisions of the SoC I don't think there will be any need to match on a dedicated compatible for bindings-validation purposes. However I understand that to be future-proof it's good practice to allow a more flexible scheme, so we can have a generic fallback and a revision-specific entry. Would compatible: items: - enum: - raspberrypi,pipspbe-bcm2712 - const: raspberrypi,pispbe do in this case ? Also, let's see what RPi think as they are certainly more informed than me on what a good revision-specific match could be > > + > > + 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. > ack > > + > > + iommus: > > + maxItems: 1 > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - clocks > > + - clock-names > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + > > + rpi1 { > > soc { > Are you sure ? This will only ever live in the 'rp1' node. > > + #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 ok Thanks j PS: on v6.8-rc1 I'm seeing LINT Documentation/devicetree/bindings usage: yamllint [-h] [-] [-c CONFIG_FILE | -d CONFIG_DATA] [--list-files] [-f {parsable,standard,colored,github,auto}] [-s] [--no-warnings] [-v] [FILE_OR_DIR ...] when running dt_binding_check My setup should be reasonably up-to-date, is it me only seeing this ? > > > + compatible = "raspberrypi,pispbe"; > ds, > Krzysztof >
Hi Krzysztof and Jacopo, On Mon, 12 Feb 2024 at 08:12, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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@raspberrypi.com> > > + - David Plowman <david.plowman@raspberrypi.com> > > + - Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > + - Naushir Patuck <naush@raspberrypi.com> > > + - Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > 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@raspberrypi.com> 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 >
On 12/02/2024 09:50, Jacopo Mondi wrote: >>> +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? >> > > The driver matches on a version register and that should be enough to > handle quirks which are specific to an IP revision in the driver > itself. > > Considering how minimal the integration with the SoC is (one clock, one > interrupt and one optional iommu reference) even if we'll get future > revisions of the SoC I don't think there will be any need to match on > a dedicated compatible for bindings-validation purposes. > > However I understand that to be future-proof it's good practice to > allow a more flexible scheme, so we can have a generic fallback and a > revision-specific entry. > > Would > > compatible: > items: > - enum: > - raspberrypi,pipspbe-bcm2712 bcm2712 is manufactured by Broadcom, not Raspberry Pi, so it should be rather Pi model? > - const: raspberrypi,pispbe > > do in this case ? > > Also, let's see what RPi think as they are certainly more informed > than me on what a good revision-specific match could be I am fine with auto-detection, though. ... >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/interrupt-controller/arm-gic.h> >>> + >>> + rpi1 { >> >> soc { >> > > Are you sure ? This will only ever live in the 'rp1' node. What is "rp1" node? Does not look like a generic name. > >>> + #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 > > ok > > Thanks > j > > PS: > on v6.8-rc1 I'm seeing > > LINT Documentation/devicetree/bindings > usage: yamllint [-h] [-] [-c CONFIG_FILE | -d CONFIG_DATA] [--list-files] > [-f {parsable,standard,colored,github,auto}] [-s] [--no-warnings] [-v] > [FILE_OR_DIR ...] > > when running dt_binding_check > > My setup should be reasonably up-to-date, is it me only seeing this ? I think you need to update your yamllint. Best regards, Krzysztof
Hi, On 12/02/2024 11:05, Krzysztof Kozlowski wrote: > On 12/02/2024 09:50, Jacopo Mondi wrote: > >>>> +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? >>> >> >> The driver matches on a version register and that should be enough to >> handle quirks which are specific to an IP revision in the driver >> itself. >> >> Considering how minimal the integration with the SoC is (one clock, one >> interrupt and one optional iommu reference) even if we'll get future >> revisions of the SoC I don't think there will be any need to match on >> a dedicated compatible for bindings-validation purposes. >> >> However I understand that to be future-proof it's good practice to >> allow a more flexible scheme, so we can have a generic fallback and a >> revision-specific entry. >> >> Would >> >> compatible: >> items: >> - enum: >> - raspberrypi,pipspbe-bcm2712 > > bcm2712 is manufactured by Broadcom, not Raspberry Pi, so it should be > rather Pi model? Indeed, this is something I don't get. If the BE is in the bcm2712, is it not a broadcom IP? Why is raspberrypi in the compatible name at all? Naush, Dave? >> - const: raspberrypi,pispbe >> >> do in this case ? >> >> Also, let's see what RPi think as they are certainly more informed >> than me on what a good revision-specific match could be > > I am fine with auto-detection, though. > > ... > >>>> + >>>> +examples: >>>> + - | >>>> + #include <dt-bindings/interrupt-controller/arm-gic.h> >>>> + >>>> + rpi1 { >>> >>> soc { >>> >> >> Are you sure ? This will only ever live in the 'rp1' node. > > What is "rp1" node? Does not look like a generic name. I don't think this is right. RP1 is a separate chip, an IO controller, on raspberrypi 5. BE is not in the RP1. Tomi
Hi Tomi On Tue, Feb 13, 2024 at 09:28:33AM +0200, Tomi Valkeinen wrote: > Hi, > > On 12/02/2024 11:05, Krzysztof Kozlowski wrote: > > On 12/02/2024 09:50, Jacopo Mondi wrote: > > > > > > > +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? > > > > > > > > > > The driver matches on a version register and that should be enough to > > > handle quirks which are specific to an IP revision in the driver > > > itself. > > > > > > Considering how minimal the integration with the SoC is (one clock, one > > > interrupt and one optional iommu reference) even if we'll get future > > > revisions of the SoC I don't think there will be any need to match on > > > a dedicated compatible for bindings-validation purposes. > > > > > > However I understand that to be future-proof it's good practice to > > > allow a more flexible scheme, so we can have a generic fallback and a > > > revision-specific entry. > > > > > > Would > > > > > > compatible: > > > items: > > > - enum: > > > - raspberrypi,pipspbe-bcm2712 > > > > bcm2712 is manufactured by Broadcom, not Raspberry Pi, so it should be > > rather Pi model? > > Indeed, this is something I don't get. If the BE is in the bcm2712, is it > not a broadcom IP? Why is raspberrypi in the compatible name at all? > > Naush, Dave? > > > > - const: raspberrypi,pispbe > > > > > > do in this case ? > > > > > > Also, let's see what RPi think as they are certainly more informed > > > than me on what a good revision-specific match could be > > > > I am fine with auto-detection, though. > > > > ... > > > > > > > + > > > > > +examples: > > > > > + - | > > > > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > > + > > > > > + rpi1 { > > > > > > > > soc { > > > > > > > > > > Are you sure ? This will only ever live in the 'rp1' node. > > > > What is "rp1" node? Does not look like a generic name. > > I don't think this is right. RP1 is a separate chip, an IO controller, on > raspberrypi 5. BE is not in the RP1. > Ah yes indeed, bad copy and paste from me. I'll s/rpi/soc as suggested by Krzysztof > Tomi > >
Hi Tomi, On Tue, 13 Feb 2024 at 07:28, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote: > > Hi, > > On 12/02/2024 11:05, Krzysztof Kozlowski wrote: > > On 12/02/2024 09:50, Jacopo Mondi wrote: > > > >>>> +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? > >>> > >> > >> The driver matches on a version register and that should be enough to > >> handle quirks which are specific to an IP revision in the driver > >> itself. > >> > >> Considering how minimal the integration with the SoC is (one clock, one > >> interrupt and one optional iommu reference) even if we'll get future > >> revisions of the SoC I don't think there will be any need to match on > >> a dedicated compatible for bindings-validation purposes. > >> > >> However I understand that to be future-proof it's good practice to > >> allow a more flexible scheme, so we can have a generic fallback and a > >> revision-specific entry. > >> > >> Would > >> > >> compatible: > >> items: > >> - enum: > >> - raspberrypi,pipspbe-bcm2712 > > > > bcm2712 is manufactured by Broadcom, not Raspberry Pi, so it should be > > rather Pi model? > > Indeed, this is something I don't get. If the BE is in the bcm2712, is > it not a broadcom IP? Why is raspberrypi in the compatible name at all? > > Naush, Dave? The Backend (and Frontend) IP are both owned solely by Raspberry Pi, and the BE is instantiated on the BCM2712. So I think "raspberry" in the compatible string is correct here. > > >> - const: raspberrypi,pispbe > >> > >> do in this case ? > >> > >> Also, let's see what RPi think as they are certainly more informed > >> than me on what a good revision-specific match could be > > > > I am fine with auto-detection, though. > > > > ... > > > >>>> + > >>>> +examples: > >>>> + - | > >>>> + #include <dt-bindings/interrupt-controller/arm-gic.h> > >>>> + > >>>> + rpi1 { > >>> > >>> soc { > >>> > >> > >> Are you sure ? This will only ever live in the 'rp1' node. > > > > What is "rp1" node? Does not look like a generic name. > > I don't think this is right. RP1 is a separate chip, an IO controller, > on raspberrypi 5. BE is not in the RP1. Oops, missed this. Yes, I think it should be the "soc" node. Regards, Naush > > Tomi >
On Tue, Feb 13, 2024 at 08:35:39AM +0000, Naushir Patuck wrote: > On Tue, 13 Feb 2024 at 07:28, Tomi Valkeinen wrote: > > On 12/02/2024 11:05, Krzysztof Kozlowski wrote: > > > On 12/02/2024 09:50, Jacopo Mondi wrote: > > > > > >>>> +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? > > >> > > >> The driver matches on a version register and that should be enough to > > >> handle quirks which are specific to an IP revision in the driver > > >> itself. > > >> > > >> Considering how minimal the integration with the SoC is (one clock, one > > >> interrupt and one optional iommu reference) even if we'll get future > > >> revisions of the SoC I don't think there will be any need to match on > > >> a dedicated compatible for bindings-validation purposes. > > >> > > >> However I understand that to be future-proof it's good practice to > > >> allow a more flexible scheme, so we can have a generic fallback and a > > >> revision-specific entry. > > >> > > >> Would > > >> > > >> compatible: > > >> items: > > >> - enum: > > >> - raspberrypi,pipspbe-bcm2712 > > > > > > bcm2712 is manufactured by Broadcom, not Raspberry Pi, so it should be > > > rather Pi model? > > > > Indeed, this is something I don't get. If the BE is in the bcm2712, is > > it not a broadcom IP? Why is raspberrypi in the compatible name at all? > > > > Naush, Dave? > > The Backend (and Frontend) IP are both owned solely by Raspberry Pi, > and the BE is instantiated on the BCM2712. So I think "raspberry" in > the compatible string is correct here. Following what we do with other SoCs, we could have compatible = "brcm,pispbe-bcm2712", "raspberrypi,pispbe"; However, that scheme is mostly used when SoC vendor license IP cores from third parties. Here, while the SoC is indeed manufactured by Broadcom, it's a Raspberry Pi-specific SoC. I don't have a personal preference. > > >> - const: raspberrypi,pispbe > > >> > > >> do in this case ? > > >> > > >> Also, let's see what RPi think as they are certainly more informed > > >> than me on what a good revision-specific match could be > > > > > > I am fine with auto-detection, though. > > > > > > ... > > > > > >>>> + > > >>>> +examples: > > >>>> + - | > > >>>> + #include <dt-bindings/interrupt-controller/arm-gic.h> > > >>>> + > > >>>> + rpi1 { > > >>> > > >>> soc { > > >>> > > >> > > >> Are you sure ? This will only ever live in the 'rp1' node. > > > > > > What is "rp1" node? Does not look like a generic name. > > > > I don't think this is right. RP1 is a separate chip, an IO controller, > > on raspberrypi 5. BE is not in the RP1. > > Oops, missed this. Yes, I think it should be the "soc" node.
On Tue, Feb 13, 2024 at 12:44:05PM +0200, Laurent Pinchart wrote: > On Tue, Feb 13, 2024 at 08:35:39AM +0000, Naushir Patuck wrote: > > On Tue, 13 Feb 2024 at 07:28, Tomi Valkeinen wrote: > > > On 12/02/2024 11:05, Krzysztof Kozlowski wrote: > > > > On 12/02/2024 09:50, Jacopo Mondi wrote: > > > > > > > >>>> +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? > > > >> > > > >> The driver matches on a version register and that should be enough to > > > >> handle quirks which are specific to an IP revision in the driver > > > >> itself. > > > >> > > > >> Considering how minimal the integration with the SoC is (one clock, one > > > >> interrupt and one optional iommu reference) even if we'll get future > > > >> revisions of the SoC I don't think there will be any need to match on > > > >> a dedicated compatible for bindings-validation purposes. > > > >> > > > >> However I understand that to be future-proof it's good practice to > > > >> allow a more flexible scheme, so we can have a generic fallback and a > > > >> revision-specific entry. > > > >> > > > >> Would > > > >> > > > >> compatible: > > > >> items: > > > >> - enum: > > > >> - raspberrypi,pipspbe-bcm2712 > > > > > > > > bcm2712 is manufactured by Broadcom, not Raspberry Pi, so it should be > > > > rather Pi model? > > > > > > Indeed, this is something I don't get. If the BE is in the bcm2712, is > > > it not a broadcom IP? Why is raspberrypi in the compatible name at all? > > > > > > Naush, Dave? > > > > The Backend (and Frontend) IP are both owned solely by Raspberry Pi, > > and the BE is instantiated on the BCM2712. So I think "raspberry" in > > the compatible string is correct here. > > Following what we do with other SoCs, we could have > > compatible = "brcm,pispbe-bcm2712", "raspberrypi,pispbe"; > > However, that scheme is mostly used when SoC vendor license IP cores > from third parties. Here, while the SoC is indeed manufactured by > Broadcom, it's a Raspberry Pi-specific SoC. > > I don't have a personal preference. I'd be okay with what you propose here, I think it is a better reflection of what is going on than that in the original patch etc. Cheers, Conor.
On Tue, Feb 13, 2024 at 12:44:05PM +0200, Laurent Pinchart wrote: > On Tue, Feb 13, 2024 at 08:35:39AM +0000, Naushir Patuck wrote: > > On Tue, 13 Feb 2024 at 07:28, Tomi Valkeinen wrote: > > > On 12/02/2024 11:05, Krzysztof Kozlowski wrote: > > > > On 12/02/2024 09:50, Jacopo Mondi wrote: > > > > > > > >>>> +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? > > > >> > > > >> The driver matches on a version register and that should be enough to > > > >> handle quirks which are specific to an IP revision in the driver > > > >> itself. > > > >> > > > >> Considering how minimal the integration with the SoC is (one clock, one > > > >> interrupt and one optional iommu reference) even if we'll get future > > > >> revisions of the SoC I don't think there will be any need to match on > > > >> a dedicated compatible for bindings-validation purposes. > > > >> > > > >> However I understand that to be future-proof it's good practice to > > > >> allow a more flexible scheme, so we can have a generic fallback and a > > > >> revision-specific entry. > > > >> > > > >> Would > > > >> > > > >> compatible: > > > >> items: > > > >> - enum: > > > >> - raspberrypi,pipspbe-bcm2712 > > > > > > > > bcm2712 is manufactured by Broadcom, not Raspberry Pi, so it should be > > > > rather Pi model? > > > > > > Indeed, this is something I don't get. If the BE is in the bcm2712, is > > > it not a broadcom IP? Why is raspberrypi in the compatible name at all? > > > > > > Naush, Dave? > > > > The Backend (and Frontend) IP are both owned solely by Raspberry Pi, > > and the BE is instantiated on the BCM2712. So I think "raspberry" in > > the compatible string is correct here. > > Following what we do with other SoCs, we could have > > compatible = "brcm,pispbe-bcm2712", "raspberrypi,pispbe"; Nit: brcm,bcm7212-pispbe Otherwise looks fine to me. Rob
diff --git a/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml b/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml new file mode 100644 index 000000000000..50111b87ad81 --- /dev/null +++ b/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml @@ -0,0 +1,68 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$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@raspberrypi.com> + - David Plowman <david.plowman@raspberrypi.com> + - Jacopo Mondi <jacopo.mondi@ideasonboard.com> + - Naushir Patuck <naush@raspberrypi.com> + - Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> + +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 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + maxItems: 1 + + clock-names: + const: isp_be + + iommus: + maxItems: 1 + +required: + - compatible + - reg + - interrupts + - clocks + - clock-names + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + + rpi1 { + #address-cells = <2>; + #size-cells = <2>; + + isp: pisp_be@880000 { + compatible = "raspberrypi,pispbe"; + reg = <0x10 0x00880000 0x0 0x4000>; + interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&firmware_clocks 7>; + clock-names = "isp_be"; + iommus = <&iommu2>; + }; + };
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 Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- .../bindings/media/raspberrypi,pispbe.yaml | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml -- 2.43.0