Re: [PATCH 3/3] pwm: Add Raspberry Pi Firmware based PWM bus

[Nicolas Saenz Julienne]

Hi Uwe,

On Mon, 2020-10-12 at 09:06 +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Oct 09, 2020 at 05:30:30PM +0200, Nicolas Saenz Julienne wrote:
> > Adds support to control the PWM bus available in official Raspberry Pi
> > PoE HAT. Only RPi's co-processor has access to it, so commands have to
> > be sent through RPi's firmware mailbox interface.
> > 
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@xxxxxxx>
> > ---
> >  drivers/pwm/Kconfig           |   7 ++
> >  drivers/pwm/Makefile          |   1 +
> >  drivers/pwm/pwm-raspberrypi.c | 216 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 224 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-raspberrypi.c
> > 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 63be5362fd3a..a76997ca37d0 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -379,6 +379,13 @@ config PWM_PXA
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called pwm-pxa.
> >  
> > +config PWM_RASPBERRYPI
> > +	tristate "Raspberry Pi Firwmware PWM support"
> 
> s/Firwmware/Firmware/

Noted, Sorry for that.

> 
> > +	depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> 
> This is more complicated than necessary.
> 
> 	depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
> 
> is logically equivalent.

It's not exactly the same, see patch 7ed915059c300 ("gpio: raspberrypi-ext: fix
firmware dependency ") which explains why this pattern is needed.

[...]

> > +static int raspberrypi_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			         const struct pwm_state *state)
> > +{
> > +	struct raspberrypi_pwm *pc = to_raspberrypi_pwm(chip);
> > +	unsigned int duty_cycle;
> > +	int ret;
> > +
> 
> You need to check for polarity here.
> 
> > +	if (!state->enabled)
> > +		duty_cycle = 0;
> > +	else
> > +		duty_cycle = state->duty_cycle * RPI_PWM_MAX_DUTY /
> > +			     RPI_PWM_PERIOD_NS;
> > +
> > +	if (duty_cycle == pc->duty_cycle)
> > +		return 0;
> > +
> > +	pc->duty_cycle = duty_cycle;
> > +	ret = raspberrypi_pwm_set_property(pc->firmware, RPI_PWM_CUR_DUTY_REG,
> > +					   pc->duty_cycle);
> > +	if (ret) {
> > +		dev_err(chip->dev, "Failed to set duty cycle: %d\n", ret);
> > +		return ret;
> > +	}
> 
> What happens if duty_cycle happens to be bigger than RPI_PWM_MAX_DUTY?
> 
> I think the right thing to do here is:
> 
> 	if (state->period < RPI_PWM_PERIOD_NS ||

Why not using state->period != RPI_PWM_PERIOD_NS here?

> 	    state->polarity != PWM_POLARITY_NORMAL)
> 		return -EINVAL;
> 
> 	if (!state->enabled)
> 		duty_cycle = 0
> 	else if (state->duty_cycle < RPI_PWM_PERIOD_NS)
> 		duty_cycle = state->duty_cycle * RPI_PWM_MAX_DUTY / RPI_PWM_PERIOD_NS;
> 	else
> 		duty_cycle = RPI_PWM_MAX_DUTY;
> 
> 	ret = raspberrypi_pwm_set_property(pc->firmware, RPI_PWM_CUR_DUTY_REG,
> 					   pc->duty_cycle);
> 	if (ret)
> 		...
> 
> 	pc->duty_cycle = duty_cycle;

OK, clearly better this way.

> > +
> > +	ret = raspberrypi_pwm_set_property(pc->firmware, RPI_PWM_CUR_DUTY_REG,
> > +					   pc->duty_cycle);
> > +	if (ret) {
> > +		dev_err(chip->dev, "Failed to set default duty cycle: %d\n", ret);
> > +		return ret;
> > +	}
> 
> Huh, why do you have to do this twice, just with different error
> messages? I assume you want to set RPI_PWM_DEF_DUTY_REG? What is the
> effect of writing this property?

Obviously, I failed to change the register name.

This is supposed to set the default duty cycle after resetting the board. I
added it so as to keep compatibility with the downstream version of this.

I'll add a comment to explain this.

[...]

> > +	pwm->args.period = RPI_PWM_PERIOD_NS;
> > +
> > +	return pwm;
> > +}
> 
> I think you don't need this function. Just fix up period in .apply().

As commented in the dt binding patch, I'll do that.

> > +static int raspberrypi_pwm_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *firmware_node;
> > +	struct device *dev = &pdev->dev;
> > +	struct rpi_firmware *firmware;
> > +	struct raspberrypi_pwm *pc;
> 
> What does "pc" stand for? I'd have used "rpipwm" or something similar.

It was cargo culted from other pwm drivers, I saw it being used on more than
one and figured it was the preferred way of naming things. I'll change it.
> 
> > +	int ret;
> > +
> > +	firmware_node = of_get_parent(dev->of_node);
> > +	if (!firmware_node) {
> > +		dev_err(dev, "Missing firmware node\n");
> > +		return -ENOENT;
> > +	}
> > +
> > +	firmware = rpi_firmware_get(firmware_node);
> > +	of_node_put(firmware_node);
> > +	if (!firmware)
> > +		return -EPROBE_DEFER;
> 
> I don't see a mechanism that prevents the driver providing the firmware
> going away while the PWM is still in use.

There isn't an explicit one. But since you depend on a symbol from the firmware
driver you won't be able to remove the kernel module before removing the PMW
one.

> > +	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> > +	if (!pc)
> > +		return -ENOMEM;
> > [...]
> > +
> > +static struct platform_driver raspberrypi_pwm_driver = {
> > +	.driver = {
> > +		.name = "raspberrypi-pwm",
> > +		.of_match_table = raspberrypi_pwm_of_match,
> > +	},
> > +	.probe = raspberrypi_pwm_probe,
> > +	.remove = raspberrypi_pwm_remove,
> > +};
> > +module_platform_driver(raspberrypi_pwm_driver);
> > +
> > +MODULE_AUTHOR("Nicolas Saenz Julienne <nsaenzjulienne@xxxxxxx>");
> > +MODULE_DESCRIPTION("Raspberry Pi Firwmare Based PWM Bus Driver");
> > +MODULE_LICENSE("GPL v2");
> > +
> 
> Please drop the empty line at the end of file.

Overall I took note of your comments and I'll make the changes. Thanks for the
review.

Regards,
Nicolas

Attachment: signature.asc
Description: This is a digitally signed message part