Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC
On Fri, Jul 22, 2016 at 02:55:56PM +0100, Lee Jones wrote:
> On Thu, 21 Jul 2016, Maxime Ripard wrote:
>
> > 1;4205;0c
> > On Thu, Jul 21, 2016 at 01:12:53PM +0100, Lee Jones wrote:
> > > On Wed, 20 Jul 2016, Jonathan Cameron wrote:
> > >
> > > > On 19/07/16 08:31, Lee Jones wrote:
> > > > > On Mon, 18 Jul 2016, Jonathan Cameron wrote:
> > > > >
> > > > >> On 15/07/16 10:59, Quentin Schulz wrote:
> > > > >>> The Allwinner SoCs all have an ADC that can also act as a
> > > > >>> touchscreen
> > > > >>> controller and a thermal sensor. For now, only the ADC and the
> > > > >>> thermal
> > > > >>> sensor drivers are probed by the MFD, the touchscreen controller
> > > > >>> support
> > > > >>> will be added later.
> > > > >>>
> > > > >>> Signed-off-by: Quentin Schulz
> > > > >> Hmm. Previous patch includes the header this one creates. Ordering
> > > > >> issue?
> > > > >> The depends kind of prevents build failures by ensuring that can't
> > > > >> be built
> > > > >> until this one is in place, but it is certainly an ugly way to do it.
> > > > >>
> > > > >> Few little bits innline.
> > > > >>> ---
> > > > >>>
> > > > >>> v2:
> > > > >>> - add license headers,
> > > > >>> - reorder alphabetically includes,
> > > > >>> - add SUNXI_GPADC_ prefixes for defines,
> > > > >>>
> > > > >>> drivers/mfd/Kconfig | 14 +++
> > > > >>> drivers/mfd/Makefile| 2 +
> > > > >>> drivers/mfd/sunxi-gpadc-mfd.c | 197
> > > > >>>
> > > > >>> include/linux/mfd/sunxi-gpadc-mfd.h | 23 +
> > > > >>> 4 files changed, 236 insertions(+)
> > > > >>> create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
> > > > >>> create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> > > > >
> > > > > [...]
> > > > >
> > > > >>> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
> > > > >>> + {
> > > > >>> + .name = "sun6i-a31-gpadc-iio",
> > > > >>> + .resources = adc_resources,
> > > > >>> + .num_resources = ARRAY_SIZE(adc_resources),
> > > > >>> + }, {
> > > > >>> + .name = "iio_hwmon",
> > > > >> I still really dislike using this to force the probe of that driver
> > > > >> but
> > > > >> kind of up to the hwmon / mfd guys on this.
> > > > >
> > > > > Can you at least say *why* you don't like it?
> > > > It just feels odd to have an mfd child that isn't really dependent
> > > > on the mfd hardware itself.
> > > >
> > > > Still if you are happy, mfd is your domain and my objections were
> > > > as you probably noticed not that strong - or well described!)
> > > >
> > > > So I'm fine with this.
> > >
> > > I see. So it's not actually part of the same IP/chip?
> >
> > The chip has a temperature sensor, and we want to expose that
> > temperature through IIO.
> >
> > But we don't really have the choice on how we probe iio-hwmon
> > here. The binding was already there, and we have to keep it.
>
> What binding?
The binding introduced for the original driver this serie aims to
replace:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/input/touchscreen/sun4i.txt
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
signature.asc
Description: PGP signature
Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC
On 22/07/16 15:55, Lee Jones wrote:
> On Thu, 21 Jul 2016, Maxime Ripard wrote:
>
>> 1;4205;0c
>> On Thu, Jul 21, 2016 at 01:12:53PM +0100, Lee Jones wrote:
>>> On Wed, 20 Jul 2016, Jonathan Cameron wrote:
>>>
On 19/07/16 08:31, Lee Jones wrote:
> On Mon, 18 Jul 2016, Jonathan Cameron wrote:
>
>> On 15/07/16 10:59, Quentin Schulz wrote:
>>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>>> controller and a thermal sensor. For now, only the ADC and the thermal
>>> sensor drivers are probed by the MFD, the touchscreen controller support
>>> will be added later.
>>>
>>> Signed-off-by: Quentin Schulz
>> Hmm. Previous patch includes the header this one creates. Ordering
>> issue?
>> The depends kind of prevents build failures by ensuring that can't be
>> built
>> until this one is in place, but it is certainly an ugly way to do it.
>>
>> Few little bits innline.
>>> ---
>>>
>>> v2:
>>> - add license headers,
>>> - reorder alphabetically includes,
>>> - add SUNXI_GPADC_ prefixes for defines,
>>>
>>> drivers/mfd/Kconfig | 14 +++
>>> drivers/mfd/Makefile| 2 +
>>> drivers/mfd/sunxi-gpadc-mfd.c | 197
>>>
>>> include/linux/mfd/sunxi-gpadc-mfd.h | 23 +
>>> 4 files changed, 236 insertions(+)
>>> create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
>>> create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
>
> [...]
>
>>> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
>>> + {
>>> + .name = "sun6i-a31-gpadc-iio",
>>> + .resources = adc_resources,
>>> + .num_resources = ARRAY_SIZE(adc_resources),
>>> + }, {
>>> + .name = "iio_hwmon",
>> I still really dislike using this to force the probe of that driver but
>> kind of up to the hwmon / mfd guys on this.
>
> Can you at least say *why* you don't like it?
It just feels odd to have an mfd child that isn't really dependent
on the mfd hardware itself.
Still if you are happy, mfd is your domain and my objections were
as you probably noticed not that strong - or well described!)
So I'm fine with this.
>>>
>>> I see. So it's not actually part of the same IP/chip?
>>
>> The chip has a temperature sensor, and we want to expose that
>> temperature through IIO.
>>
>> But we don't really have the choice on how we probe iio-hwmon
>> here. The binding was already there, and we have to keep it.
>
> What binding?
>
The somewhat 'interesting' one for iio-hwmon I would guess is what
Maxime is referring to. Here they are using what was effectively
the old 'board file' methods inside the driver itself. Which is
fine and done in quite a few places - though we have some issues
with deferred probing that we can improve upon.
Anyhow, it's real hardware and as this doesn't have anything to
do with device tree there is no need to get around the objections
of lack of generality that bothers the dt maintainers :)
Lets go with it as it is. Sorry for confusing everyone!
p.s. I'm pitching a tech session for the KS at the moment
on ksummit-discuss to hit the fiddlier question of bindings
/ cross subsystem interactions if anyone is interested.
Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC
On Thu, 21 Jul 2016, Maxime Ripard wrote:
> 1;4205;0c
> On Thu, Jul 21, 2016 at 01:12:53PM +0100, Lee Jones wrote:
> > On Wed, 20 Jul 2016, Jonathan Cameron wrote:
> >
> > > On 19/07/16 08:31, Lee Jones wrote:
> > > > On Mon, 18 Jul 2016, Jonathan Cameron wrote:
> > > >
> > > >> On 15/07/16 10:59, Quentin Schulz wrote:
> > > >>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> > > >>> controller and a thermal sensor. For now, only the ADC and the thermal
> > > >>> sensor drivers are probed by the MFD, the touchscreen controller
> > > >>> support
> > > >>> will be added later.
> > > >>>
> > > >>> Signed-off-by: Quentin Schulz
> > > >> Hmm. Previous patch includes the header this one creates. Ordering
> > > >> issue?
> > > >> The depends kind of prevents build failures by ensuring that can't be
> > > >> built
> > > >> until this one is in place, but it is certainly an ugly way to do it.
> > > >>
> > > >> Few little bits innline.
> > > >>> ---
> > > >>>
> > > >>> v2:
> > > >>> - add license headers,
> > > >>> - reorder alphabetically includes,
> > > >>> - add SUNXI_GPADC_ prefixes for defines,
> > > >>>
> > > >>> drivers/mfd/Kconfig | 14 +++
> > > >>> drivers/mfd/Makefile| 2 +
> > > >>> drivers/mfd/sunxi-gpadc-mfd.c | 197
> > > >>>
> > > >>> include/linux/mfd/sunxi-gpadc-mfd.h | 23 +
> > > >>> 4 files changed, 236 insertions(+)
> > > >>> create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
> > > >>> create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> > > >
> > > > [...]
> > > >
> > > >>> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
> > > >>> + {
> > > >>> + .name = "sun6i-a31-gpadc-iio",
> > > >>> + .resources = adc_resources,
> > > >>> + .num_resources = ARRAY_SIZE(adc_resources),
> > > >>> + }, {
> > > >>> + .name = "iio_hwmon",
> > > >> I still really dislike using this to force the probe of that driver but
> > > >> kind of up to the hwmon / mfd guys on this.
> > > >
> > > > Can you at least say *why* you don't like it?
> > > It just feels odd to have an mfd child that isn't really dependent
> > > on the mfd hardware itself.
> > >
> > > Still if you are happy, mfd is your domain and my objections were
> > > as you probably noticed not that strong - or well described!)
> > >
> > > So I'm fine with this.
> >
> > I see. So it's not actually part of the same IP/chip?
>
> The chip has a temperature sensor, and we want to expose that
> temperature through IIO.
>
> But we don't really have the choice on how we probe iio-hwmon
> here. The binding was already there, and we have to keep it.
What binding?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC
1;4205;0c
On Thu, Jul 21, 2016 at 01:12:53PM +0100, Lee Jones wrote:
> On Wed, 20 Jul 2016, Jonathan Cameron wrote:
>
> > On 19/07/16 08:31, Lee Jones wrote:
> > > On Mon, 18 Jul 2016, Jonathan Cameron wrote:
> > >
> > >> On 15/07/16 10:59, Quentin Schulz wrote:
> > >>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> > >>> controller and a thermal sensor. For now, only the ADC and the thermal
> > >>> sensor drivers are probed by the MFD, the touchscreen controller support
> > >>> will be added later.
> > >>>
> > >>> Signed-off-by: Quentin Schulz
> > >> Hmm. Previous patch includes the header this one creates. Ordering
> > >> issue?
> > >> The depends kind of prevents build failures by ensuring that can't be
> > >> built
> > >> until this one is in place, but it is certainly an ugly way to do it.
> > >>
> > >> Few little bits innline.
> > >>> ---
> > >>>
> > >>> v2:
> > >>> - add license headers,
> > >>> - reorder alphabetically includes,
> > >>> - add SUNXI_GPADC_ prefixes for defines,
> > >>>
> > >>> drivers/mfd/Kconfig | 14 +++
> > >>> drivers/mfd/Makefile| 2 +
> > >>> drivers/mfd/sunxi-gpadc-mfd.c | 197
> > >>>
> > >>> include/linux/mfd/sunxi-gpadc-mfd.h | 23 +
> > >>> 4 files changed, 236 insertions(+)
> > >>> create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
> > >>> create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> > >
> > > [...]
> > >
> > >>> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
> > >>> + {
> > >>> + .name = "sun6i-a31-gpadc-iio",
> > >>> + .resources = adc_resources,
> > >>> + .num_resources = ARRAY_SIZE(adc_resources),
> > >>> + }, {
> > >>> + .name = "iio_hwmon",
> > >> I still really dislike using this to force the probe of that driver but
> > >> kind of up to the hwmon / mfd guys on this.
> > >
> > > Can you at least say *why* you don't like it?
> > It just feels odd to have an mfd child that isn't really dependent
> > on the mfd hardware itself.
> >
> > Still if you are happy, mfd is your domain and my objections were
> > as you probably noticed not that strong - or well described!)
> >
> > So I'm fine with this.
>
> I see. So it's not actually part of the same IP/chip?
The chip has a temperature sensor, and we want to expose that
temperature through IIO.
But we don't really have the choice on how we probe iio-hwmon
here. The binding was already there, and we have to keep it.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
signature.asc
Description: PGP signature
Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC
On Wed, 20 Jul 2016, Jonathan Cameron wrote:
> On 19/07/16 08:31, Lee Jones wrote:
> > On Mon, 18 Jul 2016, Jonathan Cameron wrote:
> >
> >> On 15/07/16 10:59, Quentin Schulz wrote:
> >>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> >>> controller and a thermal sensor. For now, only the ADC and the thermal
> >>> sensor drivers are probed by the MFD, the touchscreen controller support
> >>> will be added later.
> >>>
> >>> Signed-off-by: Quentin Schulz
> >> Hmm. Previous patch includes the header this one creates. Ordering issue?
> >> The depends kind of prevents build failures by ensuring that can't be built
> >> until this one is in place, but it is certainly an ugly way to do it.
> >>
> >> Few little bits innline.
> >>> ---
> >>>
> >>> v2:
> >>> - add license headers,
> >>> - reorder alphabetically includes,
> >>> - add SUNXI_GPADC_ prefixes for defines,
> >>>
> >>> drivers/mfd/Kconfig | 14 +++
> >>> drivers/mfd/Makefile| 2 +
> >>> drivers/mfd/sunxi-gpadc-mfd.c | 197
> >>>
> >>> include/linux/mfd/sunxi-gpadc-mfd.h | 23 +
> >>> 4 files changed, 236 insertions(+)
> >>> create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
> >>> create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> >
> > [...]
> >
> >>> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
> >>> + {
> >>> + .name = "sun6i-a31-gpadc-iio",
> >>> + .resources = adc_resources,
> >>> + .num_resources = ARRAY_SIZE(adc_resources),
> >>> + }, {
> >>> + .name = "iio_hwmon",
> >> I still really dislike using this to force the probe of that driver but
> >> kind of up to the hwmon / mfd guys on this.
> >
> > Can you at least say *why* you don't like it?
> It just feels odd to have an mfd child that isn't really dependent
> on the mfd hardware itself.
>
> Still if you are happy, mfd is your domain and my objections were
> as you probably noticed not that strong - or well described!)
>
> So I'm fine with this.
I see. So it's not actually part of the same IP/chip?
> > How else would it get probed?
> >
> >> I don't have any better suggestions though..
> >>> + },
> >>> +};
> >
> > [...]
> >
> >>> + if (ret) {
> >>> + dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
> >>> + regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
> >>> + return ret;
> >>> + }
> >>> +
> >>> + dev_info(&pdev->dev, "successfully loaded\n");
> >> Seems like noise to me, but not my subsystem :)
> >
> > Agreed, I don't allow this either.
> >
> > [...]
> >
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC
On 19/07/16 08:31, Lee Jones wrote:
> On Mon, 18 Jul 2016, Jonathan Cameron wrote:
>
>> On 15/07/16 10:59, Quentin Schulz wrote:
>>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>>> controller and a thermal sensor. For now, only the ADC and the thermal
>>> sensor drivers are probed by the MFD, the touchscreen controller support
>>> will be added later.
>>>
>>> Signed-off-by: Quentin Schulz
>> Hmm. Previous patch includes the header this one creates. Ordering issue?
>> The depends kind of prevents build failures by ensuring that can't be built
>> until this one is in place, but it is certainly an ugly way to do it.
>>
>> Few little bits innline.
>>> ---
>>>
>>> v2:
>>> - add license headers,
>>> - reorder alphabetically includes,
>>> - add SUNXI_GPADC_ prefixes for defines,
>>>
>>> drivers/mfd/Kconfig | 14 +++
>>> drivers/mfd/Makefile| 2 +
>>> drivers/mfd/sunxi-gpadc-mfd.c | 197
>>>
>>> include/linux/mfd/sunxi-gpadc-mfd.h | 23 +
>>> 4 files changed, 236 insertions(+)
>>> create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
>>> create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
>
> [...]
>
>>> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
>>> + {
>>> + .name = "sun6i-a31-gpadc-iio",
>>> + .resources = adc_resources,
>>> + .num_resources = ARRAY_SIZE(adc_resources),
>>> + }, {
>>> + .name = "iio_hwmon",
>> I still really dislike using this to force the probe of that driver but
>> kind of up to the hwmon / mfd guys on this.
>
> Can you at least say *why* you don't like it?
It just feels odd to have an mfd child that isn't really dependent
on the mfd hardware itself.
Still if you are happy, mfd is your domain and my objections were
as you probably noticed not that strong - or well described!)
So I'm fine with this.
>
> How else would it get probed?
>
>> I don't have any better suggestions though..
>>> + },
>>> +};
>
> [...]
>
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
>>> + regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
>>> + return ret;
>>> + }
>>> +
>>> + dev_info(&pdev->dev, "successfully loaded\n");
>> Seems like noise to me, but not my subsystem :)
>
> Agreed, I don't allow this either.
>
> [...]
>
Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC
On 18/07/2016 15:02, Maxime Ripard wrote:
> On Fri, Jul 15, 2016 at 11:59:13AM +0200, Quentin Schulz wrote:
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. For now, only the ADC and the thermal
>> sensor drivers are probed by the MFD, the touchscreen controller support
>> will be added later.
>>
>> Signed-off-by: Quentin Schulz
>> ---
[...]
>> +config MFD_SUNXI_ADC
>> +tristate "ADC MFD core driver for sunxi platforms"
>> +select MFD_CORE
>> +select REGMAP_MMIO
>
> It should also depends on the architectures supported (and probably
> COMPILE_TEST)
>
ACK.
[...]
>> +
>> +sunxi_gpadc_mfd_dev->regmap =
>> +devm_regmap_init_mmio(sunxi_gpadc_mfd_dev->dev,
>> + sunxi_gpadc_mfd_dev->regs,
>> + &sunxi_gpadc_mfd_regmap_config);
>
> This is usually on a single line (even if it exceeds 80 chars). Or
> maybe you can use a shorter variable name (like dev, or mfd).
>
I'll go with a shorter name.
>> +if (IS_ERR(sunxi_gpadc_mfd_dev->regmap)) {
>> +ret = PTR_ERR(sunxi_gpadc_mfd_dev->regmap);
>> +dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
>> +return ret;
>> +}
>> +
>> +irq = platform_get_irq(pdev, 0);
>> +ret = regmap_add_irq_chip(sunxi_gpadc_mfd_dev->regmap, irq,
>> + IRQF_ONESHOT, 0,
>> + &sunxi_gpadc_mfd_regmap_irq_chip,
>> + &sunxi_gpadc_mfd_dev->regmap_irqc);
>> +if (ret) {
>> +dev_err(&pdev->dev, "failed to add irq chip: %d\n", ret);
>> +return ret;
>> +}
>
> You should probably make sure that you clear all the interrupts before
> enabling them.
>
ACK. Thanks, didn't think of that.
>> +if (of_device_is_compatible(pdev->dev.of_node,
>> +"allwinner,sun4i-a10-ts"))
>> +ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
>> + sun4i_gpadc_mfd_cells,
>> + ARRAY_SIZE(sun4i_gpadc_mfd_cells), NULL,
>> + 0, NULL);
>> +else if (of_device_is_compatible(pdev->dev.of_node,
>> + "allwinner,sun5i-a13-ts"))
>> +ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
>> + sun5i_gpadc_mfd_cells,
>> + ARRAY_SIZE(sun5i_gpadc_mfd_cells), NULL,
>> + 0, NULL);
>> +else if (of_device_is_compatible(pdev->dev.of_node,
>> + "allwinner,sun6i-a31-ts"))
>> +ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
>> + sun6i_gpadc_mfd_cells,
>> + ARRAY_SIZE(sun6i_gpadc_mfd_cells), NULL,
>> + 0, NULL);
>
> This huge if / else can be removed by putting those structures in the
> data pointer of of_device_id.
>
Indeed. It is what I am using for the ADC driver, don't know why I
didn't think of this for the MFD as well.
[...]
>> diff --git a/include/linux/mfd/sunxi-gpadc-mfd.h
>> b/include/linux/mfd/sunxi-gpadc-mfd.h
>> new file mode 100644
>> index 000..7155845
>> --- /dev/null
>> +++ b/include/linux/mfd/sunxi-gpadc-mfd.h
>> @@ -0,0 +1,23 @@
>> +/* Header of ADC MFD core driver for sunxi platforms
>> + *
>> + * Copyright (c) 2016 Quentin Schulz
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published
>> by
>> + * the Free Software Foundation.
>> + */
>> +
>> +#ifndef __SUNXI_GPADC_MFD__H__
>> +#define __SUNXI_GPADC_MFD__H__
>> +
>> +#define SUNXI_GPADC_TP_INT_FIFOC0x10
>> +#define SUNXI_GPADC_TP_INT_FIFOS0x14
>
> Why do you declare only these two registers there?
>
Because these are used by the MFD while the others not. Maybe it's
better to put all register and bit defines in sunxi-gpadc-mfd.h? Anyway,
just found out it would be clearer to use the defines for the interrupts
rather than directly "BIT(x)". So that makes two more defines here.
Should we put everything in sunxi-gpadc-mfd.h since the MFD is needed
for the ADC, (future) touchscreen and iio_hwmon drivers?
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC
On 18/07/2016 15:25, Jonathan Cameron wrote:
> On 15/07/16 10:59, Quentin Schulz wrote:
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. For now, only the ADC and the thermal
>> sensor drivers are probed by the MFD, the touchscreen controller support
>> will be added later.
>>
>> Signed-off-by: Quentin Schulz
> Hmm. Previous patch includes the header this one creates. Ordering issue?
> The depends kind of prevents build failures by ensuring that can't be built
> until this one is in place, but it is certainly an ugly way to do it.
>
> Few little bits innline.
[...]
>> +static int sunxi_gpadc_mfd_probe(struct platform_device *pdev)
>> +{
>> +struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev = NULL;
>> +struct resource *mem = NULL;
> Neither of the above assignments is necessary as both will be explicitly
> assigned before they are otherwise used.
ACK.
[...]
>> +dev_info(&pdev->dev, "successfully loaded\n");
> Seems like noise to me, but not my subsystem :)
ACK.
[...]
Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC
On Mon, 18 Jul 2016, Jonathan Cameron wrote:
> On 15/07/16 10:59, Quentin Schulz wrote:
> > The Allwinner SoCs all have an ADC that can also act as a touchscreen
> > controller and a thermal sensor. For now, only the ADC and the thermal
> > sensor drivers are probed by the MFD, the touchscreen controller support
> > will be added later.
> >
> > Signed-off-by: Quentin Schulz
> Hmm. Previous patch includes the header this one creates. Ordering issue?
> The depends kind of prevents build failures by ensuring that can't be built
> until this one is in place, but it is certainly an ugly way to do it.
>
> Few little bits innline.
> > ---
> >
> > v2:
> > - add license headers,
> > - reorder alphabetically includes,
> > - add SUNXI_GPADC_ prefixes for defines,
> >
> > drivers/mfd/Kconfig | 14 +++
> > drivers/mfd/Makefile| 2 +
> > drivers/mfd/sunxi-gpadc-mfd.c | 197
> >
> > include/linux/mfd/sunxi-gpadc-mfd.h | 23 +
> > 4 files changed, 236 insertions(+)
> > create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
> > create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
[...]
> > +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
> > + {
> > + .name = "sun6i-a31-gpadc-iio",
> > + .resources = adc_resources,
> > + .num_resources = ARRAY_SIZE(adc_resources),
> > + }, {
> > + .name = "iio_hwmon",
> I still really dislike using this to force the probe of that driver but
> kind of up to the hwmon / mfd guys on this.
Can you at least say *why* you don't like it?
How else would it get probed?
> I don't have any better suggestions though..
> > + },
> > +};
[...]
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
> > + regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
> > + return ret;
> > + }
> > +
> > + dev_info(&pdev->dev, "successfully loaded\n");
> Seems like noise to me, but not my subsystem :)
Agreed, I don't allow this either.
[...]
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC
On 15/07/16 10:59, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. For now, only the ADC and the thermal
> sensor drivers are probed by the MFD, the touchscreen controller support
> will be added later.
>
> Signed-off-by: Quentin Schulz
Hmm. Previous patch includes the header this one creates. Ordering issue?
The depends kind of prevents build failures by ensuring that can't be built
until this one is in place, but it is certainly an ugly way to do it.
Few little bits innline.
> ---
>
> v2:
> - add license headers,
> - reorder alphabetically includes,
> - add SUNXI_GPADC_ prefixes for defines,
>
> drivers/mfd/Kconfig | 14 +++
> drivers/mfd/Makefile| 2 +
> drivers/mfd/sunxi-gpadc-mfd.c | 197
>
> include/linux/mfd/sunxi-gpadc-mfd.h | 23 +
> 4 files changed, 236 insertions(+)
> create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
> create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 1bcf601..67b55d0 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -82,6 +82,20 @@ config MFD_ATMEL_FLEXCOM
> by the probe function of this MFD driver according to a device tree
> property.
>
> +config MFD_SUNXI_ADC
> + tristate "ADC MFD core driver for sunxi platforms"
> + select MFD_CORE
> + select REGMAP_MMIO
> + help
> + Select this to get support for Allwinner SoCs (A10, A13 and A31) ADC.
> + This driver will only map the hardware interrupt and registers, you
> + have to select individual drivers based on this MFD to be able to use
> + the ADC or the thermal sensor. This will try to probe the ADC driver
> + sunxi-gpadc-iio and the hwmon driver iio_hwmon.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called sunxi-gpadc-mfd.
> +
> config MFD_ATMEL_HLCDC
> tristate "Atmel HLCDC (High-end LCD Controller)"
> select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 42a66e1..dcf43cd 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -201,6 +201,8 @@ obj-$(CONFIG_MFD_DLN2)+= dln2.o
> obj-$(CONFIG_MFD_RT5033) += rt5033.o
> obj-$(CONFIG_MFD_SKY81452) += sky81452.o
>
> +obj-$(CONFIG_MFD_SUNXI_ADC) += sunxi-gpadc-mfd.o
> +
> 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
> diff --git a/drivers/mfd/sunxi-gpadc-mfd.c b/drivers/mfd/sunxi-gpadc-mfd.c
> new file mode 100644
> index 000..f0005a6
> --- /dev/null
> +++ b/drivers/mfd/sunxi-gpadc-mfd.c
> @@ -0,0 +1,197 @@
> +/* ADC MFD core driver for sunxi platforms
> + *
> + * Copyright (c) 2016 Quentin Schulz
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> by
> + * the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#define SUNXI_IRQ_FIFO_DATA 0
> +#define SUNXI_IRQ_TEMP_DATA 1
> +
> +static struct resource adc_resources[] = {
> + {
> + .name = "FIFO_DATA_PENDING",
> + .start = SUNXI_IRQ_FIFO_DATA,
> + .end= SUNXI_IRQ_FIFO_DATA,
> + .flags = IORESOURCE_IRQ,
> + }, {
> + .name = "TEMP_DATA_PENDING",
> + .start = SUNXI_IRQ_TEMP_DATA,
> + .end= SUNXI_IRQ_TEMP_DATA,
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +static const struct regmap_irq sunxi_gpadc_mfd_regmap_irq[] = {
> + REGMAP_IRQ_REG(SUNXI_IRQ_FIFO_DATA, 0, BIT(16)),
> + REGMAP_IRQ_REG(SUNXI_IRQ_TEMP_DATA, 0, BIT(18)),
> +};
> +
> +static const struct regmap_irq_chip sunxi_gpadc_mfd_regmap_irq_chip = {
> + .name = "sunxi_gpadc_mfd_irq_chip",
> + .status_base = SUNXI_GPADC_TP_INT_FIFOS,
> + .ack_base = SUNXI_GPADC_TP_INT_FIFOS,
> + .mask_base = SUNXI_GPADC_TP_INT_FIFOC,
> + .init_ack_masked = true,
> + .mask_invert = true,
> + .irqs = sunxi_gpadc_mfd_regmap_irq,
> + .num_irqs = ARRAY_SIZE(sunxi_gpadc_mfd_regmap_irq),
> + .num_regs = 1,
> +};
> +
> +static struct mfd_cell sun4i_gpadc_mfd_cells[] = {
> + {
> + .name = "sun4i-a10-gpadc-iio",
> + .resources = adc_resources,
> + .num_resources = ARRAY_SIZE(adc_resources),
> + }, {
> + .name = "iio_hwmon",
> + }
> +};
> +
> +static struct mfd_cell sun5i_gpadc_mfd_cells[] = {
> + {
> + .name = "sun5i-a13-gpadc-iio",
> + .resources = adc_resources,
> + .num_resources = ARRAY_SIZE(adc_resources),
> +
Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC
On Fri, Jul 15, 2016 at 11:59:13AM +0200, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. For now, only the ADC and the thermal
> sensor drivers are probed by the MFD, the touchscreen controller support
> will be added later.
>
> Signed-off-by: Quentin Schulz
> ---
>
> v2:
> - add license headers,
> - reorder alphabetically includes,
> - add SUNXI_GPADC_ prefixes for defines,
>
> drivers/mfd/Kconfig | 14 +++
> drivers/mfd/Makefile| 2 +
> drivers/mfd/sunxi-gpadc-mfd.c | 197
>
> include/linux/mfd/sunxi-gpadc-mfd.h | 23 +
> 4 files changed, 236 insertions(+)
> create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
> create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 1bcf601..67b55d0 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -82,6 +82,20 @@ config MFD_ATMEL_FLEXCOM
> by the probe function of this MFD driver according to a device tree
> property.
>
> +config MFD_SUNXI_ADC
> + tristate "ADC MFD core driver for sunxi platforms"
> + select MFD_CORE
> + select REGMAP_MMIO
It should also depends on the architectures supported (and probably
COMPILE_TEST)
> + help
> + Select this to get support for Allwinner SoCs (A10, A13 and A31) ADC.
> + This driver will only map the hardware interrupt and registers, you
> + have to select individual drivers based on this MFD to be able to use
> + the ADC or the thermal sensor. This will try to probe the ADC driver
> + sunxi-gpadc-iio and the hwmon driver iio_hwmon.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called sunxi-gpadc-mfd.
> +
> config MFD_ATMEL_HLCDC
> tristate "Atmel HLCDC (High-end LCD Controller)"
> select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 42a66e1..dcf43cd 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -201,6 +201,8 @@ obj-$(CONFIG_MFD_DLN2)+= dln2.o
> obj-$(CONFIG_MFD_RT5033) += rt5033.o
> obj-$(CONFIG_MFD_SKY81452) += sky81452.o
>
> +obj-$(CONFIG_MFD_SUNXI_ADC) += sunxi-gpadc-mfd.o
> +
> 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
> diff --git a/drivers/mfd/sunxi-gpadc-mfd.c b/drivers/mfd/sunxi-gpadc-mfd.c
> new file mode 100644
> index 000..f0005a6
> --- /dev/null
> +++ b/drivers/mfd/sunxi-gpadc-mfd.c
> @@ -0,0 +1,197 @@
> +/* ADC MFD core driver for sunxi platforms
> + *
> + * Copyright (c) 2016 Quentin Schulz
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> by
> + * the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#define SUNXI_IRQ_FIFO_DATA 0
> +#define SUNXI_IRQ_TEMP_DATA 1
> +
> +static struct resource adc_resources[] = {
> + {
> + .name = "FIFO_DATA_PENDING",
> + .start = SUNXI_IRQ_FIFO_DATA,
> + .end= SUNXI_IRQ_FIFO_DATA,
> + .flags = IORESOURCE_IRQ,
> + }, {
> + .name = "TEMP_DATA_PENDING",
> + .start = SUNXI_IRQ_TEMP_DATA,
> + .end= SUNXI_IRQ_TEMP_DATA,
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +static const struct regmap_irq sunxi_gpadc_mfd_regmap_irq[] = {
> + REGMAP_IRQ_REG(SUNXI_IRQ_FIFO_DATA, 0, BIT(16)),
> + REGMAP_IRQ_REG(SUNXI_IRQ_TEMP_DATA, 0, BIT(18)),
> +};
> +
> +static const struct regmap_irq_chip sunxi_gpadc_mfd_regmap_irq_chip = {
> + .name = "sunxi_gpadc_mfd_irq_chip",
> + .status_base = SUNXI_GPADC_TP_INT_FIFOS,
> + .ack_base = SUNXI_GPADC_TP_INT_FIFOS,
> + .mask_base = SUNXI_GPADC_TP_INT_FIFOC,
> + .init_ack_masked = true,
> + .mask_invert = true,
> + .irqs = sunxi_gpadc_mfd_regmap_irq,
> + .num_irqs = ARRAY_SIZE(sunxi_gpadc_mfd_regmap_irq),
> + .num_regs = 1,
> +};
> +
> +static struct mfd_cell sun4i_gpadc_mfd_cells[] = {
> + {
> + .name = "sun4i-a10-gpadc-iio",
> + .resources = adc_resources,
> + .num_resources = ARRAY_SIZE(adc_resources),
> + }, {
> + .name = "iio_hwmon",
> + }
> +};
> +
> +static struct mfd_cell sun5i_gpadc_mfd_cells[] = {
> + {
> + .name = "sun5i-a13-gpadc-iio",
> + .resources = adc_resources,
> + .num_resources = ARRAY_SIZE(adc_resources),
> + }, {
> + .name = "iio_hwmon",
> + },
> +};
> +
> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
> + {
> +
[PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC
The Allwinner SoCs all have an ADC that can also act as a touchscreen
controller and a thermal sensor. For now, only the ADC and the thermal
sensor drivers are probed by the MFD, the touchscreen controller support
will be added later.
Signed-off-by: Quentin Schulz
---
v2:
- add license headers,
- reorder alphabetically includes,
- add SUNXI_GPADC_ prefixes for defines,
drivers/mfd/Kconfig | 14 +++
drivers/mfd/Makefile| 2 +
drivers/mfd/sunxi-gpadc-mfd.c | 197
include/linux/mfd/sunxi-gpadc-mfd.h | 23 +
4 files changed, 236 insertions(+)
create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 1bcf601..67b55d0 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -82,6 +82,20 @@ config MFD_ATMEL_FLEXCOM
by the probe function of this MFD driver according to a device tree
property.
+config MFD_SUNXI_ADC
+ tristate "ADC MFD core driver for sunxi platforms"
+ select MFD_CORE
+ select REGMAP_MMIO
+ help
+ Select this to get support for Allwinner SoCs (A10, A13 and A31) ADC.
+ This driver will only map the hardware interrupt and registers, you
+ have to select individual drivers based on this MFD to be able to use
+ the ADC or the thermal sensor. This will try to probe the ADC driver
+ sunxi-gpadc-iio and the hwmon driver iio_hwmon.
+
+ To compile this driver as a module, choose M here: the
+ module will be called sunxi-gpadc-mfd.
+
config MFD_ATMEL_HLCDC
tristate "Atmel HLCDC (High-end LCD Controller)"
select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 42a66e1..dcf43cd 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -201,6 +201,8 @@ obj-$(CONFIG_MFD_DLN2) += dln2.o
obj-$(CONFIG_MFD_RT5033) += rt5033.o
obj-$(CONFIG_MFD_SKY81452) += sky81452.o
+obj-$(CONFIG_MFD_SUNXI_ADC)+= sunxi-gpadc-mfd.o
+
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
diff --git a/drivers/mfd/sunxi-gpadc-mfd.c b/drivers/mfd/sunxi-gpadc-mfd.c
new file mode 100644
index 000..f0005a6
--- /dev/null
+++ b/drivers/mfd/sunxi-gpadc-mfd.c
@@ -0,0 +1,197 @@
+/* ADC MFD core driver for sunxi platforms
+ *
+ * Copyright (c) 2016 Quentin Schulz
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#define SUNXI_IRQ_FIFO_DATA0
+#define SUNXI_IRQ_TEMP_DATA1
+
+static struct resource adc_resources[] = {
+ {
+ .name = "FIFO_DATA_PENDING",
+ .start = SUNXI_IRQ_FIFO_DATA,
+ .end= SUNXI_IRQ_FIFO_DATA,
+ .flags = IORESOURCE_IRQ,
+ }, {
+ .name = "TEMP_DATA_PENDING",
+ .start = SUNXI_IRQ_TEMP_DATA,
+ .end= SUNXI_IRQ_TEMP_DATA,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static const struct regmap_irq sunxi_gpadc_mfd_regmap_irq[] = {
+ REGMAP_IRQ_REG(SUNXI_IRQ_FIFO_DATA, 0, BIT(16)),
+ REGMAP_IRQ_REG(SUNXI_IRQ_TEMP_DATA, 0, BIT(18)),
+};
+
+static const struct regmap_irq_chip sunxi_gpadc_mfd_regmap_irq_chip = {
+ .name = "sunxi_gpadc_mfd_irq_chip",
+ .status_base = SUNXI_GPADC_TP_INT_FIFOS,
+ .ack_base = SUNXI_GPADC_TP_INT_FIFOS,
+ .mask_base = SUNXI_GPADC_TP_INT_FIFOC,
+ .init_ack_masked = true,
+ .mask_invert = true,
+ .irqs = sunxi_gpadc_mfd_regmap_irq,
+ .num_irqs = ARRAY_SIZE(sunxi_gpadc_mfd_regmap_irq),
+ .num_regs = 1,
+};
+
+static struct mfd_cell sun4i_gpadc_mfd_cells[] = {
+ {
+ .name = "sun4i-a10-gpadc-iio",
+ .resources = adc_resources,
+ .num_resources = ARRAY_SIZE(adc_resources),
+ }, {
+ .name = "iio_hwmon",
+ }
+};
+
+static struct mfd_cell sun5i_gpadc_mfd_cells[] = {
+ {
+ .name = "sun5i-a13-gpadc-iio",
+ .resources = adc_resources,
+ .num_resources = ARRAY_SIZE(adc_resources),
+ }, {
+ .name = "iio_hwmon",
+ },
+};
+
+static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
+ {
+ .name = "sun6i-a31-gpadc-iio",
+ .resources = adc_resources,
+ .num_resources = ARRAY_SIZE(adc_resources),
+ }, {
+ .name = "iio_hwmon",
+ },
+};
+
+static const struct regmap_config sunxi_gpadc_mfd_regmap_config = {
+ .reg_bits = 32,
+ .val_bits
