On Thu, Jun 2, 2022 at 8:27 AM ChiYuan Huang <u0084500@xxxxxxxxx> 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@xxxxxxxxx> 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[]; > > > +}; -- With Best Regards, Andy Shevchenko