linux-fbdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Mark Rutland <mark.rutland@arm.com>
To: Daniel Jeong <gshark.jeong@gmail.com>
Cc: Jingoo Han <jg1.han@samsung.com>, Bryan Wu <cooloney@gmail.com>,
	Lee Jones <lee.jones@linaro.org>,
	Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	"grant.likely@linaro.org" <grant.likely@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	Daniel Jeong <daniel.jeong@ti.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-fbdev@vger.kernel.org" <linux-fbdev@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>
Subject: Re: [RFC v3 2/2] backlight: device tree: add new tps611xx backlight driver
Date: Wed, 25 Jun 2014 09:32:59 +0000	[thread overview]
Message-ID: <20140625093259.GC29789@leverpostej> (raw)
In-Reply-To: <1403594330-15387-3-git-send-email-gshark.jeong@gmail.com>

Hello,

Please fix the subject: this is a _binding_, not a driver.

On Tue, Jun 24, 2014 at 08:18:50AM +0100, Daniel Jeong wrote:
> This commit is about tps611xx device tree documentation.
> 
> Signed-off-by: Daniel Jeong <gshark.jeong@gmail.com>
> ---
>  .../video/backlight/tps611xx-backlight.txt         |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/backlight/tps611xx-backlight.txt
> 
> diff --git a/Documentation/devicetree/bindings/video/backlight/tps611xx-backlight.txt b/Documentation/devicetree/bindings/video/backlight/tps611xx-backlight.txt
> new file mode 100644
> index 0000000..01f110d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/backlight/tps611xx-backlight.txt
> @@ -0,0 +1,16 @@
> +TPS611xx family of backlight driver based on EasyScale.
> +
> +It supports tps61158, tps61161, tps61163 and tps61165.

What supports these? The driver?

The binding should describe the _hardware_, not the driver.

> +Required properties:
> +- compatible: "ti,tps61158_bl", "ti,tps61161_bl", "ti,tps61163_bl", "ti,tps61165_bl"

Use '-' rather than '_' in compatible strings and properties.

Is there any reason for the "bl" suffix? Does that actually form part of
the name of the unit, or is that just to point out it's a backlight? If
it's the latter, drop it.

Are these all very similar? Can I use a particular string as a fallback?

To allow for future expansion, the addition of notes, and to make this
easier to read, I would recommend formatting this something like:

- compatible: should contain at least one of:
  * "ti,tps61158"
  * "ti,tps61161"
  * "ti,tps61163"
  * "ti,tps61165"

> +- rfa_en: enable request for acknowledge. ( 0 : disable , 1 : enable )

Use empty properties for describing booleans.

That said, why does this need to be in the DT? When would I want this
and when wouldn't I? Why can't the driver choose?

> +- en_gpio_num: gpio number for en pin.

Use the GPIO bindings. The numbering Linux uses internally is completely
arbitrary, so this is completely broken.

Are there other pins we might need to describe? Regulators? Clocks?

Mark.

      reply	other threads:[~2014-06-25  9:32 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-06-24  7:18 [RFC v3 0/2] backlight: add new tps611xx backlight driver Daniel Jeong
2014-06-24  7:18 ` [RFC v3 1/2] " Daniel Jeong
2014-06-24 11:17   ` Jingoo Han
     [not found] ` <1403594330-15387-1-git-send-email-gshark.jeong-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2014-06-24  7:18   ` [RFC v3 2/2] backlight: device tree: " Daniel Jeong
2014-06-25  9:32     ` Mark Rutland [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=20140625093259.GC29789@leverpostej \
    --to=mark.rutland@arm.com \
    --cc=cooloney@gmail.com \
    --cc=daniel.jeong@ti.com \
    --cc=devicetree@vger.kernel.org \
    --cc=grant.likely@linaro.org \
    --cc=gshark.jeong@gmail.com \
    --cc=jg1.han@samsung.com \
    --cc=lee.jones@linaro.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-fbdev@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=plagnioj@jcrosoft.com \
    --cc=rdunlap@infradead.org \
    --cc=robh+dt@kernel.org \
    --cc=tomi.valkeinen@ti.com \
    /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).