On Wed, Feb 24, 2016 at 5:16 PM, David Rivshin (Allworx) [off-list ref] wrote:

On Wed, 24 Feb 2016 13:45:14 -0600
Rob Herring [off-list ref] wrote:

quoted

On Wed, Feb 24, 2016 at 12:57 PM, David Rivshin (Allworx)
[off-list ref] wrote:

quoted

On Tue, 23 Feb 2016 19:15:08 -0600
Rob Herring [off-list ref] wrote:

quoted

On Tue, Feb 23, 2016 at 01:17:24PM -0500, David Rivshin (Allworx) wrote:

quoted

From: David Rivshin <redacted>

This adds a binding description for the is31fl3236/35/18/16 I2C LED
drivers.

Signed-off-by: David Rivshin <redacted>
---
 .../devicetree/bindings/leds/leds-is31fl32xx.txt   | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt

Acked-by: Rob Herring <robh@kernel.org>

Thanks, Rob. I just want to double check whether you noticed some
binding-related questions I had in the coverletter [1]. In hindsight
I should probably have included them in the patch comments as well so
they'd show up in patchwork. For convenience, I'll repeat them here:

I had not.

quoted

I choose to have 'reg' be 1-based in order to follow the hardware
documentation. It seems 0-based is the normal choice, although HW
docs also usually use the 0-based numbering. Perhaps being consistent
with other bindings is more important than being consistent with the
datasheet's numbering?

Usually reg is some type of address, so you should follow whatever is
more natural for accessing the h/w.

In this case I don't think there is a natural "address" in the HW.
There are a number of (non-contiguous) registers (or parts thereof)
which together control a "channel". They are not so much an "single-
LED controller" block which is replicated N times, but an monolithic
"N-channel LED controller".

That is also why originally I had used a 'chan' property instead. But,
I then saw that ePAPR required a 'reg' property to use the @n labeling,
and it seemed all bindings for similar devices used a 'reg' property.

quoted

Ideally, it is not just an index for s/w convenience.

Thanks, that is useful clarification.

quoted

quoted

I used the 'reg' property for the LED channel, as it seemed ePAPR
required that name. I also considered naming the property 'chan',
and not pretending that it represented a bus address at all (and
then removing the @n from the subnode names). That would solve the
'reg' question above as a side-effect, but would be inconstant with
other LED bindings (tca6507, pca963x, tlc59108, etc).

I would expect the reg value is either the register (base) address for
each channel or a bit offset in a register if they are all in the same
register. Or it could be how the pins are labeled on the package if
neither of those work.

I don't think that 'reg' being a register base address or bit offset
would work in this case, for the reasons mentioned above.
The only really obvious numbering is that the datasheets name them
OUT1 through OUTn, and I'd expect schematics to follow that naming.
So that seemed like the natural way to refer to them in the devicetree,
and let the driver compute the various register addresses.

Just for the sake of illustration here are some of the important
registers for these devices:
 3236:
   - 0x01-0x24: per channel PWM duty cycle registers (low-to-high)
   - 0x26-0x49: per channel enable bit and current divisor
 3235:
   - 0x01-0x20: per channel PWM duty cycle registers (low-to-high)
   - 0x2A-0x45: per channel enable bit and current divisor
 3218:
   - 0x01-0x12: per channel PWM duty cycle registers (low-to-high)
   - 0x13h: OUT1-6 packed enable bits, LSB to MSB
   - 0x14h: OUT7-12 packed enable bits, LSB to MSB
   - 0x15h: OUT13-18 packed enable bits, LSB to MSB
 3216:
   - 0x01: OUT9-16 packed enable bits, LSB to MSB
   - 0x02: OUT1-8 packed enable bits, LSB to MSB
   - 0x04: OUT9-16 GPIO (vs LED) bits
   - 0x10-0x1F: per channel PWM duty cycle registers (high-to-low)
   - 0x20-0xAF: 8 sets of enable and PWM registers for animation

quoted

quoted

Note that the recently-added (and only in for-next) SN3218 driver
uses a 0-based 'reg' property, and the SN3218 has the same HW doc
numbering as the IS31FL32xx family (indeed the IS31FL3218 appears
to be a rebranded SN3218). So perhaps that sets the precedent if
there was not one before?

If they are the same, then you should use the SN3218 binding. If it
has problems, then it is not too late to fix it.

Given the above, do you think that the SN3218 binding should be changed
so that 'reg' is 1-based instead of 0-based?

quoted

You may want to add
another compatible string if they are not truly the exact same die.

Are you implying that if the IS31FL3218 and SN3218 are the same die
(which they may well be), that it would *not* be appropriate to have
two "compatible" strings for them (in the same binding)?
I was assuming that having two compatible strings would be preferable
regardless, as they are sold under two different part numbers and
companies. The only reason I noticed they were the same was because I
was looking for an example of another device that numbered channels
starting with '1' and was surprised how similar the datasheet was.