| 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: Fri, 20 Jun 2014 09:20:20 +0900
From: Chanwoo Choi <cw00.choi@...sung.com>
To: Tomasz Figa <tomasz.figa@...il.com>
Cc: jic23@...nel.org, ch.naveen@...sung.com, t.figa@...sung.com,
kgene.kim@...sung.com, robh+dt@...nel.org, pawel.moll@....com,
mark.rutland@....com, ijc+devicetree@...lion.org.uk,
galak@...eaurora.org, rdunlap@...radead.org,
sachin.kamat@...aro.org, linux-iio@...r.kernel.org,
linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure
to improve readability
Hi Tomasz,
On 06/18/2014 04:55 PM, Tomasz Figa wrote:
> Hi Chanwoo,
>
> On 18.06.2014 04:20, Chanwoo Choi wrote:
>> This patchset add 'exynos_adc_ops' structure which includes some functions
>> to control ADC operation according to ADC version (v1 or v2).
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@...sung.com>
>> ---
>> drivers/iio/adc/exynos_adc.c | 174 +++++++++++++++++++++++++++++--------------
>> 1 file changed, 120 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>> index 010578f..c30def6 100644
>> --- a/drivers/iio/adc/exynos_adc.c
>> +++ b/drivers/iio/adc/exynos_adc.c
>> @@ -90,6 +90,7 @@ struct exynos_adc {
>> struct clk *clk;
>> unsigned int irq;
>> struct regulator *vdd;
>> + struct exynos_adc_ops *ops;
>>
>> struct completion completion;
>>
>> @@ -97,6 +98,13 @@ struct exynos_adc {
>> unsigned int version;
>> };
>>
>> +struct exynos_adc_ops {
>> + void (*init_hw)(struct exynos_adc *info);
>> + void (*clear_irq)(struct exynos_adc *info);
>> + void (*start_conv)(struct exynos_adc *info, unsigned long addr);
>> + void (*stop_conv)(struct exynos_adc *info);
>> +};
>> +
>> static const struct of_device_id exynos_adc_match[] = {
>> { .compatible = "samsung,exynos-adc-v1", .data = (void *)ADC_V1 },
>> { .compatible = "samsung,exynos-adc-v2", .data = (void *)ADC_V2 },
>> @@ -112,30 +120,98 @@ static inline unsigned int exynos_adc_get_version(struct platform_device *pdev)
>> return (unsigned int)match->data;
>> }
>>
>> -static void exynos_adc_hw_init(struct exynos_adc *info)
>> +static void exynos_adc_v1_init_hw(struct exynos_adc *info)
>> +{
>> + u32 con1;
>> +
>> + /* set default prescaler values and Enable prescaler */
>> + con1 = ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
>> +
>> + /* Enable 12-bit ADC resolution */
>> + con1 |= ADC_V1_CON_RES;
>> + writel(con1, ADC_V1_CON(info->regs));
>> +}
>> +
>> +static void exynos_adc_v1_start_conv(struct exynos_adc *info, unsigned long addr)
>> +{
>> + u32 con1;
>> +
>> + writel(addr, ADC_V1_MUX(info->regs));
>> +
>> + con1 = readl(ADC_V1_CON(info->regs));
>> + writel(con1 | ADC_CON_EN_START, ADC_V1_CON(info->regs));
>> +}
>> +
>> +static void exynos_adc_v1_clear_irq(struct exynos_adc *info)
>> +{
>> + writel(1, ADC_V1_INTCLR(info->regs));
>> +}
>> +
>> +static void exynos_adc_v1_stop_conv(struct exynos_adc *info)
>> +{
>> + u32 con;
>> +
>> + con = readl(ADC_V1_CON(info->regs));
>> + con |= ADC_V1_CON_STANDBY;
>> + writel(con, ADC_V1_CON(info->regs));
>> +}
>> +
>> +static struct exynos_adc_ops exynos_adc_v1_ops = {
>> + .init_hw = exynos_adc_v1_init_hw,
>> + .clear_irq = exynos_adc_v1_clear_irq,
>> + .start_conv = exynos_adc_v1_start_conv,
>> + .stop_conv = exynos_adc_v1_stop_conv,
>> +};
>> +
>> +static void exynos_adc_v2_init_hw(struct exynos_adc *info)
>> {
>> u32 con1, con2;
>>
>> - if (info->version == ADC_V2) {
>> - con1 = ADC_V2_CON1_SOFT_RESET;
>> - writel(con1, ADC_V2_CON1(info->regs));
>> + con1 = ADC_V2_CON1_SOFT_RESET;
>> + writel(con1, ADC_V2_CON1(info->regs));
>>
>> - con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
>> - ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
>> - writel(con2, ADC_V2_CON2(info->regs));
>> + con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
>> + ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
>> + writel(con2, ADC_V2_CON2(info->regs));
>>
>> - /* Enable interrupts */
>> - writel(1, ADC_V2_INT_EN(info->regs));
>> - } else {
>> - /* set default prescaler values and Enable prescaler */
>> - con1 = ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
>> + /* Enable interrupts */
>> + writel(1, ADC_V2_INT_EN(info->regs));
>> +}
>>
>> - /* Enable 12-bit ADC resolution */
>> - con1 |= ADC_V1_CON_RES;
>> - writel(con1, ADC_V1_CON(info->regs));
>> - }
>> +static void exynos_adc_v2_start_conv(struct exynos_adc *info, unsigned long addr)
>> +{
>> + u32 con1, con2;
>> +
>> + con2 = readl(ADC_V2_CON2(info->regs));
>> + con2 &= ~ADC_V2_CON2_ACH_MASK;
>> + con2 |= ADC_V2_CON2_ACH_SEL(addr);
>> + writel(con2, ADC_V2_CON2(info->regs));
>> +
>> + con1 = readl(ADC_V2_CON1(info->regs));
>> + writel(con1 | ADC_CON_EN_START, ADC_V2_CON1(info->regs));
>> +}
>> +
>> +static void exynos_adc_v2_clear_irq(struct exynos_adc *info)
>> +{
>> + writel(1, ADC_V2_INT_ST(info->regs));
>> +}
>> +
>> +static void exynos_adc_v2_stop_conv(struct exynos_adc *info)
>> +{
>> + u32 con;
>> +
>> + con = readl(ADC_V2_CON1(info->regs));
>> + con &= ~ADC_CON_EN_START;
>> + writel(con, ADC_V2_CON1(info->regs));
>> }
>>
>> +static struct exynos_adc_ops exynos_adc_v2_ops = {
>> + .init_hw = exynos_adc_v2_init_hw,
>> + .start_conv = exynos_adc_v2_start_conv,
>> + .clear_irq = exynos_adc_v2_clear_irq,
>> + .stop_conv = exynos_adc_v2_stop_conv,
>> +};
>> +
>> static int exynos_read_raw(struct iio_dev *indio_dev,
>> struct iio_chan_spec const *chan,
>> int *val,
>> @@ -144,7 +220,6 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
>> {
>> struct exynos_adc *info = iio_priv(indio_dev);
>> unsigned long timeout;
>> - u32 con1, con2;
>> int ret;
>>
>> if (mask != IIO_CHAN_INFO_RAW)
>> @@ -154,28 +229,15 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
>> reinit_completion(&info->completion);
>>
>> /* Select the channel to be used and Trigger conversion */
>> - if (info->version == ADC_V2) {
>> - con2 = readl(ADC_V2_CON2(info->regs));
>> - con2 &= ~ADC_V2_CON2_ACH_MASK;
>> - con2 |= ADC_V2_CON2_ACH_SEL(chan->address);
>> - writel(con2, ADC_V2_CON2(info->regs));
>> -
>> - con1 = readl(ADC_V2_CON1(info->regs));
>> - writel(con1 | ADC_CON_EN_START,
>> - ADC_V2_CON1(info->regs));
>> - } else {
>> - writel(chan->address, ADC_V1_MUX(info->regs));
>> -
>> - con1 = readl(ADC_V1_CON(info->regs));
>> - writel(con1 | ADC_CON_EN_START,
>> - ADC_V1_CON(info->regs));
>> - }
>> + if (info->ops->start_conv)
>> + info->ops->start_conv(info, chan->address);
>>
>> timeout = wait_for_completion_timeout
>> (&info->completion, EXYNOS_ADC_TIMEOUT);
>> if (timeout == 0) {
>> dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
>> - exynos_adc_hw_init(info);
>> + if (info->ops->init_hw)
>> + info->ops->init_hw(info);
>> ret = -ETIMEDOUT;
>> } else {
>> *val = info->value;
>> @@ -193,13 +255,11 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
>> struct exynos_adc *info = (struct exynos_adc *)dev_id;
>>
>> /* Read value */
>> - info->value = readl(ADC_V1_DATX(info->regs)) &
>> - ADC_DATX_MASK;
>> + info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
>> +
>> /* clear irq */
>> - if (info->version == ADC_V2)
>> - writel(1, ADC_V2_INT_ST(info->regs));
>> - else
>> - writel(1, ADC_V1_INTCLR(info->regs));
>> + if (info->ops->clear_irq)
>> + info->ops->clear_irq(info);
>>
>> complete(&info->completion);
>>
>> @@ -277,6 +337,19 @@ static int exynos_adc_probe(struct platform_device *pdev)
>>
>> info = iio_priv(indio_dev);
>>
>> + info->version = exynos_adc_get_version(pdev);
>> + switch (info->version) {
>> + case ADC_V1:
>> + info->ops = &exynos_adc_v1_ops;
>> + break;
>> + case ADC_V2:
>> + info->ops = &exynos_adc_v2_ops;
>> + break;
>> + default:
>> + dev_err(&pdev->dev, "failed to identify ADC version\n");
>> + return -EINVAL;
>> + };
>
> Just drop the enum completely and assign the struct pointer directly to
> driver data fields in match tables. I also suspect that the struct
> should be "variant" not "ops", because it will also require other data
> than function pointers, e.g. "needs_sclk".
OK, I will use "variant" structure instead of "ops" and drop enum vairable of ADC version.
Best Regards,
Chanwoo Choi
--
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