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

[ChiYuan Huang]

Lee Jones <lee.jones@xxxxxxxxxx> 於 2020年12月2日 週三 下午4:49寫道：
>
> 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"
>
OK, I'll change this line to
"Richtek RT4831 fource channel WLED and 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"
>
Thanks. looks better
> [...]
>
> > > > +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"
Do you mean that only keep this comment line is better? NO more
redundant description like "before ....".
If yes, I'll only leave  the comment like as you said in remove/shutdown ops.
> > > > +     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"
Same as above.

Thanks for all the suggestion.
If any misunderstanding, please kindly let me know.
>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog