| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230308140252.GM9667@google.com>
Date: Wed, 8 Mar 2023 14:02:52 +0000
From: Lee Jones <lee@...nel.org>
To: ChiYuan Huang <cy_huang@...htek.com>
Cc: ChiaEn Wu <chiaen_wu@...htek.com>, corbet@....net, pavel@....cz,
matthias.bgg@...il.com, andriy.shevchenko@...ux.intel.com,
jacek.anaszewski@...il.com,
angelogioacchino.delregno@...labora.com, linux-doc@...r.kernel.org,
peterwu.pub@...il.com, linux-leds@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
szunichen@...il.com, Alice Chen <alice_chen@...htek.com>
Subject: Re: [PATCH v17 RESEND 1/3] leds: rgb: mt6370: Add MediaTek MT6370
current sink type LED Indicator support
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@...htek.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.
> > >
> > > 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@...il.com>
> > > Co-developed-by: Alice Chen <alice_chen@...htek.com>
> > > Signed-off-by: Alice Chen <alice_chen@...htek.com>
> > > Signed-off-by: ChiYuan Huang <cy_huang@...htek.com>
> > > Signed-off-by: ChiaEn Wu <chiaen_wu@...htek.com>
> > > ---
> > > 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 [李琼斯]
Powered by blists - more mailing lists