[v1,1/2] dt-bindings: leds: add binding for aw200xx

  Add YAML devicetree binding for AWINIC AW20036/AW20052/AW20074
led driver.

Signed-off-by: Martin Kurbanov <mmkurbanov@sberdevices.ru>
---
 .../bindings/leds/leds-aw200xx.yaml           | 110 ++++++++++++++++++
 include/dt-bindings/leds/leds-aw200xx.h       |  48 ++++++++
 2 files changed, 158 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-aw200xx.yaml
 create mode 100644 include/dt-bindings/leds/leds-aw200xx.h
  

On 24/11/2022 21:48, Martin Kurbanov wrote:
> Add YAML devicetree binding for AWINIC AW20036/AW20052/AW20074
> led driver.
> 
> Signed-off-by: Martin Kurbanov <mmkurbanov@sberdevices.ru>
> ---
>  .../bindings/leds/leds-aw200xx.yaml           | 110 ++++++++++++++++++
>  include/dt-bindings/leds/leds-aw200xx.h       |  48 ++++++++
>  2 files changed, 158 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-aw200xx.yaml
>  create mode 100644 include/dt-bindings/leds/leds-aw200xx.h
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-aw200xx.yaml b/Documentation/devicetree/bindings/leds/leds-aw200xx.yaml
> new file mode 100644
> index 000000000000..3bdadcbc2ee2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-aw200xx.yaml

Filename based on compatibles, so "awinic,aw200xx.yaml"

> @@ -0,0 +1,110 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-aw200xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AWINIC AW200XX LED Driver

What does the "Driver" mean? Linux Driver? If yes, then drop it. Same in
other places.

> +
> +maintainers:
> +  - Martin Kurbanov <mmkurbanov@sberdevices.ru>
> +
> +description: |
> +  This controller is present on AW20036/AW20054/AW20072.
> +  It is a 3x12/6x9/6x12 matrix LED driver programmed via
> +  an I2C interface, up to 36/54/72 LEDs or 12/18/24 RGBs,
> +  3 pattern controllers for auto breathing or group dimming control.
> +
> +  For more product information please see the link below:
> +  aw20036 - https://www.awinic.com/Public/Uploads/uploadfile/files/20200509/20200509151532_5eb65894d205a.pdf
> +  aw20054 - https://www.awinic.com/Public/Uploads/uploadfile/files/20200509/20200509151602_5eb658b2b77cb.pdf
> +  aw20072 - https://www.awinic.com/Public/Uploads/uploadfile/files/20200509/20200509151754_5eb659227a145.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - awinic,aw20036
> +      - awinic,aw20054
> +      - awinic,aw20072
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  display-size:

Is it a standard property? Does not look like. Non-standard properties
need vendor prefix and type ($ref).

> +    maxItems: 1
> +    description:
> +      Leds matrix size, see dt-bindings/leds/leds-aw200xx.h

But judging by your constants, you have the same number of columns, just
rows differ, so probably you want to describe here number of rows.

> +
> +  imax:
> +    maxItems: 1
> +    description:
> +      Maximum supply current, see dt-bindings/leds/leds-aw200xx.h

No. Use existing properties from common.yaml. This looks like
led-max-microamp and it is per LED, not per entire device.

> +
> +patternProperties:
> +  "^led@[0-9a-f]$":
> +    type: object
> +    $ref: common.yaml#

unevaluatedProperties: false

> +
> +    properties:
> +      reg:
> +        description:
> +          LED number
> +        maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +  - display-size
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/leds/common.h>
> +    #include <dt-bindings/leds/leds-aw200xx.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        led-controller@3a {
> +            compatible = "awinic,aw20036";
> +            reg = <0x3a>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            interrupt-parent = <&gpio_intc>;
> +            interrupts = <13 IRQ_TYPE_LEVEL_LOW>;
> +
> +            display-size = <AW20036_DSIZE_3X12>;
> +            imax = <AW200XX_IMAX_60MA>;
> +
> +            led@0 {
> +                reg = <0x0>;
> +                color = <LED_COLOR_ID_RED>;
> +            };
> +
> +            led@1 {
> +                reg = <0x1>;
> +                color = <LED_COLOR_ID_GREEN>;
> +            };
> +
> +            led@2 {
> +                reg = <0x2>;
> +                color = <LED_COLOR_ID_BLUE>;
> +            };
> +        };
> +    };
> +
> +...
> diff --git a/include/dt-bindings/leds/leds-aw200xx.h b/include/dt-bindings/leds/leds-aw200xx.h
> new file mode 100644
> index 000000000000..6b2ba4c3c6b1
> --- /dev/null
> +++ b/include/dt-bindings/leds/leds-aw200xx.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */

Dual license, like bindings.

> +/**
> + * This header provides constants for aw200xx LED bindings.
> + *
> + * Copyright (c) 2022, SberDevices. All Rights Reserved.
> + *
> + * Author: Martin Kurbanov <mmkurbanov@sberdevices.ru>
> + */
> +#ifndef _DT_BINDINGS_LEDS_AW200XX_H
> +#define _DT_BINDINGS_LEDS_AW200XX_H
> +
> +/* Global max current (IMAX) */
> +#define AW200XX_IMAX_3_3MA  8
> +#define AW200XX_IMAX_6_7MA  9

No. Bindings are not for storing register constants. Feel free to store
here IDs (ID start from 0 or 1 and is incremented by 1)... but how the
IMAX even matches any need for "ID"?

> +#define AW200XX_IMAX_10MA   0
> +#define AW200XX_IMAX_13_3MA 11
> +#define AW200XX_IMAX_20MA   1
> +#define AW200XX_IMAX_26_7MA 13
> +#define AW200XX_IMAX_30MA   2
> +#define AW200XX_IMAX_40MA   3
> +#define AW200XX_IMAX_53_3MA 15
> +#define AW200XX_IMAX_60MA   4
> +#define AW200XX_IMAX_80MA   5
> +#define AW200XX_IMAX_120MA  6
> +#define AW200XX_IMAX_160MA  7
> +
> +/* Display size for aw20036 */
> +#define AW20036_DSIZE_1X12 0
> +#define AW20036_DSIZE_2X12 1
> +#define AW20036_DSIZE_3X12 2
> +
> +/* Display size for aw20054 */
> +#define AW20054_DSIZE_1X9 0
> +#define AW20054_DSIZE_2X9 1
> +#define AW20054_DSIZE_3X9 2
> +#define AW20054_DSIZE_4X9 3
> +#define AW20054_DSIZE_5X9 4
> +#define AW20054_DSIZE_6X9 5
> +
> +/* Display size for aw20072 */
> +#define AW20072_DSIZE_1X12 0
> +#define AW20072_DSIZE_2X12 1
> +#define AW20072_DSIZE_3X12 2
> +#define AW20072_DSIZE_4X12 3
> +#define AW20072_DSIZE_5X12 4
> +#define AW20072_DSIZE_6X12 5

Drop all constants and instead use number of rows without specifying it
in binding.

So in total entire file can be dropped.

> +
> +#endif /* !_DT_BINDINGS_LEDS_AW200XX_H */

Best regards,
Krzysztof
  

Hi. Thank you for quick reply. 

On 25.11.2022 11:29, Krzysztof Kozlowski wrote:
>> +
>> +  imax:
>> +    maxItems: 1
>> +    description:
>> +      Maximum supply current, see dt-bindings/leds/leds-aw200xx.h
> 
> No. Use existing properties from common.yaml. This looks like
> led-max-microamp and it is per LED, not per entire device.

The AW200XX LED chip does not support imax setup per led.
Imax is the global parameter over the all leds. I suppose, it's better
to add vendor prefix or take minimum from all subnodes?
How do you think?


>> +/* Global max current (IMAX) */
>> +#define AW200XX_IMAX_3_3MA  8
>> +#define AW200XX_IMAX_6_7MA  9
> 
> No. Bindings are not for storing register constants. Feel free to store
> here IDs (ID start from 0 or 1 and is incremented by 1)... but how the
> IMAX even matches any need for "ID"?

IMAX can be chosen from the predefined values in the
datasheet (10mA, 20mA, etc). Do you mean the IMAX should be round down
to nearest supported value in the driver?

-- 
Best Regards,
Kurbanov Martin
  

On 28/11/2022 18:43, Martin Kurbanov wrote:
> Hi. Thank you for quick reply. 
> 
> On 25.11.2022 11:29, Krzysztof Kozlowski wrote:
>>> +
>>> +  imax:
>>> +    maxItems: 1
>>> +    description:
>>> +      Maximum supply current, see dt-bindings/leds/leds-aw200xx.h
>>
>> No. Use existing properties from common.yaml. This looks like
>> led-max-microamp and it is per LED, not per entire device.
> 
> The AW200XX LED chip does not support imax setup per led.
> Imax is the global parameter over the all leds. I suppose, it's better
> to add vendor prefix or take minimum from all subnodes?
> How do you think?

Have in mind that led-max-microamp is a required property in some cases,
so skipping it and using per-device properties does not solve the
problem of adjusting proper currents. What if each LED you set for
something which in total gives more than your imax?

> 
> 
>>> +/* Global max current (IMAX) */
>>> +#define AW200XX_IMAX_3_3MA  8
>>> +#define AW200XX_IMAX_6_7MA  9
>>
>> No. Bindings are not for storing register constants. Feel free to store
>> here IDs (ID start from 0 or 1 and is incremented by 1)... but how the
>> IMAX even matches any need for "ID"?
> 
> IMAX can be chosen from the predefined values in the
> datasheet (10mA, 20mA, etc). Do you mean the IMAX should be round down
> to nearest supported value in the driver?

What Linux driver support does not matter here. Bindings should reflect
hardware and the same time not store register constants but logical
values (for current this is in uA).

Best regards,
Krzysztof
  

Hello Krzysztof,

On Fri, Dec 02, 2022 at 05:41:37PM +0100, Krzysztof Kozlowski wrote:
> On 28/11/2022 18:43, Martin Kurbanov wrote:
> > Hi. Thank you for quick reply. 
> > 
> > On 25.11.2022 11:29, Krzysztof Kozlowski wrote:
> >>> +
> >>> +  imax:
> >>> +    maxItems: 1
> >>> +    description:
> >>> +      Maximum supply current, see dt-bindings/leds/leds-aw200xx.h
> >>
> >> No. Use existing properties from common.yaml. This looks like
> >> led-max-microamp and it is per LED, not per entire device.
> > 
> > The AW200XX LED chip does not support imax setup per led.
> > Imax is the global parameter over the all leds. I suppose, it's better
> > to add vendor prefix or take minimum from all subnodes?
> > How do you think?
> 
> Have in mind that led-max-microamp is a required property in some cases,
> so skipping it and using per-device properties does not solve the
> problem of adjusting proper currents. What if each LED you set for
> something which in total gives more than your imax?
> 

You are right. From my point of view too, we must build our solutions from
HW capabilities. In the current situation, AW200XX chips support global
Imax value, so it's acceptable decision to use vendor prefix for global
imax parameter, why not?

...
  

On 02/12/2022 19:53, Dmitry Rokosov wrote:
> Hello Krzysztof,
> 
> On Fri, Dec 02, 2022 at 05:41:37PM +0100, Krzysztof Kozlowski wrote:
>> On 28/11/2022 18:43, Martin Kurbanov wrote:
>>> Hi. Thank you for quick reply. 
>>>
>>> On 25.11.2022 11:29, Krzysztof Kozlowski wrote:
>>>>> +
>>>>> +  imax:
>>>>> +    maxItems: 1
>>>>> +    description:
>>>>> +      Maximum supply current, see dt-bindings/leds/leds-aw200xx.h
>>>>
>>>> No. Use existing properties from common.yaml. This looks like
>>>> led-max-microamp and it is per LED, not per entire device.
>>>
>>> The AW200XX LED chip does not support imax setup per led.
>>> Imax is the global parameter over the all leds. I suppose, it's better
>>> to add vendor prefix or take minimum from all subnodes?
>>> How do you think?
>>
>> Have in mind that led-max-microamp is a required property in some cases,
>> so skipping it and using per-device properties does not solve the
>> problem of adjusting proper currents. What if each LED you set for
>> something which in total gives more than your imax?
>>
> 
> You are right. From my point of view too, we must build our solutions from
> HW capabilities. 

And there was no proposal to go around HW capabilities. We talk only
about representation.

> In the current situation, AW200XX chips support global
> Imax value, so it's acceptable decision to use vendor prefix for global
> imax parameter, why not?

Jacek made his statement some time ago quite clear:

https://lore.kernel.org/all/5785F17D.3010108@samsung.com/

"If you question the idea of having different maximum brightness per
sub-LEDs controlled by the same device, then it means that you have
objections to the entire idea of LED subsystem max_brightness property,
whereas it has been broadly accepted and successfully used for years."

Best regards,
Krzysztof