| Message ID | 9816cd272d19802ec6eeff0c7c29e85d4a0ade88.1689857295.git.andrea.collamati@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | add MCP4728 I2C DAC driver | expand |
Hey Andrea, On Thu, Jul 20, 2023 at 05:40:02PM +0200, Andrea Collamati wrote: > Add documentation for MCP4728 > > Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com> > --- > .../bindings/iio/dac/microchip,mcp4728.yaml | 48 +++++++++++++++++++ > 1 file changed, 48 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml > > diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml > new file mode 100644 > index 000000000000..6fd9be076245 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml > @@ -0,0 +1,48 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/dac/microchip,mcp4728.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Microchip MCP4728 DAC > + > +description: > + MCP4728 is a quad channel, 12-bit voltage output > + Digital-to-Analog Converter with non-volatile > + memory and I2C compatible Serial Interface. > + https://www.microchip.com/en-us/product/mcp4728 > + > +maintainers: > + - Andrea Collamati <andrea.collamati@gmail.com> > + > +properties: > + compatible: > + enum: > + - microchip,mcp4728 This can just be compatible: const: microchip,mcp47288 since you only have a single item in your enum. Otherwise, this looks good to me. Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Despite the email address, I have no knowledge of the hardware in question, I'm just reviewing the binding. Thanks, Conor. > + reg: > + maxItems: 1 > + > + vdd-supply: > + description: | > + Provides both power and acts as the reference supply on the MCP4728 > + when Internal Vref is not selected. > + > +required: > + - compatible > + - reg > + - vdd-supply > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + mcp4728@60 { > + compatible = "microchip,mcp4728"; > + reg = <0x60>; > + vdd-supply = <&vdac_vdd>; > + }; > + }; > -- > 2.34.1 >
On 20/07/2023 17:40, Andrea Collamati wrote: > Add documentation for MCP4728 > > Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com> > --- > .../bindings/iio/dac/microchip,mcp4728.yaml | 48 +++++++++++++++++++ > 1 file changed, 48 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml > > diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml > new file mode 100644 > index 000000000000..6fd9be076245 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml > @@ -0,0 +1,48 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/dac/microchip,mcp4728.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Microchip MCP4728 DAC > + > +description: > + MCP4728 is a quad channel, 12-bit voltage output > + Digital-to-Analog Converter with non-volatile > + memory and I2C compatible Serial Interface. > + https://www.microchip.com/en-us/product/mcp4728 > + > +maintainers: > + - Andrea Collamati <andrea.collamati@gmail.com> > + > +properties: > + compatible: > + enum: > + - microchip,mcp4728 This is a friendly reminder during the review process. It seems my previous comments were not fully addressed. Maybe my feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them. Thank you. > + reg: > + maxItems: 1 > + > + vdd-supply: > + description: | > + Provides both power and acts as the reference supply on the MCP4728 > + when Internal Vref is not selected. > + > +required: > + - compatible > + - reg > + - vdd-supply > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + mcp4728@60 { The same... Probably more comments were ignored, so: This is a friendly reminder during the review process. It seems my previous comments were not fully addressed. Maybe my feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them. Thank you. Best regards, Krzysztof
On 21/07/2023 10:21, Krzysztof Kozlowski wrote: >> + - | >> + i2c { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + mcp4728@60 { > > The same... Probably more comments were ignored, so: > > This is a friendly reminder during the review process. > > It seems my previous comments were not fully addressed. Maybe my > feedback got lost between the quotes, maybe you just forgot to apply it. > Please go back to the previous discussion and either implement all > requested changes or keep discussing them. Damn, this is my third comment about the same. Here was second: https://lore.kernel.org/all/5e5d1a1e-f106-9dd6-c19e-f933e8e70dd4@kernel.org/ so you nicely ignore feedback. NAK. Best regards, Krzysztof
Hi Krzysztof, On 7/21/23 10:21, Krzysztof Kozlowski wrote: >> Add documentation for MCP4728 >> >> Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com> >> --- >> .../bindings/iio/dac/microchip,mcp4728.yaml | 48 +++++++++++++++++++ >> 1 file changed, 48 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml >> >> diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml >> new file mode 100644 >> index 000000000000..6fd9be076245 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml >> @@ -0,0 +1,48 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/iio/dac/microchip,mcp4728.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Microchip MCP4728 DAC >> + >> +description: >> + MCP4728 is a quad channel, 12-bit voltage output >> + Digital-to-Analog Converter with non-volatile >> + memory and I2C compatible Serial Interface. >> + https://www.microchip.com/en-us/product/mcp4728 >> + >> +maintainers: >> + - Andrea Collamati <andrea.collamati@gmail.com> >> + >> +properties: >> + compatible: >> + enum: >> + - microchip,mcp4728 > This is a friendly reminder during the review process. Sorry but I didn't understand all your requests: - I changed in the title mcp4728 with MCP4728 - I added description but I don't know which blank line or whitespaces should be removed. Can you tell me please?
On 7/21/23 10:22, Krzysztof Kozlowski wrote: > On 21/07/2023 10:21, Krzysztof Kozlowski wrote: >>> + - | >>> + i2c { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + mcp4728@60 { >> The same... Probably more comments were ignored, so: >> >> This is a friendly reminder during the review process. >> >> It seems my previous comments were not fully addressed. Maybe my >> feedback got lost between the quotes, maybe you just forgot to apply it. >> Please go back to the previous discussion and either implement all >> requested changes or keep discussing them. Sorry, you are right. I missed to change the node name. { #address-cells = <1>; #size-cells = <0>; dac@60 { could be ok? Thank you Andrea
On 21/07/2023 13:58, Andrea Collamati wrote: > Hi Krzysztof, > > On 7/21/23 10:21, Krzysztof Kozlowski wrote: >>> Add documentation for MCP4728 >>> >>> Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com> >>> --- >>> .../bindings/iio/dac/microchip,mcp4728.yaml | 48 +++++++++++++++++++ >>> 1 file changed, 48 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml >>> new file mode 100644 >>> index 000000000000..6fd9be076245 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml >>> @@ -0,0 +1,48 @@ >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/iio/dac/microchip,mcp4728.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Microchip MCP4728 DAC >>> + >>> +description: >>> + MCP4728 is a quad channel, 12-bit voltage output >>> + Digital-to-Analog Converter with non-volatile >>> + memory and I2C compatible Serial Interface. >>> + https://www.microchip.com/en-us/product/mcp4728 >>> + >>> +maintainers: >>> + - Andrea Collamati <andrea.collamati@gmail.com> >>> + >>> +properties: >>> + compatible: >>> + enum: >>> + - microchip,mcp4728 >> This is a friendly reminder during the review process. > > Sorry but I didn't understand all your requests: > > - I changed in the title mcp4728 with MCP4728 > > - I added description > > but I don't know which blank line or whitespaces should be removed. > > Can you tell me please? You forgot to add blank line. Open example-schema and compare. Also, you had white-space errors. Editors should show it to you. Git maybe as well. Best regards, Krzysztof
Hi Conor, On 7/20/23 19:01, Conor Dooley wrote: >> Add documentation for MCP4728 >> >> Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com> >> --- >> .../bindings/iio/dac/microchip,mcp4728.yaml | 48 +++++++++++++++++++ >> 1 file changed, 48 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml >> >> diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml >> new file mode 100644 >> index 000000000000..6fd9be076245 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml >> @@ -0,0 +1,48 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/iio/dac/microchip,mcp4728.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Microchip MCP4728 DAC >> + >> +description: >> + MCP4728 is a quad channel, 12-bit voltage output >> + Digital-to-Analog Converter with non-volatile >> + memory and I2C compatible Serial Interface. >> + https://www.microchip.com/en-us/product/mcp4728 >> + >> +maintainers: >> + - Andrea Collamati <andrea.collamati@gmail.com> >> + >> +properties: >> + compatible: >> + enum: >> + - microchip,mcp4728 > This can just be > compatible: > const: microchip,mcp47288 > since you only have a single item in your enum. I will in include in v4. Thank you. Andrea
diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file mode 100644 index 000000000000..6fd9be076245 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml @@ -0,0 +1,48 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/dac/microchip,mcp4728.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Microchip MCP4728 DAC + +description: + MCP4728 is a quad channel, 12-bit voltage output + Digital-to-Analog Converter with non-volatile + memory and I2C compatible Serial Interface. + https://www.microchip.com/en-us/product/mcp4728 + +maintainers: + - Andrea Collamati <andrea.collamati@gmail.com> + +properties: + compatible: + enum: + - microchip,mcp4728 + reg: + maxItems: 1 + + vdd-supply: + description: | + Provides both power and acts as the reference supply on the MCP4728 + when Internal Vref is not selected. + +required: + - compatible + - reg + - vdd-supply + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + mcp4728@60 { + compatible = "microchip,mcp4728"; + reg = <0x60>; + vdd-supply = <&vdac_vdd>; + }; + };
Add documentation for MCP4728 Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com> --- .../bindings/iio/dac/microchip,mcp4728.yaml | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml