Re: [PATCH] ASoC: ssm2305: Add amplifier driver

[Marco Felsch <m.felsch@pengutronix.de>]

On 18-05-17 12:52, Lars-Peter Clausen wrote:
> 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@pengutronix.de>
> 
> 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.
> 

Okay I will fix it in v2.

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

My opinion was that the pin can be hardwired by the application but
you are absolutely right, that makes the driver needless. I will return
an error and mark the gpio as required.

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

Okay, I will remove it.

> > +
> > +	return devm_snd_soc_register_component(dev, &ssm2305_component_driver,
> > +					       NULL, 0);
> > +}
> 


Regards,
Marco