Messages in this thread | | | Subject | Re: [PATCH v3 02/11] mfd: wcd934x: add support to wcd9340/wcd9341 codec | From | Srinivas Kandagatla <> | Date | Mon, 11 Nov 2019 12:48:27 +0000 |
| |
On 11/11/2019 11:18, Lee Jones wrote: > On Tue, 29 Oct 2019, Srinivas Kandagatla wrote: > >> Qualcomm WCD9340/WCD9341 Codec is a standalone Hi-Fi audio codec IC. >> >> This codec has integrated SoundWire controller, pin controller and >> interrupt controller. >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> --- > > No changelog?
I have done that in cover letter. If you prefer it here, I can add that in next version.
> >> drivers/mfd/Kconfig | 12 + >> drivers/mfd/Makefile | 1 + >> drivers/mfd/wcd934x.c | 306 +++++++++++++++ >> include/linux/mfd/wcd934x/registers.h | 529 ++++++++++++++++++++++++++ >> include/linux/mfd/wcd934x/wcd934x.h | 31 ++ >> 5 files changed, 879 insertions(+) >> create mode 100644 drivers/mfd/wcd934x.c >> create mode 100644 include/linux/mfd/wcd934x/registers.h >> create mode 100644 include/linux/mfd/wcd934x/wcd934x.h > > This driver reads much better now. Thanks for making the changes. > >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >> index ae24d3ea68ea..9fe7e54b13bf 100644 >> --- a/drivers/mfd/Kconfig >> +++ b/drivers/mfd/Kconfig >> @@ -1967,6 +1967,18 @@ config MFD_STMFX >> additional drivers must be enabled in order to use the functionality >> of the device. >> >> +config MFD_WCD934X >> + tristate "Support for WCD9340/WCD9341 Codec" >> + depends on SLIMBUS >> + select REGMAP >> + select REGMAP_SLIMBUS >> + select REGMAP_IRQ >> + select MFD_CORE >> + help >> + Support for the Qualcomm WCD9340/WCD9341 Codec. >> + This driver provides common support wcd934x audio codec and its >> + associated Pin Controller, Soundwire Controller and Audio codec. > > Your capitalisation of devices is all over the place in both your help > section and in the commit message. Either capitalise them all or none > of them. Personally I would prefer all, rather than none. What ever > you choose, please be consistent. > > Same for "wcd934x", this should read "WCD934x" in all comments and the > help.
I agree, will fix it along with other Nits you suggested.
[...] >> +static void wcd934x_slim_remove(struct slim_device *sdev) >> +{ >> + struct wcd934x_ddata *ddata = dev_get_drvdata(&sdev->dev); >> + >> + regulator_bulk_disable(WCD934X_MAX_SUPPLY, ddata->supplies); >> + mfd_remove_devices(&sdev->dev); >> + kfree(ddata); >> +} >> + >> +static const struct slim_device_id wcd934x_slim_id[] = { >> + { SLIM_MANF_ID_QCOM, SLIM_PROD_CODE_WCD9340, 0x1, 0x0 }, > > What do the last parameters mean? Might be better to define them.
This is Instance ID and Device ID of SLIMBus enumeration address.
> >> + {} >> +}; > > [...] >
| |