mbox series

[00/14] Add Mediatek MT6370 PMIC support

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

Message

ChiaEn Wu May 31, 2022, 10:27 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.

Alice Chen (3):
  leds: mt6370: Add Mediatek MT6370 Indicator support
  leds: flashlight: mt6370: Add Mediatek MT6370 flashlight support
  dt-bindings: leds: Add Mediatek MT6370 flashlight binding
    documentation

ChiYuan Huang (7):
  mfd: mt6370: Add Mediatek MT6370 support
  usb: typec: tcpci_mt6370: Add Mediatek MT6370 tcpci driver
  regulator: mt6370: Add mt6370 DisplayBias and VibLDO support
  dt-bindings: usb: Add Mediatek MT6370 TCPC binding documentation
  dt-bindings: leds: mt6370: Add Mediatek mt6370 indicator documentation
  dt-bindings: backlight: Add Mediatek MT6370 backlight binding
    documentation
  dt-bindings: mfd: Add Mediatek MT6370 binding documentation

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

 .../backlight/mediatek,mt6370-backlight.yaml  |  110 ++
 .../leds/mediatek,mt6370-flashlight.yaml      |   48 +
 .../leds/mediatek,mt6370-indicator.yaml       |   57 +
 .../bindings/mfd/mediatek,mt6370.yaml         |  282 ++++
 .../power/supply/mediatek,mt6370-charger.yaml |   60 +
 .../bindings/usb/mediatek,mt6370-tcpc.yaml    |   35 +
 drivers/iio/adc/Kconfig                       |    9 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/mt6370-adc.c                  |  257 ++++
 drivers/leds/Kconfig                          |   11 +
 drivers/leds/Makefile                         |    1 +
 drivers/leds/flash/Kconfig                    |    9 +
 drivers/leds/flash/Makefile                   |    1 +
 drivers/leds/flash/leds-mt6370-flash.c        |  665 ++++++++++
 drivers/leds/leds-mt6370.c                    |  994 +++++++++++++++
 drivers/mfd/Kconfig                           |   13 +
 drivers/mfd/Makefile                          |    1 +
 drivers/mfd/mt6370.c                          |  273 ++++
 drivers/power/supply/Kconfig                  |   11 +
 drivers/power/supply/Makefile                 |    1 +
 drivers/power/supply/mt6370-charger.c         | 1132 +++++++++++++++++
 drivers/regulator/Kconfig                     |    8 +
 drivers/regulator/Makefile                    |    1 +
 drivers/regulator/mt6370-regulator.c          |  389 ++++++
 drivers/usb/typec/tcpm/Kconfig                |    8 +
 drivers/usb/typec/tcpm/Makefile               |    1 +
 drivers/usb/typec/tcpm/tcpci_mt6370.c         |  212 +++
 drivers/video/backlight/Kconfig               |    8 +
 drivers/video/backlight/Makefile              |    1 +
 drivers/video/backlight/mt6370-backlight.c    |  338 +++++
 .../dt-bindings/iio/adc/mediatek,mt6370_adc.h |   18 +
 include/dt-bindings/mfd/mediatek,mt6370.h     |   83 ++
 32 files changed, 5038 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/power/supply/mt6370-charger.c
 create mode 100644 drivers/regulator/mt6370-regulator.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
 create mode 100644 include/dt-bindings/mfd/mediatek,mt6370.h

Comments

Krzysztof Kozlowski June 1, 2022, 7:34 a.m. UTC | #1
On 31/05/2022 12:28, ChiaEn Wu wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Add mt6370 backlight binding documentation.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
>  .../backlight/mediatek,mt6370-backlight.yaml  | 110 ++++++++++++++++++
>  1 file changed, 110 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/backlight/mediatek,mt6370-backlight.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/mediatek,mt6370-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/mediatek,mt6370-backlight.yaml
> new file mode 100644
> index 000000000000..81d72ed44be4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/backlight/mediatek,mt6370-backlight.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/backlight/mediatek,mt6370-backlight.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek MT6370 Backlight
> +
> +maintainers:
> +  - ChiaEn Wu <chiaen_wu@richtek.com>
> +
> +description: |
> +  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.

Do not repeat entire device description, but describe only this part.
Your other pieces of MFD were doing it correctly, so why here it is
different?

> +
> +  For the LCD backlight, it can provide 4 channel WLED driving capability.
> +  Each channel driving current is up to 30mA
> +
> +allOf:
> +  - $ref: common.yaml#
> +
> +properties:
> +  compatible:
> +    const: mediatek,mt6370-backlight
> +
> +  default-brightness:
> +    minimum: 0
> +    maximum: 2048
> +
> +  max-brightness:
> +    minimum: 0
> +    maximum: 2048
> +
> +  enable-gpios:
> +    description: External backlight 'enable' pin
> +    maxItems: 1
> +
> +  mediatek,bled-pwm-enable:
> +    description: |
> +      Enable external PWM input for backlight dimming
> +    type: boolean
> +
> +  mediatek,bled-pwm-hys-enable:
> +    description: |
> +      Enable the backlight input-hysteresis for PWM mode
> +    type: boolean
> +
> +  mediatek,bled-pwm-hys-sel:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    enum: [0, 1, 2, 3]
> +    description: |
> +      Backlight PWM hysteresis input level selection.
> +      value mapping:
> +        - 0: 1bit
> +        - 1: 2bit
> +        - 2: 4bit
> +        - 3: 6bit

Please explain what is this input level and what is the meaning of
6bit... In any case you cannot have values mapping, but instead you
should use the logical values (so 1, 2, 4, and 6).

If "sel" is shortcut of "selection" please drop it.

> +
> +  mediatek,bled-ovp-shutdown:
> +    description: |
> +      Enable the backlight shutdown when OVP level triggered
> +    type: boolean
> +
> +  mediatek,bled-ovp-level-sel:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    enum: [0, 1, 2, 3]
> +    description: |
> +      Backlight OVP level selection.
> +      value mapping:
> +        - 0: 17V
> +        - 1: 21V
> +        - 2: 25V
> +        - 3: 29V

Logical values in microvolts. Name it according to unit suffices and
drop any useless parts of property name like "level selection". It is
simply - mediatek,bled-ovp-microvolt.

> +
> +  mediatek,bled-ocp-shutdown:
> +    description: |
> +      Enable the backlight shutdown when OCP level triggerred.
> +    type: boolean
> +
> +  mediatek,bled-ocp-level-sel:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    enum: [0, 1, 2, 3]
> +    description: |
> +      Backlight OC level selection.
> +      value mapping:
> +        - 0: 900mA
> +        - 1: 1200mA
> +        - 2: 1500mA
> +        - 3: 1800mA

Same comments.

> +
> +  mediatek,bled-channel-use:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    description: |
> +      Backlight LED channel to be used.
> +      Each bit mapping to:
> +        - 0: CH4
> +        - 1: CH3
> +        - 2: CH2
> +        - 3: CH1> +    minimum: 1
> +    maximum: 15
> +
> +required:
> +  - compatible
> +  - mediatek,bled-channel-use
> +
> +additionalProperties: false


Best regards,
Krzysztof
Andy Shevchenko June 1, 2022, 9:48 a.m. UTC | #2
On Tue, May 31, 2022 at 1:16 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote:
>
> From: Alice Chen <alice_chen@richtek.com>

All below comments are applicable to the rest of the series as well
(one way or another), so please fix all your patches where it's
appropriate.

>
> Add Mediatek MT6370 Indicator support

What indicator?
Please also keep attention on English punctuation (missed period).

...

> +       help
> +         Support 4 channels and reg/pwm/breath mode.
> +         Isink4 can also use as a CHG_VIN power good Indicator.

be used

> +         Say Y here to enable support for
> +         MT6370_RGB_LED device.

...

> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>

> +#include <linux/of.h>

Are you sure this is the correct header? Seems you need
mod_devicetable.h instead.

> +#include <linux/property.h>
> +#include <linux/regmap.h>

...

> +struct mt6370_priv {
> +       struct mutex lock;

Do you use regmap locking?

> +       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;
> +       struct mt6370_led leds[];
> +};

...

> +static const unsigned int common_tfreqs[] = {
> +       10000, 5000, 2000, 1000, 500, 200, 5, 1

Leave a comma at the end.

> +};
> +
> +static const unsigned int mt6372_tfreqs[] = {
> +       8000, 4000, 2000, 1000, 500, 250, 8, 4

Ditto.

> +};
Andy Shevchenko June 1, 2022, 9:51 a.m. UTC | #3
On Wed, Jun 1, 2022 at 11:48 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, May 31, 2022 at 1:16 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote:
> >
> > From: Alice Chen <alice_chen@richtek.com>
>
> All below comments are applicable to the rest of the series as well
> (one way or another), so please fix all your patches where it's
> appropriate.

Forgot to mention, please consider using

  return dev_err_probe();

pattern in the ->probe() and related funcitons. It will save a lot of LOCs.

> > Add Mediatek MT6370 Indicator support
>
> What indicator?
> Please also keep attention on English punctuation (missed period).
>
> ...
>
> > +       help
> > +         Support 4 channels and reg/pwm/breath mode.
> > +         Isink4 can also use as a CHG_VIN power good Indicator.
>
> be used
>
> > +         Say Y here to enable support for
> > +         MT6370_RGB_LED device.
>
> ...
>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
>
> > +#include <linux/of.h>
>
> Are you sure this is the correct header? Seems you need
> mod_devicetable.h instead.
>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
>
> ...
>
> > +struct mt6370_priv {
> > +       struct mutex lock;
>
> Do you use regmap locking?
>
> > +       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;
> > +       struct mt6370_led leds[];
> > +};
>
> ...
>
> > +static const unsigned int common_tfreqs[] = {
> > +       10000, 5000, 2000, 1000, 500, 200, 5, 1
>
> Leave a comma at the end.
>
> > +};
> > +
> > +static const unsigned int mt6372_tfreqs[] = {
> > +       8000, 4000, 2000, 1000, 500, 250, 8, 4
>
> Ditto.
>
> > +};
>
>
> --
> With Best Regards,
> Andy Shevchenko
cy_huang June 2, 2022, 6:26 a.m. UTC | #4
On Wed, Jun 01, 2022 at 11:48:58AM +0200, Andy Shevchenko wrote:
> On Tue, May 31, 2022 at 1:16 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote:
> >
> > From: Alice Chen <alice_chen@richtek.com>
> 
> All below comments are applicable to the rest of the series as well
> (one way or another), so please fix all your patches where it's
> appropriate.
> 
> >
> > Add Mediatek MT6370 Indicator support
> 
> What indicator?
It's RGB curent sink type LED driver (maximum supported current is only 24mA).
> Please also keep attention on English punctuation (missed period).
> 
Ack in next.
> ...
>
> > +       help
> > +         Support 4 channels and reg/pwm/breath mode.
> > +         Isink4 can also use as a CHG_VIN power good Indicator.
> 
> be used
> 
Ack in next.
> > +         Say Y here to enable support for
> > +         MT6370_RGB_LED device.
> 
> ...
> 
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
> 
> > +#include <linux/of.h>
> 
> Are you sure this is the correct header? Seems you need
> mod_devicetable.h instead.
> 
It's the correct header and be used for the struct 'of_device_id'.
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> 
> ...
> 
> > +struct mt6370_priv {
> > +       struct mutex lock;
> 
> Do you use regmap locking?
>
MFD regmap register already the access lock.

This lock is just to guarantee only one user can access the RGB register
part.

Sorry, from the comment, do you want us to rename or remove this 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;
> > +       struct mt6370_led leds[];
> > +};
> 
> ...
> 
> > +static const unsigned int common_tfreqs[] = {
> > +       10000, 5000, 2000, 1000, 500, 200, 5, 1
> 
> Leave a comma at the end.
> 
Ack in next.
> > +};
> > +
> > +static const unsigned int mt6372_tfreqs[] = {
> > +       8000, 4000, 2000, 1000, 500, 250, 8, 4
> 
> Ditto.
> 
Ack in next.
> > +};
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
Andy Shevchenko June 2, 2022, 9:17 a.m. UTC | #5
On Thu, Jun 2, 2022 at 8:27 AM ChiYuan Huang <u0084500@gmail.com> wrote:
> On Wed, Jun 01, 2022 at 11:48:58AM +0200, Andy Shevchenko wrote:
> > On Tue, May 31, 2022 at 1:16 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote:

...

> > What indicator?
> It's RGB curent sink type LED driver (maximum supported current is only 24mA).

Make your commit messages a slightly more verbose.

...

> > > +#include <linux/of.h>
> >
> > Are you sure this is the correct header? Seems you need
> > mod_devicetable.h instead.
> >
> It's the correct header and be used for the struct 'of_device_id'.

Nope. Run the following command
$ git grep -n 'struct of_device_id {' -- include/linux/

...

> > > +struct mt6370_priv {
> > > +       struct mutex lock;
> >
> > Do you use regmap locking?
> >
> MFD regmap register already the access lock.
>
> This lock is just to guarantee only one user can access the RGB register
> part.
>
> Sorry, from the comment, do you want us to rename or remove this lock?

My point is, since you have two locks, explain why you need each of them.

> > > +       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;
> > > +       struct mt6370_led leds[];
> > > +};
cy_huang June 2, 2022, 9:35 a.m. UTC | #6
Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年6月2日 週四 下午5:18寫道:
>
> On Thu, Jun 2, 2022 at 8:27 AM ChiYuan Huang <u0084500@gmail.com> wrote:
> > On Wed, Jun 01, 2022 at 11:48:58AM +0200, Andy Shevchenko wrote:
> > > On Tue, May 31, 2022 at 1:16 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote:
>
> ...
>
> > > What indicator?
> > It's RGB curent sink type LED driver (maximum supported current is only 24mA).
>
> Make your commit messages a slightly more verbose.
>
OK, will refine the commit message in next.
> ...
>
> > > > +#include <linux/of.h>
> > >
> > > Are you sure this is the correct header? Seems you need
> > > mod_devicetable.h instead.
> > >
> > It's the correct header and be used for the struct 'of_device_id'.
>
> Nope. Run the following command
> $ git grep -n 'struct of_device_id {' -- include/linux/
>
Got it, thanks.
> ...
>
> > > > +struct mt6370_priv {
> > > > +       struct mutex lock;
> > >
> > > Do you use regmap locking?
> > >
> > MFD regmap register already the access lock.
> >
> > This lock is just to guarantee only one user can access the RGB register
> > part.
> >
> > Sorry, from the comment, do you want us to rename or remove this lock?
>
> My point is, since you have two locks, explain why you need each of them.
>
OK, will leave a comment line to explain the usage of this 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;
> > > > +       struct mt6370_led leds[];
> > > > +};
>
>
> --
> With Best Regards,
> Andy Shevchenko