| 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
| ||
|
Date: Wed, 13 May 2020 14:50:31 +0530
From: Jishnu Prakash <jprakash@...eaurora.org>
To: Amit Kucheria <amit.kucheria@...durent.com>
Cc: Andy Gross <agross@...nel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>,
Matthias Kaehlcke <mka@...omium.org>,
Linus Walleij <linus.walleij@...aro.org>,
Jonathan.Cameron@...wei.com, smohanad@...eaurora.org,
kgunda@...eaurora.org, aghayal@...eaurora.org,
Jonathan Cameron <jic23@...nel.org>,
Hartmut Knaack <knaack.h@....de>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald-Stadler <pmeerw@...erw.net>,
linux-arm-msm <linux-arm-msm@...r.kernel.org>,
linux-iio@...r.kernel.org, linux-arm-msm-owner@...r.kernel.org
Subject: Re: [PATCH V3 3/4] iio: adc: Add support for PMIC7 ADC
Hi Amit,
On 5/5/2020 3:33 PM, Amit Kucheria wrote:
> Hi Jishnu,
>
> It is nice to cc a reviewer from a previous version. Just saw this.
>
> On Mon, Apr 27, 2020 at 6:55 PM Jishnu Prakash <jprakash@...eaurora.org> wrote:
>> The ADC architecture on PMIC7 is changed as compared to PMIC5. The
>> major change from PMIC5 is that all SW communication to ADC goes through
>> PMK8350, which communicates with other PMICs through PBS when the ADC
>> on PMK8350 works in master mode. The SID register is used to identify the
>> PMICs with which the PBS needs to communicate. Add support for the same.
>>
>> Signed-off-by: Jishnu Prakash <jprakash@...eaurora.org>
>> ---
>> drivers/iio/adc/qcom-spmi-adc5.c | 239 +++++++++++++++++++++++++++++++++-
>> drivers/iio/adc/qcom-vadc-common.c | 260 +++++++++++++++++++++++++++++++++++++
>> drivers/iio/adc/qcom-vadc-common.h | 15 +++
>> 3 files changed, 510 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iio/adc/qcom-spmi-adc5.c b/drivers/iio/adc/qcom-spmi-adc5.c
>> index 21fdcde..a66eeb7 100644
>> --- a/drivers/iio/adc/qcom-spmi-adc5.c
>> +++ b/drivers/iio/adc/qcom-spmi-adc5.c
>> @@ -1,6 +1,6 @@
>> // SPDX-License-Identifier: GPL-2.0
>> /*
>> - * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2018, 2020, The Linux Foundation. All rights reserved.
>> */
>>
>> #include <linux/bitops.h>
>> @@ -23,6 +23,7 @@
>>
>> #define ADC5_USR_REVISION1 0x0
>> #define ADC5_USR_STATUS1 0x8
>> +#define ADC5_USR_STATUS1_CONV_FAULT BIT(7)
>> #define ADC5_USR_STATUS1_REQ_STS BIT(1)
>> #define ADC5_USR_STATUS1_EOC BIT(0)
>> #define ADC5_USR_STATUS1_REQ_STS_EOC_MASK 0x3
>> @@ -65,6 +66,9 @@
>>
>> #define ADC5_USR_IBAT_DATA1 0x53
>>
>> +#define ADC_CHANNEL_OFFSET 0x8
>> +#define ADC_CHANNEL_MASK GENMASK(7, 0)
>> +
>> /*
>> * Conversion time varies based on the decimation, clock rate, fast average
>> * samples and measurements queued across different VADC peripherals.
>> @@ -79,6 +83,11 @@
>> #define ADC5_HW_SETTLE_DIFF_MINOR 3
>> #define ADC5_HW_SETTLE_DIFF_MAJOR 5
>>
>> +/* For PMIC7 */
>> +#define ADC_APP_SID 0x40
>> +#define ADC_APP_SID_MASK GENMASK(3, 0)
>> +#define ADC7_CONV_TIMEOUT msecs_to_jiffies(10)
>> +
>> enum adc5_cal_method {
>> ADC5_NO_CAL = 0,
>> ADC5_RATIOMETRIC_CAL,
>> @@ -96,6 +105,7 @@ enum adc5_cal_val {
>> * @cal_method: calibration method.
>> * @cal_val: calibration value
>> * @decimation: sampling rate supported for the channel.
>> + * @sid: slave id of PMIC owning the channel, for PMIC7.
>> * @prescale: channel scaling performed on the input signal.
>> * @hw_settle_time: the time between AMUX being configured and the
>> * start of conversion.
>> @@ -110,6 +120,7 @@ struct adc5_channel_prop {
>> enum adc5_cal_method cal_method;
>> enum adc5_cal_val cal_val;
>> unsigned int decimation;
>> + unsigned int sid;
>> unsigned int prescale;
>> unsigned int hw_settle_time;
>> unsigned int avg_samples;
>> @@ -140,6 +151,7 @@ struct adc5_chip {
>> bool poll_eoc;
>> struct completion complete;
>> struct mutex lock;
>> + int irq_eoc;
>> const struct adc5_data *data;
>> };
>>
>> @@ -155,6 +167,8 @@ static const struct vadc_prescale_ratio adc5_prescale_ratios[] = {
>> {.num = 1, .den = 16}
>> };
>>
>> +static const struct adc5_data adc7_data_pmic;
>> +
>> static int adc5_read(struct adc5_chip *adc, u16 offset, u8 *data, int len)
>> {
>> return regmap_bulk_read(adc->regmap, adc->base + offset, data, len);
>> @@ -165,6 +179,11 @@ static int adc5_write(struct adc5_chip *adc, u16 offset, u8 *data, int len)
>> return regmap_bulk_write(adc->regmap, adc->base + offset, data, len);
>> }
>>
>> +static int adc5_masked_write(struct adc5_chip *adc, u16 offset, u8 mask, u8 val)
>> +{
>> + return regmap_update_bits(adc->regmap, adc->base + offset, mask, val);
>> +}
>> +
>> static int adc5_prescaling_from_dt(u32 num, u32 den)
>> {
>> unsigned int pre;
>> @@ -314,6 +333,47 @@ static int adc5_configure(struct adc5_chip *adc,
>> return adc5_write(adc, ADC5_USR_DIG_PARAM, buf, sizeof(buf));
>> }
>>
>> +static int adc7_configure(struct adc5_chip *adc,
>> + struct adc5_channel_prop *prop)
>> +{
>> + int ret;
>> + u8 conv_req = 0, buf[4];
>> +
>> + ret = adc5_masked_write(adc, ADC_APP_SID, ADC_APP_SID_MASK, prop->sid);
>> + if (ret)
>> + return ret;
>> +
>> + ret = adc5_read(adc, ADC5_USR_DIG_PARAM, buf, sizeof(buf));
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* Digital param selection */
>> + adc5_update_dig_param(adc, prop, &buf[0]);
>> +
>> + /* Update fast average sample value */
>> + buf[1] &= 0xff & ~ADC5_USR_FAST_AVG_CTL_SAMPLES_MASK;
> Did you answer Andy's query about this in v2? I couldn't find it.
I missed it at earlier, I have replied now and will make the change in
my latest post.
>
>> + buf[1] |= prop->avg_samples;
>> +
>> + /* Select ADC channel */
>> + buf[2] = prop->channel;
>> +
>> + /* Select HW settle delay for channel */
>> + buf[3] &= 0xff & ~ADC5_USR_HW_SETTLE_DELAY_MASK;
> Same here.
I missed it at earlier, I have replied now and will make the change in
my latest post.
>
>> + buf[3] |= prop->hw_settle_time;
>> +
>> + /* Select CONV request */
>> + conv_req = ADC5_USR_CONV_REQ_REQ;
>> +
>> + if (!adc->poll_eoc)
>> + reinit_completion(&adc->complete);
>> +
>> + ret = adc5_write(adc, ADC5_USR_DIG_PARAM, buf, sizeof(buf));
>> + if (ret)
>> + return ret;
>> +
>> + return adc5_write(adc, ADC5_USR_CONV_REQ, &conv_req, 1);
> Is it required to call adc5_write() twice here in sequence on the
> adc7? adc5_configure() above manages just fine by having an extra
> element in buf and one call.
In PMIC5, that write is used to write to registers 0x42-0x47. Register
0x46 has been removed for PMIC7 ADC, so we use two writes, for 0x42-0x45
and for 0x47 alone.
>
>> +}
>> +
>> static int adc5_do_conversion(struct adc5_chip *adc,
>> struct adc5_channel_prop *prop,
>> struct iio_chan_spec const *chan,
>> @@ -355,6 +415,43 @@ static int adc5_do_conversion(struct adc5_chip *adc,
>> return ret;
>> }
>>
>> +static int adc7_do_conversion(struct adc5_chip *adc,
>> + struct adc5_channel_prop *prop,
>> + struct iio_chan_spec const *chan,
>> + u16 *data_volt, u16 *data_cur)
>> +{
>> + int ret;
>> + u8 status;
>> +
>> + mutex_lock(&adc->lock);
>> +
>> + ret = adc7_configure(adc, prop);
>> + if (ret) {
>> + dev_err(adc->dev, "ADC configure failed with %d\n", ret);
>> + goto unlock;
>> + }
>> +
>> + /* No support for polling mode at present */
>> + wait_for_completion_timeout(&adc->complete, ADC7_CONV_TIMEOUT);
>> +
>> + ret = adc5_read(adc, ADC5_USR_STATUS1, &status, 1);
>> + if (ret < 0)
>> + goto unlock;
>> +
>> + if (status & ADC5_USR_STATUS1_CONV_FAULT) {
>> + dev_err(adc->dev, "Unexpected conversion fault\n");
>> + ret = -EIO;
>> + goto unlock;
>> + }
>> +
>> + ret = adc5_read_voltage_data(adc, data_volt);
>> +
>> +unlock:
>> + mutex_unlock(&adc->lock);
>> +
>> + return ret;
>> +}
>> +
>> static irqreturn_t adc5_isr(int irq, void *dev_id)
>> {
>> struct adc5_chip *adc = dev_id;
>> @@ -377,6 +474,56 @@ static int adc5_of_xlate(struct iio_dev *indio_dev,
>> return -EINVAL;
>> }
>>
>> +static int adc7_of_xlate(struct iio_dev *indio_dev,
>> + const struct of_phandle_args *iiospec)
>> +{
>> + struct adc5_chip *adc = iio_priv(indio_dev);
>> + int i, v_channel;
>> +
>> + for (i = 0; i < adc->nchannels; i++) {
>> + v_channel = (adc->chan_props[i].sid << ADC_CHANNEL_OFFSET |
>> + adc->chan_props[i].channel);
>> + if (v_channel == iiospec->args[0])
>> + return i;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static int adc7_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan, int *val, int *val2,
>> + long mask)
> Just a nit. Since all your adc7 functions follow your adc5 functions,
> please move this below adc5_read_raw for consistency.
I'll do it in the next post.
>
>> +{
>> + struct adc5_chip *adc = iio_priv(indio_dev);
>> + struct adc5_channel_prop *prop;
>> + u16 adc_code_volt, adc_code_cur;
>> + int ret;
>> +
>> + prop = &adc->chan_props[chan->address];
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_PROCESSED:
>> + ret = adc7_do_conversion(adc, prop, chan,
> Except for this bit (call to adc7_do_conversion instead of
> adc5_do_conversion), this function is identical to adc5_read_raw.
>
> How about making adc5_read_raw and adc7_read_raw a shim around a
> function adc_read_raw_common() that takes a function pointer?
I'll do this in the next post.
>
>> + &adc_code_volt, &adc_code_cur);
>> + if (ret)
>> + return ret;
>> +
>> + ret = qcom_adc5_hw_scale(prop->scale_fn_type,
>> + &adc5_prescale_ratios[prop->prescale],
>> + adc->data,
>> + adc_code_volt, val);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + return IIO_VAL_INT;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int adc5_read_raw(struct iio_dev *indio_dev,
>> struct iio_chan_spec const *chan, int *val, int *val2,
>> long mask)
>> @@ -415,6 +562,11 @@ static const struct iio_info adc5_info = {
>> .of_xlate = adc5_of_xlate,
>> };
>>
>> +static const struct iio_info adc7_info = {
>> + .read_raw = adc7_read_raw,
>> + .of_xlate = adc7_of_xlate,
>> +};
>> +
>> struct adc5_channels {
>> const char *datasheet_name;
>> unsigned int prescale_index;
>> @@ -477,6 +629,39 @@ static const struct adc5_channels adc5_chans_pmic[ADC5_MAX_CHANNEL] = {
>> SCALE_HW_CALIB_PM5_SMB_TEMP)
>> };
>>
>> +static const struct adc5_channels adc7_chans_pmic[ADC5_MAX_CHANNEL] = {
>> + [ADC7_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 0,
>> + SCALE_HW_CALIB_DEFAULT)
>> + [ADC7_1P25VREF] = ADC5_CHAN_VOLT("vref_1p25", 0,
>> + SCALE_HW_CALIB_DEFAULT)
>> + [ADC7_VPH_PWR] = ADC5_CHAN_VOLT("vph_pwr", 1,
>> + SCALE_HW_CALIB_DEFAULT)
>> + [ADC7_VBAT_SNS] = ADC5_CHAN_VOLT("vbat_sns", 3,
>> + SCALE_HW_CALIB_DEFAULT)
>> + [ADC7_DIE_TEMP] = ADC5_CHAN_TEMP("die_temp", 0,
>> + SCALE_HW_CALIB_PMIC_THERM_PM7)
>> + [ADC7_AMUX_THM1_100K_PU] = ADC5_CHAN_TEMP("amux_thm1_pu2", 0,
>> + SCALE_HW_CALIB_THERM_100K_PU_PM7)
>> + [ADC7_AMUX_THM2_100K_PU] = ADC5_CHAN_TEMP("amux_thm2_pu2", 0,
>> + SCALE_HW_CALIB_THERM_100K_PU_PM7)
>> + [ADC7_AMUX_THM3_100K_PU] = ADC5_CHAN_TEMP("amux_thm3_pu2", 0,
>> + SCALE_HW_CALIB_THERM_100K_PU_PM7)
>> + [ADC7_AMUX_THM4_100K_PU] = ADC5_CHAN_TEMP("amux_thm4_pu2", 0,
>> + SCALE_HW_CALIB_THERM_100K_PU_PM7)
>> + [ADC7_AMUX_THM5_100K_PU] = ADC5_CHAN_TEMP("amux_thm5_pu2", 0,
>> + SCALE_HW_CALIB_THERM_100K_PU_PM7)
>> + [ADC7_AMUX_THM6_100K_PU] = ADC5_CHAN_TEMP("amux_thm6_pu2", 0,
> Use space instead of tab for indentation in all the THMx channels to
> make it look more in line.
I'll do this in the next post.
>
>> + SCALE_HW_CALIB_THERM_100K_PU_PM7)
>> + [ADC7_GPIO1_100K_PU] = ADC5_CHAN_TEMP("gpio1_pu2", 0,
>> + SCALE_HW_CALIB_THERM_100K_PU_PM7)
>> + [ADC7_GPIO2_100K_PU] = ADC5_CHAN_TEMP("gpio2_pu2", 0,
>> + SCALE_HW_CALIB_THERM_100K_PU_PM7)
>> + [ADC7_GPIO3_100K_PU] = ADC5_CHAN_TEMP("gpio3_pu2", 0,
>> + SCALE_HW_CALIB_THERM_100K_PU_PM7)
>> + [ADC7_GPIO4_100K_PU] = ADC5_CHAN_TEMP("gpio4_pu2", 0,
>> + SCALE_HW_CALIB_THERM_100K_PU_PM7)
>> +};
>> +
>> static const struct adc5_channels adc5_chans_rev2[ADC5_MAX_CHANNEL] = {
>> [ADC5_REF_GND] = ADC5_CHAN_VOLT("ref_gnd", 0,
>> SCALE_HW_CALIB_DEFAULT)
>> @@ -511,6 +696,7 @@ static int adc5_get_dt_channel_data(struct adc5_chip *adc,
>> {
>> const char *name = node->name, *channel_name;
>> u32 chan, value, varr[2];
>> + u32 sid = 0;
>> int ret;
>> struct device *dev = adc->dev;
>>
>> @@ -520,6 +706,15 @@ static int adc5_get_dt_channel_data(struct adc5_chip *adc,
>> return ret;
>> }
>>
>> + /* Value read from "reg" is virtual channel number */
>> +
>> + /* virtual channel number = sid << 8 | channel number */
>> +
>> + if (adc->data == &adc7_data_pmic) {
>> + sid = chan >> ADC_CHANNEL_OFFSET;
>> + chan = chan & ADC_CHANNEL_MASK;
>> + }
>> +
>> if (chan > ADC5_PARALLEL_ISENSE_VBAT_IDATA ||
>> !data->adc_chans[chan].datasheet_name) {
>> dev_err(dev, "%s invalid channel number %d\n", name, chan);
>> @@ -528,6 +723,7 @@ static int adc5_get_dt_channel_data(struct adc5_chip *adc,
>>
>> /* the channel has DT description */
>> prop->channel = chan;
>> + prop->sid = sid;
>>
>> channel_name = of_get_property(node,
>> "label", NULL) ? : node->name;
>> @@ -578,8 +774,9 @@ static int adc5_get_dt_channel_data(struct adc5_chip *adc,
>> pr_debug("dig_ver:minor:%d, major:%d\n", dig_version[0],
>> dig_version[1]);
>> /* Digital controller >= 5.3 have hw_settle_2 option */
>> - if (dig_version[0] >= ADC5_HW_SETTLE_DIFF_MINOR &&
>> - dig_version[1] >= ADC5_HW_SETTLE_DIFF_MAJOR)
>> + if ((dig_version[0] >= ADC5_HW_SETTLE_DIFF_MINOR &&
>> + dig_version[1] >= ADC5_HW_SETTLE_DIFF_MAJOR) ||
>> + adc->data == &adc7_data_pmic)
>> ret = adc5_hw_settle_time_from_dt(value,
>> data->hw_settle_2);
>> else
>> @@ -629,6 +826,7 @@ static const struct adc5_data adc5_data_pmic = {
>> .full_scale_code_volt = 0x70e4,
>> .full_scale_code_cur = 0x2710,
>> .adc_chans = adc5_chans_pmic,
>> + .info = &adc5_info,
> This belongs in a separate patch along with the change in
> adc5_data_pmic_rev2 below
I'll add a separate patch for this in the next post.
>
>> .decimation = (unsigned int [ADC5_DECIMATION_SAMPLES_MAX])
>> {250, 420, 840},
>> .hw_settle_1 = (unsigned int [VADC_HW_SETTLE_SAMPLES_MAX])
>> @@ -639,10 +837,23 @@ static const struct adc5_data adc5_data_pmic = {
>> 1, 2, 4, 8, 16, 32, 64, 128},
>> };
>>
>> +static const struct adc5_data adc7_data_pmic = {
>> + .full_scale_code_volt = 0x70e4,
>> + .adc_chans = adc7_chans_pmic,
>> + .info = &adc7_info,
>> + .decimation = (unsigned int [ADC5_DECIMATION_SAMPLES_MAX])
>> + {85, 340, 1360},
>> + .hw_settle_2 = (unsigned int [VADC_HW_SETTLE_SAMPLES_MAX])
>> + {15, 100, 200, 300, 400, 500, 600, 700,
>> + 1000, 2000, 4000, 8000, 16000, 32000,
>> + 64000, 128000},
>> +};
>> +
>> static const struct adc5_data adc5_data_pmic_rev2 = {
>> .full_scale_code_volt = 0x4000,
>> .full_scale_code_cur = 0x1800,
>> .adc_chans = adc5_chans_rev2,
>> + .info = &adc5_info,
> Separate patch.
I'll add a separate patch for this in the next post.
>
>> .decimation = (unsigned int [ADC5_DECIMATION_SAMPLES_MAX])
>> {256, 512, 1024},
>> .hw_settle_1 = (unsigned int [VADC_HW_SETTLE_SAMPLES_MAX])
>> @@ -659,6 +870,10 @@ static const struct of_device_id adc5_match_table[] = {
>> .data = &adc5_data_pmic,
>> },
>> {
>> + .compatible = "qcom,spmi-adc7",
>> + .data = &adc7_data_pmic,
>> + },
>> + {
>> .compatible = "qcom,spmi-adc-rev2",
>> .data = &adc5_data_pmic_rev2,
>> },
>> @@ -752,6 +967,9 @@ static int adc5_probe(struct platform_device *pdev)
>> adc->regmap = regmap;
>> adc->dev = dev;
>> adc->base = reg;
>> +
>> + platform_set_drvdata(pdev, adc);
>> +
>> init_completion(&adc->complete);
>> mutex_init(&adc->lock);
>>
>> @@ -761,6 +979,8 @@ static int adc5_probe(struct platform_device *pdev)
>> return ret;
>> }
>>
>> + indio_dev->info = adc->data->info;
>> +
>> irq_eoc = platform_get_irq(pdev, 0);
>> if (irq_eoc < 0) {
>> if (irq_eoc == -EPROBE_DEFER || irq_eoc == -EINVAL)
>> @@ -773,23 +993,34 @@ static int adc5_probe(struct platform_device *pdev)
>> return ret;
>> }
>>
>> + adc->irq_eoc = irq_eoc;
>> +
> I think this belongs in a separate patch along with the adc5_exit
> function below.
I'll add a separate patch for this in the next post.
>
>> indio_dev->dev.parent = dev;
>> indio_dev->dev.of_node = node;
>> indio_dev->name = pdev->name;
>> indio_dev->modes = INDIO_DIRECT_MODE;
>> - indio_dev->info = &adc5_info;
>> indio_dev->channels = adc->iio_chans;
>> indio_dev->num_channels = adc->nchannels;
>>
>> return devm_iio_device_register(dev, indio_dev);
>> }
>>
>> +static int adc5_exit(struct platform_device *pdev)
>> +{
>> + struct adc5_chip *adc = platform_get_drvdata(pdev);
>> +
>> + if (adc->irq_eoc >= 0)
>> + disable_irq(adc->irq_eoc);
>> + return 0;
>> +}
>> +
>> static struct platform_driver adc5_driver = {
>> .driver = {
>> .name = "qcom-spmi-adc5.c",
>> .of_match_table = adc5_match_table,
>> },
>> .probe = adc5_probe,
>> + .remove = adc5_exit,
> This really belongs in a separate patch
I'll add a separate patch for this and the above changes mentioned in
the next post.
>
>> };
>> module_platform_driver(adc5_driver);
>>
>>
Powered by blists - more mailing lists