linux-kernel - Re: [PATCH v7] iio: adc: add exynos adc driver under iio framwork

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:	Fri, 15 Feb 2013 14:26:10 +0100
From:	Lars-Peter Clausen <lars@...afoo.de>
To:	Naveen Krishna Ch <naveenkrishna.ch@...il.com>
CC:	Naveen Krishna Chatradhi <ch.naveen@...sung.com>,
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
	"linux-samsung-soc@...r.kernel.org" 
	<linux-samsung-soc@...r.kernel.org>, dianders@...omium.org,
	gregkh@...uxfoundation.org
Subject: Re: [PATCH v7] iio: adc: add exynos adc driver under iio framwork

On 02/15/2013 02:17 PM, Naveen Krishna Ch wrote:
> On 15 February 2013 18:43, Lars-Peter Clausen <lars@...afoo.de> wrote:
>> On 02/15/2013 07:56 AM, Naveen Krishna Chatradhi wrote:
>>> This patch adds New driver to support:
>>> 1. Supports ADC IF found on EXYNOS4412/EXYNOS5250
>>>    and future SoCs from Samsung
>>> 2. Add ADC driver under iio/adc framework
>>> 3. Also adds the Documentation for device tree bindings
>>>
>>> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@...sung.com>
>>
>> Looks good.
>>
>> Reviewed-by: Lars-Peter Clausen <lars@...afoo.de>
>>
>> One minor thing though, there are a couple of places where you break a line
>> into multiple lines, even though the line fits easily inside the 80 chars
>> per line limit. In my opinion this doesn't help the legibility of the code.
>> E.g.:
>>
>> +       info->value = readl(ADC_V1_DATX(info->regs)) &
>> +                                               ADC_DATX_MASK;
>>
>> There is no need to respin the patch just for this, but if you happen to
>> make another version of the patch, that's something to consider.
>>
>>> ---
>>> Changes since v1:
>>>
>>> 1. Fixed comments from Lars
>>> 2. Added support for ADC on EXYNOS5410
>>>
>>> Changes since v2:
>>>
>>> 1. Changed the instance name for (struct iio_dev *) to indio_dev
>>> 2. Changed devm_request_irq to request_irq
>>>
>>> Few doubts regarding the mappings and child device handling.
>>> Kindly, suggest me better methods.
>>>
>>> Changes since v3:
>>>
>>> 1. Added clk_prepare_disable and regulator_disable calls in _remove()
>>> 2. Moved init_completion before irq_request
>>> 3. Added NULL pointer check for devm_request_and_ioremap() return value.
>>> 4. Use number of channels as per the ADC version
>>> 5. Change the define ADC_V1_CHANNEL to ADC_CHANNEL
>>> 6. Update the Documentation to include EXYNOS5410 compatible
>>>
>>> Changes since v4:
>>>
>>> 1. if devm_request_and_ioremap() failes, free iio_device before returning
>>>
>>> Changes since v5:
>>>
>>> 1. Fixed comments from Olof (ADC hardware version handling)
>>> 2. Rebased on top of comming OF framework for IIO by "Guenter Roeck".
>>>
>>> Changes since v6:
>>>
>>> 1. Addressed comments from Lars-Peter Clausen
>>
>>
>> btw. these kind of change logs are not really helpful, it would be better to
>> list the actual changes made.
> Hello Lars,
> 
> No other changes from my side. But, I can send another version.
> Do you want me to list the latest change alone instead of the whole
> change list ?

Hi,

No need to resend the patch, this is just something to consider for the
future. A changelog entry which reads like "Addressed Jon Does comments" is
not really useful since most people will probably not know or not longer
remember all the details of those comments, instead a nice list of all the
changes which have been made is much better. E.g.:

Changes since v6:
	* Fixed debugfs_read_reg
	* Introduced timeout when waiting for the data ready IRQ
	* Slightly reformatted exynos_read_raw for better legibility

- Lars


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/