linux-kernel - Re: [PATCH v3 02/11] mfd: wcd934x: add support to wcd9340/wcd9341 codec

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 11 Nov 2019 12:48:27 +0000
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     robh@...nel.org, broonie@...nel.org, linus.walleij@...aro.org,
        vinod.koul@...aro.org, alsa-devel@...a-project.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        spapothi@...eaurora.org, bgoswami@...eaurora.org,
        linux-gpio@...r.kernel.org
Subject: Re: [PATCH v3 02/11] mfd: wcd934x: add support to wcd9340/wcd9341
 codec



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@...aro.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.


> 
>> +	{}
>> +};
> 
> [...]
>