On 06/02/17 12:19, liurenzhong wrote:
> Hi Jonathan,
Hi Allen,
>
> Thanks for your suggestion, I'm sorry to reply after a long time .
> This is some problems ask for your advice:
Not to worry on time, we are all busy people. I've had patches that have
taken me literally years to reply to reviews on ;) (there's one I posted
just yesterday after 3 years.)
One small procedural point. It's always helpful to reply inline within
the patch. To be honest I couldn't remember what we were referring to in
quite a bit of this, so the context is helpful! (easy enough to follow
here, but it's good practice that helps a lot when things get more complex).
>
> 1, It's nice to use just "reset/reset.txt" or ".../reset/reset.txt"
> from Rob's suggestion, but in the filtered DT tree,
> "../reset/reset.txt" is used more times, which one we should get,
> "../" or ".../"?
Go with Rob's view on this. Device tree bindings are his (and Mark's)
domain. If he changes his mind then that's fine as well.
>
> 2, The ADC on hisilicon BVT socs can work in single scanning mode or
> continuous scanning mode. The single mode get conversion value one
> time after start the configure, while the continuous scanning mode
> will get conversion value each scan time after start the configure
> while stopping the adc configure. For more expansibility, the ADC
> driver use the continuous scanning mode and stop the adc configure
> after get one time conversion value. Is it necessary to change our
> driver to single scanning mode or just add a short description?
I think a short description would be fine. Perhaps as short as:
'A single cycle of continuous mode capture is used for polled operation.
This stops continuous mode after the cycle is complete.'
or something like that.
It sounds like this continuous sampling functionality will lend itself
nicely to the addition of a streaming (called buffered in iio) mode
in the driver as a possible future addition - but that certainly isn't
necessary to have a useful driver.
> 3, This drvier is only support ADC IF found on hi3516cv300 and
> hi3519v101 now , and future SoCs from Hisilicon BVT would get some
> changes on channel number or getting data ways and so on, so the
> driver use "V1" on match table and add other version while any change
> happens. Which match table can we provide and is't OK like this?
>> +static const struct of_device_id hibvt_lsadc_match[] = {
>> + {
>> + .compatible = "hisilicon,hi3516cv300-lsadc",
>> + .data = &lsadc_data_v1,
>> + },
>> + {
>> + .compatible = "hisilicon,hi3519v101-lsadc",
>> + .data = &lsadc_data_v1,
>> + },
>> + {},
>> +};
That's absolutely fine though I would like a prefix on the lsadc part as
it is generic enough that it's just possible it'll clash with a name in
some header in the future. The hibvt prefix you have used elsewhere
would be good for this.
.data = &hibvt_lsadc_data_v1,
Also remember that you can always change internal naming in future if a
different naming scheme makes more sense at that time.
You could (for now) drop the data_v1 structure entirely and leave
allowing for other variants as an exercise to be done when they show up.
e.g.:
1) Patch with no variant support is applied.
... some time passes...
2) Patch to add variant support
3) Patch to add new variants.
2, 3 in a series together as nice simple steps with the use of patch 2
becoming clear in patch 3.
This is a common thing to have with drivers as the hardware evolves.
Thanks and looking forward to v2,
Jonathan
>
> Awaiting for your reply Thank you very much!
>
> Best regards
> /Allen
>
> -----邮件原件-----
> 发件人: Jonathan Cameron [mailto:jic23@xxxxxxxxxx]
> 发送时间: 2017年1月8日 1:52
> 收件人: liurenzhong <liurenzhong@xxxxxxxxxxxxx>; knaack.h@xxxxxx; lars@xxxxxxxxxx; pmeerw@xxxxxxxxxx; robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx
> 抄送: akinobu.mita@xxxxxxxxx; ludovic.desroches@xxxxxxxxx; krzk@xxxxxxxxxx; vilhelm.gray@xxxxxxxxx; ksenija.stanojevic@xxxxxxxxx; zhiyong.tao@xxxxxxxxxxxx; daniel.baluta@xxxxxxxxx; leonard.crestez@xxxxxxxxx; ray.jui@xxxxxxxxxxxx; raveendra.padasalagi@xxxxxxxxxxxx; mranostay@xxxxxxxxx; amsfield22@xxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Xuejiancheng <xuejiancheng@xxxxxxxxxxxxx>; Lixu (kevin) <kevin.lixu@xxxxxxxxxxxxx>
> 主题: Re: [PATCH] adc: add adc driver for Hisilicon BVT SOCs
>
> On 07/01/17 05:16, Allen Liu wrote:
>> Add ADC driver for the ADC controller found on HiSilicon BVT SOCs, like Hi3516CV300, etc.
>> The ADC controller is primarily in charge of detecting voltage.
>>
>> Reviewed-by: Kevin Li <kevin.lixu@xxxxxxxxxxxxx>
>> Signed-off-by: Allen Liu <liurenzhong@xxxxxxxxxxxxx>
> Hi Allen,
>
> One quick submission process note first. It is very important to clearly identify new versions of a patch and what changes have occurred since the previous posting.
>
> So the email title should have been [PATCH V2] adc...
>
> Also, below the --- please add a brief change log.
>
> The driver is coming together nicely. A few minor points inline.
>
> Jonathan
>> ---
>> .../devicetree/bindings/iio/adc/hibvt-lsadc.txt | 23 ++
>> drivers/iio/adc/Kconfig | 10 +
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/hibvt_lsadc.c | 335 +++++++++++++++++++++
>> 4 files changed, 369 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/iio/adc/hibvt-lsadc.txt
>> create mode 100644 drivers/iio/adc/hibvt_lsadc.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/hibvt-lsadc.txt
>> b/Documentation/devicetree/bindings/iio/adc/hibvt-lsadc.txt
>> new file mode 100644
>> index 0000000..fce1ff4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/hibvt-lsadc.txt
>> @@ -0,0 +1,23 @@
>> +Hisilicon BVT Low Speed (LS) A/D Converter bindings
>> +
>> +Required properties:
>> +- compatible: should be "hisilicon,<name>-lsadc"
>> + - "hisilicon,hi3516cv300-lsadc": for hi3516cv300
>> +
>> +- reg: physical base address of the controller and length of memory mapped
>> + region.
>> +- interrupts: The interrupt number for the ADC device.
> Ideally refer to the standard interrupt binding document.
> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>
>> +
>> +Optional properties:
>> +- resets: Must contain an entry for each entry in reset-names if need support
>> + this option. See ../../reset/reset.txt for details.
> Don't use a relative path in a binding document. It's far too likely to be broken by a reorganization of the docs and cannot be grepped for.
>> +- reset-names: Must include the name "lsadc-crg".
>> +
>> +Example:
>> + adc: adc@120e0000 {
>> + compatible = "hisilicon,hi3516cv300-lsadc";
>> + reg = <0x120e0000 0x1000>;
>> + interrupts = <19>;
>> + resets = <&crg 0x7c 3>;
>> + reset-names = "lsadc-crg";
>> + };
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index
>> 99c0514..0443f51 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -225,6 +225,16 @@ config HI8435
>> This driver can also be built as a module. If so, the module will be
>> called hi8435.
>>
>> +config HIBVT_LSADC
>> + tristate "HIBVT LSADC driver"
>> + depends on ARCH_HISI || COMPILE_TEST
>> + help
>> + Say yes here to build support for the LSADC found in SoCs from
>> + hisilicon BVT chip.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called hibvt_lsadc.
>> +
>> config INA2XX_ADC
>> tristate "Texas Instruments INA2xx Power Monitors IIO driver"
>> depends on I2C && !SENSORS_INA2XX
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index
>> 7a40c04..6554d92 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -23,6 +23,7 @@ obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
>> obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>> obj-$(CONFIG_FSL_MX25_ADC) += fsl-imx25-gcq.o
>> obj-$(CONFIG_HI8435) += hi8435.o
>> +obj-$(CONFIG_HIBVT_LSADC) += hibvt_lsadc.o
>> obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
>> obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
>> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o diff --git
>> a/drivers/iio/adc/hibvt_lsadc.c b/drivers/iio/adc/hibvt_lsadc.c new
>> file mode 100644 index 0000000..aaf2024
>> --- /dev/null
>> +++ b/drivers/iio/adc/hibvt_lsadc.c
>> @@ -0,0 +1,335 @@
>> +/*
>> + * Hisilicon BVT Low Speed (LS) A/D Converter
>> + * Copyright (C) 2016 HiSilicon Technologies Co., Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> +modify
>> + * it under the terms of the GNU General Public License as published
>> +by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/clk.h>
>> +#include <linux/completion.h>
>> +#include <linux/delay.h>
>> +#include <linux/reset.h>
>> +#include <linux/regulator/consumer.h> #include <linux/iio/iio.h>
>> +
>> +/* hisilicon bvt adc registers definitions */
>> +#define HIBVT_LSADC_CONFIG 0x00
>> +#define HIBVT_CONFIG_DEGLITCH BIT(17)
>> +#define HIBVT_CONFIG_RESET BIT(15)
>> +#define HIBVT_CONFIG_POWERDOWN BIT(14)
>> +#define HIBVT_CONFIG_MODE BIT(13)
>> +#define HIBVT_CONFIG_CHNC BIT(10)
>> +#define HIBVT_CONFIG_CHNB BIT(9)
>> +#define HIBVT_CONFIG_CHNA BIT(8)
>> +
>> +#define HIBVT_LSADC_TIMESCAN 0x08
>> +#define HIBVT_LSADC_INTEN 0x10
>> +#define HIBVT_LSADC_INTSTATUS 0x14
>> +#define HIBVT_LSADC_INTCLR 0x18
>> +#define HIBVT_LSADC_START 0x1C
>> +#define HIBVT_LSADC_STOP 0x20
>> +#define HIBVT_LSADC_ACTBIT 0x24
>> +#define HIBVT_LSADC_CHNDATA 0x2C
>> +
>> +#define HIBVT_LSADC_CON_EN (1u << 0)
>> +#define HIBVT_LSADC_CON_DEN (0u << 0)
>> +
>> +#define HIBVT_LSADC_NUM_BITS_V1 10
>> +#define HIBVT_LSADC_CHN_MASK_v1 0x7
>> +
>> +/* fix clk:3000000, default tscan set 10ms */
>> +#define HIBVT_LSADC_TSCAN_MS (10*3000)
>> +
>> +#define HIBVT_LSADC_TIMEOUT msecs_to_jiffies(100)
>> +
>> +/* default voltage scale for every channel <mv> */ static int
>> +g_hibvt_lsadc_voltage[] = {
>> + 3300, 3300, 3300
> Is default due to an external reference voltage or is there an internal regulator? If it is external it should really be described using the regulator framework.
>
> Const?
>> +};
>> +
>> +struct hibvt_lsadc {
>> + void __iomem *regs;
>> + struct completion completion;
>> + struct reset_control *reset;
>> + const struct hibvt_lsadc_data *data;
>> + unsigned int cur_chn;
>> + unsigned int value;
>> +};
>> +
>> +struct hibvt_lsadc_data {
>> + int num_bits;
>> + const struct iio_chan_spec *channels;
>> + int num_channels;
>> +
>> + void (*clear_irq)(struct hibvt_lsadc *info, int mask);
>> + void (*start_conv)(struct hibvt_lsadc *info);
>> + void (*stop_conv)(struct hibvt_lsadc *info); };
>> +
>> +static int hibvt_lsadc_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val, int *val2, long mask) {
>> + struct hibvt_lsadc *info = iio_priv(indio_dev);
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + mutex_lock(&indio_dev->mlock);
>> +
>> + reinit_completion(&info->completion);
>> +
>> + /* Select the channel to be used */
>> + info->cur_chn = chan->channel;
>> +
>> + if (info->data->start_conv)
>> + info->data->start_conv(info);
>> +
>> + if (!wait_for_completion_timeout(&info->completion,
>> + HIBVT_LSADC_TIMEOUT)) {
>> + if (info->data->stop_conv)
>> + info->data->stop_conv(info);
>> + mutex_unlock(&indio_dev->mlock);
>> + return -ETIMEDOUT;
>> + }
>> +
>> + *val = info->value;
>> + mutex_unlock(&indio_dev->mlock);
>> + return IIO_VAL_INT;
>> + case IIO_CHAN_INFO_SCALE:
>> + *val = g_hibvt_lsadc_voltage[chan->channel];
>> + *val2 = info->data->num_bits;
>> + return IIO_VAL_FRACTIONAL_LOG2;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static irqreturn_t hibvt_lsadc_isr(int irq, void *dev_id) {
>> + struct hibvt_lsadc *info = (struct hibvt_lsadc *)dev_id;
>> + int mask;
>> +
>> + mask = readl(info->regs + HIBVT_LSADC_INTSTATUS);
>> + if ((mask & HIBVT_LSADC_CHN_MASK_v1) == 0)
>> + return IRQ_NONE;
>> +
>> + /* Clear irq */
>> + mask &= HIBVT_LSADC_CHN_MASK_v1;
>> + if (info->data->clear_irq)
>> + info->data->clear_irq(info, mask);
>> +
>> + /* Read value */
>> + info->value = readl(info->regs +
>> + HIBVT_LSADC_CHNDATA + (info->cur_chn << 2));
>> + info->value &= GENMASK(info->data->num_bits - 1, 0);
>> +
>> + /* stop adc */
>> + if (info->data->stop_conv)
>> + info->data->stop_conv(info);
>> +
>> + complete(&info->completion);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static const struct iio_info hibvt_lsadc_iio_info = {
>> + .read_raw = hibvt_lsadc_read_raw,
>> + .driver_module = THIS_MODULE,
>> +};
>> +
>> +#define HIBVT_LSADC_CHANNEL(_index, _id) { \
>> + .type = IIO_VOLTAGE, \
>> + .indexed = 1, \
>> + .channel = _index, \
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>> + BIT(IIO_CHAN_INFO_SCALE), \
>> + .datasheet_name = _id, \
>> +}
>> +
>> +static const struct iio_chan_spec hibvt_lsadc_iio_channels[] = {
>> + HIBVT_LSADC_CHANNEL(0, "adc0"),
>> + HIBVT_LSADC_CHANNEL(1, "adc1"),
>> + HIBVT_LSADC_CHANNEL(2, "adc2"),
>> +};
>> +
>> +static void hibvt_lsadc_v1_clear_irq(struct hibvt_lsadc *info, int
>> +mask) {
>> + writel(mask, info->regs + HIBVT_LSADC_INTCLR); }
>> +
>> +static void hibvt_lsadc_v1_start_conv(struct hibvt_lsadc *info) {
>> + unsigned int con;
>> +
>> + /* set number bit */
> set number of bits?
>> + con = GENMASK(info->data->num_bits - 1, 0);
>> + writel(con, (info->regs + HIBVT_LSADC_ACTBIT));
>> +
>> + /* config */
>> + con = readl(info->regs + HIBVT_LSADC_CONFIG);
>> + con &= ~HIBVT_CONFIG_RESET;
>> + con |= (HIBVT_CONFIG_POWERDOWN | HIBVT_CONFIG_DEGLITCH |
>> + HIBVT_CONFIG_MODE);
>> + con &= ~(HIBVT_CONFIG_CHNA | HIBVT_CONFIG_CHNB | HIBVT_CONFIG_CHNC);
>> + con |= (HIBVT_CONFIG_CHNA << info->cur_chn);
>> + writel(con, (info->regs + HIBVT_LSADC_CONFIG));
>> +
>> + /* set timescan */
>> + writel(HIBVT_LSADC_TSCAN_MS, (info->regs + HIBVT_LSADC_TIMESCAN));
>> +
>> + /* clear interrupt */
>> + writel(HIBVT_LSADC_CHN_MASK_v1, info->regs + HIBVT_LSADC_INTCLR);
>> +
>> + /* enable interrupt */
>> + writel(HIBVT_LSADC_CON_EN, (info->regs + HIBVT_LSADC_INTEN));
>> +
>> + /* start scan */
>> + writel(HIBVT_LSADC_CON_EN, (info->regs + HIBVT_LSADC_START)); }
>> +
>> +static void hibvt_lsadc_v1_stop_conv(struct hibvt_lsadc *info) {
>> + /* reset the timescan */
> This isn't a particularly common pice of terminology, perhaps a short
> description here of what timescan is and why we should reset it would
> make the code easier to follow.
>
>> + writel(HIBVT_LSADC_CON_DEN, (info->regs + HIBVT_LSADC_TIMESCAN));
>> +
>> + /* disable interrupt */
>> + writel(HIBVT_LSADC_CON_DEN, (info->regs + HIBVT_LSADC_INTEN));
>> +
>> + /* stop scan */
>> + writel(HIBVT_LSADC_CON_EN, (info->regs + HIBVT_LSADC_STOP)); }
>> +
>> +static const struct hibvt_lsadc_data lsadc_data_v1 = {
>> + .num_bits = HIBVT_LSADC_NUM_BITS_V1,
>> + .channels = hibvt_lsadc_iio_channels,
>> + .num_channels = ARRAY_SIZE(hibvt_lsadc_iio_channels),
>> +
>> + .clear_irq = hibvt_lsadc_v1_clear_irq,
>> + .start_conv = hibvt_lsadc_v1_start_conv,
>> + .stop_conv = hibvt_lsadc_v1_stop_conv, };
>> +
>> +static const struct of_device_id hibvt_lsadc_match[] = {
>> + {
>> + .compatible = "hisilicon,hi3516cv300-lsadc",
>> + .data = &lsadc_data_v1,
> The usual convention is to only introduce 'variant' type data as a precursor patch to a series including the support of new parts.
>
> It is acceptable to post a version with this in if you are shortly to submit the follow up that adds other device support. If you are doing this, please put a note in the patch description to that effect. Note that if the additional support doesn't turn up, the driver may we get 'simplified'
> by someone else.
>
> I'd also generally expect to see this match table further down - directly above where it is used. Makes for ever so slightly easier reviewing!
>> + },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, hibvt_lsadc_match);
>> +
>> +/* Reset LSADC Controller */
>> +static void hibvt_lsadc_reset_controller(struct reset_control *reset)
>> +{
>> + reset_control_assert(reset);
>> + usleep_range(10, 20);
>> + reset_control_deassert(reset);
>> +}
>> +
>> +static int hibvt_lsadc_probe(struct platform_device *pdev) {
>> + struct hibvt_lsadc *info = NULL;
>> + struct device_node *np = pdev->dev.of_node;
>> + struct iio_dev *indio_dev = NULL;
>> + struct resource *mem;
>> + const struct of_device_id *match;
>> + int ret;
>> + int irq;
>> +
>> + if (!np)
>> + return -ENODEV;
>> +
>> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
>> + if (!indio_dev) {
>> + dev_err(&pdev->dev, "failed allocating iio device\n");
>> + return -ENOMEM;
>> + }
>> + info = iio_priv(indio_dev);
>> +
>> + match = of_match_device(hibvt_lsadc_match, &pdev->dev);
>> + info->data = match->data;
>> +
>> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + info->regs = devm_ioremap_resource(&pdev->dev, mem);
>> + if (IS_ERR(info->regs))
>> + return PTR_ERR(info->regs);
>> +
>> + /*
>> + * The reset should be an optional property, as it should work
>> + * with old devicetrees as well
>> + */
>> + info->reset = devm_reset_control_get(&pdev->dev, "lsadc-crg");
>> + if (IS_ERR(info->reset)) {
>> + ret = PTR_ERR(info->reset);
>> + if (ret != -ENOENT)
>> + return ret;
>> +
>> + dev_dbg(&pdev->dev, "no reset control found\n");
>> + info->reset = NULL;
>> + }
>> +
>> + init_completion(&info->completion);
>> +
>> + irq = platform_get_irq(pdev, 0);
>> + if (irq < 0) {
>> + dev_err(&pdev->dev, "no irq resource?\n");
>> + return irq;
>> + }
>> +
>> + ret = devm_request_irq(&pdev->dev, irq, hibvt_lsadc_isr,
>> + 0, dev_name(&pdev->dev), info);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "failed requesting irq %d\n", irq);
>> + return ret;
>> + }
>> +
>> + if (info->reset)
>> + hibvt_lsadc_reset_controller(info->reset);
>> +
>> + platform_set_drvdata(pdev, indio_dev);
>> +
>> + indio_dev->name = dev_name(&pdev->dev);
>> + indio_dev->dev.parent = &pdev->dev;
>> + indio_dev->dev.of_node = pdev->dev.of_node;
>> + indio_dev->info = &hibvt_lsadc_iio_info;
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> + indio_dev->channels = info->data->channels;
>> + indio_dev->num_channels = info->data->num_channels;
>> +
>> + ret = devm_iio_device_register(&pdev->dev, indio_dev);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "failed register iio device\n");
>> + return ret;
> Drop this return ret and just return ret instead of the return 0 below.
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static struct platform_driver hibvt_lsadc_driver = {
>> + .probe = hibvt_lsadc_probe,
>> + .driver = {
>> + .name = "hibvt-lsadc",
>> + .of_match_table = hibvt_lsadc_match,
>> + },
>> +};
>> +
>> +module_platform_driver(hibvt_lsadc_driver);
>> +
>> +MODULE_AUTHOR("Allen Liu <liurenzhong@xxxxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("hisilicon BVT LSADC driver"); MODULE_LICENSE("GPL
>> +v2");
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
