Re: [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability

From: Tomasz Figa
Date: Thu Jun 19 2014 - 20:25:25 EST

Next message: Greg Kroah-Hartman: "Re: [PATCH v2 2/2] staging: alarm-dev: Support to Compile as Module"
Previous message: Tomasz Figa: "Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC"
In reply to: Chanwoo Choi: "Re: [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability"
Next in thread: Chanwoo Choi: "[PATCHv4 4/4] ARM: dts: Fix wrong compatible string for Exynos3250 ADC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 20.06.2014 02:20, Chanwoo Choi wrote:
> 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@xxxxxxxxxxx>
>>> Acked-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>>> ---
>>> 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.

Sounds good, thanks.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Greg Kroah-Hartman: "Re: [PATCH v2 2/2] staging: alarm-dev: Support to Compile as Module"
Previous message: Tomasz Figa: "Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of ADC to support Exynos3250 ADC"
In reply to: Chanwoo Choi: "Re: [PATCHv4 1/4] iio: adc: exynos_adc: Add exynos_adc_ops structure to improve readability"
Next in thread: Chanwoo Choi: "[PATCHv4 4/4] ARM: dts: Fix wrong compatible string for Exynos3250 ADC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]