On Tue, 07 Mar 2023, ChiYuan Huang wrote:
> Hi, Lee:
>
> Thanks's for the reviewing.
> To prevent the misunderstanding, reply as below.
> No reply means will do.
>
> On Sun, Mar 05, 2023 at 08:55:33AM +0000, Lee Jones wrote:
> > On Thu, 23 Feb 2023, ChiaEn Wu wrote:
> >
> > > From: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> > >
> > > 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.
> > >
> > > Add support for the MediaTek MT6370 Current Sink Type LED Indicator
> > > driver. It can control four channels current-sink RGB LEDs with 3 modes:
> > > constant current, PWM, and breath mode.
> > >
> > > Acked-by: Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx>
> > > Co-developed-by: Alice Chen <alice_chen@xxxxxxxxxxx>
> > > Signed-off-by: Alice Chen <alice_chen@xxxxxxxxxxx>
> > > Signed-off-by: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> > > Signed-off-by: ChiaEn Wu <chiaen_wu@xxxxxxxxxxx>
> > > ---
> > > v17
> > > - Update the year of Copyright from 2022 to 2023
> > >
> > > ---
> > > drivers/leds/rgb/Kconfig | 13 +
> > > drivers/leds/rgb/Makefile | 1 +
> > > drivers/leds/rgb/leds-mt6370-rgb.c | 1009 ++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 1023 insertions(+)
> > > create mode 100644 drivers/leds/rgb/leds-mt6370-rgb.c
[...]
> > > +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;
> > > + u32 val = 0;
> > > + int i;
> > > +
> > > + if (len < P_MAX_PATTERNS && val_len < P_MAX_PATTERNS / 2)
> > > + return -EINVAL;
> > > +
> > > + /*
> > > + * Pattern list
> > > + * tr1: byte 0, b'[7: 4]
> >
> > Perhaps this is standard formatting and I'm just not aware of it, but
> > the space is throwing me and making me think twice. Does this mean bits
> > 7 through 4, so b'11110000?
> >
> Yes. like as you said. Sorry for the redudant space make you confused.
> I'm not sure whether it's a standard formating or not.
> Sometimes, in datasheet, we'll use this format to represent the bitfield for functions.
> Your format is also good for it.
> And which one is preferable?
Since neither of us are sure, just take the space out for now please.
> > > + * 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);
> > > +
> > > + if (i % 2) {
> > > + val |= sel;
> > > + } else {
> > > + val <<= 8;
> > > + val |= sel << 4;
> > > + }
> > > + }
[...]
> > > +static int mt6370_led_register(struct device *dev, struct mt6370_led *led,
> > > + struct led_init_data *init_data)
> > > +{
> > > + struct mt6370_priv *priv = led->priv;
> > > + int ret;
> > > +
> > > + if (led->index == MT6370_VIRTUAL_MULTICOLOR) {
> >
> > This too could be split into separate functions to tidy things up a
> > little.
> >
> Like as below?
>
> if (led->index == MT6370_VIRTUAL_MULTICOLOR)
> return mt6370_multicolor_led_register(....)
>
> if (led->index == MT6370_LED_ISNK4) {
> .....
> }
>
> ret = mt6370_init_isnk_default_state(...)
Yes, that kind of thing.
> Since the multilor case directly return from the sub-function, else can be removed.
> > > + ret = mt6370_mc_brightness_set(&led->mc.led_cdev, 0);
> > > + if (ret)
> > > + return dev_err_probe(dev, ret, "Couldn't set multicolor brightness\n");
> > > +
> > > + ret = devm_led_classdev_multicolor_register_ext(dev, &led->mc,
> > > + init_data);
> > > + if (ret)
> > > + return dev_err_probe(dev, ret, "Couldn't register multicolor\n");
> > > + } else {
> > > + if (led->index == MT6370_LED_ISNK4) {
> > > + ret = regmap_field_write(priv->fields[F_CHGIND_EN], 1);
> > > + if (ret)
> > > + return dev_err_probe(dev, ret, "Failed to set CHRIND to SW\n");
> > > + }
> > > +
> > > + ret = mt6370_isnk_init_default_state(led);
> > > + if (ret)
> > > + return dev_err_probe(dev, ret, "Failed to init %d isnk state\n",
> > > + led->index);
> > > +
> > > + ret = devm_led_classdev_register_ext(dev, &led->isink,
> > > + init_data);
> > > + if (ret)
> > > + return dev_err_probe(dev, ret, "Couldn't register isink %d\n", led->index);
> > > + }
> > > +
> > > + 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) {
> >
> > Are there nice human readable associates of these (vendor?) IDS?
> >
> For clearly understanding, I'll define these two as 'MT6372_VENDOR_ID' and 'MT6372C_VENDOR_ID'.
Much better, thank you.
--
Lee Jones [李琼斯]
