Re: [PATCH v3 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Lee,

Thank you for the review.

On Tue, Jun 28, 2016 at 6:28 PM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> On Fri, 24 Jun 2016, Ksenija Stanojevic wrote:
>
>> Add core files for mxs-lradc MFD driver.
>>
>> Note:  this patch won't compile in iio/testing without this patch:
>> a8f447be8056 ("mfd: Add resource managed APIs for mfd_add_devices")
>>
>> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@xxxxxxxxx>
>> Acked-by: Jonathan Cameron <jic23@xxxxxxxxxx>
>> ---
>> Changes in v3:
>>  - add note to commit message
>>  - move switch statement into if(touch_ret == 0) branch
>>  - add MODULE_AUTHOR
>>
>> Changes in v2:
>>  - do not change spacing in Kconfig
>>  - make struct mfd_cell part of struct mxs_lradc
>>  - use switch instead of if in mxs_lradc_irq_mask
>>  - use only necessary header files in mxs_lradc.h
>>  - use devm_mfd_add_device
>>  - use separate function to register mfd device
>>  - change licence to GPL
>>  - add copyright
>>
>>  drivers/mfd/Kconfig           |  17 ++++
>>  drivers/mfd/Makefile          |   1 +
>>  drivers/mfd/mxs-lradc.c       | 217 ++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mfd/mxs-lradc.h | 204 +++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 439 insertions(+)
>>  create mode 100644 drivers/mfd/mxs-lradc.c
>>  create mode 100644 include/linux/mfd/mxs-lradc.h
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 9ca66de..62593ad 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -271,6 +271,23 @@ config MFD_MC13XXX_I2C
>>       help
>>         Select this if your MC13xxx is connected via an I2C bus.
>>
>> +config MFD_MXS_LRADC
>> +     tristate "Freescale i.MX23/i.MX28 LRADC"
>> +     depends on ARCH_MXS || COMPILE_TEST
>> +     select MFD_CORE
>> +     select STMP_DEVICE
>> +     help
>> +       Say yes here to build support for the low-resolution
>> +       analog-to-digital converter (LRADC) found on the i.MX23 and i.MX28
>
> Low Resolution Analogue-to-Digital Converter
>
>> +       processors. This driver provides common support for accessing the
>> +       device, additional drivers must be enabled in order to use the
>> +       functionality of the device:
>> +             mxs-lradc-adc for ADC readings
>> +             mxs-lradc-ts  for touchscreen support
>> +
>> +       This driver can also be built as a module. If so, the module will be
>> +       called mxs-lradc.
>> +
>>  config MFD_HI6421_PMIC
>>       tristate "HiSilicon Hi6421 PMU/Codec IC"
>>       depends on OF
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 0f230a6..ecf6399 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -198,3 +198,4 @@ intel-soc-pmic-objs               := intel_soc_pmic_core.o intel_soc_pmic_crc.o
>>  intel-soc-pmic-$(CONFIG_INTEL_PMC_IPC)       += intel_soc_pmic_bxtwc.o
>>  obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o
>>  obj-$(CONFIG_MFD_MT6397)     += mt6397-core.o
>> +obj-$(CONFIG_MFD_MXS_LRADC)  += mxs-lradc.o
>> diff --git a/drivers/mfd/mxs-lradc.c b/drivers/mfd/mxs-lradc.c
>> new file mode 100644
>> index 0000000..d53a524
>> --- /dev/null
>> +++ b/drivers/mfd/mxs-lradc.c
>> @@ -0,0 +1,217 @@
>> +/*
>> + * Freescale MXS LRADC driver
>> + *
>> + * Copyright (c) 2012 DENX Software Engineering, GmbH.
>
>
> You need to update the copyright.
>
> Authors:
>
>> + * Marek Vasut <marex@xxxxxxx>
>> + * Ksenija Stanojevic <ksenija.stanojevic@xxxxxxxxx>
>> + *
>> + * 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/clk.h>
>> +#include <linux/device.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/mxs-lradc.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +
>> +static const char * const mx23_lradc_irq_names[] = {
>> +     "mxs-lradc-touchscreen",
>> +     "mxs-lradc-channel0",
>> +     "mxs-lradc-channel1",
>> +     "mxs-lradc-channel2",
>> +     "mxs-lradc-channel3",
>> +     "mxs-lradc-channel4",
>> +     "mxs-lradc-channel5",
>> +     "mxs-lradc-channel6",
>> +     "mxs-lradc-channel7",
>> +};
>> +
>> +static const char * const mx28_lradc_irq_names[] = {
>> +     "mxs-lradc-touchscreen",
>> +     "mxs-lradc-thresh0",
>> +     "mxs-lradc-thresh1",
>> +     "mxs-lradc-channel0",
>> +     "mxs-lradc-channel1",
>> +     "mxs-lradc-channel2",
>> +     "mxs-lradc-channel3",
>> +     "mxs-lradc-channel4",
>> +     "mxs-lradc-channel5",
>> +     "mxs-lradc-channel6",
>> +     "mxs-lradc-channel7",
>> +     "mxs-lradc-button0",
>> +     "mxs-lradc-button1",
>> +};
>> +
>> +struct mxs_lradc_of_config {
>> +     const int               irq_count;
>> +     const char * const      *irq_name;
>> +};
>> +
>> +static const struct mxs_lradc_of_config mxs_lradc_of_config[] = {
>> +     [IMX23_LRADC] = {
>> +             .irq_count      = ARRAY_SIZE(mx23_lradc_irq_names),
>> +             .irq_name       = mx23_lradc_irq_names,
>> +     },
>> +     [IMX28_LRADC] = {
>> +             .irq_count      = ARRAY_SIZE(mx28_lradc_irq_names),
>> +             .irq_name       = mx28_lradc_irq_names,
>> +     },
>> +};
>
> Please use DEFINE_RES_IRQ_NAMED() instead.
>
>> +static const struct of_device_id mxs_lradc_dt_ids[] = {
>> +     { .compatible = "fsl,imx23-lradc", .data = (void *)IMX23_LRADC, },
>> +     { .compatible = "fsl,imx28-lradc", .data = (void *)IMX28_LRADC, },
>> +     { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
>> +
>> +static int mxs_lradc_add_device(struct platform_device *pdev,
>> +                             struct mxs_lradc *lradc, char *name, int i)
>> +{
>> +     struct mfd_cell *cell;
>> +
>> +     cell = &lradc->cells[i];
>> +     cell->name = name;
>> +     cell->platform_data = lradc;
>> +     cell->pdata_size = sizeof(*lradc);
>> +
>> +     return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0, NULL);
>> +}
>
> Please don't roll your own API.
>
> Use 'struct mfd_cell' like everyone else does.

It has been suggested in previous reviews to use separate function to
register mfd device, and to make mfd_cell allocate dynamically because
struc mxs-lradc is allocated dynamically.
But I can revrse changes and make mfd_cells allocate staticaly
wthout separate function.

>> +static int mxs_lradc_probe(struct platform_device *pdev)
>> +{
>> +     const struct of_device_id *of_id =
>> +             of_match_device(mxs_lradc_dt_ids, &pdev->dev);
>> +     const struct mxs_lradc_of_config *of_cfg =
>> +             &mxs_lradc_of_config[(enum mxs_lradc_id)of_id->data];
>
> This implementation is wayyyy more complicated than it needs to be.
>
> You've entangled DT probing with the MFD API.
>
> Please use the MFD API properly.
>
>> +     struct device *dev = &pdev->dev;
>> +     struct device_node *node = dev->of_node;
>> +     struct mxs_lradc *lradc;
>> +     struct resource *iores;
>> +     int ret = 0, touch_ret, i;
>> +     u32 ts_wires = 0;
>> +
>> +     lradc = devm_kzalloc(&pdev->dev, sizeof(*lradc), GFP_KERNEL);
>> +     if (!lradc)
>> +             return -ENOMEM;
>> +     lradc->soc = (enum mxs_lradc_id)of_id->data;
>
> If you're going to pull the platform ID out like this, please pull the
> *_match_* function down, just above this line.  And separate them both
> from allocating memory.

Ok, but then I will also need to pull this line also:
const struct mxs_lradc_of_config *of_cfg =
 &mxs_lradc_of_config[(enum mxs_lradc_id)of_id->data];

>> +     /* Grab the memory area */
>
> This comment doesn't add anything.

Ok, I will remove it.

> We know what these calls do, and they don't "grab" anything.
>
>> +     iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     lradc->base = devm_ioremap_resource(dev, iores);
>> +     if (IS_ERR(lradc->base))
>> +             return PTR_ERR(lradc->base);
>> +
>> +     lradc->clk = devm_clk_get(&pdev->dev, NULL);
>> +     if (IS_ERR(lradc->clk)) {
>> +             dev_err(dev, "Failed to get the delay unit clock\n");
>> +             return PTR_ERR(lradc->clk);
>> +     }
>
> '\n' here.  Nice spacing makes code easier to follow.
>
>> +     ret = clk_prepare_enable(lradc->clk);
>> +     if (ret != 0) {
>> +             dev_err(dev, "Failed to enable the delay unit clock\n");
>> +             return ret;
>> +     }
>> +
>> +     touch_ret = of_property_read_u32(node, "fsl,lradc-touchscreen-wires",
>> +                                      &ts_wires);
>
> Why do you need more than one 'ret' variable?

Yeah, it's not really needed.

>> +     if (touch_ret == 0) {
>> +             lradc->buffer_vchans = BUFFER_VCHANS_LIMITED;
>
> '\n' here  or else we think it's related to the switch().
>
>> +             switch (ts_wires) {
>> +             case 4:
>> +                     lradc->use_touchscreen = MXS_LRADC_TOUCHSCREEN_4WIRE;
>
> This is not a good name for a variable.  It looks like a bool here,
> but you're initialising with a configuration option.

Ok, will touchscreen_wire be ok?

>> +                     break;
>> +             case 5:
>> +                     if (lradc->soc == IMX28_LRADC) {
>> +                             lradc->use_touchscreen =
>> +                                     MXS_LRADC_TOUCHSCREEN_5WIRE;
>> +                             break;
>> +                     }
>> +                     /* fall through an error message for i.MX23 */
>
> "to an error"
>
>> +             default:
>> +                     dev_err(&pdev->dev,
>> +                             "Unsupported number of touchscreen wires (%d)\n",
>> +                             ts_wires);
>> +                     return -EINVAL;
>
> The clock is still enabled.

Ok, I will use goto err_clk

>> +             }
>> +     } else {
>> +             lradc->buffer_vchans = BUFFER_VCHANS_ALL;
>> +     }
>> +
>> +     lradc->irq_count = of_cfg->irq_count;
>> +     lradc->irq_name = of_cfg->irq_name;
>> +     for (i = 0; i < lradc->irq_count; i++) {
>> +             lradc->irq[i] = platform_get_irq(pdev, i);
>> +             if (lradc->irq[i] < 0) {
>> +                     ret = lradc->irq[i];
>> +                     goto err_clk;
>> +             }
>> +     }
>
> You shouldn't be 'get'ing the IRQ here.
>
> Where are you consuming these IRQs?

There are used in adc (mxs-lradc-adc) and touchscreen (mxs-lradc-ts) part.

>> +     platform_set_drvdata(pdev, lradc);
>> +
>> +     ret = stmp_reset_block(lradc->base);
>> +     if (ret)
>> +             return ret;
>
>
> The clock is still enabled.
>
Ok, I will use goto err_clk

>> +     ret = mxs_lradc_add_device(pdev, lradc, DRIVER_NAME_ADC, 0);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "Failed to add the ADC subdevice\n");
>> +             goto err_clk;
>> +     }
>
> Do this using statically defined mfd_cells.
>
>> +     if (!lradc->use_touchscreen)
>> +             return 0;
>> +
>> +     ret = mxs_lradc_add_device(pdev, lradc, DRIVER_NAME_TS, 1);
>> +     if (ret) {
>> +             dev_err(&pdev->dev,
>> +                     "Failed to add the touchscreen subdevice\n");
>> +             goto err_clk;
>> +     }
>> +
>> +     return 0;
>> +
>> +err_clk:
>> +     clk_disable_unprepare(lradc->clk);
>> +
>> +     return ret;
>> +}
>> +
>> +static int mxs_lradc_remove(struct platform_device *pdev)
>> +{
>> +     struct mxs_lradc *lradc = platform_get_drvdata(pdev);
>> +
>> +     clk_disable_unprepare(lradc->clk);
>> +
>> +     return 0;
>> +}
>> +
>> +static struct platform_driver mxs_lradc_driver = {
>> +     .driver = {
>> +             .name = "mxs-lradc",
>> +             .of_match_table = mxs_lradc_dt_ids,
>> +     },
>> +     .probe = mxs_lradc_probe,
>> +     .remove = mxs_lradc_remove,
>> +};
>> +
>
> This line is superfluous.
>
>> +module_platform_driver(mxs_lradc_driver);
>> +
>> +MODULE_AUTHOR("Ksenija Stanojevic <ksenija.stanojevic@xxxxxxxxx>");
>> +MODULE_DESCRIPTION("Freescale i.MX23/i.MX28 LRADC driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:mxs-lradc");
>> diff --git a/include/linux/mfd/mxs-lradc.h b/include/linux/mfd/mxs-lradc.h
>> new file mode 100644
>> index 0000000..c1475c0
>> --- /dev/null
>> +++ b/include/linux/mfd/mxs-lradc.h
>> @@ -0,0 +1,204 @@
>> +/*
>> + * Freescale MXS LRADC driver
>> + *
>> + * Copyright (c) 2012 DENX Software Engineering, GmbH.
>
> You need to update the copyright.
>
> Author:
>
>> + * Marek Vasut <marex@xxxxxxx>
>> + *
>> + * 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.
>> + */
>> +
>> +#ifndef __MXS_LRADC_H
>> +#define __MXS_LRADC_H
>
> __MFD_MXS_*
>
>> +#include <linux/bitops.h>
>> +#include <linux/io.h>
>> +#include <linux/stmp_device.h>
>> +
>> +#define DRIVER_NAME_ADC "mxs-lradc-adc"
>> +#define DRIVER_NAME_TS "mxs-lradc-ts"
>
> Don't define these.  Just use the proper names.

Ok.

>> +#define LRADC_MAX_DELAY_CHANS        4
>> +#define LRADC_MAX_MAPPED_CHANS       8
>> +#define LRADC_MAX_TOTAL_CHANS        16
>> +
>> +#define LRADC_DELAY_TIMER_HZ 2000
>> +
>> +#define LRADC_CTRL0                          0x00
>> +# define LRADC_CTRL0_MX28_TOUCH_DETECT_ENABLE        BIT(23)
>> +# define LRADC_CTRL0_MX28_TOUCH_SCREEN_TYPE  BIT(22)
>> +# define LRADC_CTRL0_MX28_YNNSW /* YM */     BIT(21)
>> +# define LRADC_CTRL0_MX28_YPNSW /* YP */     BIT(20)
>> +# define LRADC_CTRL0_MX28_YPPSW /* YP */     BIT(19)
>> +# define LRADC_CTRL0_MX28_XNNSW /* XM */     BIT(18)
>> +# define LRADC_CTRL0_MX28_XNPSW /* XM */     BIT(17)
>> +# define LRADC_CTRL0_MX28_XPPSW /* XP */     BIT(16)
>> +
>> +# define LRADC_CTRL0_MX23_TOUCH_DETECT_ENABLE        BIT(20)
>> +# define LRADC_CTRL0_MX23_YM                 BIT(19)
>> +# define LRADC_CTRL0_MX23_XM                 BIT(18)
>> +# define LRADC_CTRL0_MX23_YP                 BIT(17)
>> +# define LRADC_CTRL0_MX23_XP                 BIT(16)
>> +
>> +# define LRADC_CTRL0_MX28_PLATE_MASK \
>> +             (LRADC_CTRL0_MX28_TOUCH_DETECT_ENABLE | \
>> +             LRADC_CTRL0_MX28_YNNSW | LRADC_CTRL0_MX28_YPNSW | \
>> +             LRADC_CTRL0_MX28_YPPSW | LRADC_CTRL0_MX28_XNNSW | \
>> +             LRADC_CTRL0_MX28_XNPSW | LRADC_CTRL0_MX28_XPPSW)
>> +
>> +# define LRADC_CTRL0_MX23_PLATE_MASK \
>> +             (LRADC_CTRL0_MX23_TOUCH_DETECT_ENABLE | \
>> +             LRADC_CTRL0_MX23_YM | LRADC_CTRL0_MX23_XM | \
>> +             LRADC_CTRL0_MX23_YP | LRADC_CTRL0_MX23_XP)
>> +
>> +#define LRADC_CTRL1                          0x10
>> +#define LRADC_CTRL1_TOUCH_DETECT_IRQ_EN              BIT(24)
>> +#define LRADC_CTRL1_LRADC_IRQ_EN(n)          (1 << ((n) + 16))
>> +#define LRADC_CTRL1_MX28_LRADC_IRQ_EN_MASK   (0x1fff << 16)
>> +#define LRADC_CTRL1_MX23_LRADC_IRQ_EN_MASK   (0x01ff << 16)
>> +#define LRADC_CTRL1_LRADC_IRQ_EN_OFFSET              16
>> +#define LRADC_CTRL1_TOUCH_DETECT_IRQ         BIT(8)
>> +#define LRADC_CTRL1_LRADC_IRQ(n)             (1 << (n))
>> +#define LRADC_CTRL1_MX28_LRADC_IRQ_MASK              0x1fff
>> +#define LRADC_CTRL1_MX23_LRADC_IRQ_MASK              0x01ff
>> +#define LRADC_CTRL1_LRADC_IRQ_OFFSET         0
>> +
>> +#define LRADC_CTRL2                          0x20
>> +#define LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET     24
>> +#define LRADC_CTRL2_TEMPSENSE_PWD            BIT(15)
>> +
>> +#define LRADC_STATUS                         0x40
>> +#define LRADC_STATUS_TOUCH_DETECT_RAW                BIT(0)
>> +
>> +#define LRADC_CH(n)                          (0x50 + (0x10 * (n)))
>> +#define LRADC_CH_ACCUMULATE                  BIT(29)
>> +#define LRADC_CH_NUM_SAMPLES_MASK            (0x1f << 24)
>> +#define LRADC_CH_NUM_SAMPLES_OFFSET          24
>> +#define LRADC_CH_NUM_SAMPLES(x) \
>> +                             ((x) << LRADC_CH_NUM_SAMPLES_OFFSET)
>> +#define LRADC_CH_VALUE_MASK                  0x3ffff
>> +#define LRADC_CH_VALUE_OFFSET                        0
>> +
>> +#define LRADC_DELAY(n)                               (0xd0 + (0x10 * (n)))
>> +#define LRADC_DELAY_TRIGGER_LRADCS_MASK              (0xffUL << 24)
>> +#define LRADC_DELAY_TRIGGER_LRADCS_OFFSET    24
>> +#define LRADC_DELAY_TRIGGER(x) \
>> +                             (((x) << LRADC_DELAY_TRIGGER_LRADCS_OFFSET) & \
>> +                             LRADC_DELAY_TRIGGER_LRADCS_MASK)
>> +#define LRADC_DELAY_KICK                     BIT(20)
>> +#define LRADC_DELAY_TRIGGER_DELAYS_MASK              (0xf << 16)
>> +#define LRADC_DELAY_TRIGGER_DELAYS_OFFSET    16
>> +#define LRADC_DELAY_TRIGGER_DELAYS(x) \
>> +                             (((x) << LRADC_DELAY_TRIGGER_DELAYS_OFFSET) & \
>> +                             LRADC_DELAY_TRIGGER_DELAYS_MASK)
>> +#define LRADC_DELAY_LOOP_COUNT_MASK          (0x1f << 11)
>> +#define LRADC_DELAY_LOOP_COUNT_OFFSET                11
>> +#define LRADC_DELAY_LOOP(x) \
>> +                             (((x) << LRADC_DELAY_LOOP_COUNT_OFFSET) & \
>> +                             LRADC_DELAY_LOOP_COUNT_MASK)
>> +#define LRADC_DELAY_DELAY_MASK                       0x7ff
>> +#define LRADC_DELAY_DELAY_OFFSET             0
>> +#define LRADC_DELAY_DELAY(x) \
>> +                             (((x) << LRADC_DELAY_DELAY_OFFSET) & \
>> +                             LRADC_DELAY_DELAY_MASK)
>> +
>> +#define LRADC_CTRL4                          0x140
>> +#define LRADC_CTRL4_LRADCSELECT_MASK(n)              (0xf << ((n) * 4))
>> +#define LRADC_CTRL4_LRADCSELECT_OFFSET(n)    ((n) * 4)
>> +#define LRADC_CTRL4_LRADCSELECT(n, x) \
>> +                             (((x) << LRADC_CTRL4_LRADCSELECT_OFFSET(n)) & \
>> +                             LRADC_CTRL4_LRADCSELECT_MASK(n))
>> +
>> +#define LRADC_RESOLUTION                     12
>> +#define LRADC_SINGLE_SAMPLE_MASK             ((1 << LRADC_RESOLUTION) - 1)
>> +
>> +enum mxs_lradc_id {
>> +     IMX23_LRADC,
>> +     IMX28_LRADC,
>> +};
>> +
>> +enum mxs_lradc_ts_wires {
>> +     MXS_LRADC_TOUCHSCREEN_NONE = 0,
>> +     MXS_LRADC_TOUCHSCREEN_4WIRE,
>> +     MXS_LRADC_TOUCHSCREEN_5WIRE,
>> +};
>> +
>> +struct mxs_lradc {
>> +     enum mxs_lradc_id       soc;
>> +
>> +     void __iomem            *base;
>> +     struct clk              *clk;
>> +
>> +     int                     irq[13];
>> +     const char * const      *irq_name;
>> +     int                     irq_count;
>
> I guess you won't be needing these.
>
>> +#define BUFFER_VCHANS_LIMITED                0x3f
>> +#define BUFFER_VCHANS_ALL            0xff
>
> Can you remove all these defines from inside the struct definition
> please?

Ok.

>> +     u8                      buffer_vchans;
>> +
>> +     /*
>> +      * Certain LRADC channels are shared between touchscreen
>> +      * and/or touch-buttons and generic LRADC block. Therefore when using
>> +      * either of these, these channels are not available for the regular
>> +      * sampling. The shared channels are as follows:
>> +      *
>> +      * CH0 -- Touch button #0
>> +      * CH1 -- Touch button #1
>> +      * CH2 -- Touch screen XPUL
>> +      * CH3 -- Touch screen YPLL
>> +      * CH4 -- Touch screen XNUL
>> +      * CH5 -- Touch screen YNLR
>> +      * CH6 -- Touch screen WIPER (5-wire only)
>> +      *
>> +      * The bit fields below represents which parts of the LRADC block are
>> +      * switched into special mode of operation. These channels can not
>> +      * be sampled as regular LRADC channels. The driver will refuse any
>> +      * attempt to sample these channels.
>> +      */
>> +#define CHAN_MASK_TOUCHBUTTON                (BIT(1) | BIT(0))
>> +#define CHAN_MASK_TOUCHSCREEN_4WIRE  (0xf << 2)
>> +#define CHAN_MASK_TOUCHSCREEN_5WIRE  (0x1f << 2)
>> +     enum mxs_lradc_ts_wires use_touchscreen;
>> +     bool                    use_touchbutton;
>> +
>> +#define MXS_LRADC_CELLS                      2
>> +     struct mfd_cell cells[MXS_LRADC_CELLS];
>> +};
>> +
>> +static inline void mxs_lradc_reg_set(struct mxs_lradc *lradc, u32 val, u32 reg)
>> +{
>> +     writel(val, lradc->base + reg + STMP_OFFSET_REG_SET);
>> +}
>> +
>> +static inline void mxs_lradc_reg_clear(struct mxs_lradc *lradc, u32 val,
>> +                                    u32 reg)
>> +{
>> +     writel(val, lradc->base + reg + STMP_OFFSET_REG_CLR);
>> +}
>> +
>> +static inline void mxs_lradc_reg_wrt(struct mxs_lradc *lradc, u32 val, u32 reg)
>> +{
>> +     writel(val, lradc->base + reg);
>> +}
>
> Why do you need to abstract the well know, accepted writel calls?

It makes code more readable.

>> +static inline u32 mxs_lradc_irq_mask(struct mxs_lradc *lradc)
>> +{
>> +     switch (lradc->soc) {
>> +     case IMX23_LRADC:
>> +             return LRADC_CTRL1_MX23_LRADC_IRQ_MASK;
>> +     case IMX28_LRADC:
>> +             return LRADC_CTRL1_MX28_LRADC_IRQ_MASK;
>> +     default:
>> +             return 0;
>> +     }
>> +}
>> +
>> +#endif /* __MXS_LRADC_H */

Regards,
Ksenija
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux