Signed-off-by: Charles Keepax <[email protected]>
---
.../devicetree/bindings/mfd/cirrus,lochnagar.txt | 85 ----------
.../devicetree/bindings/mfd/cirrus,lochnagar.yaml | 183 +++++++++++++++++++++
MAINTAINERS | 12 +-
3 files changed, 189 insertions(+), 91 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt
create mode 100644 Documentation/devicetree/bindings/mfd/cirrus,lochnagar.yaml
diff --git a/Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt b/Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt
deleted file mode 100644
index 3bf92ad37fa1b..0000000000000
--- a/Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt
+++ /dev/null
@@ -1,85 +0,0 @@
-Cirrus Logic Lochnagar Audio Development Board
-
-Lochnagar is an evaluation and development board for Cirrus Logic
-Smart CODEC and Amp devices. It allows the connection of most Cirrus
-Logic devices on mini-cards, as well as allowing connection of
-various application processor systems to provide a full evaluation
-platform. Audio system topology, clocking and power can all be
-controlled through the Lochnagar, allowing the device under test
-to be used in a variety of possible use cases.
-
-Also see these documents for generic binding information:
- [1] GPIO : ../gpio/gpio.txt
-
-And these for relevant defines:
- [2] include/dt-bindings/pinctrl/lochnagar.h
- [3] include/dt-bindings/clock/lochnagar.h
-
-And these documents for the required sub-node binding details:
- [4] Clock: ../clock/cirrus,lochnagar.txt
- [5] Pinctrl: ../pinctrl/cirrus,lochnagar.txt
- [6] Regulator: ../regulator/cirrus,lochnagar.txt
- [7] Sound: ../sound/cirrus,lochnagar.txt
- [8] Hardware Monitor: ../hwmon/cirrus,lochnagar.txt
-
-Required properties:
-
- - compatible : One of the following strings:
- "cirrus,lochnagar1"
- "cirrus,lochnagar2"
-
- - reg : I2C slave address
-
- - reset-gpios : Reset line to the Lochnagar, see [1].
-
-Required sub-nodes:
-
- - lochnagar-clk : Binding for the clocking components, see [4].
-
- - lochnagar-pinctrl : Binding for the pin control components, see [5].
-
-Optional sub-nodes:
-
- - Bindings for the regulator components, see [6]. Only available on
- Lochnagar 2.
-
- - lochnagar-sc : Binding for the sound card components, see [7].
- Only available on Lochnagar 2.
- - lochnagar-hwmon : Binding for the hardware monitor components, see [8].
- Only available on Lochnagar 2.
-
-Optional properties:
-
- - present-gpios : Host present line, indicating the presence of a
- host system, see [1]. This can be omitted if the present line is
- tied in hardware.
-
-Example:
-
-lochnagar: lochnagar@22 {
- compatible = "cirrus,lochnagar2";
- reg = <0x22>;
-
- reset-gpios = <&gpio0 55 0>;
- present-gpios = <&gpio0 60 0>;
-
- lochnagar-clk {
- compatible = "cirrus,lochnagar2-clk";
- ...
- };
-
- lochnagar-pinctrl {
- compatible = "cirrus,lochnagar-pinctrl";
- ...
- };
-
- lochnagar-sc {
- compatible = "cirrus,lochnagar2-soundcard";
- ...
- };
-
- lochnagar-hwmon {
- compatible = "cirrus,lochnagar2-hwmon";
- ...
- };
-};
diff --git a/Documentation/devicetree/bindings/mfd/cirrus,lochnagar.yaml b/Documentation/devicetree/bindings/mfd/cirrus,lochnagar.yaml
new file mode 100644
index 0000000000000..b2967f141b2af
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/cirrus,lochnagar.yaml
@@ -0,0 +1,183 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/cirrus,lochnagar.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cirrus Logic Lochnagar Audio Development Board
+
+maintainers:
+ - [email protected]
+
+description: |
+ Lochnagar is an evaluation and development board for Cirrus Logic
+ Smart CODEC and Amp devices. It allows the connection of most Cirrus
+ Logic devices on mini-cards, as well as allowing connection of
+ various application processor systems to provide a full evaluation
+ platform. Audio system topology, clocking and power can all be
+ controlled through the Lochnagar, allowing the device under test
+ to be used in a variety of possible use cases.
+
+ Also see these documents for generic binding information:
+ [1] GPIO : ../gpio/gpio.txt
+
+ And these for relevant defines:
+ [2] include/dt-bindings/pinctrl/lochnagar.h
+ [3] include/dt-bindings/clock/lochnagar.h
+
+ And these documents for the required sub-node binding details:
+ [4] Clock: ../clock/cirrus,lochnagar.yaml
+ [5] Pinctrl: ../pinctrl/cirrus,lochnagar.yaml
+ [6] Regulator: ../regulator/cirrus,lochnagar.yaml
+ [7] Sound: ../sound/cirrus,lochnagar.yaml
+ [8] Hardware Monitor: ../hwmon/cirrus,lochnagar.yaml
+
+allOf:
+ - $ref: /schemas/clock/cirrus,lochnagar.yaml#
+ - $ref: /schemas/pinctrl/cirrus,lochnagar.yaml#
+ - $ref: /schemas/regulator/cirrus,lochnagar.yaml#
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - cirrus,lochnagar2
+ then:
+ allOf:
+ - $ref: /schemas/sound/cirrus,lochnagar.yaml#
+ - $ref: /schemas/hwmon/cirrus,lochnagar.yaml#
+
+properties:
+ compatible:
+ contains:
+ enum:
+ - cirrus,lochnagar1
+ - cirrus,lochnagar2
+
+ reg:
+ description:
+ I2C slave address.
+ const: 0x22
+
+ reset-gpios:
+ description: |
+ Reset line to the Lochnagar, see [1].
+ maxItems: 1
+
+ present-gpios:
+ description: |
+ Host present line, indicating the presence of a
+ host system, see [1]. This can be omitted if the present line is
+ tied in hardware.
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - reset-gpios
+ - lochnagar-clk
+ - lochnagar-pinctrl
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clk/lochnagar.h>
+ #include <dt-bindings/pinctrl/lochnagar.h>
+ i2c@e0004000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0xe0004000 0x1000>;
+
+ lochnagar: lochnagar@22 {
+ compatible = "cirrus,lochnagar2";
+ reg = <0x22>;
+
+ reset-gpios = <&gpio0 55 0>;
+ present-gpios = <&gpio0 60 0>;
+
+ lochnagarclk: lochnagar-clk {
+ compatible = "cirrus,lochnagar2-clk";
+
+ #clock-cells = <1>;
+ clocks = <&clkaudio>, <&clkpmic>;
+ clock-names = "ln-gf-mclk2", "ln-pmic-32k";
+
+ assigned-clocks = <&lochnagarclk LOCHNAGAR_CDC_MCLK1>,
+ <&lochnagarclk LOCHNAGAR_CDC_MCLK2>;
+ assigned-clock-parents = <&clkaudio>, <&clkpmic>;
+ };
+
+ clkpmic: lochnagar-pmic32k {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <32768>;
+ };
+
+ lochnagar-pinctrl {
+ compatible = "cirrus,lochnagar-pinctrl";
+
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-ranges = <&lochnagar 0 0 LOCHNAGAR2_PIN_NUM_GPIOS>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinsettings>;
+
+ pinsettings: pin-settings {
+ ap2aif {
+ input-enable;
+ groups = "gf-aif1";
+ function = "codec-aif3";
+ };
+ codec2aif {
+ output-enable;
+ groups = "codec-aif3";
+ function = "gf-aif1";
+ };
+ };
+ };
+
+ lochnagar-sc {
+ compatible = "cirrus,lochnagar2-soundcard";
+
+ #sound-dai-cells = <1>;
+
+ clocks = <&lochnagarclk LOCHNAGAR_SOUNDCARD_MCLK>;
+ clock-names = "mclk";
+ };
+
+ lochnagar-hwmon {
+ compatible = "cirrus,lochnagar2-hwmon";
+ };
+
+ MIC1VDD {
+ compatible = "cirrus,lochnagar2-mic1vdd";
+
+ MICBIAS1-supply = <&eric>;
+
+ cirrus,micbias-input = <3>;
+ };
+
+ MICVDD {
+ compatible = "cirrus,lochnagar2-micvdd";
+
+ SYSVDD-supply = <&wallvdd>;
+
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ };
+
+ VDD1V8 {
+ compatible = "regulator-fixed";
+
+ regulator-name = "VDD1V8";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-boot-on;
+ regulator-always-on;
+
+ vin-supply = <&wallvdd>;
+ };
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index e64e5db314976..a2b94e8cf585c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4062,12 +4062,12 @@ M: Charles Keepax <[email protected]>
M: Richard Fitzgerald <[email protected]>
L: [email protected]
S: Supported
-F: Documentation/devicetree/bindings/clock/cirrus,lochnagar.txt
-F: Documentation/devicetree/bindings/hwmon/cirrus,lochnagar.txt
-F: Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt
-F: Documentation/devicetree/bindings/pinctrl/cirrus,lochnagar.txt
-F: Documentation/devicetree/bindings/regulator/cirrus,lochnagar.txt
-F: Documentation/devicetree/bindings/sound/cirrus,lochnagar.txt
+F: Documentation/devicetree/bindings/clock/cirrus,lochnagar.yaml
+F: Documentation/devicetree/bindings/hwmon/cirrus,lochnagar.yaml
+F: Documentation/devicetree/bindings/mfd/cirrus,lochnagar.yaml
+F: Documentation/devicetree/bindings/pinctrl/cirrus,lochnagar.yaml
+F: Documentation/devicetree/bindings/regulator/cirrus,lochnagar.yaml
+F: Documentation/devicetree/bindings/sound/cirrus,lochnagar.yaml
F: Documentation/hwmon/lochnagar.rst
F: drivers/clk/clk-lochnagar.c
F: drivers/hwmon/lochnagar-hwmon.c
--
2.11.0
On Mon, Apr 27, 2020 at 11:28:12AM +0100, Charles Keepax wrote:
> Signed-off-by: Charles Keepax <[email protected]>
> ---
> + lochnagar-pinctrl {
> + compatible = "cirrus,lochnagar-pinctrl";
> +
> + gpio-controller;
> + #gpio-cells = <2>;
> + gpio-ranges = <&lochnagar 0 0 LOCHNAGAR2_PIN_NUM_GPIOS>;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinsettings>;
This seems to cause the following error:
Documentation/devicetree/bindings/mfd/cirrus,lochnagar.example.dt.yaml:
lochnagar@22: lochnagar-pinctrl:pin-settings:phandle: [[4]] is not of type 'object'
I think the schema is correct and the problem is one of tooling,
I have been poking at it for a while but can't seem to find a way
to silence this one. Any advice would be greatly appreciated.
Thanks,
Charles
> +
> + pinsettings: pin-settings {
> + ap2aif {
> + input-enable;
> + groups = "gf-aif1";
> + function = "codec-aif3";
> + };
> + codec2aif {
> + output-enable;
> + groups = "codec-aif3";
> + function = "gf-aif1";
> + };
> + };
> + };
On Mon, Apr 27, 2020 at 10:36:14AM +0000, Charles Keepax wrote:
> On Mon, Apr 27, 2020 at 11:28:12AM +0100, Charles Keepax wrote:
> > Signed-off-by: Charles Keepax <[email protected]>
> > ---
> > + lochnagar-pinctrl {
> > + compatible = "cirrus,lochnagar-pinctrl";
> > +
> > + gpio-controller;
> > + #gpio-cells = <2>;
> > + gpio-ranges = <&lochnagar 0 0 LOCHNAGAR2_PIN_NUM_GPIOS>;
> > +
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinsettings>;
>
> This seems to cause the following error:
>
> Documentation/devicetree/bindings/mfd/cirrus,lochnagar.example.dt.yaml:
> lochnagar@22: lochnagar-pinctrl:pin-settings:phandle: [[4]] is not of type 'object'
>
> I think the schema is correct and the problem is one of tooling,
> I have been poking at it for a while but can't seem to find a way
> to silence this one. Any advice would be greatly appreciated.
The problem is the "^.*$" patterns to match child node names also match
properties like 'phandle'. Ideally, you'd have some pattern to match on
for the node names.
There is work-around doing:
"^.*$":
if:
type: object
then:
properties:
...
But I much prefer to see node names updated. '-pins$' is a common
pattern.
Rob
On Mon, Apr 27, 2020 at 11:28:12AM +0100, Charles Keepax wrote:
> Signed-off-by: Charles Keepax <[email protected]>
> ---
> .../devicetree/bindings/mfd/cirrus,lochnagar.txt | 85 ----------
> .../devicetree/bindings/mfd/cirrus,lochnagar.yaml | 183 +++++++++++++++++++++
> MAINTAINERS | 12 +-
> 3 files changed, 189 insertions(+), 91 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt
> create mode 100644 Documentation/devicetree/bindings/mfd/cirrus,lochnagar.yaml
> diff --git a/Documentation/devicetree/bindings/mfd/cirrus,lochnagar.yaml b/Documentation/devicetree/bindings/mfd/cirrus,lochnagar.yaml
> new file mode 100644
> index 0000000000000..b2967f141b2af
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/cirrus,lochnagar.yaml
> @@ -0,0 +1,183 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/cirrus,lochnagar.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus Logic Lochnagar Audio Development Board
> +
> +maintainers:
> + - [email protected]
> +
> +description: |
> + Lochnagar is an evaluation and development board for Cirrus Logic
> + Smart CODEC and Amp devices. It allows the connection of most Cirrus
> + Logic devices on mini-cards, as well as allowing connection of
> + various application processor systems to provide a full evaluation
> + platform. Audio system topology, clocking and power can all be
> + controlled through the Lochnagar, allowing the device under test
> + to be used in a variety of possible use cases.
> +
> + Also see these documents for generic binding information:
> + [1] GPIO : ../gpio/gpio.txt
> +
> + And these for relevant defines:
> + [2] include/dt-bindings/pinctrl/lochnagar.h
> + [3] include/dt-bindings/clock/lochnagar.h
> +
> + And these documents for the required sub-node binding details:
> + [4] Clock: ../clock/cirrus,lochnagar.yaml
> + [5] Pinctrl: ../pinctrl/cirrus,lochnagar.yaml
> + [6] Regulator: ../regulator/cirrus,lochnagar.yaml
> + [7] Sound: ../sound/cirrus,lochnagar.yaml
> + [8] Hardware Monitor: ../hwmon/cirrus,lochnagar.yaml
> +
> +allOf:
> + - $ref: /schemas/clock/cirrus,lochnagar.yaml#
> + - $ref: /schemas/pinctrl/cirrus,lochnagar.yaml#
> + - $ref: /schemas/regulator/cirrus,lochnagar.yaml#
The preferred form is to define the child nodes in this schema and then
reference the child schema:
lochnagar-clk:
type: object
$ref: /schemas/clock/cirrus,lochnagar.yaml#
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - cirrus,lochnagar2
> + then:
> + allOf:
> + - $ref: /schemas/sound/cirrus,lochnagar.yaml#
> + - $ref: /schemas/hwmon/cirrus,lochnagar.yaml#
That makes this part a bit more challenging:
then:
required:
- lochnagar-sc
else:
not:
required:
- lochnagar-sc
> +
> +properties:
> + compatible:
> + contains:
So this is valid?
"foo", "cirrus,lochnagar1"
I think you just want to drop 'contains'.
> + enum:
> + - cirrus,lochnagar1
> + - cirrus,lochnagar2
> +
> + reg:
> + description:
> + I2C slave address.
No need for generic descriptions of common properties.
> + const: 0x22
> +
> + reset-gpios:
> + description: |
> + Reset line to the Lochnagar, see [1].
> + maxItems: 1
> +
> + present-gpios:
> + description: |
> + Host present line, indicating the presence of a
> + host system, see [1]. This can be omitted if the present line is
> + tied in hardware.
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> + - reset-gpios
> + - lochnagar-clk
> + - lochnagar-pinctrl
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clk/lochnagar.h>
> + #include <dt-bindings/pinctrl/lochnagar.h>
> + i2c@e0004000 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0xe0004000 0x1000>;
> +
> + lochnagar: lochnagar@22 {
> + compatible = "cirrus,lochnagar2";
> + reg = <0x22>;
> +
> + reset-gpios = <&gpio0 55 0>;
> + present-gpios = <&gpio0 60 0>;
> +
> + lochnagarclk: lochnagar-clk {
> + compatible = "cirrus,lochnagar2-clk";
> +
> + #clock-cells = <1>;
> + clocks = <&clkaudio>, <&clkpmic>;
> + clock-names = "ln-gf-mclk2", "ln-pmic-32k";
> +
> + assigned-clocks = <&lochnagarclk LOCHNAGAR_CDC_MCLK1>,
> + <&lochnagarclk LOCHNAGAR_CDC_MCLK2>;
> + assigned-clock-parents = <&clkaudio>, <&clkpmic>;
> + };
> +
> + clkpmic: lochnagar-pmic32k {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <32768>;
> + };
> +
> + lochnagar-pinctrl {
> + compatible = "cirrus,lochnagar-pinctrl";
> +
> + gpio-controller;
> + #gpio-cells = <2>;
> + gpio-ranges = <&lochnagar 0 0 LOCHNAGAR2_PIN_NUM_GPIOS>;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinsettings>;
> +
> + pinsettings: pin-settings {
> + ap2aif {
> + input-enable;
> + groups = "gf-aif1";
> + function = "codec-aif3";
> + };
> + codec2aif {
> + output-enable;
> + groups = "codec-aif3";
> + function = "gf-aif1";
> + };
> + };
> + };
> +
> + lochnagar-sc {
> + compatible = "cirrus,lochnagar2-soundcard";
> +
> + #sound-dai-cells = <1>;
> +
> + clocks = <&lochnagarclk LOCHNAGAR_SOUNDCARD_MCLK>;
> + clock-names = "mclk";
> + };
> +
> + lochnagar-hwmon {
> + compatible = "cirrus,lochnagar2-hwmon";
> + };
> +
> + MIC1VDD {
> + compatible = "cirrus,lochnagar2-mic1vdd";
> +
> + MICBIAS1-supply = <&eric>;
> +
> + cirrus,micbias-input = <3>;
> + };
> +
> + MICVDD {
> + compatible = "cirrus,lochnagar2-micvdd";
> +
> + SYSVDD-supply = <&wallvdd>;
> +
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + };
> +
> + VDD1V8 {
> + compatible = "regulator-fixed";
> +
> + regulator-name = "VDD1V8";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-boot-on;
> + regulator-always-on;
> +
> + vin-supply = <&wallvdd>;
> + };
> + };
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e64e5db314976..a2b94e8cf585c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4062,12 +4062,12 @@ M: Charles Keepax <[email protected]>
> M: Richard Fitzgerald <[email protected]>
> L: [email protected]
> S: Supported
> -F: Documentation/devicetree/bindings/clock/cirrus,lochnagar.txt
> -F: Documentation/devicetree/bindings/hwmon/cirrus,lochnagar.txt
> -F: Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt
> -F: Documentation/devicetree/bindings/pinctrl/cirrus,lochnagar.txt
> -F: Documentation/devicetree/bindings/regulator/cirrus,lochnagar.txt
> -F: Documentation/devicetree/bindings/sound/cirrus,lochnagar.txt
> +F: Documentation/devicetree/bindings/clock/cirrus,lochnagar.yaml
> +F: Documentation/devicetree/bindings/hwmon/cirrus,lochnagar.yaml
> +F: Documentation/devicetree/bindings/mfd/cirrus,lochnagar.yaml
> +F: Documentation/devicetree/bindings/pinctrl/cirrus,lochnagar.yaml
> +F: Documentation/devicetree/bindings/regulator/cirrus,lochnagar.yaml
> +F: Documentation/devicetree/bindings/sound/cirrus,lochnagar.yaml
> F: Documentation/hwmon/lochnagar.rst
> F: drivers/clk/clk-lochnagar.c
> F: drivers/hwmon/lochnagar-hwmon.c
> --
> 2.11.0
>
On Fri, May 01, 2020 at 03:47:21PM -0500, Rob Herring wrote:
> On Mon, Apr 27, 2020 at 10:36:14AM +0000, Charles Keepax wrote:
> > On Mon, Apr 27, 2020 at 11:28:12AM +0100, Charles Keepax wrote:
> > > Signed-off-by: Charles Keepax <[email protected]>
> > > ---
> > > + lochnagar-pinctrl {
> > > + compatible = "cirrus,lochnagar-pinctrl";
> > > +
> > > + gpio-controller;
> > > + #gpio-cells = <2>;
> > > + gpio-ranges = <&lochnagar 0 0 LOCHNAGAR2_PIN_NUM_GPIOS>;
> > > +
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&pinsettings>;
> >
> > This seems to cause the following error:
> >
> > Documentation/devicetree/bindings/mfd/cirrus,lochnagar.example.dt.yaml:
> > lochnagar@22: lochnagar-pinctrl:pin-settings:phandle: [[4]] is not of type 'object'
> >
> > I think the schema is correct and the problem is one of tooling,
> > I have been poking at it for a while but can't seem to find a way
> > to silence this one. Any advice would be greatly appreciated.
>
> The problem is the "^.*$" patterns to match child node names also match
> properties like 'phandle'. Ideally, you'd have some pattern to match on
> for the node names.
>
> There is work-around doing:
>
> "^.*$":
> if:
> type: object
> then:
> properties:
> ...
>
I believe I did try this and run into some other problems.
> But I much prefer to see node names updated. '-pins$' is a common
> pattern.
>
I have no problem requiring a pins suffix on the sub-nodes. Will
add that and send a new version, fixing up your other comments as
well. Thank you for your suggestions and review.
Thanks,
Charles