Re: [PATCH v8 11/11] pwm: Add Raspberry Pi Firmware based PWM bus
Hello Nicolas, On Fri, Mar 12, 2021 at 01:24:54PM +0100, 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 > > --- > > Changes since v7: > - Remove unwarranted RPI_PWM_DEF_DUTY_REG usage > > Changes since v6: > - Use %pe > - Round divisions properly > - Use dev_err_probe() > - Pass check_patch > > Changes since v3: > - Rename compatible string to be more explicit WRT to bus's limitations > > Changes since v2: > - Use devm_rpi_firmware_get() > - Rename driver > - Small cleanups > > Changes since v1: > - Use default pwm bindings and get rid of xlate() function > - Correct spelling errors > - Correct apply() function > - Round values > - Fix divisions in arm32 mode > - Small cleanups > > drivers/pwm/Kconfig | 9 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-raspberrypi-poe.c | 206 ++ > 3 files changed, 216 insertions(+) > create mode 100644 drivers/pwm/pwm-raspberrypi-poe.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index a7a7a9f26aef..d3371ac7b871 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -431,6 +431,15 @@ config PWM_PXA > To compile this driver as a module, choose M here: the module > will be called pwm-pxa. > > +config PWM_RASPBERRYPI_POE > + tristate "Raspberry Pi Firwmware PoE Hat PWM support" > + # Make sure not 'y' when RASPBERRYPI_FIRMWARE is 'm'. This can only > + # happen when COMPILE_TEST=y, hence the added !RASPBERRYPI_FIRMWARE. > + depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && > !RASPBERRYPI_FIRMWARE) > + help > + Enable Raspberry Pi firmware controller PWM bus used to control the > + official RPI PoE hat > + > config PWM_RCAR > tristate "Renesas R-Car PWM support" > depends on ARCH_RENESAS || COMPILE_TEST > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 4e35a55fa7b6..d3879619bd76 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -39,6 +39,7 @@ obj-$(CONFIG_PWM_NTXEC) += pwm-ntxec.o > obj-$(CONFIG_PWM_OMAP_DMTIMER) += pwm-omap-dmtimer.o > obj-$(CONFIG_PWM_PCA9685)+= pwm-pca9685.o > obj-$(CONFIG_PWM_PXA)+= pwm-pxa.o > +obj-$(CONFIG_PWM_RASPBERRYPI_POE)+= pwm-raspberrypi-poe.o > obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o > obj-$(CONFIG_PWM_RENESAS_TPU)+= pwm-renesas-tpu.o > obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o > diff --git a/drivers/pwm/pwm-raspberrypi-poe.c > b/drivers/pwm/pwm-raspberrypi-poe.c > new file mode 100644 > index ..71ade5e55069 > --- /dev/null > +++ b/drivers/pwm/pwm-raspberrypi-poe.c > @@ -0,0 +1,206 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright 2020 Nicolas Saenz Julienne 2021? > + * For more information on Raspberry Pi's PoE hat see: > + * https://www.raspberrypi.org/products/poe-hat/ Out of personal interest: Is this hat also able to power a RPi CM4? > + * Limitations: > + * - No disable bit, so a disabled PWM is simulated by duty_cycle 0 > + * - Only normal polarity > + * - Fixed 12.5 kHz period > + * > + * The current period is completed when HW is reconfigured. > + */ Other than that as mentioned in the previous round: This looks good, Reviewed-by: Uwe Kleine-König What is your thought about how to get this series merged? At least input, staging, armsoc, clk, reset anf firmware are touched. Do you prepare a branch for merging in the relevant trees (once you have all the necessary Acks)? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v7 11/11] pwm: Add Raspberry Pi Firmware based PWM bus
Hello Nicolas,
On Thu, Mar 11, 2021 at 02:01:00PM +0100, Nicolas Saenz Julienne wrote:
> On Wed, 2021-03-10 at 12:50 +0100, Uwe Kleine-König wrote:
> > On Mon, Jan 18, 2021 at 01:32:44PM +0100, Nicolas Saenz Julienne wrote:
>
> [...]
>
> > > + /*
> > > + * This sets the default duty cycle after resetting the board, we
> > > + * updated it every time to mimic Raspberry Pi's downstream's driver
> > > + * behaviour.
> > > + */
> > > + ret = raspberrypi_pwm_set_property(rpipwm->firmware,
> > > RPI_PWM_DEF_DUTY_REG,
> > > +duty_cycle);
> > > + if (ret) {
> > > + dev_err(chip->dev, "Failed to set default duty cycle: %pe\n",
> > > + ERR_PTR(ret));
> > > + return ret;
> >
> > This only has an effect for the next reboot, right?
>
> It effects all reboots until it's further changed.
>
> > If so I wonder if it is a good idea in general. (Think: The current PWM
> > setting enables a motor that makes a self-driving car move at 100 km/h.
> > Consider the rpi crashes, do I want to car to pick up driving 100 km/h at
> > power up even before Linux is up again?)
>
> I get your point. But this isn't used as a general purpose PWM. For now the
> interface is solely there to drive a PWM fan that's arguably harmless. This
> doesn't mean that the RPi foundation will not reuse the firmware interface for
> other means in the future. In such case we can always use a new DT compatible
> and bypass this feature (the current DT string is
> 'raspberrypi,firmware-poe-pwm', which is specific to this use-case).
>
> My aim here is to be on par feature wise with RPi's downstream implementation.
Just because the downstream kernel does it should not be the (single)
reason to do that. My gut feeling is: For a motor restoring the PWM
config on reboot is bad and for a fan it doesn't really hurt if it
doesn't restart automatically. So I'd prefer to to drop this feature.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v7 11/11] pwm: Add Raspberry Pi Firmware based PWM bus
Hello Nicolas,
On Mon, Jan 18, 2021 at 01:32:44PM +0100, Nicolas Saenz Julienne wrote:
> diff --git a/drivers/pwm/pwm-raspberrypi-poe.c
> b/drivers/pwm/pwm-raspberrypi-poe.c
> new file mode 100644
> index ..ca845e8fabe6
> --- /dev/null
> +++ b/drivers/pwm/pwm-raspberrypi-poe.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2020 Nicolas Saenz Julienne
> + * For more information on Raspberry Pi's PoE hat see:
> + * https://www.raspberrypi.org/products/poe-hat/
> + *
> + * Limitations:
> + * - No disable bit, so a disabled PWM is simulated by duty_cycle 0
> + * - Only normal polarity
> + * - Fixed 12.5 kHz period
> + *
> + * The current period is completed when HW is reconfigured.
nice.
> + */
> +
> [...]
> +static int raspberrypi_pwm_apply(struct pwm_chip *chip, struct pwm_device
> *pwm,
> + const struct pwm_state *state)
> +{
> + struct raspberrypi_pwm *rpipwm = raspberrypi_pwm_from_chip(chip);
> + unsigned int duty_cycle;
> + int ret;
> +
> + if (state->period < RPI_PWM_PERIOD_NS ||
> + state->polarity != PWM_POLARITY_NORMAL)
> + return -EINVAL;
> +
> + if (!state->enabled)
> + duty_cycle = 0;
> + else if (state->duty_cycle < RPI_PWM_PERIOD_NS)
> + duty_cycle = DIV_ROUND_DOWN_ULL(state->duty_cycle *
> RPI_PWM_MAX_DUTY,
> + RPI_PWM_PERIOD_NS);
> + else
> + duty_cycle = RPI_PWM_MAX_DUTY;
> +
> + if (duty_cycle == rpipwm->duty_cycle)
> + return 0;
> +
> + ret = raspberrypi_pwm_set_property(rpipwm->firmware,
> RPI_PWM_CUR_DUTY_REG,
> +duty_cycle);
> + if (ret) {
> + dev_err(chip->dev, "Failed to set duty cycle: %pe\n",
> + ERR_PTR(ret));
> + return ret;
> + }
> +
> + /*
> + * This sets the default duty cycle after resetting the board, we
> + * updated it every time to mimic Raspberry Pi's downstream's driver
> + * behaviour.
> + */
> + ret = raspberrypi_pwm_set_property(rpipwm->firmware,
> RPI_PWM_DEF_DUTY_REG,
> +duty_cycle);
> + if (ret) {
> + dev_err(chip->dev, "Failed to set default duty cycle: %pe\n",
> + ERR_PTR(ret));
> + return ret;
This only has an effect for the next reboot, right? If so I wonder if it
is a good idea in general. (Think: The current PWM setting enables a
motor that makes a self-driving car move at 100 km/h. Consider the rpi
crashes, do I want to car to pick up driving 100 km/h at power up even
before Linux is up again?) And if we agree it's a good idea: Should
raspberrypi_pwm_apply return 0 if setting the duty cycle succeeded and
only setting the default didn't?
Other than that the patch looks fine.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] vme: make remove callback return void
The driver core ignores the return value of struct bus_type::remove()
because there is only little that can be done. To simplify the quest to
make this function return void, let struct vme_driver::remove return void,
too. There is only a single vme driver and it already returns 0
unconditionally in .remove().
Also fix the bus remove function to always return 0.
Signed-off-by: Uwe Kleine-König
---
drivers/staging/vme/devices/vme_user.c | 4 +---
drivers/vme/vme.c | 4 ++--
include/linux/vme.h| 2 +-
3 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/vme/devices/vme_user.c
b/drivers/staging/vme/devices/vme_user.c
index fd0ea4dbcb91..35d7260e2271 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -689,7 +689,7 @@ static int vme_user_probe(struct vme_dev *vdev)
return err;
}
-static int vme_user_remove(struct vme_dev *dev)
+static void vme_user_remove(struct vme_dev *dev)
{
int i;
@@ -717,8 +717,6 @@ static int vme_user_remove(struct vme_dev *dev)
/* Unregister the major and minor device numbers */
unregister_chrdev_region(MKDEV(VME_MAJOR, 0), VME_DEVS);
-
- return 0;
}
static struct vme_driver vme_user_driver = {
diff --git a/drivers/vme/vme.c b/drivers/vme/vme.c
index 54d7963c1078..1b15afea28ee 100644
--- a/drivers/vme/vme.c
+++ b/drivers/vme/vme.c
@@ -1997,9 +1997,9 @@ static int vme_bus_remove(struct device *dev)
driver = dev->platform_data;
if (driver->remove)
- return driver->remove(vdev);
+ driver->remove(vdev);
- return -ENODEV;
+ return 0;
}
struct bus_type vme_bus_type = {
diff --git a/include/linux/vme.h b/include/linux/vme.h
index 7e82bf500f01..b204a9b4be1b 100644
--- a/include/linux/vme.h
+++ b/include/linux/vme.h
@@ -122,7 +122,7 @@ struct vme_driver {
const char *name;
int (*match)(struct vme_dev *);
int (*probe)(struct vme_dev *);
- int (*remove)(struct vme_dev *);
+ void (*remove)(struct vme_dev *);
struct device_driver driver;
struct list_head devices;
};
--
2.29.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 11/11] pwm: Add Raspberry Pi Firmware based PWM bus
}
> +
> + /*
> + * This sets the default duty cycle after resetting the board, we
> + * updated it every time to mimic Raspberry Pi's downstream's driver
> + * behaviour.
> + */
> + ret = raspberrypi_pwm_set_property(rpipwm->firmware,
> RPI_PWM_DEF_DUTY_REG,
> +duty_cycle);
> + if (ret) {
> + dev_err(chip->dev, "Failed to set default duty cycle: %d\n",
> ret);
> + return ret;
> + }
> +
> +rpipwm->duty_cycle = duty_cycle;
Please use tabs for indention. (The general hint is to use checkpatch
which (I hope) tells you about problems like this.)
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops raspberrypi_pwm_ops = {
> + .get_state = raspberrypi_pwm_get_state,
> + .apply = raspberrypi_pwm_apply,
> + .owner = THIS_MODULE,
> +};
> +
> +static int raspberrypi_pwm_probe(struct platform_device *pdev)
> +{
> + struct device_node *firmware_node;
> + struct device *dev = >dev;
> + struct rpi_firmware *firmware;
> + struct raspberrypi_pwm *rpipwm;
> + int ret;
> +
> + firmware_node = of_get_parent(dev->of_node);
> + if (!firmware_node) {
> + dev_err(dev, "Missing firmware node\n");
> + return -ENOENT;
> + }
> +
> + firmware = devm_rpi_firmware_get(>dev, firmware_node);
> + of_node_put(firmware_node);
> + if (!firmware)
> + return -EPROBE_DEFER;
Please use dev_err_probe to benefit from recording an error message in
this case.
> + rpipwm = devm_kzalloc(>dev, sizeof(*rpipwm), GFP_KERNEL);
> + if (!rpipwm)
> + return -ENOMEM;
> +
> + rpipwm->firmware = firmware;
> + rpipwm->chip.dev = dev;
> + rpipwm->chip.ops = _pwm_ops;
> + rpipwm->chip.base = -1;
> + rpipwm->chip.npwm = RASPBERRYPI_FIRMWARE_PWM_NUM;
> +
> + platform_set_drvdata(pdev, rpipwm);
> +
> + ret = raspberrypi_pwm_get_property(rpipwm->firmware,
> RPI_PWM_CUR_DUTY_REG,
> +>duty_cycle);
> + if (ret) {
> + dev_err(dev, "Failed to get duty cycle: %d\n", ret);
Please use %pe for the error codes (directly or still better by using
dev_err_probe here, too).
> + return ret;
> + }
> +
> + return pwmchip_add(>chip);
> +}
> [...]
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: greybus: Add TODO item about modernizing the pwm code
drivers/staging/greybus/pwm.c uses the old style PWM callbacks, new drivers should stick to the atomic API instead. Signed-off-by: Uwe Kleine-König --- On 12/8/20 10:39 AM, Johan Hovold wrote: > No sign off? > > Please also add a staging prefix since this part of greybus still lives > there. That after all these years I still fail occasionally to add a sign-off ... /me shakes his head about himself. Anyhow, here comes a v2, also with the requested prefix. drivers/staging/greybus/TODO | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/staging/greybus/TODO b/drivers/staging/greybus/TODO index 31f1f2cb401c..6461e0132fe3 100644 --- a/drivers/staging/greybus/TODO +++ b/drivers/staging/greybus/TODO @@ -1,3 +1,5 @@ * Convert all uses of the old GPIO API from to the GPIO descriptor API in and look up GPIO lines from device tree or ACPI. +* Make pwm.c use the struct pwm_ops::apply instead of ::config, ::set_polarity, + ::enable and ::disable. -- 2.29.2 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] greybus: Add TODO item about modernizing the pwm code
drivers/staging/greybus/pwm.c uses the old style PWM callbacks, new drivers should stick to the atomic API instead. --- drivers/staging/greybus/TODO | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/staging/greybus/TODO b/drivers/staging/greybus/TODO index 31f1f2cb401c..6461e0132fe3 100644 --- a/drivers/staging/greybus/TODO +++ b/drivers/staging/greybus/TODO @@ -1,3 +1,5 @@ * Convert all uses of the old GPIO API from to the GPIO descriptor API in and look up GPIO lines from device tree or ACPI. +* Make pwm.c use the struct pwm_ops::apply instead of ::config, ::set_polarity, + ::enable and ::disable. -- 2.29.2 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] mtd: nand: Rename nand.h into rawnand.h
Hello Boris, you could easily split this patch per architecture/subsystem if you in a first patch move the content of nand.h to rawnand.h and make nand.h just #include rawnand.h. Then you can switch one user at a time and when all are converted to use rawnand.h you can drop the #include. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ | ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] gpu: ipu-v3: Move i.MX IPUv3 core driver out of staging
On Thu, Feb 27, 2014 at 12:41:36PM -0800, Greg Kroah-Hartman wrote: On Tue, Feb 25, 2014 at 01:09:57PM +0100, Philipp Zabel wrote: Am Dienstag, den 25.02.2014, 12:43 +0100 schrieb Philipp Zabel: The i.MX Image Processing Unit (IPU) contains a number of image processing blocks that sit right in the middle between DRM and V4L2. Some of the modules, such as Display Controller, Processor, and Interface (DC, DP, DI) or CMOS Sensor Interface (CSI) and their FIFOs could be assigned to either framework, but others, such as the dma controller (IDMAC) and image converter (IC) can be used by both. The IPUv3 core driver provides an internal API to access the modules, to be used by both DRM and V4L2 IPUv3 drivers. [...] This one is missing: diff --git a/drivers/staging/imx-drm/imx-hdmi.c b/drivers/staging/imx-drm/imx-hdmi.c index cb4eb76..9aeb863 100644 --- a/drivers/staging/imx-drm/imx-hdmi.c +++ b/drivers/staging/imx-drm/imx-hdmi.c @@ -28,7 +28,8 @@ #include drm/drm_edid.h #include drm/drm_encoder_slave.h -#include ipu-v3/imx-ipu-v3.h +#include video/imx-ipu-v3.h + What do you mean? I can't apply this patch? I understand it as: Please squash this hunk changing the #include into patch 3. But note that I don't know anything about the stuff Philipp is working on. Thanks Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ | ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
