| Message ID | 1484926369-30910-5-git-send-email-andrea.merello@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Jan 20, 2017 at 04:32:49PM +0100, Andrea Merello wrote: > Signed-off-by: Andrea Merello <andrea.merello@gmail.com> > --- > Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt > index 2897e6d..71d31f9 100644 > --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt > +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt > @@ -34,6 +34,9 @@ Required properties: > Optional elements: "m_axi_mm2s_aclk", "m_axi_s2mm_aclk", > "m_axi_sg_aclk" > > +Required properties for AXI DMA: > +- xlnx,lengthregwidth: Should be the width of the length register as configured in h/w. Ah it should be before the patch you are using it! Now, what happens on older firmware where this property is not there.. Also CC DT folks on all DT related changes > Required properties for VDMA: > - xlnx,num-fstores: Should be the number of framebuffers as configured in h/w. why are you removing this
On Sun, Feb 5, 2017 at 6:38 AM, Vinod Koul <vinod.koul@intel.com> wrote: > On Fri, Jan 20, 2017 at 04:32:49PM +0100, Andrea Merello wrote: >> Signed-off-by: Andrea Merello <andrea.merello@gmail.com> >> --- >> Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt >> index 2897e6d..71d31f9 100644 >> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt >> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt >> @@ -34,6 +34,9 @@ Required properties: >> Optional elements: "m_axi_mm2s_aclk", "m_axi_s2mm_aclk", >> "m_axi_sg_aclk" >> >> +Required properties for AXI DMA: >> +- xlnx,lengthregwidth: Should be the width of the length register as configured in h/w. > > Ah it should be before the patch you are using it! ah, OK > Now, what happens on older firmware where this property is not there.. Yes, it does refuse to load.. But al least, instead of behaving incorrectly due to the driver assuming the HW has a certain maximum transfer length that might not match the real one, we get a message that tell us to add this property. I thought it was acceptable to introduce a new mandatory property because I've already seen changes like this (IIRC was about I2C clocks), if this is not the case I can just spit a warning and stick with the old default value in case the prop is missing.. > Also CC DT folks on all DT related changes OK. >> Required properties for VDMA: >> - xlnx,num-fstores: Should be the number of framebuffers as configured in h/w. > > why are you removing this Did I ? That minus sign is in the original text, it isn't a negative diff marker. > -- > ~Vinod -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 06, 2017 at 08:20:35AM +0100, Andrea Merello wrote: > On Sun, Feb 5, 2017 at 6:38 AM, Vinod Koul <vinod.koul@intel.com> wrote: > > On Fri, Jan 20, 2017 at 04:32:49PM +0100, Andrea Merello wrote: > >> Signed-off-by: Andrea Merello <andrea.merello@gmail.com> > >> --- > >> Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt > >> index 2897e6d..71d31f9 100644 > >> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt > >> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt > >> @@ -34,6 +34,9 @@ Required properties: > >> Optional elements: "m_axi_mm2s_aclk", "m_axi_s2mm_aclk", > >> "m_axi_sg_aclk" > >> > >> +Required properties for AXI DMA: > >> +- xlnx,lengthregwidth: Should be the width of the length register as configured in h/w. > > > > Ah it should be before the patch you are using it! > > ah, OK > > > Now, what happens on older firmware where this property is not there.. > > Yes, it does refuse to load.. But al least, instead of behaving > incorrectly due to the driver assuming the HW has a certain maximum > transfer length that might not match the real one, we get a message > that tell us to add this property. > > I thought it was acceptable to introduce a new mandatory property > because I've already seen changes like this (IIRC was about I2C > clocks), if this is not the case I can just spit a warning and stick > with the old default value in case the prop is missing.. I am not sure about that part, IIUC ABI treats this as regression. ABIs are supposed to be backward compatible. > > > Also CC DT folks on all DT related changes > > OK. Lets hear from Rob as well on this
On Mon, Feb 6, 2017 at 10:50 AM, Vinod Koul <vinod.koul@intel.com> wrote: > On Mon, Feb 06, 2017 at 08:20:35AM +0100, Andrea Merello wrote: >> On Sun, Feb 5, 2017 at 6:38 AM, Vinod Koul <vinod.koul@intel.com> wrote: >> > On Fri, Jan 20, 2017 at 04:32:49PM +0100, Andrea Merello wrote: >> >> Signed-off-by: Andrea Merello <andrea.merello@gmail.com> >> >> --- >> >> Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt | 3 +++ >> >> 1 file changed, 3 insertions(+) >> >> >> >> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt >> >> index 2897e6d..71d31f9 100644 >> >> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt >> >> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt >> >> @@ -34,6 +34,9 @@ Required properties: >> >> Optional elements: "m_axi_mm2s_aclk", "m_axi_s2mm_aclk", >> >> "m_axi_sg_aclk" >> >> >> >> +Required properties for AXI DMA: >> >> +- xlnx,lengthregwidth: Should be the width of the length register as configured in h/w. How about some hyphens... >> > >> > Ah it should be before the patch you are using it! >> >> ah, OK >> >> > Now, what happens on older firmware where this property is not there.. >> >> Yes, it does refuse to load.. But al least, instead of behaving >> incorrectly due to the driver assuming the HW has a certain maximum >> transfer length that might not match the real one, we get a message >> that tell us to add this property. >> >> I thought it was acceptable to introduce a new mandatory property >> because I've already seen changes like this (IIRC was about I2C >> clocks), if this is not the case I can just spit a warning and stick >> with the old default value in case the prop is missing.. > > I am not sure about that part, IIUC ABI treats this as regression. ABIs are > supposed to be backward compatible. Correct. New properties should generally be optional. Rob -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt index 2897e6d..71d31f9 100644 --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt @@ -34,6 +34,9 @@ Required properties: Optional elements: "m_axi_mm2s_aclk", "m_axi_s2mm_aclk", "m_axi_sg_aclk" +Required properties for AXI DMA: +- xlnx,lengthregwidth: Should be the width of the length register as configured in h/w. + Required properties for VDMA: - xlnx,num-fstores: Should be the number of framebuffers as configured in h/w.
Signed-off-by: Andrea Merello <andrea.merello@gmail.com> --- Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt | 3 +++ 1 file changed, 3 insertions(+)