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

[Marek Vasut <marex@denx.de>]

On 04/29/2016 03:43 PM, Ksenija Stanojević wrote:
> On Fri, Apr 29, 2016 at 3:15 PM, Marek Vasut <marex@denx.de> wrote:
>> On 04/29/2016 01:47 PM, Ksenija Stanojevic wrote:
>>> Add core files for mxs-lradc MFD driver.
>>>
>>> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
>>> ---
>>>  drivers/mfd/Kconfig           |  33 +++++--
>>>  drivers/mfd/Makefile          |   1 +
>>>  drivers/mfd/mxs-lradc.c       | 213 ++++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/mfd/mxs-lradc.h | 210 +++++++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 449 insertions(+), 8 deletions(-)
>>>  create mode 100644 drivers/mfd/mxs-lradc.c
>>>  create mode 100644 include/linux/mfd/mxs-lradc.h
>>
>> Is there any chance you can also remove the same code from lradc ?
> 
> You mean drivers/iio/adc/mxs-lradc.c. I thought to remove it once this
> patch set was accepted,
> but I can include that in patch set.

I'd much rather see one driver mutate into another than having two
drivers in the tree. If that's possible without crazy amount of effort
that is.

>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>> index eea61e3..fff44d6 100644
>>> --- a/drivers/mfd/Kconfig
>>> +++ b/drivers/mfd/Kconfig
>>> @@ -16,7 +16,7 @@ config MFD_CS5535
>>>       depends on PCI && (X86_32 || (X86 && COMPILE_TEST))
>>>       ---help---
>>>         This is the core driver for CS5535/CS5536 MFD functions.  This is
>>> -          necessary for using the board's GPIO and MFGPT functionality.
>>> +       necessary for using the board's GPIO and MFGPT functionality.
>>
>> Probably shouldn't be part of the patch ?
> 
> Yeah, sorry about that.

:)

>>>  config MFD_ACT8945A
>>>       tristate "Active-semi ACT8945A"
>>> @@ -319,6 +319,23 @@ config MFD_HI6421_PMIC
>>>         menus in order to enable them.
>>>         We communicate with the Hi6421 via memory-mapped I/O.
>>>
>>> +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
>>> +       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 HTC_EGPIO
>>>       bool "HTC EGPIO support"
>>>       depends on GPIOLIB && ARM
>>> @@ -650,7 +667,7 @@ config EZX_PCAP
>>>         needed for MMC, TouchScreen, Sound, USB, etc..
>>>
>>>  config MFD_VIPERBOARD
>>> -        tristate "Nano River Technologies Viperboard"
>>> +     tristate "Nano River Technologies Viperboard"
>>
>> Shouldn't be part of this patch
> 
> No, I should have check it twice.

Yeah, but that's what the patch review process is for.

[...]

>>> +     ret = mfd_add_devices(&pdev->dev, -1, &lradc_ts_dev, 1, NULL, 0, NULL);
>>
>> devm_mfd_add_devices()?
>>
>> You might want to split registration of each MFD subdev into separate
>> function.
> 
> Ok, I will fix it in v2

Just be careful not to introduce a race with enabling/disabling the
clock. That's something I am not sure about and something which might
bite you.

>>> +     if (ret) {
>>> +             dev_err(&pdev->dev,
>>> +                     "Failed to add the touchscreen subdevice\n");
>>> +             goto err_remove_adc;
>>> +     }
>>> +
>>> +     return 0;
>>> +
>>> +err_remove_adc:
>>> +     mfd_remove_devices(&pdev->dev);
>>> +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);
>>> +
>>> +     mfd_remove_devices(&pdev->dev);
>>> +     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,
>>> +};
>>> +
>>> +module_platform_driver(mxs_lradc_driver);
>>> +
>>> +MODULE_DESCRIPTION("Freescale i.MX23/i.MX28 LRADC driver");
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_ALIAS("platform:mxs-lradc");
>>
>> [...]
>>
>>
>> --
>> Best regards,
>> Marek Vasut


-- 
Best regards,
Marek Vasut