Messages in this thread | | | Date | Wed, 6 Mar 2019 11:08:54 +0800 | From | Tom Li <> | Subject | Re: [PATCH v2 1/7] mfd: yeeloong_kb3310b: support KB3310B EC for Lemote Yeeloong laptops. |
| |
On Tue, Mar 05, 2019 at 11:50:51PM +0000, Paul Burton wrote: > Hi Tom, > > Overall I think this is much better than the older out of tree platform > driver - thanks for cleaning it up.
I'm glad to hear that.
> One general comment - it would be good to run the patches through > ./scripts/checkpatch.pl & fix up any warnings it gives unless you have a > good reason to disagree with them.
The entire patchset has already been checked by checkpatch.pl, and all generated warnings have been corrected before submission. So I don't think the script can help me improving the style beyond this point. Thanks for pointing out all the additional style issues.
> Nit: the lines of asterisks aren't part of the kernel's general comment > style & I think it would looks cleaner to remove them.
Sure, I will remove these asterisks banners.
In fact I learned to use these asterisks from other kernel drivers to separate logical sections... and as a said, checkpatch.pl does not generate a warning about it, so I suspect that, historically, a clear policy about asterisks banners was never enforced. After finish working on these patches, perhaps it would a good time to patch checkpatch.pl and add a check for it?
> I'm not sure I follow why the power management code prevents use of > regmap? > > Are you talking about the wakeup_loongson() function? Perhaps it would > make sense for the suspend code to be part of one of the possible > subdrivers you mention. The lemote-2f seems to be the only system that > provides an implementation of wakeup_loongson() so perhaps a driver > could instead just register its own struct platform_suspend_ops & avoid > the need for code in arch/mips to care about the EC.
I'll reword my misleading commit message about using regmap.
Let me clarify it - nothing prevents the use of regmap. I originally thought it would be a good idea since the manual locking can be eliminated.
But later I realized it would mean more changes. In particular, not only pm.c needs to be a new subdriver, the shutdown code in ml2f_reboot() also calls ec_write(), and it needs to be converted to a subdriver in drivers/power/reset/ too. Yes, it would be good to refactor these code and move them to a more suitable place, but currently, the primary goal is to add platform drivers, not to touch functions I've just fixed and possibly introduce new regressions...
As you've seen, there is a TODO item in [PATCH 6/7] in this series, it says the CS5536 code also needs some major refactoring: the GPIO needs to be changed, the clocksource driver needs to be merged with the clockevent driver under drivers/clocksource, etc. I think we can, as well, save the pm.c/reset.c for later. Currently, I think it's better to focus on the platform drivers first.
If you still have objections on it, please let me know.
> > +#define DRV_NAME "yeeloong_kb3310b: " > > Defining pr_fmt() would be cleaner - you wouldn't need to manually > include DRV_NAME in your messages later.
Noted.
> > +static struct kb3310b_chip *kb3310b_fwinfo; > > + > > +static const struct mfd_cell kb3310b_cells[] = { > > + { > > + .name = "yeeloong_sci" > > + }, > > + { > > + .name = "yeeloong_hwmon" > > + }, > > + { > > + .name = "yeeloong_battery" > > + }, > > + { > > + .name = "yeeloong_backlight" > > + }, > > + { > > + .name = "yeeloong_lcd" > > + }, > > + { > > + .name = "yeeloong_hotkey" > > + }, > > Nit: I think it'd look cleaner if you remove the newlines within each > array entry, eg: > > { .name = "yeeloong_sci" }, > { .name = "yeelong_hwmon" }, > ...
Yes.
> > + > > +static DEFINE_SPINLOCK(kb3310b_command_lock); > > Since this is only used in kb3310b_query_seq() could you just declare it > (still static) inside that function?
No problem.
Thanks for your review, I'll correct all the issues above and send v3 today, if there's no addition problem in v3, I will start sending the actual platform drivers (battery/hwmon/etc) for the next round of review.
P.S: This time, I hope Lee Jones, as the MFD subsystem maintainer, has received my mail and my patch, including this one. Unfortunately, all signs indicated he hasn't received it. Jones, if you have received this mail but currently too busy to review, please reply to confirm, thanks!
Sincerely yours,
Tom Li
| |