Hi Mark, Thank you for your review. > -----Original Message----- > From: Mark Brown [mailto:broonie@xxxxxxxxxx] > Sent: 2016年1月16日 1:58 > To: Yang, Wenyou <Wenyou.Yang@xxxxxxxxx> > Cc: Liam Girdwood <lgirdwood@xxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; > Pawel Moll <pawel.moll@xxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>; Ian > Campbell <ijc+devicetree@xxxxxxxxxxxxxx>; Kumar Gala <galak@xxxxxxxxxxxxxx>; > Ferre, Nicolas <Nicolas.FERRE@xxxxxxxxx>; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 1/2] regulator: act8945a: add regulator driver for ACT8945A > > On Fri, Jan 08, 2016 at 10:08:57AM +0800, Wenyou Yang wrote: > > > + np = of_get_child_by_name(pdev->dev.of_node, "regulators"); > > + if (!np) { > > + dev_err(&pdev->dev, "regulator node not found\n"); > > + return -EINVAL; > > + } > > + > > + matches = act8945a_matches; > > + num_matches = ARRAY_SIZE(act8945a_matches); > > + > > + ret = of_regulator_match(&pdev->dev, np, matches, num_matches); > > Don't open code this, use the core regulators_node and of_match implementation > instead. Good. In the next version, I use the code implementation, and remove the redundant code. Thank you for your advice. > > > + dev_info(act8945a_dev->dev, "%s regulator registered\n", > > + desc->name); > > This is just noise, please remove it - let the core handle notifying users. It is fixed in version 2. > > > +static int __init act8945a_pmic_init(void) { > > + return platform_driver_register(&act8945a_pmic_driver); > > +} > > +subsys_initcall(act8945a_pmic_init); > > Please use module_platform_driver() unless there is a strong reason to do > otherwise. It is fixed in version 2 too. Best Regards, Wenyou Yang ?韬{.n?????%??檩??w?{.n????z谵{???塄}?财??j:+v?? ? ???2??璀??摺?囤??z夸z罐?+?????w棹f
