On Thu, Oct 22, 2020 at 9:05 PM Nicolas Saenz Julienne
[off-list ref] 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.

 drivers/pwm/pwm-raspberrypi.c | 221 ++++++++++++++++++++++++++++++++++

Name is completely confusing.
Please, make it unique enough to understand that this is exactly the
device it serves for.

For example, pwm-rpi-poe is better.

...

+ *  - Only normal polarity

Can't it be emulated? Isn't it 100% - duty cycle % ?

+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>

...

+       ret = rpi_firmware_property(firmware, RPI_FIRMWARE_SET_POE_HAT_VAL,
+                                   &msg, sizeof(msg));
+       if (ret)
+               return ret;

+       else if (msg.ret)

Redundant 'else'

+               return -EIO;

...

+       ret = rpi_firmware_property(firmware, RPI_FIRMWARE_GET_POE_HAT_VAL,
+                                   &msg, sizeof(msg));
+       if (ret)
+               return ret;

+       else if (msg.ret)

Ditto.

+               return -EIO;

...

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

Looks like a hack.

...

+       ret = pwmchip_remove(&rpipwm->chip);
+       if (!ret)
+               rpi_firmware_put(rpipwm->firmware);
+
+       return ret;

Can't you use the usual pattern?

  ret = ...
  if (ret)
    return ret;
  ...
  return 0;

-- 
With Best Regards,
Andy Shevchenko