On 05/17/2018 11:22 AM, Marco Felsch wrote:
> The ssm2305 is a simple Class-D audio amplifier. A application can
> turn on/off the device by a gpio. It's also possible to hardwire the
> shutdown pin.
>
> Tested on a i.MX6 based custom board.
>
> Signed-off-by: Marco Felsch <m.felsch@xxxxxxxxxxxxxx>
Hi,
Thanks for the patch. Looks mostly good, some comments inline.
> ---
> .../devicetree/bindings/sound/adi,ssm2305.txt | 15 +++
> sound/soc/codecs/Kconfig | 7 ++
> sound/soc/codecs/Makefile | 2 +
> sound/soc/codecs/ssm2305.c | 105 ++++++++++++++++++
> 4 files changed, 129 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/sound/adi,ssm2305.txt
> create mode 100644 sound/soc/codecs/ssm2305.c
>
> diff --git a/Documentation/devicetree/bindings/sound/adi,ssm2305.txt b/Documentation/devicetree/bindings/sound/adi,ssm2305.txt
> new file mode 100644
> index 000000000000..fc4c99225538
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/adi,ssm2305.txt
> @@ -0,0 +1,15 @@
> +Analog Devices SSM2305 Speaker Amplifier
> +========================================
> +
> +Required properties:
> + - compatible : "adi,ssm2305"
> +
> +Optional properties:
> + - ssm2305,shutdown-gpio : the gpio connected to the shutdown pin
I think policy is to use only the -gpios suffix for new bindings.
> +
> +Example:
> +
> +ssm2305: analog-amplifier {
> + compatible = "adi,ssm2305";
> + ssm2305,shutdown-gpio = <&gpio3 20 GPIO_ACTIVE_LOW>;
> +};
[...]
> +
> +static int ssm2305_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct ssm2305 *priv;
> + int err;
> +
> + /* Allocate the private data */
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, priv);
> +
> + /* Shutdown gpio is optional */
If it is really optional you should use gpiod_get_optional. But I wonder
what is the point of the driver if the GPIO is not present?
> + priv->gpiod_shutdown = devm_gpiod_get(dev, "ssm2305,shutdown",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(priv->gpiod_shutdown)) {
> + err = PTR_ERR(priv->gpiod_shutdown);
> + if (err != -EPROBE_DEFER)
> + dev_warn(dev, "Failed to get 'shutdown' gpio: %d\n",
> + err);
Should err be returned here?
> + }
> +
> + dev_info(dev, "probed\n");
That's a bit too much noise, imagine every driver did this.
> +
> + return devm_snd_soc_register_component(dev, &ssm2305_component_driver,
> + NULL, 0);
> +}
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
