On Wed, 02 Dec 2020, ChiYuan Huang wrote:
> Lee Jones <lee.jones@xxxxxxxxxx> 於 2020年11月26日 週四 上午12:42寫道：
> >
> > On Mon, 02 Nov 2020, cy_huang wrote:
> >
> > > From: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> > >
> > > Adds support Richtek RT4831 MFD core.
> > > RT4831 includes backlight and DSV part that can provode display panel
> > > for postive and negative voltage and WLED driving.
> > >
> > > Signed-off-by: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> > > ---
> > > drivers/mfd/Kconfig | 11 +++++
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/rt4831-core.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 131 insertions(+)
> > > create mode 100644 drivers/mfd/rt4831-core.c
> > >
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index 8b99a13..a22f002 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -1088,6 +1088,17 @@ config MFD_RDC321X
> > > southbridge which provides access to GPIOs and Watchdog using the
> > > southbridge PCI device configuration space.
> > >
> > > +config MFD_RT4831
> > > + tristate "Richtek RT4831 WLED and DSV IC"
> >
> > Please expand on WLED and DSV.
> >
> > This is documentation and should leave nothing to the imagination.
> >
> Rewrite to "Richtek RT4831 four channel WLED and display bias
> voltage", is it okay?
I had to look-up WLED, but I guess it's okay.
"Display Bias Voltage"
> > > + depends on I2C
> > > + select MFD_CORE
> > > + select REGMAP_I2C
> > > + help
> > > + This enables support for the Richtek RT4831.
> > > + RT4831 includes WLED driver and DisplayBias voltage(+/-) regulator.
> > > + It's common used to provide the display power and to drive the
> > > + display backlight WLED.
> >
> > Please don't line-wrap unnecessarily.
> >
> > Please re-work the last sentence, as it doesn't quite make sense.
> >
> Rewrite the whole sentence like as below
> "This enables support for the Richtek RT4831. It includes 4 channel
> WLED driving and Display Bias voltage output. It's commonly used to
> provide the LCD power and to drive LCD backlight."
"Display Bias Voltage"
"provide power to the LCD display"
[...]
> > > +static int rt4831_probe(struct i2c_client *client)
> > > +{
> > > + struct gpio_desc *enable;
> > > + struct regmap *regmap;
> > > + unsigned int val;
> > > + int ret;
> > > +
> > > + enable = devm_gpiod_get_optional(&client->dev, "enable", GPIOD_OUT_HIGH);
> > > + if (IS_ERR(enable)) {
> > > + dev_err(&client->dev, "Failed to get chip enable gpio\n");
> >
> > "Failed to get 'enable' GPIO chip"
> >
> May I remove "chip" word? It seems redundant.
> "Failed to get 'enable' GPIO" is better.
> Because 'enable' is a physical input pin for RT4831.
Sounds good.
[...]
> > > +static int rt4831_remove(struct i2c_client *client)
> > > +{
> > > + struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> > > +
> > > + /* Make sure all off before module removal */
> >
> > "Disable all <thing your disabling> are disabled before ..."
This should have said:
"Ensure all <thing your disabling> are disabled before ..."
> May I rewrite it to "Configure WLED driving and DSV output all to be
> disabled before MFD module removal"?
You don't need to mention MFD or modules here since we know how the
Device Driver model works and what .remove() does.
What about:
"Disable WLED and DSV outputs"
> > > + return regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
> > > +}
> > > +
> > > +static void rt4831_shutdown(struct i2c_client *client)
> > > +{
> > > + struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> > > +
> > > + /* Make sure all off before machine shutdown */
> >
> > As above.
> >
> like as above ".... before 'machine shutdown'
"Disable WLED and DSV outputs"
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
