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).
...
helpSupport 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 forMT6370_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.
+};
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).
...
helpSupport 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 forMT6370_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, 1Leave a comma at the end.
+};
+static const unsigned int mt6372_tfreqs[] = {
8000, 4000, 2000, 1000, 500, 250, 8, 4Ditto.
+};
-- With Best Regards, Andy Shevchenko
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.
...
helpSupport 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 forMT6370_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, 1Leave a comma at the end.
Ack in next.
+};
+static const unsigned int mt6372_tfreqs[] = {
8000, 4000, 2000, 1000, 500, 250, 8, 4Ditto.
Ack in next.
+};
-- With Best Regards, Andy Shevchenko
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[];+};
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
dri-devel@lists.freedesktop.org
-
Andy Shevchenko -
ChiYuan Huang