Re: [PATCH v1 1/2] mfd: rt4831: Adds support for Richtek RT4831 MFD core

[Lee Jones <lee.jones@linaro.org>]

On Wed, 02 Dec 2020, ChiYuan Huang wrote:

> Lee Jones <lee.jones@linaro.org> 於 2020年11月26日 週四 上午12:42寫道：
> >
> > On Mon, 02 Nov 2020, cy_huang wrote:
> >
> > > From: ChiYuan Huang <cy_huang@richtek.com>
> > >
> > > 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@richtek.com>
> > > ---
> > >  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