Date: Wed, 2 Dec 2009 21:31:31 +0100
From: Antonio Ospite <ospite@studenti.unina.it>
To: Liam Girdwood <lrg@slimlogic.co.uk>
Cc: Richard Purdie <rpurdie@rpsys.net>,
       Mark Brown <broonie@opensource.wolfsonmicro.com>,
       Daniel Ribeiro <drwyrm@gmail.com>, linux-kernel@vger.kernel.org,
       linux-arm-kernel@lists.infradead.org, openezx-devel@lists.openezx.org
Subject: Re: [PATCH] leds: Add LED class driver for regulator driven LEDs.
Message-Id: <20091202213131.1393a956.ospite@studenti.unina.it>
In-Reply-To: <1259778235.3588.141.camel@odin>
References: <1259775625-25973-1-git-send-email-ospite@studenti.unina.it>
	<1259778235.3588.141.camel@odin>
Mime-Version: 1.0
Content-Type: multipart/signed; protocol="application/pgp-signature";
 micalg="PGP-SHA1";
 boundary="Signature=_Wed__2_Dec_2009_21_31_31_+0100_Z/3DhWz1W8X_RjeO"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3370
Lines: 127

--Signature=_Wed__2_Dec_2009_21_31_31_+0100_Z/3DhWz1W8X_RjeO
Content-Type: text/plain; charset=US-ASCII
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, 02 Dec 2009 18:23:55 +0000
Liam Girdwood <lrg@slimlogic.co.uk> wrote:

> On Wed, 2009-12-02 at 18:40 +0100, Antonio Ospite wrote:
> > This driver provides an interface for controlling LEDs (or vibrators)
> > connected to PMICs for which there is a regulator framework driver.
> >=20
> > This driver can be used, for instance, to control vibrator on all Motor=
ola EZX
> > phones using the pcap-regulator driver services.
> >=20
> > Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
>=20
> Some very minor points below.
>=20

Thanks for reviewing the code.

[...]
> > +
> > +static void led_work(struct work_struct *work)
> > +{
> > +	struct regulator_led *led;
> > +	int voltage;
> > +	int ret;
> > +
> > +	led =3D container_of(work, struct regulator_led, work);
> > +
> > +	mutex_lock(&led->mutex);
> > +
> > +	if (led->value =3D=3D 0) {
>=20
> LED_OFF instead of 0 here ?
>

Ok.

> > +		regulator_led_disable(led);
> > +		goto out;
> > +	}
> > +
> > +	voltage =3D led_regulator_get_vdd(led->vcc, led->value);
> > +	dev_dbg(led->cdev.dev, "brightness: %d voltage: %d",
> > +			led->value, voltage);
> > +
> > +	ret =3D regulator_set_voltage(led->vcc, voltage, voltage);
> > +	if (ret !=3D 0)
> > +		dev_err(led->cdev.dev, "Failed to set voltage %d: %d\n",
> > +			voltage, ret);
>=20
> Some regulators may not support voltage change (via hw design or
> constraints) so set_voltage() will fail. We probably want to handle this
> regulator type so we don't call set_voltage().
>

Ok, I'll read more about the regulator framework to find out how to
check regulator capabilities.

> > +
> > +	regulator_led_enable(led);
> > +
> > +out:
> > +	mutex_unlock(&led->mutex);
> > +}
> > +
[...]
> > +static int regulator_led_probe(struct platform_device *pdev)
> > +{
> > +	struct led_regulator_platform_data *pdata =3D pdev->dev.platform_data;
> > +	struct regulator_led *led;
> > +	struct regulator *vcc;
> > +	int ret;
> > +
> > +	if (pdata =3D=3D NULL) {
> > +		dev_err(&pdev->dev, "no platform data\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	vcc =3D regulator_get(&pdev->dev, pdata->supply);
> > +	if (IS_ERR(vcc)) {
> > +		dev_err(&pdev->dev, "Cannot get vcc for %s\n", pdata->name);
> > +		return PTR_ERR(vcc);;
>=20
> double ;; here
>

Thanks.

[...]

Regards,
   Antonio=20

--=20
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

--Signature=_Wed__2_Dec_2009_21_31_31_+0100_Z/3DhWz1W8X_RjeO
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iEYEARECAAYFAksWzqMACgkQ5xr2akVTsAFkygCcCNXdtanf9dkZCIIJQ/P5RTr0
2/QAoJ1Yfca7WtSBPde3Z9Ea49Had5my
=WT27
-----END PGP SIGNATURE-----

--Signature=_Wed__2_Dec_2009_21_31_31_+0100_Z/3DhWz1W8X_RjeO--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/