[PATCH 0/2] pwm: meson: add pwm support for A1

[PATCH 0/2] pwm: meson: add pwm support for A1

[George Stark]

Add support for Amlogic meson A1 SoC family PWM.

George Stark (2):
  dt-bindings: pwm: amlogic: Add new bindings for meson A1 pwm
  pwm: meson: support meson A1 SoC family

 .../devicetree/bindings/pwm/pwm-amlogic.yaml  |  2 ++
 drivers/pwm/pwm-meson.c                       | 35 +++++++++++++++++++
 2 files changed, 37 insertions(+)

--
2.25.1

[PATCH 1/2] dt-bindings: pwm: amlogic: Add new bindings for meson A1 pwm

[George Stark]

The chip has 3 dual channel PWM modules AB, CD, EF.

Signed-off-by: George Stark <gnstark@salutedevices.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
index 1d71d4f8f328..ef6daf1760ff 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
+++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
@@ -37,6 +37,7 @@ properties:
       - enum:
           - amlogic,meson8-pwm-v2
           - amlogic,meson-s4-pwm
+          - amlogic,meson-a1-pwm
       - items:
           - enum:
               - amlogic,meson8b-pwm-v2
@@ -126,6 +127,7 @@ allOf:
           contains:
             enum:
               - amlogic,meson-s4-pwm
+              - amlogic,meson-a1-pwm
     then:
       properties:
         clocks:
-- 
2.25.1

Re: [PATCH 1/2] dt-bindings: pwm: amlogic: Add new bindings for meson A1 pwm

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

On Tue, Apr 23, 2024 at 07:10:05PM +0300, George Stark wrote:
> The chip has 3 dual channel PWM modules AB, CD, EF.
> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>

a would sort before s.

With the re-order,
Acked-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

> ---
>  Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> index 1d71d4f8f328..ef6daf1760ff 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> +++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> @@ -37,6 +37,7 @@ properties:
>        - enum:
>            - amlogic,meson8-pwm-v2
>            - amlogic,meson-s4-pwm
> +          - amlogic,meson-a1-pwm
>        - items:
>            - enum:
>                - amlogic,meson8b-pwm-v2
> @@ -126,6 +127,7 @@ allOf:
>            contains:
>              enum:
>                - amlogic,meson-s4-pwm
> +              - amlogic,meson-a1-pwm
>      then:
>        properties:
>          clocks:
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/2] dt-bindings: pwm: amlogic: Add new bindings for meson A1 pwm

[Jerome Brunet]

On Tue 23 Apr 2024 at 17:56, Conor Dooley <conor@kernel.org> wrote:

> [[PGP Signed Part:Undecided]]
> On Tue, Apr 23, 2024 at 07:10:05PM +0300, George Stark wrote:
>> The chip has 3 dual channel PWM modules AB, CD, EF.
>> 
>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
>
> a would sort before s.
>
> With the re-order,
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>
> Thanks,
> Conor.
>
>> ---
>>  Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>> index 1d71d4f8f328..ef6daf1760ff 100644
>> --- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>> @@ -37,6 +37,7 @@ properties:
>>        - enum:
>>            - amlogic,meson8-pwm-v2
>>            - amlogic,meson-s4-pwm
>> +          - amlogic,meson-a1-pwm

AFAICT, the a1 interface is exactly as the s4 interface.
So a1 should list s4 as a fallback and the driver should match on the s4.

>>        - items:
>>            - enum:
>>                - amlogic,meson8b-pwm-v2
>> @@ -126,6 +127,7 @@ allOf:
>>            contains:
>>              enum:
>>                - amlogic,meson-s4-pwm
>> +              - amlogic,meson-a1-pwm
>>      then:
>>        properties:
>>          clocks:
>> -- 
>> 2.25.1
>> 
>
> [[End of PGP Signed Part]]


-- 
Jerome

Re: [PATCH 1/2] dt-bindings: pwm: amlogic: Add new bindings for meson A1 pwm

[Kelvin Zhang]

On 2024/4/24 01:44, Jerome Brunet wrote:
> 
> On Tue 23 Apr 2024 at 17:56, Conor Dooley <conor@kernel.org> wrote:
> 
>> [[PGP Signed Part:Undecided]]
>> On Tue, Apr 23, 2024 at 07:10:05PM +0300, George Stark wrote:
>>> The chip has 3 dual channel PWM modules AB, CD, EF.
>>>
>>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>>> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
>>
>> a would sort before s.
>>
>> With the re-order,
>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>>
>> Thanks,
>> Conor.
>>
>>> ---
>>>   Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>>> index 1d71d4f8f328..ef6daf1760ff 100644
>>> --- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>>> +++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
>>> @@ -37,6 +37,7 @@ properties:
>>>         - enum:
>>>             - amlogic,meson8-pwm-v2
>>>             - amlogic,meson-s4-pwm
>>> +          - amlogic,meson-a1-pwm
> 
> AFAICT, the a1 interface is exactly as the s4 interface.
> So a1 should list s4 as a fallback and the driver should match on the s4.

Hi George,
For your information, we are preparing S4 submission.
Thanks!

> 
>>>         - items:
>>>             - enum:
>>>                 - amlogic,meson8b-pwm-v2
>>> @@ -126,6 +127,7 @@ allOf:
>>>             contains:
>>>               enum:
>>>                 - amlogic,meson-s4-pwm
>>> +              - amlogic,meson-a1-pwm
>>>       then:
>>>         properties:
>>>           clocks:
>>> --
>>> 2.25.1
>>>
>>
>> [[End of PGP Signed Part]]
> 
> 
> --
> Jerome
> 
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 1/2] dt-bindings: pwm: amlogic: Add new bindings for meson A1 pwm

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]

On Tue, Apr 23, 2024 at 07:44:35PM +0200, Jerome Brunet wrote:
> 
> On Tue 23 Apr 2024 at 17:56, Conor Dooley <conor@kernel.org> wrote:
> 
> > [[PGP Signed Part:Undecided]]
> > On Tue, Apr 23, 2024 at 07:10:05PM +0300, George Stark wrote:
> >> The chip has 3 dual channel PWM modules AB, CD, EF.
> >> 
> >> Signed-off-by: George Stark <gnstark@salutedevices.com>
> >> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> >
> > a would sort before s.
> >
> > With the re-order,
> > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> >
> > Thanks,
> > Conor.
> >
> >> ---
> >>  Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml | 2 ++
> >>  1 file changed, 2 insertions(+)
> >> 
> >> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> >> index 1d71d4f8f328..ef6daf1760ff 100644
> >> --- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> >> +++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> >> @@ -37,6 +37,7 @@ properties:
> >>        - enum:
> >>            - amlogic,meson8-pwm-v2
> >>            - amlogic,meson-s4-pwm
> >> +          - amlogic,meson-a1-pwm
> 
> AFAICT, the a1 interface is exactly as the s4 interface.
> So a1 should list s4 as a fallback and the driver should match on the s4.

Crap, I should have checked. Please make use of the fallback :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 2/2] pwm: meson: support meson A1 SoC family

[George Stark]

From: George Stark <gnstark@sberdevices.ru>

Add a compatible string and configuration for the meson A1 SoC family
PWM. Additionally, provide an external clock initialization helper
specifically designed for these PWM IPs.

Signed-off-by: George Stark <gnstark@sberdevices.ru>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 drivers/pwm/pwm-meson.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index ea96c5973488..529a541ba7b6 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -462,6 +462,33 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip)
 	return meson_pwm_init_clocks_meson8b(chip, mux_parent_data);
 }
 
+static int meson_pwm_init_channels_ext_clock(struct pwm_chip *chip)
+{
+	struct device *dev = pwmchip_parent(chip);
+	struct meson_pwm *meson = to_meson_pwm(chip);
+	struct meson_pwm_channel *channels = meson->channels;
+	struct clk_bulk_data *clks = NULL;
+	unsigned int i;
+	int res;
+
+	res = devm_clk_bulk_get_all(dev, &clks);
+	if (res < 0) {
+		dev_err(dev, "can't get device clocks\n");
+		return res;
+	}
+
+	if (res != MESON_NUM_PWMS) {
+		dev_err(dev, "clock count must be %d, got %d\n",
+			MESON_NUM_PWMS, res);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < MESON_NUM_PWMS; i++)
+		channels[i].clk = clks[i].clk;
+
+	return 0;
+}
+
 static const struct meson_pwm_data pwm_meson8b_data = {
 	.parent_names = { "xtal", NULL, "fclk_div4", "fclk_div3" },
 	.channels_init = meson_pwm_init_channels_meson8b_legacy,
@@ -500,11 +527,19 @@ static const struct meson_pwm_data pwm_meson8_v2_data = {
 	.channels_init = meson_pwm_init_channels_meson8b_v2,
 };
 
+static const struct meson_pwm_data pwm_meson_ext_clock_data = {
+	.channels_init = meson_pwm_init_channels_ext_clock,
+};
+
 static const struct of_device_id meson_pwm_matches[] = {
 	{
 		.compatible = "amlogic,meson8-pwm-v2",
 		.data = &pwm_meson8_v2_data
 	},
+	{
+		.compatible = "amlogic,meson-a1-pwm",
+		.data = &pwm_meson_ext_clock_data
+	},
 	/* The following compatibles are obsolete */
 	{
 		.compatible = "amlogic,meson8b-pwm",
-- 
2.25.1

Re: [PATCH 2/2] pwm: meson: support meson A1 SoC family

[Jerome Brunet]

On Tue 23 Apr 2024 at 19:10, George Stark <gnstark@salutedevices.com> wrote:

> From: George Stark <gnstark@sberdevices.ru>
>
> Add a compatible string and configuration for the meson A1 SoC family
> PWM. Additionally, provide an external clock initialization helper
> specifically designed for these PWM IPs.
>
> Signed-off-by: George Stark <gnstark@sberdevices.ru>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
>  drivers/pwm/pwm-meson.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index ea96c5973488..529a541ba7b6 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -462,6 +462,33 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip)
>  	return meson_pwm_init_clocks_meson8b(chip, mux_parent_data);
>  }
>  
> +static int meson_pwm_init_channels_ext_clock(struct pwm_chip *chip)

That kind on naming (ext) is almost sure to clash with whatever comes next.
Just use the name of the first SoC using the method, a1 for instance.

> +{
> +	struct device *dev = pwmchip_parent(chip);
> +	struct meson_pwm *meson = to_meson_pwm(chip);
> +	struct meson_pwm_channel *channels = meson->channels;
> +	struct clk_bulk_data *clks = NULL;
> +	unsigned int i;
> +	int res;
> +
> +	res = devm_clk_bulk_get_all(dev, &clks);
> +	if (res < 0) {
> +		dev_err(dev, "can't get device clocks\n");
> +		return res;
> +	}

I don't think allocating the 'clk_bulk_data *clks' is necessary or safe.
We know exactly how many clocks we expect, there is no need for a get all.

> +
> +	if (res != MESON_NUM_PWMS) {
> +		dev_err(dev, "clock count must be %d, got %d\n",
> +			MESON_NUM_PWMS, res);
> +		return -EINVAL;
> +	}

... and this only catches the problem after the fact.

It is probably convinient but not necessary.

> +
> +	for (i = 0; i < MESON_NUM_PWMS; i++)
> +		channels[i].clk = clks[i].clk;

channels[i].clk could be assigned directly of_clk_get() using clock
indexes. No extra allocation needed.

> +
> +	return 0;
> +}
> +
>  static const struct meson_pwm_data pwm_meson8b_data = {
>  	.parent_names = { "xtal", NULL, "fclk_div4", "fclk_div3" },
>  	.channels_init = meson_pwm_init_channels_meson8b_legacy,
> @@ -500,11 +527,19 @@ static const struct meson_pwm_data pwm_meson8_v2_data = {
>  	.channels_init = meson_pwm_init_channels_meson8b_v2,
>  };
>  
> +static const struct meson_pwm_data pwm_meson_ext_clock_data = {
> +	.channels_init = meson_pwm_init_channels_ext_clock,
> +};
> +
>  static const struct of_device_id meson_pwm_matches[] = {
>  	{
>  		.compatible = "amlogic,meson8-pwm-v2",
>  		.data = &pwm_meson8_v2_data
>  	},
> +	{
> +		.compatible = "amlogic,meson-a1-pwm",
> +		.data = &pwm_meson_ext_clock_data
> +	},
>  	/* The following compatibles are obsolete */
>  	{
>  		.compatible = "amlogic,meson8b-pwm",


-- 
Jerome

Re: [PATCH 2/2] pwm: meson: support meson A1 SoC family

[George Stark]

Hello Jerome

Thanks for the review


On 4/23/24 20:35, Jerome Brunet wrote:
> 
> On Tue 23 Apr 2024 at 19:10, George Stark <gnstark@salutedevices.com> wrote:
> 
>> From: George Stark <gnstark@sberdevices.ru>
>>
>> Add a compatible string and configuration for the meson A1 SoC family
>> PWM. Additionally, provide an external clock initialization helper
>> specifically designed for these PWM IPs.
>>
>> Signed-off-by: George Stark <gnstark@sberdevices.ru>
>> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
>> ---
>>   drivers/pwm/pwm-meson.c | 35 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 35 insertions(+)
>>
>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>> index ea96c5973488..529a541ba7b6 100644
>> --- a/drivers/pwm/pwm-meson.c
>> +++ b/drivers/pwm/pwm-meson.c
>> @@ -462,6 +462,33 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip)
>>   	return meson_pwm_init_clocks_meson8b(chip, mux_parent_data);
>>   }
>>   
>> +static int meson_pwm_init_channels_ext_clock(struct pwm_chip *chip)
> 
> That kind on naming (ext) is almost sure to clash with whatever comes next.
> Just use the name of the first SoC using the method, a1 for instance.

It's true that pwm core in a1 s4, t7 etc is the same AFAWK.
I just want to clarify your proposal:
I add a1 compatible to the dt-bindings with s4 as fallback,
t7 compatible will be added later in the same way.

Here in the driver I don't mention a1 at all and use s4-centric naming e.g.:

{
	.compatible = "amlogic,meson-s4-pwm",
	.data = &pwm_meson_s4_data
},
static const struct meson_pwm_data pwm_meson_s4_data = {
	.channels_init = meson_pwm_init_channels_s4,
};

right?

>> +{
>> +	struct device *dev = pwmchip_parent(chip);
>> +	struct meson_pwm *meson = to_meson_pwm(chip);
>> +	struct meson_pwm_channel *channels = meson->channels;
>> +	struct clk_bulk_data *clks = NULL;
>> +	unsigned int i;
>> +	int res;
>> +
>> +	res = devm_clk_bulk_get_all(dev, &clks);
>> +	if (res < 0) {
>> +		dev_err(dev, "can't get device clocks\n");
>> +		return res;
>> +	}
> 
> I don't think allocating the 'clk_bulk_data *clks' is necessary or safe.
> We know exactly how many clocks we expect, there is no need for a get all.
> 
>> +
>> +	if (res != MESON_NUM_PWMS) {
>> +		dev_err(dev, "clock count must be %d, got %d\n",
>> +			MESON_NUM_PWMS, res);
>> +		return -EINVAL;
>> +	}
> 
> ... and this only catches the problem after the fact.
> 
> It is probably convinient but not necessary.
> 
>> +
>> +	for (i = 0; i < MESON_NUM_PWMS; i++)
>> +		channels[i].clk = clks[i].clk;
> 
> channels[i].clk could be assigned directly of_clk_get() using clock
> indexes. No extra allocation needed.

if we use of_clk_get then we'll have to free the clock objects in the
end. Could we use devm_clk_bulk_get instead?

>> +
>> +	return 0;
>> +}
>> +
>>   static const struct meson_pwm_data pwm_meson8b_data = {
>>   	.parent_names = { "xtal", NULL, "fclk_div4", "fclk_div3" },
>>   	.channels_init = meson_pwm_init_channels_meson8b_legacy,
>> @@ -500,11 +527,19 @@ static const struct meson_pwm_data pwm_meson8_v2_data = {
>>   	.channels_init = meson_pwm_init_channels_meson8b_v2,
>>   };
>>   
>> +static const struct meson_pwm_data pwm_meson_ext_clock_data = {
>> +	.channels_init = meson_pwm_init_channels_ext_clock,
>> +};
>> +
>>   static const struct of_device_id meson_pwm_matches[] = {
>>   	{
>>   		.compatible = "amlogic,meson8-pwm-v2",
>>   		.data = &pwm_meson8_v2_data
>>   	},
>> +	{
>> +		.compatible = "amlogic,meson-a1-pwm",
>> +		.data = &pwm_meson_ext_clock_data
>> +	},
>>   	/* The following compatibles are obsolete */
>>   	{
>>   		.compatible = "amlogic,meson8b-pwm",
> 
> 

-- 
Best regards
George

Re: [PATCH 2/2] pwm: meson: support meson A1 SoC family

[Jerome Brunet]

On Wed 24 Apr 2024 at 02:00, George Stark <gnstark@salutedevices.com> wrote:

> Hello Jerome
>
> Thanks for the review
>
>
> On 4/23/24 20:35, Jerome Brunet wrote:
>> On Tue 23 Apr 2024 at 19:10, George Stark <gnstark@salutedevices.com>
>> wrote:
>> 
>>> From: George Stark <gnstark@sberdevices.ru>
>>>
>>> Add a compatible string and configuration for the meson A1 SoC family
>>> PWM. Additionally, provide an external clock initialization helper
>>> specifically designed for these PWM IPs.
>>>
>>> Signed-off-by: George Stark <gnstark@sberdevices.ru>
>>> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
>>> ---
>>>   drivers/pwm/pwm-meson.c | 35 +++++++++++++++++++++++++++++++++++
>>>   1 file changed, 35 insertions(+)
>>>
>>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>>> index ea96c5973488..529a541ba7b6 100644
>>> --- a/drivers/pwm/pwm-meson.c
>>> +++ b/drivers/pwm/pwm-meson.c
>>> @@ -462,6 +462,33 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip)
>>>   	return meson_pwm_init_clocks_meson8b(chip, mux_parent_data);
>>>   }
>>>   +static int meson_pwm_init_channels_ext_clock(struct pwm_chip *chip)
>> That kind on naming (ext) is almost sure to clash with whatever comes
>> next.
>> Just use the name of the first SoC using the method, a1 for instance.
>
> It's true that pwm core in a1 s4, t7 etc is the same AFAWK.
> I just want to clarify your proposal:
> I add a1 compatible to the dt-bindings with s4 as fallback,
> t7 compatible will be added later in the same way.

If you know t7 is compatible as well, please add it to.

Other people (including from amlogic) have responded to thread around
the s4 pwm topic. You should probably Cc them of your future submission.

They may help test and review

>
> Here in the driver I don't mention a1 at all and use s4-centric naming e.g.:
>
> {
> 	.compatible = "amlogic,meson-s4-pwm",
> 	.data = &pwm_meson_s4_data
> },
> static const struct meson_pwm_data pwm_meson_s4_data = {
> 	.channels_init = meson_pwm_init_channels_s4,
> };
>
> right?
>
yes

>>> +{
>>> +	struct device *dev = pwmchip_parent(chip);
>>> +	struct meson_pwm *meson = to_meson_pwm(chip);
>>> +	struct meson_pwm_channel *channels = meson->channels;
>>> +	struct clk_bulk_data *clks = NULL;
>>> +	unsigned int i;
>>> +	int res;
>>> +
>>> +	res = devm_clk_bulk_get_all(dev, &clks);
>>> +	if (res < 0) {
>>> +		dev_err(dev, "can't get device clocks\n");
>>> +		return res;
>>> +	}
>> I don't think allocating the 'clk_bulk_data *clks' is necessary or safe.
>> We know exactly how many clocks we expect, there is no need for a get all.
>> 
>>> +
>>> +	if (res != MESON_NUM_PWMS) {
>>> +		dev_err(dev, "clock count must be %d, got %d\n",
>>> +			MESON_NUM_PWMS, res);
>>> +		return -EINVAL;
>>> +	}
>> ... and this only catches the problem after the fact.
>> It is probably convinient but not necessary.
>> 
>>> +
>>> +	for (i = 0; i < MESON_NUM_PWMS; i++)
>>> +		channels[i].clk = clks[i].clk;
>> channels[i].clk could be assigned directly of_clk_get() using clock
>> indexes. No extra allocation needed.
>
> if we use of_clk_get then we'll have to free the clock objects in the
> end. Could we use devm_clk_bulk_get instead?

Add the devm variant of of_clk_get() if you must.
Use devm_add_action_or_reset() otherwise

>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   static const struct meson_pwm_data pwm_meson8b_data = {
>>>   	.parent_names = { "xtal", NULL, "fclk_div4", "fclk_div3" },
>>>   	.channels_init = meson_pwm_init_channels_meson8b_legacy,
>>> @@ -500,11 +527,19 @@ static const struct meson_pwm_data pwm_meson8_v2_data = {
>>>   	.channels_init = meson_pwm_init_channels_meson8b_v2,
>>>   };
>>>   +static const struct meson_pwm_data pwm_meson_ext_clock_data = {
>>> +	.channels_init = meson_pwm_init_channels_ext_clock,
>>> +};
>>> +
>>>   static const struct of_device_id meson_pwm_matches[] = {
>>>   	{
>>>   		.compatible = "amlogic,meson8-pwm-v2",
>>>   		.data = &pwm_meson8_v2_data
>>>   	},
>>> +	{
>>> +		.compatible = "amlogic,meson-a1-pwm",
>>> +		.data = &pwm_meson_ext_clock_data
>>> +	},
>>>   	/* The following compatibles are obsolete */
>>>   	{
>>>   		.compatible = "amlogic,meson8b-pwm",
>> 


-- 
Jerome

[PATCH 0/2] add support for meson a1 PWM in dts and bindings

[George Stark]

Add support for meson a1 PWM in dts and bindings. Due to pwm driver code is fully
the same for a1 and s4 then a1 compatible defined with s4 as fallback.

George Stark (2):
  dt-bindings: pwm: amlogic: Add new bindings for meson A1 pwm
  arm64: dts: meson: a1: add definitions for meson pwm

 .../devicetree/bindings/pwm/pwm-amlogic.yaml  |   2 +
 arch/arm64/boot/dts/amlogic/meson-a1.dtsi     | 215 ++++++++++++++++++
 2 files changed, 217 insertions(+)

--
2.25.1

[PATCH 1/2] dt-bindings: pwm: amlogic: Add new bindings for meson A1 pwm

[George Stark]

The chip has 3 dual-channel PWM modules PWM_AB, PWM_CD, PWM_EF.

Signed-off-by: George Stark <gnstark@salutedevices.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
index 1d71d4f8f328..63c6018b6b7c 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
+++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
@@ -37,6 +37,7 @@ properties:
       - enum:
           - amlogic,meson8-pwm-v2
           - amlogic,meson-s4-pwm
+          - amlogic,meson-a1-pwm, amlogic,meson-s4-pwm
       - items:
           - enum:
               - amlogic,meson8b-pwm-v2
@@ -126,6 +127,7 @@ allOf:
           contains:
             enum:
               - amlogic,meson-s4-pwm
+              - amlogic,meson-a1-pwm
     then:
       properties:
         clocks:
-- 
2.25.1

Re: [PATCH 1/2] dt-bindings: pwm: amlogic: Add new bindings for meson A1 pwm

[Krzysztof Kozlowski]

On 01/07/2024 15:01, George Stark wrote:
> The chip has 3 dual-channel PWM modules PWM_AB, PWM_CD, PWM_EF.
> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> index 1d71d4f8f328..63c6018b6b7c 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> +++ b/Documentation/devicetree/bindings/pwm/pwm-amlogic.yaml
> @@ -37,6 +37,7 @@ properties:
>        - enum:
>            - amlogic,meson8-pwm-v2
>            - amlogic,meson-s4-pwm
> +          - amlogic,meson-a1-pwm, amlogic,meson-s4-pwm

This is not valid, you cannot have such syntax (and there is no single
DT schema file like this).

It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

Best regards,
Krzysztof