Received-SPF: pass (google.com: domain of linux-kernel+bounces-156814-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <d990d835-e4bb-4248-b17e-da8907cf16e7@amlogic.com>
Date: Wed, 24 Apr 2024 19:44:08 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 1/2] pwm: meson: Add support for Amlogic S4 PWM
To: Jerome Brunet <jbrunet@baylibre.com>, kelvin.zhang@amlogic.com,
 George Stark <gnstark@salutedevices.com>
Cc: =?UTF-8?Q?Uwe_Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>,
 Neil Armstrong <neil.armstrong@linaro.org>,
 Kevin Hilman <khilman@baylibre.com>,
 Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
 Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>,
 Conor Dooley <conor+dt@kernel.org>, linux-pwm@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org, linux-amlogic@lists.infradead.org,
 linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
References: <20240424-s4-pwm-v4-0-ee22effd40d0@amlogic.com>
 <20240424-s4-pwm-v4-1-ee22effd40d0@amlogic.com>
 <1jil07f3ps.fsf@starbuckisacylon.baylibre.com>
Content-Language: en-GB
From: Junyi Zhao <junyi.zhao@amlogic.com>
In-Reply-To: <1jil07f3ps.fsf@starbuckisacylon.baylibre.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?WTJWR1c3ZUpwSEc1M3ZGVzJkL21GNXhLcUN3WTVVYnlUdHdpTnMvbnR1TVRr?=
 =?utf-8?B?VGMyZ1UyVDZBbjhWS3lRUG9IUW9YWEkyYmpGU0hiL1ZqUDM2VUxMcDBmNjlr?=
 =?utf-8?B?SXNCaml3Wkd1ZFVYSFhJYVlsQjlSQUt4VXhWd0xyVCttWTgySGJhYjBUZEJC?=
 =?utf-8?B?ZTlBalhjbmt4c1VGelRRbHNoM3JPS2FUZlREdjdTOE1ORTFnTytSMnRsWEli?=
 =?utf-8?B?OVc1bjQwK1ZoZ1QvNkR3L0ZFWTljaWxxNjNrcEsvRCtscjZYTnBpNTU3WjFG?=
 =?utf-8?B?Q0U3WGozb1Zza3RiSXg2WGpBK3M1di9DQXJCa085bzlrUVh5V0plVGhIMGRZ?=
 =?utf-8?B?UmpzNWd6U3pzZXVvUmk1dnNsM0hQUkpvNnliOGNaTVNyNkszK3QrVW9OaVE0?=
 =?utf-8?B?ZytCQW9ZVG91TlV0ckdFMHBPc00yb3E1Sll2R1V4UFVDcmtUb1dWU2JueTNM?=
 =?utf-8?B?YnQydHBaVGxhS3ViQVZrV1daRkR0TVhhV1ZIZ0J5MG1sL0FzUXNNanZUL0RU?=
 =?utf-8?B?RWh4VVp1MVFMaWEvWldqWGh1OUtrdFl6WWJOK2N1K0JSTmtvNjByVXhkVkZN?=
 =?utf-8?B?bndEazN0YnNRdjZaYkM5RVpCejA5RWtobUIxalBsc3N0YUoxTk93RU9JNEdG?=
 =?utf-8?B?NmZwdE9TMjNpVWl0VkhUNFAyYTI3dk1CY0dyeFpwWmdGUUttMjNYWjZFbnN3?=
 =?utf-8?B?T3J3R0lERGtWYVhqVDBSbWplajE0eDI3V3o1dG5GaDlWdUp6T3VVS0hTRUpp?=
 =?utf-8?B?ZEk2eWxSelNJdlk1c2laV3ozZ2pYRHBPTXZLa25TL2U4ME9QSzFFR3RSSVox?=
 =?utf-8?B?Y0RVZzFFdnBmdGR0dDZlL2N6WlZOeEhEQTdNRXB2aG1XOS9aY1lwdGxNSjZT?=
 =?utf-8?B?OEVYcVlLeTVWS2luZC9sWlJaWHhtRmgxUnlpdlpYYnpMbnJBQm8vZ0Vra2JH?=
 =?utf-8?B?bVloZ0w3RzhpbzJBMHlOdXNIblpLYTM2WlFkblhlNjZqQXBDM00wM3RNSHF5?=
 =?utf-8?B?V0w0ZEhzMDRISk5HQkJ5MURpbXJsbXlvWVdFMjNDaTBhVVA1NWtGajR2QWhv?=
 =?utf-8?B?eFhnOUkzWWU4Y2Nsd043V3Z6UENwdlpQbFd2enN3N0pQMjNXYWVuNUtDREJ6?=
 =?utf-8?B?SkhKSDJnTmNPcW40eUEwSU5EQUF0Q3V4U3dkLzR2dkM5Rk5jMVVuakRLY2lH?=
 =?utf-8?B?YmR1c1ZBZWRQYWdOd0J4Q2ttREJ6cjhDVFVXU0doTk5HOWRVZlYyWEJPRU9j?=
 =?utf-8?B?OHFPaDlnVVNlaURQUzNzdGJmemhvN1lnOTU1UGZESGljMStydDdKK2tsV2d6?=
 =?utf-8?B?UWZzcUxYenc1QnR2ZDhPUitMbURWdUJpMWRYZ09vR29USWZiY2xmSlZCOG9F?=
 =?utf-8?B?NFJtWmxhMWNGQWwvWGdVTnlxd2pDSnNHYXd3M2tyQUxwbGxkc0g1dkRoQ2xP?=
 =?utf-8?B?S0FuZHpqRThubHpRUnJHMk56dzhzOFh1TXdkUktldFViWVh3VUFwVkV4VXI5?=
 =?utf-8?B?VzU5SStCRWdUOEZMelJHMXdDTnRXSDYwS1BLLzJaS1pScHRSYjhyOXIxUlhH?=
 =?utf-8?B?ZGxYYWdJbHcvRlJLS3Nlc3lhcWUyb3RPM1REVTlOYjFQcmF0ZmRqaHQ3U0dM?=
 =?utf-8?B?VU9qQnRrbVpXbWdQTWdPZ0t5aGd0dDRTV2RDUTJBTDdleElXRHlXMlpYTnZC?=
 =?utf-8?B?aFFJaWlsSEVmYzlsVVNlZmJsc2lBZkNubDdUczFLSzZwaFZZYU5vTWhTeUJQ?=
 =?utf-8?B?R096UzZJNkF1SW5yV1haMEEyMFE3aWdKU0p2NURCY0lYdTVSYldqSHdaRHRR?=
 =?utf-8?B?Ym1UMXJqazdTMXlUbHRiVTB6dmRuS1pIRG1CQ1FyV1c4QjY1WWp0Q21pYW05?=
 =?utf-8?B?QytQQ1Q3YzBYVUZLa1Q3R2RzSXdsNVBxWGxFRWFsOSt3VVJaSUkxRzdVa043?=
 =?utf-8?B?dEEreU55RUgxcmRNVkYwUzVtMmx4UUZKc1AwRXZabWxVeXRzZW5lWW5wRDRp?=
 =?utf-8?B?S3pUZXYxZ3JJU3pObi9uYkZZdExLbHIxVmp2d0VGOGNCVnAwOFEvK2J3am9W?=
 =?utf-8?B?SVdGd0kvZzRnSVAvU0toRDFBUGdpanZJLy8rdWkyVnhLdERtRjUzMEhZeDdT?=
 =?utf-8?Q?t2odUm94jR9MmOHg7LAWiEwFV?=
X-OriginatorOrg: amlogic.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 307286c3-e97f-4493-25c7-08dc6453f4d5
X-MS-Exchange-CrossTenant-AuthSource: TYZPR03MB5743.apcprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Apr 2024 11:44:53.4559
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 0df2add9-25ca-4b3a-acb4-c99ddf0b1114
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: g0C4yg5GuSS7l8CEwI0XSd4sgjlLLkG4c8uwdsNHg/shwyYP99QMd0GVyxrMoFTkgsJW/CiLwJ2pC9BaXQIYTw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: JH0PR03MB7344


On 2024/4/24 18:32, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> On Wed 24 Apr 2024 at 18:28, Kelvin Zhang via B4 Relay <devnull+kelvin.zhang.amlogic.com@kernel.org> wrote:
> 
>> From: Junyi Zhao <junyi.zhao@amlogic.com>
>>
>> This patch adds support for Amlogic S4 PWM.
>>
>> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
>> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
>> ---
>>   drivers/pwm/pwm-meson.c | 37 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>> index ea96c5973488..6abc823745e4 100644
>> --- a/drivers/pwm/pwm-meson.c
>> +++ b/drivers/pwm/pwm-meson.c
>> @@ -462,6 +462,35 @@ 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_meson_s4(struct pwm_chip *chip)
>> +{
>> +     int i, ret;
>> +     struct device *dev = pwmchip_parent(chip);
>> +     struct device_node *np = dev->of_node;
>> +     struct meson_pwm *meson = to_meson_pwm(chip);
>> +     struct meson_pwm_channel *channel;
>> +
>> +     for (i = 0; i < MESON_NUM_PWMS; i++) {
>> +             channel = &meson->channels[i];
>> +             channel->clk = of_clk_get(np, i);
>> +             if (IS_ERR(channel->clk)) {
>> +                     ret = PTR_ERR(channel->clk);
>> +                     dev_err_probe(dev, ret, "Failed to get clk\n");
>> +                     goto err;
>> +             }
>> +     }
>> +
>> +     return 0;
>> +
>> +err:
>> +     while (--i >= 0) {
>> +             channel = &meson->channels[i];
>> +             clk_put(channel->clk);
> 
> Fine on error but leaks on module unload.
> 
> Same as George,
> 
> Add the devm variant of of_clk_get() if you must.
> Use devm_add_action_or_reset() otherwise
Hi jerom，but we have discussed before.devm variant such as follows：
devm_clk_get_enable(struct device * dev, char * id)
struct clk *devm_clk_get(struct device *dev, const char *id)
struct clk *devm_clk_get_optional(struct device *dev, const char *id)

after i check api parm ,these api's 2rd parm "id" is string not index.
because dt binding have no name property. could we use devm？
> 
> Could please synchronize this series with George and deal with all the
> supported SoCs ? a1, s4, t7, c3 ...
> 
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>>   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,6 +529,10 @@ 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_s4_data = {
>> +     .channels_init = meson_pwm_init_channels_meson_s4,
>> +};
>> +
>>   static const struct of_device_id meson_pwm_matches[] = {
>>        {
>>                .compatible = "amlogic,meson8-pwm-v2",
>> @@ -538,6 +571,10 @@ static const struct of_device_id meson_pwm_matches[] = {
>>                .compatible = "amlogic,meson-g12a-ao-pwm-cd",
>>                .data = &pwm_g12a_ao_cd_data
>>        },
>> +     {
>> +             .compatible = "amlogic,meson-s4-pwm",
>> +             .data = &pwm_meson_s4_data
>> +     },
>>        {},
>>   };
>>   MODULE_DEVICE_TABLE(of, meson_pwm_matches);
> 
> 
> --
> Jerome