mbox series

[v5,00/13] Add MediaTek MT6370 PMIC support

Message ID 20220715112607.591-1-peterwu.pub@gmail.com
Headers show
Series Add MediaTek MT6370 PMIC support | expand

Message

ChiaEn Wu July 15, 2022, 11:25 a.m. UTC
From: ChiaEn Wu <chiaen_wu@richtek.com>

This patch series add MediaTek MT6370 PMIC support. The MT6370 is a
highly-integrated smart power management IC, which includes a single
cell Li-Ion/Li-Polymer switching battery charger, a USB
Type-C & Power Delivery (PD) controller, dual Flash LED current sources,
a RGB LED driver, a backlight WLED driver, a display bias driver and a
general LDO for portable devices.

In this series of patches, we based on Andy Shevchenko's mfd patch used to
adjust the Makefile order.
(https://lore.kernel.org/all/20220616182524.7956-2-andriy.shevchenko@linux.intel.com/)
Among with this, we took some changes for MT6370 and refined the MT6370 device
tree files to comply with DT specifications.

"[PATCH v5 06/13] dt-bindings: mfd: Add MediaTek MT6370" depends on previous
DT binding patches, so before applying this patch, please apply other DT
patches first. Thanks!

Thank you,
ChiaEn Wu

---
Changes in v5:
- In patch 07/13:
	- Add the comma in the last REGMAP_IRQ_REG_LINE(),
	   DEFINE_RES_IRQ_NAMED() and MFD_CELL_RES()
	- Add the prefix in the first parameter of all mfd_cell
	- Move enum and struct mt6370_info to mt6370.h
	- Remove struct device *dev in struct mt6370_info
	- Revise the description of Kconfig help text
	- Revise MODULE_DESCRIPTION()

- In patch 08/13:
	- Add comma for the last index of mt6370_reg_init.
	- Use dev_err_probe to decrease LOC.
	- Use 'dev' variable to make probe function more clean.
	- Refine kconfig text.
	- Remove both 'else' in set_vbus callback.
	- Remove comma for of_device_id if the assigned member is only one.

- In patch 09/13:
	- Replace using snprintf() with sysfs_emit() in mt6370_adc_read_label()
	- Remove macro ADC_CONV_TIME_US
	- Revise all variable ordering
	- Revise the description of Kconfig help text
	- Revise MODULE_DESCRIPTION()

- In patch 10/13:
	- Replace unsigned int type of pwr_rdy with bool in
	   mt6370_chg_set_online()
	- Remove redundant 'else' in mt6370_chg_field_get()
	- Revise 'if-else' in mt6370_chg_field_set()
	- Revise 'if' condition in mt6370_chg_enable_irq()
	- Revise all text 'otg' --> 'OTG'
	- Revise MT6370_MIVR_IBUS_TH_100_MA --> MT6370_MIVR_IBUS_TH_100_mA
	- Revise the description of Kconfig help text

- In patch 12/13:
	- Refine descriptions.
	- Refine the macro name.
	- Refine the bracket and blanks.

- In patch 13/13:
	- Add missed <mod_devicetable.h>
	- Add struct device *dev in probe() to make code cleaning
	- Remove useless including header file <gpio/driver.h>, <of.h>
	- Remove useless variable uasage in mt6370_init_backlight_properties()
	- Remove redundant checking enable_gpio in mt6370_bl_update_status()
	- Remove redundant parentheses in mt6370_bl_get_brightness()
	- Revise the description of Kconfig help text
	- Revise the calculation of hys_th_steps


Changes in v4:
- In patch 02/13:
	- Add minItems of "io-channel-names"
	- Replace text "Mediatek" with "MediaTek"

- In patch 06/13:
	- Roll back all "$ref: " to v2 patch style (using "/schemas/...")

- In patch 07/13:
	- Replace text "Mediatek" with "MediaTek" in Kconfig
	- Replace "first break and then return" with "return directly"
	   in "mt6370_check_vendor_info()"
	- Add module name related description in Kconfig helptext
	- Add Copyright in the source code
	- Add header file "mt6370.h" for all "#define IRQ"
	- Adjust Makefile order of MT6370
	- Refine "bank_idx" and "bank_addr" in
	  "mt6375_regmap_read()" / "mt6375_regmap_write()"
	- Refine redundant "else if" in "mt6370_regmap_read()"

- In patch 08/13:
	- Replace text "Mediatek" with "MediaTek" in Kconfig
	- Replace "first ret=regulator_(dis/en)able and then return"
	   with "return directly" in "mt6370_tcpc_set_vbus()"
	- Replace header file <linux/of.h> with <linux/mod_devicetable.h>
	- Add Copyright in the source code
	- Add module name related description in Kconfig helptext
	- Remove header file <linux/of.h>
	- Refine all probe error by using dev_err_probe()

- In patch 09/13:
	- Replace text "Mediatek" with "MediaTek"
	- Replace all "first dev_err() and then return" with
	   "return dev_err_probe()"
	- Add Copyright in the source code
	- Add module name related description in Kconfig
	- Add unit suffix of macro "ADC_CONV_POLLING_TIME"
	- Add new macro "ADC_CONV_TIME_MS"
	- Adjust the position of include file <mediatek,mt6370_adc.h>
	- Adjust the postions between <linux/module.h> and
	   <linux/mod_devicetable.h>
	- Fix some incorrect characters

- In patch 10/13:
	- Replace text "Mediatek" with "MediaTek" in Kconfig and
	   MODULE_DESCRIPTION()
	- Replace "mt6370_chg_val_to_reg" and "mt6370_chg_reg_to_val"
	   with "linear_range" API
	- Replace "first break and then return" with "return directly"
	   in all cases of get/set power_supply_property
	- Replace all "first dev_err() and then return" with "return
	   dev_err_probe()"
	- Replace all "return IS_ERR(priv->rdev) ? PTR_ERR(priv->rdev) : 0"
	   with "PTR_ERR_OR_ZERO()"
	- Replace "priv->dev->of_node" with "dev_of_node()"
	- Add Copyright in the source code
	- Add module name related description in Kconfig helptext
	- Add proper unit of "MT6370_MIVR_IBUS_TH"
	- Add error check in "mt6370_chg_get_status"
	- Remove including <mediatek,mt6370_adc.h> header file
	- Remove redundant comma of every enum terminator line
	- Remove unwanted blank lines
	- Remove the useless label (toggle_cfo_exit:)
	- Remove using atomic
	- Remove using of_match_ptr()
	- Fix some incorrect characters
	- Fix updating wrong bits when using ena_gpiod of OTG regulator
	- Adjust the probe order in probe()

- In patch 11/13:
	- Replace text "Mediatek" with "MediaTek" in Kconfig
	- Replace text "const" with "constant" in Kconfig
	- Add Copyright in the source code

- In patch 12/13:
	- Replace text "Mediatek" with "MediaTek" in Kconfig
	- Add Copyright in the source code

- In patch 13/13:
	- Replace text "Mediatek" with "MediaTek" in Kconfig
	- Add Copyright in the source code
	- Revise the comment of "PWM HYS STEPS"


Changes in v3:
- Remove ADC ABI file, which is added in v2 Patch 7
- In patch 02/14:
	- Add items and remove maxItems of io-channels
	- Add io-channel-names and describe each item
	- Add "unevaluatedProperties: false" in "usb-otg-vbus-regulator"
	- Rename "enable-gpio" to "enable-gpios" in "usb-otg-vbus-regulator"
- In patch 03/14:
	- Use leds-class-multicolor.yaml instead of common.yaml.
	- Split multi-led and led node.
	- Add subdevice "led" in "multi-led".
- In patch 04/14:
	- Remove the description of enum.
- In patch 05/14:
	- Rename "mediatek,bled-pwm-hys-input-threshold-steps" to
	  "mediatek,bled-pwm-hys-input-th-steps"
	- Refine "bled-pwm-hys-input-th-steps", "bled-ovp-microvolt",
	  "bled-ocp-microamp" enum values
- In patch 06/14:
	- Use " in entire patchset
	- Refine ADC description
	- Rename "enable-gpio" to "enable-gpios" in "regualtor"
- In patch 07/14:
	- Refine Kconfig help text
	- Refine error message of unknown vendor ID in
	  mt6370_check_vendor_info()
	- Refine return value handling of mt6370_regmap_read()
	- Refine all probe error by using dev_err_probe()
	- Refine "bank_idx" and "bank_addr" in mt6370_regmap_read() and
	  mt6370_regmap_write()
	- Add "#define VENID*" and drop the comments in
	  mt6370_check_vendor_info()
	- Drop "MFD" in MODULE_DESCRIPTION()
- In patch 09/14:
	- Refine Kconfig help text
- In patch 10/14:
	- Refine Kconfig help text
	- Refine all channel value in read_scale()
		a. current: uA --> mA
		b. voltage: uV --> mV
		c. temperature: degrees Celsius --> milli degrees Celsius
	- Add "default:" condition of switch statement in read_scale() and read_raw()
	- Add error message for reading ADC register failed
	- Add the comment for adc_lock
	- Add <linux/mod_devicetable.h> header file for struct of_device_id
	- Replace "adc" text with "ADC" in all of the error messages
- In patch 12/14:
	- Refine the grammer of the Kconfig.
	- Change reg mode to the const current mode.
- In patch 14/14:
	- Refine bool properties parsing (pwm-enable, ovp-shutdown, ocp-shutdown) in DT
	  parsing function
	- Refine u32 and u8 properties parsing (pwm-hys-input-th-steps, ovp-microvolt,
	  ocp-microamp), from using register value to using actual value
	- Refine error string of "channle-use" parsing failed
	- Refine Kconfig help text


Changes in v2:
- In patch 01/15:
	- Add "unevaluatedProperties: false".
	- Delete "DT bindings".
	- Refine the description to fit in 80 columns.
	- Skip the connector description.
- In patch 02/15:
	- Refine items description of interrupt-name
	- Rename "usb-otg-vbus" to "usb-otg-vbus-regulator"
	- Add constraint properties for ADC
- In patch 03/15:
	- Skip not useful description of "^(multi-)?led@[0-3]$"
	  and reg.
	- Due to the dependency, remove the mention of mfd
	  document directory.
	- Delete Soft-start property. In design aspect, we think
	  soft-restart should always be enabled, our new chip
	  has deleted the related setting register , also, we don’t
	  allow user adjust this parameter in this chip.
	- Refine the commit message.
- In patch 04/15:
	- Skip not useful description of "^led@[0-1]$" and reg.
	- Add apace after '#'.
	- Refine the commit message.
- In patch 05/15:
	- Remove "binding documentation" in subject title
	- Refine description of mt6370 backlight binding
	  document
	- Refine properties name(bled-pwm-hys-input-bit,
	  bled-ovp-microvolt, bled-ocp-microamp) and their
	  description
- In patch 06/15:
	- Refine ADC and Regulator descriptions
	- Refine include header usage in example
	- Refine node name to generic node name("pmic@34")
	- Refine led example indentation
	- Refine license of mediatek,mt6370_adc.h
	- Rename the dts example from IRQ define to number.
	- Remove mediatek,mt6370.h
- In patch 07/15:
	- Add ABI documentation for mt6370 non-standard ADC
	  sysfs interfaces.
- In patch 08/15:
	- Add all IRQ define into mt6370.c.
	- Refine include header usage
- In patch 09/15:
	- No changes.
- In patch 10/15:
	- Use 'gpiod_get_from_of_node' to replace
	  'fwnode_gpiod_get_index'.
- In patch 11/15:
	- Refine Kconfig mt6370 help text
	- Refine mask&shift to FIELD_PREP()
	- Refine mutex lock name ("lock" -> "adc_lock")
	- Refine mt6370_adc_read_scale()
	- Refine mt6370_adc_read_offset()
	- Refine mt6370_channel_labels[] by using enum to index
	  chan spec
	- Refine MT6370_ADC_CHAN()
	- Refine indio_dev->name
	- Remove useless include header files
- In patch 12/15:
	- Refine mt6370_chg_otg_rdesc.of_match
	  ("mt6370,otg-vbus" -> "usb-otg-vbus-regulator") to match
	  DT binding
- In patch 13/15:
	- Refine Kconfig description.
	- Remove include "linux/of.h" and use
	  "linux/mod_devicetable.h".
	- Place a comma for the last element of the const
	  unsigned int array.
	- Add a comment line for the mutex 'lock'.
	- In probe function, use 'dev_err_probe' in some
	  judgement to reduce the LOC.
	- Refine include header usage.
	  BIT/GENMASK -> linux/bits.h
	  FIELD_GET -> linux/bitfield.h
- In patch 14/15:
	- Add blank line.
	- Replace container_of() with to_mt6370_led() .
	- Refine description of ramping.
	- Refine the mt6370_init_common_properties function.
	- Refine the probe return.
- In patch 15/15:
	- Refine MT6370 help text in Kconfig
	- Refine DT Parse function
	- Remove useless enum
	- Add comment for 6372 backward compatible in
	  bl_update_status() and
	  check_vendor_info()
	- Using dev_err_probe(); insteads dev_err()&return; in
	  the probe()

Alice Chen (2):
  dt-bindings: leds: Add MediaTek MT6370 flashlight
  leds: flashlight: mt6370: Add MediaTek MT6370 flashlight support

ChiYuan Huang (7):
  dt-bindings: usb: Add MediaTek MT6370 TCPC
  dt-bindings: leds: mt6370: Add MediaTek MT6370 current sink type LED
    indicator
  dt-bindings: backlight: Add MediaTek MT6370 backlight
  dt-bindings: mfd: Add MediaTek MT6370
  mfd: mt6370: Add MediaTek MT6370 support
  usb: typec: tcpci_mt6370: Add Mediatek MT6370 tcpci driver
  leds: mt6370: Add MediaTek MT6370 current sink type LED Indicator
    support

ChiaEn Wu (4):
  dt-bindings: power: supply: Add MediaTek MT6370 Charger
  iio: adc: mt6370: Add MediaTek MT6370 support
  power: supply: mt6370: Add MediaTek MT6370 charger driver
  video: backlight: mt6370: Add MediaTek MT6370 support

 .../leds/backlight/mediatek,mt6370-backlight.yaml  |   92 ++
 .../bindings/leds/mediatek,mt6370-flashlight.yaml  |   41 +
 .../bindings/leds/mediatek,mt6370-indicator.yaml   |   77 ++
 .../devicetree/bindings/mfd/mediatek,mt6370.yaml   |  280 ++++++
 .../power/supply/mediatek,mt6370-charger.yaml      |   88 ++
 .../bindings/usb/mediatek,mt6370-tcpc.yaml         |   36 +
 drivers/iio/adc/Kconfig                            |   12 +
 drivers/iio/adc/Makefile                           |    1 +
 drivers/iio/adc/mt6370-adc.c                       |  273 +++++
 drivers/leds/Kconfig                               |   14 +
 drivers/leds/Makefile                              |    1 +
 drivers/leds/flash/Kconfig                         |   12 +
 drivers/leds/flash/Makefile                        |    1 +
 drivers/leds/flash/leds-mt6370-flash.c             |  661 ++++++++++++
 drivers/leds/leds-mt6370.c                         |  994 ++++++++++++++++++
 drivers/mfd/Kconfig                                |   16 +
 drivers/mfd/Makefile                               |    1 +
 drivers/mfd/mt6370.c                               |  281 ++++++
 drivers/mfd/mt6370.h                               |   99 ++
 drivers/power/supply/Kconfig                       |   14 +
 drivers/power/supply/Makefile                      |    1 +
 drivers/power/supply/mt6370-charger.c              | 1062 ++++++++++++++++++++
 drivers/usb/typec/tcpm/Kconfig                     |   11 +
 drivers/usb/typec/tcpm/Makefile                    |    1 +
 drivers/usb/typec/tcpm/tcpci_mt6370.c              |  207 ++++
 drivers/video/backlight/Kconfig                    |   12 +
 drivers/video/backlight/Makefile                   |    1 +
 drivers/video/backlight/mt6370-backlight.c         |  339 +++++++
 include/dt-bindings/iio/adc/mediatek,mt6370_adc.h  |   18 +
 29 files changed, 4646 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/backlight/mediatek,mt6370-backlight.yaml
 create mode 100644 Documentation/devicetree/bindings/leds/mediatek,mt6370-flashlight.yaml
 create mode 100644 Documentation/devicetree/bindings/leds/mediatek,mt6370-indicator.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/mediatek,mt6370.yaml
 create mode 100644 Documentation/devicetree/bindings/power/supply/mediatek,mt6370-charger.yaml
 create mode 100644 Documentation/devicetree/bindings/usb/mediatek,mt6370-tcpc.yaml
 create mode 100644 drivers/iio/adc/mt6370-adc.c
 create mode 100644 drivers/leds/flash/leds-mt6370-flash.c
 create mode 100644 drivers/leds/leds-mt6370.c
 create mode 100644 drivers/mfd/mt6370.c
 create mode 100644 drivers/mfd/mt6370.h
 create mode 100644 drivers/power/supply/mt6370-charger.c
 create mode 100644 drivers/usb/typec/tcpm/tcpci_mt6370.c
 create mode 100644 drivers/video/backlight/mt6370-backlight.c
 create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6370_adc.h

Comments

AngeloGioacchino Del Regno July 15, 2022, 12:38 p.m. UTC | #1
Il 15/07/22 13:26, ChiaEn Wu ha scritto:
> From: ChiaEn Wu <chiaen_wu@richtek.com>
> 
> MediaTek MT6370 is a SubPMIC consisting of a single cell battery charger
> with ADC monitoring, RGB LEDs, dual channel flashlight, WLED backlight
> driver, display bias voltage supply, one general purpose LDO, and the
> USB Type-C & PD controller complies with the latest USB Type-C and PD
> standards.
> 
> This adds support for MediaTek MT6370 Backlight driver. It's commonly used
> to drive the display WLED. There are 4 channels inside, and each channel
> supports up to 30mA of current capability with 2048 current steps in
> exponential or linear mapping curves.
> 
> Signed-off-by: ChiaEn Wu <chiaen_wu@richtek.com>

Hello ChiaEn,

I propose to move this one to drivers/leds (or drivers/pwm) and, instead of
registering a backlight device, register a PWM device.

This way you will be able to reuse the generic backlight-pwm driver, as you'd
be feeding the PWM device exposed by this driver to the generic one: this will
most importantly make it easy to chain it with MTK_DISP_PWM (mtk-pwm-disp)
with a devicetree that looks like...

	pwmleds-disp {

		compatible = "pwm-leds";



		disp_led: disp-pwm {

			label = "backlight-pwm";

			pwms = <&pwm0 0 500000>;

			max-brightness = <1024>;

		};

	};



	backlight_lcd0: backlight {

		compatible = "led-backlight";

		leds = <&disp_led>, <&pmic_bl_led>;



		default-brightness-level = <300>;

	};

Regards,
Angelo
AngeloGioacchino Del Regno July 15, 2022, 12:38 p.m. UTC | #2
Il 15/07/22 13:26, ChiaEn Wu ha scritto:
> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> The MediaTek MT6370 is a highly-integrated smart power management IC,
> which includes a single cell Li-Ion/Li-Polymer switching battery
> charger, a USB Type-C & Power Delivery (PD) controller, dual
> Flash LED current sources, a RGB LED driver, a backlight WLED driver,
> a display bias driver and a general LDO for portable devices.
> 
> In MediaTek MT6370, there are four channel current-sink RGB LEDs that
> support hardware pattern for constant current, PWM, and breath mode.
> Isink4 channel can also be used as a CHG_VIN power good indicator.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
>   drivers/leds/Kconfig       |  14 +
>   drivers/leds/Makefile      |   1 +
>   drivers/leds/leds-mt6370.c | 994 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 1009 insertions(+)
>   create mode 100644 drivers/leds/leds-mt6370.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index a49979f..71bacb5 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -244,6 +244,20 @@ config LEDS_MT6323
>   	  This option enables support for on-chip LED drivers found on
>   	  Mediatek MT6323 PMIC.
>   
> +config LEDS_MT6370_RGB
> +	tristate "LED Support for MediaTek MT6370 PMIC"
> +	depends on LEDS_CLASS
> +	depends on MFD_MT6370
> +	select LINEAR_RANGE
> +	help
> +	  Say Y here to enable support for MT6370_RGB LED device.
> +	  In MT6370, there are four channel current-sink LED drivers that
> +	  support hardware pattern for constant current, PWM, and breath mode.
> +	  Isink4 channel can also be used as a CHG_VIN power good indicator.
> +
> +	  This driver can also be built as a module. If so the module
> +	  will be called "leds-mt6370.ko".
> +
>   config LEDS_S3C24XX
>   	tristate "LED Support for Samsung S3C24XX GPIO LEDs"
>   	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 4fd2f92..557be42 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -62,6 +62,7 @@ obj-$(CONFIG_LEDS_MIKROTIK_RB532)	+= leds-rb532.o
>   obj-$(CONFIG_LEDS_MLXCPLD)		+= leds-mlxcpld.o
>   obj-$(CONFIG_LEDS_MLXREG)		+= leds-mlxreg.o
>   obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
> +obj-$(CONFIG_LEDS_MT6370_RGB)		+= leds-mt6370.o
>   obj-$(CONFIG_LEDS_NET48XX)		+= leds-net48xx.o
>   obj-$(CONFIG_LEDS_NETXBIG)		+= leds-netxbig.o
>   obj-$(CONFIG_LEDS_NIC78BX)		+= leds-nic78bx.o
> diff --git a/drivers/leds/leds-mt6370.c b/drivers/leds/leds-mt6370.c
> new file mode 100644
> index 0000000..1038232
> --- /dev/null
> +++ b/drivers/leds/leds-mt6370.c
> @@ -0,0 +1,994 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 Richtek Technology Corp.
> + *
> + * Author: Alice Chen <alice_chen@richtek.com>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/kernel.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/linear_range.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +enum {
> +	MT6370_LED_ISNK1 = 0,
> +	MT6370_LED_ISNK2,
> +	MT6370_LED_ISNK3,
> +	MT6370_LED_ISNK4,
> +	MT6370_MAX_LEDS
> +};
> +
> +enum mt6370_led_mode {
> +	MT6370_LED_PWM_MODE = 0,
> +	MT6370_LED_BREATH_MODE,
> +	MT6370_LED_REG_MODE,
> +	MT6370_LED_MAX_MODE
> +};
> +
> +enum mt6370_led_field {
> +	F_RGB_EN = 0,
> +	F_CHGIND_EN,
> +	F_LED1_CURR,
> +	F_LED2_CURR,
> +	F_LED3_CURR,
> +	F_LED4_CURR,
> +	F_LED1_MODE,
> +	F_LED2_MODE,
> +	F_LED3_MODE,
> +	F_LED4_MODE,
> +	F_LED1_DUTY,
> +	F_LED2_DUTY,
> +	F_LED3_DUTY,
> +	F_LED4_DUTY,
> +	F_LED1_FREQ,
> +	F_LED2_FREQ,
> +	F_LED3_FREQ,
> +	F_LED4_FREQ,
> +	F_MAX_FIELDS
> +};
> +
> +enum mt6370_led_ranges {
> +	R_LED123_CURR = 0,
> +	R_LED4_CURR,
> +	R_LED_TRFON,
> +	R_LED_TOFF,
> +	R_MAX_RANGES,
> +};
> +
> +enum mt6370_pattern {
> +	P_LED_TR1 = 0,
> +	P_LED_TR2,
> +	P_LED_TF1,
> +	P_LED_TF2,
> +	P_LED_TON,
> +	P_LED_TOFF,
> +	P_MAX_PATTERNS
> +};
> +
> +#define MT6370_REG_DEV_INFO			0x100
> +#define MT6370_REG_RGB1_DIM			0x182
> +#define MT6370_REG_RGB2_DIM			0x183
> +#define MT6370_REG_RGB3_DIM			0x184
> +#define MT6370_REG_RGB_EN			0x185
> +#define MT6370_REG_RGB1_ISNK			0x186
> +#define MT6370_REG_RGB2_ISNK			0x187
> +#define MT6370_REG_RGB3_ISNK			0x188
> +#define MT6370_REG_RGB1_TR			0x189
> +#define MT6370_REG_RGB_CHRIND_DIM		0x192
> +#define MT6370_REG_RGB_CHRIND_CTRL		0x193
> +#define MT6370_REG_RGB_CHRIND_TR		0x194
> +
> +#define MT6372_REG_RGB_EN			0x182
> +#define MT6372_REG_RGB1_ISNK			0x183
> +#define MT6372_REG_RGB2_ISNK			0x184
> +#define MT6372_REG_RGB3_ISNK			0x185
> +#define MT6372_REG_RGB4_ISNK			0x186
> +#define MT6372_REG_RGB1_DIM			0x187
> +#define MT6372_REG_RGB2_DIM			0x188
> +#define MT6372_REG_RGB3_DIM			0x189
> +#define MT6372_REG_RGB4_DIM			0x18A
> +#define MT6372_REG_RGB12_FREQ			0x18B
> +#define MT6372_REG_RGB34_FREQ			0x18C
> +#define MT6372_REG_RGB1_TR			0x18D
> +
> +#define MT6370_VENID_MASK			GENMASK(7, 4)
> +#define MT6370_CHEN_BIT(id)			BIT(MT6370_LED_ISNK4 - id)
> +#define MT6370_VIRTUAL_MULTICOLOR		5
> +#define MC_CHANNEL_NUM				3
> +#define MT6370_PWM_DUTY				31
> +#define MT6372_PMW_DUTY				255

Please fix this typo: PMW -> PWM

> +
> +#define STATE_OFF				0
> +#define STATE_KEEP				1
> +#define STATE_ON				2

I propose, instead:

enum mt6370_state {
	STATE_OFF = 0,
	STATE_KEEP,
	STATE_ON,
	STATE_MAX,
};

> +
> +struct mt6370_led {
> +	union {
> +		struct led_classdev isink;
> +		struct led_classdev_mc mc;
> +	};
> +	struct mt6370_priv *priv;
> +	u32 default_state;
> +	u32 index;
> +};
> +
> +struct mt6370_priv {
> +	/* Per LED access lock */
> +	struct mutex lock;
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct regmap_field *fields[F_MAX_FIELDS];
> +	const struct reg_field *reg_fields;
> +	const struct linear_range *ranges;
> +	struct reg_cfg *reg_cfgs;
> +	unsigned int leds_count;
> +	unsigned int leds_active;
> +	bool is_mt6372;

Instead of using a bool for that, you can enhance the flexibility of
this driver (and also somewhat reduce cpu cycles) by using:

	struct mt6370_pdata *pdata;

where a good definition of that structure would be...

struct mt6370_pdata {

	const unsigned int *tfreq;

	unsigned int tfreq_len;

	u8 pwm_duty;

	u8 reg_rgb1_tr;	



	/*

	 * You can set this one to -1 on MT6372 to indicate that

	 * this register does not exist (hence use that in function

	 * mt6370_get_breath_reg_base() to replace priv->is_mt6372)

	 */

	s16 reg_rgb_chrind_tr;

};

> +	struct mt6370_led leds[];
> +};
> +
> +static const struct reg_field common_reg_fields[F_MAX_FIELDS] = {
> +	[F_RGB_EN]	= REG_FIELD(MT6370_REG_RGB_EN, 4, 7),
> +	[F_CHGIND_EN]	= REG_FIELD(MT6370_REG_RGB_CHRIND_DIM, 7, 7),
> +	[F_LED1_CURR]	= REG_FIELD(MT6370_REG_RGB1_ISNK, 0, 2),
> +	[F_LED2_CURR]	= REG_FIELD(MT6370_REG_RGB2_ISNK, 0, 2),
> +	[F_LED3_CURR]	= REG_FIELD(MT6370_REG_RGB3_ISNK, 0, 2),
> +	[F_LED4_CURR]	= REG_FIELD(MT6370_REG_RGB_CHRIND_CTRL, 0, 1),
> +	[F_LED1_MODE]	= REG_FIELD(MT6370_REG_RGB1_DIM, 5, 6),
> +	[F_LED2_MODE]	= REG_FIELD(MT6370_REG_RGB2_DIM, 5, 6),
> +	[F_LED3_MODE]	= REG_FIELD(MT6370_REG_RGB3_DIM, 5, 6),
> +	[F_LED4_MODE]	= REG_FIELD(MT6370_REG_RGB_CHRIND_DIM, 5, 6),
> +	[F_LED1_DUTY]	= REG_FIELD(MT6370_REG_RGB1_DIM, 0, 4),
> +	[F_LED2_DUTY]	= REG_FIELD(MT6370_REG_RGB2_DIM, 0, 4),
> +	[F_LED3_DUTY]	= REG_FIELD(MT6370_REG_RGB3_DIM, 0, 4),
> +	[F_LED4_DUTY]	= REG_FIELD(MT6370_REG_RGB_CHRIND_DIM, 0, 4),
> +	[F_LED1_FREQ]	= REG_FIELD(MT6370_REG_RGB1_ISNK, 3, 5),
> +	[F_LED2_FREQ]	= REG_FIELD(MT6370_REG_RGB2_ISNK, 3, 5),
> +	[F_LED3_FREQ]	= REG_FIELD(MT6370_REG_RGB3_ISNK, 3, 5),
> +	[F_LED4_FREQ]	= REG_FIELD(MT6370_REG_RGB_CHRIND_CTRL, 2, 4)
> +};
> +
> +static const struct reg_field mt6372_reg_fields[F_MAX_FIELDS] = {
> +	[F_RGB_EN]	= REG_FIELD(MT6372_REG_RGB_EN, 4, 7),
> +	[F_CHGIND_EN]	= REG_FIELD(MT6372_REG_RGB_EN, 3, 3),
> +	[F_LED1_CURR]	= REG_FIELD(MT6372_REG_RGB1_ISNK, 0, 3),
> +	[F_LED2_CURR]	= REG_FIELD(MT6372_REG_RGB2_ISNK, 0, 3),
> +	[F_LED3_CURR]	= REG_FIELD(MT6372_REG_RGB3_ISNK, 0, 3),
> +	[F_LED4_CURR]	= REG_FIELD(MT6372_REG_RGB4_ISNK, 0, 3),
> +	[F_LED1_MODE]	= REG_FIELD(MT6372_REG_RGB1_ISNK, 6, 7),
> +	[F_LED2_MODE]	= REG_FIELD(MT6372_REG_RGB2_ISNK, 6, 7),
> +	[F_LED3_MODE]	= REG_FIELD(MT6372_REG_RGB3_ISNK, 6, 7),
> +	[F_LED4_MODE]	= REG_FIELD(MT6372_REG_RGB4_ISNK, 6, 7),
> +	[F_LED1_DUTY]	= REG_FIELD(MT6372_REG_RGB1_DIM, 0, 7),
> +	[F_LED2_DUTY]	= REG_FIELD(MT6372_REG_RGB2_DIM, 0, 7),
> +	[F_LED3_DUTY]	= REG_FIELD(MT6372_REG_RGB3_DIM, 0, 7),
> +	[F_LED4_DUTY]	= REG_FIELD(MT6372_REG_RGB4_DIM, 0, 7),
> +	[F_LED1_FREQ]	= REG_FIELD(MT6372_REG_RGB12_FREQ, 5, 7),
> +	[F_LED2_FREQ]	= REG_FIELD(MT6372_REG_RGB12_FREQ, 2, 4),
> +	[F_LED3_FREQ]	= REG_FIELD(MT6372_REG_RGB34_FREQ, 5, 7),
> +	[F_LED4_FREQ]	= REG_FIELD(MT6372_REG_RGB34_FREQ, 2, 4)
> +};
> +
> +/* Current unit: microamp, time unit: millisecond */
> +static const struct linear_range common_led_ranges[R_MAX_RANGES] = {
> +	[R_LED123_CURR]	= { 4000, 1, 6, 4000 },
> +	[R_LED4_CURR]	= { 2000, 1, 3, 2000 },
> +	[R_LED_TRFON]	= { 125, 0, 15, 200 },
> +	[R_LED_TOFF]	= { 250, 0, 15, 400 }
> +};
> +
> +static const struct linear_range mt6372_led_ranges[R_MAX_RANGES] = {
> +	[R_LED123_CURR]	= { 2000, 1, 14, 2000 },
> +	[R_LED4_CURR]	= { 2000, 1, 14, 2000 },
> +	[R_LED_TRFON]	= { 125, 0, 15, 250 },
> +	[R_LED_TOFF]	= { 250, 0, 15, 500 }
> +};
> +
> +static enum mt6370_led_field mt6370_get_led_current_field(unsigned int led_no)
> +{
> +	switch (led_no) {
> +	case MT6370_LED_ISNK1:
> +		return F_LED1_CURR;
> +	case MT6370_LED_ISNK2:
> +		return F_LED2_CURR;
> +	case MT6370_LED_ISNK3:
> +		return F_LED3_CURR;
> +	default:
> +		return F_LED4_CURR;
> +	}
> +}
> +
> +static int mt6370_set_led_brightness(struct mt6370_priv *priv,
> +				     unsigned int led_no, unsigned int level)
> +{
> +	enum mt6370_led_field sel_field;
> +
> +	sel_field = mt6370_get_led_current_field(led_no);
> +
> +	return regmap_field_write(priv->fields[sel_field], level);
> +}
> +
> +static int mt6370_get_led_brightness(struct mt6370_priv *priv,
> +				     unsigned int led_no, unsigned int *level)
> +{
> +	enum mt6370_led_field sel_field;
> +
> +	sel_field = mt6370_get_led_current_field(led_no);
> +
> +	return regmap_field_read(priv->fields[sel_field], level);
> +}
> +
> +static int mt6370_set_led_duty(struct mt6370_priv *priv, unsigned int led_no,
> +			       unsigned int ton, unsigned int toff)
> +{
> +	enum mt6370_led_field sel_field;
> +	unsigned int divisor, ratio;
> +
> +	divisor = priv->is_mt6372 ? MT6372_PMW_DUTY : MT6370_PWM_DUTY;
> +	ratio = ton * divisor / (ton + toff);
> +
> +	switch (led_no) {
> +	case MT6370_LED_ISNK1:
> +		sel_field = F_LED1_DUTY;
> +		break;
> +	case MT6370_LED_ISNK2:
> +		sel_field = F_LED2_DUTY;
> +		break;
> +	case MT6370_LED_ISNK3:
> +		sel_field = F_LED3_DUTY;
> +		break;
> +	default:
> +		sel_field = F_LED4_DUTY;
> +	}
> +
> +	return regmap_field_write(priv->fields[sel_field], ratio);
> +}
> +
> +static const unsigned int common_tfreqs[] = {
> +	10000, 5000, 2000, 1000, 500, 200, 5, 1,
> +};
> +
> +static const unsigned int mt6372_tfreqs[] = {
> +	8000, 4000, 2000, 1000, 500, 250, 8, 4,
> +};
> +
> +static int mt6370_set_led_freq(struct mt6370_priv *priv, unsigned int led_no,
> +			       unsigned int ton, unsigned int toff)
> +{
> +	enum mt6370_led_field sel_field;
> +	const unsigned int *tfreq;
> +	unsigned int tfreq_len, tsum;
> +	int i;
> +

...so, if you have pdata, here you'll do:

	struct mt6370_pdata *pdata = priv->pdata;


> +	if (priv->is_mt6372) {
> +		tfreq = mt6372_tfreqs;
> +		tfreq_len = ARRAY_SIZE(mt6372_tfreqs);
> +	} else {
> +		tfreq = common_tfreqs;
> +		tfreq_len = ARRAY_SIZE(common_tfreqs);
> +	}
> +
> +	tsum = ton + toff;
> +
> +	if (tsum > tfreq[0] || tsum < tfreq[tfreq_len - 1])

	if (tsum > pdata->tfreq[0] || tsum < pdata->tfreq[pdata->tfreq_len - 1])

( etc etc etc )

> +		return -EOPNOTSUPP;
> +
> +	for (i = 0; i < tfreq_len; i++) {
> +		if (tsum >= tfreq[i])
> +			break;
> +	}
> +
> +	switch (led_no) {
> +	case MT6370_LED_ISNK1:
> +		sel_field = F_LED1_FREQ;
> +		break;
> +	case MT6370_LED_ISNK2:
> +		sel_field = F_LED2_FREQ;
> +		break;
> +	case MT6370_LED_ISNK3:
> +		sel_field = F_LED3_FREQ;
> +		break;
> +	default:
> +		sel_field = F_LED4_FREQ;
> +	}
> +
> +	return regmap_field_write(priv->fields[sel_field], i);
> +}
> +
> +static void mt6370_get_breath_reg_base(struct mt6370_priv *priv,
> +				       unsigned int led_no, unsigned int *base)
> +{

	if (pdata->reg_rgb_chrind_tr < 0) {
		*base = pdata->reg_rgb1_tr + led_no * 3;
		return;
	}

> +	if (priv->is_mt6372) {
> +		*base = MT6372_REG_RGB1_TR + led_no * 3;
> +		return;
> +	}
> +
> +	switch (led_no) {
> +	case MT6370_LED_ISNK1:
> +	case MT6370_LED_ISNK2:
> +	case MT6370_LED_ISNK3:
> +		*base = MT6370_REG_RGB1_TR + led_no * 3;
> +		break;
> +	default:
> +		*base = MT6370_REG_RGB_CHRIND_TR;
> +	}
> +}
> +
> +static int mt6370_gen_breath_pattern(struct mt6370_priv *priv,
> +				     struct led_pattern *pattern, u32 len,
> +				     u8 *pattern_val, u32 val_len)
> +{
> +	enum mt6370_led_ranges sel_range;
> +	struct led_pattern *curr;
> +	unsigned int sel;
> +	u8 val[P_MAX_PATTERNS / 2] = {0};
> +	int i;
> +
> +	if (len < P_MAX_PATTERNS && val_len < P_MAX_PATTERNS / 2)
> +		return -EINVAL;
> +
> +	/*
> +	 * Pattern list
> +	 * tr1:	byte 0, b'[7: 4]
> +	 * tr2:	byte 0, b'[3: 0]
> +	 * tf1:	byte 1, b'[7: 4]
> +	 * tf2:	byte 1, b'[3: 0]
> +	 * ton:	byte 2, b'[7: 4]
> +	 * toff: byte 2, b'[3: 0]
> +	 */
> +	for (i = 0; i < P_MAX_PATTERNS; i++) {
> +		curr = pattern + i;
> +
> +		sel_range = i == P_LED_TOFF ? R_LED_TOFF : R_LED_TRFON;
> +
> +		linear_range_get_selector_within(priv->ranges + sel_range,
> +						 curr->delta_t, &sel);
> +
> +		val[i / 2] |= sel << (4 * ((i + 1) % 2));
> +	}
> +
> +	memcpy(pattern_val, val, 3);
> +	return 0;
> +}
> +
> +static int mt6370_set_led_mode(struct mt6370_priv *priv, unsigned int led_no,
> +			       enum mt6370_led_mode mode)
> +{
> +	enum mt6370_led_field sel_field;
> +
> +	switch (led_no) {
> +	case MT6370_LED_ISNK1:
> +		sel_field = F_LED1_MODE;
> +		break;
> +	case MT6370_LED_ISNK2:
> +		sel_field = F_LED2_MODE;
> +		break;
> +	case MT6370_LED_ISNK3:
> +		sel_field = F_LED3_MODE;
> +		break;
> +	default:
> +		sel_field = F_LED4_MODE;
> +	}
> +
> +	return regmap_field_write(priv->fields[sel_field], mode);
> +}
> +
> +static int mt6370_mc_brightness_set(struct led_classdev *lcdev,
> +				    enum led_brightness level)
> +{
> +	struct led_classdev_mc *mccdev = lcdev_to_mccdev(lcdev);
> +	struct mt6370_led *led = container_of(mccdev, struct mt6370_led, mc);
> +	struct mt6370_priv *priv = led->priv;
> +	struct mc_subled *subled;
> +	unsigned int enable, disable;
> +	int i, ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	led_mc_calc_color_components(mccdev, level);
> +
> +	ret = regmap_field_read(priv->fields[F_RGB_EN], &enable);
> +	if (ret)
> +		goto out;
> +
> +	disable = enable;
> +
> +	for (i = 0; i < mccdev->num_colors; i++) {
> +		u32 brightness;
> +
> +		subled = mccdev->subled_info + i;
> +		brightness = min(subled->brightness, lcdev->max_brightness);
> +		disable &= ~MT6370_CHEN_BIT(subled->channel);
> +
> +		if (level == LED_OFF) {
> +			enable &= ~MT6370_CHEN_BIT(subled->channel);
> +
> +			ret = mt6370_set_led_mode(priv, subled->channel,
> +						  MT6370_LED_REG_MODE);
> +			if (ret)
> +				goto out;
> +
> +			continue;
> +		}
> +
> +		if (brightness == LED_OFF) {
> +			enable &= ~MT6370_CHEN_BIT(subled->channel);
> +			continue;
> +		}
> +
> +		enable |= MT6370_CHEN_BIT(subled->channel);
> +
> +		ret = mt6370_set_led_brightness(priv, subled->channel,
> +						brightness);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	ret = regmap_field_write(priv->fields[F_RGB_EN], disable);
> +	if (ret)
> +		goto out;
> +
> +	ret = regmap_field_write(priv->fields[F_RGB_EN], enable);
> +
> +out:
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}
> +
> +static int mt6370_mc_blink_set(struct led_classdev *lcdev,
> +			       unsigned long *delay_on,
> +			       unsigned long *delay_off)
> +{
> +	struct led_classdev_mc *mccdev = lcdev_to_mccdev(lcdev);
> +	struct mt6370_led *led = container_of(mccdev, struct mt6370_led, mc);
> +	struct mt6370_priv *priv = led->priv;
> +	struct mc_subled *subled;
> +	unsigned int enable, disable;
> +	int i, ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	if (!*delay_on && !*delay_off)
> +		*delay_on = *delay_off = 500;
> +
> +	ret = regmap_field_read(priv->fields[F_RGB_EN], &enable);
> +	if (ret)
> +		goto out;
> +
> +	disable = enable;
> +
> +	for (i = 0; i < mccdev->num_colors; i++) {
> +		subled = mccdev->subled_info + i;
> +
> +		disable &= ~MT6370_CHEN_BIT(subled->channel);
> +
> +		ret = mt6370_set_led_duty(priv, subled->channel, *delay_on,
> +					  *delay_off);
> +		if (ret)
> +			goto out;
> +
> +		ret = mt6370_set_led_freq(priv, subled->channel, *delay_on,
> +					  *delay_off);
> +		if (ret)
> +			goto out;
> +
> +		ret = mt6370_set_led_mode(priv, subled->channel,
> +					  MT6370_LED_PWM_MODE);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	/* Toggle to make pattern timing the same */
> +	ret = regmap_field_write(priv->fields[F_RGB_EN], disable);
> +	if (ret)
> +		goto out;
> +
> +	ret = regmap_field_write(priv->fields[F_RGB_EN], enable);
> +
> +out:
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}
> +
> +static int mt6370_mc_pattern_set(struct led_classdev *lcdev,
> +			struct led_pattern *pattern, u32 len, int repeat)
> +{
> +	struct led_classdev_mc *mccdev = lcdev_to_mccdev(lcdev);
> +	struct mt6370_led *led = container_of(mccdev, struct mt6370_led, mc);
> +	struct mt6370_priv *priv = led->priv;
> +	struct mc_subled *subled;
> +	unsigned int reg_base, enable, disable;
> +	u8 params[P_MAX_PATTERNS / 2];
> +	int i, ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	ret = mt6370_gen_breath_pattern(priv, pattern, len, params,
> +					sizeof(params));
> +	if (ret)
> +		goto out;
> +
> +	ret = regmap_field_read(priv->fields[F_RGB_EN], &enable);
> +	if (ret)
> +		goto out;
> +
> +	disable = enable;
> +
> +	for (i = 0; i < mccdev->num_colors; i++) {
> +		subled = mccdev->subled_info + i;
> +
> +		mt6370_get_breath_reg_base(priv, subled->channel, &reg_base);
> +		disable &= ~MT6370_CHEN_BIT(subled->channel);
> +
> +		ret = regmap_raw_write(priv->regmap, reg_base, params,
> +				       sizeof(params));
> +		if (ret)
> +			goto out;
> +
> +		ret = mt6370_set_led_mode(priv, subled->channel,
> +					  MT6370_LED_BREATH_MODE);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	/* Toggle to make pattern timing be the same */
> +	ret = regmap_field_write(priv->fields[F_RGB_EN], disable);
> +	if (ret)
> +		goto out;
> +
> +	ret = regmap_field_write(priv->fields[F_RGB_EN], enable);
> +
> +out:
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}
> +
> +static inline int mt6370_mc_pattern_clear(struct led_classdev *lcdev)
> +{
> +	struct led_classdev_mc *mccdev = lcdev_to_mccdev(lcdev);
> +	struct mt6370_led *led = container_of(mccdev, struct mt6370_led, mc);
> +	struct mt6370_priv *priv = led->priv;
> +	struct mc_subled *subled;
> +	int i, ret = 0;
> +
> +	mutex_lock(&led->priv->lock);
> +
> +	for (i = 0; i < mccdev->num_colors; i++) {
> +		subled = mccdev->subled_info + i;
> +
> +		ret = mt6370_set_led_mode(priv, subled->channel,
> +					  MT6370_LED_REG_MODE);
> +		if (ret)
> +			break;
> +	}
> +
> +	mutex_unlock(&led->priv->lock);
> +
> +	return ret;
> +}
> +
> +static int mt6370_isnk_brightness_set(struct led_classdev *lcdev,
> +				      enum led_brightness level)
> +{
> +	struct mt6370_led *led = container_of(lcdev, struct mt6370_led, isink);
> +	struct mt6370_priv *priv = led->priv;
> +	unsigned int enable;
> +	int ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	ret = regmap_field_read(priv->fields[F_RGB_EN], &enable);
> +	if (ret)
> +		goto out;
> +
> +	if (level == LED_OFF) {
> +		enable &= ~MT6370_CHEN_BIT(led->index);
> +
> +		ret = mt6370_set_led_mode(priv, led->index,
> +					  MT6370_LED_REG_MODE);
> +		if (ret)
> +			goto out;
> +
> +		ret = regmap_field_write(priv->fields[F_RGB_EN], enable);
> +	} else {
> +		enable |= MT6370_CHEN_BIT(led->index);
> +
> +		ret = mt6370_set_led_brightness(priv, led->index, level);
> +		if (ret)
> +			goto out;
> +
> +		ret = regmap_field_write(priv->fields[F_RGB_EN], enable);
> +	}
> +
> +out:
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}
> +
> +static int mt6370_isnk_blink_set(struct led_classdev *lcdev,
> +				 unsigned long *delay_on,
> +				 unsigned long *delay_off)
> +{
> +	struct mt6370_led *led = container_of(lcdev, struct mt6370_led, isink);
> +	struct mt6370_priv *priv = led->priv;
> +	int ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	if (!*delay_on && !*delay_off)
> +		*delay_on = *delay_off = 500;
> +
> +	ret = mt6370_set_led_duty(priv, led->index, *delay_on, *delay_off);
> +	if (ret)
> +		goto out;
> +
> +	ret = mt6370_set_led_freq(priv, led->index, *delay_on, *delay_off);
> +	if (ret)
> +		goto out;
> +
> +	ret = mt6370_set_led_mode(priv, led->index, MT6370_LED_PWM_MODE);
> +
> +out:
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}
> +
> +static int mt6370_isnk_pattern_set(struct led_classdev *lcdev,
> +				   struct led_pattern *pattern, u32 len,
> +				   int repeat)
> +{
> +	struct mt6370_led *led = container_of(lcdev, struct mt6370_led, isink);
> +	struct mt6370_priv *priv = led->priv;
> +	unsigned int reg_base;
> +	u8 params[P_MAX_PATTERNS / 2];
> +	int ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	ret = mt6370_gen_breath_pattern(priv, pattern, len, params,
> +					sizeof(params));
> +	if (ret)
> +		goto out;
> +
> +	mt6370_get_breath_reg_base(priv, led->index, &reg_base);
> +
> +	ret = regmap_raw_write(priv->regmap, reg_base, params, sizeof(params));
> +	if (ret)
> +		goto out;
> +
> +	ret = mt6370_set_led_mode(priv, led->index, MT6370_LED_BREATH_MODE);
> +
> +out:
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}
> +
> +static inline int mt6370_isnk_pattern_clear(struct led_classdev *lcdev)
> +{
> +	struct mt6370_led *led = container_of(lcdev, struct mt6370_led, isink);
> +	struct mt6370_priv *priv = led->priv;
> +	int ret;
> +
> +	mutex_lock(&led->priv->lock);
> +	ret = mt6370_set_led_mode(priv, led->index, MT6370_LED_REG_MODE);
> +	mutex_unlock(&led->priv->lock);
> +
> +	return ret;
> +}
> +
> +static int mt6370_init_led_properties(struct mt6370_led *led,
> +				      struct led_init_data *init_data)
> +{
> +	struct mt6370_priv *priv = led->priv;
> +	struct led_classdev *lcdev;
> +	struct fwnode_handle *child;
> +	enum mt6370_led_ranges sel_range;
> +	u32 max_uA, max_level;
> +	const char * const states[] = { "off", "keep", "on" };
> +	const char *stat_str;
> +	int ret;
> +
> +	if (led->index == MT6370_VIRTUAL_MULTICOLOR) {
> +		struct mc_subled *sub_led;
> +		u32 num_color = 0;
> +
> +		sub_led = devm_kzalloc(priv->dev,
> +				       sizeof(*sub_led) * MC_CHANNEL_NUM,
> +				       GFP_KERNEL);
> +		if (!sub_led)
> +			return -ENOMEM;
> +
> +		fwnode_for_each_child_node(init_data->fwnode, child) {
> +			u32 reg, color;
> +
> +			ret = fwnode_property_read_u32(child, "reg", &reg);
> +			if (ret || reg > MT6370_LED_ISNK3 ||
> +			    priv->leds_active & BIT(reg))
> +				return -EINVAL;
> +
> +			ret = fwnode_property_read_u32(child, "color", &color);
> +			if (ret) {
> +				dev_err(priv->dev,
> +					"led %d, no color specified\n",
> +					led->index);
> +				return ret;
> +			}
> +
> +			priv->leds_active |= BIT(reg);
> +			sub_led[num_color].color_index = color;
> +			sub_led[num_color].channel = reg;
> +			num_color++;
> +		}
> +
> +		if (num_color < 2) {
> +			dev_err(priv->dev,
> +				"Multicolor must include 2 or more led channel\n");
> +			return -EINVAL;
> +		}
> +
> +		led->mc.num_colors = num_color;
> +		led->mc.subled_info = sub_led;
> +
> +		lcdev = &led->mc.led_cdev;
> +		lcdev->brightness_set_blocking = mt6370_mc_brightness_set;
> +		lcdev->blink_set = mt6370_mc_blink_set;
> +		lcdev->pattern_set = mt6370_mc_pattern_set;
> +		lcdev->pattern_clear = mt6370_mc_pattern_clear;
> +	} else {
> +		lcdev = &led->isink;
> +		lcdev->brightness_set_blocking = mt6370_isnk_brightness_set;
> +		lcdev->blink_set = mt6370_isnk_blink_set;
> +		lcdev->pattern_set = mt6370_isnk_pattern_set;
> +		lcdev->pattern_clear = mt6370_isnk_pattern_clear;
> +	}
> +
> +	ret = fwnode_property_read_u32(init_data->fwnode, "led-max-microamp",
> +				       &max_uA);
> +	if (ret) {
> +		dev_warn(priv->dev,
> +		   "Not specified led-max-microamp, config to the minimum\n");
> +		max_uA = 0;
> +	}
> +
> +	if (led->index == MT6370_LED_ISNK4)
> +		sel_range = R_LED4_CURR;
> +	else
> +		sel_range = R_LED123_CURR;
> +
> +	linear_range_get_selector_within(priv->ranges + sel_range, max_uA,
> +					 &max_level);
> +
> +	lcdev->max_brightness = max_level;
> +
> +	fwnode_property_read_string(init_data->fwnode, "linux,default-trigger",
> +			&lcdev->default_trigger);
> +
> +	if (!fwnode_property_read_string(init_data->fwnode, "default-state",
> +					 &stat_str)) {
> +		ret = match_string(states, ARRAY_SIZE(states), stat_str);
> +		if (ret < 0)
> +			ret = STATE_OFF;
> +
> +		led->default_state = ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mt6370_isnk_init_default_state(struct mt6370_led *led)
> +{
> +	struct mt6370_priv *priv = led->priv;
> +	unsigned int enable, level;
> +	int ret;
> +
> +	ret = mt6370_get_led_brightness(priv, led->index, &level);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_field_read(priv->fields[F_RGB_EN], &enable);
> +	if (ret)
> +		return ret;
> +
> +	if (!(enable & MT6370_CHEN_BIT(led->index)))
> +		level = LED_OFF;
> +
> +	switch (led->default_state) {
> +	case STATE_ON:
> +		led->isink.brightness = led->isink.max_brightness;
> +		break;
> +	case STATE_KEEP:
> +		led->isink.brightness = min(level, led->isink.max_brightness);
> +		break;
> +	default:
> +		led->isink.brightness = LED_OFF;
> +	}
> +
> +	return mt6370_isnk_brightness_set(&led->isink, led->isink.brightness);
> +}
> +
> +static int mt6370_led_register(struct device *parent, struct mt6370_led *led,
> +			       struct led_init_data *init_data)
> +{
> +	struct mt6370_priv *priv = led->priv;
> +	int ret;
> +
> +	if (led->index == MT6370_VIRTUAL_MULTICOLOR) {
> +		ret = mt6370_mc_brightness_set(&led->mc.led_cdev, LED_OFF);
> +		if (ret) {
> +			dev_err(parent, "Couldn't set multicolor brightness\n");
> +			return ret;
> +		}
> +
> +		ret = devm_led_classdev_multicolor_register_ext(parent,
> +								&led->mc,
> +								init_data);
> +		if (ret) {
> +			dev_err(parent, "Couldn't register multicolor\n");
> +			return ret;
> +		}
> +	} else {
> +		if (led->index == MT6370_LED_ISNK4) {
> +			ret = regmap_field_write(priv->fields[F_CHGIND_EN], 1);
> +			if (ret) {
> +				dev_err(parent, "Failed to set CHRIND to SW\n");
> +				return ret;
> +			}
> +		}
> +
> +		ret = mt6370_isnk_init_default_state(led);
> +		if (ret) {
> +			dev_err(parent, "Failed to init %d isnk state\n",
> +				led->index);
> +			return ret;
> +		}
> +
> +		ret = devm_led_classdev_register_ext(parent, &led->isink,
> +						     init_data);
> +		if (ret) {
> +			dev_err(parent, "Couldn't register isink %d\n",
> +				led->index);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int mt6370_check_vendor_info(struct mt6370_priv *priv)
> +{
> +	unsigned int devinfo, vid;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, MT6370_REG_DEV_INFO, &devinfo);
> +	if (ret)
> +		return ret;
> +
> +	vid = FIELD_GET(MT6370_VENID_MASK, devinfo);
> +	if (vid == 0x9 || vid == 0xb) {
> +		priv->reg_fields = mt6372_reg_fields;
> +		priv->ranges = mt6372_led_ranges;
> +		priv->is_mt6372 = true;

		priv->pdata = &mt6372_pdata;

> +	} else {
> +		priv->reg_fields = common_reg_fields;
> +		priv->ranges = common_led_ranges;

		priv->pdata = &mt6370_pdata;

> +	}
> +
> +	return 0;
> +}

Regards,
Angelo
Andy Shevchenko July 15, 2022, 1:05 p.m. UTC | #3
On Fri, Jul 15, 2022 at 2:40 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
> Il 15/07/22 13:26, ChiaEn Wu ha scritto:

...

> > +#define STATE_OFF                            0
> > +#define STATE_KEEP                           1
> > +#define STATE_ON                             2
>
> I propose, instead:
>
> enum mt6370_state {
>         STATE_OFF = 0,
>         STATE_KEEP,
>         STATE_ON,

>         STATE_MAX,

Usually we don't put commas at the terminator entries.

> };
Andy Shevchenko July 15, 2022, 1:18 p.m. UTC | #4
On Fri, Jul 15, 2022 at 1:28 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote:
>
> From: ChiaEn Wu <chiaen_wu@richtek.com>
>
> MediaTek MT6370 is a SubPMIC consisting of a single cell battery charger
> with ADC monitoring, RGB LEDs, dual channel flashlight, WLED backlight
> driver, display bias voltage supply, one general purpose LDO, and the
> USB Type-C & PD controller complies with the latest USB Type-C and PD
> standards.
>
> This adds support the MT6370 ADC driver for system monitoring, including

This adds --> Add a

> charger current, voltage, and temperature.

...

> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/iio/iio.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/sysfs.h>

...

> +#define MT6370_AICR_400MA              0x6
> +#define MT6370_ICHG_500MA              0x4
> +#define MT6370_ICHG_900MA              0x8

_mA ?

...

> +       ret = regmap_read_poll_timeout(priv->regmap,
> +                                      MT6370_REG_CHG_ADC, reg_val,
> +                                      !(reg_val & MT6370_ADC_START_MASK),
> +                                      ADC_CONV_POLLING_TIME_US,
> +                                      ADC_CONV_TIME_MS * 1000 * 3);

1000 --> MILLI ?

...

> +static int mt6370_adc_probe(struct platform_device *pdev)
> +{

Given comment in one place in the entire series would be good to use
in another where appropriate. For example, here it would also be nice
to have a temporary variable

  struct device *dev = &pdev->dev;

It will shorten some lines.

> +       struct mt6370_adc_data *priv;
> +       struct iio_dev *indio_dev;
> +       struct regmap *regmap;
> +       int ret;

> +}
Daniel Thompson July 15, 2022, 4:29 p.m. UTC | #5
On Fri, Jul 15, 2022 at 02:38:45PM +0200, AngeloGioacchino Del Regno wrote:
> Il 15/07/22 13:26, ChiaEn Wu ha scritto:
> > From: ChiaEn Wu <chiaen_wu@richtek.com>
> >
> > MediaTek MT6370 is a SubPMIC consisting of a single cell battery charger
> > with ADC monitoring, RGB LEDs, dual channel flashlight, WLED backlight
> > driver, display bias voltage supply, one general purpose LDO, and the
> > USB Type-C & PD controller complies with the latest USB Type-C and PD
> > standards.
> >
> > This adds support for MediaTek MT6370 Backlight driver. It's commonly used
> > to drive the display WLED. There are 4 channels inside, and each channel
> > supports up to 30mA of current capability with 2048 current steps in
> > exponential or linear mapping curves.
> >
> > Signed-off-by: ChiaEn Wu <chiaen_wu@richtek.com>
>
> Hello ChiaEn,
>
> I propose to move this one to drivers/leds (or drivers/pwm) and, instead of
> registering a backlight device, register a PWM device.
>
> This way you will be able to reuse the generic backlight-pwm driver, as you'd
> be feeding the PWM device exposed by this driver to the generic one: this will
> most importantly make it easy to chain it with MTK_DISP_PWM (mtk-pwm-disp)
> with a devicetree that looks like...

Out of interest, does MT6370 have the same structure for backlights as the prior
systems using mtk-pwm-disp or was mtk-pwm-disp simply a normal(-ish) PWM
that relied on something on the board for all the constant current
driver hardware?


>
> 	pwmleds-disp {
> 		compatible = "pwm-leds";
>
> 		disp_led: disp-pwm {
> 			label = "backlight-pwm";
> 			pwms = <&pwm0 0 500000>;
> 			max-brightness = <1024>;
> 		};
> 	};
>
> 	backlight_lcd0: backlight {
> 		compatible = "led-backlight";
> 		leds = <&disp_led>, <&pmic_bl_led>;
> 		default-brightness-level = <300>;
> 	};

I think this proposal has to start with the devicetree bindings rather
than the driver. Instead I think the question is: does this proposal
result in DT bindings that better describe the underlying hardware?

This device has lots of backlight centric features (OCP, OVP, single
control with multiple outputs, exponential curves, etc) and its not
clear where they would fit into the "PWM" bindings.

Come to think of it I'm also a little worried also about the whole linear
versus exponential curve thing since I thought LED drivers were required
to use exponential curves.


Daniel.
AngeloGioacchino Del Regno July 18, 2022, 8:27 a.m. UTC | #6
Il 15/07/22 18:29, Daniel Thompson ha scritto:
> On Fri, Jul 15, 2022 at 02:38:45PM +0200, AngeloGioacchino Del Regno wrote:
>> Il 15/07/22 13:26, ChiaEn Wu ha scritto:
>>> From: ChiaEn Wu <chiaen_wu@richtek.com>
>>>
>>> MediaTek MT6370 is a SubPMIC consisting of a single cell battery charger
>>> with ADC monitoring, RGB LEDs, dual channel flashlight, WLED backlight
>>> driver, display bias voltage supply, one general purpose LDO, and the
>>> USB Type-C & PD controller complies with the latest USB Type-C and PD
>>> standards.
>>>
>>> This adds support for MediaTek MT6370 Backlight driver. It's commonly used
>>> to drive the display WLED. There are 4 channels inside, and each channel
>>> supports up to 30mA of current capability with 2048 current steps in
>>> exponential or linear mapping curves.
>>>
>>> Signed-off-by: ChiaEn Wu <chiaen_wu@richtek.com>
>>
>> Hello ChiaEn,
>>
>> I propose to move this one to drivers/leds (or drivers/pwm) and, instead of
>> registering a backlight device, register a PWM device.
>>
>> This way you will be able to reuse the generic backlight-pwm driver, as you'd
>> be feeding the PWM device exposed by this driver to the generic one: this will
>> most importantly make it easy to chain it with MTK_DISP_PWM (mtk-pwm-disp)
>> with a devicetree that looks like...
> 
> Out of interest, does MT6370 have the same structure for backlights as the prior
> systems using mtk-pwm-disp or was mtk-pwm-disp simply a normal(-ish) PWM
> that relied on something on the board for all the constant current
> driver hardware?
> 
> 

As per my understanding, mtk-pwm-disp is chained to other multimedia features of
the display block of MediaTek SoCs, such as the AAL (adaptive ambient light),
CABC (content adaptive backlight control) etc, other than being a normal(ish)
PWM... that's the reason of my request.

Moreover, in the end, this PMIC's backlight controller is just a "fancy" PWM
controller, with OCP/OVP.

>>
>> 	pwmleds-disp {
>> 		compatible = "pwm-leds";
>>
>> 		disp_led: disp-pwm {
>> 			label = "backlight-pwm";
>> 			pwms = <&pwm0 0 500000>;
>> 			max-brightness = <1024>;
>> 		};
>> 	};
>>
>> 	backlight_lcd0: backlight {
>> 		compatible = "led-backlight";
>> 		leds = <&disp_led>, <&pmic_bl_led>;
>> 		default-brightness-level = <300>;
>> 	};
> 
> I think this proposal has to start with the devicetree bindings rather
> than the driver. Instead I think the question is: does this proposal
> result in DT bindings that better describe the underlying hardware?
> 

 From how I understand it - yes: we have a fancy PWM (&pwm0) that we use
to control display backlight (backlight-pwm)...

Obviously, here we're not talking about OLEDs, but LCDs, where the backlight
is made of multiple strings of WhiteLED (effectively, a "pwm-leds" controlled
"led-backlight").

Using PWM will also allow for a little more fine-grained board specific
configuration, as I think that this PMIC (and/or variants of it) will be
used in completely different form factors: I think that's going to be both
smartphones and tablets/laptops... and I want to avoid vendor properties
to configure the PWM part in a somehow different way.

> This device has lots of backlight centric features (OCP, OVP, single
> control with multiple outputs, exponential curves, etc) and its not
> clear where they would fit into the "PWM" bindings.
> 

For OCP and OVP, the only bindings that fit would be regulators, but that's
not a regulator... and that's about it - I don't really have arguments for
that.

What I really want to see here is usage of "generic" drivers like led_bl
and/or pwm_bl as to get some "standardization" around with all the benefits
that this carries.

> Come to think of it I'm also a little worried also about the whole linear
> versus exponential curve thing since I thought LED drivers were required
> to use exponential curves.
> 

That probably depends on how the controller interprets the data, I guess,
but I agree with you on this thought.

Regards,
Angelo
ChiaEn Wu July 18, 2022, 11:17 a.m. UTC | #7
On Mon, Jul 18, 2022 at 4:27 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>

<snip>

> >>
> >> Hello ChiaEn,
> >>
> >> I propose to move this one to drivers/leds (or drivers/pwm) and, instead of
> >> registering a backlight device, register a PWM device.
> >>
> >> This way you will be able to reuse the generic backlight-pwm driver, as you'd
> >> be feeding the PWM device exposed by this driver to the generic one: this will
> >> most importantly make it easy to chain it with MTK_DISP_PWM (mtk-pwm-disp)
> >> with a devicetree that looks like...
> >
> > Out of interest, does MT6370 have the same structure for backlights as the prior
> > systems using mtk-pwm-disp or was mtk-pwm-disp simply a normal(-ish) PWM
> > that relied on something on the board for all the constant current
> > driver hardware?
> >
> >
>
> As per my understanding, mtk-pwm-disp is chained to other multimedia features of
> the display block of MediaTek SoCs, such as the AAL (adaptive ambient light),
> CABC (content adaptive backlight control) etc, other than being a normal(ish)
> PWM... that's the reason of my request.
>
> Moreover, in the end, this PMIC's backlight controller is just a "fancy" PWM
> controller, with OCP/OVP.
>
> >>
> >>      pwmleds-disp {
> >>              compatible = "pwm-leds";
> >>
> >>              disp_led: disp-pwm {
> >>                      label = "backlight-pwm";
> >>                      pwms = <&pwm0 0 500000>;
> >>                      max-brightness = <1024>;
> >>              };
> >>      };
> >>
> >>      backlight_lcd0: backlight {
> >>              compatible = "led-backlight";
> >>              leds = <&disp_led>, <&pmic_bl_led>;
> >>              default-brightness-level = <300>;
> >>      };
> >
> > I think this proposal has to start with the devicetree bindings rather
> > than the driver. Instead I think the question is: does this proposal
> > result in DT bindings that better describe the underlying hardware?
> >
>
>  From how I understand it - yes: we have a fancy PWM (&pwm0) that we use
> to control display backlight (backlight-pwm)...
>
> Obviously, here we're not talking about OLEDs, but LCDs, where the backlight
> is made of multiple strings of WhiteLED (effectively, a "pwm-leds" controlled
> "led-backlight").
>
> Using PWM will also allow for a little more fine-grained board specific
> configuration, as I think that this PMIC (and/or variants of it) will be
> used in completely different form factors: I think that's going to be both
> smartphones and tablets/laptops... and I want to avoid vendor properties
> to configure the PWM part in a somehow different way.
>
> > This device has lots of backlight centric features (OCP, OVP, single
> > control with multiple outputs, exponential curves, etc) and its not
> > clear where they would fit into the "PWM" bindings.
> >
>
> For OCP and OVP, the only bindings that fit would be regulators, but that's
> not a regulator... and that's about it - I don't really have arguments for
> that.
>
> What I really want to see here is usage of "generic" drivers like led_bl
> and/or pwm_bl as to get some "standardization" around with all the benefits
> that this carries.
>
> > Come to think of it I'm also a little worried also about the whole linear
> > versus exponential curve thing since I thought LED drivers were required
> > to use exponential curves.
> >
>
> That probably depends on how the controller interprets the data, I guess,
> but I agree with you on this thought.

Hi Angelo,

MT6370 is just a SubPMIC, not an SoC, and is applied in cellular
telephones, tablet PCs, and portable instruments.
And the PWM mode of the MT6370 backlight driver is optional, and not
must be enabled.