On 23/01/2025 20:35, Ben Zong-You Xie wrote:
>
> +config PWM_ATCPIT100
> + tristate "Andes ATCPIT100 PWM support"
> + depends on OF && HAS_IOMEM
> + depends on RISCV || COMPILE_TEST
> + select REGMAP_MMIO
> + help
> + Generic PWM framework driver for ATCPIT100 on Andes AE350 platform
Is AE350 a type of a SoC? Looks like. "depends on RISCV" is wrong -
there is nothing RISC-V specific here. You must depend on given
SoC/platform.
> +
> + The ATCPIT100 Programmable Interval Timer (PIT) is a set of compact
> + multi-function timers, which can be used as pulse width
> + modulators (PWM) as well as simple timers. ATCPIT100 supports up to 4
> + PIT channels. Each PIT channel can be a simple timer or PWM, or a
> + combination of timer and PWM.
> +
> + To compile this driver as a module, choose M here: the module
...
> +static int atcpit100_pwm_get_state(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + int clk;
> + int ret;
> + unsigned int ctrl_val;
> + unsigned int reload_val;
> + u16 pwm_high;
> + u16 pwm_low;
> + unsigned int channel = pwm->hwpwm;
> + struct atcpit100_pwm *ap = to_atcpit100_pwm(chip);
> +
> + ret = regmap_read(ap->regmap, ATCPIT100_CHANNEL_CTRL(channel),
> + &ctrl_val);
> + if (ret)
> + return ret;
> +
> + clk = (ctrl_val & ATCPIT100_CHANNEL_CTRL_CLK) ? ATCPIT100_CLK_APB
> + : ATCPIT100_CLK_EXT;
> + state->enabled =
Don't wrap here...
> + regmap_test_bits(ap->regmap, ATCPIT100_CHANNEL_ENABLE,
but wrap at arguments.
> + ATCPIT100_CHANNEL_ENABLE_PWM(channel));
> + state->polarity = PWM_POLARITY_NORMAL;
> + ret = regmap_read(ap->regmap, ATCPIT100_CHANNEL_RELOAD(channel),
> + &reload_val);
> + if (ret)
> + return ret;
> +
> + pwm_high = FIELD_GET(ATCPIT100_CHANNEL_RELOAD_HIGH, reload_val);
> + pwm_low = FIELD_GET(ATCPIT100_CHANNEL_RELOAD_LOW, reload_val);
> + state->duty_cycle =
> + DIV64_U64_ROUND_UP((pwm_high + 1) * NSEC_PER_SEC,
> + ap->clk_rate[clk]);
> + state->period =
> + state->duty_cycle +
> + DIV64_U64_ROUND_UP((pwm_low + 1) * NSEC_PER_SEC,
> + ap->clk_rate[clk]);
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops atcpit_pwm_ops = {
> + .apply = atcpit100_pwm_apply,
> + .get_state = atcpit100_pwm_get_state,
> +};
> +
> +static int atcpit100_pwm_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct atcpit100_pwm *ap;
> + struct pwm_chip *chip;
> + void __iomem *base;
> + int ret;
> +
> + chip = devm_pwmchip_alloc(dev, ATCPIT100_CHANNEL_MAX, sizeof(*ap));
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
> +
> + ap = to_atcpit100_pwm(chip);
> + base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + ap->ext_clk = devm_clk_get_enabled(dev, "ext");
> + ap->clk_rate[ATCPIT100_CLK_EXT] = ap->ext_clk ?
> + clk_get_rate(ap->ext_clk) : 0;
> + ap->apb_clk = devm_clk_get_enabled(dev, "apb");
> + ap->clk_rate[ATCPIT100_CLK_APB] = ap->apb_clk ?
> + clk_get_rate(ap->apb_clk) : 0;
> + if (IS_ERR(ap->ext_clk) && IS_ERR(ap->apb_clk)) {
Drop {}. See Linux coding style.
> + return dev_err_probe(dev, PTR_ERR(ap->ext_clk),
> + "failed to obtain any clock\n");
> + }
> +
> + if (ap->clk_rate[ATCPIT100_CLK_EXT] > NSEC_PER_SEC ||
> + ap->clk_rate[ATCPIT100_CLK_APB] > NSEC_PER_SEC)
> + return dev_err_probe(dev, -EINVAL, "pwm clock out of range\n");
> +
> + ap->regmap = devm_regmap_init_mmio(dev, base,
> + &atcpit100_pwm_regmap_config);
> + if (IS_ERR(ap->regmap)) {
Drop {}. See Linux coding style.
> + return dev_err_probe(dev, PTR_ERR(ap->regmap),
> + "failed to init register map\n");
> + }
> +
> + chip->ops = &atcpit_pwm_ops;
> + ret = devm_pwmchip_add(dev, chip);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to add pwm chip\n");
> +
> + dev_info(dev, "pwm driver probed\n");
Drop all such "success" messages.
Best regards,
Krzysztof
