| 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
| ||
|
Message-ID: <505D851E.6060605@kernel.org>
Date: Sat, 22 Sep 2012 10:30:06 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Lars-Peter Clausen <lars@...afoo.de>
CC: "Kim, Milo" <Milo.Kim@...com>, Jonathan Cameron <jic23@....ac.uk>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6] iio: adc: add new lp8788 adc driver
On 09/18/2012 09:18 PM, Jonathan Cameron wrote:
> On 09/18/2012 09:19 AM, Lars-Peter Clausen wrote:
>> On 09/17/2012 11:35 AM, Kim, Milo wrote:
>>> TI LP8788 PMU provides regulators, battery charger, ADC,
>>> RTC, backlight driver and current sinks.
>>>
>>> This patch enables the LP8788 ADC functions.
>>>
>>> The LP8788 ADC has several ADC input selection and supports 12bit resolution.
>>> Internal operation of getting ADC is access to registers of LP8788.
>>> The LP8788 ADC uses exported functions for accessing these registers.
>>> (exported by LP8788 MFD device driver)
>>>
>>> This driver supports IIO_CHAN_INFO_RAW and SCALE.
>>> So the IIO consumer can calculate the value with raw and scale.
>>> The unit of scale is micro.
>>>
>>> (ADC Input Selection)
>>>
>>> Voltage: battery voltage (MAX 5.0, 5.5 and 6.0V)
>>> charger input voltage
>>> four general ADC inputs
>>> coin cell voltage
>>> Current: battery charging current
>>> Temperature: IC temperature
>>>
>>> (The IIO map for the IIO consumer)
>>>
>>> The ADC input is configurable in the platform side.
>>> Even though this platform data is not defined,
>>> the default IIO map is created for supporting the power supply driver.
>>> The battery voltage and temperature are used inside this driver.
>>>
>>> (History)
>>>
>>> Patch v6.
>>> (a) Fix scale value for each ADC input selection
>>> Voltage and current type are mili unit and temperature is degree.
>>> To calculate the IC temperature,
>>> temp = raw * scaleint + (raw * scalepart)/ 1000000, scaleint is always 0.
>>> = raw * 0.061050, raw: 0 ~ 4095
>>> Then range of IC temperature(ADC result) is 0 ~ 250'C
>>>
>>> (b) Reorganization of the IIO channel Spec
>>> Remove address, scan_type and scan_index and rollback the datasheet name.
>>> The reason why 'address' field is unnecessary is no relation with each channel.
>>> Moreover, to get the raw ADC value, the address info is not only one register
>>> but also several registers.
>>> Therefore specific function(lp8788_get_adc_result) is called rather than
>>> using one 'address' field.
>>>
>>> (c) Fix coding style
>>> Remove duplicated checking routine while unregistering the IIO map.
>>> Fix code for space and parenthesis.
>>>
>>> Patch v5.
>>> Fix default consumer name as 'lp8788-charger'.
>>> Add mutex for ADC read operation.
>>> Reorganization on lp8788_adc_read_raw().
>>>
>>> Patch v4.
>>> Fix adc_raw function: support RAW and SCALE channel info.
>>> Change LP8788 ADC platform data - iio map.
>>> Enables the default IIO map.
>>>
>>> Patch v3.
>>> Fix wrong size of allocating iio private data.
>>> Fix coding styles.
>>>
>>> Patch v2.
>>> Support RAW and SCALE interface for IIO consumer.
>>> Clean up the iio channel spec macro.
>>>
>>> Signed-off-by: Milo(Woogyom) Kim <milo.kim@...com>
>>
>> Looks good to me,
>>
>> Reviewed-by: Lars-Peter Clausen <lars@...afoo.de>
> I've added this to my local tree but not pushed out just
> yet on basis you might want to change the behaviour Lars
> has pointed out...
>
> I don't here it'll go as is in a day or so.
Added to togreg branch of iio.git
>
> Jonathan
>>
>> One comment though, not sure if it is critical or not.
>>
>>> +static int lp8788_get_adc_result(struct lp8788_adc *adc, enum lp8788_adc_id id,
>>> + int *val)
>>> +{
>>> + unsigned int msb;
>>> + unsigned int lsb;
>>> + unsigned int result;
>>> + u8 data;
>>> + u8 rawdata[2];
>>> + int size = ARRAY_SIZE(rawdata);
>>> + int retry = 5;
>>> + int ret;
>>> +
>>> + data = (id << 1) | ADC_CONV_START;
>>> + ret = lp8788_write_byte(adc->lp, LP8788_ADC_CONF, data);
>>> + if (ret)
>>> + goto err_io;
>>> +
>>> + /* retry until adc conversion is done */
>>> + data = 0;
>>> + while (retry--) {
>>> + usleep_range(100, 200);
>>> +
>>> + ret = lp8788_read_byte(adc->lp, LP8788_ADC_DONE, &data);
>>> + if (ret)
>>> + goto err_io;
>>> +
>>> + /* conversion done */
>>> + if (data)
>>> + break;
>>
>> Could as well go into the while header like this:
>> while (!data && retry--)
>>
>>> + }
>>> +
>>
>> You still sample the data, even if there was a timeout and ADC_DONE is not set.
>>
>>> + ret = lp8788_read_multi_bytes(adc->lp, LP8788_ADC_RAW, rawdata, size);
>>> + if (ret)
>>> + goto err_io;
>>> +
>>> + msb = (rawdata[0] << 4) & 0x00000ff0;
>>> + lsb = (rawdata[1] >> 4) & 0x0000000f;
>>> + result = msb | lsb;
>>> + *val = result;
>>> +
>>> + return 0;
>>> +
>>> +err_io:
>>> + return ret;
>>> +}
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
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/
Powered by blists - more mailing lists