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

[Uwe Kleine-König]

Hello Nicolas,

On Tue, Oct 13, 2020 at 01:20:00PM +0200, Nicolas Saenz Julienne wrote:
> On Mon, 2020-10-12 at 09:06 +0200, Uwe Kleine-König wrote:
> > > +	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.

Hmm, this is strange, but if Arnd doesn't know a better solution, then
be it so. Is this idiom usual enough to not justify a comment?

> > 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?

From the consumer's point of view having to hit the only correct period
is hard. So the usual convention is to provide the biggest period not
bigger than the requested one. (The idea for the future is to provide a
pwm_round_state() function which allows to find out the effect of
pwm_apply_state() with the same arguments. This then allows to
efficiently find the best setting for the consumer.)

> > 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.

fine.

> > > +	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.

this prevents the that the module is unloaded, but not that the driver
is unbound.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature