From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f177.google.com (mail-pl1-f177.google.com [209.85.214.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id EDC8521B192 for ; Fri, 17 Jul 2026 00:33:26 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.177 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784248408; cv=none; b=GDIcQKuli0xGlxvGp0yP/F6zqWht7S2VApzbFuASWw5/hUphYBvjR4oFqnEzoQfo9iBRpwzLkkgw1NR4UX16SGB5g/nM9LNAdV2rlPHhTcZDamQ+nCjbsrFGAqypQEibUpnUTBCcowqB4xajmbiQettuVILxDEJbIz7e3/Jn+Vg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784248408; c=relaxed/simple; bh=LUeZ7PoROwJXdCUV2ckt7VTKtyjdbcIz+8qX4WtsqLc=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=qjuVNR5y81GcKaY+ZO04t1b79HHBG8WlW5vDsGaRIdOImSA8NQOK0TxA4g6Coiz5EMsb9bT/TXuufqiIsCtSrHgZwDivxhkJd4DWKBY7dAR1sKpDD9xvWA89SowoDbc2yCCizkGaw8mVmROpq7FB5ioOpbs3eDClQdeKSyIOocg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=USeF9gf2; arc=none smtp.client-ip=209.85.214.177 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="USeF9gf2" Received: by mail-pl1-f177.google.com with SMTP id d9443c01a7336-2cce6a0c9c3so66054405ad.1 for ; Thu, 16 Jul 2026 17:33:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1784248406; x=1784853206; darn=vger.kernel.org; h=content-transfer-encoding:content-type:in-reply-to:from :content-language:references:cc:to:subject:user-agent:mime-version :date:message-id:from:to:cc:subject:date:message-id:reply-to :content-type; bh=1uPNpnjtf15pOqkeyn/OTIVkD0Q5Q/QxgSoCVuNMeg0=; b=USeF9gf2qVZKlAk03bVlbxi0v2wbQy7kXyVTB/Z8EkOv6JXD8zkGGJQFw7ZezBm954 ebmZ95iIBVn++MBKrXUhJfx47LyqHgtzSsQ0GK8SkYlDPxvyek9Nrqy4JBOxLtxrppI/ Wc4lXzRTfcDBYGKeL+wNtWnxybI7Xq/u7mGH2y4eYNQEVDx4+NMWpX5bgpYv272TBgrG UjzRC2s/2PrA2X44XYEPKHS8b0G7pp+IzMVzEkRUQp4WJ+mtwIPvs/Pu+qlMPToPxM+G YLuPESdBgmKYXNQlm893ZYEAV4E+1sRCHh7iduA3wntNLcmfJ/ME1ImpNpTeJKi87n0Y 65+w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1784248406; x=1784853206; h=content-transfer-encoding:content-type:in-reply-to:from :content-language:references:cc:to:subject:user-agent:mime-version :date:message-id:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to:content-type; bh=1uPNpnjtf15pOqkeyn/OTIVkD0Q5Q/QxgSoCVuNMeg0=; b=XSlYKbp66n+NRNFoadezNtRZysg5h6WPtmqSPKVyvDlzCTfFH7nSQahDV3mhF8Q5rU kly31hxUb5vcdywyzbtQ1Zq55Tsth7+kdlp9Jbe364eUvhYchSFBLdVfpMYbtCqUrhno /E2BsnIkBWaLAUNNaWbg4bLAYj838Zrb9KCI2ruXy09sNucUx9ksGqLzQZxDs6fpqxtN F3Ys/P0G37Dw+2R23f8qqm2i5cP3115O5do20p7ptSO/IGHeTJfJ1ZyK42gjm78/aSsO U0vWYwOJqPX/FqyLJ19qtjQLWDufjDpMo+WpKcruZtHdpr7+QOhtv5pDnPbLv/WS+O6/ 9uuw== X-Forwarded-Encrypted: i=1; AHgh+Ro5x/E6T80DS3+CWNs8h9BVqrtdPE1PR0JiNv3m1tzDytiY/z3Hji3SyLD1NVkRtky9Y5xSZePJyXrM@vger.kernel.org X-Gm-Message-State: AOJu0Yz3Y/bHdUd2hqS5veNVnXTbh1kHQ/T0+r9/zPoLfu+DRkGzH10R yf3fUqirjhkIms8DSyr5txR1svjLueLMTIjczQVOM/N2anqTVYnA2W2C X-Gm-Gg: AfdE7cm9Uh9z3XNjc5CKrRx/DEijD3HjLx/hYlZD37Ve86R36f2kN/tgx6Fr2iRaCnj flnRnF0lUEhdYE1HHud2vkurOyh0Ru0gK9LEfuGchMqeu2yV+imvLgQ4l+cRtCX1cDKCK6tKAdy VaC2nTfgkmVK2S1dsdkNgGOj4//2pJTl4ytXQIzW9Lsld34/eQRfh2KTUQxm9CEXKFeTbIRTYyO zoqX5qhZRHLR7Xul7qEHIWuzvnNB6H8G6v1ZLqUyZmwETmVRSJNp39DXvtNjVpnZ9fX4/Ws96ci zV1PGWhlOmg0gHjd7RtQKHT+nWDSQOwHWJ+NRNTnirH+4UlJgRvcWI8H3wiPdND03DL07mqvijZ fx+fVP4KWwk7H5CvO9k9rg/vXVOB7xwVYZHEcYMdAlnkBXEFnz73EjdRg2YjP3cduFdiQbNZnzm UBlKzAGrtXyaz/B9gpnoNUZD23hpKjKiggj4Sb0WxWg+78DhZiuOjXWA== X-Received: by 2002:a17:903:3807:b0:2c2:bd7f:ccd4 with SMTP id d9443c01a7336-2cf349175f9mr3229355ad.21.1784248406238; Thu, 16 Jul 2026 17:33:26 -0700 (PDT) Received: from [172.19.1.42] (60-250-196-139.hinet-ip.hinet.net. [60.250.196.139]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-2cf344f4481sm1139735ad.32.2026.07.16.17.33.23 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 16 Jul 2026 17:33:25 -0700 (PDT) Message-ID: Date: Fri, 17 Jul 2026 08:33:22 +0800 Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 2/2] pwm: Add Nuvoton MA35D1 PWM controller support To: =?UTF-8?Q?Uwe_Kleine-K=C3=B6nig?= Cc: robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, linux-arm-kernel@lists.infradead.org, linux-pwm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, cwweng@nuvoton.com, Trevor Gamblin References: <20260617025925.2539334-1-cwweng.linux@gmail.com> <20260617025925.2539334-3-cwweng.linux@gmail.com> Content-Language: en-US From: Chi-Wen Weng In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Uwe Kleine-König 於 2026/7/16 下午 11:29 寫道: > Hello, > > On Wed, Jun 17, 2026 at 10:59:25AM +0800, Chi-Wen Weng wrote: >> +#include >> +#include >> +#include >> +#include >> +#include > Please don't include that file, should pull in > the things you need from that file. > >> +#include >> +#include >> +#include >> + >> +#define MA35D1_REG_PWM_CTL0 0x00 >> +#define MA35D1_REG_PWM_CTL1 0x04 >> +#define MA35D1_REG_PWM_CNTEN 0x20 >> +#define MA35D1_REG_PWM_PERIOD(ch) (0x30 + 4 * (ch)) >> +#define MA35D1_REG_PWM_CMPDAT(ch) (0x50 + 4 * (ch)) >> +#define MA35D1_REG_PWM_WGCTL0 0xb0 >> +#define MA35D1_REG_PWM_WGCTL1 0xb4 >> +#define MA35D1_REG_PWM_POLCTL 0xd4 >> +#define MA35D1_REG_PWM_POEN 0xd8 >> + >> +#define MA35D1_PWM_CTL1_CNTMODE_MASK(ch) BIT(16 + (ch)) >> +#define MA35D1_PWM_CTL1_OUTMODE_MASK(ch) BIT(24 + ((ch) / 2)) >> + >> +#define MA35D1_PWM_WGCTL_ACTION_MASK 0x3 >> +#define MA35D1_PWM_WGCTL_ACTION_LOW 1 >> +#define MA35D1_PWM_WGCTL_ACTION_HIGH 2 > If you make this: > > #define MA35D1_PWM_WGCTL_ACTION(ch) GENMASK(2 * (ch) + 2, 2 * (ch)) > #define MA35D1_PWM_WGCTL_ACTION_LOW 1 > #define MA35D1_PWM_WGCTL_ACTION_HIGH 2 > > you can drop the static inlines below. > >> + >> +#define MA35D1_PWM_WGCTL_ZERO_HIGH(ch) \ >> + (MA35D1_PWM_WGCTL_ACTION_HIGH << (2 * (ch))) >> +#define MA35D1_PWM_WGCTL_CMP_UP_LOW(ch) \ >> + (MA35D1_PWM_WGCTL_ACTION_LOW << (2 * (ch))) >> + >> +#define MA35D1_PWM_CNTEN_EN(ch) BIT(ch) >> +#define MA35D1_PWM_POEN_EN(ch) BIT(ch) >> +#define MA35D1_PWM_POLCTL_INV(ch) BIT(ch) >> + >> +#define MA35D1_PWM_MAX_CMPDAT 0xffff >> +#define MA35D1_PWM_MAX_PERIOD 0xfffe >> +#define MA35D1_PWM_MAX_PERIOD_CYCLES (MA35D1_PWM_MAX_PERIOD + 1) > This is irritating with similar names and different values/semantic. > >> +#define MA35D1_PWM_NUM_CHANNELS 6 >> + >> [...] >> +static int nuvoton_pwm_probe(struct platform_device *pdev) >> +{ >> [...] >> + nuvoton_pwm_init(nvtpwm); > This clobbers what the hardware is doing. The idea here is to not modify > the hardware settings at probe time to keep e.g. a backlight configured > as it was setup by the bootloader and only modify on explicit calls to > .apply(). > >> + >> + chip->ops = &nuvoton_pwm_ops; >> + chip->atomic = true; >> + >> + ret = devm_pwmchip_add(dev, chip); >> + if (ret) >> + return dev_err_probe(dev, ret, "Unable to add PWM chip\n"); >> + >> + return 0; >> +} > Best regards > Uwe Hi Uwe, Thanks for the review. On the include, I will drop as already provides what is needed here. For the WGCTL helpers, I will rework this to avoid the extra static inline helpers and compute the 2-bit action field mask/value locally when configuring a channel. The WGCTL action field is 2 bits wide, so I will use the bit range [2 * ch + 1 : 2 * ch] for the mask. I will also rename the maximum value definitions to make the semantics clearer. The intent is to keep PERIOD below the 16-bit register field maximum so that CMPDAT can be programmed greater than PERIOD to generate a 100% duty cycle. Most importantly, you are right about the probe-time initialization. The driver should not reconfigure CTL1/WGCTL for all channels during probe, as that can disturb a PWM output already configured and enabled by firmware, such as a backlight. In the next version I will remove the controller initialization from probe(). Instead, the driver will configure only the channel being changed from the .apply() callback. The disable path will only clear POENn and CNTENn and will not touch the waveform/control configuration. Best regards, Chi-Wen