* Re: [PATCH v5 2/2] ARM: dts: add support for Turris Omnia
From: Andreas Färber @ 2016-11-27 16:00 UTC (permalink / raw)
To: Gregory CLEMENT, Uwe Kleine-König
Cc: Mark Rutland, Andrew Lunn, Jason Cooper,
devicetree-u79uwXL29TY76Z2rM5mHXA, Tomas Hlavacek, Rob Herring,
Bedřicha Košatu,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Sebastian Hesselbarth, Michal Hrusecky
In-Reply-To: <87lgw7ilg9.fsf-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Am 25.11.2016 um 17:16 schrieb Gregory CLEMENT:
> On ven., nov. 25 2016, Uwe Kleine-König <uwe-rXY34ruvC2xidJT2blvkqNi2O/JbrIOy@public.gmane.org> wrote:
>> This machine is an open hardware router by cz.nic driven by a
>> Marvell Armada 385.
>>
>> Signed-off-by: Uwe Kleine-König <uwe-rXY34ruvC2xidJT2blvkqNi2O/JbrIOy@public.gmane.org>
>
> Applied on mvebu/dt with few changes:
[...]
>> +&spi0 {
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&spi0_pins &spi0cs0_pins>;
>> + status = "okay";
>> +
>> + spi-nor@0 {
>> + compatible = "spansion,s25fl164k", "jedec,spi-nor";
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + reg = <0>;
>> + spi-max-frequency = <40000000>;
>> +
>
> +
> + partitions {
> + compatible = "fixed-partitions";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> it is mandatory since v4.4 to use this pattern for partitions.
>
>
>> + partition@0 {
>> + reg = <0x0 0x00100000>;
>> + label = "U-Boot";
>> + };
>> +
>> + partition@1 {
> @0x100000
> We should use the reg value here ^
The unit name should be without 0x though. In your tree you seem to have
it correctly.
@Uwe: Note that I had already told CZ.NIC's Michal ~two weeks ago that I
have a WIP .dts for the Omnia - looks like no one knows what the other
is doing. :( My branch includes cleanups for 385 .dtsi and bug fixes for
the switch that I am not seeing in your series:
https://github.com/afaerber/linux/commits/omnia-next
I am still looking into phy backtraces when the network interfaces go down.
@Gregory: Can we please follow up with cleaning up these ugly
internal-regs and pcie-controller nodes for consistency?
Regards,
Andreas
>> + reg = <0x00100000 0x00700000>;
>> + label = "Rescue system";
>> + };
>
> + };
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 5/7] IIO: add bindings for stm32 IIO timer driver
From: Jonathan Cameron @ 2016-11-27 15:51 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: Lee Jones, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, Mark Rutland,
alexandre.torgue-qxv4g6HH51o, devicetree-u79uwXL29TY76Z2rM5mHXA,
Linux Kernel Mailing List, Thierry Reding,
linux-pwm-u79uwXL29TY76Z2rM5mHXA, knaack.h-Mmb7MZpHnFY,
Lars-Peter Clausen, Peter Meerwald-Stadler,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Fabrice Gasnier, Gerald Baeza, Arnaud Pouliquen, Linus Walleij,
Linaro Kernel Mailman List, Benjamin Gaignard
In-Reply-To: <CA+M3ks6LC5M3B01nRWh-bO79OOE11QcsypDXGpFAPzZ7Goc=Fw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 27/11/16 15:45, Benjamin Gaignard wrote:
> 2016-11-27 15:25 GMT+01:00 Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>> On 24/11/16 15:14, Benjamin Gaignard wrote:
>>> Define bindings for stm32 IIO timer
>>>
>>> version 2:
>>> - only keep one compatible
>>> - add DT parameters to set lists of the triggers:
>>> one list describe the triggers created by the device
>>> another one give the triggers accepted by the device
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard-qxv4g6HH51o@public.gmane.org>
>>> ---
>>> .../bindings/iio/timer/stm32-iio-timer.txt | 41 ++++++++++++++++++++++
>>> 1 file changed, 41 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt b/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
>>> new file mode 100644
>>> index 0000000..840b417
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
>>> @@ -0,0 +1,41 @@
>>> +timer IIO trigger bindings for STM32
>>> +
>>> +Must be a sub-node of STM32 general purpose timer driver
>> Add a cross reference...
>
> I will add it in v3
>
>>> +
>>> +Required parameters:
>>> +- compatible: must be "st,stm32-iio-timer"
>> st,stm32-adc-timer or something like that.
>
> I would prefer use st,stm32-timer-trigger because triggers can be used
> for multiple other devices (dac, adc, timers)
>
>>> +- interrupts: Interrupt for this device
>>> + See ../interrupt-controller/st,stm32-exti.txt
>>> +
>>> +Optional parameters:
>>> +- st,input-triggers-names: List of the possible input triggers for
>>> + the device
>>> +- st,output-triggers-names: List of the possible output triggers for
>>> + the device
>> What are input / output triggers?
>
> each hardware block can be the source of triggers (output triggers) or customer
> of some other trigger (input triggers).That what I have tried to
> describe in those two
> parameters
So this is really about using one timer as a prescaler for another?
I'd be tempted in the first instance to drop that functionality and just
describe the ones that drive the device sampling.
It's complex to describe and there is enough complexity in here already
to keep things busy for a while!
>
>>> +
>>> +Possible triggers are defined in include/dt-bindings/iio/timer/st,stm32-iio-timer.h
>>> +
>>> +Example:
>>> + gptimer1: gptimer1@40010000 {
>>> + compatible = "st,stm32-gptimer";
>>> + reg = <0x40010000 0x400>;
>>> + clocks = <&rcc 0 160>;
>>> + clock-names = "clk_int";
>>> +
>>> + pwm1@0 {
>>> + compatible = "st,stm32-pwm";
>>> + st,pwm-num-chan = <4>;
>>> + st,breakinput;
>>> + st,complementary;
>>> + };
>>> +
>>> + iiotimer1@0 {
>>> + compatible = "st,stm32-iio-timer";
>>> + interrupts = <27>;
>>> + st,input-triggers-names = TIM5_TRGO,
>>> + TIM2_TRGO,
>>> + TIM4_TRGO,
>>> + TIM3_TRGO;
>>> + st,output-triggers-names = TIM1_TRGO;
>>> + };
>>> + };
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH v8 3/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature
From: H. Nikolaus Schaller @ 2016-11-27 15:47 UTC (permalink / raw)
To: Jonathan Cameron, Dmitry Torokhov
Cc: Jonathan Cameron, Sebastian Reichel, Mark Rutland,
Benoît Cousson, Tony Lindgren, Russell King, Arnd Bergmann,
Michael Welling, Mika Penttilä, Javier Martinez Canillas,
Igor Grinberg, Andrew F. Davis, Mark Brown, Rob Herring,
Alexander Stein, Eric Engestrom, Hans de Goede,
Benjamin Tissoires
In-Reply-To: <8d3fc752-e69c-f568-4f71-4f1768a3fffe@kernel.org>
Hi Jonathan,
> Am 27.11.2016 um 12:02 schrieb Jonathan Cameron <jic23@kernel.org>:
>
> On 24/11/16 18:05, H. Nikolaus Schaller wrote:
>>
>>> Am 24.11.2016 um 18:38 schrieb Jonathan Cameron <jic23@jic23.retrosnub.co.uk>:
>>>
>>>
>>>
>>> On 22 November 2016 14:02:30 GMT+00:00, "H. Nikolaus Schaller" <hns@goldelico.com> wrote:
>>>> The tsc2007 chip not only has a resistive touch screen controller but
>>>> also an external AUX adc imput which can be used for an ambient
>>>> light sensor, battery voltage monitoring or any general purpose.
>>>>
>>>> Additionally it can measure the chip temperature.
>>>>
>>>> This extension provides an iio interface for these adc channels.
>>>>
>>>> Since it is not wasting much resources and is very straightforward,
>>>> we simply provide all other adc channels as optional iio interfaces
>>>> as weel. This can be used for debugging or special applications.
>>>>
>>>> This patch also splits the tsc2007 driver in several source files:
>>>> tsc2007.h -- constants, structs and stubs
>>>> tsc2007_core.c -- functional parts of the original driver
>>>> tsc2007_iio.c -- the optional iio stuff
>>>>
>>>> Makefile magic allows to conditionally link the iio stuff
>>>> if CONFIG_IIO=y or =m in a way that it works with
>>>> CONFIG_TOUCHSCREEN_TSC2007=m.
>>>>
>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>> Reviewed-by: Jonathan Cameron <jic23@kernel.org>
>>>> ---
>>>> drivers/input/touchscreen/Makefile | 7 +
>>>> drivers/input/touchscreen/tsc2007.h | 116
>>>> ++++++++++++++++
>>>> .../touchscreen/{tsc2007.c => tsc2007_core.c} | 95 +++----------
>>>> drivers/input/touchscreen/tsc2007_iio.c | 150
>>>> +++++++++++++++++++++
>>>> 4 files changed, 294 insertions(+), 74 deletions(-)
>>>> create mode 100644 drivers/input/touchscreen/tsc2007.h
>>>> rename drivers/input/touchscreen/{tsc2007.c => tsc2007_core.c} (86%)
>>>> create mode 100644 drivers/input/touchscreen/tsc2007_iio.c
>>>>
>>>> diff --git a/drivers/input/touchscreen/Makefile
>>>> b/drivers/input/touchscreen/Makefile
>>>> index 81b8645..3be0d19 100644
>>>> --- a/drivers/input/touchscreen/Makefile
>>>> +++ b/drivers/input/touchscreen/Makefile
>>>> @@ -80,6 +80,13 @@ obj-$(CONFIG_TOUCHSCREEN_TSC_SERIO) += tsc40.o
>>>> obj-$(CONFIG_TOUCHSCREEN_TSC200X_CORE) += tsc200x-core.o
>>>> obj-$(CONFIG_TOUCHSCREEN_TSC2004) += tsc2004.o
>>>> obj-$(CONFIG_TOUCHSCREEN_TSC2005) += tsc2005.o
>>>> +tsc2007-y := tsc2007_core.o
>>>> +ifeq ($(CONFIG_IIO),y)
>>>> +tsc2007-y += tsc2007_iio.o
>>>> +endif
>>>> +ifeq ($(CONFIG_IIO),m)
>>>> +tsc2007-y += tsc2007_iio.o
>>>
>>> Not tsc2007-m ?
>>>
>>> I don't follow how this works!
>>
>> I guess tsc2007-y is an internal collector variable name
>> for multiple .o components. Sort of a "library" object.
>>
>> While
>>
>> obj-y += tsc2007.o adds it to the kernel
>> obj-m += tsc2007.o adds it to the modules
>>
>> I am not sure if my explanation is correct but it appears
>> to work that way.
>>
>> Anyways what shall we do? If CONFIG_TOUCHSCREEN_TSC2007=y
>> and IIO=m we have a problem that we need dynamic binding.
> Yes, we just need to block that particular combination. Only build in the IIO support
> if it is also built in. That's way I thought we'd want to add it tsc2007-m which would
> only be used if tsc2007 as a whole was built as a module.
Yes, that is what one could expect.
> Otherwise it would be ignored (I think!)
>
> I'm not seeing this structure anywhere else in kernel
It is also not described in that way:
http://lxr.free-electrons.com/source/Documentation/kbuild/modules.txt#L146
it only talks about obj-m and <module_name>-y but not <module_name>-m
> - hence cc'd Yann and the Kbuild list
> to see if they can offer some advices.
Thanks!
BTW, the other tsc2007 and ads7846 patches could already be merged (if there
are no more changes needed) since this one only depends on the result of applying
all others before.
>
> As a quick summary, we are looking to add IIO support to this driver in the following circumstances.
>
> IIO and this driver are modules. (ideally handling the dependencies nicely)
> IIO and this driver are both built in.
>
> Problem case is driver built in and IIO as a module.
>
> Jonathan
>>
>>>> +endif
>>>> obj-$(CONFIG_TOUCHSCREEN_TSC2007) += tsc2007.o
>>>> obj-$(CONFIG_TOUCHSCREEN_UCB1400) += ucb1400_ts.o
>>>> obj-$(CONFIG_TOUCHSCREEN_WACOM_W8001) += wacom_w8001.o
>>>> diff --git a/drivers/input/touchscreen/tsc2007.h
>>>> b/drivers/input/touchscreen/tsc2007.h
>>>> new file mode 100644
>>>> index 0000000..c25932f
>>>> --- /dev/null
>>>> +++ b/drivers/input/touchscreen/tsc2007.h
>>>> @@ -0,0 +1,116 @@
>>>> +/*
>>>> + * Copyright (c) 2008 MtekVision Co., Ltd.
>>>> + * Kwangwoo Lee <kwlee@mtekvision.com>
>>>> + *
>>>> + * Using code from:
>>>> + * - ads7846.c
>>>> + * Copyright (c) 2005 David Brownell
>>>> + * Copyright (c) 2006 Nokia Corporation
>>>> + * - corgi_ts.c
>>>> + * Copyright (C) 2004-2005 Richard Purdie
>>>> + * - omap_ts.[hc], ads7846.h, ts_osk.c
>>>> + * Copyright (C) 2002 MontaVista Software
>>>> + * Copyright (C) 2004 Texas Instruments
>>>> + * Copyright (C) 2005 Dirk Behme
>>>> + *
>>>> + * 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 <linux/input/touchscreen.h>
>>>> +
>>>> +#define TSC2007_MEASURE_TEMP0 (0x0 << 4)
>>>> +#define TSC2007_MEASURE_AUX (0x2 << 4)
>>>> +#define TSC2007_MEASURE_TEMP1 (0x4 << 4)
>>>> +#define TSC2007_ACTIVATE_XN (0x8 << 4)
>>>> +#define TSC2007_ACTIVATE_YN (0x9 << 4)
>>>> +#define TSC2007_ACTIVATE_YP_XN (0xa << 4)
>>>> +#define TSC2007_SETUP (0xb << 4)
>>>> +#define TSC2007_MEASURE_X (0xc << 4)
>>>> +#define TSC2007_MEASURE_Y (0xd << 4)
>>>> +#define TSC2007_MEASURE_Z1 (0xe << 4)
>>>> +#define TSC2007_MEASURE_Z2 (0xf << 4)
>>>> +
>>>> +#define TSC2007_POWER_OFF_IRQ_EN (0x0 << 2)
>>>> +#define TSC2007_ADC_ON_IRQ_DIS0 (0x1 << 2)
>>>> +#define TSC2007_ADC_OFF_IRQ_EN (0x2 << 2)
>>>> +#define TSC2007_ADC_ON_IRQ_DIS1 (0x3 << 2)
>>>> +
>>>> +#define TSC2007_12BIT (0x0 << 1)
>>>> +#define TSC2007_8BIT (0x1 << 1)
>>>> +
>>>> +#define MAX_12BIT ((1 << 12) - 1)
>>>> +
>>>> +#define ADC_ON_12BIT (TSC2007_12BIT | TSC2007_ADC_ON_IRQ_DIS0)
>>>> +
>>>> +#define READ_Y (ADC_ON_12BIT | TSC2007_MEASURE_Y)
>>>> +#define READ_Z1 (ADC_ON_12BIT | TSC2007_MEASURE_Z1)
>>>> +#define READ_Z2 (ADC_ON_12BIT | TSC2007_MEASURE_Z2)
>>>> +#define READ_X (ADC_ON_12BIT | TSC2007_MEASURE_X)
>>>> +#define PWRDOWN (TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
>>>> +
>>>> +struct ts_event {
>>>> + u16 x;
>>>> + u16 y;
>>>> + u16 z1, z2;
>>>> +};
>>>> +
>>>> +struct tsc2007 {
>>>> + struct input_dev *input;
>>>> + char phys[32];
>>>> +
>>>> + struct i2c_client *client;
>>>> +
>>>> + u16 model;
>>>> + u16 x_plate_ohms;
>>>> +
>>>> + struct touchscreen_properties prop;
>>>> +
>>>> + bool report_resistance;
>>>> + u16 min_x;
>>>> + u16 min_y;
>>>> + u16 max_x;
>>>> + u16 max_y;
>>>> + u16 max_rt;
>>>> + unsigned long poll_period; /* in jiffies */
>>>> + int fuzzx;
>>>> + int fuzzy;
>>>> + int fuzzz;
>>>> +
>>>> + unsigned int gpio;
>>>> + int irq;
>>>> +
>>>> + wait_queue_head_t wait;
>>>> + bool stopped;
>>>> + bool pendown;
>>>> +
>>>> + int (*get_pendown_state)(struct device *);
>>>> + void (*clear_penirq)(void);
>>>> +
>>>> + struct mutex mlock;
>>>> + struct iio_dev *iio_dev; /* optional */
>>>> +};
>>>> +
>>>> +int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd);
>>>> +u32 tsc2007_calculate_resistance(struct tsc2007 *tsc,
>>>> + struct ts_event *tc);
>>>> +bool tsc2007_is_pen_down(struct tsc2007 *ts);
>>>> +
>>>> +#if IS_ENABLED(CONFIG_IIO)
>>>> +
>>>> +/* defined in tsc2007_iio.c */
>>>> +int tsc2007_iio_configure(struct tsc2007 *ts);
>>>> +void tsc2007_iio_unconfigure(struct tsc2007 *ts);
>>>> +
>>>> +#else /* CONFIG_IIO */
>>>> +
>>>> +static inline int tsc2007_iio_configure(struct tsc2007 *ts)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +static inline void tsc2007_iio_unconfigure(struct tsc2007 *ts)
>>>> +{
>>>> +}
>>>> +
>>>> +#endif /* CONFIG_IIO */
>>>> diff --git a/drivers/input/touchscreen/tsc2007.c
>>>> b/drivers/input/touchscreen/tsc2007_core.c
>>>> similarity index 86%
>>>> rename from drivers/input/touchscreen/tsc2007.c
>>>> rename to drivers/input/touchscreen/tsc2007_core.c
>>>> index 76b462b..812ded8 100644
>>>> --- a/drivers/input/touchscreen/tsc2007.c
>>>> +++ b/drivers/input/touchscreen/tsc2007_core.c
>>>> @@ -27,79 +27,11 @@
>>>> #include <linux/i2c.h>
>>>> #include <linux/i2c/tsc2007.h>
>>>> #include <linux/of_device.h>
>>>> -#include <linux/of.h>
>>>> #include <linux/of_gpio.h>
>>>> -#include <linux/input/touchscreen.h>
>>>> -
>>>> -#define TSC2007_MEASURE_TEMP0 (0x0 << 4)
>>>> -#define TSC2007_MEASURE_AUX (0x2 << 4)
>>>> -#define TSC2007_MEASURE_TEMP1 (0x4 << 4)
>>>> -#define TSC2007_ACTIVATE_XN (0x8 << 4)
>>>> -#define TSC2007_ACTIVATE_YN (0x9 << 4)
>>>> -#define TSC2007_ACTIVATE_YP_XN (0xa << 4)
>>>> -#define TSC2007_SETUP (0xb << 4)
>>>> -#define TSC2007_MEASURE_X (0xc << 4)
>>>> -#define TSC2007_MEASURE_Y (0xd << 4)
>>>> -#define TSC2007_MEASURE_Z1 (0xe << 4)
>>>> -#define TSC2007_MEASURE_Z2 (0xf << 4)
>>>> -
>>>> -#define TSC2007_POWER_OFF_IRQ_EN (0x0 << 2)
>>>> -#define TSC2007_ADC_ON_IRQ_DIS0 (0x1 << 2)
>>>> -#define TSC2007_ADC_OFF_IRQ_EN (0x2 << 2)
>>>> -#define TSC2007_ADC_ON_IRQ_DIS1 (0x3 << 2)
>>>> -
>>>> -#define TSC2007_12BIT (0x0 << 1)
>>>> -#define TSC2007_8BIT (0x1 << 1)
>>>> -
>>>> -#define MAX_12BIT ((1 << 12) - 1)
>>>> -
>>>> -#define ADC_ON_12BIT (TSC2007_12BIT | TSC2007_ADC_ON_IRQ_DIS0)
>>>> -
>>>> -#define READ_Y (ADC_ON_12BIT | TSC2007_MEASURE_Y)
>>>> -#define READ_Z1 (ADC_ON_12BIT | TSC2007_MEASURE_Z1)
>>>> -#define READ_Z2 (ADC_ON_12BIT | TSC2007_MEASURE_Z2)
>>>> -#define READ_X (ADC_ON_12BIT | TSC2007_MEASURE_X)
>>>> -#define PWRDOWN (TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
>>>> -
>>>> -struct ts_event {
>>>> - u16 x;
>>>> - u16 y;
>>>> - u16 z1, z2;
>>>> -};
>>>> -
>>>> -struct tsc2007 {
>>>> - struct input_dev *input;
>>>> - char phys[32];
>>>> -
>>>> - struct i2c_client *client;
>>>> -
>>>> - u16 model;
>>>> - u16 x_plate_ohms;
>>>> -
>>>> - struct touchscreen_properties prop;
>>>> -
>>>> - bool report_resistance;
>>>> - u16 min_x;
>>>> - u16 min_y;
>>>> - u16 max_x;
>>>> - u16 max_y;
>>>> - u16 max_rt;
>>>> - unsigned long poll_period; /* in jiffies */
>>>> - int fuzzx;
>>>> - int fuzzy;
>>>> - int fuzzz;
>>>> -
>>>> - unsigned gpio;
>>>> - int irq;
>>>> -
>>>> - wait_queue_head_t wait;
>>>> - bool stopped;
>>>> +#include "tsc2007.h"
>>>>
>>>> - int (*get_pendown_state)(struct device *);
>>>> - void (*clear_penirq)(void);
>>>> -};
>>>>
>>>> -static inline int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
>>>> +int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
>>>> {
>>>> s32 data;
>>>> u16 val;
>>>> @@ -137,7 +69,7 @@ static void tsc2007_read_values(struct tsc2007 *tsc,
>>>> struct ts_event *tc)
>>>> tsc2007_xfer(tsc, PWRDOWN);
>>>> }
>>>>
>>>> -static u32 tsc2007_calculate_resistance(struct tsc2007 *tsc,
>>>> +u32 tsc2007_calculate_resistance(struct tsc2007 *tsc,
>>>> struct ts_event *tc)
>>>> {
>>>> u32 rt = 0;
>>>> @@ -158,7 +90,7 @@ static u32 tsc2007_calculate_resistance(struct
>>>> tsc2007 *tsc,
>>>> return rt;
>>>> }
>>>>
>>>> -static bool tsc2007_is_pen_down(struct tsc2007 *ts)
>>>> +bool tsc2007_is_pen_down(struct tsc2007 *ts)
>>>> {
>>>> /*
>>>> * NOTE: We can't rely on the pressure to determine the pen down
>>>> @@ -191,7 +123,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void
>>>> *handle)
>>>> while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>>>>
>>>> /* pen is down, continue with the measurement */
>>>> +
>>>> + mutex_lock(&ts->mlock);
>>>> tsc2007_read_values(ts, &tc);
>>>> + mutex_unlock(&ts->mlock);
>>>>
>>>> rt = tsc2007_calculate_resistance(ts, &tc);
>>>>
>>>> @@ -441,7 +376,8 @@ static void tsc2007_call_exit_platform_hw(void
>>>> *data)
>>>> static int tsc2007_probe(struct i2c_client *client,
>>>> const struct i2c_device_id *id)
>>>> {
>>>> - const struct tsc2007_platform_data *pdata =
>>>> dev_get_platdata(&client->dev);
>>>> + const struct tsc2007_platform_data *pdata =
>>>> + dev_get_platdata(&client->dev);
>>>> struct tsc2007 *ts;
>>>> struct input_dev *input_dev;
>>>> int err;
>>>> @@ -463,7 +399,9 @@ static int tsc2007_probe(struct i2c_client *client,
>>>> ts->client = client;
>>>> ts->irq = client->irq;
>>>> ts->input = input_dev;
>>>> +
>>>> init_waitqueue_head(&ts->wait);
>>>> + mutex_init(&ts->mlock);
>>>>
>>>> snprintf(ts->phys, sizeof(ts->phys),
>>>> "%s/input0", dev_name(&client->dev));
>>>> @@ -534,7 +472,7 @@ static int tsc2007_probe(struct i2c_client *client,
>>>> if (err < 0) {
>>>> dev_err(&client->dev,
>>>> "Failed to setup chip: %d\n", err);
>>>> - return err; /* usually, chip does not respond */
>>>> + return err; /* chip does not respond */
>>>> }
>>>>
>>>> err = input_register_device(input_dev);
>>>> @@ -544,6 +482,14 @@ static int tsc2007_probe(struct i2c_client
>>>> *client,
>>>> return err;
>>>> }
>>>>
>>>> + return tsc2007_iio_configure(ts);
>>>> +}
>>>> +
>>>> +static int tsc2007_remove(struct i2c_client *client)
>>>> +{
>>>> + struct tsc2007 *ts = i2c_get_clientdata(client);
>>>> +
>>>> + tsc2007_iio_unconfigure(ts);
>>>> return 0;
>>>> }
>>>>
>>>> @@ -569,6 +515,7 @@ static struct i2c_driver tsc2007_driver = {
>>>> },
>>>> .id_table = tsc2007_idtable,
>>>> .probe = tsc2007_probe,
>>>> + .remove = tsc2007_remove,
>>>> };
>>>>
>>>> module_i2c_driver(tsc2007_driver);
>>>> diff --git a/drivers/input/touchscreen/tsc2007_iio.c
>>>> b/drivers/input/touchscreen/tsc2007_iio.c
>>>> new file mode 100644
>>>> index 0000000..ed79944
>>>> --- /dev/null
>>>> +++ b/drivers/input/touchscreen/tsc2007_iio.c
>>>> @@ -0,0 +1,150 @@
>>>> +/*
>>>> + * Copyright (c) 2016 Golden Delicious Comp. GmbH&Co. KG
>>>> + * Nikolaus Schaller <hns@goldelico.com>
>>>> + *
>>>> + * 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 <linux/i2c.h>
>>>> +#include <linux/iio/iio.h>
>>>> +#include "tsc2007.h"
>>>> +
>>>> +struct tsc2007_iio {
>>>> + struct tsc2007 *ts;
>>>> +};
>>>> +
>>>> +#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
>>>> +{ \
>>>> + .datasheet_name = _name, \
>>>> + .type = _type, \
>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>>>> + BIT(_chan_info), \
>>>> + .indexed = 1, \
>>>> + .channel = _chan, \
>>>> +}
>>>> +
>>>> +static const struct iio_chan_spec tsc2007_iio_channel[] = {
>>>> + TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>>> + TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>>> + TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>>> + TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>>> + TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>>> + TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms?
>>>> */
>>>> + TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
>>>> + TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
>>>> + TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
>>>> +};
>>>> +
>>>> +static int tsc2007_read_raw(struct iio_dev *indio_dev,
>>>> + struct iio_chan_spec const *chan, int *val, int *val2, long mask)
>>>> +{
>>>> + struct tsc2007_iio *iio = iio_priv(indio_dev);
>>>> + struct tsc2007 *tsc = iio->ts;
>>>> + int adc_chan = chan->channel;
>>>> + int ret = 0;
>>>> +
>>>> + if (adc_chan >= ARRAY_SIZE(tsc2007_iio_channel))
>>>> + return -EINVAL;
>>>> +
>>>> + if (mask != IIO_CHAN_INFO_RAW)
>>>> + return -EINVAL;
>>>> +
>>>> + mutex_lock(&tsc->mlock);
>>>> +
>>>> + switch (chan->channel) {
>>>> + case 0:
>>>> + *val = tsc2007_xfer(tsc, READ_X);
>>>> + break;
>>>> + case 1:
>>>> + *val = tsc2007_xfer(tsc, READ_Y);
>>>> + break;
>>>> + case 2:
>>>> + *val = tsc2007_xfer(tsc, READ_Z1);
>>>> + break;
>>>> + case 3:
>>>> + *val = tsc2007_xfer(tsc, READ_Z2);
>>>> + break;
>>>> + case 4:
>>>> + *val = tsc2007_xfer(tsc, (ADC_ON_12BIT | TSC2007_MEASURE_AUX));
>>>> + break;
>>>> + case 5: {
>>>> + struct ts_event tc;
>>>> +
>>>> + tc.x = tsc2007_xfer(tsc, READ_X);
>>>> + tc.z1 = tsc2007_xfer(tsc, READ_Z1);
>>>> + tc.z2 = tsc2007_xfer(tsc, READ_Z2);
>>>> + *val = tsc2007_calculate_resistance(tsc, &tc);
>>>> + break;
>>>> + }
>>>> + case 6:
>>>> + *val = tsc2007_is_pen_down(tsc);
>>>> + break;
>>>> + case 7:
>>>> + *val = tsc2007_xfer(tsc,
>>>> + (ADC_ON_12BIT | TSC2007_MEASURE_TEMP0));
>>>> + break;
>>>> + case 8:
>>>> + *val = tsc2007_xfer(tsc,
>>>> + (ADC_ON_12BIT | TSC2007_MEASURE_TEMP1));
>>>> + break;
>>>> + }
>>>> +
>>>> + /* Prepare for next touch reading - power down ADC, enable PENIRQ */
>>>> + tsc2007_xfer(tsc, PWRDOWN);
>>>> +
>>>> + mutex_unlock(&tsc->mlock);
>>>> +
>>>> + ret = IIO_VAL_INT;
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static const struct iio_info tsc2007_iio_info = {
>>>> + .read_raw = tsc2007_read_raw,
>>>> + .driver_module = THIS_MODULE,
>>>> +};
>>>> +
>>>> +int tsc2007_iio_configure(struct tsc2007 *ts)
>>>> +{
>>>> + int err;
>>>> + struct iio_dev *indio_dev;
>>>> + struct tsc2007_iio *iio;
>>>> +
>>>> + indio_dev = devm_iio_device_alloc(&ts->client->dev,
>>>> + sizeof(struct tsc2007_iio));
>>>> + if (!indio_dev) {
>>>> + dev_err(&ts->client->dev, "iio_device_alloc failed\n");
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + iio = iio_priv(indio_dev);
>>>> + iio->ts = ts;
>>>> + ts->iio_dev = (void *) indio_dev;
>>>> +
>>>> + indio_dev->name = "tsc2007";
>>>> + indio_dev->dev.parent = &ts->client->dev;
>>>> + indio_dev->info = &tsc2007_iio_info;
>>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>>> + indio_dev->channels = tsc2007_iio_channel;
>>>> + indio_dev->num_channels = ARRAY_SIZE(tsc2007_iio_channel);
>>>> +
>>>> + err = iio_device_register(indio_dev);
>>>> + if (err < 0) {
>>>> + dev_err(&ts->client->dev, "iio_device_register() failed: %d\n",
>>>> + err);
>>>> + return err;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(tsc2007_iio_configure);
>>>> +
>>>> +void tsc2007_iio_unconfigure(struct tsc2007 *ts)
>>>> +{
>>>> + struct iio_dev *indio_dev = ts->iio_dev;
>>>> +
>>>> + iio_device_unregister(indio_dev);
>>>> +}
>>>> +EXPORT_SYMBOL(tsc2007_iio_unconfigure);
>>>
>>> --
>>> Sent from my Android device with K-9 Mail. Please excuse my brevity.
>>
>
BR,
Nikolaus
^ permalink raw reply
* Re: [PATCH v2 5/7] IIO: add bindings for stm32 IIO timer driver
From: Benjamin Gaignard @ 2016-11-27 15:45 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lee Jones, robh+dt, Mark Rutland, alexandre.torgue, devicetree,
Linux Kernel Mailing List, Thierry Reding, linux-pwm, knaack.h,
Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio,
linux-arm-kernel, Fabrice Gasnier, Gerald Baeza, Arnaud Pouliquen,
Linus Walleij, Linaro Kernel Mailman List, Benjamin Gaignard
In-Reply-To: <b2002a81-49fe-af2a-f13a-9f09831aa530@kernel.org>
2016-11-27 15:25 GMT+01:00 Jonathan Cameron <jic23@kernel.org>:
> On 24/11/16 15:14, Benjamin Gaignard wrote:
>> Define bindings for stm32 IIO timer
>>
>> version 2:
>> - only keep one compatible
>> - add DT parameters to set lists of the triggers:
>> one list describe the triggers created by the device
>> another one give the triggers accepted by the device
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>> .../bindings/iio/timer/stm32-iio-timer.txt | 41 ++++++++++++++++++++++
>> 1 file changed, 41 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt b/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
>> new file mode 100644
>> index 0000000..840b417
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
>> @@ -0,0 +1,41 @@
>> +timer IIO trigger bindings for STM32
>> +
>> +Must be a sub-node of STM32 general purpose timer driver
> Add a cross reference...
I will add it in v3
>> +
>> +Required parameters:
>> +- compatible: must be "st,stm32-iio-timer"
> st,stm32-adc-timer or something like that.
I would prefer use st,stm32-timer-trigger because triggers can be used
for multiple other devices (dac, adc, timers)
>> +- interrupts: Interrupt for this device
>> + See ../interrupt-controller/st,stm32-exti.txt
>> +
>> +Optional parameters:
>> +- st,input-triggers-names: List of the possible input triggers for
>> + the device
>> +- st,output-triggers-names: List of the possible output triggers for
>> + the device
> What are input / output triggers?
each hardware block can be the source of triggers (output triggers) or customer
of some other trigger (input triggers).That what I have tried to
describe in those two
parameters
>> +
>> +Possible triggers are defined in include/dt-bindings/iio/timer/st,stm32-iio-timer.h
>> +
>> +Example:
>> + gptimer1: gptimer1@40010000 {
>> + compatible = "st,stm32-gptimer";
>> + reg = <0x40010000 0x400>;
>> + clocks = <&rcc 0 160>;
>> + clock-names = "clk_int";
>> +
>> + pwm1@0 {
>> + compatible = "st,stm32-pwm";
>> + st,pwm-num-chan = <4>;
>> + st,breakinput;
>> + st,complementary;
>> + };
>> +
>> + iiotimer1@0 {
>> + compatible = "st,stm32-iio-timer";
>> + interrupts = <27>;
>> + st,input-triggers-names = TIM5_TRGO,
>> + TIM2_TRGO,
>> + TIM4_TRGO,
>> + TIM3_TRGO;
>> + st,output-triggers-names = TIM1_TRGO;
>> + };
>> + };
>>
>
^ permalink raw reply
* Re: [PATCH v2 6/7] IIO: add STM32 IIO timer driver
From: Jonathan Cameron @ 2016-11-27 15:42 UTC (permalink / raw)
To: Benjamin Gaignard, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
alexandre.torgue-qxv4g6HH51o, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
linux-pwm-u79uwXL29TY76Z2rM5mHXA, knaack.h-Mmb7MZpHnFY,
lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: fabrice.gasnier-qxv4g6HH51o, gerald.baeza-qxv4g6HH51o,
arnaud.pouliquen-qxv4g6HH51o,
linus.walleij-QSEj5FYQhm4dnm+yROfE0A,
linaro-kernel-cunTk1MwBs8s++Sfvej+rw, Benjamin Gaignard
In-Reply-To: <1480000463-9625-7-git-send-email-benjamin.gaignard-qxv4g6HH51o@public.gmane.org>
I delved into the datasheet after trying to figure this out, so I think
I now sort of understand your intent, but please do answer the questions
inline.
On 24/11/16 15:14, Benjamin Gaignard wrote:
> Timers IPs can be used to generate triggers for other IPs like
> DAC, ADC or other timers.
> Each trigger may result of timer internals signals like counter enable,
> reset or edge, this configuration could be done through "master_mode"
> device attribute.
>
> A timer device could be triggered by other timers, we use the trigger
> name and is_stm32_iio_timer_trigger() function to distinguish them
> and configure IP input switch.
The presence of an IIO device in here was a suprise.. What is it actually for?
I think this needs some examples of usage to make it clear what the aim is.
I was basically expecting to see a driver instantiating one iio trigger
per timer that can act as a trigger. Those would each have sampling frequency
controls and basica enable / disable.
I'm seeing something much more complex here so additional explanation is
needed.
>
> Timer may also decide on which event (edge, level) they could
> be activated by a trigger, this configuration is done by writing in
> "slave_mode" device attribute.
Really? Sounds like magic numbers in sysfs which is never a good idea.
Please document those attributes / or break them up into elements that
don't require magic numbers.
>
> Since triggers could also be used by DAC or ADC their names are defined
> in include/dt-bindings/iio/timer/st,stm32-iio-timer.h so those IPs will be able
> to configure themselves in valid_trigger function
>
> Trigger have a "sampling_frequency" attribute which allow to configure
> timer sampling frequency without using pwm interface
>
> version 2:
> - keep only one compatible
Hmm. I'm not sure I like this as such. We are actually dealing with lots
of instances of a hardware block with only a small amount of shared
infrastrcuture (which is classic mfd teritory). So to my mind we
should have a separate device for each.
> - use st,input-triggers-names and st,output-triggers-names
> to know which triggers are accepted and/or create by the device
I'm not following why we have this cascade setup?
These are triggers, not devices in the IIO context. We need some detailed
description of why you have it setup like this. This would include the
ABI with examples of how you are using it.
Basically I don't currently understand what you are doing :(
Thanks,
Jonathan
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard-qxv4g6HH51o@public.gmane.org>
> ---
> drivers/iio/Kconfig | 2 +-
> drivers/iio/Makefile | 1 +
> drivers/iio/timer/Kconfig | 15 +
> drivers/iio/timer/Makefile | 1 +
> drivers/iio/timer/stm32-iio-timer.c | 448 +++++++++++++++++++++
> drivers/iio/trigger/Kconfig | 1 -
> include/dt-bindings/iio/timer/st,stm32-iio-timer.h | 23 ++
> include/linux/iio/timer/stm32-iio-timers.h | 16 +
> 8 files changed, 505 insertions(+), 2 deletions(-)
> create mode 100644 drivers/iio/timer/Kconfig
> create mode 100644 drivers/iio/timer/Makefile
> create mode 100644 drivers/iio/timer/stm32-iio-timer.c
> create mode 100644 include/dt-bindings/iio/timer/st,stm32-iio-timer.h
> create mode 100644 include/linux/iio/timer/stm32-iio-timers.h
>
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index 6743b18..2de2a80 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -90,5 +90,5 @@ source "drivers/iio/potentiometer/Kconfig"
> source "drivers/iio/pressure/Kconfig"
> source "drivers/iio/proximity/Kconfig"
> source "drivers/iio/temperature/Kconfig"
> -
> +source "drivers/iio/timer/Kconfig"
> endif # IIO
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index 87e4c43..b797c08 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -32,4 +32,5 @@ obj-y += potentiometer/
> obj-y += pressure/
> obj-y += proximity/
> obj-y += temperature/
> +obj-y += timer/
> obj-y += trigger/
> diff --git a/drivers/iio/timer/Kconfig b/drivers/iio/timer/Kconfig
> new file mode 100644
> index 0000000..7a73bc6
> --- /dev/null
> +++ b/drivers/iio/timer/Kconfig
> @@ -0,0 +1,15 @@
> +#
> +# Timers drivers
> +
> +menu "Timers"
> +
> +config IIO_STM32_TIMER
> + tristate "stm32 iio timer"
> + depends on ARCH_STM32
> + depends on OF
> + select IIO_TRIGGERED_EVENT
> + select MFD_STM32_GP_TIMER
> + help
> + Select this option to enable stm32 timers hardware IPs
> +
> +endmenu
> diff --git a/drivers/iio/timer/Makefile b/drivers/iio/timer/Makefile
> new file mode 100644
> index 0000000..a360c9f
> --- /dev/null
> +++ b/drivers/iio/timer/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_IIO_STM32_TIMER) += stm32-iio-timer.o
> diff --git a/drivers/iio/timer/stm32-iio-timer.c b/drivers/iio/timer/stm32-iio-timer.c
> new file mode 100644
> index 0000000..35f2687
> --- /dev/null
> +++ b/drivers/iio/timer/stm32-iio-timer.c
> @@ -0,0 +1,448 @@
> +/*
> + * stm32-iio-timer.c
> + *
> + * Copyright (C) STMicroelectronics 2016
> + * Author: Benjamin Gaignard <benjamin.gaignard-qxv4g6HH51o@public.gmane.org> for STMicroelectronics.
> + * License terms: GNU General Public License (GPL), version 2
> + */
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/timer/stm32-iio-timers.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_event.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/stm32-gptimer.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#define DRIVER_NAME "stm32-iio-timer"
> +
> +struct stm32_iio_timer_dev {
> + struct device *dev;
> + struct regmap *regmap;
> + struct clk *clk;
> + int irq;
> + bool own_timer;
> + unsigned int sampling_frequency;
> + struct iio_trigger *active_trigger;
> +};
> +
> +static ssize_t _store_frequency(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct iio_trigger *trig = to_iio_trigger(dev);
> + struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
> + unsigned int freq;
> + int ret;
> +
> + ret = kstrtouint(buf, 10, &freq);
> + if (ret)
> + return ret;
> +
> + stm32->sampling_frequency = freq;
> +
> + return len;
> +}
> +
> +static ssize_t _read_frequency(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_trigger *trig = to_iio_trigger(dev);
> + struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
> + unsigned long long freq = stm32->sampling_frequency;
> + u32 psc, arr, cr1;
> +
> + regmap_read(stm32->regmap, TIM_CR1, &cr1);
> + regmap_read(stm32->regmap, TIM_PSC, &psc);
> + regmap_read(stm32->regmap, TIM_ARR, &arr);
> +
> + if (psc && arr && (cr1 & TIM_CR1_CEN)) {
> + freq = (unsigned long long)clk_get_rate(stm32->clk);
> + do_div(freq, psc);
> + do_div(freq, arr);
> + }
> +
> + return sprintf(buf, "%d\n", (unsigned int)freq);
> +}
> +
> +static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
> + _read_frequency,
> + _store_frequency);
> +
> +static struct attribute *stm32_trigger_attrs[] = {
> + &iio_dev_attr_sampling_frequency.dev_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group stm32_trigger_attr_group = {
> + .attrs = stm32_trigger_attrs,
> +};
> +
> +static const struct attribute_group *stm32_trigger_attr_groups[] = {
> + &stm32_trigger_attr_group,
> + NULL,
> +};
> +
> +static
> +ssize_t _show_master_mode(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
> + u32 cr2;
> +
> + regmap_read(stm32->regmap, TIM_CR2, &cr2);
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n", (cr2 >> 4) & 0x7);
> +}
> +
> +static
> +ssize_t _store_master_mode(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
> + u8 mode;
> + int ret;
> +
> + ret = kstrtou8(buf, 10, &mode);
> + if (ret)
> + return ret;
> +
> + if (mode > 0x7)
> + return -EINVAL;
> +
> + regmap_update_bits(stm32->regmap, TIM_CR2, TIM_CR2_MMS, mode << 4);
> +
> + return len;
> +}
> +
> +static IIO_DEVICE_ATTR(master_mode, S_IRUGO | S_IWUSR,
> + _show_master_mode,
> + _store_master_mode,
> + 0);
> +
> +static
> +ssize_t _show_slave_mode(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
> + u32 smcr;
> +
> + regmap_read(stm32->regmap, TIM_SMCR, &smcr);
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n", smcr & 0x3);
> +}
> +
> +static
> +ssize_t _store_slave_mode(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct stm32_iio_timer_dev *stm32 = iio_priv(indio_dev);
> + u8 mode;
> + int ret;
> +
> + ret = kstrtou8(buf, 10, &mode);
> + if (ret)
> + return ret;
> +
> + if (mode > 0x7)
> + return -EINVAL;
How is something called slave mode going to be fed a number between 0 and 7?
Rule of thumb is no magic numbers in sysfs and right now this is looking
rather cryptic to say the least!
> +
> + regmap_update_bits(stm32->regmap, TIM_SMCR, TIM_SMCR_SMS, mode);
> +
> + return len;
> +}
> +
> +static IIO_DEVICE_ATTR(slave_mode, S_IRUGO | S_IWUSR,
There is an iritating move (in terms of noise it's generating) to use values
directly instead fo these defines. Still if you don't fix it here I'll only
get a patch 'fixing' it soon after...
> + _show_slave_mode,
> + _store_slave_mode,
> + 0);
> +
> +static struct attribute *stm32_timer_attrs[] = {
> + &iio_dev_attr_master_mode.dev_attr.attr,
> + &iio_dev_attr_slave_mode.dev_attr.attr,
New ABI so must be documented under Documentation/ABI/testing/sysfs-bus-iio-*
> + NULL,
> +};
> +
> +static const struct attribute_group stm32_timer_attr_group = {
> + .attrs = stm32_timer_attrs,
> +};
> +
> +static int stm32_timer_start(struct stm32_iio_timer_dev *stm32)
> +{
> + unsigned long long prd, div;
> + int prescaler = 0;
> + u32 max_arr = 0xFFFF, cr1;
> +
> + if (stm32->sampling_frequency == 0)
> + return 0;
> +
> + /* Period and prescaler values depends of clock rate */
> + div = (unsigned long long)clk_get_rate(stm32->clk);
> +
> + do_div(div, stm32->sampling_frequency);
> +
> + prd = div;
> +
> + while (div > max_arr) {
> + prescaler++;
> + div = prd;
> + do_div(div, (prescaler + 1));
> + }
> + prd = div;
> +
> + if (prescaler > MAX_TIM_PSC) {
> + dev_err(stm32->dev, "prescaler exceeds the maximum value\n");
> + return -EINVAL;
> + }
> +
> + /* Check that we own the timer */
> + regmap_read(stm32->regmap, TIM_CR1, &cr1);
> + if ((cr1 & TIM_CR1_CEN) && !stm32->own_timer)
> + return -EBUSY;
> +
> + if (!stm32->own_timer) {
> + stm32->own_timer = true;
> + clk_enable(stm32->clk);
> + }
> +
> + regmap_write(stm32->regmap, TIM_PSC, prescaler);
> + regmap_write(stm32->regmap, TIM_ARR, prd - 1);
> + regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_ARPE, TIM_CR1_ARPE);
> +
> + /* Force master mode to update mode */
> + regmap_update_bits(stm32->regmap, TIM_CR2, TIM_CR2_MMS, 0x20);
> +
> + /* Make sure that registers are updated */
> + regmap_update_bits(stm32->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
> +
> + /* Enable interrupt */
> + regmap_write(stm32->regmap, TIM_SR, 0);
> + regmap_update_bits(stm32->regmap, TIM_DIER, TIM_DIER_UIE, TIM_DIER_UIE);
> +
> + /* Enable controller */
> + regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
> +
> + return 0;
> +}
> +
> +static int stm32_timer_stop(struct stm32_iio_timer_dev *stm32)
> +{
> + if (!stm32->own_timer)
> + return 0;
> +
> + /* Stop timer */
> + regmap_update_bits(stm32->regmap, TIM_DIER, TIM_DIER_UIE, 0);
> + regmap_update_bits(stm32->regmap, TIM_CR1, TIM_CR1_CEN, 0);
> + regmap_write(stm32->regmap, TIM_PSC, 0);
> + regmap_write(stm32->regmap, TIM_ARR, 0);
> +
> + clk_disable(stm32->clk);
> +
> + stm32->own_timer = false;
> + stm32->active_trigger = NULL;
> +
> + return 0;
> +}
> +
> +static int stm32_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> + struct stm32_iio_timer_dev *stm32 = iio_trigger_get_drvdata(trig);
> +
> + stm32->active_trigger = trig;
> +
> + if (state)
> + return stm32_timer_start(stm32);
> + else
> + return stm32_timer_stop(stm32);
> +}
> +
> +static irqreturn_t stm32_timer_irq_handler(int irq, void *private)
> +{
> + struct stm32_iio_timer_dev *stm32 = private;
> + u32 sr;
> +
> + regmap_read(stm32->regmap, TIM_SR, &sr);
> + regmap_write(stm32->regmap, TIM_SR, 0);
> +
> + if ((sr & TIM_SR_UIF) && stm32->active_trigger)
> + iio_trigger_poll(stm32->active_trigger);
This is acting like a trigger cascading off another trigger?
Normally this interrupt handler would be directly associated with the
trigger hardware - in this case the timer.
> +
> + return IRQ_HANDLED;
> +}
> +
> +static const struct iio_trigger_ops timer_trigger_ops = {
> + .owner = THIS_MODULE,
> + .set_trigger_state = stm32_set_trigger_state,
> +};
> +
> +static int stm32_setup_iio_triggers(struct stm32_iio_timer_dev *stm32)
> +{
> + int ret;
> + struct property *p;
> + const char *cur = NULL;
> +
> + p = of_find_property(stm32->dev->of_node,
> + "st,output-triggers-names", NULL);
> +
> + while ((cur = of_prop_next_string(p, cur)) != NULL) {
> + struct iio_trigger *trig;
> +
> + trig = devm_iio_trigger_alloc(stm32->dev, "%s", cur);
> + if (!trig)
> + return -ENOMEM;
> +
> + trig->dev.parent = stm32->dev->parent;
> + trig->ops = &timer_trigger_ops;
> + trig->dev.groups = stm32_trigger_attr_groups;
> + iio_trigger_set_drvdata(trig, stm32);
> +
> + ret = devm_iio_trigger_register(stm32->dev, trig);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * is_stm32_iio_timer_trigger
> + * @trig: trigger to be checked
> + *
> + * return true if the trigger is a valid stm32 iio timer trigger
> + * either return false
> + */
> +bool is_stm32_iio_timer_trigger(struct iio_trigger *trig)
> +{
> + return (trig->ops == &timer_trigger_ops);
> +}
> +EXPORT_SYMBOL(is_stm32_iio_timer_trigger);
> +
> +static int stm32_validate_trigger(struct iio_dev *indio_dev,
> + struct iio_trigger *trig)
> +{
> + struct stm32_iio_timer_dev *dev = iio_priv(indio_dev);
> + int ret;
> +
> + if (!is_stm32_iio_timer_trigger(trig))
> + return -EINVAL;
> +
> + ret = of_property_match_string(dev->dev->of_node,
> + "st,input-triggers-names",
> + trig->name);
> +
> + if (ret < 0)
> + return ret;
> +
> + regmap_update_bits(dev->regmap, TIM_SMCR, TIM_SMCR_TS, ret << 4);
> +
> + return 0;
> +}
> +
> +static const struct iio_info stm32_trigger_info = {
> + .driver_module = THIS_MODULE,
> + .validate_trigger = stm32_validate_trigger,
> + .attrs = &stm32_timer_attr_group,
> +};
> +
> +static struct stm32_iio_timer_dev *stm32_setup_iio_device(struct device *dev)
> +{
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(struct stm32_iio_timer_dev));
> + if (!indio_dev)
> + return NULL;
This is 'unusual'. Why does a trigger driver need an iio_dev at all?
> +
> + indio_dev->name = dev_name(dev);
> + indio_dev->dev.parent = dev;
> + indio_dev->info = &stm32_trigger_info;
> + indio_dev->modes = INDIO_EVENT_TRIGGERED;
> + indio_dev->num_channels = 0;
> + indio_dev->dev.of_node = dev->of_node;
> +
> + ret = iio_triggered_event_setup(indio_dev,
> + NULL,
> + stm32_timer_irq_handler);
So the iio_dev exists to provide the ability to fire this interrupt from
another trigger? Why do you want to do this?
> + if (ret)
> + return NULL;
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret) {
> + iio_triggered_event_cleanup(indio_dev);
> + return NULL;
> + }
> +
> + return iio_priv(indio_dev);
> +}
> +
> +static int stm32_iio_timer_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct stm32_iio_timer_dev *stm32;
> + struct stm32_gptimer_dev *mfd = dev_get_drvdata(pdev->dev.parent);
> + int ret;
> +
> + stm32 = stm32_setup_iio_device(dev);
> + if (!stm32)
> + return -ENOMEM;
> +
> + stm32->dev = dev;
> + stm32->regmap = mfd->regmap;
> + stm32->clk = mfd->clk;
> +
> + stm32->irq = platform_get_irq(pdev, 0);
> + if (stm32->irq < 0)
> + return -EINVAL;
> +
> + ret = devm_request_irq(stm32->dev, stm32->irq,
> + stm32_timer_irq_handler, IRQF_SHARED,
> + "iiotimer_event", stm32);
> + if (ret)
> + return ret;
> +
> + ret = stm32_setup_iio_triggers(stm32);
> + if (ret)
> + return ret;
> +
> + platform_set_drvdata(pdev, stm32);
> +
> + return 0;
> +}
> +
> +static int stm32_iio_timer_remove(struct platform_device *pdev)
> +{
> + struct stm32_iio_timer_dev *stm32 = platform_get_drvdata(pdev);
> +
> + iio_triggered_event_cleanup((struct iio_dev *)stm32);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id stm32_trig_of_match[] = {
> + {
> + .compatible = "st,stm32-iio-timer",
> + },
> +};
> +MODULE_DEVICE_TABLE(of, stm32_trig_of_match);
> +
> +static struct platform_driver stm32_iio_timer_driver = {
> + .probe = stm32_iio_timer_probe,
> + .remove = stm32_iio_timer_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + .of_match_table = stm32_trig_of_match,
> + },
> +};
> +module_platform_driver(stm32_iio_timer_driver);
> +
> +MODULE_ALIAS("platform:" DRIVER_NAME);
> +MODULE_DESCRIPTION("STMicroelectronics STM32 iio timer driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/iio/trigger/Kconfig b/drivers/iio/trigger/Kconfig
> index 809b2e7..f2af4fe 100644
> --- a/drivers/iio/trigger/Kconfig
> +++ b/drivers/iio/trigger/Kconfig
> @@ -46,5 +46,4 @@ config IIO_SYSFS_TRIGGER
>
> To compile this driver as a module, choose M here: the
> module will be called iio-trig-sysfs.
> -
Clear this out...
> endmenu
> diff --git a/include/dt-bindings/iio/timer/st,stm32-iio-timer.h b/include/dt-bindings/iio/timer/st,stm32-iio-timer.h
> new file mode 100644
> index 0000000..d39bf16
> --- /dev/null
> +++ b/include/dt-bindings/iio/timer/st,stm32-iio-timer.h
> @@ -0,0 +1,23 @@
> +/*
> + * st,stm32-iio-timer.h
> + *
> + * Copyright (C) STMicroelectronics 2016
> + * Author: Benjamin Gaignard <benjamin.gaignard-qxv4g6HH51o@public.gmane.org> for STMicroelectronics.
> + * License terms: GNU General Public License (GPL), version 2
> + */
> +
> +#ifndef _DT_BINDINGS_IIO_TIMER_H_
> +#define _DT_BINDINGS_IIO_TIMER_H_
> +
> +#define TIM1_TRGO "tim1_trgo"
> +#define TIM2_TRGO "tim2_trgo"
> +#define TIM3_TRGO "tim3_trgo"
> +#define TIM4_TRGO "tim4_trgo"
> +#define TIM5_TRGO "tim5_trgo"
> +#define TIM6_TRGO "tim6_trgo"
> +#define TIM7_TRGO "tim7_trgo"
> +#define TIM8_TRGO "tim8_trgo"
> +#define TIM9_TRGO "tim9_trgo"
> +#define TIM12_TRGO "tim12_trgo"
> +
> +#endif
> diff --git a/include/linux/iio/timer/stm32-iio-timers.h b/include/linux/iio/timer/stm32-iio-timers.h
> new file mode 100644
> index 0000000..5d1b86c
> --- /dev/null
> +++ b/include/linux/iio/timer/stm32-iio-timers.h
> @@ -0,0 +1,16 @@
> +/*
> + * stm32-iio-timers.h
> + *
> + * Copyright (C) STMicroelectronics 2016
> + * Author: Benjamin Gaignard <benjamin.gaignard-qxv4g6HH51o@public.gmane.org> for STMicroelectronics.
> + * License terms: GNU General Public License (GPL), version 2
> + */
> +
> +#ifndef _STM32_IIO_TIMERS_H_
> +#define _STM32_IIO_TIMERS_H_
> +
> +#include <dt-bindings/iio/timer/st,stm32-iio-timer.h>
> +
> +bool is_stm32_iio_timer_trigger(struct iio_trigger *trig);
> +
> +#endif
>
^ permalink raw reply
* Re: [PATCH v2 1/7] MFD: add bindings for stm32 general purpose timer driver
From: Jonathan Cameron @ 2016-11-27 15:41 UTC (permalink / raw)
To: Benjamin Gaignard, lee.jones, robh+dt, mark.rutland,
alexandre.torgue, devicetree, linux-kernel, thierry.reding,
linux-pwm, knaack.h, lars, pmeerw, linux-iio, linux-arm-kernel
Cc: fabrice.gasnier, gerald.baeza, arnaud.pouliquen, linus.walleij,
linaro-kernel, Benjamin Gaignard
In-Reply-To: <2b751c4a-3038-4220-05d8-d745c51a3691@kernel.org>
On 27/11/16 14:10, Jonathan Cameron wrote:
> On 24/11/16 15:14, Benjamin Gaignard wrote:
>> Add bindings information for stm32 general purpose timer
>>
>> version 2:
>> - rename stm32-mfd-timer to stm32-gptimer
>> - only keep one compatible string
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>> .../bindings/mfd/stm32-general-purpose-timer.txt | 43 ++++++++++++++++++++++
>> 1 file changed, 43 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt b/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
>> new file mode 100644
>> index 0000000..2f10e67
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
>> @@ -0,0 +1,43 @@
>> +STM32 general purpose timer driver
>> +
>> +Required parameters:
>> +- compatible: must be "st,stm32-gptimer"
>> +
>> +- reg: Physical base address and length of the controller's
>> + registers.
>> +- clock-names: Set to "clk_int".
>> +- clocks: Phandle to the clock used by the timer module.
>> + For Clk properties, please refer to ../clock/clock-bindings.txt
>> +
>> +Optional parameters:
>> +- resets: Phandle to the parent reset controller.
>> + See ..reset/st,stm32-rcc.txt
>> +
>> +Optional subnodes:
>> +- pwm: See ../pwm/pwm-stm32.txt
>> +- iiotimer: See ../iio/timer/stm32-iio-timer.txt
> Naming issue here. Can't mention IIO as that's a linux subsystem and all
> bindings must be independent of OS.
>
> Perhaps adc-trigger-timer?
>> +
>> +Example:
>> + gptimer1: gptimer1@40010000 {
>> + compatible = "st,stm32-gptimer";
>> + reg = <0x40010000 0x400>;
>> + clocks = <&rcc 0 160>;
>> + clock-names = "clk_int";
>> +
>> + pwm1@0 {
>> + compatible = "st,stm32-pwm";
>> + st,pwm-num-chan = <4>;
>> + st,breakinput;
>> + st,complementary;
>> + };
>> +
>> + iiotimer1@0 {
>> + compatible = "st,stm32-iio-timer";
> Again, avoid the use of iio in here (same issue you had with mfd in the previous
> version).
>> + interrupts = <27>;
>> + st,input-triggers-names = TIM5_TRGO,
> Docs for these should be introduced before they are used in an example.
> Same for the PWM ones above. Expand the detail fo the example as you add
> the other elements.
I've just dived into the datasheet for these timers.
http://www.st.com/content/ccc/resource/technical/document/reference_manual/3d/6d/5a/66/b4/99/40/d4/DM00031020.pdf/files/DM00031020.pdf/jcr:content/translations/en.DM00031020.pdf
I think you need a binding that describes the capabilities of each of the timers
explicitly. Down to the level of whether there is a repetition counter or not.
Each should exists as a separate entity in device tree.
They then have an existence as timers separate to the description of what they
are used for.
Here the only way we are saying they exist is by their use which doesn't feel
right to me.
So I think you need to move back to what you had in the first place. The key
thing is that ever timer needs describing fully. They are different enough
that for example the datasheet doesn't even try to describe them in one section.
(it has 4 separate chapters covering different sets of these hardware blocks).
The naming isn't really based on index, we are talking different hardware
that the datasheet authors have decided not to give different names to!
If they'd called them
advanced timers
generic timers
basic timers
really basic timers meant for driving the DAC (6 and 7)
We'd all have been quite happy with different compatible strings giving away
what they can do.
What you have here is far too specific to what you are trying to do with them
right now.
These things are separately capable of timing capture (which is I guess where
the IIO device later comes in).
So my expectation is that we end up potentially instantiating:
1) An MFD to handle the shared elements of the timers.
2) Up to 12ish timers each with separate existence as a device in the driver model
and in device tree.
(nasty corner cases here are using timers as perscalers for other timers - I'd be
tempted to leave that for now)
Note that each of these devices has a different register set I think? Any shared
bits are handled via the mfd above (if we even need that MFD which I'm starting
to doubt looking at the datasheet).
3) Up to N pwms again with there own existence in the device model. These don't
do much other than wrap the timer and stick it in output mode.
4) Up to N iio triggers - this is basically using the timer as a periodic interrupt
(though without the interrupt having visibility to the kernel) which fires off
sampling on associated ADCs.
5) Up to N iio capture devices for all channels that support timing capture.
Note there is also hardware encoder capture support in these which should be
correctly handled as well. This comes back to an ancient discussion on the
TI ecap units which have similar capabilities (driver never went anywhere but
I think that was because the author left TI).
Certainly for the IIO devices these should no be bound up into one instance
as you have done here.
Anyhow, I fear that right now this discussion is missing the key ingredient
that the hardware is horrendously variable in it's capabilities and really
is 4-5 different types of hardware that just happen to share a few bits of
their offsets in their register maps.
So after all that I'm almost more confused than I was at the start!
Jonathan
>> + TIM2_TRGO,
>> + TIM4_TRGO,
>> + TIM3_TRGO;
>> + st,output-triggers-names = TIM1_TRGO;
>> + };
>> + };
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH 00/39] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb
From: Boris Brezillon @ 2016-11-27 15:04 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Marek Vasut, Brian Norris,
Richard Weinberger, David Woodhouse, Cyrille Pitchen, Rob Herring,
Mark Rutland, Andy Shevchenko
In-Reply-To: <1480183585-592-1-git-send-email-yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
+Andy
Hi Masahiro,
On Sun, 27 Nov 2016 03:05:46 +0900
Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote:
> As I said in the 1st round series, I am tackling on this driver
> to use it for my SoCs.
>
> The previous series was just cosmetic things, but this series
> includes *real* changes.
>
> After some more cleanups, I will start to add changes that
> are really necessary.
> One of the biggest problems I want to solve is a bunch of
> hard-coded parameters that prevent me from using this driver for
> my SoCs.
>
> I will introduce capability flags that are associated with DT
> compatible and make platform-dependent parameters overridable.
>
> I still have lots of reworks to get done (so probably 3rd round
> series will come), but I hope it is getting better and
> I am showing a big picture now.
>
Thanks for posting this 2nd round of patches, I know have a clearer
view of what you're trying to achieve.
Could you be a bit more specific about the remaining rework (your 3rd
round)?
Also, if you don't mind, I'd like to have reviews and testing from intel
users before applying the series. Can you Cc Andy (and possibly other
intel maintainers) for the next round.
Thanks,
Boris
>
>
> Masahiro Yamada (39):
> mtd: nand: allow to set only one of ECC size and ECC strength from DT
> mtd: nand: denali: remove unused CONFIG option and macros
> mtd: nand: denali: remove redundant define of BANK(x)
> mtd: nand: denali: remove more unused struct members
> mtd: nand: denali: fix comment of denali_nand_info::flash_mem
> mtd: nand: denali: fix write_oob_data() function
> mtd: nand: denali: transfer OOB only when oob_required is set
> mtd: nand: denali: introduce capability flag
> mtd: nand: denali: fix erased page check code
> mtd: nand: denali: remove redundant if conditional of erased_check
> mtd: nand: denali: increment ecc_stats.failed by one per error
> mtd: nand: denali: return 0 for uncorrectable ECC error
> mtd: nand: denali: increment ecc_stats->corrected
> mtd: nand: denali: replace uint{8/16/32}_t with u{8/16/32}
> mtd: nand: denali: improve readability of handle_ecc()
> mtd: nand: denali: rename handle_ecc() to denali_sw_ecc_fixup()
> mtd: nand: denali: support HW_ECC_FIXUP capability
> mtd: nand: denali: move denali_read_page_raw() above
> denali_read_page()
> mtd: nand: denali: perform erased check against raw transferred page
> mtd: nand: denali_dt: enable HW_ECC_FIXUP capability for DT platform
> mtd: nand: denali: support 64bit capable DMA engine
> mtd: nand: denali_dt: remove dma-mask DT property
> mtd: nand: denali_dt: use pdev instead of ofdev for platform_device
> mtd: nand: denali: add NEW_N_BANKS_FORMAT capability
> mtd: nand: denali: use nand_chip to hold frequently accessed data
> mtd: nand: denali: call nand_set_flash_node() to set DT node
> mtd: nand: denali: do not set mtd->name
> mtd: nand: denali: move multi NAND fixup code to a helper function
> mtd: nand: denali: refactor multi NAND fixup code in more generic way
> mtd: nand: denali: set DEVICES_CONNECTED 1 if not set
> mtd: nand: denali: remove meaningless writes to read-only registers
> mtd: nand: denali: remove unnecessary writes to ECC_CORRECTION
> mtd: nand: denali: support 1024 byte ECC step size
> mtd: nand: denali: fix the condition for 15 bit ECC strength
> mtd: nand: denali: calculate ecc.strength and ecc.bytes generically
> mtd: nand: denali: allow to use SoC-specific ECC strength
> mtd: nand: denali: support "nand-ecc-strength" DT property
> mtd: nand: denali: remove Toshiba, Hynix specific fixup code
> mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants
>
> .../devicetree/bindings/mtd/denali-nand.txt | 19 +-
> drivers/mtd/nand/Kconfig | 11 -
> drivers/mtd/nand/denali.c | 740 ++++++++++++---------
> drivers/mtd/nand/denali.h | 84 +--
> drivers/mtd/nand/denali_dt.c | 95 ++-
> drivers/mtd/nand/denali_pci.c | 2 +
> drivers/mtd/nand/nand_base.c | 6 -
> 7 files changed, 515 insertions(+), 442 deletions(-)
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 5/7] IIO: add bindings for stm32 IIO timer driver
From: Jonathan Cameron @ 2016-11-27 14:25 UTC (permalink / raw)
To: Benjamin Gaignard, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
alexandre.torgue-qxv4g6HH51o, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
linux-pwm-u79uwXL29TY76Z2rM5mHXA, knaack.h-Mmb7MZpHnFY,
lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: fabrice.gasnier-qxv4g6HH51o, gerald.baeza-qxv4g6HH51o,
arnaud.pouliquen-qxv4g6HH51o,
linus.walleij-QSEj5FYQhm4dnm+yROfE0A,
linaro-kernel-cunTk1MwBs8s++Sfvej+rw, Benjamin Gaignard
In-Reply-To: <1480000463-9625-6-git-send-email-benjamin.gaignard-qxv4g6HH51o@public.gmane.org>
On 24/11/16 15:14, Benjamin Gaignard wrote:
> Define bindings for stm32 IIO timer
>
> version 2:
> - only keep one compatible
> - add DT parameters to set lists of the triggers:
> one list describe the triggers created by the device
> another one give the triggers accepted by the device
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard-qxv4g6HH51o@public.gmane.org>
> ---
> .../bindings/iio/timer/stm32-iio-timer.txt | 41 ++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt b/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
> new file mode 100644
> index 0000000..840b417
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/timer/stm32-iio-timer.txt
> @@ -0,0 +1,41 @@
> +timer IIO trigger bindings for STM32
> +
> +Must be a sub-node of STM32 general purpose timer driver
Add a cross reference...
> +
> +Required parameters:
> +- compatible: must be "st,stm32-iio-timer"
st,stm32-adc-timer or something like that.
> +- interrupts: Interrupt for this device
> + See ../interrupt-controller/st,stm32-exti.txt
> +
> +Optional parameters:
> +- st,input-triggers-names: List of the possible input triggers for
> + the device
> +- st,output-triggers-names: List of the possible output triggers for
> + the device
What are input / output triggers?
> +
> +Possible triggers are defined in include/dt-bindings/iio/timer/st,stm32-iio-timer.h
> +
> +Example:
> + gptimer1: gptimer1@40010000 {
> + compatible = "st,stm32-gptimer";
> + reg = <0x40010000 0x400>;
> + clocks = <&rcc 0 160>;
> + clock-names = "clk_int";
> +
> + pwm1@0 {
> + compatible = "st,stm32-pwm";
> + st,pwm-num-chan = <4>;
> + st,breakinput;
> + st,complementary;
> + };
> +
> + iiotimer1@0 {
> + compatible = "st,stm32-iio-timer";
> + interrupts = <27>;
> + st,input-triggers-names = TIM5_TRGO,
> + TIM2_TRGO,
> + TIM4_TRGO,
> + TIM3_TRGO;
> + st,output-triggers-names = TIM1_TRGO;
> + };
> + };
>
^ permalink raw reply
* Re: [PATCH v2 3/7] PWM: add pwm-stm32 DT bindings
From: Jonathan Cameron @ 2016-11-27 14:19 UTC (permalink / raw)
To: Benjamin Gaignard, lee.jones, robh+dt, mark.rutland,
alexandre.torgue, devicetree, linux-kernel, thierry.reding,
linux-pwm, knaack.h, lars, pmeerw, linux-iio, linux-arm-kernel
Cc: fabrice.gasnier, gerald.baeza, arnaud.pouliquen, linus.walleij,
linaro-kernel, Benjamin Gaignard
In-Reply-To: <1480000463-9625-4-git-send-email-benjamin.gaignard@st.com>
On 24/11/16 15:14, Benjamin Gaignard wrote:
> Define bindings for pwm-stm32
>
> version 2:
> - use parameters instead of compatible of handle the hardware configuration
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
> .../devicetree/bindings/pwm/pwm-stm32.txt | 37 ++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-stm32.txt
>
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-stm32.txt b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> new file mode 100644
> index 0000000..36263f0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-stm32.txt
> @@ -0,0 +1,37 @@
> +STMicroelectronics PWM driver bindings for STM32
> +
> +Must be a sub-node of STM32 general purpose timer driver
> +
> +Required parameters:
> +- compatible: Must be "st,stm32-pwm"
> +- pinctrl-names: Set to "default".
> +- pinctrl-0: List of phandles pointing to pin configuration nodes
> + for PWM module.
> + For Pinctrl properties, please refer to [1].
> +
> +Optional parameters:
> +- st,breakinput: Set if the hardware have break input capabilities
> +- st,breakinput-polarity: Set break input polarity. Default is 0
> + The value define the active polarity:
> + - 0 (active LOW)
> + - 1 (active HIGH)
> +- st,pwm-num-chan: Number of available PWM channels. Default is 0.
> +- st,32bits-counter: Set if the hardware have a 32 bits counter
> +- st,complementary: Set if the hardware have complementary output channels
> +
> +[1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +
> +Example:
> + gptimer1: gptimer1@40010000 {
Given the example includes chunks for the other binding, make sure to have
explicit cross references in the document.
> + compatible = "st,stm32-gptimer";
> + reg = <0x40010000 0x400>;
> + clocks = <&rcc 0 160>;
> + clock-names = "clk_int";
> +
> + pwm1@0 {
> + compatible = "st,stm32-pwm";
> + st,pwm-num-chan = <4>;
> + st,breakinput;
> + st,complementary;
> + };
> + };
>
^ permalink raw reply
* Re: [PATCH v2 1/7] MFD: add bindings for stm32 general purpose timer driver
From: Jonathan Cameron @ 2016-11-27 14:10 UTC (permalink / raw)
To: Benjamin Gaignard, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
alexandre.torgue-qxv4g6HH51o, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
linux-pwm-u79uwXL29TY76Z2rM5mHXA, knaack.h-Mmb7MZpHnFY,
lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: fabrice.gasnier-qxv4g6HH51o, gerald.baeza-qxv4g6HH51o,
arnaud.pouliquen-qxv4g6HH51o,
linus.walleij-QSEj5FYQhm4dnm+yROfE0A,
linaro-kernel-cunTk1MwBs8s++Sfvej+rw, Benjamin Gaignard
In-Reply-To: <1480000463-9625-2-git-send-email-benjamin.gaignard-qxv4g6HH51o@public.gmane.org>
On 24/11/16 15:14, Benjamin Gaignard wrote:
> Add bindings information for stm32 general purpose timer
>
> version 2:
> - rename stm32-mfd-timer to stm32-gptimer
> - only keep one compatible string
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard-qxv4g6HH51o@public.gmane.org>
> ---
> .../bindings/mfd/stm32-general-purpose-timer.txt | 43 ++++++++++++++++++++++
> 1 file changed, 43 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
>
> diff --git a/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt b/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
> new file mode 100644
> index 0000000..2f10e67
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/stm32-general-purpose-timer.txt
> @@ -0,0 +1,43 @@
> +STM32 general purpose timer driver
> +
> +Required parameters:
> +- compatible: must be "st,stm32-gptimer"
> +
> +- reg: Physical base address and length of the controller's
> + registers.
> +- clock-names: Set to "clk_int".
> +- clocks: Phandle to the clock used by the timer module.
> + For Clk properties, please refer to ../clock/clock-bindings.txt
> +
> +Optional parameters:
> +- resets: Phandle to the parent reset controller.
> + See ..reset/st,stm32-rcc.txt
> +
> +Optional subnodes:
> +- pwm: See ../pwm/pwm-stm32.txt
> +- iiotimer: See ../iio/timer/stm32-iio-timer.txt
Naming issue here. Can't mention IIO as that's a linux subsystem and all
bindings must be independent of OS.
Perhaps adc-trigger-timer?
> +
> +Example:
> + gptimer1: gptimer1@40010000 {
> + compatible = "st,stm32-gptimer";
> + reg = <0x40010000 0x400>;
> + clocks = <&rcc 0 160>;
> + clock-names = "clk_int";
> +
> + pwm1@0 {
> + compatible = "st,stm32-pwm";
> + st,pwm-num-chan = <4>;
> + st,breakinput;
> + st,complementary;
> + };
> +
> + iiotimer1@0 {
> + compatible = "st,stm32-iio-timer";
Again, avoid the use of iio in here (same issue you had with mfd in the previous
version).
> + interrupts = <27>;
> + st,input-triggers-names = TIM5_TRGO,
Docs for these should be introduced before they are used in an example.
Same for the PWM ones above. Expand the detail fo the example as you add
the other elements.
> + TIM2_TRGO,
> + TIM4_TRGO,
> + TIM3_TRGO;
> + st,output-triggers-names = TIM1_TRGO;
> + };
> + };
>
^ permalink raw reply
* Re: [PATCH 2/2] input: touchscreen: sample averaging for imx6ul_tsc
From: Fabio Estevam @ 2016-11-27 12:39 UTC (permalink / raw)
To: Guy Shapiro
Cc: Fabio Estevam, Mark Rutland, devicetree@vger.kernel.org,
Haibo Chen, Dmitry Torokhov, robh+dt@kernel.org, linux-input,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <1480232698-23075-2-git-send-email-guy.shapiro@mobi-wize.com>
On Sun, Nov 27, 2016 at 5:44 AM, Guy Shapiro <guy.shapiro@mobi-wize.com> wrote:
> The i.MX6UL internal touchscreen controller contains an option to
> average upon samples. This feature reduces noise from the produced
> touch locations.
>
> This patch adds sample averaging support to the imx6ul_tsc device
> driver.
>
> Signed-off-by: Guy Shapiro <guy.shapiro@mobi-wize.com>
Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
^ permalink raw reply
* Re: [PATCH 1/2] Documentation: sample averaging for imx6ul_tsc
From: Fabio Estevam @ 2016-11-27 12:38 UTC (permalink / raw)
To: Guy Shapiro
Cc: Dmitry Torokhov, Fabio Estevam, Mark Rutland,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Haibo Chen,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
In-Reply-To: <1480232698-23075-1-git-send-email-guy.shapiro-2HKgp+mgmS5l57MIdRCFDg@public.gmane.org>
On Sun, Nov 27, 2016 at 5:44 AM, Guy Shapiro <guy.shapiro-2HKgp+mgmS5l57MIdRCFDg@public.gmane.org> wrote:
> The i.MX6UL internal touchscreen controller contains an option to
> average upon samples. This feature reduces noise from the produced
> touch locations.
>
> This patch introduces a new device tree optional property for this
> feature. It provides control over the amount of averaged samples per
> touch event.
>
> The property was inspired by a similar property on the
> "brcm,iproc-touchscreen" binding.
>
> Signed-off-by: Guy Shapiro <guy.shapiro-2HKgp+mgmS5l57MIdRCFDg@public.gmane.org>
Reviewed-by: Fabio Estevam <fabio.estevam-3arQi8VN3Tc@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 0/7] mux controller abstraction and iio/i2c muxes
From: Jonathan Cameron @ 2016-11-27 12:00 UTC (permalink / raw)
To: Peter Rosin, Lars-Peter Clausen, linux-kernel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Peter Meerwald-Stadler, Jonathan Corbet, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, devicetree, linux-iio, linux-doc
In-Reply-To: <3870654b-8cf5-30cb-74cd-c5fb1559adba@axentia.se>
On 23/11/16 11:47, Peter Rosin wrote:
> On 2016-11-22 21:58, Lars-Peter Clausen wrote:
>> On 11/21/2016 02:17 PM, Peter Rosin wrote:
>> [...]
>>> I have a piece of hardware that is using the same 3 GPIO pins
>>> to control four 8-way muxes. Three of them control ADC lines
>>> to an ADS1015 chip with an iio driver, and the last one
>>> controls the SDA line of an i2c bus. We have some deployed
>>> code to handle this, but you do not want to see it or ever
>>> hear about it. I'm not sure why I even mention it. Anyway,
>>> the situation has nagged me to no end for quite some time.
>>>
>>> So, after first getting more intimate with the i2c muxing code
>>> and later discovering the drivers/iio/inkern.c file and
>>> writing a couple of drivers making use of it, I came up with
>>> what I think is an acceptable solution; add a generic mux
>>> controller driver (and subsystem) that is shared between all
>>> instances, and combine that with an iio mux driver and a new
>>> generic i2c mux driver. The new i2c mux I called "simple"
>>> since it is only hooking the i2c muxing and the new mux
>>> controller (much like the alsa simple card driver does for ASoC).
>>
>> While abstracting this properly is all nice and good and the way it should
>> be done, but it also adds a lot of complexity and the devicetree adds a lot
>> of restrictions on what can actually be represented.
>
> This is a characterization without any specifics. But is the
> characterization true? You have two complaints, complexity
> and restrictions with bindings.
>
>> There is a certain point where the fabric on a PCB becomes so complex that
>> it deserves to be a device on its own (like the audio fabric drivers).
>> Especially when the hardware is built with a certain application in mind and
>> the driver is supposed to impose policy which reflects this application. The
>> latter can often not properly be described with the primitives the
>> devicetree can offer.
>>
>> And I think your setup is very borderline what can be done in a declarative
>> way only and it adds a lot of complexity over a more imperative solution in
>> form of a driver. I think it is worth investigating about having a driver
>> that is specific to your fabric and handles the interdependencies of the
>> discrete components.
>
> So, there are three "new" concepts:
>
> 1. Sticking a mux in front of an AD-converter. That's not all that
> novel, nor complex. Quite the opposite, I'd say. In fact, I find it
> a bit amazing that there is no in-kernel support for it.
As ever first person who needs it and has the skills to write it gets to do it ;)
Congratulations Peter ;)
>
> 2. Reusing the same GPIO-pins to drive different muxes. There are
> obviously chips that work this way (as Jonathan pointed out) and
> these will at some point get used in Linux devices. I guess they
> already are used, but that people handle them in userspace. Or
> something? If this is complex, which I question, it will still need
> support at some point. At least that's what I believe.
>
> 3. Using the same GPIO pins to mux things handled by different
> subsystems. Right, this is a bit crazy, and I'd rather not have this
> requirement, but this HW is what it is so I'll need to handle it in
> some way. It is also what stops me from falling back to a userspace
> solution, which is probably connected to why #1 and #2 is not supported
> by the kernel; everybody probably does muxing in userspace. Which is
> not necessarily a good idea, nor how it's supposed to be done...
>
> So, the only thing that's out of the ordinary (as I see it), is #3.
> The question that then needs an answer is how the in-kernel solution
> for #1 and #2 would look if we do not consider #3.
>
> And I claim that the desired solution to #1 and #2 is pretty close
> to my proposal.
>
> A. You do not want mux-controller drivers in every subsystem that
> needs them.
Agreed.
>
> B. You do not want every mux-consumer to know the specifics of how to
> operate every kind of mux; there are muxes that are not controlled
> with GPIO pins...
>
> C. When you implement muxing in a subsystem, there will in some cases
> be a need to handle parallel muxing, where there is a need to share
> mux-controllers.
>
> It just feels right to separate out the mux-controller and refer to
> it from where a mux is needed. It solves #1 and #2. And, of course,
> as a bonus #3 is also solved. But my bias is obvious.
>
> And that leads us to the restrictions with the bindings. And the same
> thing happens; the solution for #2 also solves #3.
>
> So how do you refer to a mux-controller from where it's needed? My
> first proposal used a dt phandle, for the second round I put them in
> the parent node. It would be super if it was possible for the mux-
> consumer to create the mux-controller device from the same dt
> node that is already bound to the mux-consumer. The problem is that
> the mux-consumer should not hard-code which mux-controller device it
> should create. The best I can think of is some kind of 'mux-compatible'
> attribute, that works like the standard 'compatible' attribute. That
> would simplify the bindings for the normal case (#1) where the mux-
> controller isn't shared (#2 and #3). Maybe it's possible to fix this
> issue somehow? I simply don't know?
As Lars stated, it's marginal. The question is not at what point do we
'have to' bother with a fabric driver, but rather at what point does it
make a our lives easier.
Take you nastiest mux case described earlier.
The ideal would be to represent the ADC and 3 muxes as (approximately) a
single ADC to userspace that just happens to have somewhere near 23 inputs.
To do that in device tree we need to describe
1 The adc
2 The three muxes
3 The software representation to pull all of these back into a single device.
That last part to my mind trips the balance to the point where a fabric driver
would make sense. It's not complex. Just a few lines of code tying all the
elements together without ending up with a fairly fiendish setup to describe in
device tree.
Also just wait until we have muxes stacked on muxes, with cross overs occuring.
Some of the ASoC parts can actually have effective loops if you try all the mux
combinations.
So question is do we have a 'simple case description' in device tree or force
fabric drivers everywhere? I think I'm in favour of the simple case - which handles
one of your two uses nicely. The second one to do the the recombining of channels after
the muxes, ends up looking to me like it needs a fabric driver.
Note we are only talking about bindings vs code based description here. I agree
entirely with the concept of a generic mux subsystem.
Jonathan
>
> Cheers,
> Peter
>
^ permalink raw reply
* Re: [RFC PATCH v2 5/7] iio: multiplexer: new iio category and iio-mux driver
From: Jonathan Cameron @ 2016-11-27 11:42 UTC (permalink / raw)
To: Peter Rosin, linux-kernel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, devicetree, linux-iio
In-Reply-To: <365eb334-d27a-e91b-7ca7-9e10c7ede9a6@axentia.se>
On 19/11/16 22:08, Peter Rosin wrote:
> On 2016-11-19 16:49, Jonathan Cameron wrote:
>> On 17/11/16 21:48, Peter Rosin wrote:
>>> When a multiplexer changes how an iio device behaves (for example
>>> by feeding different signals to an ADC), this driver can be used
>>> create one virtual iio channel for each multiplexer state.
>>>
>>> Depends on the generic multiplexer driver.
>> I'm not really following what all the ext info stuff in here is about.
>> Could you add a little more description of that?
>
> Certainly. I have two needs for this series. The first one is simple
> when it comes to the iio part and complex because the mux is shared
> between three 8-way muxes on three of the inputs to an ADS-1015 ADC.
> The forth ADC line to the ADS-1015 is not muxed. Those three muxes
> are of course GPIO-controlled and share GPIO pins. And the GPIO
> pins also control an i2c bus that is muxed 8-ways as well. There are
> eight (possible) batteries, and we monitor voltage/current/temp with
> the 3 muxed ADC lines, and 8 chargers sit on i2c behind the i2c mux.
> I guess it felt natural for the HW designer to select battery with
> the GPIO lines, but that do not fit too well with the code as it
> is without this series...
>
> For this first need, the iio mux does not need ext_info.
>
> The second need is simple in the mux part but worse in the iio
> department. It's another 8-way mux that simply muxes an ADC line,
> so that is simple. However, the ADC line is the envelope detector
> that just got added to linux-next, and it has two ext_info
> attributes that needs to be set differently for different mux
> states. Two of the states need "invert" to be false, the rest need
> "invert" to be true. And it is also preferable to have different
> values for "compare_interval" for different mux states since the
> signals on the diffrent mux states have the different frequency
> characteristics.
>
> True, I could have the ext-info attributes go straight through
> the mux, and just start by writing values to "invert"
> and "compare_interval", and only then read a sample. But then I
> would need to lock out other users during the duration of this
> transaction. I believe that the best place to put that lock is
> in the iio mux (when it locks its control-mux) and not leave it
> to userspace to solve this in some brittle cooperative manner.
>
>> Perhaps an example of how it is used and what the resulting interface
>> looks like?
>
> The resulting interface is just a copy of the (ext_info) interface
> exposed by the parent channel (with a cache that is rewritten to
> the parent on every iio mux state change). I have plans to add code
> to not rewrite ext_info attributes that have never been changed in
> any mux state.
>
> Below I have an example file listing.
>
> device0 is a dpot (mcp4561).
> device1 is a dac (dpot-dac, wrapping the above dpot).
> device2 is an adc (envelope-detector, wrapping the above dac)
> device3 is a mux (iio-mux, wrapping the above adc)
>
> The 8-way iio-mux have no signals attached on mux states 0 and 1, which
> is why the first channel for device 3 is in_altvoltage2.
>
> Ultimately, I would like some knob to hide devices 0, 1 and 2 from
> userspace. They need/should only be visible to in-kernel users. Or
> is there such a knob already?
>
There isn't and this feeds into what Lars was suggesting with a fabric
driver. The complexity of the description in device tree is getting really
very nasty indeed, perhaps we are better off allowing for complex cases
to have a driver that directly hooks into all the relevant elements and
can do magic channel remapping etc.
That could then register the 3 muxes and acquire all the chanenls required
to build a single many channel device that hides all the complexity from userspace.
I've considered working out how to do an IIO multiplexer before in software which would
allow us to make 'fake' devices that wrap multiple physical devices. In the general
case it has always felt to complex as we'd want it to handle triggered and buffered
data flows. That brings all sorts of nasty data alignment problems with it as there
is no guarantee the devices are producing aligned data at all.
In the specific case though a driver can bundle up everything it needs to create
a pseudo device (for triggered flows we'd probably need a little magic to hold off the
trigger but that's not hard). With sysfs only access it would be simple to do but
hard to describe. Hence bringing us back to fabric drivers.
Thanks for the description. Good to understand what you are trying to handle.
Jonathan
> Cheers,
> Peter
>
> $ ls /sys/bus/iio/devices/iio\:device*
> /sys/bus/iio/devices/iio:device0:
> dev out_resistance_raw_available
> name out_resistance_scale
> of_node power
> out_resistance0_raw subsystem
> out_resistance1_raw uevent
>
> /sys/bus/iio/devices/iio:device1:
> dev out_voltage0_scale
> name power
> of_node subsystem
> out_voltage0_raw uevent
> out_voltage0_raw_available
>
> /sys/bus/iio/devices/iio:device2:
> dev name
> in_altvoltage0_compare_interval of_node
> in_altvoltage0_invert power
> in_altvoltage0_raw subsystem
> in_altvoltage0_scale uevent
>
> /sys/bus/iio/devices/iio:device3:
> dev in_altvoltage5_raw
> in_altvoltage2_compare_interval in_altvoltage5_scale
> in_altvoltage2_invert in_altvoltage6_compare_interval
> in_altvoltage2_raw in_altvoltage6_invert
> in_altvoltage2_scale in_altvoltage6_raw
> in_altvoltage3_compare_interval in_altvoltage6_scale
> in_altvoltage3_invert in_altvoltage7_compare_interval
> in_altvoltage3_raw in_altvoltage7_invert
> in_altvoltage3_scale in_altvoltage7_raw
> in_altvoltage4_compare_interval in_altvoltage7_scale
> in_altvoltage4_invert name
> in_altvoltage4_raw of_node
> in_altvoltage4_scale power
> in_altvoltage5_compare_interval subsystem
> in_altvoltage5_invert uevent
>
^ permalink raw reply
* Re: [PATCH v8 3/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature
From: Jonathan Cameron @ 2016-11-27 11:02 UTC (permalink / raw)
To: H. Nikolaus Schaller, Jonathan Cameron
Cc: Sebastian Reichel, Dmitry Torokhov, Mark Rutland,
Benoît Cousson, Tony Lindgren, Russell King, Arnd Bergmann,
Michael Welling, Mika Penttilä, Javier Martinez Canillas,
Igor Grinberg, Andrew F. Davis, Mark Brown, Rob Herring,
Alexander Stein, Eric Engestrom, Hans de Goede,
Benjamin Tissoires
In-Reply-To: <811B6F6A-2E3D-45B4-A984-74ABE0E37192-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
On 24/11/16 18:05, H. Nikolaus Schaller wrote:
>
>> Am 24.11.2016 um 18:38 schrieb Jonathan Cameron <jic23-tko9wxEg+fIOOJlXag/Snyp2UmYkHbXO@public.gmane.org>:
>>
>>
>>
>> On 22 November 2016 14:02:30 GMT+00:00, "H. Nikolaus Schaller" <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org> wrote:
>>> The tsc2007 chip not only has a resistive touch screen controller but
>>> also an external AUX adc imput which can be used for an ambient
>>> light sensor, battery voltage monitoring or any general purpose.
>>>
>>> Additionally it can measure the chip temperature.
>>>
>>> This extension provides an iio interface for these adc channels.
>>>
>>> Since it is not wasting much resources and is very straightforward,
>>> we simply provide all other adc channels as optional iio interfaces
>>> as weel. This can be used for debugging or special applications.
>>>
>>> This patch also splits the tsc2007 driver in several source files:
>>> tsc2007.h -- constants, structs and stubs
>>> tsc2007_core.c -- functional parts of the original driver
>>> tsc2007_iio.c -- the optional iio stuff
>>>
>>> Makefile magic allows to conditionally link the iio stuff
>>> if CONFIG_IIO=y or =m in a way that it works with
>>> CONFIG_TOUCHSCREEN_TSC2007=m.
>>>
>>> Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
>>> Reviewed-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> ---
>>> drivers/input/touchscreen/Makefile | 7 +
>>> drivers/input/touchscreen/tsc2007.h | 116
>>> ++++++++++++++++
>>> .../touchscreen/{tsc2007.c => tsc2007_core.c} | 95 +++----------
>>> drivers/input/touchscreen/tsc2007_iio.c | 150
>>> +++++++++++++++++++++
>>> 4 files changed, 294 insertions(+), 74 deletions(-)
>>> create mode 100644 drivers/input/touchscreen/tsc2007.h
>>> rename drivers/input/touchscreen/{tsc2007.c => tsc2007_core.c} (86%)
>>> create mode 100644 drivers/input/touchscreen/tsc2007_iio.c
>>>
>>> diff --git a/drivers/input/touchscreen/Makefile
>>> b/drivers/input/touchscreen/Makefile
>>> index 81b8645..3be0d19 100644
>>> --- a/drivers/input/touchscreen/Makefile
>>> +++ b/drivers/input/touchscreen/Makefile
>>> @@ -80,6 +80,13 @@ obj-$(CONFIG_TOUCHSCREEN_TSC_SERIO) += tsc40.o
>>> obj-$(CONFIG_TOUCHSCREEN_TSC200X_CORE) += tsc200x-core.o
>>> obj-$(CONFIG_TOUCHSCREEN_TSC2004) += tsc2004.o
>>> obj-$(CONFIG_TOUCHSCREEN_TSC2005) += tsc2005.o
>>> +tsc2007-y := tsc2007_core.o
>>> +ifeq ($(CONFIG_IIO),y)
>>> +tsc2007-y += tsc2007_iio.o
>>> +endif
>>> +ifeq ($(CONFIG_IIO),m)
>>> +tsc2007-y += tsc2007_iio.o
>>
>> Not tsc2007-m ?
>>
>> I don't follow how this works!
>
> I guess tsc2007-y is an internal collector variable name
> for multiple .o components. Sort of a "library" object.
>
> While
>
> obj-y += tsc2007.o adds it to the kernel
> obj-m += tsc2007.o adds it to the modules
>
> I am not sure if my explanation is correct but it appears
> to work that way.
>
> Anyways what shall we do? If CONFIG_TOUCHSCREEN_TSC2007=y
> and IIO=m we have a problem that we need dynamic binding.
Yes, we just need to block that particular combination. Only build in the IIO support
if it is also built in. That's way I thought we'd want to add it tsc2007-m which would
only be used if tsc2007 as a whole was built as a module.
Otherwise it would be ignored (I think!)
I'm not seeing this structure anywhere else in kernel - hence cc'd Yann and the Kbuild list
to see if they can offer some advices.
As a quick summary, we are looking to add IIO support to this driver in the following circumstances.
IIO and this driver are modules. (ideally handling the dependencies nicely)
IIO and this driver are both built in.
Problem case is driver built in and IIO as a module.
Jonathan
>
>>> +endif
>>> obj-$(CONFIG_TOUCHSCREEN_TSC2007) += tsc2007.o
>>> obj-$(CONFIG_TOUCHSCREEN_UCB1400) += ucb1400_ts.o
>>> obj-$(CONFIG_TOUCHSCREEN_WACOM_W8001) += wacom_w8001.o
>>> diff --git a/drivers/input/touchscreen/tsc2007.h
>>> b/drivers/input/touchscreen/tsc2007.h
>>> new file mode 100644
>>> index 0000000..c25932f
>>> --- /dev/null
>>> +++ b/drivers/input/touchscreen/tsc2007.h
>>> @@ -0,0 +1,116 @@
>>> +/*
>>> + * Copyright (c) 2008 MtekVision Co., Ltd.
>>> + * Kwangwoo Lee <kwlee-ec7hoAtq5SbSUeElwK9/Pw@public.gmane.org>
>>> + *
>>> + * Using code from:
>>> + * - ads7846.c
>>> + * Copyright (c) 2005 David Brownell
>>> + * Copyright (c) 2006 Nokia Corporation
>>> + * - corgi_ts.c
>>> + * Copyright (C) 2004-2005 Richard Purdie
>>> + * - omap_ts.[hc], ads7846.h, ts_osk.c
>>> + * Copyright (C) 2002 MontaVista Software
>>> + * Copyright (C) 2004 Texas Instruments
>>> + * Copyright (C) 2005 Dirk Behme
>>> + *
>>> + * 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 <linux/input/touchscreen.h>
>>> +
>>> +#define TSC2007_MEASURE_TEMP0 (0x0 << 4)
>>> +#define TSC2007_MEASURE_AUX (0x2 << 4)
>>> +#define TSC2007_MEASURE_TEMP1 (0x4 << 4)
>>> +#define TSC2007_ACTIVATE_XN (0x8 << 4)
>>> +#define TSC2007_ACTIVATE_YN (0x9 << 4)
>>> +#define TSC2007_ACTIVATE_YP_XN (0xa << 4)
>>> +#define TSC2007_SETUP (0xb << 4)
>>> +#define TSC2007_MEASURE_X (0xc << 4)
>>> +#define TSC2007_MEASURE_Y (0xd << 4)
>>> +#define TSC2007_MEASURE_Z1 (0xe << 4)
>>> +#define TSC2007_MEASURE_Z2 (0xf << 4)
>>> +
>>> +#define TSC2007_POWER_OFF_IRQ_EN (0x0 << 2)
>>> +#define TSC2007_ADC_ON_IRQ_DIS0 (0x1 << 2)
>>> +#define TSC2007_ADC_OFF_IRQ_EN (0x2 << 2)
>>> +#define TSC2007_ADC_ON_IRQ_DIS1 (0x3 << 2)
>>> +
>>> +#define TSC2007_12BIT (0x0 << 1)
>>> +#define TSC2007_8BIT (0x1 << 1)
>>> +
>>> +#define MAX_12BIT ((1 << 12) - 1)
>>> +
>>> +#define ADC_ON_12BIT (TSC2007_12BIT | TSC2007_ADC_ON_IRQ_DIS0)
>>> +
>>> +#define READ_Y (ADC_ON_12BIT | TSC2007_MEASURE_Y)
>>> +#define READ_Z1 (ADC_ON_12BIT | TSC2007_MEASURE_Z1)
>>> +#define READ_Z2 (ADC_ON_12BIT | TSC2007_MEASURE_Z2)
>>> +#define READ_X (ADC_ON_12BIT | TSC2007_MEASURE_X)
>>> +#define PWRDOWN (TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
>>> +
>>> +struct ts_event {
>>> + u16 x;
>>> + u16 y;
>>> + u16 z1, z2;
>>> +};
>>> +
>>> +struct tsc2007 {
>>> + struct input_dev *input;
>>> + char phys[32];
>>> +
>>> + struct i2c_client *client;
>>> +
>>> + u16 model;
>>> + u16 x_plate_ohms;
>>> +
>>> + struct touchscreen_properties prop;
>>> +
>>> + bool report_resistance;
>>> + u16 min_x;
>>> + u16 min_y;
>>> + u16 max_x;
>>> + u16 max_y;
>>> + u16 max_rt;
>>> + unsigned long poll_period; /* in jiffies */
>>> + int fuzzx;
>>> + int fuzzy;
>>> + int fuzzz;
>>> +
>>> + unsigned int gpio;
>>> + int irq;
>>> +
>>> + wait_queue_head_t wait;
>>> + bool stopped;
>>> + bool pendown;
>>> +
>>> + int (*get_pendown_state)(struct device *);
>>> + void (*clear_penirq)(void);
>>> +
>>> + struct mutex mlock;
>>> + struct iio_dev *iio_dev; /* optional */
>>> +};
>>> +
>>> +int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd);
>>> +u32 tsc2007_calculate_resistance(struct tsc2007 *tsc,
>>> + struct ts_event *tc);
>>> +bool tsc2007_is_pen_down(struct tsc2007 *ts);
>>> +
>>> +#if IS_ENABLED(CONFIG_IIO)
>>> +
>>> +/* defined in tsc2007_iio.c */
>>> +int tsc2007_iio_configure(struct tsc2007 *ts);
>>> +void tsc2007_iio_unconfigure(struct tsc2007 *ts);
>>> +
>>> +#else /* CONFIG_IIO */
>>> +
>>> +static inline int tsc2007_iio_configure(struct tsc2007 *ts)
>>> +{
>>> + return 0;
>>> +}
>>> +static inline void tsc2007_iio_unconfigure(struct tsc2007 *ts)
>>> +{
>>> +}
>>> +
>>> +#endif /* CONFIG_IIO */
>>> diff --git a/drivers/input/touchscreen/tsc2007.c
>>> b/drivers/input/touchscreen/tsc2007_core.c
>>> similarity index 86%
>>> rename from drivers/input/touchscreen/tsc2007.c
>>> rename to drivers/input/touchscreen/tsc2007_core.c
>>> index 76b462b..812ded8 100644
>>> --- a/drivers/input/touchscreen/tsc2007.c
>>> +++ b/drivers/input/touchscreen/tsc2007_core.c
>>> @@ -27,79 +27,11 @@
>>> #include <linux/i2c.h>
>>> #include <linux/i2c/tsc2007.h>
>>> #include <linux/of_device.h>
>>> -#include <linux/of.h>
>>> #include <linux/of_gpio.h>
>>> -#include <linux/input/touchscreen.h>
>>> -
>>> -#define TSC2007_MEASURE_TEMP0 (0x0 << 4)
>>> -#define TSC2007_MEASURE_AUX (0x2 << 4)
>>> -#define TSC2007_MEASURE_TEMP1 (0x4 << 4)
>>> -#define TSC2007_ACTIVATE_XN (0x8 << 4)
>>> -#define TSC2007_ACTIVATE_YN (0x9 << 4)
>>> -#define TSC2007_ACTIVATE_YP_XN (0xa << 4)
>>> -#define TSC2007_SETUP (0xb << 4)
>>> -#define TSC2007_MEASURE_X (0xc << 4)
>>> -#define TSC2007_MEASURE_Y (0xd << 4)
>>> -#define TSC2007_MEASURE_Z1 (0xe << 4)
>>> -#define TSC2007_MEASURE_Z2 (0xf << 4)
>>> -
>>> -#define TSC2007_POWER_OFF_IRQ_EN (0x0 << 2)
>>> -#define TSC2007_ADC_ON_IRQ_DIS0 (0x1 << 2)
>>> -#define TSC2007_ADC_OFF_IRQ_EN (0x2 << 2)
>>> -#define TSC2007_ADC_ON_IRQ_DIS1 (0x3 << 2)
>>> -
>>> -#define TSC2007_12BIT (0x0 << 1)
>>> -#define TSC2007_8BIT (0x1 << 1)
>>> -
>>> -#define MAX_12BIT ((1 << 12) - 1)
>>> -
>>> -#define ADC_ON_12BIT (TSC2007_12BIT | TSC2007_ADC_ON_IRQ_DIS0)
>>> -
>>> -#define READ_Y (ADC_ON_12BIT | TSC2007_MEASURE_Y)
>>> -#define READ_Z1 (ADC_ON_12BIT | TSC2007_MEASURE_Z1)
>>> -#define READ_Z2 (ADC_ON_12BIT | TSC2007_MEASURE_Z2)
>>> -#define READ_X (ADC_ON_12BIT | TSC2007_MEASURE_X)
>>> -#define PWRDOWN (TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
>>> -
>>> -struct ts_event {
>>> - u16 x;
>>> - u16 y;
>>> - u16 z1, z2;
>>> -};
>>> -
>>> -struct tsc2007 {
>>> - struct input_dev *input;
>>> - char phys[32];
>>> -
>>> - struct i2c_client *client;
>>> -
>>> - u16 model;
>>> - u16 x_plate_ohms;
>>> -
>>> - struct touchscreen_properties prop;
>>> -
>>> - bool report_resistance;
>>> - u16 min_x;
>>> - u16 min_y;
>>> - u16 max_x;
>>> - u16 max_y;
>>> - u16 max_rt;
>>> - unsigned long poll_period; /* in jiffies */
>>> - int fuzzx;
>>> - int fuzzy;
>>> - int fuzzz;
>>> -
>>> - unsigned gpio;
>>> - int irq;
>>> -
>>> - wait_queue_head_t wait;
>>> - bool stopped;
>>> +#include "tsc2007.h"
>>>
>>> - int (*get_pendown_state)(struct device *);
>>> - void (*clear_penirq)(void);
>>> -};
>>>
>>> -static inline int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
>>> +int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
>>> {
>>> s32 data;
>>> u16 val;
>>> @@ -137,7 +69,7 @@ static void tsc2007_read_values(struct tsc2007 *tsc,
>>> struct ts_event *tc)
>>> tsc2007_xfer(tsc, PWRDOWN);
>>> }
>>>
>>> -static u32 tsc2007_calculate_resistance(struct tsc2007 *tsc,
>>> +u32 tsc2007_calculate_resistance(struct tsc2007 *tsc,
>>> struct ts_event *tc)
>>> {
>>> u32 rt = 0;
>>> @@ -158,7 +90,7 @@ static u32 tsc2007_calculate_resistance(struct
>>> tsc2007 *tsc,
>>> return rt;
>>> }
>>>
>>> -static bool tsc2007_is_pen_down(struct tsc2007 *ts)
>>> +bool tsc2007_is_pen_down(struct tsc2007 *ts)
>>> {
>>> /*
>>> * NOTE: We can't rely on the pressure to determine the pen down
>>> @@ -191,7 +123,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void
>>> *handle)
>>> while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>>>
>>> /* pen is down, continue with the measurement */
>>> +
>>> + mutex_lock(&ts->mlock);
>>> tsc2007_read_values(ts, &tc);
>>> + mutex_unlock(&ts->mlock);
>>>
>>> rt = tsc2007_calculate_resistance(ts, &tc);
>>>
>>> @@ -441,7 +376,8 @@ static void tsc2007_call_exit_platform_hw(void
>>> *data)
>>> static int tsc2007_probe(struct i2c_client *client,
>>> const struct i2c_device_id *id)
>>> {
>>> - const struct tsc2007_platform_data *pdata =
>>> dev_get_platdata(&client->dev);
>>> + const struct tsc2007_platform_data *pdata =
>>> + dev_get_platdata(&client->dev);
>>> struct tsc2007 *ts;
>>> struct input_dev *input_dev;
>>> int err;
>>> @@ -463,7 +399,9 @@ static int tsc2007_probe(struct i2c_client *client,
>>> ts->client = client;
>>> ts->irq = client->irq;
>>> ts->input = input_dev;
>>> +
>>> init_waitqueue_head(&ts->wait);
>>> + mutex_init(&ts->mlock);
>>>
>>> snprintf(ts->phys, sizeof(ts->phys),
>>> "%s/input0", dev_name(&client->dev));
>>> @@ -534,7 +472,7 @@ static int tsc2007_probe(struct i2c_client *client,
>>> if (err < 0) {
>>> dev_err(&client->dev,
>>> "Failed to setup chip: %d\n", err);
>>> - return err; /* usually, chip does not respond */
>>> + return err; /* chip does not respond */
>>> }
>>>
>>> err = input_register_device(input_dev);
>>> @@ -544,6 +482,14 @@ static int tsc2007_probe(struct i2c_client
>>> *client,
>>> return err;
>>> }
>>>
>>> + return tsc2007_iio_configure(ts);
>>> +}
>>> +
>>> +static int tsc2007_remove(struct i2c_client *client)
>>> +{
>>> + struct tsc2007 *ts = i2c_get_clientdata(client);
>>> +
>>> + tsc2007_iio_unconfigure(ts);
>>> return 0;
>>> }
>>>
>>> @@ -569,6 +515,7 @@ static struct i2c_driver tsc2007_driver = {
>>> },
>>> .id_table = tsc2007_idtable,
>>> .probe = tsc2007_probe,
>>> + .remove = tsc2007_remove,
>>> };
>>>
>>> module_i2c_driver(tsc2007_driver);
>>> diff --git a/drivers/input/touchscreen/tsc2007_iio.c
>>> b/drivers/input/touchscreen/tsc2007_iio.c
>>> new file mode 100644
>>> index 0000000..ed79944
>>> --- /dev/null
>>> +++ b/drivers/input/touchscreen/tsc2007_iio.c
>>> @@ -0,0 +1,150 @@
>>> +/*
>>> + * Copyright (c) 2016 Golden Delicious Comp. GmbH&Co. KG
>>> + * Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
>>> + *
>>> + * 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 <linux/i2c.h>
>>> +#include <linux/iio/iio.h>
>>> +#include "tsc2007.h"
>>> +
>>> +struct tsc2007_iio {
>>> + struct tsc2007 *ts;
>>> +};
>>> +
>>> +#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
>>> +{ \
>>> + .datasheet_name = _name, \
>>> + .type = _type, \
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>>> + BIT(_chan_info), \
>>> + .indexed = 1, \
>>> + .channel = _chan, \
>>> +}
>>> +
>>> +static const struct iio_chan_spec tsc2007_iio_channel[] = {
>>> + TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>> + TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>> + TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>> + TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>> + TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
>>> + TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms?
>>> */
>>> + TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
>>> + TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
>>> + TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
>>> +};
>>> +
>>> +static int tsc2007_read_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan, int *val, int *val2, long mask)
>>> +{
>>> + struct tsc2007_iio *iio = iio_priv(indio_dev);
>>> + struct tsc2007 *tsc = iio->ts;
>>> + int adc_chan = chan->channel;
>>> + int ret = 0;
>>> +
>>> + if (adc_chan >= ARRAY_SIZE(tsc2007_iio_channel))
>>> + return -EINVAL;
>>> +
>>> + if (mask != IIO_CHAN_INFO_RAW)
>>> + return -EINVAL;
>>> +
>>> + mutex_lock(&tsc->mlock);
>>> +
>>> + switch (chan->channel) {
>>> + case 0:
>>> + *val = tsc2007_xfer(tsc, READ_X);
>>> + break;
>>> + case 1:
>>> + *val = tsc2007_xfer(tsc, READ_Y);
>>> + break;
>>> + case 2:
>>> + *val = tsc2007_xfer(tsc, READ_Z1);
>>> + break;
>>> + case 3:
>>> + *val = tsc2007_xfer(tsc, READ_Z2);
>>> + break;
>>> + case 4:
>>> + *val = tsc2007_xfer(tsc, (ADC_ON_12BIT | TSC2007_MEASURE_AUX));
>>> + break;
>>> + case 5: {
>>> + struct ts_event tc;
>>> +
>>> + tc.x = tsc2007_xfer(tsc, READ_X);
>>> + tc.z1 = tsc2007_xfer(tsc, READ_Z1);
>>> + tc.z2 = tsc2007_xfer(tsc, READ_Z2);
>>> + *val = tsc2007_calculate_resistance(tsc, &tc);
>>> + break;
>>> + }
>>> + case 6:
>>> + *val = tsc2007_is_pen_down(tsc);
>>> + break;
>>> + case 7:
>>> + *val = tsc2007_xfer(tsc,
>>> + (ADC_ON_12BIT | TSC2007_MEASURE_TEMP0));
>>> + break;
>>> + case 8:
>>> + *val = tsc2007_xfer(tsc,
>>> + (ADC_ON_12BIT | TSC2007_MEASURE_TEMP1));
>>> + break;
>>> + }
>>> +
>>> + /* Prepare for next touch reading - power down ADC, enable PENIRQ */
>>> + tsc2007_xfer(tsc, PWRDOWN);
>>> +
>>> + mutex_unlock(&tsc->mlock);
>>> +
>>> + ret = IIO_VAL_INT;
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static const struct iio_info tsc2007_iio_info = {
>>> + .read_raw = tsc2007_read_raw,
>>> + .driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +int tsc2007_iio_configure(struct tsc2007 *ts)
>>> +{
>>> + int err;
>>> + struct iio_dev *indio_dev;
>>> + struct tsc2007_iio *iio;
>>> +
>>> + indio_dev = devm_iio_device_alloc(&ts->client->dev,
>>> + sizeof(struct tsc2007_iio));
>>> + if (!indio_dev) {
>>> + dev_err(&ts->client->dev, "iio_device_alloc failed\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + iio = iio_priv(indio_dev);
>>> + iio->ts = ts;
>>> + ts->iio_dev = (void *) indio_dev;
>>> +
>>> + indio_dev->name = "tsc2007";
>>> + indio_dev->dev.parent = &ts->client->dev;
>>> + indio_dev->info = &tsc2007_iio_info;
>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>> + indio_dev->channels = tsc2007_iio_channel;
>>> + indio_dev->num_channels = ARRAY_SIZE(tsc2007_iio_channel);
>>> +
>>> + err = iio_device_register(indio_dev);
>>> + if (err < 0) {
>>> + dev_err(&ts->client->dev, "iio_device_register() failed: %d\n",
>>> + err);
>>> + return err;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(tsc2007_iio_configure);
>>> +
>>> +void tsc2007_iio_unconfigure(struct tsc2007 *ts)
>>> +{
>>> + struct iio_dev *indio_dev = ts->iio_dev;
>>> +
>>> + iio_device_unregister(indio_dev);
>>> +}
>>> +EXPORT_SYMBOL(tsc2007_iio_unconfigure);
>>
>> --
>> Sent from my Android device with K-9 Mail. Please excuse my brevity.
>
^ permalink raw reply
* Re: [PATCH v2 2/3] ARM: dts: sunxi: add support for Orange Pi Zero board
From: Icenowy Zheng @ 2016-11-27 9:36 UTC (permalink / raw)
To: Jonathan Corbet, Maxime Ripard, Chen-Yu Tsai, Mark Rutland,
Russell King, Hans de Goede
Cc: devicetree@vger.kernel.org, Vishnu Patekar, Arnd Bergmann,
linux-doc@vger.kernel.org, Andre Przywara,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20161121162421.800-2-icenowy@aosc.xyz>
22.11.2016, 00:26, "Icenowy Zheng" <icenowy@aosc.xyz>:
> Orange Pi Zero is a board that came with the new Allwinner H2+ SoC.
>
> Add a device tree file for it.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> ---
> Changes since v2:
> - Use generic pinconf binding instead of legacy allwinner pinctrl binding.
> - removed uart3, which is not accessible on Orange Pi Zero.
> - Removed sun8i-h2plus.dtsi and make Orange Pi Zero dts directly include
> sun8i-h3.dtsi.
> - Removed allwinner,sun8i-h3 compatible.
>
> arch/arm/boot/dts/Makefile | 1 +
> arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts | 137 +++++++++++++++++++++++
> 2 files changed, 138 insertions(+)
> create mode 100644 arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 802a10d..51a1dd7 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -834,6 +834,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
> sun8i-a33-sinlinx-sina33.dtb \
> sun8i-a83t-allwinner-h8homlet-v2.dtb \
> sun8i-a83t-cubietruck-plus.dtb \
> + sun8i-h2plus-orangepi-zero.dtb \
> sun8i-h3-bananapi-m2-plus.dtb \
> sun8i-h3-nanopi-neo.dtb \
> sun8i-h3-orangepi-2.dtb \
> diff --git a/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts b/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
> new file mode 100644
> index 0000000..b428e47
> --- /dev/null
> +++ b/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
> @@ -0,0 +1,137 @@
> +/*
> + * Copyright (C) 2016 Icenowy Zheng <icenowy@aosc.xyz>
> + *
> + * Based on sun8i-h3-orangepi-one.dts, which is:
> + * Copyright (C) 2016 Hans de Goede <hdegoede@redhat.com>
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + * a) This file 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 file 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.
> + *
> + * Or, alternatively,
> + *
> + * b) Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use,
> + * copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following
> + * conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +/dts-v1/;
> +#include "sun8i-h3.dtsi"
> +#include "sunxi-common-regulators.dtsi"
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/pinctrl/sun4i-a10.h>
> +
> +/ {
> + model = "Xunlong Orange Pi Zero";
> + compatible = "xunlong,orangepi-zero", "allwinner,sun8i-h2plus";
> +
> + aliases {
> + serial0 = &uart0;
> + };
> +
> + chosen {
> + stdout-path = "serial0:115200n8";
> + };
> +
> + leds {
> + compatible = "gpio-leds";
> + pinctrl-names = "default";
> + pinctrl-0 = <&leds_opi0>, <&leds_r_opi0>;
> +
> + pwr_led {
> + label = "orangepi:green:pwr";
> + gpios = <&r_pio 0 10 GPIO_ACTIVE_HIGH>;
> + default-state = "on";
> + };
> +
> + status_led {
> + label = "orangepi:red:status";
> + gpios = <&pio 0 17 GPIO_ACTIVE_HIGH>;
> + };
> + };
> +};
> +
> +&ehci1 {
> + status = "okay";
> +};
> +
> +&mmc0 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin>;
> + vmmc-supply = <®_vcc3v3>;
> + bus-width = <4>;
> + cd-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */
> + cd-inverted;
> + status = "okay";
> +};
> +
> +&ohci1 {
> + status = "okay";
> +};
> +
> +&pio {
> + leds_opi0: led_pins@0 {
> + pins = "PA17";
> + function = "gpio_out";
> + };
> +};
> +
> +&r_pio {
> + leds_r_opi0: led_pins@0 {
> + pins = "PL10";
> + function = "gpio_out";
> + };
> +};
> +
> +&uart0 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart0_pins_a>;
> + status = "okay";
> +};
> +
> +&uart1 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart1_pins>;
> + status = "disabled";
> +};
> +
> +&uart2 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart2_pins>;
> + status = "disabled";
> +};
> +
> +&usbphy {
> + /* USB VBUS is always on */
> + status = "okay";
> +};
Something more interesting happened.
Xunlong made a add-on board for Orange Pi Zero, which exposes the two USB Controllers exported at expansion bus as USB Type-A connectors.
Also it exposes a analog A/V jack and a microphone.
Should I enable {e,o}hci{2.3} in the device tree?
> --
> 2.10.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 2/2] input: touchscreen: sample averaging for imx6ul_tsc
From: Guy Shapiro @ 2016-11-27 7:44 UTC (permalink / raw)
To: dmitry.torokhov
Cc: fabio.estevam, mark.rutland, Guy Shapiro, devicetree, haibo.chen,
robh+dt, linux-input, linux-arm-kernel
In-Reply-To: <1480232698-23075-1-git-send-email-guy.shapiro@mobi-wize.com>
The i.MX6UL internal touchscreen controller contains an option to
average upon samples. This feature reduces noise from the produced
touch locations.
This patch adds sample averaging support to the imx6ul_tsc device
driver.
Signed-off-by: Guy Shapiro <guy.shapiro@mobi-wize.com>
---
drivers/input/touchscreen/imx6ul_tsc.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/input/touchscreen/imx6ul_tsc.c b/drivers/input/touchscreen/imx6ul_tsc.c
index 8275267..31724d9 100644
--- a/drivers/input/touchscreen/imx6ul_tsc.c
+++ b/drivers/input/touchscreen/imx6ul_tsc.c
@@ -25,6 +25,7 @@
/* ADC configuration registers field define */
#define ADC_AIEN (0x1 << 7)
#define ADC_CONV_DISABLE 0x1F
+#define ADC_AVGE (0x1 << 5)
#define ADC_CAL (0x1 << 7)
#define ADC_CALF 0x2
#define ADC_12BIT_MODE (0x2 << 2)
@@ -32,6 +33,7 @@
#define ADC_CLK_DIV_8 (0x03 << 5)
#define ADC_SHORT_SAMPLE_MODE (0x0 << 4)
#define ADC_HARDWARE_TRIGGER (0x1 << 13)
+#define ADC_AVGS_SHIFT 14
#define SELECT_CHANNEL_4 0x04
#define SELECT_CHANNEL_1 0x01
#define DISABLE_CONVERSION_INT (0x0 << 7)
@@ -86,6 +88,7 @@ struct imx6ul_tsc {
int measure_delay_time;
int pre_charge_time;
+ int average_samples;
struct completion completion;
};
@@ -107,6 +110,8 @@ static int imx6ul_adc_init(struct imx6ul_tsc *tsc)
adc_cfg = readl(tsc->adc_regs + REG_ADC_CFG);
adc_cfg |= ADC_12BIT_MODE | ADC_IPG_CLK;
adc_cfg |= ADC_CLK_DIV_8 | ADC_SHORT_SAMPLE_MODE;
+ if (tsc->average_samples)
+ adc_cfg |= (tsc->average_samples - 1) << ADC_AVGS_SHIFT;
adc_cfg &= ~ADC_HARDWARE_TRIGGER;
writel(adc_cfg, tsc->adc_regs + REG_ADC_CFG);
@@ -118,6 +123,8 @@ static int imx6ul_adc_init(struct imx6ul_tsc *tsc)
/* start ADC calibration */
adc_gc = readl(tsc->adc_regs + REG_ADC_GC);
adc_gc |= ADC_CAL;
+ if (tsc->average_samples)
+ adc_gc |= ADC_AVGE;
writel(adc_gc, tsc->adc_regs + REG_ADC_GC);
timeout = wait_for_completion_timeout
@@ -450,6 +457,16 @@ static int imx6ul_tsc_probe(struct platform_device *pdev)
if (err)
tsc->pre_charge_time = 0xfff;
+ err = of_property_read_u32(np, "average-samples",
+ &tsc->average_samples);
+ if (err)
+ tsc->average_samples = 0;
+ if (tsc->average_samples > 4) {
+ dev_err(&pdev->dev, "average-samples (%u) must be [0-4]\n",
+ tsc->average_samples);
+ return -EINVAL;
+ }
+
err = input_register_device(tsc->input);
if (err) {
dev_err(&pdev->dev,
--
2.1.4
^ permalink raw reply related
* [PATCH 1/2] Documentation: sample averaging for imx6ul_tsc
From: Guy Shapiro @ 2016-11-27 7:44 UTC (permalink / raw)
To: dmitry.torokhov
Cc: fabio.estevam, mark.rutland, Guy Shapiro, devicetree, haibo.chen,
robh+dt, linux-input, linux-arm-kernel
The i.MX6UL internal touchscreen controller contains an option to
average upon samples. This feature reduces noise from the produced
touch locations.
This patch introduces a new device tree optional property for this
feature. It provides control over the amount of averaged samples per
touch event.
The property was inspired by a similar property on the
"brcm,iproc-touchscreen" binding.
Signed-off-by: Guy Shapiro <guy.shapiro@mobi-wize.com>
---
.../devicetree/bindings/input/touchscreen/imx6ul_tsc.txt | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/imx6ul_tsc.txt b/Documentation/devicetree/bindings/input/touchscreen/imx6ul_tsc.txt
index 853dff9..a66069f 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/imx6ul_tsc.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/imx6ul_tsc.txt
@@ -17,6 +17,13 @@ Optional properties:
This value depends on the touch screen.
- pre-charge-time: the touch screen need some time to precharge.
This value depends on the touch screen.
+- average-samples: Number of data samples which are averaged for each read.
+ Valid values 0-4
+ 0 = 1 sample
+ 1 = 4 samples
+ 2 = 8 samples
+ 3 = 16 samples
+ 4 = 32 samples
Example:
tsc: tsc@02040000 {
@@ -32,5 +39,6 @@ Example:
xnur-gpio = <&gpio1 3 GPIO_ACTIVE_LOW>;
measure-delay-time = <0xfff>;
pre-charge-time = <0xffff>;
+ average-samples = <4>;
status = "okay";
};
--
2.1.4
^ permalink raw reply related
* Re: [PATCH 2/2] net: phy: realtek: fix enabling of the TX-delay for RTL8211F
From: Florian Fainelli @ 2016-11-27 5:55 UTC (permalink / raw)
To: Martin Blumenstingl, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
sean.wang-NuS5LvNUpcJWk0Htik3J/w, netdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
jbrunet-rdvid1DuHRBWk0Htik3J/w
In-Reply-To: <20161125131201.19994-3-martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
On 11/25/2016 05:12 AM, Martin Blumenstingl wrote:
> The old logic always enabled the TX-delay when the phy-mode was set to
> PHY_INTERFACE_MODE_RGMII. There are dedicated phy-modes which tell the
> PHY driver to enable the RX and/or TX delays:
> - PHY_INTERFACE_MODE_RGMII should disable the RX and TX delay in the
> PHY (if required, the MAC should add the delays in this case)
> - PHY_INTERFACE_MODE_RGMII_ID should enable RX and TX delay in the PHY
> - PHY_INTERFACE_MODE_RGMII_TXID should enable the TX delay in the PHY
> - PHY_INTERFACE_MODE_RGMII_RXID should enable the RX delay in the PHY
> (currently not supported by RTL8211F)
>
> With this patch we enable the TX delay for PHY_INTERFACE_MODE_RGMII_ID
> and PHY_INTERFACE_MODE_RGMII_TXID.
> Additionally we now explicity disable the TX-delay, which seems to be
> enabled automatically after a hard-reset of the PHY (by triggering it's
> reset pin) to get a consistent state (as defined by the phy-mode).
>
> This fixes a compatibility problem with some SoCs where the TX-delay was
> also added by the MAC. With the TX-delay being applied twice the TX
> clock was off and TX traffic was broken or very slow (<10Mbit/s) on
> 1000Mbit/s links.
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
Reviewed-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/2] Documentation: devicetree: clarify usage of the RGMII phy-modes
From: Florian Fainelli @ 2016-11-27 5:53 UTC (permalink / raw)
To: Martin Blumenstingl, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
sean.wang-NuS5LvNUpcJWk0Htik3J/w, netdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
jbrunet-rdvid1DuHRBWk0Htik3J/w
In-Reply-To: <20161125131201.19994-2-martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
On 11/25/2016 05:12 AM, Martin Blumenstingl wrote:
> RGMII requires special RX and/or TX delays depending on the actual
> hardware circuit/wiring. These delays can be added by the MAC, the PHY
> or the designer of the circuit (the latter means that no delay has to
> be added by PHY or MAC).
> There are 4 RGMII phy-modes used describe where a delay should be
> applied:
> - rgmii: the RX and TX delays are either added by the MAC (where the
> exact delay is typically configurable, and can be turned off when no
> extra delay is needed) or not needed at all (because the hardware
> wiring adds the delay already). The PHY should neither add the RX nor
> TX delay in this case.
> - rgmii-rxid: configures the PHY to enable the RX delay. The MAC should
> not add the RX delay in this case.
> - rgmii-txid: configures the PHY to enable the TX delay. The MAC should
> not add the TX delay in this case.
> - rgmii-id: combines rgmii-rxid and rgmii-txid and thus configures the
> PHY to enable the RX and TX delays. The MAC should neither add the RX
> nor TX delay in this case.
>
> Document these cases in the ethernet.txt documentation to make it clear
> when to use each mode.
> If applied incorrectly one might end up with MAC and PHY both enabling
> for example the TX delay, which breaks ethernet TX traffic on 1000Mbit/s
> links.
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
Reviewed-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 1/2] i2c: aspeed: added driver for Aspeed I2C
From: Vladimir Zapolskiy @ 2016-11-26 22:31 UTC (permalink / raw)
To: Brendan Higgins, mark.rutland
Cc: wsa, robh+dt, linux-i2c, devicetree, joel, openbmc
In-Reply-To: <1478311099-6771-2-git-send-email-brendanhiggins@google.com>
Hello Brendan,
please find some review notes below.
On 11/05/2016 03:58 AM, Brendan Higgins wrote:
> Added initial master and slave support for Aspeed I2C controller.
> Supports fourteen busses present in ast24xx and ast25xx BMC SoCs by
> Aspeed.
>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
> Changes for v2:
> - Added single module_init (multiple was breaking some builds).
> Changes for v3:
> - Removed "bus" device tree param; now extracted from bus address offset
> Changes for v4:
> - I2C adapter number is now generated dynamically unless specified in alias.
> ---
> drivers/i2c/busses/Kconfig | 10 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-aspeed.c | 807 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 818 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-aspeed.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index d252276..b6caa5d 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1009,6 +1009,16 @@ config I2C_RCAR
> This driver can also be built as a module. If so, the module
> will be called i2c-rcar.
>
> +config I2C_ASPEED
> + tristate "Aspeed AST2xxx SoC I2C Controller"
> + depends on ARCH_ASPEED
> + help
> + If you say yes to this option, support will be included for the
> + Aspeed AST2xxx SoC I2C controller.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-aspeed.
> +
Please try to keep the list ordered, it would be better, if you
add I2C_ASPEED before I2C_AT91.
> comment "External I2C/SMBus adapter drivers"
>
> config I2C_DIOLAN_U2C
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 29764cc..826e780 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -99,6 +99,7 @@ obj-$(CONFIG_I2C_XILINX) += i2c-xiic.o
> obj-$(CONFIG_I2C_XLR) += i2c-xlr.o
> obj-$(CONFIG_I2C_XLP9XX) += i2c-xlp9xx.o
> obj-$(CONFIG_I2C_RCAR) += i2c-rcar.o
> +obj-$(CONFIG_I2C_ASPEED) += i2c-aspeed.o
>
Same comment as above, put it in front of i2c-at91.o
> # External I2C/SMBus adapter drivers
> obj-$(CONFIG_I2C_DIOLAN_U2C) += i2c-diolan-u2c.o
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> new file mode 100644
> index 0000000..88e078a
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -0,0 +1,807 @@
> +/*
> + * I2C adapter for the ASPEED I2C bus.
> + *
> + * Copyright (C) 2012-2020 ASPEED Technology Inc.
I don't think that the copyright dated by 2020 in advance is legal here.
Please change it to the expected ...-2016 value.
> + * Copyright 2016 IBM Corporation
> + * Copyright 2016 Google, Inc.
> + *
> + * 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 <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/errno.h>
> +#include <linux/interrupt.h>
> +#include <linux/completion.h>
> +#include <linux/slab.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
Please sort the headers alphabetically.
> +
> +/* I2C Register */
> +#define ASPEED_I2C_FUN_CTRL_REG 0x00
> +#define ASPEED_I2C_AC_TIMING_REG1 0x04
> +#define ASPEED_I2C_AC_TIMING_REG2 0x08
> +#define ASPEED_I2C_INTR_CTRL_REG 0x0c
> +#define ASPEED_I2C_INTR_STS_REG 0x10
> +#define ASPEED_I2C_CMD_REG 0x14
> +#define ASPEED_I2C_DEV_ADDR_REG 0x18
> +#define ASPEED_I2C_BYTE_BUF_REG 0x20
> +#define ASPEED_I2C_OFFSET_START 0x40
Unused macro, please remove.
> +#define ASPEED_I2C_OFFSET_INCREMENT 0x40
Unused macro, please remove.
> +
> +#define ASPEED_I2C_NUM_BUS 14
> +
> +/* Global Register Definition */
> +/* 0x00 : I2C Interrupt Status Register */
> +/* 0x08 : I2C Interrupt Target Assignment */
> +
> +/* Device Register Definition */
> +/* 0x00 : I2CD Function Control Register */
> +#define ASPEED_I2CD_MULTI_MASTER_DIS BIT(15)
Unused macro, please remove.
> +#define ASPEED_I2CD_SDA_DRIVE_1T_EN BIT(8)
Unused macro, please remove.
> +#define ASPEED_I2CD_M_SDA_DRIVE_1T_EN BIT(7)
> +#define ASPEED_I2CD_M_HIGH_SPEED_EN BIT(6)
> +#define ASPEED_I2CD_SLAVE_EN BIT(1)
Unused macro, please remove. You add slave support, may be you
need this control, but it is unused.
> +#define ASPEED_I2CD_MASTER_EN BIT(0)
> +
> +/* 0x08 : I2CD Clock and AC Timing Control Register #2 */
> +#define ASPEED_NO_TIMEOUT_CTRL 0
> +
> +
> +/* 0x0c : I2CD Interrupt Control Register &
> + * 0x10 : I2CD Interrupt Status Register
> + *
> + * These share bit definitions, so use the same values for the enable &
> + * status bits.
> + */
> +#define ASPEED_I2CD_INTR_SDA_DL_TIMEOUT BIT(14)
> +#define ASPEED_I2CD_INTR_BUS_RECOVER_DONE BIT(13)
> +#define ASPEED_I2CD_INTR_SLAVE_MATCH BIT(7)
> +#define ASPEED_I2CD_INTR_SCL_TIMEOUT BIT(6)
> +#define ASPEED_I2CD_INTR_ABNORMAL BIT(5)
> +#define ASPEED_I2CD_INTR_NORMAL_STOP BIT(4)
> +#define ASPEED_I2CD_INTR_ARBIT_LOSS BIT(3)
> +#define ASPEED_I2CD_INTR_RX_DONE BIT(2)
> +#define ASPEED_I2CD_INTR_TX_NAK BIT(1)
> +#define ASPEED_I2CD_INTR_TX_ACK BIT(0)
> +
> +/* 0x14 : I2CD Command/Status Register */
> +#define ASPEED_I2CD_SCL_LINE_STS BIT(18)
> +#define ASPEED_I2CD_SDA_LINE_STS BIT(17)
> +#define ASPEED_I2CD_BUS_BUSY_STS BIT(16)
> +#define ASPEED_I2CD_BUS_RECOVER_CMD BIT(11)
> +
> +/* Command Bit */
> +#define ASPEED_I2CD_M_STOP_CMD BIT(5)
> +#define ASPEED_I2CD_M_S_RX_CMD_LAST BIT(4)
> +#define ASPEED_I2CD_M_RX_CMD BIT(3)
> +#define ASPEED_I2CD_S_TX_CMD BIT(2)
> +#define ASPEED_I2CD_M_TX_CMD BIT(1)
> +#define ASPEED_I2CD_M_START_CMD BIT(0)
> +
> +/* 0x18 : I2CD Slave Device Address Register */
> +#define ASPEED_I2CD_DEV_ADDR_MASK GENMASK(6, 0)
> +
> +enum aspeed_i2c_slave_state {
> + ASPEED_I2C_SLAVE_START,
> + ASPEED_I2C_SLAVE_READ_REQUESTED,
> + ASPEED_I2C_SLAVE_READ_PROCESSED,
> + ASPEED_I2C_SLAVE_WRITE_REQUESTED,
> + ASPEED_I2C_SLAVE_WRITE_RECEIVED,
> + ASPEED_I2C_SLAVE_STOP,
> +};
> +
> +struct aspeed_i2c_bus {
> + struct i2c_adapter adap;
> + struct device *dev;
> + void __iomem *base;
> + spinlock_t lock;
> + struct completion cmd_complete;
> + int irq;
> + /* Transaction state. */
> + struct i2c_msg *msg;
> + int msg_pos;
> + u32 cmd_err;
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> + struct i2c_client *slave;
> + enum aspeed_i2c_slave_state slave_state;
> +#endif
> +};
> +
> +struct aspeed_i2c_controller {
> + struct device *dev;
> + void __iomem *base;
> + int irq;
> + struct irq_domain *irq_domain;
> +};
> +
> +static inline void aspeed_i2c_write(struct aspeed_i2c_bus *bus, u32 val,
> + u32 reg)
> +{
> + writel(val, bus->base + reg);
> +}
> +
> +static inline u32 aspeed_i2c_read(struct aspeed_i2c_bus *bus, u32 reg)
> +{
> + return readl(bus->base + reg);
> +}
> +
> +static u8 aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
The function may return errors as negative numbers, please
change its return type to 'int'.
> +{
> + u32 command;
> + unsigned long time_left;
> + unsigned long flags;
> + int ret = 0;
Please whenever possible use "reverse christmas tree" order while
declaring local variables, this applies to all functions, here it
should be
unsigned long time_left, flags;
int ret = 0;
u32 command;
> +
> + spin_lock_irqsave(&bus->lock, flags);
> + command = aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG);
> + /* Bus is idle: no recovery needed. */
> + if ((command & ASPEED_I2CD_SDA_LINE_STS) &&
> + (command & ASPEED_I2CD_SCL_LINE_STS))
> + goto out;
> +
> + dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
> + command);
> +
> + /* Bus held: put bus in stop state. */
> + if ((command & ASPEED_I2CD_SDA_LINE_STS) &&
> + !(command & ASPEED_I2CD_SCL_LINE_STS)) {
> + aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD,
> + ASPEED_I2C_CMD_REG);
> + reinit_completion(&bus->cmd_complete);
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> + time_left = wait_for_completion_interruptible_timeout(
> + &bus->cmd_complete, bus->adap.timeout * HZ);
> +
> + spin_lock_irqsave(&bus->lock, flags);
> + if (time_left == 0)
> + ret = -ETIMEDOUT;
> + else if (bus->cmd_err)
> + ret = -EIO;
Since you use wait_for_completion_interruptible_timeout(), please
handle possible -ERESTARTSYS error if the execution is interrupted.
You have 5 quite similar wait_for_completion_interruptible_timeout()
functions in the driver, please add it to all of them, or you may
think how to wrap it properly into a new function.
> + /* Bus error. */
> + } else if (!(command & ASPEED_I2CD_SDA_LINE_STS)) {
You don't need to introduce "else if" here, at this point
!(command & ASPEED_I2CD_SDA_LINE_STS) expression is always true.
Please drop dev_dbg("bus hang") statement from above (or put it to
an if-branch from two ones described below) and for simplicity
rearrange the code:
if (command & ASPEED_I2CD_SDA_LINE_STS) {
/* Bus is idle: no recovery needed. */
if (command & ASPEED_I2CD_SCL_LINE_STS)
goto out;
/* Bus held: put bus in stop state. */
.....
} else {
/* Bus error. */
.....
}
> + aspeed_i2c_write(bus, ASPEED_I2CD_BUS_RECOVER_CMD,
> + ASPEED_I2C_CMD_REG);
> + reinit_completion(&bus->cmd_complete);
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> + time_left = wait_for_completion_interruptible_timeout(
> + &bus->cmd_complete, bus->adap.timeout * HZ);
> +
> + spin_lock_irqsave(&bus->lock, flags);
> + if (time_left == 0)
> + ret = -ETIMEDOUT;
> + else if (bus->cmd_err)
> + ret = -EIO;
> + /* Recovery failed. */
> + else if (!(aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG) &
> + ASPEED_I2CD_SDA_LINE_STS))
> + ret = -EIO;
> + }
> +
> +out:
> + spin_unlock_irqrestore(&bus->lock, flags);
Please insert an empty line here to improve readability.
> + return ret;
> +}
> +
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> +{
> + bool irq_handled = true;
> + u32 command;
> + u32 irq_status;
> + u32 status_ack = 0;
> + u8 value;
> + struct i2c_client *slave = bus->slave;
See a comment above about order of declared local variables.
> +
> + spin_lock(&bus->lock);
> + if (!slave) {
> + irq_handled = false;
> + goto out;
> + }
Add an empty line here.
> + command = aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG);
> + irq_status = aspeed_i2c_read(bus, ASPEED_I2C_INTR_STS_REG);
> +
> + /* Slave was requested, restart state machine. */
> + if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
> + status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
> + bus->slave_state = ASPEED_I2C_SLAVE_START;
> + }
Add an empty line here.
> + /* Slave is not currently active, irq was for someone else. */
> + if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
> + irq_handled = false;
> + goto out;
> + }
> +
> + dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
> + irq_status, command);
> +
> + /* Slave was sent something. */
> + if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
> + value = aspeed_i2c_read(bus, ASPEED_I2C_BYTE_BUF_REG) >> 8;
> + /* Handle address frame. */
> + if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
> + if (value & 0x1)
> + bus->slave_state =
> + ASPEED_I2C_SLAVE_READ_REQUESTED;
> + else
> + bus->slave_state =
> + ASPEED_I2C_SLAVE_WRITE_REQUESTED;
> + }
> + status_ack |= ASPEED_I2CD_INTR_RX_DONE;
> + }
> +
> + /* Slave was asked to stop. */
> + if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> + status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
> + bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> + }
Add an empty line here.
> + if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> + status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> + bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> + }
> +
> + if (bus->slave_state == ASPEED_I2C_SLAVE_READ_REQUESTED) {
> + if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
> + dev_err(bus->dev, "Unexpected ACK on read request.\n");
> + bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
> +
> + i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
> + aspeed_i2c_write(bus, value, ASPEED_I2C_BYTE_BUF_REG);
> + aspeed_i2c_write(bus, ASPEED_I2CD_S_TX_CMD, ASPEED_I2C_CMD_REG);
> + } else if (bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
> + status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> + if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
> + dev_err(bus->dev,
> + "Expected ACK after processed read.\n");
> + i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
> + aspeed_i2c_write(bus, value, ASPEED_I2C_BYTE_BUF_REG);
> + aspeed_i2c_write(bus, ASPEED_I2CD_S_TX_CMD, ASPEED_I2C_CMD_REG);
> + } else if (bus->slave_state == ASPEED_I2C_SLAVE_WRITE_REQUESTED) {
> + bus->slave_state = ASPEED_I2C_SLAVE_WRITE_RECEIVED;
> + i2c_slave_event(slave, I2C_SLAVE_WRITE_REQUESTED, &value);
> + } else if (bus->slave_state == ASPEED_I2C_SLAVE_WRITE_RECEIVED) {
> + i2c_slave_event(slave, I2C_SLAVE_WRITE_RECEIVED, &value);
> + } else if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
> + i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
> + }
Please change this if-else-if-else-if-... construction of state machine
changes into switch-case.
> +
> + if (status_ack != irq_status)
> + dev_err(bus->dev,
> + "irq handled != irq. expected %x, but was %x\n",
> + irq_status, status_ack);
> + aspeed_i2c_write(bus, status_ack, ASPEED_I2C_INTR_STS_REG);
> +
> +out:
> + spin_unlock(&bus->lock);
> + return irq_handled;
> +}
> +#endif
> +
> +static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> +{
> + const u32 errs = ASPEED_I2CD_INTR_ARBIT_LOSS |
> + ASPEED_I2CD_INTR_ABNORMAL |
> + ASPEED_I2CD_INTR_SCL_TIMEOUT |
> + ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |
> + ASPEED_I2CD_INTR_TX_NAK;
You don't need this local variable, it is used only once, please
declare a macro and put it somewhere close to ASPEED_I2CD_INTR_*
macro in the beginning of the file.
> + u32 irq_status;
> +
> + spin_lock(&bus->lock);
> + irq_status = aspeed_i2c_read(bus, ASPEED_I2C_INTR_STS_REG);
> + bus->cmd_err = irq_status & errs;
> +
> + dev_dbg(bus->dev, "master irq status 0x%08x\n", irq_status);
> +
> + /* No message to transfer. */
> + if (bus->cmd_err ||
> + (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) ||
> + (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE)) {
> + complete(&bus->cmd_complete);
> + goto out;
> + } else if (!bus->msg || bus->msg_pos >= bus->msg->len)
> + goto out;
> +
> + if ((bus->msg->flags & I2C_M_RD) &&
> + (irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
> + bus->msg->buf[bus->msg_pos++] = aspeed_i2c_read(
> + bus, ASPEED_I2C_BYTE_BUF_REG) >> 8;
> + if (bus->msg_pos + 1 < bus->msg->len)
> + aspeed_i2c_write(bus, ASPEED_I2CD_M_RX_CMD,
> + ASPEED_I2C_CMD_REG);
> + else if (bus->msg_pos < bus->msg->len)
> + aspeed_i2c_write(bus, ASPEED_I2CD_M_RX_CMD |
> + ASPEED_I2CD_M_S_RX_CMD_LAST,
> + ASPEED_I2C_CMD_REG);
> + } else if (!(bus->msg->flags & I2C_M_RD) &&
> + (irq_status & ASPEED_I2CD_INTR_TX_ACK)) {
> + aspeed_i2c_write(bus, bus->msg->buf[bus->msg_pos++],
> + ASPEED_I2C_BYTE_BUF_REG);
> + aspeed_i2c_write(bus, ASPEED_I2CD_M_TX_CMD, ASPEED_I2C_CMD_REG);
> + }
> +
> + /* Transmission complete: notify caller. */
> + if (bus->msg_pos >= bus->msg->len)
> + complete(&bus->cmd_complete);
> +out:
> + aspeed_i2c_write(bus, irq_status, ASPEED_I2C_INTR_STS_REG);
> + spin_unlock(&bus->lock);
Please insert an empty line here to improve readability.
> + return true;
> +}
> +
> +static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> +{
> + struct aspeed_i2c_bus *bus = dev_id;
> +
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> + if (aspeed_i2c_slave_irq(bus)) {
> + dev_dbg(bus->dev, "irq handled by slave.\n");
> + return IRQ_HANDLED;
> + }
> +#endif
Please insert an empty line here to improve readability.
> + if (aspeed_i2c_master_irq(bus)) {
> + dev_dbg(bus->dev, "irq handled by master.\n");
> + return IRQ_HANDLED;
> + }
Please insert an empty line here to improve readability.
> + dev_err(bus->dev, "irq not handled properly!\n");
> + return IRQ_HANDLED;
If interrupt is not handled, return IRQ_NONE.
> +}
> +
> +static int aspeed_i2c_master_single_xfer(struct i2c_adapter *adap,
> + struct i2c_msg *msg)
> +{
> + struct aspeed_i2c_bus *bus = adap->algo_data;
> + unsigned long flags;
> + u8 slave_addr;
> + u32 command = ASPEED_I2CD_M_START_CMD | ASPEED_I2CD_M_TX_CMD;
> + int ret = msg->len;
> + unsigned long time_left;
> +
> + spin_lock_irqsave(&bus->lock, flags);
> + bus->msg = msg;
> + bus->msg_pos = 0;
> + slave_addr = msg->addr << 1;
Please insert an empty line here to improve readability.
> + if (msg->flags & I2C_M_RD) {
> + slave_addr |= 1;
> + command |= ASPEED_I2CD_M_RX_CMD;
> + if (msg->len == 1)
> + command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
> + }
Please insert an empty line here to improve readability.
> + aspeed_i2c_write(bus, slave_addr, ASPEED_I2C_BYTE_BUF_REG);
> + aspeed_i2c_write(bus, command, ASPEED_I2C_CMD_REG);
> + reinit_completion(&bus->cmd_complete);
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> + time_left = wait_for_completion_interruptible_timeout(
> + &bus->cmd_complete, bus->adap.timeout * HZ * msg->len);
> + if (time_left == 0)
> + return -ETIMEDOUT;
> +
> + spin_lock_irqsave(&bus->lock, flags);
> + if (bus->cmd_err)
> + ret = -EIO;
Please insert an empty line here to improve readability.
> + bus->msg = NULL;
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> + return ret;
> +}
> +
> +static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
> + struct i2c_msg *msgs, int num)
> +{
> + struct aspeed_i2c_bus *bus = adap->algo_data;
> + int ret;
> + int i;
> + unsigned long flags;
> + unsigned long time_left;
> +
> + /* If bus is busy, attempt recovery. We assume a single master
> + * environment.
> + */
> + if (aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG) &
> + ASPEED_I2CD_BUS_BUSY_STS) {
> + ret = aspeed_i2c_recover_bus(bus);
> + if (ret)
> + return ret;
> + }
> +
> + for (i = 0; i < num; i++) {
> + ret = aspeed_i2c_master_single_xfer(adap, &msgs[i]);
> + if (ret < 0)
> + break;
> + /* TODO: Support other forms of I2C protocol mangling. */
> + if (msgs[i].flags & I2C_M_STOP) {
> + spin_lock_irqsave(&bus->lock, flags);
> + aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD,
> + ASPEED_I2C_CMD_REG);
> + reinit_completion(&bus->cmd_complete);
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> + time_left = wait_for_completion_interruptible_timeout(
> + &bus->cmd_complete,
> + bus->adap.timeout * HZ);
> + if (time_left == 0)
> + return -ETIMEDOUT;
> + }
> + }
> +
> + spin_lock_irqsave(&bus->lock, flags);
> + aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD, ASPEED_I2C_CMD_REG);
> + reinit_completion(&bus->cmd_complete);
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> + time_left = wait_for_completion_interruptible_timeout(
> + &bus->cmd_complete, bus->adap.timeout * HZ);
> + if (time_left == 0)
> + return -ETIMEDOUT;
> +
> + /* If nothing went wrong, return number of messages transferred. */
> + if (ret < 0)
> + return ret;
> + else
> + return i;
> +}
> +
> +static u32 aspeed_i2c_functionality(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
> +}
> +
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static int aspeed_i2c_reg_slave(struct i2c_client *client)
> +{
> + struct aspeed_i2c_bus *bus;
> + unsigned long flags;
> + u32 addr_reg_val;
> + u32 func_ctrl_reg_val;
> +
> + bus = client->adapter->algo_data;
> + spin_lock_irqsave(&bus->lock, flags);
> + if (bus->slave) {
> + spin_unlock_irqrestore(&bus->lock, flags);
> + return -EINVAL;
> + }
> +
> + /* Set slave addr. */
> + addr_reg_val = aspeed_i2c_read(bus, ASPEED_I2C_DEV_ADDR_REG);
> + addr_reg_val &= ~ASPEED_I2CD_DEV_ADDR_MASK;
> + addr_reg_val |= client->addr & ASPEED_I2CD_DEV_ADDR_MASK;
> + aspeed_i2c_write(bus, addr_reg_val, ASPEED_I2C_DEV_ADDR_REG);
> +
> + /* Switch from master mode to slave mode. */
> + func_ctrl_reg_val = aspeed_i2c_read(bus, ASPEED_I2C_FUN_CTRL_REG);
> + func_ctrl_reg_val &= ~ASPEED_I2CD_MASTER_EN;
> + func_ctrl_reg_val |= ASPEED_I2CD_SLAVE_EN;
> + aspeed_i2c_write(bus, func_ctrl_reg_val, ASPEED_I2C_FUN_CTRL_REG);
> +
> + bus->slave = client;
> + bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> + spin_unlock_irqrestore(&bus->lock, flags);
Please insert an empty line here to improve readability.
> + return 0;
> +}
> +
> +static int aspeed_i2c_unreg_slave(struct i2c_client *client)
> +{
> + struct aspeed_i2c_bus *bus = client->adapter->algo_data;
> + unsigned long flags;
> + u32 func_ctrl_reg_val;
> +
> + spin_lock_irqsave(&bus->lock, flags);
> + if (!bus->slave) {
> + spin_unlock_irqrestore(&bus->lock, flags);
> + return -EINVAL;
> + }
> +
> + /* Switch from slave mode to master mode. */
> + func_ctrl_reg_val = aspeed_i2c_read(bus, ASPEED_I2C_FUN_CTRL_REG);
> + func_ctrl_reg_val &= ~ASPEED_I2CD_SLAVE_EN;
> + func_ctrl_reg_val |= ASPEED_I2CD_MASTER_EN;
> + aspeed_i2c_write(bus, func_ctrl_reg_val, ASPEED_I2C_FUN_CTRL_REG);
> +
> + bus->slave = NULL;
> + spin_unlock_irqrestore(&bus->lock, flags);
Please insert an empty line here to improve readability.
> + return 0;
> +}
> +#endif
> +
> +static const struct i2c_algorithm aspeed_i2c_algo = {
> + .master_xfer = aspeed_i2c_master_xfer,
> + .functionality = aspeed_i2c_functionality,
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> + .reg_slave = aspeed_i2c_reg_slave,
> + .unreg_slave = aspeed_i2c_unreg_slave,
> +#endif
> +};
> +
> +static u32 aspeed_i2c_get_clk_reg_val(u32 divider_ratio)
> +{
> + unsigned int inc = 0, div;
> + u32 scl_low, scl_high, data;
> +
> + for (div = 0; divider_ratio >= 16; div++) {
> + inc |= (divider_ratio & 1);
> + divider_ratio >>= 1;
> + }
Please insert an empty line here to improve readability.
> + divider_ratio += inc;
> + scl_low = (divider_ratio >> 1) - 1;
> + scl_high = divider_ratio - scl_low - 2;
> + data = 0x77700300 | (scl_high << 16) | (scl_low << 12) | div;
> + return data;
> +}
> +
> +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
> + struct platform_device *pdev)
> +{
> + struct clk *pclk;
> + u32 clk_freq;
> + u32 divider_ratio;
> + int ret;
> +
> + pclk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(pclk)) {
> + dev_err(&pdev->dev, "clk_get failed\n");
> + return PTR_ERR(pclk);
> + }
> + ret = of_property_read_u32(pdev->dev.of_node,
> + "clock-frequency", &clk_freq);
> + if (ret < 0) {
> + dev_err(&pdev->dev,
> + "Could not read clock-frequency property\n");
> + clk_freq = 100000;
> + }
> + divider_ratio = clk_get_rate(pclk) / clk_freq;
> + /* We just need the clock rate, we don't actually use the clk object. */
> + devm_clk_put(&pdev->dev, pclk);
Does the controller have a clock supply? If yes, shall the clock be
enabled before issuing command to the controller?
> +
> + /* Set AC Timing */
> + if (clk_freq / 1000 > 400) {
> + aspeed_i2c_write(bus, aspeed_i2c_read(bus,
> + ASPEED_I2C_FUN_CTRL_REG) |
> + ASPEED_I2CD_M_HIGH_SPEED_EN |
> + ASPEED_I2CD_M_SDA_DRIVE_1T_EN |
> + ASPEED_I2CD_SDA_DRIVE_1T_EN,
> + ASPEED_I2C_FUN_CTRL_REG);
> +
> + aspeed_i2c_write(bus, 0x3, ASPEED_I2C_AC_TIMING_REG2);
> + aspeed_i2c_write(bus, aspeed_i2c_get_clk_reg_val(divider_ratio),
> + ASPEED_I2C_AC_TIMING_REG1);
> + } else {
> + aspeed_i2c_write(bus, aspeed_i2c_get_clk_reg_val(divider_ratio),
> + ASPEED_I2C_AC_TIMING_REG1);
> + aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
> + ASPEED_I2C_AC_TIMING_REG2);
> + }
> +
> + return 0;
> +}
> +
> +static void noop(struct irq_data *data) { }
> +
> +static struct irq_chip aspeed_i2c_irqchip = {
> + .name = "ast-i2c",
> + .irq_unmask = noop,
> + .irq_mask = noop,
Thomas or Marc should review the irqchip code for correctness,
inform them.
> +};
> +
> +static int aspeed_i2c_probe_bus(struct platform_device *pdev)
> +{
> + struct aspeed_i2c_bus *bus;
> + struct aspeed_i2c_controller *controller =
> + dev_get_drvdata(pdev->dev.parent);
How do you ensure that "controller" device _driver_ is initialized
at this point? This is a critical race condition.
> + struct resource *res;
> + int ret, irq;
> + u32 hwirq;
> +
> + bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> + if (!bus)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + bus->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(bus->base))
> + return PTR_ERR(bus->base);
> +
> + bus->irq = platform_get_irq(pdev, 0);
> + if (bus->irq < 0)
> + return -ENXIO;
Remove check for error here, on error it will be returned by the
following devm_request_irq().
> + ret = of_property_read_u32(pdev->dev.of_node, "interrupts", &hwirq);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "no I2C 'interrupts' property\n");
> + return -ENXIO;
return -EINVAL;
> + }
> + irq = irq_create_mapping(controller->irq_domain, hwirq);
> + irq_set_chip_data(irq, controller);
> + irq_set_chip_and_handler(irq, &aspeed_i2c_irqchip, handle_simple_irq);
> + ret = devm_request_irq(&pdev->dev, bus->irq, aspeed_i2c_bus_irq,
> + 0, dev_name(&pdev->dev), bus);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to request interrupt\n");
> + return -ENXIO;
return ret;
In general I'm very surprised to see irqchip declaration and
initialization as a part of an I2C bus driver.
What is it used for? If you keep it, most probably IRQCHIP maintainers
(Thomas, Marc) shall review and ack the code.
> + }
> +
> + /* Initialize the I2C adapter */
> + spin_lock_init(&bus->lock);
> + init_completion(&bus->cmd_complete);
> + bus->adap.owner = THIS_MODULE;
> + bus->adap.retries = 0;
> + bus->adap.timeout = 5;
I don't think that this is a correct value, because here timeout unit
is in jiffies. Please update it here and everywhere in the driver,
where you use the value.
> + bus->adap.algo = &aspeed_i2c_algo;
> + bus->adap.algo_data = bus;
> + bus->adap.dev.parent = &pdev->dev;
> + bus->adap.dev.of_node = pdev->dev.of_node;
> + snprintf(bus->adap.name, sizeof(bus->adap.name), "Aspeed i2c");
> +
> + bus->dev = &pdev->dev;
> +
> + /* reset device: disable master & slave functions */
> + aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
> +
> + ret = aspeed_i2c_init_clk(bus, pdev);
> + if (ret < 0)
> + return ret;
I'm not sure here, probably you start leaking resources allocated by
irq_create_mapping().
> +
> + /* Enable Master Mode */
> + aspeed_i2c_write(bus, aspeed_i2c_read(bus, ASPEED_I2C_FUN_CTRL_REG) |
> + ASPEED_I2CD_MASTER_EN |
> + ASPEED_I2CD_MULTI_MASTER_DIS, ASPEED_I2C_FUN_CTRL_REG);
> +
> + /* Set interrupt generation of I2C controller */
> + aspeed_i2c_write(bus, ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |
> + ASPEED_I2CD_INTR_BUS_RECOVER_DONE |
> + ASPEED_I2CD_INTR_SCL_TIMEOUT |
> + ASPEED_I2CD_INTR_ABNORMAL |
> + ASPEED_I2CD_INTR_NORMAL_STOP |
> + ASPEED_I2CD_INTR_ARBIT_LOSS |
> + ASPEED_I2CD_INTR_RX_DONE |
> + ASPEED_I2CD_INTR_TX_NAK |
> + ASPEED_I2CD_INTR_TX_ACK,
Please declare a macro which combines all enabled interrupt events, and
put it somewhere close to ASPEED_I2CD_INTR_* macro definitions.
> + ASPEED_I2C_INTR_CTRL_REG);
> +
> + ret = i2c_add_adapter(&bus->adap);
> + if (ret < 0)
> + return -ENXIO;
Incorrect, you should propogate the error and "return ret;" here.
> +
> + platform_set_drvdata(pdev, bus);
> +
> + dev_info(bus->dev, "i2c bus %d registered, irq %d\n",
> + bus->adap.nr, bus->irq);
> +
> + return 0;
> +}
> +
> +static int aspeed_i2c_remove_bus(struct platform_device *pdev)
> +{
> + struct aspeed_i2c_bus *bus = platform_get_drvdata(pdev);
> +
> + i2c_del_adapter(&bus->adap);
Please insert an empty line here to improve readability.
> + return 0;
> +}
> +
> +static const struct of_device_id aspeed_i2c_bus_of_table[] = {
> + { .compatible = "aspeed,ast2400-i2c-bus", },
> + { .compatible = "aspeed,ast2500-i2c-bus", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table);
> +
> +static struct platform_driver aspeed_i2c_bus_driver = {
> + .probe = aspeed_i2c_probe_bus,
> + .remove = aspeed_i2c_remove_bus,
> + .driver = {
> + .name = "ast-i2c-bus",
> + .of_match_table = aspeed_i2c_bus_of_table,
> + },
> +};
> +
> +static void aspeed_i2c_controller_irq(struct irq_desc *desc)
> +{
> + struct aspeed_i2c_controller *c = irq_desc_get_handler_data(desc);
> + unsigned long p, status;
> + unsigned int bus_irq;
> +
> + status = readl(c->base);
> + for_each_set_bit(p, &status, ASPEED_I2C_NUM_BUS) {
> + bus_irq = irq_find_mapping(c->irq_domain, p);
> + generic_handle_irq(bus_irq);
> + }
This is a processing of a cascaded interrupt, you should wrap
the code into chained_irq_enter()/chained_irq_exit().
> +}
> +
> +static int aspeed_i2c_probe_controller(struct platform_device *pdev)
> +{
> + struct aspeed_i2c_controller *controller;
> + struct device_node *np;
> + struct resource *res;
> +
> + controller = kzalloc(sizeof(*controller), GFP_KERNEL);
> + if (!controller)
> + return -ENOMEM;
You don't free the allocated memory on driver release, please convert
allocation to devm_kzalloc().
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + controller->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(controller->base))
> + return PTR_ERR(controller->base);
> +
> + controller->irq = platform_get_irq(pdev, 0);
> + if (controller->irq < 0)
> + return -ENXIO;
> +
> + controller->irq_domain = irq_domain_add_linear(pdev->dev.of_node,
> + ASPEED_I2C_NUM_BUS, &irq_domain_simple_ops, NULL);
> + if (!controller->irq_domain)
> + return -ENXIO;
return -ENOMEM is the correct error here.
Please insert an empty line here to improve readability.
> + controller->irq_domain->name = "ast-i2c-domain";
> +
> + irq_set_chained_handler_and_data(controller->irq,
> + aspeed_i2c_controller_irq, controller);
> +
> + controller->dev = &pdev->dev;
> +
> + platform_set_drvdata(pdev, controller);
> +
> + dev_info(controller->dev, "i2c controller registered, irq %d\n",
> + controller->irq);
> +
> + for_each_child_of_node(pdev->dev.of_node, np) {
> + of_platform_device_create(np, NULL, &pdev->dev);
> + of_node_put(np);
This is invalid use of of_node_put(np) inside for_each_child_of_node().
Just remove it to make the statement valid.
> + }
> +
> + return 0;
> +}
> +
> +static int aspeed_i2c_remove_controller(struct platform_device *pdev)
> +{
> + struct aspeed_i2c_controller *controller = platform_get_drvdata(pdev);
> +
> + irq_domain_remove(controller->irq_domain);
Please insert an empty line here to improve readability.
> + return 0;
> +}
> +
> +static const struct of_device_id aspeed_i2c_controller_of_table[] = {
> + { .compatible = "aspeed,ast2400-i2c-controller", },
> + { .compatible = "aspeed,ast2500-i2c-controller", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_i2c_controller_of_table);
> +
> +static struct platform_driver aspeed_i2c_controller_driver = {
> + .probe = aspeed_i2c_probe_controller,
> + .remove = aspeed_i2c_remove_controller,
> + .driver = {
> + .name = "ast-i2c-controller",
> + .of_match_table = aspeed_i2c_controller_of_table,
> + },
> +};
> +
> +static int __init aspeed_i2c_driver_init(void)
> +{
> + int ret;
> +
> + ret = platform_driver_register(&aspeed_i2c_controller_driver);
> + if (ret < 0)
> + return ret;
Please insert an empty line here to improve readability.
> + return platform_driver_register(&aspeed_i2c_bus_driver);
No, on error you must unregister &aspeed_i2c_controller_driver.
> +}
> +module_init(aspeed_i2c_driver_init);
> +
> +static void __exit aspeed_i2c_driver_exit(void)
> +{
> + platform_driver_unregister(&aspeed_i2c_bus_driver);
> + platform_driver_unregister(&aspeed_i2c_controller_driver);
> +}
> +module_exit(aspeed_i2c_driver_exit);
> +
> +MODULE_AUTHOR("Brendan Higgins <brendanhiggins@google.com>");
> +MODULE_DESCRIPTION("Aspeed I2C Bus Driver");
> +MODULE_LICENSE("GPL");
>
^ permalink raw reply
* Re: [PATCH] of: Fix issue where code would fall through to error case.
From: Frank Rowand @ 2016-11-26 21:39 UTC (permalink / raw)
To: Rob Herring, Moritz Fischer
Cc: Moritz Fischer,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Pantelis Antoniou, moritz-62aBmqa6xEOcmJEhUYGoYg,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CAL_JsqJK=Jf7JCmqD8EgEg3ngONWz=1Fu-Jj5h6-wkskTk_iXw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 11/23/16 13:58, Rob Herring wrote:
> On Thu, Nov 17, 2016 at 6:10 PM, Moritz Fischer
> <moritz.fischer.private-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Thu, Nov 17, 2016 at 4:02 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On 11/17/16 15:40, Frank Rowand wrote:
>>>> On 11/17/16 15:25, Moritz Fischer wrote:
>>>>> No longer fall through into the error case that prints out
>>>>> an error if no error (err = 0) occurred.
>>>>>
>>>>> Fixes d9181b20a83(of: Add back an error message, restructured)
>>>>> Signed-off-by: Moritz Fischer <moritz.fischer-+aYTwkv1SeIAvxtiuMwx3w@public.gmane.org>
>>>>> ---
>>>>> drivers/of/resolver.c | 6 +++++-
>>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>>>>> index 783bd09..785076d 100644
>>>>> --- a/drivers/of/resolver.c
>>>>> +++ b/drivers/of/resolver.c
>>>>> @@ -358,9 +358,13 @@ int of_resolve_phandles(struct device_node *overlay)
>>>>>
>>>>> err = update_usages_of_a_phandle_reference(overlay, prop, phandle);
>>>>> if (err)
>>>>> - break;
>>>>> + goto err_out;
>>>>> }
>>>>>
>>>>> + of_node_put(tree_symbols);
>>>>> +
>>>>> + return 0;
>>>>> +
>>>>> err_out:
>>>>> pr_err("overlay phandle fixup failed: %d\n", err);
>>>>> out:
>>>>
>>>> Thanks for catching that.
>>>>
>>>> Rob, please apply.
>>>>
>>>> Reviewed-by: Frank Rowand <frank.rowand-mEdOJwZ7QcZBDgjK7y7TUQ@public.gmane.org>
>>>>
>>>> -Frank
>>>
>>> On second thought, isn't the common pattern when clean up is needed for
>>> both the no-error path and the error path something like:
>>>
>>>
>>> out:
>>> of_node_put(tree_symbols);
>>> return err;
>>>
>>> err_out:
>>> pr_err("overlay phandle fixup failed: %d\n", err);
>>> goto out;
>>> }
>>>
>>>
>>> I don't have a strong opinion, whatever Rob wants to take is fine with me.
>>
>> Same here. I tried to avoid the jumping back part, but if that's the
>> common pattern,
>> I can submit a v2 doing that instead.
>
> Both are ugly. Just do:
>
> if (err)
> pr_err(...);
>
> Rob
Agreed. Thanks for the touch of sanity Rob.
-Frank
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 4/5] regulator: Add support for TI TWL6032
From: Rosia, Nicolae @ 2016-11-26 20:24 UTC (permalink / raw)
To: lkp@intel.com
Cc: linux-kernel@vger.kernel.org, gg@slimlogic.co.uk,
robh+dt@kernel.org, devicetree@vger.kernel.org,
lee.jones@linaro.org, linux-omap@vger.kernel.org,
broonie@kernel.org, baruch@tkos.co.il, mark.rutland@arm.com,
lgirdwood@gmail.com, linux-arm-kernel@lists.infradead.org,
paul.gortmaker@windriver.com, tony@atomide.com
In-Reply-To: <201611270236.UFgW5zDA%fengguang.wu@intel.com>
Hi,
On Sun, 2016-11-27 at 02:55 +0800, kbuild test robot wrote:
> Hi Nicolae,
>
> [auto build test ERROR on omap/for-next]
> [also build test ERROR on v4.9-rc6]
> [cannot apply to next-20161125]
> [if your patch is applied to the wrong git tree, please drop us a
> note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Nicolae-Rosia/mfd-tw
> l-improvements-and-new-regulator-driver/20161127-022201
> base: https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-
> omap.git for-next
> config: i386-allmodconfig (attached as .config)
> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386
>
> All error/warnings (new ones prefixed by >>):
>
> In file included from drivers/regulator/twl6032-regulator.c:11:0:
> > > drivers/regulator/twl6032-regulator.c:557:31: error:
> > > 'twl6032_regulator_driver_ids' undeclared here (not in a
> > > function)
>
> MODULE_DEVICE_TABLE(platform, twl6032_regulator_driver_ids);
> ^
> include/linux/module.h:213:21: note: in definition of macro
> 'MODULE_DEVICE_TABLE'
> extern const typeof(name)
> __mod_##type##__##name##_device_table \
> ^~~~
> > > include/linux/module.h:213:27: error:
> > > '__mod_platform__twl6032_regulator_driver_ids_device_table'
> > > aliased to undefined symbol 'twl6032_regulator_driver_ids'
>
> extern const typeof(name)
> __mod_##type##__##name##_device_table \
> ^
> > > drivers/regulator/twl6032-regulator.c:557:1: note: in expansion
> > > of macro 'MODULE_DEVICE_TABLE'
>
> MODULE_DEVICE_TABLE(platform, twl6032_regulator_driver_ids);
> ^~~~~~~~~~~~~~~~~~~
>
> vim +/twl6032_regulator_driver_ids +557 drivers/regulator/twl6032-
> regulator.c
>
> 551
> 552 static const struct of_device_id twl6032_dt_match[] = {
> 553 { .compatible = "ti,twl6032-regulator" },
> 554 { /* last entry */ }
> 555 };
> 556
> > 557 MODULE_DEVICE_TABLE(platform,
> twl6032_regulator_driver_ids);
> 558
> 559 static struct platform_driver twl6032_regulator_driver
> = {
> 560 .driver = {
Thanks, I did not notice this since I was only testing using built-in
module.
I will wait for comments before sending V2, untill then here's an
inline patch with the fix.
Best regards,
Nicolae
^ permalink raw reply
* Re: [PATCH 4/5] regulator: Add support for TI TWL6032
From: kbuild test robot @ 2016-11-26 18:55 UTC (permalink / raw)
Cc: kbuild-all-JC7UmRfGjtg, Lee Jones, Mark Brown, Rob Herring,
Mark Rutland, Tony Lindgren, Liam Girdwood, Paul Gortmaker,
Graeme Gregory, Baruch Siach, linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Nicolae Rosia
In-Reply-To: <20161126181326.14951-5-Nicolae_Rosia-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2168 bytes --]
Hi Nicolae,
[auto build test ERROR on omap/for-next]
[also build test ERROR on v4.9-rc6]
[cannot apply to next-20161125]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Nicolae-Rosia/mfd-twl-improvements-and-new-regulator-driver/20161127-022201
base: https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git for-next
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All error/warnings (new ones prefixed by >>):
In file included from drivers/regulator/twl6032-regulator.c:11:0:
>> drivers/regulator/twl6032-regulator.c:557:31: error: 'twl6032_regulator_driver_ids' undeclared here (not in a function)
MODULE_DEVICE_TABLE(platform, twl6032_regulator_driver_ids);
^
include/linux/module.h:213:21: note: in definition of macro 'MODULE_DEVICE_TABLE'
extern const typeof(name) __mod_##type##__##name##_device_table \
^~~~
>> include/linux/module.h:213:27: error: '__mod_platform__twl6032_regulator_driver_ids_device_table' aliased to undefined symbol 'twl6032_regulator_driver_ids'
extern const typeof(name) __mod_##type##__##name##_device_table \
^
>> drivers/regulator/twl6032-regulator.c:557:1: note: in expansion of macro 'MODULE_DEVICE_TABLE'
MODULE_DEVICE_TABLE(platform, twl6032_regulator_driver_ids);
^~~~~~~~~~~~~~~~~~~
vim +/twl6032_regulator_driver_ids +557 drivers/regulator/twl6032-regulator.c
551
552 static const struct of_device_id twl6032_dt_match[] = {
553 { .compatible = "ti,twl6032-regulator" },
554 { /* last entry */ }
555 };
556
> 557 MODULE_DEVICE_TABLE(platform, twl6032_regulator_driver_ids);
558
559 static struct platform_driver twl6032_regulator_driver = {
560 .driver = {
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 56830 bytes --]
^ permalink raw reply
* Re: [PATCH v4 0/7] mux controller abstraction and iio/i2c muxes
From: Peter Rosin @ 2016-11-26 18:38 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Jonathan Cameron,
Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Jonathan Corbet, Arnd Bergmann, Greg Kroah-Hartman,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1480000687-5630-1-git-send-email-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
On 2016-11-24 16:18, Peter Rosin wrote:
> v3 -> v4 changes
> - added support for having the mux-controller in a child node of a
> mux-consumer if it is a sole consumer, to hopefully even further satisfy
> the complaint from Rob (and later Lars-Peter) about dt complexity.
> - the above came at the cost of some rather horrible refcounting code,
> please review and suggest how it should be done...
>
> v2 -> v3 changes
> - have the mux-controller in the parent node of any mux-controller consumer,
> to hopefully satisfy complaint from Rob about dt complexity.
I did some further tests and both of these attempts to support fancier
devicetree bindings have severe problems. I will remove them for v5 and
go back to having a phandle reference to the mux-controller from the
consumer (unless I get some revelation of course and just get it). I'm
simply not yet understanding the driver model well enough to pull this
off at the moment...
Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox