Re: [v3 2/2] pwm: atcpit100: add Andes PWM driver support

[Krzysztof Kozlowski <krzk@kernel.org>]

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