On Wed, Jun 1, 2022 at 11:48 AM Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > On Tue, May 31, 2022 at 1:16 PM ChiaEn Wu <peterwu.pub@xxxxxxxxx> wrote: > > > > From: Alice Chen <alice_chen@xxxxxxxxxxx> > > 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 -- With Best Regards, Andy Shevchenko