| Message ID | 20230606193507.35024-1-AVKrasnov@sberdevices.ru |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | [v1] dt-bindings: nand: meson: Fix 'nand-rb' property | expand |
On Tue, 06 Jun 2023 22:35:07 +0300, Arseniy Krasnov wrote: > Add description of 'nand-rb' property. Use "Fixes" because this property > must be supported since the beginning. For this controller 'nand-rb' is > stored in the controller node (not in chip), because it has only single > r/b wire for all chips. > > Fixes: fbc00b5e746f ("dt-bindings: nand: meson: convert txt to yaml") > Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > --- > Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml | 3 +++ > 1 file changed, 3 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: nand-rb: size (5) error for type uint32-array /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.example.dtb: nand-controller@ffe07800: nand-rb: b'true\x00' is not of type 'object', 'array', 'boolean', 'null' From schema: /usr/local/lib/python3.10/dist-packages/dtschema/schemas/dt-core.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230606193507.35024-1-AVKrasnov@sberdevices.ru The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
Hi Arseniy, AVKrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 22:35:07 +0300: > Add description of 'nand-rb' property. Use "Fixes" because this property > must be supported since the beginning. For this controller 'nand-rb' is > stored in the controller node (not in chip), because it has only single > r/b wire for all chips. Sorry if I mislead you in the first place, but you could definitely have two chips and only one with RB wired. It needs to be defined in the chips. Please keep the bindings and driver changes related to this in the same series. > > Fixes: fbc00b5e746f ("dt-bindings: nand: meson: convert txt to yaml") > Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > --- > Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml > index 28fb9a7dd70f..866edf800b81 100644 > --- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml > +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml > @@ -37,6 +37,8 @@ properties: > - const: core > - const: device > > + nand-rb: true > + > patternProperties: > "^nand@[0-7]$": > type: object > @@ -81,6 +83,7 @@ examples: > > pinctrl-0 = <&nand_pins>; > pinctrl-names = "default"; > + nand-rb = "true"; > > #address-cells = <1>; > #size-cells = <0>; Thanks, Miquèl
Hello Miquel, On 07.06.2023 10:58, Miquel Raynal wrote: > Hi Arseniy, > > AVKrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 22:35:07 +0300: > >> Add description of 'nand-rb' property. Use "Fixes" because this property >> must be supported since the beginning. For this controller 'nand-rb' is >> stored in the controller node (not in chip), because it has only single >> r/b wire for all chips. > > Sorry if I mislead you in the first place, but you could definitely > have two chips and only one with RB wired. It needs to be defined in > the chips. Ok, so to clarify: is it ok, that in bindings this property will be placed in the chip, but in driver, i'm trying to read it from the controller node (thus in dts file it will be also in controller node)? Because in driver there is no sense to store this value in chip structure. In fact, in driver, I can read it from the chip node, but set 'no_rb_pin' flag in the controller structure. Thanks, Arseniy > > Please keep the bindings and driver changes related to this in the same > series. Ack > >> >> Fixes: fbc00b5e746f ("dt-bindings: nand: meson: convert txt to yaml") >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >> --- >> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml >> index 28fb9a7dd70f..866edf800b81 100644 >> --- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml >> +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml >> @@ -37,6 +37,8 @@ properties: >> - const: core >> - const: device >> >> + nand-rb: true >> + >> patternProperties: >> "^nand@[0-7]$": >> type: object >> @@ -81,6 +83,7 @@ examples: >> >> pinctrl-0 = <&nand_pins>; >> pinctrl-names = "default"; >> + nand-rb = "true"; >> >> #address-cells = <1>; >> #size-cells = <0>; > > > Thanks, > Miquèl
On 07/06/2023 10:40, Arseniy Krasnov wrote: > Hello Miquel, > > On 07.06.2023 10:58, Miquel Raynal wrote: > >> Hi Arseniy, >> >> AVKrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 22:35:07 +0300: >> >>> Add description of 'nand-rb' property. Use "Fixes" because this property >>> must be supported since the beginning. For this controller 'nand-rb' is >>> stored in the controller node (not in chip), because it has only single >>> r/b wire for all chips. >> >> Sorry if I mislead you in the first place, but you could definitely >> have two chips and only one with RB wired. It needs to be defined in >> the chips. > > Ok, so to clarify: is it ok, that in bindings this property will be placed in the > chip, but in driver, i'm trying to read it from the controller node (thus in > dts file it will be also in controller node)? No, because how would your DTS pass validation? I understand you did not test the bindings, but this will improve, right? > Because in driver there is no sense > to store this value in chip structure. Driver does not shape the DTS. Hardware does. Best regards, Krzysztof
On 07.06.2023 11:53, Krzysztof Kozlowski wrote: > On 07/06/2023 10:40, Arseniy Krasnov wrote: >> Hello Miquel, >> >> On 07.06.2023 10:58, Miquel Raynal wrote: >> >>> Hi Arseniy, >>> >>> AVKrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 22:35:07 +0300: >>> >>>> Add description of 'nand-rb' property. Use "Fixes" because this property >>>> must be supported since the beginning. For this controller 'nand-rb' is >>>> stored in the controller node (not in chip), because it has only single >>>> r/b wire for all chips. >>> >>> Sorry if I mislead you in the first place, but you could definitely >>> have two chips and only one with RB wired. It needs to be defined in >>> the chips. >> >> Ok, so to clarify: is it ok, that in bindings this property will be placed in the >> chip, but in driver, i'm trying to read it from the controller node (thus in >> dts file it will be also in controller node)? > > No, because how would your DTS pass validation? I understand you did not > test the bindings, but this will improve, right? Ok, i'll follow DTS layout in the driver, "test the bindings" You mean "make dt_binding_check"? > >> Because in driver there is no sense >> to store this value in chip structure. > > Driver does not shape the DTS. Hardware does. Ok, Thanks Thanks, Arseniy > > > > Best regards, > Krzysztof >
On 07.06.2023 12:08, Krzysztof Kozlowski wrote: > On 07/06/2023 10:57, Arseniy Krasnov wrote: >> >> >> On 07.06.2023 11:53, Krzysztof Kozlowski wrote: >>> On 07/06/2023 10:40, Arseniy Krasnov wrote: >>>> Hello Miquel, >>>> >>>> On 07.06.2023 10:58, Miquel Raynal wrote: >>>> >>>>> Hi Arseniy, >>>>> >>>>> AVKrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 22:35:07 +0300: >>>>> >>>>>> Add description of 'nand-rb' property. Use "Fixes" because this property >>>>>> must be supported since the beginning. For this controller 'nand-rb' is >>>>>> stored in the controller node (not in chip), because it has only single >>>>>> r/b wire for all chips. >>>>> >>>>> Sorry if I mislead you in the first place, but you could definitely >>>>> have two chips and only one with RB wired. It needs to be defined in >>>>> the chips. >>>> >>>> Ok, so to clarify: is it ok, that in bindings this property will be placed in the >>>> chip, but in driver, i'm trying to read it from the controller node (thus in >>>> dts file it will be also in controller node)? >>> >>> No, because how would your DTS pass validation? I understand you did not >>> test the bindings, but this will improve, right? >> >> Ok, i'll follow DTS layout in the driver, "test the bindings" You mean "make dt_binding_check"? > > Yes. They were sent without testing. > > But please also test your DTS with dtbs_check. Got it! Thanks, Arseniy > > > Best regards, > Krzysztof >
On 07/06/2023 10:57, Arseniy Krasnov wrote: > > > On 07.06.2023 11:53, Krzysztof Kozlowski wrote: >> On 07/06/2023 10:40, Arseniy Krasnov wrote: >>> Hello Miquel, >>> >>> On 07.06.2023 10:58, Miquel Raynal wrote: >>> >>>> Hi Arseniy, >>>> >>>> AVKrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 22:35:07 +0300: >>>> >>>>> Add description of 'nand-rb' property. Use "Fixes" because this property >>>>> must be supported since the beginning. For this controller 'nand-rb' is >>>>> stored in the controller node (not in chip), because it has only single >>>>> r/b wire for all chips. >>>> >>>> Sorry if I mislead you in the first place, but you could definitely >>>> have two chips and only one with RB wired. It needs to be defined in >>>> the chips. >>> >>> Ok, so to clarify: is it ok, that in bindings this property will be placed in the >>> chip, but in driver, i'm trying to read it from the controller node (thus in >>> dts file it will be also in controller node)? >> >> No, because how would your DTS pass validation? I understand you did not >> test the bindings, but this will improve, right? > > Ok, i'll follow DTS layout in the driver, "test the bindings" You mean "make dt_binding_check"? Yes. They were sent without testing. But please also test your DTS with dtbs_check. Best regards, Krzysztof
Hi Arseniy, avkrasnov@sberdevices.ru wrote on Wed, 7 Jun 2023 12:04:29 +0300: > On 07.06.2023 12:08, Krzysztof Kozlowski wrote: > > On 07/06/2023 10:57, Arseniy Krasnov wrote: > >> > >> > >> On 07.06.2023 11:53, Krzysztof Kozlowski wrote: > >>> On 07/06/2023 10:40, Arseniy Krasnov wrote: > >>>> Hello Miquel, > >>>> > >>>> On 07.06.2023 10:58, Miquel Raynal wrote: > >>>> > >>>>> Hi Arseniy, > >>>>> > >>>>> AVKrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 22:35:07 +0300: > >>>>> > >>>>>> Add description of 'nand-rb' property. Use "Fixes" because this property > >>>>>> must be supported since the beginning. For this controller 'nand-rb' is > >>>>>> stored in the controller node (not in chip), because it has only single > >>>>>> r/b wire for all chips. > >>>>> > >>>>> Sorry if I mislead you in the first place, but you could definitely > >>>>> have two chips and only one with RB wired. It needs to be defined in > >>>>> the chips. > >>>> > >>>> Ok, so to clarify: is it ok, that in bindings this property will be placed in the > >>>> chip, but in driver, i'm trying to read it from the controller node (thus in > >>>> dts file it will be also in controller node)? The bindings and your driver internal representation are two different things. Anyway, as mentioned above, wiring the RB line to one die and not the other would be valid hardware design and would require the rb property to be in the chip node. Please perform a per-chip property read in the driver as well. > >>> > >>> No, because how would your DTS pass validation? I understand you did not > >>> test the bindings, but this will improve, right? > >> > >> Ok, i'll follow DTS layout in the driver, "test the bindings" You mean "make dt_binding_check"? > > > > Yes. They were sent without testing. > > > > But please also test your DTS with dtbs_check. > > Got it! > > Thanks, Arseniy > > > > > > > Best regards, > > Krzysztof > > Thanks, Miquèl
On 07.06.2023 12:36, Miquel Raynal wrote: > Hi Arseniy, > > avkrasnov@sberdevices.ru wrote on Wed, 7 Jun 2023 12:04:29 +0300: > >> On 07.06.2023 12:08, Krzysztof Kozlowski wrote: >>> On 07/06/2023 10:57, Arseniy Krasnov wrote: >>>> >>>> >>>> On 07.06.2023 11:53, Krzysztof Kozlowski wrote: >>>>> On 07/06/2023 10:40, Arseniy Krasnov wrote: >>>>>> Hello Miquel, >>>>>> >>>>>> On 07.06.2023 10:58, Miquel Raynal wrote: >>>>>> >>>>>>> Hi Arseniy, >>>>>>> >>>>>>> AVKrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 22:35:07 +0300: >>>>>>> >>>>>>>> Add description of 'nand-rb' property. Use "Fixes" because this property >>>>>>>> must be supported since the beginning. For this controller 'nand-rb' is >>>>>>>> stored in the controller node (not in chip), because it has only single >>>>>>>> r/b wire for all chips. >>>>>>> >>>>>>> Sorry if I mislead you in the first place, but you could definitely >>>>>>> have two chips and only one with RB wired. It needs to be defined in >>>>>>> the chips. >>>>>> >>>>>> Ok, so to clarify: is it ok, that in bindings this property will be placed in the >>>>>> chip, but in driver, i'm trying to read it from the controller node (thus in >>>>>> dts file it will be also in controller node)? > > The bindings and your driver internal representation are two different > things. Anyway, as mentioned above, wiring the RB line to one die and > not the other would be valid hardware design and would require the rb > property to be in the chip node. Please perform a per-chip property read > in the driver as well. Done, I resend both patches (bindings + driver update) as a single patchset. Your review comments for driver code were also fixed. > >>>>> >>>>> No, because how would your DTS pass validation? I understand you did not >>>>> test the bindings, but this will improve, right? >>>> >>>> Ok, i'll follow DTS layout in the driver, "test the bindings" You mean "make dt_binding_check"? >>> >>> Yes. They were sent without testing. >>> >>> But please also test your DTS with dtbs_check. Done Thanks, Arseniy >> >> Got it! >> >> Thanks, Arseniy >> >>> >>> >>> Best regards, >>> Krzysztof >>> > > > Thanks, > Miquèl
On 07.06.2023 17:58, Krzysztof Kozlowski wrote: > On 07/06/2023 16:52, Arseniy Krasnov wrote: >> >> >> On 07.06.2023 12:36, Miquel Raynal wrote: >>> Hi Arseniy, >>> >>> avkrasnov@sberdevices.ru wrote on Wed, 7 Jun 2023 12:04:29 +0300: >>> >>>> On 07.06.2023 12:08, Krzysztof Kozlowski wrote: >>>>> On 07/06/2023 10:57, Arseniy Krasnov wrote: >>>>>> >>>>>> >>>>>> On 07.06.2023 11:53, Krzysztof Kozlowski wrote: >>>>>>> On 07/06/2023 10:40, Arseniy Krasnov wrote: >>>>>>>> Hello Miquel, >>>>>>>> >>>>>>>> On 07.06.2023 10:58, Miquel Raynal wrote: >>>>>>>> >>>>>>>>> Hi Arseniy, >>>>>>>>> >>>>>>>>> AVKrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 22:35:07 +0300: >>>>>>>>> >>>>>>>>>> Add description of 'nand-rb' property. Use "Fixes" because this property >>>>>>>>>> must be supported since the beginning. For this controller 'nand-rb' is >>>>>>>>>> stored in the controller node (not in chip), because it has only single >>>>>>>>>> r/b wire for all chips. >>>>>>>>> >>>>>>>>> Sorry if I mislead you in the first place, but you could definitely >>>>>>>>> have two chips and only one with RB wired. It needs to be defined in >>>>>>>>> the chips. >>>>>>>> >>>>>>>> Ok, so to clarify: is it ok, that in bindings this property will be placed in the >>>>>>>> chip, but in driver, i'm trying to read it from the controller node (thus in >>>>>>>> dts file it will be also in controller node)? >>> >>> The bindings and your driver internal representation are two different >>> things. Anyway, as mentioned above, wiring the RB line to one die and >>> not the other would be valid hardware design and would require the rb >>> property to be in the chip node. Please perform a per-chip property read >>> in the driver as well. >> >> Done, I resend both patches (bindings + driver update) as a single patchset. Your review comments >> for driver code were also fixed. > > No, please send new version, not the same. New version means with fixed > comments and with patch changelog. Sorry, Yes, I mean new version, here it is: https://lore.kernel.org/linux-mtd/20230607145026.2899547-1-AVKrasnov@sberdevices.ru/ There I fixed bindings and tested it. Thanks, Arseniy > > Best regards, > Krzysztof >
On 07/06/2023 16:52, Arseniy Krasnov wrote: > > > On 07.06.2023 12:36, Miquel Raynal wrote: >> Hi Arseniy, >> >> avkrasnov@sberdevices.ru wrote on Wed, 7 Jun 2023 12:04:29 +0300: >> >>> On 07.06.2023 12:08, Krzysztof Kozlowski wrote: >>>> On 07/06/2023 10:57, Arseniy Krasnov wrote: >>>>> >>>>> >>>>> On 07.06.2023 11:53, Krzysztof Kozlowski wrote: >>>>>> On 07/06/2023 10:40, Arseniy Krasnov wrote: >>>>>>> Hello Miquel, >>>>>>> >>>>>>> On 07.06.2023 10:58, Miquel Raynal wrote: >>>>>>> >>>>>>>> Hi Arseniy, >>>>>>>> >>>>>>>> AVKrasnov@sberdevices.ru wrote on Tue, 6 Jun 2023 22:35:07 +0300: >>>>>>>> >>>>>>>>> Add description of 'nand-rb' property. Use "Fixes" because this property >>>>>>>>> must be supported since the beginning. For this controller 'nand-rb' is >>>>>>>>> stored in the controller node (not in chip), because it has only single >>>>>>>>> r/b wire for all chips. >>>>>>>> >>>>>>>> Sorry if I mislead you in the first place, but you could definitely >>>>>>>> have two chips and only one with RB wired. It needs to be defined in >>>>>>>> the chips. >>>>>>> >>>>>>> Ok, so to clarify: is it ok, that in bindings this property will be placed in the >>>>>>> chip, but in driver, i'm trying to read it from the controller node (thus in >>>>>>> dts file it will be also in controller node)? >> >> The bindings and your driver internal representation are two different >> things. Anyway, as mentioned above, wiring the RB line to one die and >> not the other would be valid hardware design and would require the rb >> property to be in the chip node. Please perform a per-chip property read >> in the driver as well. > > Done, I resend both patches (bindings + driver update) as a single patchset. Your review comments > for driver code were also fixed. No, please send new version, not the same. New version means with fixed comments and with patch changelog. Best regards, Krzysztof
On 07/06/2023 16:55, Arseniy Krasnov wrote: >>>> The bindings and your driver internal representation are two different >>>> things. Anyway, as mentioned above, wiring the RB line to one die and >>>> not the other would be valid hardware design and would require the rb >>>> property to be in the chip node. Please perform a per-chip property read >>>> in the driver as well. >>> >>> Done, I resend both patches (bindings + driver update) as a single patchset. Your review comments >>> for driver code were also fixed. >> >> No, please send new version, not the same. New version means with fixed >> comments and with patch changelog. > > Sorry, Yes, I mean new version, here it is: > https://lore.kernel.org/linux-mtd/20230607145026.2899547-1-AVKrasnov@sberdevices.ru/ > > There I fixed bindings and tested it. I still see v1 and there is no changelog. So it's the same? Best regards, Krzysztof
On 07.06.2023 18:07, Krzysztof Kozlowski wrote: > On 07/06/2023 16:55, Arseniy Krasnov wrote: >>>>> The bindings and your driver internal representation are two different >>>>> things. Anyway, as mentioned above, wiring the RB line to one die and >>>>> not the other would be valid hardware design and would require the rb >>>>> property to be in the chip node. Please perform a per-chip property read >>>>> in the driver as well. >>>> >>>> Done, I resend both patches (bindings + driver update) as a single patchset. Your review comments >>>> for driver code were also fixed. >>> >>> No, please send new version, not the same. New version means with fixed >>> comments and with patch changelog. >> >> Sorry, Yes, I mean new version, here it is: >> https://lore.kernel.org/linux-mtd/20230607145026.2899547-1-AVKrasnov@sberdevices.ru/ >> >> There I fixed bindings and tested it. > > I still see v1 and there is no changelog. So it's the same? No, v1 is for new patchset from the link above. Both patches (bindings and driver) are updated there. I attached changelog here as reply for cover letter of the patchset: https://lore.kernel.org/linux-mtd/a1e048aa-ec64-bd0b-aa17-e3e9bdf18090@sberdevices.ru/ Thanks, Arseniy > > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml index 28fb9a7dd70f..866edf800b81 100644 --- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml @@ -37,6 +37,8 @@ properties: - const: core - const: device + nand-rb: true + patternProperties: "^nand@[0-7]$": type: object @@ -81,6 +83,7 @@ examples: pinctrl-0 = <&nand_pins>; pinctrl-names = "default"; + nand-rb = "true"; #address-cells = <1>; #size-cells = <0>;
Add description of 'nand-rb' property. Use "Fixes" because this property must be supported since the beginning. For this controller 'nand-rb' is stored in the controller node (not in chip), because it has only single r/b wire for all chips. Fixes: fbc00b5e746f ("dt-bindings: nand: meson: convert txt to yaml") Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> --- Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml | 3 +++ 1 file changed, 3 insertions(+)