| Message ID | 1468526499-8840-1-git-send-email-shuahkh@osg.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shuah, On 07/14/2016 10:01 PM, Shuah Khan wrote: > Add add missing documentation for samsung,exynos4212-jpeg codec, > reorder entries to improve readability and make it easier to add > new entries. > > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > --- > Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt > index 38941db..093614c 100644 > --- a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt > +++ b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt > @@ -3,9 +3,9 @@ Samsung S5P/EXYNOS SoC series JPEG codec > Required properties: > > - compatible : should be one of: > - "samsung,s5pv210-jpeg", "samsung,exynos4210-jpeg", > - "samsung,exynos3250-jpeg", "samsung,exynos5420-jpeg", > - "samsung,exynos5433-jpeg"; > + "samsung,s5pv210-jpeg", "samsung,exynos3250-jpeg", > + "samsung,exynos4210-jpeg", "samsung,exynos4212-jpeg", > + "samsung,exynos5420-jpeg", "samsung,exynos5433-jpeg"; > - reg : address and length of the JPEG codec IP register set; > - interrupts : specifies the JPEG codec IP interrupt; > - clock-names : should contain: > Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
On 07/14/2016 10:01 PM, Shuah Khan wrote: > Add add missing documentation for samsung,exynos4212-jpeg codec, > reorder entries to improve readability and make it easier to add > new entries. > > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > --- > Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt > index 38941db..093614c 100644 > --- a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt > +++ b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt > @@ -3,9 +3,9 @@ Samsung S5P/EXYNOS SoC series JPEG codec > Required properties: > > - compatible : should be one of: > - "samsung,s5pv210-jpeg", "samsung,exynos4210-jpeg", > - "samsung,exynos3250-jpeg", "samsung,exynos5420-jpeg", > - "samsung,exynos5433-jpeg"; > + "samsung,s5pv210-jpeg", "samsung,exynos3250-jpeg", > + "samsung,exynos4210-jpeg", "samsung,exynos4212-jpeg", > + "samsung,exynos5420-jpeg", "samsung,exynos5433-jpeg"; I am not convinced that this is needed because exynos4212-jpeg is equal to exynos4210-jpeg... It is a little bit weird because Jacek's commit 3246fdaa0ac2d9 ("[media] s5p-jpeg: Add support for Exynos3250 SoC") changed the driver data for Exynos4210 from S5P to Exynos4 without explaining it... However if these compatibles are exactly equal then only one should be preferred. It makes everything easier. Second can be still documented e.g. as deprecated. Best regards, Krzysztof
Hi Krzysztof, On 07/15/2016 09:28 AM, Krzysztof Kozlowski wrote: > On 07/14/2016 10:01 PM, Shuah Khan wrote: >> Add add missing documentation for samsung,exynos4212-jpeg codec, >> reorder entries to improve readability and make it easier to add >> new entries. >> >> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> >> --- >> Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt >> index 38941db..093614c 100644 >> --- a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt >> +++ b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt >> @@ -3,9 +3,9 @@ Samsung S5P/EXYNOS SoC series JPEG codec >> Required properties: >> >> - compatible : should be one of: >> - "samsung,s5pv210-jpeg", "samsung,exynos4210-jpeg", >> - "samsung,exynos3250-jpeg", "samsung,exynos5420-jpeg", >> - "samsung,exynos5433-jpeg"; >> + "samsung,s5pv210-jpeg", "samsung,exynos3250-jpeg", >> + "samsung,exynos4210-jpeg", "samsung,exynos4212-jpeg", >> + "samsung,exynos5420-jpeg", "samsung,exynos5433-jpeg"; > > I am not convinced that this is needed because exynos4212-jpeg is equal > to exynos4210-jpeg... It is a little bit weird because Jacek's commit > 3246fdaa0ac2d9 ("[media] s5p-jpeg: Add support for Exynos3250 SoC") > changed the driver data for Exynos4210 from S5P to Exynos4 without > explaining it... It was probably a bug fix since 4210 and 4212 have the same JPEG IP. I agree that it isn't best practice to smuggle bug fixes like that. > However if these compatibles are exactly equal then > only one should be preferred. It makes everything easier. Second can be > still documented e.g. as deprecated. Still, both of them are present in the driver. Shouldn't it be reflected in the documentation?
On 07/15/2016 10:14 AM, Jacek Anaszewski wrote: >> However if these compatibles are exactly equal then >> only one should be preferred. It makes everything easier. Second can be >> still documented e.g. as deprecated. > > Still, both of them are present in the driver. Shouldn't it be reflected > in the documentation? Right, it is a good practice, so how about: - compatible : should be one of: "samsung,s5pv210-jpeg", "samsung,exynos3250-jpeg", "samsung,exynos4210-jpeg", "samsung,exynos5420-jpeg", "samsung,exynos5433-jpeg"; Deprecated: "samsung,exynos4212-jpeg" (or any other formatting) plus update to DTS changing it to 4210? Best regards, Krzysztof
On 07/15/2016 10:17 AM, Krzysztof Kozlowski wrote: > On 07/15/2016 10:14 AM, Jacek Anaszewski wrote: >>> However if these compatibles are exactly equal then >>> only one should be preferred. It makes everything easier. Second can be >>> still documented e.g. as deprecated. >> >> Still, both of them are present in the driver. Shouldn't it be reflected >> in the documentation? > > Right, it is a good practice, so how about: > > - compatible : should be one of: > "samsung,s5pv210-jpeg", "samsung,exynos3250-jpeg", > "samsung,exynos4210-jpeg", "samsung,exynos5420-jpeg", > "samsung,exynos5433-jpeg"; > > Deprecated: "samsung,exynos4212-jpeg" > > (or any other formatting) > plus update to DTS changing it to 4210? Why newer 4212 version should be made deprecated?
On 07/15/2016 10:28 AM, Jacek Anaszewski wrote: > On 07/15/2016 10:17 AM, Krzysztof Kozlowski wrote: >> On 07/15/2016 10:14 AM, Jacek Anaszewski wrote: >>>> However if these compatibles are exactly equal then >>>> only one should be preferred. It makes everything easier. Second can be >>>> still documented e.g. as deprecated. >>> >>> Still, both of them are present in the driver. Shouldn't it be reflected >>> in the documentation? >> >> Right, it is a good practice, so how about: >> >> - compatible : should be one of: >> "samsung,s5pv210-jpeg", "samsung,exynos3250-jpeg", >> "samsung,exynos4210-jpeg", "samsung,exynos5420-jpeg", >> "samsung,exynos5433-jpeg"; >> >> Deprecated: "samsung,exynos4212-jpeg" >> >> (or any other formatting) >> plus update to DTS changing it to 4210? > > Why newer 4212 version should be made deprecated? I don't mind the other way. However it seems logical to me that newer chip is compatible with existing one so the existing one (older) is used. When adding support for new devices, for most of re-usable drivers we use old compatibles. But as I said, it doesn't really matter to me. BR, Krzysztof
On 07/15/2016 10:33 AM, Krzysztof Kozlowski wrote: > On 07/15/2016 10:28 AM, Jacek Anaszewski wrote: >> On 07/15/2016 10:17 AM, Krzysztof Kozlowski wrote: >>> On 07/15/2016 10:14 AM, Jacek Anaszewski wrote: >>>>> However if these compatibles are exactly equal then >>>>> only one should be preferred. It makes everything easier. Second can be >>>>> still documented e.g. as deprecated. >>>> >>>> Still, both of them are present in the driver. Shouldn't it be reflected >>>> in the documentation? >>> >>> Right, it is a good practice, so how about: >>> >>> - compatible : should be one of: >>> "samsung,s5pv210-jpeg", "samsung,exynos3250-jpeg", >>> "samsung,exynos4210-jpeg", "samsung,exynos5420-jpeg", >>> "samsung,exynos5433-jpeg"; >>> >>> Deprecated: "samsung,exynos4212-jpeg" >>> >>> (or any other formatting) >>> plus update to DTS changing it to 4210? >> >> Why newer 4212 version should be made deprecated? > > I don't mind the other way. However it seems logical to me that newer > chip is compatible with existing one so the existing one (older) is > used. When adding support for new devices, for most of re-usable drivers > we use old compatibles. But as I said, it doesn't really matter to me. Frankly speaking marking a compatible deprecated looks weird to me. It can be interpreted in the way that the device itself is deprecated or it is not fully reliable. I'd just accept the patch in the original form.
On 07/15/2016 11:18 AM, Jacek Anaszewski wrote: > On 07/15/2016 10:33 AM, Krzysztof Kozlowski wrote: >> On 07/15/2016 10:28 AM, Jacek Anaszewski wrote: >>> On 07/15/2016 10:17 AM, Krzysztof Kozlowski wrote: >>>> On 07/15/2016 10:14 AM, Jacek Anaszewski wrote: >>>>>> However if these compatibles are exactly equal then >>>>>> only one should be preferred. It makes everything easier. Second >>>>>> can be >>>>>> still documented e.g. as deprecated. >>>>> >>>>> Still, both of them are present in the driver. Shouldn't it be >>>>> reflected >>>>> in the documentation? >>>> >>>> Right, it is a good practice, so how about: >>>> >>>> - compatible : should be one of: >>>> "samsung,s5pv210-jpeg", "samsung,exynos3250-jpeg", >>>> "samsung,exynos4210-jpeg", "samsung,exynos5420-jpeg", >>>> "samsung,exynos5433-jpeg"; >>>> >>>> Deprecated: "samsung,exynos4212-jpeg" >>>> >>>> (or any other formatting) >>>> plus update to DTS changing it to 4210? >>> >>> Why newer 4212 version should be made deprecated? >> >> I don't mind the other way. However it seems logical to me that newer >> chip is compatible with existing one so the existing one (older) is >> used. When adding support for new devices, for most of re-usable drivers >> we use old compatibles. But as I said, it doesn't really matter to me. > > Frankly speaking marking a compatible deprecated looks weird to me. > It can be interpreted in the way that the device itself is deprecated > or it is not fully reliable. Marking a compatible or a property deprecated is commonly used, if needed of course. It has nothing to do with device being deprecated. This is documentation for bindings and deprecation affects only bindings. It is not weird or something strange. We already did this for some of Exynos compatibles (later removing them) and there are quite many examples in Documentation already. Best regards, Krzysztof > I'd just accept the patch in the original > form.
On 07/15/2016 11:30 AM, Krzysztof Kozlowski wrote: > On 07/15/2016 11:18 AM, Jacek Anaszewski wrote: >> On 07/15/2016 10:33 AM, Krzysztof Kozlowski wrote: >>> On 07/15/2016 10:28 AM, Jacek Anaszewski wrote: >>>> On 07/15/2016 10:17 AM, Krzysztof Kozlowski wrote: >>>>> On 07/15/2016 10:14 AM, Jacek Anaszewski wrote: >>>>>>> However if these compatibles are exactly equal then >>>>>>> only one should be preferred. It makes everything easier. Second >>>>>>> can be >>>>>>> still documented e.g. as deprecated. >>>>>> >>>>>> Still, both of them are present in the driver. Shouldn't it be >>>>>> reflected >>>>>> in the documentation? >>>>> >>>>> Right, it is a good practice, so how about: >>>>> >>>>> - compatible : should be one of: >>>>> "samsung,s5pv210-jpeg", "samsung,exynos3250-jpeg", >>>>> "samsung,exynos4210-jpeg", "samsung,exynos5420-jpeg", >>>>> "samsung,exynos5433-jpeg"; >>>>> >>>>> Deprecated: "samsung,exynos4212-jpeg" >>>>> >>>>> (or any other formatting) >>>>> plus update to DTS changing it to 4210? >>>> >>>> Why newer 4212 version should be made deprecated? >>> >>> I don't mind the other way. However it seems logical to me that newer >>> chip is compatible with existing one so the existing one (older) is >>> used. When adding support for new devices, for most of re-usable drivers >>> we use old compatibles. But as I said, it doesn't really matter to me. >> >> Frankly speaking marking a compatible deprecated looks weird to me. >> It can be interpreted in the way that the device itself is deprecated >> or it is not fully reliable. > > Marking a compatible or a property deprecated is commonly used, if > needed of course. It has nothing to do with device being deprecated. > This is documentation for bindings and deprecation affects only > bindings. It is not weird or something strange. We already did this for > some of Exynos compatibles (later removing them) and there are quite > many examples in Documentation already. If this is broadly accepted pattern, then I will not argue against. Let's proceed as you proposed.
Hi Krzysztof and Jacek, Thanks for the review. On 07/15/2016 03:37 AM, Jacek Anaszewski wrote: > On 07/15/2016 11:30 AM, Krzysztof Kozlowski wrote: >> On 07/15/2016 11:18 AM, Jacek Anaszewski wrote: >>> On 07/15/2016 10:33 AM, Krzysztof Kozlowski wrote: >>>> On 07/15/2016 10:28 AM, Jacek Anaszewski wrote: >>>>> On 07/15/2016 10:17 AM, Krzysztof Kozlowski wrote: >>>>>> On 07/15/2016 10:14 AM, Jacek Anaszewski wrote: >>>>>>>> However if these compatibles are exactly equal then >>>>>>>> only one should be preferred. It makes everything easier. Second >>>>>>>> can be >>>>>>>> still documented e.g. as deprecated. >>>>>>> >>>>>>> Still, both of them are present in the driver. Shouldn't it be >>>>>>> reflected >>>>>>> in the documentation? >>>>>> >>>>>> Right, it is a good practice, so how about: >>>>>> >>>>>> - compatible : should be one of: >>>>>> "samsung,s5pv210-jpeg", "samsung,exynos3250-jpeg", >>>>>> "samsung,exynos4210-jpeg", "samsung,exynos5420-jpeg", >>>>>> "samsung,exynos5433-jpeg"; >>>>>> >>>>>> Deprecated: "samsung,exynos4212-jpeg" >>>>>> >>>>>> (or any other formatting) >>>>>> plus update to DTS changing it to 4210? I am sending the v2 doc patch that adds samsung,exynos4212-jpeg as deprecated to begin with. thanks, -- Shuah
diff --git a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt index 38941db..093614c 100644 --- a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt +++ b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt @@ -3,9 +3,9 @@ Samsung S5P/EXYNOS SoC series JPEG codec Required properties: - compatible : should be one of: - "samsung,s5pv210-jpeg", "samsung,exynos4210-jpeg", - "samsung,exynos3250-jpeg", "samsung,exynos5420-jpeg", - "samsung,exynos5433-jpeg"; + "samsung,s5pv210-jpeg", "samsung,exynos3250-jpeg", + "samsung,exynos4210-jpeg", "samsung,exynos4212-jpeg", + "samsung,exynos5420-jpeg", "samsung,exynos5433-jpeg"; - reg : address and length of the JPEG codec IP register set; - interrupts : specifies the JPEG codec IP interrupt; - clock-names : should contain:
Add add missing documentation for samsung,exynos4212-jpeg codec, reorder entries to improve readability and make it easier to add new entries. Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> --- Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)