| Message ID | 20231110062039.103339-1-william.qiu@starfivetech.com |
|---|---|
| Headers | show |
| Series | StarFive's Pulse Width Modulation driver support | expand |
On 2023/11/10 20:24, Krzysztof Kozlowski wrote: > On 10/11/2023 07:20, William Qiu wrote: >> Add documentation to describe OpenCores Pulse Width Modulation >> controller driver. >> >> Signed-off-by: William Qiu <william.qiu@starfivetech.com> >> Reviewed-by: Hal Feng <hal.feng@starfivetech.com> >> --- >> .../bindings/pwm/opencores,pwm.yaml | 56 +++++++++++++++++++ >> 1 file changed, 56 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm.yaml >> >> diff --git a/Documentation/devicetree/bindings/pwm/opencores,pwm.yaml b/Documentation/devicetree/bindings/pwm/opencores,pwm.yaml >> new file mode 100644 >> index 000000000000..8f776bbc1112 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pwm/opencores,pwm.yaml >> @@ -0,0 +1,56 @@ >> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/pwm/opencores,pwm.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: OpenCores PWM controller >> + >> +maintainers: >> + - William Qiu <william.qiu@starfivetech.com> >> + >> +description: >> + OpenCores PTC ip core contains a PWM controller. When operating in PWM mode, the PTC core >> + generates binary signal with user-programmable low and high periods. All PTC counters and >> + registers are 32-bit. > > Wrap at 80 (as Coding Style asks) > Will update. >> + >> +allOf: >> + - $ref: pwm.yaml# >> + >> +properties: >> + compatible: >> + oneOf: >> + - items: >> + - enum: >> + - starfive,jh7100-pwm >> + - starfive,jh7110-pwm >> + - const: opencores,pwm > > That's a very, very generic compatible. Are you sure, 100% sure, that > all designs from OpenCores from now till next 100 years will be 100% > compatible? > My description is not accurate enough, this is OpenCores PTC IP, and PWM is one of those modes, so it might be better to replace compatible with "opencores, ptc-pwm" What do you think? Best Regrads, William > > Best regards, > Krzysztof >
On 13/11/2023 10:42, William Qiu wrote: > Will update. >>> + >>> +allOf: >>> + - $ref: pwm.yaml# >>> + >>> +properties: >>> + compatible: >>> + oneOf: >>> + - items: >>> + - enum: >>> + - starfive,jh7100-pwm >>> + - starfive,jh7110-pwm >>> + - const: opencores,pwm >> >> That's a very, very generic compatible. Are you sure, 100% sure, that >> all designs from OpenCores from now till next 100 years will be 100% >> compatible? >> > My description is not accurate enough, this is OpenCores PTC IP, and PWM > is one of those modes, so it might be better to replace compatible with > "opencores, ptc-pwm" > > What do you think? Sorry, maybe this answers maybe doesn't. What is "PTC"? Best regards, Krzysztof
On Mon, Nov 13, 2023 at 09:07:15PM +0100, Krzysztof Kozlowski wrote: > On 13/11/2023 10:42, William Qiu wrote: > > Will update. > >>> + > >>> +allOf: > >>> + - $ref: pwm.yaml# > >>> + > >>> +properties: > >>> + compatible: > >>> + oneOf: > >>> + - items: > >>> + - enum: > >>> + - starfive,jh7100-pwm > >>> + - starfive,jh7110-pwm > >>> + - const: opencores,pwm > >> > >> That's a very, very generic compatible. Are you sure, 100% sure, that > >> all designs from OpenCores from now till next 100 years will be 100% > >> compatible? > >> > > My description is not accurate enough, this is OpenCores PTC IP, and PWM > > is one of those modes, so it might be better to replace compatible with > > "opencores, ptc-pwm" > > > > What do you think? > > Sorry, maybe this answers maybe doesn't. What is "PTC"? "pwm timer counter". AFAIU, the IP can be configured to provide all 3. I think that William pointed out on an earlier revision that they have only implemented the pwm on their hardware. I don't think putting in "ptc" is a sufficient differentiator though, as clearly there could be several different versions of "ptc-pwm" that have the same concern about "all designs from OpenCores for now till the next 100 years" being compatible. Cheers. Conor.
On Wed, Nov 22, 2023 at 03:03:36PM +0800, William Qiu wrote: > > > On 2023/11/14 4:17, Conor Dooley wrote: > > On Mon, Nov 13, 2023 at 09:07:15PM +0100, Krzysztof Kozlowski wrote: > >> On 13/11/2023 10:42, William Qiu wrote: > >> > Will update. > >> >>> + > >> >>> +allOf: > >> >>> + - $ref: pwm.yaml# > >> >>> + > >> >>> +properties: > >> >>> + compatible: > >> >>> + oneOf: > >> >>> + - items: > >> >>> + - enum: > >> >>> + - starfive,jh7100-pwm > >> >>> + - starfive,jh7110-pwm > >> >>> + - const: opencores,pwm > >> >> > >> >> That's a very, very generic compatible. Are you sure, 100% sure, that > >> >> all designs from OpenCores from now till next 100 years will be 100% > >> >> compatible? > >> >> > >> > My description is not accurate enough, this is OpenCores PTC IP, and PWM > >> > is one of those modes, so it might be better to replace compatible with > >> > "opencores, ptc-pwm" > >> > > >> > What do you think? > >> > >> Sorry, maybe this answers maybe doesn't. What is "PTC"? > > > > "pwm timer counter". AFAIU, the IP can be configured to provide all 3. > > I think that William pointed out on an earlier revision that they have > > only implemented the pwm on their hardware. > > I don't think putting in "ptc" is a sufficient differentiator though, as > > clearly there could be several different versions of "ptc-pwm" that have > > the same concern about "all designs from OpenCores for now till the next > > 100 years" being compatible. Perhaps noting what "ptc" stands for in the description field would be a good idea. > After discussion and review of materials, we plan to use "opencores,ptc-pwm-v1" > as this version of compatible, so that it can also be compatible in the future. > > What do you think? Do we know that it is actually "v1" of the IP? I would suggest using the version that actually matches the version of the IP that you are using in your SoC. Thanks, Conor.
On Fri, Nov 24, 2023 at 03:38:41PM +0800, William Qiu wrote: > > > On 2023/11/23 1:36, Conor Dooley wrote: > > On Wed, Nov 22, 2023 at 03:03:36PM +0800, William Qiu wrote: > >> > >> > >> On 2023/11/14 4:17, Conor Dooley wrote: > >> > On Mon, Nov 13, 2023 at 09:07:15PM +0100, Krzysztof Kozlowski wrote: > >> >> On 13/11/2023 10:42, William Qiu wrote: > >> >> > Will update. > >> >> >>> + > >> >> >>> +allOf: > >> >> >>> + - $ref: pwm.yaml# > >> >> >>> + > >> >> >>> +properties: > >> >> >>> + compatible: > >> >> >>> + oneOf: > >> >> >>> + - items: > >> >> >>> + - enum: > >> >> >>> + - starfive,jh7100-pwm > >> >> >>> + - starfive,jh7110-pwm > >> >> >>> + - const: opencores,pwm > >> >> >> > >> >> >> That's a very, very generic compatible. Are you sure, 100% sure, that > >> >> >> all designs from OpenCores from now till next 100 years will be 100% > >> >> >> compatible? > >> >> >> > >> >> > My description is not accurate enough, this is OpenCores PTC IP, and PWM > >> >> > is one of those modes, so it might be better to replace compatible with > >> >> > "opencores, ptc-pwm" > >> >> > > >> >> > What do you think? > >> >> > >> >> Sorry, maybe this answers maybe doesn't. What is "PTC"? > >> > > >> > "pwm timer counter". AFAIU, the IP can be configured to provide all 3. > >> > I think that William pointed out on an earlier revision that they have > >> > only implemented the pwm on their hardware. > >> > I don't think putting in "ptc" is a sufficient differentiator though, as > >> > clearly there could be several different versions of "ptc-pwm" that have > >> > the same concern about "all designs from OpenCores for now till the next > >> > 100 years" being compatible. > > > > Perhaps noting what "ptc" stands for in the description field would be a > > good idea. > > > I will add. > >> After discussion and review of materials, we plan to use "opencores,ptc-pwm-v1" > >> as this version of compatible, so that it can also be compatible in the future. > >> > >> What do you think? > > > > Do we know that it is actually "v1" of the IP? I would suggest using the > > version that actually matches the version of the IP that you are using > > in your SoC. > > > > Thanks, > > Conor. > > There is no version list on their official website, so it is not certain whether > it is v1, but at least the driver is the first version. > > What do you think is the best way? I don't have an account, so I cannot open the "ptc_spec.pdf at this link: https://opencores.org/projects/ptc/downloads but I would take whatever documentation you have for the spec and see what it says as the revision on the front cover.