linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Pavel Machek <pavel@ucw.cz>
To: Dan Murphy <dmurphy@ti.com>
Cc: robh+dt@kernel.org, jacek.anaszewski@gmail.com,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-leds@vger.kernel.org
Subject: Re: [PATCH v2 2/2] leds: lm3697: Introduce the lm3697 driver
Date: Wed, 8 Aug 2018 21:59:09 +0200	[thread overview]
Message-ID: <20180808195909.GC20912@amd> (raw)
In-Reply-To: <20180807160442.8937-2-dmurphy@ti.com>

[-- Attachment #1: Type: text/plain, Size: 3451 bytes --]

Hi!

> Introduce the lm3697 LED driver for
> backlighting and display.
> 
> Datasheet location:
> http://www.ti.com/lit/ds/symlink/lm3697.pdf
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>

> +
> +#define LM3697_HVLED1_2_3_A		0
> +#define LM3697_HVLED1_B_HVLED2_3_A	1
> +#define LM3697_HVLED2_B_HVLED1_3_A	2
> +#define LM3697_HVLED1_2_B_HVLED3_A	3
> +#define LM3697_HVLED3_B_HVLED1_2_A	4
> +#define LM3697_HVLED1_3_B_HVLED2_A	5
> +#define LM3697_HVLED1_A_HVLED2_3_B	6
> +#define LM3697_HVLED1_2_3_B		7

That's rather long and verbose way to describe a bitmap, right?

> +static const struct regmap_config lm3697_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +
> +	.max_register = LM3697_CTRL_ENABLE,
> +	.reg_defaults = lm3697_reg_defs,
> +	.num_reg_defaults = ARRAY_SIZE(lm3697_reg_defs),
> +	.cache_type = REGCACHE_RBTREE,
> +};

Is rbtree good idea? You don't have that many registers.

> +static int lm3697_init(struct lm3697 *priv)
> +{
> +	int ret;
> +
....
> +		regmap_write(priv->regmap, LM3697_RESET, LM3697_SW_RESET);

No error checking required here?

> +	if (priv->control_bank_config < LM3697_HVLED1_2_3_A ||
> +	    priv->control_bank_config > LM3697_HVLED1_2_3_B) {
> +		dev_err(&priv->client->dev, "Control bank configuration is out of range\n");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	device_for_each_child_node(priv->dev, child) {
> +		led = &priv->leds[i];
> +
> +		ret = fwnode_property_read_u32(child, "reg", &led->control_bank);
> +		if (ret) {
> +			dev_err(&priv->client->dev, "reg DT property missing\n");
> +			goto child_out;
> +		}
> +
> +		fwnode_property_read_string(child, "linux,default-trigger",
> +					    &led->led_dev.default_trigger);
> +
> +		ret = fwnode_property_read_string(child, "label", &name);
> +		if (ret)
> +			snprintf(led->label, sizeof(led->label),
> +				"%s::", priv->client->name);
> +		else
> +			snprintf(led->label, sizeof(led->label),
> +				 "%s:%s", priv->client->name, name);
> +
> +
> +		led->priv = priv;
> +		led->led_dev.name = led->label;
> +		led->led_dev.brightness_set_blocking = lm3697_brightness_set;
> +
> +		ret = devm_led_classdev_register(priv->dev, &led->led_dev);
> +		if (ret) {
> +			dev_err(&priv->client->dev, "led register err: %d\n", ret);
> +			goto child_out;
> +		}
> +
> +		if (priv->control_bank_config == LM3697_HVLED1_2_3_A ||
> +		    priv->control_bank_config == LM3697_HVLED1_2_3_B)
> +			goto child_out;

This checks if we have just one bank, I see it. Should it also check
the led actually uses the correct bank?

> +		i++;
> +		fwnode_handle_put(child);
> +	}
> +
> +child_out:
> +	fwnode_handle_put(child);

Is not the fwnode_handle_put() done twice for non-error case?

> +	ret = lm3697_init(led);
> +	if (ret)
> +		return ret;
> +
> +	return ret;
> +}

The if is not needed here.

> +static int lm3697_remove(struct i2c_client *client)
> +{
> +	struct lm3697 *led = i2c_get_clientdata(client);
> +	int ret;
> +
> +	ret = regmap_update_bits(led->regmap, LM3697_CTRL_ENABLE,
> +				 LM3697_CTRL_A_B_EN, 0);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Failed to disable regulator\n");
> +		return ret;

Misleading, this does nothing with regulators.

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

  reply	other threads:[~2018-08-08 19:59 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-07 16:04 [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver Dan Murphy
2018-08-07 16:04 ` [PATCH v2 2/2] leds: lm3697: Introduce the " Dan Murphy
2018-08-08 19:59   ` Pavel Machek [this message]
2018-08-14 13:54     ` Dan Murphy
2018-08-08  7:56 ` [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for " Michal Vokáč
2018-08-08  9:52   ` Jacek Anaszewski
2018-08-08 19:59 ` Pavel Machek
2018-08-08 20:42   ` Dan Murphy
2018-08-08 21:02     ` Pavel Machek
2018-08-08 21:04       ` Dan Murphy
2018-08-08 21:09         ` Pavel Machek
2018-08-08 21:41           ` Dan Murphy
2018-08-08 21:45             ` Pavel Machek
2018-08-08 21:50               ` Dan Murphy
2018-08-08 21:58                 ` Pavel Machek
2018-08-08 21:09         ` Jacek Anaszewski
2018-08-08 21:45           ` Dan Murphy
2018-08-09 12:09             ` Jacek Anaszewski
2018-08-09 13:24               ` Pavel Machek
2018-08-09 13:30               ` Dan Murphy
2018-08-09 14:48                 ` Jacek Anaszewski
2018-08-09 15:01                   ` Dan Murphy
2018-08-09 21:59                 ` Pavel Machek
2018-08-08 22:00 ` Pavel Machek

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=20180808195909.GC20912@amd \
    --to=pavel@ucw.cz \
    --cc=devicetree@vger.kernel.org \
    --cc=dmurphy@ti.com \
    --cc=jacek.anaszewski@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).