| Message ID | 3985690b29340982a45314bdcc914c554621e909.1725536870.git.unicorn_wang@outlook.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: Add pwm driver for Sophgo SG2042 | expand |
Hello, On Thu, Sep 05, 2024 at 08:10:42PM +0800, Chen Wang wrote: > From: Chen Wang <unicorn_wang@outlook.com> > > Add a PWM driver for PWM controller in Sophgo SG2042 SoC. > > Signed-off-by: Chen Wang <unicorn_wang@outlook.com> > --- > drivers/pwm/Kconfig | 9 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-sophgo-sg2042.c | 148 ++++++++++++++++++++++++++++++++ > 3 files changed, 158 insertions(+) > create mode 100644 drivers/pwm/pwm-sophgo-sg2042.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 3e53838990f5..6287d63a84fd 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -577,6 +577,15 @@ config PWM_SL28CPLD > To compile this driver as a module, choose M here: the module > will be called pwm-sl28cpld. > > +config PWM_SOPHGO_SG2042 > + tristate "Sophgo SG2042 PWM support" > + depends on ARCH_SOPHGO || COMPILE_TEST > + help > + PWM driver for Sophgo SG2042 PWM controller. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm_sophgo_sg2042. > + > config PWM_SPEAR > tristate "STMicroelectronics SPEAr PWM support" > depends on PLAT_SPEAR || COMPILE_TEST > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 0be4f3e6dd43..ef2555e83183 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -52,6 +52,7 @@ obj-$(CONFIG_PWM_RZ_MTU3) += pwm-rz-mtu3.o > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o > obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o > obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o > +obj-$(CONFIG_PWM_SOPHGO_SG2042) += pwm-sophgo-sg2042.o > obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o > obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o > obj-$(CONFIG_PWM_STI) += pwm-sti.o > diff --git a/drivers/pwm/pwm-sophgo-sg2042.c b/drivers/pwm/pwm-sophgo-sg2042.c > new file mode 100644 > index 000000000000..cf11ad54b4de > --- /dev/null > +++ b/drivers/pwm/pwm-sophgo-sg2042.c > @@ -0,0 +1,148 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Sophgo SG2042 PWM Controller Driver > + * > + * Copyright (C) 2024 Sophgo Technology Inc. > + * Copyright (C) 2024 Chen Wang <unicorn_wang@outlook.com> > + */ > + > +#include <linux/clk.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > + > +#include <asm/div64.h> > + > +/* > + * Offset RegisterName > + * 0x0000 HLPERIOD0 > + * 0x0004 PERIOD0 > + * 0x0008 HLPERIOD1 > + * 0x000C PERIOD1 > + * 0x0010 HLPERIOD2 > + * 0x0014 PERIOD2 > + * 0x0018 HLPERIOD3 > + * 0x001C PERIOD3 > + * Four groups and every group is composed of HLPERIOD & PERIOD > + */ > +#define REG_HLPERIOD 0x0 > +#define REG_PERIOD 0x4 > + > +#define REG_GROUP 0x8 > + > +#define SG2042_PWM_CHANNELNUM 4 > + > +/** > + * struct sg2042_pwm_chip - private data of PWM chip > + * @base: base address of mapped PWM registers > + * @base_clk: base clock used to drive the pwm controller > + */ > +struct sg2042_pwm_chip { > + void __iomem *base; > + struct clk *base_clk; > +}; > + > +static inline > +struct sg2042_pwm_chip *to_sg2042_pwm_chip(struct pwm_chip *chip) > +{ > + return pwmchip_get_drvdata(chip); > +} > + > +static void pwm_sg2042_config(void __iomem *base, unsigned int channo, u32 period, u32 hlperiod) > +{ > + writel(period, base + REG_GROUP * channo + REG_PERIOD); > + writel(hlperiod, base + REG_GROUP * channo + REG_HLPERIOD); > +} I suggest to use the following instead: #define SG2042_HLPERIOD(chan) ((chan) * 8 + 0) #define SG2042_PERIOD(chan) ((chan) * 8 + 4) ... static void pwm_sg2042_config(void __iomem *base, unsigned int chan, u32 period, u32 hlperiod) { writel(period, base + SG2042_PERIOD(chan)); writel(hlperiod, base + SG2042_HLPERIOD(chan)); } The (subjective?) advantage is that the definition of SG2042_HLPERIOD contains information about all channel's HLPERIOD register and the usage in pwm_sg2042_config is obviously(?) right. (Another advantage is that the register names have a prefix unique to the driver, so there is no danger of mixing it up with some other hardware's driver that might also have a register named "PERIOD".) Is this racy? i.e. can it happen that between the two writel the output is defined by the new period and old duty_cycle? How does the hardware behave on reconfiguration? Does it complete the currently running period? Please document that in a comment at the start of the driver like many other drivers do. (See sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c ) > +static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct sg2042_pwm_chip *sg2042_pwm = to_sg2042_pwm_chip(chip); > + u32 hlperiod; > + u32 period; > + u64 f_clk; > + u64 p; > + > + if (!state->enabled) { > + pwm_sg2042_config(sg2042_pwm->base, pwm->hwpwm, 0, 0); > + return 0; > + } Here you're missing (I guess): if (state->polarity == PWM_POLARITY_INVERSED) return -EINVAL; > + /* > + * Period of High level (duty_cycle) = HLPERIOD x Period_clk > + * Period of One Cycle (period) = PERIOD x Period_clk > + */ > + f_clk = clk_get_rate(sg2042_pwm->base_clk); > + > + p = f_clk * state->period; This might overflow. > + do_div(p, NSEC_PER_SEC); > + period = (u32)p; This gets very wrong if p happens to be bigger than U32_MAX. If you ensure f_clk <= NSEC_PER_SEC in .probe() (in combination with a call to clk_rate_exclusive_get()), you can do: period_cycles = min(mul_u64_u64_div_u64(f_clk, state->period, NSEC_PER_SEC), U32_MAX); duty_cycles = min(mul_u64_u64_div_u64(f_clk, state->duty_cycle, NSEC_PER_SEC), U32_MAX); This would also allow to store the clkrate in the driver private struct and drop the pointer to the clk from there. > + p = f_clk * state->duty_cycle; > + do_div(p, NSEC_PER_SEC); > + hlperiod = (u32)p; > + > + dev_dbg(pwmchip_parent(chip), "chan[%d]: period=%u, hlperiod=%u\n", > + pwm->hwpwm, period, hlperiod); pwm->hwpwm is an unsigned int, so use %u for it. > + pwm_sg2042_config(sg2042_pwm->base, pwm->hwpwm, period, hlperiod); > + > + return 0; > +} > + > +static const struct pwm_ops pwm_sg2042_ops = { > + .apply = pwm_sg2042_apply, No .get_state() possible? Please use a single space before =. > +}; > + > +static const struct of_device_id sg2042_pwm_match[] = { > + { .compatible = "sophgo,sg2042-pwm" }, > + { }, Please drop the , after the sentinel entry. > +}; > +MODULE_DEVICE_TABLE(of, sg2042_pwm_match); > + > +static int pwm_sg2042_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct sg2042_pwm_chip *sg2042_pwm; > + struct pwm_chip *chip; > + int ret; > + > + chip = devm_pwmchip_alloc(&pdev->dev, SG2042_PWM_CHANNELNUM, sizeof(*sg2042_pwm)); > + if (IS_ERR(chip)) > + return PTR_ERR(chip); > + sg2042_pwm = to_sg2042_pwm_chip(chip); > + > + chip->ops = &pwm_sg2042_ops; > + > + sg2042_pwm->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(sg2042_pwm->base)) > + return PTR_ERR(sg2042_pwm->base); > + > + sg2042_pwm->base_clk = devm_clk_get_enabled(&pdev->dev, "apb"); > + if (IS_ERR(sg2042_pwm->base_clk)) > + return dev_err_probe(dev, PTR_ERR(sg2042_pwm->base_clk), > + "failed to get base clk\n"); > + > + ret = devm_pwmchip_add(&pdev->dev, chip); > + if (ret < 0) > + return dev_err_probe(dev, ret, "failed to register PWM chip\n"); > + > + platform_set_drvdata(pdev, chip); This is unused and should/can be dropped. > + > + return 0; > +} > + > +static struct platform_driver pwm_sg2042_driver = { > + .driver = { > + .name = "sg2042-pwm", > + .of_match_table = of_match_ptr(sg2042_pwm_match), > + }, > + .probe = pwm_sg2042_probe, > +}; > +module_platform_driver(pwm_sg2042_driver); Please use a single space before =. > +MODULE_AUTHOR("Chen Wang"); > +MODULE_DESCRIPTION("Sophgo SG2042 PWM driver"); > +MODULE_LICENSE("GPL"); > -- > 2.34.1 > Best regards Uwe
Hi Chen, kernel test robot noticed the following build warnings: [auto build test WARNING on 431c1646e1f86b949fa3685efc50b660a364c2b6] url: https://github.com/intel-lab-lkp/linux/commits/Chen-Wang/dt-bindings-pwm-sophgo-add-bindings-for-sg2042/20240905-201303 base: 431c1646e1f86b949fa3685efc50b660a364c2b6 patch link: https://lore.kernel.org/r/3985690b29340982a45314bdcc914c554621e909.1725536870.git.unicorn_wang%40outlook.com patch subject: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM config: m68k-randconfig-r133-20240907 (https://download.01.org/0day-ci/archive/20240908/202409080100.h6lX5Asm-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 14.1.0 reproduce: (https://download.01.org/0day-ci/archive/20240908/202409080100.h6lX5Asm-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409080100.h6lX5Asm-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/pwm/pwm-sophgo-sg2042.c:99:34: warning: 'sg2042_pwm_match' defined but not used [-Wunused-const-variable=] 99 | static const struct of_device_id sg2042_pwm_match[] = { | ^~~~~~~~~~~~~~~~ vim +/sg2042_pwm_match +99 drivers/pwm/pwm-sophgo-sg2042.c 98 > 99 static const struct of_device_id sg2042_pwm_match[] = { 100 { .compatible = "sophgo,sg2042-pwm" }, 101 { }, 102 }; 103 MODULE_DEVICE_TABLE(of, sg2042_pwm_match); 104
hi, I wonder why CONFIG_PWM_SOPHGO_SG2042 is enabeld for m68k? Please remove this. Regards, Chen On 2024/9/8 1:58, kernel test robot wrote: > Hi Chen, > > kernel test robot noticed the following build warnings: > > [auto build test WARNING on 431c1646e1f86b949fa3685efc50b660a364c2b6] > > url: https://github.com/intel-lab-lkp/linux/commits/Chen-Wang/dt-bindings-pwm-sophgo-add-bindings-for-sg2042/20240905-201303 > base: 431c1646e1f86b949fa3685efc50b660a364c2b6 > patch link: https://lore.kernel.org/r/3985690b29340982a45314bdcc914c554621e909.1725536870.git.unicorn_wang%40outlook.com > patch subject: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM > config: m68k-randconfig-r133-20240907 (https://download.01.org/0day-ci/archive/20240908/202409080100.h6lX5Asm-lkp@intel.com/config) > compiler: m68k-linux-gcc (GCC) 14.1.0 > reproduce: (https://download.01.org/0day-ci/archive/20240908/202409080100.h6lX5Asm-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202409080100.h6lX5Asm-lkp@intel.com/ > > All warnings (new ones prefixed by >>): > >>> drivers/pwm/pwm-sophgo-sg2042.c:99:34: warning: 'sg2042_pwm_match' defined but not used [-Wunused-const-variable=] > 99 | static const struct of_device_id sg2042_pwm_match[] = { > | ^~~~~~~~~~~~~~~~ > > > vim +/sg2042_pwm_match +99 drivers/pwm/pwm-sophgo-sg2042.c > > 98 > > 99 static const struct of_device_id sg2042_pwm_match[] = { > 100 { .compatible = "sophgo,sg2042-pwm" }, > 101 { }, > 102 }; > 103 MODULE_DEVICE_TABLE(of, sg2042_pwm_match); > 104 >
Hi Chen, On Mon, Sep 9, 2024 at 10:26 AM Chen Wang <unicorn_wang@outlook.com> wrote: > I wonder why CONFIG_PWM_SOPHGO_SG2042 is enabeld for m68k? Please remove > this. Because it depends on ARCH_SOPHGO || COMPILE_TEST. So it can be enabled on all architectures when compile-testing. > On 2024/9/8 1:58, kernel test robot wrote: > > kernel test robot noticed the following build warnings: > > > > [auto build test WARNING on 431c1646e1f86b949fa3685efc50b660a364c2b6] > > > > url: https://github.com/intel-lab-lkp/linux/commits/Chen-Wang/dt-bindings-pwm-sophgo-add-bindings-for-sg2042/20240905-201303 > > base: 431c1646e1f86b949fa3685efc50b660a364c2b6 > > patch link: https://lore.kernel.org/r/3985690b29340982a45314bdcc914c554621e909.1725536870.git.unicorn_wang%40outlook.com > > patch subject: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM > > config: m68k-randconfig-r133-20240907 (https://download.01.org/0day-ci/archive/20240908/202409080100.h6lX5Asm-lkp@intel.com/config) > > compiler: m68k-linux-gcc (GCC) 14.1.0 > > reproduce: (https://download.01.org/0day-ci/archive/20240908/202409080100.h6lX5Asm-lkp@intel.com/reproduce) > > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > > the same patch/commit), kindly add following tags > > | Reported-by: kernel test robot <lkp@intel.com> > > | Closes: https://lore.kernel.org/oe-kbuild-all/202409080100.h6lX5Asm-lkp@intel.com/ > > > > All warnings (new ones prefixed by >>): > > > >>> drivers/pwm/pwm-sophgo-sg2042.c:99:34: warning: 'sg2042_pwm_match' defined but not used [-Wunused-const-variable=] > > 99 | static const struct of_device_id sg2042_pwm_match[] = { > > | ^~~~~~~~~~~~~~~~ > > > > > > vim +/sg2042_pwm_match +99 drivers/pwm/pwm-sophgo-sg2042.c > > > > 98 > > > 99 static const struct of_device_id sg2042_pwm_match[] = { > > 100 { .compatible = "sophgo,sg2042-pwm" }, > > 101 { }, > > 102 }; > > 103 MODULE_DEVICE_TABLE(of, sg2042_pwm_match); > > 104 Gr{oetje,eeting}s, Geert
On 2024/9/9 16:45, Geert Uytterhoeven wrote: > Hi Chen, > > On Mon, Sep 9, 2024 at 10:26 AM Chen Wang <unicorn_wang@outlook.com> wrote: >> I wonder why CONFIG_PWM_SOPHGO_SG2042 is enabeld for m68k? Please remove >> this. > Because it depends on ARCH_SOPHGO || COMPILE_TEST. > So it can be enabled on all architectures when compile-testing. Thanks, it's really a potential defect when CONFIG_OF is not set, will fix this in next version. >> On 2024/9/8 1:58, kernel test robot wrote: >>> kernel test robot noticed the following build warnings: >>> >>> [auto build test WARNING on 431c1646e1f86b949fa3685efc50b660a364c2b6] >>> >>> url: https://github.com/intel-lab-lkp/linux/commits/Chen-Wang/dt-bindings-pwm-sophgo-add-bindings-for-sg2042/20240905-201303 >>> base: 431c1646e1f86b949fa3685efc50b660a364c2b6 >>> patch link: https://lore.kernel.org/r/3985690b29340982a45314bdcc914c554621e909.1725536870.git.unicorn_wang%40outlook.com >>> patch subject: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM >>> config: m68k-randconfig-r133-20240907 (https://download.01.org/0day-ci/archive/20240908/202409080100.h6lX5Asm-lkp@intel.com/config) >>> compiler: m68k-linux-gcc (GCC) 14.1.0 >>> reproduce: (https://download.01.org/0day-ci/archive/20240908/202409080100.h6lX5Asm-lkp@intel.com/reproduce) >>> >>> If you fix the issue in a separate patch/commit (i.e. not just a new version of >>> the same patch/commit), kindly add following tags >>> | Reported-by: kernel test robot <lkp@intel.com> >>> | Closes: https://lore.kernel.org/oe-kbuild-all/202409080100.h6lX5Asm-lkp@intel.com/ >>> >>> All warnings (new ones prefixed by >>): >>> >>>>> drivers/pwm/pwm-sophgo-sg2042.c:99:34: warning: 'sg2042_pwm_match' defined but not used [-Wunused-const-variable=] >>> 99 | static const struct of_device_id sg2042_pwm_match[] = { >>> | ^~~~~~~~~~~~~~~~ >>> >>> >>> vim +/sg2042_pwm_match +99 drivers/pwm/pwm-sophgo-sg2042.c >>> >>> 98 >>> > 99 static const struct of_device_id sg2042_pwm_match[] = { >>> 100 { .compatible = "sophgo,sg2042-pwm" }, >>> 101 { }, >>> 102 }; >>> 103 MODULE_DEVICE_TABLE(of, sg2042_pwm_match); >>> 104 > Gr{oetje,eeting}s, > > Geert >
On 2024/9/6 0:03, Uwe Kleine-König wrote: > Hello, > > On Thu, Sep 05, 2024 at 08:10:42PM +0800, Chen Wang wrote: [......] >> +static void pwm_sg2042_config(void __iomem *base, unsigned int channo, u32 period, u32 hlperiod) >> +{ >> + writel(period, base + REG_GROUP * channo + REG_PERIOD); >> + writel(hlperiod, base + REG_GROUP * channo + REG_HLPERIOD); >> +} > I suggest to use the following instead: > > #define SG2042_HLPERIOD(chan) ((chan) * 8 + 0) > #define SG2042_PERIOD(chan) ((chan) * 8 + 4) > > ... > > static void pwm_sg2042_config(void __iomem *base, unsigned int chan, u32 period, u32 hlperiod) > { > writel(period, base + SG2042_PERIOD(chan)); > writel(hlperiod, base + SG2042_HLPERIOD(chan)); > } > > The (subjective?) advantage is that the definition of SG2042_HLPERIOD > contains information about all channel's HLPERIOD register and the usage > in pwm_sg2042_config is obviously(?) right. > > (Another advantage is that the register names have a prefix unique to > the driver, so there is no danger of mixing it up with some other > hardware's driver that might also have a register named "PERIOD".) Agree, will fix this in next version. > Is this racy? i.e. can it happen that between the two writel the output > is defined by the new period and old duty_cycle? > > How does the hardware behave on reconfiguration? Does it complete the > currently running period? Please document that in a comment at the start > of the driver like many other drivers do. (See > > sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c > > ) When hlperiod or period is modified, PWM will start to output waveforms with the new configuration after completing the running period. Will document in a comment as other drivers do. >> +static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm, >> + const struct pwm_state *state) >> +{ >> + struct sg2042_pwm_chip *sg2042_pwm = to_sg2042_pwm_chip(chip); >> + u32 hlperiod; >> + u32 period; >> + u64 f_clk; >> + u64 p; >> + >> + if (!state->enabled) { >> + pwm_sg2042_config(sg2042_pwm->base, pwm->hwpwm, 0, 0); >> + return 0; >> + } > Here you're missing (I guess): > > if (state->polarity == PWM_POLARITY_INVERSED) > return -EINVAL; Yes, it is required, will add this in next version, thanks. >> + /* >> + * Period of High level (duty_cycle) = HLPERIOD x Period_clk >> + * Period of One Cycle (period) = PERIOD x Period_clk >> + */ >> + f_clk = clk_get_rate(sg2042_pwm->base_clk); >> + >> + p = f_clk * state->period; > This might overflow. > >> + do_div(p, NSEC_PER_SEC); >> + period = (u32)p; > This gets very wrong if p happens to be bigger than U32_MAX. > > If you ensure f_clk <= NSEC_PER_SEC in .probe() (in combination with a > call to clk_rate_exclusive_get()), you can do: > > period_cycles = min(mul_u64_u64_div_u64(f_clk, state->period, NSEC_PER_SEC), U32_MAX); > duty_cycles = min(mul_u64_u64_div_u64(f_clk, state->duty_cycle, NSEC_PER_SEC), U32_MAX); > > This would also allow to store the clkrate in the driver private struct > and drop the pointer to the clk from there. f_clk should be 100M and <=NSEC_PER_SEC, will improve the code as your suggestion, thanks. >> + p = f_clk * state->duty_cycle; >> + do_div(p, NSEC_PER_SEC); >> + hlperiod = (u32)p; >> + >> + dev_dbg(pwmchip_parent(chip), "chan[%d]: period=%u, hlperiod=%u\n", >> + pwm->hwpwm, period, hlperiod); > pwm->hwpwm is an unsigned int, so use %u for it. Ok, will fix. >> + pwm_sg2042_config(sg2042_pwm->base, pwm->hwpwm, period, hlperiod); >> + >> + return 0; >> +} >> + >> +static const struct pwm_ops pwm_sg2042_ops = { >> + .apply = pwm_sg2042_apply, > No .get_state() possible? Please use a single space before =. Will add .get_state() and improve the format. >> +}; >> + >> +static const struct of_device_id sg2042_pwm_match[] = { >> + { .compatible = "sophgo,sg2042-pwm" }, >> + { }, > Please drop the , after the sentinel entry. OK >> +}; >> +MODULE_DEVICE_TABLE(of, sg2042_pwm_match); >> + >> +static int pwm_sg2042_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct sg2042_pwm_chip *sg2042_pwm; >> + struct pwm_chip *chip; >> + int ret; >> + >> + chip = devm_pwmchip_alloc(&pdev->dev, SG2042_PWM_CHANNELNUM, sizeof(*sg2042_pwm)); >> + if (IS_ERR(chip)) >> + return PTR_ERR(chip); >> + sg2042_pwm = to_sg2042_pwm_chip(chip); >> + >> + chip->ops = &pwm_sg2042_ops; >> + >> + sg2042_pwm->base = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(sg2042_pwm->base)) >> + return PTR_ERR(sg2042_pwm->base); >> + >> + sg2042_pwm->base_clk = devm_clk_get_enabled(&pdev->dev, "apb"); >> + if (IS_ERR(sg2042_pwm->base_clk)) >> + return dev_err_probe(dev, PTR_ERR(sg2042_pwm->base_clk), >> + "failed to get base clk\n"); >> + >> + ret = devm_pwmchip_add(&pdev->dev, chip); >> + if (ret < 0) >> + return dev_err_probe(dev, ret, "failed to register PWM chip\n"); >> + >> + platform_set_drvdata(pdev, chip); > This is unused and should/can be dropped. OK, will drop this, thanks for point it out. I should have cleaned it up. >> + >> + return 0; >> +} >> + >> +static struct platform_driver pwm_sg2042_driver = { >> + .driver = { >> + .name = "sg2042-pwm", >> + .of_match_table = of_match_ptr(sg2042_pwm_match), >> + }, >> + .probe = pwm_sg2042_probe, >> +}; >> +module_platform_driver(pwm_sg2042_driver); > Please use a single space before =. Ok, will improve the format. >> +MODULE_AUTHOR("Chen Wang"); >> +MODULE_DESCRIPTION("Sophgo SG2042 PWM driver"); >> +MODULE_LICENSE("GPL"); >> -- >> 2.34.1 >> > Best regards > Uwe
Hello Chen, On Mon, Sep 09, 2024 at 05:08:41PM +0800, Chen Wang wrote: > On 2024/9/9 16:45, Geert Uytterhoeven wrote: > > On Mon, Sep 9, 2024 at 10:26 AM Chen Wang <unicorn_wang@outlook.com> wrote: > > > I wonder why CONFIG_PWM_SOPHGO_SG2042 is enabeld for m68k? Please remove > > > this. > > Because it depends on ARCH_SOPHGO || COMPILE_TEST. > > So it can be enabled on all architectures when compile-testing. > Thanks, it's really a potential defect when CONFIG_OF is not set, will fix > this in next version. BTW, the right approach to fix that is to not use of_match_ptr when initializing pwm_sg2042_driver.driver.of_match_table. Best regards Uwe
On 2024/9/9 19:45, Uwe Kleine-König wrote: > Hello Chen, > > On Mon, Sep 09, 2024 at 05:08:41PM +0800, Chen Wang wrote: >> On 2024/9/9 16:45, Geert Uytterhoeven wrote: >>> On Mon, Sep 9, 2024 at 10:26 AM Chen Wang <unicorn_wang@outlook.com> wrote: >>>> I wonder why CONFIG_PWM_SOPHGO_SG2042 is enabeld for m68k? Please remove >>>> this. >>> Because it depends on ARCH_SOPHGO || COMPILE_TEST. >>> So it can be enabled on all architectures when compile-testing. >> Thanks, it's really a potential defect when CONFIG_OF is not set, will fix >> this in next version. > BTW, the right approach to fix that is to not use of_match_ptr when > initializing pwm_sg2042_driver.driver.of_match_table. > > Best regards > Uwe Thank you Uwe, that's what I said potential defect, have fixed here. Thanks, Chen
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 3e53838990f5..6287d63a84fd 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -577,6 +577,15 @@ config PWM_SL28CPLD To compile this driver as a module, choose M here: the module will be called pwm-sl28cpld. +config PWM_SOPHGO_SG2042 + tristate "Sophgo SG2042 PWM support" + depends on ARCH_SOPHGO || COMPILE_TEST + help + PWM driver for Sophgo SG2042 PWM controller. + + To compile this driver as a module, choose M here: the module + will be called pwm_sophgo_sg2042. + config PWM_SPEAR tristate "STMicroelectronics SPEAr PWM support" depends on PLAT_SPEAR || COMPILE_TEST diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 0be4f3e6dd43..ef2555e83183 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -52,6 +52,7 @@ obj-$(CONFIG_PWM_RZ_MTU3) += pwm-rz-mtu3.o obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o +obj-$(CONFIG_PWM_SOPHGO_SG2042) += pwm-sophgo-sg2042.o obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o obj-$(CONFIG_PWM_STI) += pwm-sti.o diff --git a/drivers/pwm/pwm-sophgo-sg2042.c b/drivers/pwm/pwm-sophgo-sg2042.c new file mode 100644 index 000000000000..cf11ad54b4de --- /dev/null +++ b/drivers/pwm/pwm-sophgo-sg2042.c @@ -0,0 +1,148 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Sophgo SG2042 PWM Controller Driver + * + * Copyright (C) 2024 Sophgo Technology Inc. + * Copyright (C) 2024 Chen Wang <unicorn_wang@outlook.com> + */ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> + +#include <asm/div64.h> + +/* + * Offset RegisterName + * 0x0000 HLPERIOD0 + * 0x0004 PERIOD0 + * 0x0008 HLPERIOD1 + * 0x000C PERIOD1 + * 0x0010 HLPERIOD2 + * 0x0014 PERIOD2 + * 0x0018 HLPERIOD3 + * 0x001C PERIOD3 + * Four groups and every group is composed of HLPERIOD & PERIOD + */ +#define REG_HLPERIOD 0x0 +#define REG_PERIOD 0x4 + +#define REG_GROUP 0x8 + +#define SG2042_PWM_CHANNELNUM 4 + +/** + * struct sg2042_pwm_chip - private data of PWM chip + * @base: base address of mapped PWM registers + * @base_clk: base clock used to drive the pwm controller + */ +struct sg2042_pwm_chip { + void __iomem *base; + struct clk *base_clk; +}; + +static inline +struct sg2042_pwm_chip *to_sg2042_pwm_chip(struct pwm_chip *chip) +{ + return pwmchip_get_drvdata(chip); +} + +static void pwm_sg2042_config(void __iomem *base, unsigned int channo, u32 period, u32 hlperiod) +{ + writel(period, base + REG_GROUP * channo + REG_PERIOD); + writel(hlperiod, base + REG_GROUP * channo + REG_HLPERIOD); +} + +static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct sg2042_pwm_chip *sg2042_pwm = to_sg2042_pwm_chip(chip); + u32 hlperiod; + u32 period; + u64 f_clk; + u64 p; + + if (!state->enabled) { + pwm_sg2042_config(sg2042_pwm->base, pwm->hwpwm, 0, 0); + return 0; + } + + /* + * Period of High level (duty_cycle) = HLPERIOD x Period_clk + * Period of One Cycle (period) = PERIOD x Period_clk + */ + f_clk = clk_get_rate(sg2042_pwm->base_clk); + + p = f_clk * state->period; + do_div(p, NSEC_PER_SEC); + period = (u32)p; + + p = f_clk * state->duty_cycle; + do_div(p, NSEC_PER_SEC); + hlperiod = (u32)p; + + dev_dbg(pwmchip_parent(chip), "chan[%d]: period=%u, hlperiod=%u\n", + pwm->hwpwm, period, hlperiod); + + pwm_sg2042_config(sg2042_pwm->base, pwm->hwpwm, period, hlperiod); + + return 0; +} + +static const struct pwm_ops pwm_sg2042_ops = { + .apply = pwm_sg2042_apply, +}; + +static const struct of_device_id sg2042_pwm_match[] = { + { .compatible = "sophgo,sg2042-pwm" }, + { }, +}; +MODULE_DEVICE_TABLE(of, sg2042_pwm_match); + +static int pwm_sg2042_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct sg2042_pwm_chip *sg2042_pwm; + struct pwm_chip *chip; + int ret; + + chip = devm_pwmchip_alloc(&pdev->dev, SG2042_PWM_CHANNELNUM, sizeof(*sg2042_pwm)); + if (IS_ERR(chip)) + return PTR_ERR(chip); + sg2042_pwm = to_sg2042_pwm_chip(chip); + + chip->ops = &pwm_sg2042_ops; + + sg2042_pwm->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(sg2042_pwm->base)) + return PTR_ERR(sg2042_pwm->base); + + sg2042_pwm->base_clk = devm_clk_get_enabled(&pdev->dev, "apb"); + if (IS_ERR(sg2042_pwm->base_clk)) + return dev_err_probe(dev, PTR_ERR(sg2042_pwm->base_clk), + "failed to get base clk\n"); + + ret = devm_pwmchip_add(&pdev->dev, chip); + if (ret < 0) + return dev_err_probe(dev, ret, "failed to register PWM chip\n"); + + platform_set_drvdata(pdev, chip); + + return 0; +} + +static struct platform_driver pwm_sg2042_driver = { + .driver = { + .name = "sg2042-pwm", + .of_match_table = of_match_ptr(sg2042_pwm_match), + }, + .probe = pwm_sg2042_probe, +}; +module_platform_driver(pwm_sg2042_driver); + +MODULE_AUTHOR("Chen Wang"); +MODULE_DESCRIPTION("Sophgo SG2042 PWM driver"); +MODULE_LICENSE("GPL");