Subject: Re: [U-Boot,1/8] adc: Add driver for Rockchip Saradc
From: "David.Wu" <[email-masked]>
To: Philipp Tomsich <[email-masked]>
Cc: [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked]
Date: Thu, 14 Sep 2017 19:10:57 +0800

Hi Philipp,

在 2017/9/14 4:40, Philipp Tomsich 写道:
> 
> 
> On Wed, 13 Sep 2017, David Wu wrote:
> 
>> The ADC can support some channels signal-ended some bits Successive 
>> Approximation
>> Register (SAR) A/D Converter, like 6-channel and 10-bit. It converts 
>> the analog
>> input signal into some bits binary digital codes.
>>
>> Signed-off-by: David Wu <[email-masked]>
> 
> Reviewed-by: Philipp Tomsich <[email-masked]>
> 
> Please see below for requested changes.
> 
>> ---
>> drivers/adc/Kconfig           |   9 ++
>> drivers/adc/Makefile          |   1 +
>> drivers/adc/rockchip-saradc.c | 188 
>> ++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 198 insertions(+)
>> create mode 100644 drivers/adc/rockchip-saradc.c
>>
>> diff --git a/drivers/adc/Kconfig b/drivers/adc/Kconfig
>> index e5335f7..830fe0f 100644
>> --- a/drivers/adc/Kconfig
>> +++ b/drivers/adc/Kconfig
>> @@ -20,6 +20,15 @@ config ADC_EXYNOS
>>       - 12-bit resolution
>>       - 600 KSPS of sample rate
>>
>> +config SARADC_ROCKCHIP
>> +    bool "Enable Rockchip SARADC driver"
>> +    help
>> +      This enables driver for Rockchip SARADC.
>> +      It provides:
>> +      - 2~6 analog input channels
>> +      - 1O-bit resolution
>> +      - 1MSPS of sample rate
>> +
>> config ADC_SANDBOX
>>     bool "Enable Sandbox ADC test driver"
>>     help
>> diff --git a/drivers/adc/Makefile b/drivers/adc/Makefile
>> index cebf26d..4b5aa69 100644
>> --- a/drivers/adc/Makefile
>> +++ b/drivers/adc/Makefile
>> @@ -8,3 +8,4 @@
>> obj-$(CONFIG_ADC) += adc-uclass.o
>> obj-$(CONFIG_ADC_EXYNOS) += exynos-adc.o
>> obj-$(CONFIG_ADC_SANDBOX) += sandbox.o
>> +obj-$(CONFIG_SARADC_ROCKCHIP) += rockchip-saradc.o
> 
> Do you feel strongly about the "SARADC_ROCKCHIP" or would "ADC_ROCKCHIP" 
> be correct as well?  I don't care either way, but this is the first 
> entry here that does not start with CONFIG_ADC_, so I am wondering...

Yes, we have other adc IPs, like tsadc,vsadc...
So i think SARADC_ROCKCHIP is better.

> 
>> diff --git a/drivers/adc/rockchip-saradc.c 
>> b/drivers/adc/rockchip-saradc.c
>> new file mode 100644
>> index 0000000..5c7c3d9
>> --- /dev/null
>> +++ b/drivers/adc/rockchip-saradc.c
>> @@ -0,0 +1,188 @@
>> +/*
>> + * (C) Copyright 2017, Fuzhou Rockchip Electronics Co., Ltd
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + *
>> + * Rockchip Saradc driver for U-Boot
> 
> Should this be SARADC (all uppercase)?

Okay, use SARADC.

> 
>> + */
>> +
>> +#include <asm/io.h>
>> +#include <clk.h>
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <errno.h>
>> +#include <adc.h>
> 
> Please see https://www.denx.de/wiki/U-Boot/CodingStyle for the include 
> order. Please revise.
> 
> @Simon: yes, I am quick study ;-)
> 
>> +
>> +#define SARADC_DATA            0x00
>> +
>> +#define SARADC_STAS            0x04
> 
> Please see https://www.denx.de/wiki/U-Boot/CodingStyle for the 
> guidelines on using structures for I/O accesses.  Please revise.
> 
>> +#define SARADC_STAS_BUSY        BIT(0)
>> +
>> +#define SARADC_CTRL            0x08
>> +#define SARADC_CTRL_POWER_CTRL        BIT(3)
>> +#define SARADC_CTRL_CHN_MASK        0x7
>> +#define SARADC_CTRL_IRQ_STATUS        BIT(6)
>> +#define SARADC_CTRL_IRQ_ENABLE        BIT(5)
>> +
>> +#define SARADC_DLY_PU_SOC        0x0c
>> +
>> +#define SARADC_TIMEOUT            (100 * 1000)
>> +
>> +struct rockchip_saradc_data {
>> +    int                num_bits;
>> +    int                num_channels;
>> +    unsigned long            clk_rate;
>> +};
>> +
>> +struct rockchip_saradc_priv {
>> +    fdt_addr_t                regs;
>> +    int                     active_channel;
>> +    const struct rockchip_saradc_data    *data;
>> +};
>> +
>> +int rockchip_saradc_channel_data(struct udevice *dev, int channel,
>> +                unsigned int *data)
>> +{
>> +    struct rockchip_saradc_priv *priv = dev_get_priv(dev);
>> +
>> +    if (channel != priv->active_channel) {
>> +        error("Requested channel is not active!");
>> +        return -EINVAL;
>> +    }
>> +
>> +    if ((readl(priv->regs + SARADC_CTRL) & SARADC_CTRL_IRQ_STATUS) != 
>> SARADC_CTRL_IRQ_STATUS)
>> +        return -EBUSY;
>> +
>> +    /* Read value */
>> +    *data = readl(priv->regs + SARADC_DATA);
>> +    *data &= (1 << priv->data->num_bits) - 1;
> 
> This recomputes the data_mask (from below).
> Can we just use the data_mask again?

yes，use data_mask is ok.

> 
>> +
>> +    /* Power down adc */
>> +    writel(0, priv->regs + SARADC_CTRL);
>> +
>> +    return 0;
>> +}
>> +
>> +int rockchip_saradc_start_channel(struct udevice *dev, int channel)
>> +{
>> +    struct rockchip_saradc_priv *priv = dev_get_priv(dev);
>> +
>> +    if (channel < 0 || channel >= priv->data->num_channels) {
>> +        error("Requested channel is invalid!");
>> +        return -EINVAL;
>> +    }
>> +
>> +    /* 8 clock periods as delay between power up and start cmd */
>> +    writel(8, priv->regs + SARADC_DLY_PU_SOC);
>> +
>> +    /* Select the channel to be used and trigger conversion */
>> +    writel(SARADC_CTRL_POWER_CTRL
>> +            | (channel & SARADC_CTRL_CHN_MASK) | SARADC_CTRL_IRQ_ENABLE,
> 
> This line does not match the style guideline: it is too wide and the 
> operator should be before the line-break.
> 
> Did you run checkpatch or use patman?

maybe lost here 😃.

> 
>> +           priv->regs + SARADC_CTRL);
>> +
>> +    priv->active_channel = channel;
>> +
>> +    return 0;
>> +}
>> +
>> +int rockchip_saradc_stop(struct udevice *dev)
>> +{
>> +    struct rockchip_saradc_priv *priv = dev_get_priv(dev);
>> +
>> +    /* Power down adc */
>> +    writel(0, priv->regs + SARADC_CTRL);
>> +
>> +    priv->active_channel = -1;
>> +
>> +    return 0;
>> +}
>> +
>> +int rockchip_saradc_probe(struct udevice *dev)
>> +{
>> +    struct rockchip_saradc_priv *priv = dev_get_priv(dev);
>> +    struct clk clk;
>> +    int ret;
>> +
>> +    ret = clk_get_by_index(dev, 0, &clk);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = clk_set_rate(&clk, priv->data->clk_rate);
>> +    if (IS_ERR_VALUE(ret))
>> +        return ret;
>> +
>> +    priv->active_channel = -1;
>> +
>> +    return 0;
>> +}
>> +
>> +int rockchip_saradc_ofdata_to_platdata(struct udevice *dev)
>> +{
>> +    struct adc_uclass_platdata *uc_pdata = dev_get_uclass_platdata(dev);
>> +    struct rockchip_saradc_priv *priv = dev_get_priv(dev);
>> +    struct rockchip_saradc_data *data =
>> +                    (struct rockchip_saradc_data 
>> *)dev_get_driver_data(dev);
>> +
>> +    priv->regs = devfdt_get_addr(dev);
> 
> Shouldn't this be dev_read_addr?

okay

> 
>> +    if (priv->regs == FDT_ADDR_T_NONE) {
>> +        error("Dev: %s - can't get address!", dev->name);
>> +        return -ENODATA;
>> +    }
>> +
>> +    priv->data = data;
>> +    uc_pdata->data_mask = (1 << priv->data->num_bits) - 1;;
>> +    uc_pdata->data_format = ADC_DATA_FORMAT_BIN;
>> +    uc_pdata->data_timeout_us = SARADC_TIMEOUT / 5;
>> +    uc_pdata->channel_mask = (1 << priv->data->num_channels) - 1;
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct adc_ops rockchip_saradc_ops = {
>> +    .start_channel = rockchip_saradc_start_channel,
>> +    .channel_data = rockchip_saradc_channel_data,
>> +    .stop = rockchip_saradc_stop,
>> +};
>> +
>> +static const struct rockchip_saradc_data saradc_data = {
>> +    .num_bits = 10,
>> +    .num_channels = 3,
>> +    .clk_rate = 1000000,
>> +};
>> +
>> +static const struct rockchip_saradc_data rk3066_tsadc_data = {
>> +    .num_bits = 12,
>> +    .num_channels = 2,
>> +    .clk_rate = 50000,
>> +};
>> +
>> +static const struct rockchip_saradc_data rk3399_saradc_data = {
>> +    .num_bits = 10,
>> +    .num_channels = 6,
>> +    .clk_rate = 1000000,
>> +};
>> +
>> +static const struct udevice_id rockchip_saradc_ids[] = {
>> +    {
>> +        .compatible = "rockchip,saradc",
>> +        .data = (ulong)&saradc_data,
>> +    },
>> +    {
>> +        .compatible = "rockchip,rk3066-tsadc",
>> +        .data = (ulong)&rk3066_tsadc_data,
>> +    }, {
>> +        .compatible = "rockchip,rk3399-saradc",
>> +        .data = (ulong)&rk3399_saradc_data,
>> +    },
>> +    { }
>> +};
>> +
>> +U_BOOT_DRIVER(rockchip_saradc) = {
>> +    .name        = "rockchip_saradc",
>> +    .id        = UCLASS_ADC,
>> +    .of_match    = rockchip_saradc_ids,
>> +    .ops        = &rockchip_saradc_ops,
>> +    .probe        = rockchip_saradc_probe,
>> +    .ofdata_to_platdata = rockchip_saradc_ofdata_to_platdata,
>> +    .priv_auto_alloc_size = sizeof(struct rockchip_saradc_priv),
>> +};
>>
> 
> 
>