mbox series

[0/4] backlight: ktz8866: improve it and support slave

Message ID 20250407095119.588920-1-mitltlatltl@gmail.com
Headers show
Series backlight: ktz8866: improve it and support slave | expand

Message

Pengyu Luo April 7, 2025, 9:51 a.m. UTC
Sending this patchset to support coming devices which are using dual
ktz8866, original driver would only handle half backlight region on
these devices, registering it twice is unreasonable,  and two devices
share the same resources(pinctrl) which is required for every single
node under recent dt-bindings, so adding new support. and improve
original driver. Details in every commit log.

Pengyu Luo (4):
  dt-bindings: backlight: kinetic,ktz8866: add ktz8866 slave compatible
  backlight: ktz8866: add slave handler
  backlight: ktz8866: improve current sinks setting
  backlight: ktz8866: add definitions to make it more readable

 .../leds/backlight/kinetic,ktz8866.yaml       | 29 +++++-
 drivers/video/backlight/ktz8866.c             | 99 ++++++++++++++++---
 2 files changed, 108 insertions(+), 20 deletions(-)

Comments

Krzysztof Kozlowski April 7, 2025, 9:57 a.m. UTC | #1
On 07/04/2025 11:51, Pengyu Luo wrote:
> Kinetic ktz8866, found in many android devices, nowadays, some oem use
> dual ktz8866 to support a larger panel and  higher brightness, add the

Just one space after 'and'.

> binding for slave case.
> 
> Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> ---
>  .../leds/backlight/kinetic,ktz8866.yaml       | 29 +++++++++++++++----
>  1 file changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/kinetic,ktz8866.yaml b/Documentation/devicetree/bindings/leds/backlight/kinetic,ktz8866.yaml
> index c914e1276..825a6fbf1 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/kinetic,ktz8866.yaml
> +++ b/Documentation/devicetree/bindings/leds/backlight/kinetic,ktz8866.yaml
> @@ -19,7 +19,9 @@ allOf:
>  
>  properties:
>    compatible:
> -    const: kinetic,ktz8866
> +    enum:
> +      - kinetic,ktz8866
> +      - kinetic,ktz8866-slave

Does not look right. That's the same device. Same devices have same
compatible.

>  
>    reg:
>      maxItems: 1
> @@ -58,9 +60,16 @@ properties:
>  required:
>    - compatible
>    - reg
> -  - vddpos-supply
> -  - vddneg-supply
> -  - enable-gpios
> +
> +if:
> +  properties:
> +    compatible:
> +      const: kinetic,ktz8866
> +then:
> +  required:
> +    - vddpos-supply
> +    - vddneg-supply
> +    - enable-gpios

I don't understand why other device does not need power.

>  
>  unevaluatedProperties: false
>  
> @@ -68,7 +77,7 @@ examples:
>    - |
>      #include <dt-bindings/gpio/gpio.h>
>  
> -    i2c {
> +    i2c0 {

No, don't change.

>          #address-cells = <1>;
>          #size-cells = <0>;
>  
> @@ -84,3 +93,13 @@ examples:
>              kinetic,enable-lcd-bias;
>          };
>      };
> +
> +    i2c1 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        backlight@11 {
> +            compatible = "kinetic,ktz8866-slave";
> +            reg = <0x11>;

No real differences here, so drop the example. Anyway, new examples
would start from - | (see other bindings).



Best regards,
Krzysztof
Daniel Thompson April 7, 2025, 4:13 p.m. UTC | #2
On Mon, Apr 07, 2025 at 05:51:18PM +0800, Pengyu Luo wrote:
> I polled all registers when the module was loading, found that
> current sinks have already been configured. Bootloader would set
> when booting. So checking it before setting the all channels.

Can you rephrase this so the problem and solution are more clearly
expressed. Perhaps template Ingo suggests here would be good:
https://www.spinics.net/lists/kernel/msg1633438.html


> Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> ---
>  drivers/video/backlight/ktz8866.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/backlight/ktz8866.c b/drivers/video/backlight/ktz8866.c
> index 017ad80dd..b67ca136d 100644
> --- a/drivers/video/backlight/ktz8866.c
> +++ b/drivers/video/backlight/ktz8866.c
> @@ -46,6 +46,8 @@
>  #define LCD_BIAS_EN 0x9F
>  #define PWM_HYST 0x5
>
> +#define CURRENT_SINKS_MASK GENMASK(5, 0)
> +

Call this BL_EN_CURRENT_SINKS_MASK and keep it next to the register it
applies to.


>  struct ktz8866_slave {
>  	struct i2c_client *client;
>  	struct regmap *regmap;
> @@ -112,11 +120,18 @@ static void ktz8866_init(struct ktz8866 *ktz)
>  {
>  	unsigned int val = 0;
>
> -	if (!of_property_read_u32(ktz->client->dev.of_node, "current-num-sinks", &val))
> +	if (!of_property_read_u32(ktz->client->dev.of_node, "current-num-sinks", &val)) {
>  		ktz8866_write(ktz, BL_EN, BIT(val) - 1);
> -	else
> -		/* Enable all 6 current sinks if the number of current sinks isn't specified. */
> -		ktz8866_write(ktz, BL_EN, BIT(6) - 1);
> +	} else {
> +		/*
> +		 * Enable all 6 current sinks if the number of current
> +		 * sinks isn't specified and the bootloader didn't set
> +		 * the value.
> +		 */
> +		ktz8866_read(ktz, BL_EN, &val);
> +		if (!(val && CURRENT_SINKS_MASK))

This is the wrong form of AND.

> +			ktz8866_write(ktz, BL_EN, CURRENT_SINKS_MASK);
> +	}


Daniel.