Re: [PATCH 3/3] leds: add sgm3140 driver

From: Luca Weiss <luca@z3ntu.xyz>
To: linux-leds@vger.kernel.org, Dan Murphy <dmurphy@ti.com>
Cc: Heiko Stuebner <heiko@sntech.de>, Icenowy Zheng <icenowy@aosc.io>,
	Jacek Anaszewski <jacek.anaszewski@gmail.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Maxime Ripard <mripard@kernel.org>, Pavel Machek <pavel@ucw.cz>,
	Rob Herring <robh+dt@kernel.org>, Shawn Guo <shawnguo@kernel.org>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	~postmarketos/upstreaming@lists.sr.ht
Subject: Re: [PATCH 3/3] leds: add sgm3140 driver
Date: Sun, 15 Mar 2020 11:42:57 +0100	[thread overview]
Message-ID: <2023791.irdbgypaU6@g550jk> (raw)
In-Reply-To: <22341236-8298-dc97-217b-46071a362207@ti.com>

Hi Dan,

On Mittwoch, 11. März 2020 14:02:44 CET Dan Murphy wrote:
> Luca
> 
> On 3/9/20 3:35 PM, Luca Weiss wrote:
> > Add a driver for the SGMICRO SGM3140 Buck/Boost Charge Pump LED driver.
> > 
> > This device is controlled by two GPIO pins, one for enabling and the
> > second one for switching between torch and flash mode.
> 
> How does one enable torch and one enable flash?
> 
> Is the flash-gpio control this or does the enable-gpio enable the flash?

Enabling torch mode means making EN pin high and FLASH pin low.
Enabling flash mode means making EN pin high and FLASH pin high.

The users of this driver can just use standard v4l2 apis or sysfs so I 
wouldn't say this is relevant for the users.

> 
> The DT binding did not indicate what the GPIOs are really going to control.

I'm not sure if this is relevant in the dt bindings because how the device 
works is described in the datasheet and not really relevant for users of the 
binding? I also didn't necessarily want to copy-paste the datasheet into the 
dt bindings because of copyright.

> 
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > ---
> > Changes since RFC:
> > 
> > - address review comments from Jacek Anaszewski:
> >    - implement strobe_get op
> >    - implement timeout_set op
> >    - init v4l2_sd_cfg variable
> >    - remove init_data.devicename assignemnt
> >    - use devm_ version of led_classdev_flash_register_ext
> >    - release child_node in case of success
> >   
> >   drivers/leds/Kconfig        |   9 ++
> >   drivers/leds/Makefile       |   1 +
> >   drivers/leds/leds-sgm3140.c | 260 ++++++++++++++++++++++++++++++++++++
> >   3 files changed, 270 insertions(+)
> >   create mode 100644 drivers/leds/leds-sgm3140.c
> > 

> SNIP

> > +	if (!child_node) {
> > +		dev_err(&pdev->dev, "No DT child node found for 
connected LED.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +
> > +	ret = of_property_read_u32(child_node, "flash-max-timeout-us",
> > +				   &priv->max_timeout);
> 
> Please use the device_property api's to retrieve DT settings.  Then
> there is no need to release the "of" child node.
> 

I'm guessing you mean

device_property_read_u32(&pdev->dev, "flash-max-timeout-us", &priv-
>max_timeout);

?

I still need the child_node for "init_data.fwnode" and v4l2_flash_init so I 
still have to call of_node_put, right?

> > +	if (ret < 0) {
> 
> if (ret)

Ack

> SNIP

> > +	/* Create V4L2 Flash subdev */
> > +	priv->v4l2_flash = v4l2_flash_init(&pdev->dev,
> > of_fwnode_handle(child_node), +					
   fled_cdev, NULL,
> > +					   &v4l2_sd_cfg);
> > +	if (IS_ERR(priv->v4l2_flash)) {
> > +		ret = PTR_ERR(priv->v4l2_flash);
> > +		goto err;
> 
> Do you need to jump here?  This should just fall out and go through err
> anyway.

Should I just do

if (IS_ERR(priv->v4l2_flash))
	ret = PTR_ERR(priv->v4l2_flash);

?

I thought about removing the goto but I decided to keep it in case code is 
added below that statement so that the goto wouldn't be forgotten. But I can 
change it of course if wanted.

> 
> Dan

Regards
Luca