Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
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
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



      parent reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-09 20:35 [PATCH 0/3] Add sgm3140 flash led driver Luca Weiss
2020-03-09 20:35 ` [PATCH 1/3] dt-bindings: Add vendor prefix for SG Micro Corp Luca Weiss
2020-03-23 20:54   ` Rob Herring
2020-03-09 20:35 ` [PATCH 2/3] dt-bindings: leds: Add binding for sgm3140 Luca Weiss
2020-03-09 22:22   ` Sakari Ailus
2020-03-11 12:49   ` Dan Murphy
2020-03-15 10:47     ` Luca Weiss
2020-03-15 10:53       ` Laurent Pinchart
2020-03-23 21:30         ` Dan Murphy
2020-03-23 20:57   ` Rob Herring
2020-03-24 20:02     ` Luca Weiss
2020-03-09 20:35 ` [PATCH 3/3] leds: add sgm3140 driver Luca Weiss
2020-03-09 22:18   ` Sakari Ailus
2020-03-09 22:49     ` Pavel Machek
2020-03-09 22:52       ` Luca Weiss
2020-03-11 13:02   ` Dan Murphy
2020-03-11 21:09     ` Jacek Anaszewski
2020-03-15 10:42     ` Luca Weiss [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2023791.irdbgypaU6@g550jk \
    --to=luca@z3ntu.xyz \
    --cc=devicetree@vger.kernel.org \
    --cc=dmurphy@ti.com \
    --cc=heiko@sntech.de \
    --cc=icenowy@aosc.io \
    --cc=jacek.anaszewski@gmail.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mripard@kernel.org \
    --cc=pavel@ucw.cz \
    --cc=robh+dt@kernel.org \
    --cc=shawnguo@kernel.org \
    --cc=~postmarketos/upstreaming@lists.sr.ht \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-LEDs Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-leds/0 linux-leds/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-leds linux-leds/ https://lore.kernel.org/linux-leds \
		linux-leds@vger.kernel.org
	public-inbox-index linux-leds

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-leds


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git