| 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: Tue, 14 Oct 2014 12:36:48 +0300
From: Stanimir Varbanov <svarbanov@...sol.com>
To: Hartmut Knaack <knaack.h@....de>
CC: Ian Campbell <ijc+devicetree@...lion.org.uk>,
Pawel Moll <pawel.moll@....com>,
Rob Herring <robh+dt@...nel.org>,
Kumar Gala <galak@...eaurora.org>,
Mark Rutland <mark.rutland@....com>,
Grant Likely <grant.likely@...aro.org>,
Jonathan Cameron <jic23@...nel.org>,
Arnd Bergmann <arnd@...db.de>, linux-arm-msm@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Angelo Compagnucci <angelo.compagnucci@...il.com>,
Doug Anderson <dianders@...omium.org>,
Fugang Duan <B38611@...escale.com>,
Johannes Thumshirn <johannes.thumshirn@....de>,
Jean Delvare <jdelvare@...e.de>,
Philippe Reynes <tremyfr@...oo.fr>,
Lee Jones <lee.jones@...aro.org>,
Josh Cartwright <joshc@...eaurora.org>,
Stephen Boyd <sboyd@...eaurora.org>,
David Collins <collinsd@...eaurora.org>,
"Ivan T. Ivanov" <iivanov@...sol.com>
Subject: Re: [PATCH v3 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver
On 10/13/2014 01:07 AM, Hartmut Knaack wrote:
> Stanimir Varbanov schrieb am 24.09.2014 14:56:
>> The voltage ADC is peripheral of Qualcomm SPMI PMIC chips. It has
>> 15bits resolution and register space inside PMIC accessible across
>> SPMI bus.
>>
>> The vadc driver registers itself through IIO interface.
> Quite a lot of changes. Please see my comments inline.
>>
>> Signed-off-by: Stanimir Varbanov <svarbanov@...sol.com>
>> Signed-off-by: Ivan T. Ivanov <iivanov@...sol.com>
>> ---
>> drivers/iio/adc/Kconfig | 10 +
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/qcom-spmi-vadc.c | 1029 +++++++++++++++++++++++++
>> include/dt-bindings/iio/qcom,spmi-pmic-vadc.h | 119 +++
>> 4 files changed, 1159 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/iio/adc/qcom-spmi-vadc.c
>> create mode 100644 include/dt-bindings/iio/qcom,spmi-pmic-vadc.h
>>
<snip>
>> +
>> +static int vadc_reset(struct vadc_priv *vadc)
>> +{
>> + u8 data;
>> + int ret;
>> +
>> + ret = vadc_write(vadc, VADC_ACCESS, VADC_ACCESS_DATA);
>> + if (ret)
>> + return ret;
>> +
>> + ret = vadc_read(vadc, VADC_PERH_RESET_CTL3, &data);
>> + if (ret)
>> + return ret;
>> +
>> + ret = vadc_write(vadc, VADC_ACCESS, VADC_ACCESS_DATA);
>> + if (ret)
>> + return ret;
>> +
>> + data |= VADC_FOLLOW_WARM_RB;
>> +
>> + return vadc_write(vadc, VADC_PERH_RESET_CTL3, data);
>> +}
>> +
>> +static int vadc_enable(struct vadc_priv *vadc, bool state)
>> +{
>> + return vadc_write(vadc, VADC_EN_CTL1, state ? VADC_EN_CTL1_SET : 0);
>> +}
>> +
>> +#ifdef DEBUG
>> +static void vadc_show_status(struct vadc_priv *vadc)
>> +{
> You could also move the #ifdef in here...
>> + u8 mode, sta1, chan, dig, en, req;
>> + int ret;
>> +
>> + ret = vadc_read(vadc, VADC_MODE_CTL, &mode);
>> + if (ret)
>> + return;
>> +
>> + ret = vadc_read(vadc, VADC_ADC_DIG_PARAM, &dig);
>> + if (ret)
>> + return;
>> +
>> + ret = vadc_read(vadc, VADC_ADC_CH_SEL_CTL, &chan);
>> + if (ret)
>> + return;
>> +
>> + ret = vadc_read(vadc, VADC_CONV_REQ, &req);
>> + if (ret)
>> + return;
>> +
>> + ret = vadc_read(vadc, VADC_STATUS1, &sta1);
>> + if (ret)
>> + return;
>> +
>> + ret = vadc_read(vadc, VADC_EN_CTL1, &en);
>> + if (ret)
>> + return;
>> +
>> + dev_dbg(vadc->dev,
>> + "mode:%02x en:%02x chan:%02x dig:%02x req:%02x sta1:%02x\n",
>> + mode, en, chan, dig, req, sta1);
> ...and the #endif here. Saves 2 lines of code ;-)
I don't like #ifdefs in function body, it makes code unreadable at least
for me :)
>> +}
>> +#else
>> +static void vadc_show_status(struct vadc_priv *vadc) {}
>> +#endif
>> +
<snip>
>> +
>> +static int vadc_get_dt_channel_data(struct device *dev,
>> + struct vadc_channel_prop *prop,
>> + struct device_node *node)
>> +{
>> + const char *name = node->name;
>> + u32 chan, value, varr[2];
>> + int ret;
>> +
>> + ret = of_property_read_u32(node, "reg", &chan);
>> + if (ret) {
>> + dev_dbg(dev, "invalid channel number %s\n", name);
>> + return ret;
>> + }
>> +
>> + if (chan > VADC_CHAN_MAX || chan < VADC_CHAN_MIN) {
>> + dev_dbg(dev, "%s invalid channel number %d\n", name, chan);
>> + return -EINVAL;
>> + }
>> +
>> + /* the channel has DT description */
>> + prop->channel = chan;
>> +
>> + ret = of_property_read_u32(node, "qcom,decimation", &value);
>> + if (!ret) {
>> + ret = vadc_decimation_from_dt(value);
>> + if (ret < 0) {
>> + dev_dbg(dev, "%02x invalid decimation %d\n",
>> + chan, value);
>> + return ret;
>> + }
>> + prop->decimation = ret;
>> + } else {
>> + prop->decimation = VADC_DEF_DECIMATION;
>> + }
>> +
>> + ret = of_property_read_u32_array(node, "qcom,pre-scaling", varr, 2);
>> + if (!ret) {
>> + ret = vadc_prescaling_from_dt(varr[0], varr[1]);
>> + if (ret < 0) {
>> + dev_dbg(dev, "%02x invalid pre-scaling <%d %d>\n",
>> + chan, varr[0], varr[1]);
>> + return ret;
>> + }
>> + prop->prescale = ret;
>> + } else {
>> + prop->prescale = vadc_chans[prop->channel].prescale_index;
>> + }
>> +
>> + ret = of_property_read_u32(node, "qcom,hw-settle-time", &value);
>> + if (!ret) {
>> + ret = vadc_hw_settle_time_from_dt(value);
>> + if (ret < 0) {
>> + dev_dbg(dev, "%02x invalid hw-settle-time %d, us\n",
> The colon inside the string is probably an unintended leftover?
correct.
>> + chan, value);
>> + return ret;
>> + }
>> + prop->hw_settle_time = ret;
>> + } else {
>> + prop->hw_settle_time = VADC_DEF_HW_SETTLE_TIME;
>> + }
>> +
>> + ret = of_property_read_u32(node, "qcom,avg-samples", &value);
>> + if (!ret) {
>> + ret = vadc_avg_samples_from_dt(value);
>> + if (ret < 0) {
>> + dev_dbg(dev, "%02x invalid avg-samples %d\n",
>> + chan, value);
>> + return ret;
>> + }
>> + prop->avg_samples = ret;
>> + } else {
>> + prop->avg_samples = VADC_DEF_AVG_SAMPLES;
>> + }
>> +
>> + if (of_property_read_bool(node, "qcom,ratiometric"))
>> + prop->calibration = VADC_CALIB_RATIOMETRIC;
>> + else
>> + prop->calibration = VADC_CALIB_ABSOLUTE;
>> +
>> + dev_dbg(dev, "%02x name %s\n", chan, name);
>> +
>> + return 0;
>> +}
>> +
>> +static int vadc_get_dt_data(struct vadc_priv *vadc, struct device_node *node)
>> +{
>> + struct iio_chan_spec *iio_chan = vadc->iio_chans;
>> + const struct vadc_channels *vadc_chan;
>> + struct vadc_channel_prop prop;
>> + struct device_node *child;
>> + int ret, index = 0;
> index could be defined unsigned, as it should only carry unsigned values.
makes sense.
>> +
>> + for_each_available_child_of_node(node, child) {
>> + ret = vadc_get_dt_channel_data(vadc->dev, &prop, child);
>> + if (ret)
>> + return ret;
>> +
>> + vadc->chan_props[index] = prop;
>> +
>> + vadc_chan = &vadc_chans[prop.channel];
>> +
>> + iio_chan->channel = prop.channel;
>> + iio_chan->datasheet_name = vadc_chan->datasheet_name;
>> + iio_chan->info_mask_separate = vadc_chan->info_mask;
>> + iio_chan->type = vadc_chan->type;
>> + iio_chan->indexed = 1;
>> + iio_chan->scan_type.sign = 's';
>> + iio_chan->scan_type.realbits = 15;
>> + iio_chan->scan_type.storagebits = 16;
>> + iio_chan->address = index++;
>> +
>> + iio_chan++;
>> + }
>> +
>> + return 0;
>> +}
>> +
<snip>
>> +
>> +static int vadc_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *node = pdev->dev.of_node;
>> + struct device *dev = &pdev->dev;
>> + struct iio_dev *indio_dev;
>> + struct vadc_priv *vadc;
>> + struct resource *res;
>> + struct regmap *regmap;
>> + int ret, irq_eoc;
>> +
>> + regmap = dev_get_regmap(dev->parent, NULL);
>> + if (!regmap)
>> + return -ENODEV;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_REG, 0);
>> + if (!res)
>> + return -ENODEV;
>> +
>> + indio_dev = devm_iio_device_alloc(dev, sizeof(*vadc));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + vadc = iio_priv(indio_dev);
>> + vadc->regmap = regmap;
>> + vadc->dev = dev;
>> + vadc->base = res->start;
>> + vadc->are_ref_measured = false;
>> + init_completion(&vadc->complete);
>> +
>> + ret = vadc_check_revision(vadc);
>> + if (ret)
>> + return ret;
>> +
>> + vadc->nchannels = of_get_available_child_count(node);
>> + if (!vadc->nchannels)
>> + return -EINVAL;
>> +
>> + vadc->iio_chans = devm_kcalloc(dev, vadc->nchannels,
>> + sizeof(*vadc->iio_chans), GFP_KERNEL);
>> + if (!vadc->iio_chans)
>> + return -ENOMEM;
>> +
>> + vadc->chan_props = devm_kcalloc(dev, vadc->nchannels,
>> + sizeof(*vadc->chan_props), GFP_KERNEL);
>> + if (!vadc->chan_props)
>> + return -ENOMEM;
>> +
>> + ret = vadc_get_dt_data(vadc, node);
>> + if (ret)
>> + return ret;
>> +
>> + irq_eoc = platform_get_irq(pdev, 0);
>> + if (irq_eoc < 0) {
>> + if (irq_eoc == -EPROBE_DEFER || irq_eoc == -EINVAL)
>> + return irq_eoc;
>> + vadc->poll_eoc = true;
>> + }
>> +
> Isn't the part below actually the else section of the part above (and device_init_wakeup() belonging up there)?
you are right, might be I wanted to avoid indentation issues. Will see
how to make it better.
>> + if (!vadc->poll_eoc) {
>> + ret = devm_request_irq(dev, irq_eoc, vadc_isr,
>> + IRQF_TRIGGER_RISING, "spmi-vadc", vadc);
>> + if (!ret)
>> + enable_irq_wake(irq_eoc);
>> + else
>> + return ret;
> Usually this is done this way:
yes, I know just oversight it.
> if (ret)
> return ret;
>
> enable_irq_wake(irq_eoc);
>> + } else {
>> + device_init_wakeup(vadc->dev, true);
>> + }
>> +
>> + ret = vadc_reset(vadc);
>> + if (ret) {
>> + dev_dbg(dev, "reset failed\n");
>> + return ret;
>> + }
>> +
>> + platform_set_drvdata(pdev, vadc);
> Why bother to call platform_set_drvdata()?
Seems leftover, will delete.
--
regards,
Stan
--
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