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
