Re: [PATCH] gpio: New driver for GPO emulation using PWM generators
On 11/23/2012 10:44 AM, Peter Ujfalusi wrote:
> Hi Grant,
>
> On 11/23/2012 10:13 AM, Peter Ujfalusi wrote:
>> Hi Grant,
>>
>> On 11/23/2012 08:55 AM, Grant Likely wrote:
>>> Ugh. and this is why I wanted the PWM and GPIO subsystems to use the
>>> same namespace and binding. But that's not your fault.
>>>
>>> It's pretty horrible to have a separate translator node to convert a PWM
>>> into a GPIO (with output only of course). The gpio properties should
>>> appear directly in the PWM node itself and the translation code should
>>> be in either the pwm or the gpio core. I don't think it should look like
>>> a separate device.
>>
>> Let me see if I understand your suggestion correctly. In the DT you suggest
>> something like this:
>>
>> twl_pwmled: pwmled {
>> compatible = "ti,twl4030-pwmled";
>> #pwm-cells = <2>;
>> #gpio-cells = <2>;
>> gpio-controller;
>> };
>
> After I thought about this.. Is this what we really want?
> After all what we have here is a PWM generator used to emulate a GPIO signal.
> The PWM itself can be used for driving a LED (standard LED or backlight and we
> have DT bindings for these already), vibra motor, or other things.
> If we combine the PWM with GPIO we should go and 'bloat' the DT node to also
> include all sort of other uses of PWM at once?
>
> IMHO it is better to keep them as separate things.
> pwm node to describe the PWM generator,
> separate nodes to describe it's uses like led, backlight, motor and gpio.
>
The difference here is that the LED, backlight, etc are all different
physical devices begin driven by the pwm pin, so it makes sense to have a
device tree node for them, while using the pwm as gpio is just a different
function of the same physical pin. So in a sense the pwm controller also
becomes a gpio controller. I like the idea of the pwm core automatically
instantiating a pwm-gpo device if it sees a gpio-controller property in the
pwm device devicetree node.
- Lars
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] gpio: New driver for GPO emulation using PWM generators
On 11/28/2012 09:54 AM, Peter Ujfalusi wrote:
> Hi Grant, Lars, Thierry,
>
> On 11/26/2012 04:46 PM, Grant Likely wrote:
>> You're effectively asking the pwm layer to behave like a gpio (which
>> is completely reasonable). Having a completely separate translation node
>> really doesn't make sense because it is entirely a software construct.
>> In fact, the way your using it is *entirely* to make the Linux driver
>> model instantiate the translation code. It has *nothing* to do with the
>> structure of the hardware. It makes complete sense that if a PWM is
>> going to be used as a GPIO, then the PWM node should conform to the GPIO
>> binding.
>
> I understand your point around this. I might say I agree with it as well...
> I spent yesterday with prototyping and I'm not really convinced that it is a
> good approach from C code point of view. I got it working, yes.
> In essence this is what I have on top of the slightly modified gpio-pwm.c
> driver I have submitted:
>
> DTS files:
> twl_pwm: pwm {
> /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
> compatible = "ti,twl6030-pwm";
> #pwm-cells = <2>;
>
> /* Enable GPIO us of the PWMs */
> gpio-controller = <1>;
> #gpio-cells = <2>;
> pwm,period_ns = <7812500>;
> };
>
> leds {
> compatible = "gpio-leds";
> backlight {
> label = "omap4::backlight";
> gpios = <&twl_pwm 1 0>; /* PWM1 of twl6030 */
> };
>
> keypad {
> label = "omap4::keypad";
> gpios = <&twl_pwm 0 0>; /* PWM0 of twl6030 */
> };
> };
>
> The bulk of the code in drivers/pwm/core.c to create the pwm-gpo device when
> it is requested going to look something like this. I have removed the error
> checks for now and I still don't have the code to clean up the allocated
> memory for the created device on error, or in case the module is unloaded. We
> should also prevent the pwm core from removal when the pwm-gpo driver is
> loaded.
> We need to create the platform device for gpo-pwm, create the pdata structure
> for it and fill it in. We also need to hand craft the pwm_lookup table so we
> can use pwm_get() to request the PWM. I have other minor changes around this
> to get things working when we booted with DT.
> So the function to do the heavy lifting is something like this:
> static void of_pwmchip_as_gpio(struct pwm_chip *chip)
> {
> struct platform_device *pdev;
> struct gpio_pwm *gpos;
> struct gpio_pwm_pdata *pdata;
> struct pwm_lookup *lookup;
> char gpodev_name[15];
> int i;
> u32 gpio_mode = 0;
> u32 period_ns = 0;
>
> of_property_read_u32(chip->dev->of_node, "gpio-controller",
>&gpio_mode);
> if (!gpio_mode)
> return;
>
> of_property_read_u32(chip->dev->of_node, "pwm,period_ns", &period_ns);
> if (!period_ns) {
> dev_err(chip->dev,
> "period_ns is not specified for GPIO use\n");
> return;
> }
>
> lookup = devm_kzalloc(chip->dev, sizeof(*lookup) * chip->npwm,
> GFP_KERNEL);
> pdata = devm_kzalloc(chip->dev, sizeof(*pdata), GFP_KERNEL);
> gpos = devm_kzalloc(chip->dev, sizeof(*gpos) * chip->npwm,
> GFP_KERNEL);
>
> pdata->gpos = gpos;
> pdata->num_gpos = chip->npwm;
> pdata->gpio_base = -1;
>
> pdev = platform_device_alloc("pwm-gpo", chip->base);
> pdev->dev.parent = chip->dev;
>
> sprintf(gpodev_name, "pwm-gpo.%d", chip->base);
> for (i = 0; i < chip->npwm; i++) {
> struct gpio_pwm *gpo = &gpos[i];
> struct pwm_lookup *pl = &lookup[i];
> char con_id[15];
>
> sprintf(con_id, "pwm-gpo.%d", chip->base + i);
>
> /* prepare GPO information */
> gpo->pwm_period_ns = period_ns;
> gpo->name = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);;
>
> /* prepare pwm lookup table */
> pl->provider = dev_name(chip->dev);
> pl->index = i;
> pl->dev_id = kmemdup(gpodev_name, sizeof(gpodev_name),
>GFP_KERNEL);
> pl->con_id = kmemdup(con_id, sizeof(con_id), GFP_KERNEL);
> }
>
> platform_device_add_data(pdev, pdata, sizeof(*pdata));
> pwm_add_table(lookup, chip->npwm);
> platform_device_add(pdev);
> }
>
The whole pwm lookup table creation is a bit ugly. I wonder if we can somehow
bypass this by using pwm_request_from_chip directly in the pwm-gpo driver.
- Lars
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
[PATCH resend] spi: Add support for specifying 3-wire mode via device tree
This patch allows to specify that a SPI device is connected in 3-wire mode via device tree. Signed-off-by: Lars-Peter Clausen Acked-by: Rob Herring --- Documentation/devicetree/bindings/spi/spi-bus.txt | 2 ++ drivers/spi/spi.c | 2 ++ 2 files changed, 4 insertions(+) diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt index 77a8b0d..296015e 100644 --- a/Documentation/devicetree/bindings/spi/spi-bus.txt +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt @@ -53,6 +53,8 @@ contain the following properties. shifted clock phase (CPHA) mode - spi-cs-high - (optional) Empty property indicating device requires chip select active high +- spi-3wire - (optional) Empty property indicating device requires + 3-wire mode. If a gpio chipselect is used for the SPI slave the gpio number will be passed via the cs_gpio diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 22c71e2..cb61ada 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -861,6 +861,8 @@ static void of_register_spi_devices(struct spi_master *master) spi->mode |= SPI_CPOL; if (of_find_property(nc, "spi-cs-high", NULL)) spi->mode |= SPI_CS_HIGH; + if (of_find_property(nc, "spi-3wire", NULL)) + spi->mode |= SPI_3WIRE; /* Device speed */ prop = of_get_property(nc, "spi-max-frequency", &len); -- 1.8.0 ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 4/8] MFD: ti_am335x_tscadc: add device tree binding information
Hi,
On 01/18/2013 11:48 AM, Patil, Rachna wrote:
> From: "Patil, Rachna"
>
> Signed-off-by: Patil, Rachna
> ---
> .../devicetree/bindings/mfd/ti_am335x_tscadc.txt | 35
>
> 1 file changed, 35 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/ti_am335x_tscadc.txt
>
> diff --git a/Documentation/devicetree/bindings/mfd/ti_am335x_tscadc.txt
> b/Documentation/devicetree/bindings/mfd/ti_am335x_tscadc.txt
> new file mode 100644
> index 000..c13c492
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/ti_am335x_tscadc.txt
> @@ -0,0 +1,35 @@
> +Texas Instruments - TSC / ADC multi-functional device
> +
> +ti_tscadc is a multi-function device with touchscreen and ADC on chip.
> +This document describes the binding for mfd device.
> +
> +Required properties:
> +- compatible: "ti,ti-tscadc"
> +- reg: Specifies the address of MFD block
> +- interrupts: IRQ line connected to the main SoC
> +- interrupt-parent: The parent interrupt controller
The subnodes and their properties also need documentation.
> +
> +Optional properties:
> +- ti,hwmods: Hardware information related to TSC/ADC MFD device
> +
> +Example:
> +
> + tscadc: tscadc@44e0d000 {
> + compatible = "ti,ti-tscadc";
> + reg = <0x44e0d000 0x1000>;
> +
> + interrupt-parent = <&intc>;
> + interrupts = <16>;
> + ti,hwmods = "adc_tsc";
> +
> + tsc {
> + wires = <4>;
> + x-plate-resistance = <200>;
> + steps-to-configure = <5>;
> + wire-config = <0x00 0x11 0x22 0x33>;
Non-standard properties should have a vendor prefix.
> + };
> +
> + adc {
> + adc-channels = <4>;
> + };
> + };
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC 10/11] iio: Add OF support
013 10:43 PM, Guenter Roeck wrote:
> Provide bindings, new API access functions, and parse OF data
> during initialization.
>
Hi Guenter,
Thanks for taking care of this.
I'd prefer to have only one iio_get_channel which handles both the dt and the
non-dt case. Otherwise we'll soon have constructs like
if (dev->of_node)
chan = of_iio_get_channel(dev->of_node, 1);
else
chan = iio_get_channel(dev_name(dev), "vcc");
appearing all over the place in drivers code. I'd keep the actual
implementation pretty close to what the clk framework does. E.g. take a look at
of_clk_get_by_name() and clk_get() in clkdev.c. And don't take a detour via the
iio_map lookup for the devicetree case, just loop over all IIO devices and
match by of_node. This should allow us to simplify the code quite a bit.
- Lars
On 01/31/2
> Signed-off-by: Guenter Roeck
> ---
> .../devicetree/bindings/iio/iio-bindings.txt | 97
> drivers/iio/inkern.c | 241
>
> include/linux/iio/consumer.h |8 +
> 3 files changed, 299 insertions(+), 47 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/iio/iio-bindings.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt
> b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> new file mode 100644
> index 000..0f51c95
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> @@ -0,0 +1,97 @@
> +This binding is a work-in-progress, and are based on clock bindings and
> +suggestions from Lars-Peter Clausen [1].
> +
> +Sources of IIO channels can be represented by any node in the device
> +tree. Those nodes are designated as IIO providers. IIO consumer
> +nodes use a phandle and IIO specifier pair to connect IIO provider
> +outputs to IIO inputs. Similar to the gpio specifiers, an IIO
> +specifier is an array of one more more cells identifying the IIO
> +output on a device. The length of an IIO specifier is defined by the
> +value of a #io-channel-cells property in the clock provider node.
> +
> +[1] http://marc.info/?l=linux-iio&m=135902119507483&w=2
> +
> +==IIO providers==
> +
> +Required properties:
> +#io-channel-cells: Number of cells in an IIO specifier; Typically 0 for nodes
> +with a single IIO output and 1 for nodes with multiple
> +IIO outputs.
> +
> +Optional properties:
> +io-channel-output-names:
> + Recommended to be a list of strings of IIO output signal
> + names indexed by the first cell in the IIO specifier.
> + However, the meaning of io-channel-output-names is domain
> + specific to the IIO provider, and is only provided to
> + encourage using the same meaning for the majority of IIO
> + providers. This format may not work for IIO providers
> + using a complex IIO specifier format. In those cases it
> + is recommended to omit this property and create a binding
> + specific names property.
> +
> + IIO consumer nodes must never directly reference
> + the provider's io-channel-output-names property.
> +
> +For example:
> +
> +adc: adc@35 {
> + compatible = "maxim,max1139";
> + reg = <0x35>;
> +#io-channel-cells = <1>;
> +io-channel-output-names = "adc1", "adc2";
> +};
> +
> +- this node defines a device with two named IIO outputs, the first named
> + "adc1" and the second named "adc2". Consumer nodes always reference
> + IIO channels by index. The names should reflect the IIO output signal
> + names for the device.
> +
> +==IIO consumers==
> +
> +Required properties:
> +io-channels: List of phandle and IIO specifier pairs, one pair
> + for each IIO input to the device. Note: if the
> + IIO provider specifies '0' for #clock-cells, then
> + only the phandle portion of the pair will appear.
> +
> +Optional properties:
> +io-channel-names:
> + List of IIO input name strings sorted in the same
> + order as the io-channels property. Consumers drivers
> + will use io-channel-names to match IIO input names
> + with IIO specifiers.
> +io-channel-ranges:
> + Empty property indicating that child nodes can inherit named
> + IIO channels from this node. Useful for bus nodes to provide
> + and IIO channel to their children.
> +
> +For example:
> +
> +device {
> +
Re: [RFC 10/11] iio: Add OF support
On 02/01/2013 03:33 PM, Guenter Roeck wrote:
> On Fri, Feb 01, 2013 at 12:58:02PM +0100, Lars-Peter Clausen wrote:
>> 013 10:43 PM, Guenter Roeck wrote:
>>> Provide bindings, new API access functions, and parse OF data
>>> during initialization.
>>>
>>
>> Hi Guenter,
>>
>> Thanks for taking care of this.
>>
>> I'd prefer to have only one iio_get_channel which handles both the dt and the
>> non-dt case. Otherwise we'll soon have constructs like
>>
>> if (dev->of_node)
>> chan = of_iio_get_channel(dev->of_node, 1);
>> else
>> chan = iio_get_channel(dev_name(dev), "vcc");
>>
>> appearing all over the place in drivers code. I'd keep the actual
>> implementation pretty close to what the clk framework does. E.g. take a look
>> at
>> of_clk_get_by_name() and clk_get() in clkdev.c. And don't take a detour via
>> the
>> iio_map lookup for the devicetree case, just loop over all IIO devices and
>> match by of_node. This should allow us to simplify the code quite a bit.
>>
> Yes, I just could not figure out how to loop over the list of iio devices.
> That
> is why I hijacked iio_map which does provide a list.
>
> Any hints ?
Yes, use bus_find_device on iio_bus_type. A nice example how to use this to
lookup device by of node is of_find_i2c_device_by_node. For IIO you also need
to make sure that dev->type is iio_dev_type, since both devices and triggers
are registered on the same bus.
>
> Thanks,
> Guenter
>
>> - Lars
>>
>> On 01/31/2
>>
>>> Signed-off-by: Guenter Roeck
>>> ---
>>> .../devicetree/bindings/iio/iio-bindings.txt | 97
>>> drivers/iio/inkern.c | 241
>>>
>>> include/linux/iio/consumer.h |8 +
>>> 3 files changed, 299 insertions(+), 47 deletions(-)
>>> create mode 100644 Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>> b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>> new file mode 100644
>>> index 000..0f51c95
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>> @@ -0,0 +1,97 @@
>>> +This binding is a work-in-progress, and are based on clock bindings and
>>> +suggestions from Lars-Peter Clausen [1].
>>> +
>>> +Sources of IIO channels can be represented by any node in the device
>>> +tree. Those nodes are designated as IIO providers. IIO consumer
>>> +nodes use a phandle and IIO specifier pair to connect IIO provider
>>> +outputs to IIO inputs. Similar to the gpio specifiers, an IIO
>>> +specifier is an array of one more more cells identifying the IIO
>>> +output on a device. The length of an IIO specifier is defined by the
>>> +value of a #io-channel-cells property in the clock provider node.
>>> +
>>> +[1] http://marc.info/?l=linux-iio&m=135902119507483&w=2
>>> +
>>> +==IIO providers==
>>> +
>>> +Required properties:
>>> +#io-channel-cells: Number of cells in an IIO specifier; Typically 0 for
>>> nodes
>>> + with a single IIO output and 1 for nodes with multiple
>>> + IIO outputs.
>>> +
>>> +Optional properties:
>>> +io-channel-output-names:
>>> + Recommended to be a list of strings of IIO output signal
>>> + names indexed by the first cell in the IIO specifier.
>>> + However, the meaning of io-channel-output-names is domain
>>> + specific to the IIO provider, and is only provided to
>>> + encourage using the same meaning for the majority of IIO
>>> + providers. This format may not work for IIO providers
>>> + using a complex IIO specifier format. In those cases it
>>> + is recommended to omit this property and create a binding
>>> + specific names property.
>>> +
>>> + IIO consumer nodes must never directly reference
>>> + the provider's io-channel-output-names property.
>>> +
>>> +For example:
>>> +
>>> +adc: adc@35 {
>>> + compatible = "maxim,max1139";
>>> + reg = <0x35>;
>>> +#io-channel-cells = <1>;
>>> +io-channel-output-names
Re: [RFC 10/11] iio: Add OF support
On 02/01/2013 08:42 PM, Guenter Roeck wrote:
> On Fri, Feb 01, 2013 at 03:59:17PM +0100, Lars-Peter Clausen wrote:
>> On 02/01/2013 03:33 PM, Guenter Roeck wrote:
>>> On Fri, Feb 01, 2013 at 12:58:02PM +0100, Lars-Peter Clausen wrote:
>>>> 013 10:43 PM, Guenter Roeck wrote:
>>>>> Provide bindings, new API access functions, and parse OF data
>>>>> during initialization.
>>>>>
>>>>
>>>> Hi Guenter,
>>>>
>>>> Thanks for taking care of this.
>>>>
>>>> I'd prefer to have only one iio_get_channel which handles both the dt and
>>>> the
>>>> non-dt case. Otherwise we'll soon have constructs like
>>>>
>>>> if (dev->of_node)
>>>>chan = of_iio_get_channel(dev->of_node, 1);
>>>> else
>>>>chan = iio_get_channel(dev_name(dev), "vcc");
>>>>
>
> Clock code has of_clk_get_by_name(node *, name), clk_get(dev, name), and
> of_clk_get(node *, index). Right now of_iio_get_channel matches of_clk_get,
> and iio_get_channel matches clk_get.
>
> Question is really how we want to API to look like. I am open to suggestions.
I'm not necessarily against having a separate of_iio_get_channe function, but
I'm not sure if we really need it, maybe use it internally as a helper and if
we really need it in some driver make it public and export it. clk_get first
calls of_clk_get_by_name, and only if didn't find a clk it falls back to the
map based lookup. I think iio_get_channel should behave the similar. of_clk_get
is kind of just a helper function for of_clk_get_by_name, not sure if we need
it as a separate function in IIO. If we find out we need it we can probably
still add it later.
- Lars
>
>>>> appearing all over the place in drivers code. I'd keep the actual
>>>> implementation pretty close to what the clk framework does. E.g. take a
look at
>>>> of_clk_get_by_name() and clk_get() in clkdev.c. And don't take a detour
via the
>>>> iio_map lookup for the devicetree case, just loop over all IIO devices and
>>>> match by of_node. This should allow us to simplify the code quite a bit.
>>>>
>>> Yes, I just could not figure out how to loop over the list of iio devices.
>>> That
>>> is why I hijacked iio_map which does provide a list.
>>>
>>> Any hints ?
>>
>> Yes, use bus_find_device on iio_bus_type. A nice example how to use this to
>> lookup device by of node is of_find_i2c_device_by_node. For IIO you also need
>> to make sure that dev->type is iio_dev_type, since both devices and triggers
>> are registered on the same bus.
>>
> Great, that works nicely, and, yes, the code is now much simpler.
>
>>>>> +static struct iio_map *iio_map_of_init(struct iio_dev *idev)
>>>>> +{
>>>>
>>>> To be honest I find this whole function a bit confusing. If we are using
>>>> devicetree, we shouldn't need to register iio_map. Maybe just skip over
>>>> io-channel-output-names for now, until we figure out if we really need
this and
>>>> what for.
>>>>
> Correct, I don't need this anymore after implementing the above.
>
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v2 4/4] iio: Add OF support
On 02/03/2013 03:06 AM, Guenter Roeck wrote:
> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
>> Hi Guenter,
>>
>> Some comments inline.
>>
>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
>>> Provide bindings and parse OF data during initialization.
>>>
>>> Signed-off-by: Guenter Roeck
>>> ---
>>> - Documentation update per feedback
>>> - Dropped io-channel-output-names from the bindings document. The
>>> property is not used in the code, and it is not entirely clear what it
>>> would be used for. If there is a need for it, we can add it back in
>>> later on.
>>> - Don't export OF specific API calls
>>> - For OF support, no longer depend on iio_map
>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the code still
>>> builds if it is not selected.
>>> - Change iio_channel_get to take device pointer as argument instead of
>>> device name. Retain old API as of_iio_channel_get_sys.
>>> - iio_channel_get now works for both OF and non-OF configurations.
>>>
>>> .../devicetree/bindings/iio/iio-bindings.txt | 76
>>> drivers/iio/inkern.c | 186
>>> 2 files changed, 262 insertions(+)
>>> create mode 100644
>>> Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>> b/Documentation/devicetree/bindings/iio/iio-bindings.txt new file mode
>>> 100644
>>> index 000..58df5f6
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>> @@ -0,0 +1,76 @@
>>> +This binding is a work-in-progress. It is derived from clock bindings,
>>> +and based on suggestions from Lars-Peter Clausen [1].
>>> +
>>> +Sources of IIO channels can be represented by any node in the device
>>> +tree. Those nodes are designated as IIO providers. IIO consumer
>>> +nodes use a phandle and IIO specifier pair to connect IIO provider
>>> +outputs to IIO inputs. Similar to the gpio specifiers, an IIO
>>> +specifier is an array of one or more cells identifying the IIO
>>> +output on a device. The length of an IIO specifier is defined by the
>>> +value of a #io-channel-cells property in the clock provider node.
>>> +
>>> +[1] http://marc.info/?l=linux-iio&m=135902119507483&w=2
>>> +
>>> +==IIO providers==
>>> +
>>> +Required properties:
>>> +#io-channel-cells: Number of cells in an IIO specifier; Typically 0 for
>>> nodes +with a single IIO output and 1 for nodes with
>>> multiple
>>> + IIO outputs.
>>> +
>>> +For example:
>>> +
>>> +adc: adc@35 {
>>> + compatible = "maxim,max1139";
>>> + reg = <0x35>;
>>> +#io-channel-cells = <1>;
>>> +};
>>> +
>>> +==IIO consumers==
>>> +
>>> +Required properties:
>>> +io-channels: List of phandle and IIO specifier pairs, one pair
>>> + for each IIO input to the device. Note: if the
>>> + IIO provider specifies '0' for #clock-cells, then
>>> + only the phandle portion of the pair will appear.
>>> +
>>> +Optional properties:
>>> +io-channel-names:
>>> + List of IIO input name strings sorted in the same
>>> + order as the io-channels property. Consumers drivers
>>> + will use io-channel-names to match IIO input names
>>> + with IIO specifiers.
>>> +io-channel-ranges:
>>> + Empty property indicating that child nodes can inherit named
>>> + IIO channels from this node. Useful for bus nodes to provide
>>> + and IIO channel to their children.
>>> +
>>> +For example:
>>> +
>>> +device {
>>> +io-channels = <&adc 1>, <&ref 0>;
>>> +io-channel-names = "vcc", "vdd";
>>> +};
>>> +
>>> +This represents a device with two IIO inputs, named "vcc" and "vdd".
>>> +The vcc channel is connected to output 1 of the &adc device, and the
>>> +vdd channel is connected to output 0 of the &ref device.
>>> +
>>> +==Example==
>>> +
>>> + adc: max1139@35 {
>>> + compat
Re: [RFC 10/11] iio: Add OF support
On 02/03/2013 12:39 PM, Jonathan Cameron wrote:
> On 02/02/2013 04:10 PM, Guenter Roeck wrote:
>> On Sat, Feb 02, 2013 at 10:29:02AM +, Jonathan Cameron wrote:
>>> On 01/31/2013 09:43 PM, Guenter Roeck wrote:
>>>> Provide bindings, new API access functions, and parse OF data
>>>> during initialization.
>>>>
>>> Firstly thanks for working on this Guenter, it's been a big hole
>>> for a while largely because non of our largest developers were
>>> actually using development platforms with device tree support.
>>>
>>> Given my knowledge of device tree is based on the odd article
>>> and looking at similar sets of bindings this morning, my comments
>>> are likely to be somewhat superficial and uninformed ;)
>>>
>>> Mostly on this one I'll take a back seat and let those who
>>> know this stuff better come to a consensus.
>>>
>>> Jonathan
>>>
>>>> Signed-off-by: Guenter Roeck
>>>> ---
>>>> .../devicetree/bindings/iio/iio-bindings.txt | 97
>>>> drivers/iio/inkern.c | 241
>>>>
>>>> include/linux/iio/consumer.h |8 +
>>>> 3 files changed, 299 insertions(+), 47 deletions(-)
>>>> create mode 100644 Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>> b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>> new file mode 100644
>>>> index 000..0f51c95
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>> @@ -0,0 +1,97 @@
>>>> +This binding is a work-in-progress, and are based on clock bindings and
>>>> +suggestions from Lars-Peter Clausen [1].
>>>> +
>>>> +Sources of IIO channels can be represented by any node in the device
>>>> +tree. Those nodes are designated as IIO providers. IIO consumer
>>>> +nodes use a phandle and IIO specifier pair to connect IIO provider
>>>> +outputs to IIO inputs. Similar to the gpio specifiers, an IIO
>>>> +specifier is an array of one more more cells identifying the IIO
>>>> +output on a device. The length of an IIO specifier is defined by the
>>>> +value of a #io-channel-cells property in the clock provider node.
>>>> +
>>>> +[1] http://marc.info/?l=linux-iio&m=135902119507483&w=2
>>>> +
>>>> +==IIO providers==
>>>> +
>>>> +Required properties:
>>>> +#io-channel-cells: Number of cells in an IIO specifier; Typically 0 for
>>>> nodes
>>>> + with a single IIO output and 1 for nodes with multiple
>>>> + IIO outputs.
>>>> +
>>>> +Optional properties:
>>>> +io-channel-output-names:
>>>> + Recommended to be a list of strings of IIO output signal
>>>> + names indexed by the first cell in the IIO specifier.
>>>> + However, the meaning of io-channel-output-names is domain
>>>> + specific to the IIO provider, and is only provided to
>>>> + encourage using the same meaning for the majority of IIO
>>>> + providers. This format may not work for IIO providers
>>>> + using a complex IIO specifier format. In those cases it
>>>> + is recommended to omit this property and create a binding
>>>> + specific names property.
>>>> +
>>>> + IIO consumer nodes must never directly reference
>>>> + the provider's io-channel-output-names property.
>>>> +
>>>> +For example:
>>>> +
>>>> +adc: adc@35 {
>>>> + compatible = "maxim,max1139";
>>>> + reg = <0x35>;
>>>> +#io-channel-cells = <1>;
>>>> +io-channel-output-names = "adc1", "adc2";
>>>> +};
>>>> +
>>>> +- this node defines a device with two named IIO outputs, the first named
>>>> + "adc1" and the second named "adc2". Consumer nodes always reference
>>>> + IIO channels by index. The names should reflect the IIO output signal
>>>> + names for the device.
>>>> +
>>>> +==IIO consumers==
>&g
Re: [RFC 10/11] iio: Add OF support
On 02/03/2013 12:47 PM, Lars-Peter Clausen wrote:
> On 02/03/2013 12:39 PM, Jonathan Cameron wrote:
>> On 02/02/2013 04:10 PM, Guenter Roeck wrote:
>>> On Sat, Feb 02, 2013 at 10:29:02AM +, Jonathan Cameron wrote:
>>>> On 01/31/2013 09:43 PM, Guenter Roeck wrote:
>>>>> Provide bindings, new API access functions, and parse OF data
>>>>> during initialization.
>>>>>
>>>> Firstly thanks for working on this Guenter, it's been a big hole
>>>> for a while largely because non of our largest developers were
>>>> actually using development platforms with device tree support.
>>>>
>>>> Given my knowledge of device tree is based on the odd article
>>>> and looking at similar sets of bindings this morning, my comments
>>>> are likely to be somewhat superficial and uninformed ;)
>>>>
>>>> Mostly on this one I'll take a back seat and let those who
>>>> know this stuff better come to a consensus.
>>>>
>>>> Jonathan
>>>>
>>>>> Signed-off-by: Guenter Roeck
>>>>> ---
>>>>> .../devicetree/bindings/iio/iio-bindings.txt | 97
>>>>> drivers/iio/inkern.c | 241
>>>>>
>>>>> include/linux/iio/consumer.h |8 +
>>>>> 3 files changed, 299 insertions(+), 47 deletions(-)
>>>>> create mode 100644 Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>> b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>> new file mode 100644
>>>>> index 000..0f51c95
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>> @@ -0,0 +1,97 @@
>>>>> +This binding is a work-in-progress, and are based on clock bindings and
>>>>> +suggestions from Lars-Peter Clausen [1].
>>>>> +
>>>>> +Sources of IIO channels can be represented by any node in the device
>>>>> +tree. Those nodes are designated as IIO providers. IIO consumer
>>>>> +nodes use a phandle and IIO specifier pair to connect IIO provider
>>>>> +outputs to IIO inputs. Similar to the gpio specifiers, an IIO
>>>>> +specifier is an array of one more more cells identifying the IIO
>>>>> +output on a device. The length of an IIO specifier is defined by the
>>>>> +value of a #io-channel-cells property in the clock provider node.
>>>>> +
>>>>> +[1] http://marc.info/?l=linux-iio&m=135902119507483&w=2
>>>>> +
>>>>> +==IIO providers==
>>>>> +
>>>>> +Required properties:
>>>>> +#io-channel-cells: Number of cells in an IIO specifier; Typically 0 for
>>>>> nodes
>>>>> +with a single IIO output and 1 for nodes with multiple
>>>>> +IIO outputs.
>>>>> +
>>>>> +Optional properties:
>>>>> +io-channel-output-names:
>>>>> + Recommended to be a list of strings of IIO output signal
>>>>> + names indexed by the first cell in the IIO specifier.
>>>>> + However, the meaning of io-channel-output-names is domain
>>>>> + specific to the IIO provider, and is only provided to
>>>>> + encourage using the same meaning for the majority of IIO
>>>>> + providers. This format may not work for IIO providers
>>>>> + using a complex IIO specifier format. In those cases it
>>>>> + is recommended to omit this property and create a binding
>>>>> + specific names property.
>>>>> +
>>>>> + IIO consumer nodes must never directly reference
>>>>> + the provider's io-channel-output-names property.
>>>>> +
>>>>> +For example:
>>>>> +
>>>>> +adc: adc@35 {
>>>>> + compatible = "maxim,max1139";
>>>>> + reg = <0x35>;
>>>>> +#io-channel-cells = <1>;
>>>>> +io-channel-output-names = "adc1", "adc2";
>>>>> +};
>>>>> +
>>>>> +- this n
Re: [PATCH v2 4/4] iio: Add OF support
On 02/03/2013 12:52 PM, Tomasz Figa wrote:
> On Sunday 03 of February 2013 12:29:23 Lars-Peter Clausen wrote:
>> On 02/03/2013 03:06 AM, Guenter Roeck wrote:
>>> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
>>>> Hi Guenter,
>>>>
>>>> Some comments inline.
>>>>
>>>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
>>>>> Provide bindings and parse OF data during initialization.
>>>>>
>>>>> Signed-off-by: Guenter Roeck
>>>>> ---
>>>>> - Documentation update per feedback
>>>>> - Dropped io-channel-output-names from the bindings document. The
>>>>> property is not used in the code, and it is not entirely clear what
>>>>> it
>>>>> would be used for. If there is a need for it, we can add it back in
>>>>> later on.
>>>>> - Don't export OF specific API calls
>>>>> - For OF support, no longer depend on iio_map
>>>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the code
>>>>> still builds if it is not selected.
>>>>> - Change iio_channel_get to take device pointer as argument instead
>>>>> of
>>>>> device name. Retain old API as of_iio_channel_get_sys.
>>>>> - iio_channel_get now works for both OF and non-OF configurations.
>>>>>
>>>>> .../devicetree/bindings/iio/iio-bindings.txt | 76
>>>>> drivers/iio/inkern.c | 186
>>>>>
>>>>> 2 files changed, 262 insertions(+)
>>>>>
>>>>> create mode 100644
>>>>>
>>>>> Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>> b/Documentation/devicetree/bindings/iio/iio-bindings.txt new file
>>>>> mode
>>>>> 100644
>>>>> index 000..58df5f6
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>> @@ -0,0 +1,76 @@
>>>>> +This binding is a work-in-progress. It is derived from clock
>>>>> bindings,
>>>>> +and based on suggestions from Lars-Peter Clausen [1].
>>>>> +
>>>>> +Sources of IIO channels can be represented by any node in the
>>>>> device
>>>>> +tree. Those nodes are designated as IIO providers. IIO consumer
>>>>> +nodes use a phandle and IIO specifier pair to connect IIO provider
>>>>> +outputs to IIO inputs. Similar to the gpio specifiers, an IIO
>>>>> +specifier is an array of one or more cells identifying the IIO
>>>>> +output on a device. The length of an IIO specifier is defined by
>>>>> the
>>>>> +value of a #io-channel-cells property in the clock provider node.
>>>>> +
>>>>> +[1] http://marc.info/?l=linux-iio&m=135902119507483&w=2
>>>>> +
>>>>> +==IIO providers==
>>>>> +
>>>>> +Required properties:
>>>>> +#io-channel-cells: Number of cells in an IIO specifier; Typically 0
>>>>> for nodes + with a single IIO output and 1 for nodes with
>>>>> multiple + IIO outputs.
>>>>> +
>>>>> +For example:
>>>>> +
>>>>> +adc: adc@35 {
>>>>> + compatible = "maxim,max1139";
>>>>> + reg = <0x35>;
>>>>> +#io-channel-cells = <1>;
>>>>> +};
>>>>> +
>>>>> +==IIO consumers==
>>>>> +
>>>>> +Required properties:
>>>>> +io-channels: List of phandle and IIO specifier pairs, one pair
>>>>> + for each IIO input to the device. Note: if the
>>>>> + IIO provider specifies '0' for #clock-cells, then
>>>>> + only the phandle portion of the pair will appear.
>>>>> +
>>>>> +Optional properties:
>>>>> +io-channel-names:
>>>>> + List of IIO input name strings sorted in the same
>>>>> + order as the io-channels property. Consumers drivers
>>>>> + will use io-channel-names to match IIO input names
>>>>> + with IIO specifiers.
>>>
Re: [PATCH v2 4/4] iio: Add OF support
On 02/03/2013 01:59 AM, Guenter Roeck wrote:
> Provide bindings and parse OF data during initialization.
>
> Signed-off-by: Guenter Roeck
> ---
> - Documentation update per feedback
> - Dropped io-channel-output-names from the bindings document. The property is
> not used in the code, and it is not entirely clear what it would be used
> for.
> If there is a need for it, we can add it back in later on.
> - Don't export OF specific API calls
> - For OF support, no longer depend on iio_map
> - Add #ifdef CONFIG_OF where appropriate, and ensure that the code still
> builds
> if it is not selected.
> - Change iio_channel_get to take device pointer as argument instead of device
> name. Retain old API as of_iio_channel_get_sys.
> - iio_channel_get now works for both OF and non-OF configurations.
>
>From my point of view this looks good in general now, Just a few comments.
> .../devicetree/bindings/iio/iio-bindings.txt | 76
> drivers/iio/inkern.c | 186
>
> 2 files changed, 262 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/iio-bindings.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt
> b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> new file mode 100644
> index 000..58df5f6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> @@ -0,0 +1,76 @@
> +This binding is a work-in-progress. It is derived from clock bindings,
> +and based on suggestions from Lars-Peter Clausen [1].
> +
> +Sources of IIO channels can be represented by any node in the device
> +tree. Those nodes are designated as IIO providers. IIO consumer
> +nodes use a phandle and IIO specifier pair to connect IIO provider
> +outputs to IIO inputs. Similar to the gpio specifiers, an IIO
> +specifier is an array of one or more cells identifying the IIO
> +output on a device. The length of an IIO specifier is defined by the
> +value of a #io-channel-cells property in the clock provider node.
> +
Is the extra space at the begining of each sentence on purpose?
> +[1] http://marc.info/?l=linux-iio&m=135902119507483&w=2
> +
> +==IIO providers==
> +
> +Required properties:
> +#io-channel-cells: Number of cells in an IIO specifier; Typically 0 for nodes
> +with a single IIO output and 1 for nodes with multiple
> +IIO outputs.
> +
> +For example:
> +
> +adc: adc@35 {
> + compatible = "maxim,max1139";
> + reg = <0x35>;
> +#io-channel-cells = <1>;
You are mixing tabs and spaces here for indention.
> +};
> +
> +==IIO consumers==
> +
> +Required properties:
> +io-channels: List of phandle and IIO specifier pairs, one pair
> + for each IIO input to the device. Note: if the
> + IIO provider specifies '0' for #clock-cells, then
> + only the phandle portion of the pair will appear.
> +
> +Optional properties:
> +io-channel-names:
> + List of IIO input name strings sorted in the same
> + order as the io-channels property. Consumers drivers
> + will use io-channel-names to match IIO input names
> + with IIO specifiers.
> +io-channel-ranges:
> + Empty property indicating that child nodes can inherit named
> + IIO channels from this node. Useful for bus nodes to provide
> + and IIO channel to their children.
> +
> +For example:
> +
> +device {
> +io-channels = <&adc 1>, <&ref 0>;
> +io-channel-names = "vcc", "vdd";
> +};
> +
> +This represents a device with two IIO inputs, named "vcc" and "vdd".
> +The vcc channel is connected to output 1 of the &adc device, and the
> +vdd channel is connected to output 0 of the &ref device.
> +
> +==Example==
> +
> + adc: max1139@35 {
> + compatible = "maxim,max1139";
> + reg = <0x35>;
> + #io-channel-cells = <1>;
> + };
> +
> + ...
> +
> + iio_hwmon {
> + compatible = "iio-hwmon";
> + io-channels = <&adc 0>, <&adc 1>, <&adc 2>,
> + <&adc 3>, <&adc 4>, <&adc 5>,
> + <&adc 6>, <&adc 7>, <&adc 8>,
> + <&adc 9>, <&adc 10>, <&adc 11>;
I'm not sure how much sense this example makes, since you can only request
those ch
Re: [PATCH v2 4/4] iio: Add OF support
On 02/03/2013 06:30 PM, Tomasz Figa wrote: > On Sunday 03 of February 2013 09:01:07 Guenter Roeck wrote: >> On Sun, Feb 03, 2013 at 12:52:40PM +0100, Tomasz Figa wrote: >>> On Sunday 03 of February 2013 12:29:23 Lars-Peter Clausen wrote: >>>> On 02/03/2013 03:06 AM, Guenter Roeck wrote: >>>>> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote: >>>>>> Hi Guenter, >>>>>> >>>>>> Some comments inline. >>>>>> >>>>>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote: >>>>>>> Provide bindings and parse OF data during initialization. >>>>>>> >>>>>>> Signed-off-by: Guenter Roeck >>>>>>> --- >>>>>>> - Documentation update per feedback >>>>>>> - Dropped io-channel-output-names from the bindings document. >>>>>>> The >>>>>>> property is not used in the code, and it is not entirely clear >>>>>>> what >>>>>>> it >>>>>>> would be used for. If there is a need for it, we can add it back >>>>>>> in >>>>>>> later on. >>>>>>> - Don't export OF specific API calls >>>>>>> - For OF support, no longer depend on iio_map >>>>>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the >>>>>>> code >>>>>>> still builds if it is not selected. >>>>>>> - Change iio_channel_get to take device pointer as argument >>>>>>> instead >>>>>>> of >>>>>>> device name. Retain old API as of_iio_channel_get_sys. >>>>>>> - iio_channel_get now works for both OF and non-OF >>>>>>> configurations. >>>>>>> >>>>>>> .../devicetree/bindings/iio/iio-bindings.txt | 76 >>>>>>> >>>>>>> drivers/iio/inkern.c | 186 >>>>>>> >>>>>>> 2 files changed, 262 insertions(+) >>>>>>> >>>>>>> create mode 100644 >>>>>>> >>>>>>> Documentation/devicetree/bindings/iio/iio-bindings.txt >>>>>>> >>>>>>> diff --git >>>>>>> a/Documentation/devicetree/bindings/iio/iio-bindings.txt >>>>>>> b/Documentation/devicetree/bindings/iio/iio-bindings.txt new >>>>>>> file >>>>>>> mode >>>>>>> 100644 >>>>>>> index 000..58df5f6 >>>>>>> --- /dev/null >>>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt >>>>>>> @@ -0,0 +1,76 @@ >>>>>>> +This binding is a work-in-progress. It is derived from clock >>>>>>> bindings, >>>>>>> +and based on suggestions from Lars-Peter Clausen [1]. >>>>>>> + >>>>>>> +Sources of IIO channels can be represented by any node in the >>>>>>> device >>>>>>> +tree. Those nodes are designated as IIO providers. IIO >>>>>>> consumer >>>>>>> +nodes use a phandle and IIO specifier pair to connect IIO >>>>>>> provider >>>>>>> +outputs to IIO inputs. Similar to the gpio specifiers, an IIO >>>>>>> +specifier is an array of one or more cells identifying the IIO >>>>>>> +output on a device. The length of an IIO specifier is defined >>>>>>> by >>>>>>> the >>>>>>> +value of a #io-channel-cells property in the clock provider >>>>>>> node. >>>>>>> + >>>>>>> +[1] http://marc.info/?l=linux-iio&m=135902119507483&w=2 >>>>>>> + >>>>>>> +==IIO providers== >>>>>>> + >>>>>>> +Required properties: >>>>>>> +#io-channel-cells: Number of cells in an IIO specifier; >>>>>>> Typically 0 >>>>>>> for nodes +with a single IIO output and 1 for nodes > with >>>>>>> multiple + IIO outputs. >>>>>>> + >>>>>>> +For example: >>>>>>> + >>>>>>> +
Re: [PATCH v2 4/4] iio: Add OF support
On 02/03/2013 05:31 PM, Guenter Roeck wrote:
> On Sun, Feb 03, 2013 at 12:29:23PM +0100, Lars-Peter Clausen wrote:
>> On 02/03/2013 03:06 AM, Guenter Roeck wrote:
>>> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
>>>> Hi Guenter,
>>>>
>>>> Some comments inline.
>>>>
>>>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
>>>>> Provide bindings and parse OF data during initialization.
>>>>>
>>>>> Signed-off-by: Guenter Roeck
>>>>> ---
>>>>> - Documentation update per feedback
>>>>> - Dropped io-channel-output-names from the bindings document. The
>>>>> property is not used in the code, and it is not entirely clear what it
>>>>> would be used for. If there is a need for it, we can add it back in
>>>>> later on.
>>>>> - Don't export OF specific API calls
>>>>> - For OF support, no longer depend on iio_map
>>>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the code still
>>>>> builds if it is not selected.
>>>>> - Change iio_channel_get to take device pointer as argument instead of
>>>>> device name. Retain old API as of_iio_channel_get_sys.
>>>>> - iio_channel_get now works for both OF and non-OF configurations.
>>>>>
>>>>> .../devicetree/bindings/iio/iio-bindings.txt | 76
>>>>> drivers/iio/inkern.c | 186
>>>>> 2 files changed, 262 insertions(+)
>>>>> create mode 100644
>>>>> Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>> b/Documentation/devicetree/bindings/iio/iio-bindings.txt new file mode
>>>>> 100644
>>>>> index 000..58df5f6
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>> @@ -0,0 +1,76 @@
>>>>> +This binding is a work-in-progress. It is derived from clock bindings,
>>>>> +and based on suggestions from Lars-Peter Clausen [1].
>>>>> +
>>>>> +Sources of IIO channels can be represented by any node in the device
>>>>> +tree. Those nodes are designated as IIO providers. IIO consumer
>>>>> +nodes use a phandle and IIO specifier pair to connect IIO provider
>>>>> +outputs to IIO inputs. Similar to the gpio specifiers, an IIO
>>>>> +specifier is an array of one or more cells identifying the IIO
>>>>> +output on a device. The length of an IIO specifier is defined by the
>>>>> +value of a #io-channel-cells property in the clock provider node.
>>>>> +
>>>>> +[1] http://marc.info/?l=linux-iio&m=135902119507483&w=2
>>>>> +
>>>>> +==IIO providers==
>>>>> +
>>>>> +Required properties:
>>>>> +#io-channel-cells: Number of cells in an IIO specifier; Typically 0 for
>>>>> nodes + with a single IIO output and 1 for nodes with
>>>>> multiple
>>>>> +IIO outputs.
>>>>> +
>>>>> +For example:
>>>>> +
>>>>> +adc: adc@35 {
>>>>> + compatible = "maxim,max1139";
>>>>> + reg = <0x35>;
>>>>> +#io-channel-cells = <1>;
>>>>> +};
>>>>> +
>>>>> +==IIO consumers==
>>>>> +
>>>>> +Required properties:
>>>>> +io-channels: List of phandle and IIO specifier pairs, one pair
>>>>> + for each IIO input to the device. Note: if the
>>>>> + IIO provider specifies '0' for #clock-cells, then
>>>>> + only the phandle portion of the pair will appear.
>>>>> +
>>>>> +Optional properties:
>>>>> +io-channel-names:
>>>>> + List of IIO input name strings sorted in the same
>>>>> + order as the io-channels property. Consumers drivers
>>>>> + will use io-channel-names to match IIO input names
>>>>> + with IIO specifiers.
>>>>> +io-channel-ranges:
>>>>> + Empty property indicating that child nodes can inherit named
>>>>> + IIO channels from this
Re: [PATCH v3 0/2] i2c/of: Populate multiplexed i2c busses from the device tree.
On 04/12/2012 11:14 PM, David Daney wrote: > From: David Daney > > v3: Integrate changes from Lars-Peter Clausen to make better use of > the of_*() infrastructure. Get rid of ugly #ifdefs. > > v2: Update bindings to use "reg" insutead of "cell-index" > > v1: Unchanged from the original RFC where I said: > > We need to populate our I2C devices from the device tree, but some > of our systems have multiplexers which currently break this process. > > The basic idea is to have the generic i2c-mux framework propagate > the device_node for the child bus into the corresponding > i2c_adapter, and then call of_i2c_register_devices(). This means > that the device tree bindings for *all* i2c muxes must have some > common properties. I try to document these in mux.txt. > > This is now tested against 3.4-rc2 and is still working well. > I've been using these patches with a pca9548 and a pca9546 for a while now. Both: Tested-by: Lars-Peter Clausen ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
[PATCH] I2C: xiic: Add OF binding support
Signed-off-by: Lars-Peter Clausen
---
Documentation/devicetree/bindings/i2c/xiic.txt | 22 ++
drivers/i2c/busses/i2c-xiic.c | 23 ++-
2 files changed, 40 insertions(+), 5 deletions(-)
create mode 100644 Documentation/devicetree/bindings/i2c/xiic.txt
diff --git a/Documentation/devicetree/bindings/i2c/xiic.txt
b/Documentation/devicetree/bindings/i2c/xiic.txt
new file mode 100644
index 000..ceabbe9
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/xiic.txt
@@ -0,0 +1,22 @@
+Xilinx IIC controller:
+
+Required properties:
+- compatible : Must be "xlnx,xps-iic-2.00.a"
+- reg : IIC register location and length
+- interrupts : IIC controller unterrupt
+- #address-cells = <1>
+- #size-cells = <0>
+
+Optional properties:
+- Child nodes conforming to i2c bus binding
+
+Example:
+
+ axi_iic_0: i2c@4080 {
+ compatible = "xlnx,xps-iic-2.00.a";
+ interrupts = < 1 2 >;
+ reg = < 0x4080 0x1 >;
+
+ #size-cells = <0>;
+ #address-cells = <1>;
+ };
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 2bded76..641d0e5 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -40,6 +40,7 @@
#include
#include
#include
+#include
#define DRIVER_NAME "xiic-i2c"
@@ -705,8 +706,6 @@ static int __devinit xiic_i2c_probe(struct platform_device
*pdev)
goto resource_missing;
pdata = (struct xiic_i2c_platform_data *) pdev->dev.platform_data;
- if (!pdata)
- return -EINVAL;
i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
if (!i2c)
@@ -730,6 +729,7 @@ static int __devinit xiic_i2c_probe(struct platform_device
*pdev)
i2c->adap = xiic_adapter;
i2c_set_adapdata(&i2c->adap, i2c);
i2c->adap.dev.parent = &pdev->dev;
+ i2c->adap.dev.of_node = pdev->dev.of_node;
xiic_reinit(i2c);
@@ -748,9 +748,13 @@ static int __devinit xiic_i2c_probe(struct platform_device
*pdev)
goto add_adapter_failed;
}
- /* add in known devices to the bus */
- for (i = 0; i < pdata->num_devices; i++)
- i2c_new_device(&i2c->adap, pdata->devices + i);
+ if (pdata) {
+ /* add in known devices to the bus */
+ for (i = 0; i < pdata->num_devices; i++)
+ i2c_new_device(&i2c->adap, pdata->devices + i);
+ }
+
+ of_i2c_register_devices(&i2c->adap);
return 0;
@@ -795,12 +799,21 @@ static int __devexit xiic_i2c_remove(struct
platform_device* pdev)
return 0;
}
+#if defined(CONFIG_OF)
+static const struct of_device_id xiic_of_match[] __devinitconst = {
+ { .compatible = "xlnx,xps-iic-2.00.a", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, xiic_of_match);
+#endif
+
static struct platform_driver xiic_i2c_driver = {
.probe = xiic_i2c_probe,
.remove = __devexit_p(xiic_i2c_remove),
.driver = {
.owner = THIS_MODULE,
.name = DRIVER_NAME,
+ .of_match_table = of_match_ptr(xiic_of_match),
},
};
--
1.7.9.5
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] I2C: xiic: Add OF binding support
On 05/03/2012 01:10 PM, Wolfram Sang wrote: > On Wed, Apr 25, 2012 at 03:48:53PM +0200, Lars-Peter Clausen wrote: >> Signed-off-by: Lars-Peter Clausen > > Applied to next, thanks. > > How are your experiences with the driver? Does it still need > EXPERIMENTAL? I had some issues under certain conditions, e.g. certain types of I2C transfers would confuse the core and fail. But I hadn't had the time yet to investigate further. So I wouldn't remove the EXPERIMENTAL yet. - Lars ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
[PATCH] spi: Add support for specifing 3-wire mode via device tree
This patch allows to specify that a SPI device is connected in 3-wire mode via
the device tree.
Signed-off-by: Lars-Peter Clausen
---
Documentation/devicetree/bindings/spi/spi-bus.txt |2 ++
drivers/spi/spi.c |2 ++
2 files changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt
b/Documentation/devicetree/bindings/spi/spi-bus.txt
index e782add..46f2f3b 100644
--- a/Documentation/devicetree/bindings/spi/spi-bus.txt
+++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
@@ -33,6 +33,8 @@ contain the following properties.
shifted clock phase (CPHA) mode
- spi-cs-high - (optional) Empty property indicating device requires
chip select active high
+- spi-3wire - (optional) Empty property indicating device requires
+ 3-wire mode.
SPI example for an MPC5200 SPI bus:
spi@f00 {
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index fc0da39..09da7de 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -856,6 +856,8 @@ static void of_register_spi_devices(struct spi_master
*master)
spi->mode |= SPI_CPOL;
if (of_find_property(nc, "spi-cs-high", NULL))
spi->mode |= SPI_CS_HIGH;
+ if (of_find_property(nc, "spi-3wire", NULL))
+ spi->mode |= SPI_3WIRE;
/* Device speed */
prop = of_get_property(nc, "spi-max-frequency", &len);
--
1.7.10.4
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] input: pwm-beeper: Add devicetree probing support
On 09/24/2012 05:56 PM, Dmitry Torokhov wrote: > On Mon, Sep 24, 2012 at 07:55:38AM -0500, Rob Herring wrote: >> On 09/24/2012 02:37 AM, Sascha Hauer wrote: >>> A very simple binding, the only property is the phandle to the PWM. >>> >>> Signed-off-by: Sascha Hauer >> >> Acked-by: Rob Herring >> >>> --- >>> Documentation/devicetree/bindings/input/pwm-beeper.txt |7 +++ >>> drivers/input/misc/pwm-beeper.c| 11 ++- >>> 2 files changed, 17 insertions(+), 1 deletion(-) >>> create mode 100644 Documentation/devicetree/bindings/input/pwm-beeper.txt >>> >>> diff --git a/Documentation/devicetree/bindings/input/pwm-beeper.txt >>> b/Documentation/devicetree/bindings/input/pwm-beeper.txt >>> new file mode 100644 >>> index 000..7388b82 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/input/pwm-beeper.txt >>> @@ -0,0 +1,7 @@ >>> +* PWM beeper device tree bindings >>> + >>> +Registers a PWM device as beeper. >>> + >>> +Required properties: >>> +- compatible: should be "pwm-beeper" >>> +- pwms: phandle to the physical pwm device >>> diff --git a/drivers/input/misc/pwm-beeper.c >>> b/drivers/input/misc/pwm-beeper.c >>> index fc84c8a..a6aa48c 100644 >>> --- a/drivers/input/misc/pwm-beeper.c >>> +++ b/drivers/input/misc/pwm-beeper.c >>> @@ -75,7 +75,10 @@ static int __devinit pwm_beeper_probe(struct >>> platform_device *pdev) >>> if (!beeper) >>> return -ENOMEM; >>> >>> - beeper->pwm = pwm_request(pwm_id, "pwm beeper"); >>> + if (pdev->dev.platform_data) >>> + beeper->pwm = pwm_request(pwm_id, "pwm beeper"); >>> + else >>> + beeper->pwm = pwm_get(&pdev->dev, NULL); > > Hmm, pwm_id == 0 is a valid ID I think, but your change makes it go into > DT branch, potentially breaking it. Yes, this a bit tricky, but we only have a single in-tree user of the pwm-beeper which uses a id != 0. And now that all the PWM providers have been converted to the new generic PWM framework the old legacy API will go away soon anyway. So this if () else branch should hopefully only be necessary for a transitional period of 1-2 releases. So I think this change should be OK. But I think the patch is missing a change to the Kconfig entry to allow the driver to be selected if the generic PWM framework is available. --- a/drivers/input/misc/Kconfig +++ b/drivers/input/misc/Kconfig @@ -444,7 +444,7 @@ config INPUT_PCF8574 config INPUT_PWM_BEEPER tristate "PWM beeper support" - depends on HAVE_PWM + depends on HAVE_PWM || PWM help Say Y here to get support for PWM based beeper devices. - Lars ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] input: pwm-beeper: Add devicetree probing support
On 09/24/2012 08:49 PM, Sascha Hauer wrote: > On Mon, Sep 24, 2012 at 06:22:33PM +0200, Lars-Peter Clausen wrote: >> On 09/24/2012 05:56 PM, Dmitry Torokhov wrote: >>> On Mon, Sep 24, 2012 at 07:55:38AM -0500, Rob Herring wrote: >>>> On 09/24/2012 02:37 AM, Sascha Hauer wrote: >>>>> A very simple binding, the only property is the phandle to the PWM. >>>>> >>>>> Signed-off-by: Sascha Hauer >>>> >>>> Acked-by: Rob Herring >>>> >>>>> --- >>>>> Documentation/devicetree/bindings/input/pwm-beeper.txt |7 +++ >>>>> drivers/input/misc/pwm-beeper.c| 11 ++- >>>>> 2 files changed, 17 insertions(+), 1 deletion(-) >>>>> create mode 100644 Documentation/devicetree/bindings/input/pwm-beeper.txt >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/input/pwm-beeper.txt >>>>> b/Documentation/devicetree/bindings/input/pwm-beeper.txt >>>>> new file mode 100644 >>>>> index 000..7388b82 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/input/pwm-beeper.txt >>>>> @@ -0,0 +1,7 @@ >>>>> +* PWM beeper device tree bindings >>>>> + >>>>> +Registers a PWM device as beeper. >>>>> + >>>>> +Required properties: >>>>> +- compatible: should be "pwm-beeper" >>>>> +- pwms: phandle to the physical pwm device >>>>> diff --git a/drivers/input/misc/pwm-beeper.c >>>>> b/drivers/input/misc/pwm-beeper.c >>>>> index fc84c8a..a6aa48c 100644 >>>>> --- a/drivers/input/misc/pwm-beeper.c >>>>> +++ b/drivers/input/misc/pwm-beeper.c >>>>> @@ -75,7 +75,10 @@ static int __devinit pwm_beeper_probe(struct >>>>> platform_device *pdev) >>>>> if (!beeper) >>>>> return -ENOMEM; >>>>> >>>>> - beeper->pwm = pwm_request(pwm_id, "pwm beeper"); >>>>> + if (pdev->dev.platform_data) >>>>> + beeper->pwm = pwm_request(pwm_id, "pwm beeper"); >>>>> + else >>>>> + beeper->pwm = pwm_get(&pdev->dev, NULL); >>> >>> Hmm, pwm_id == 0 is a valid ID I think, but your change makes it go into >>> DT branch, potentially breaking it. > > My bad, I missed that platform_data is casted to an unsigned long. I > thought I would test for a pointer. > The obvious clean way would be to use a pointer for platform_data, but > given that this will vanish anyway soon, I think we could just test for > existence of dev->of_node instead of dev->platform_data. I think the plan is to convert the existing board file platforms to pwm_table and then remove the old pwm_request API. So this wouldn't work too well if we'd test for of_node. Maybe we can just run pwm_get unconditionally and fallback to pwm_request if it failed. That's also what the PWM backlight driver currently does. > >> >> Yes, this a bit tricky, but we only have a single in-tree user of the >> pwm-beeper which uses a id != 0. And now that all the PWM providers have >> been converted to the new generic PWM framework the old legacy API will go >> away soon anyway. So this if () else branch should hopefully only be >> necessary for a transitional period of 1-2 releases. So I think this change >> should be OK. >> >> But I think the patch is missing a change to the Kconfig entry to allow the >> driver to be selected if the generic PWM framework is available. >> >> --- a/drivers/input/misc/Kconfig >> +++ b/drivers/input/misc/Kconfig >> @@ -444,7 +444,7 @@ config INPUT_PCF8574 >> >> config INPUT_PWM_BEEPER >> tristate "PWM beeper support" >> -depends on HAVE_PWM >> +depends on HAVE_PWM || PWM > > Is this the preferred way to do this? Instead of doing the above I added > a 'select HAVE_PWM' to the pwm framework instead. I found a patch for that, > but there were comments to it that this is not good > Thierry said that this is his preferred solution. Given that HAVE_PWM will be extinct soon anyway I think it is fine. - Lars ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 7/8] serial: xilinx_uartps: get clock rate info from dts
On 10/31/2012 08:28 PM, Josh Cartwright wrote:
> Add support for specifying clock information for the uart clk via the
> device tree. This eliminates the need to hardcode rates in the device
> tree.
>
> Signed-off-by: Josh Cartwright
> ---
> arch/arm/boot/dts/zynq-7000.dtsi | 4 ++--
> drivers/tty/serial/xilinx_uartps.c | 30 +-
> 2 files changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi
> b/arch/arm/boot/dts/zynq-7000.dtsi
> index bb3085c..5fb763f 100644
> --- a/arch/arm/boot/dts/zynq-7000.dtsi
> +++ b/arch/arm/boot/dts/zynq-7000.dtsi
> @@ -44,14 +44,14 @@
> compatible = "xlnx,xuartps";
> reg = <0xE000 0x1000>;
> interrupts = <0 27 4>;
> - clock = <5000>;
> + clocks = <&uart_clk 0>;
> };
>
> uart1: uart@e0001000 {
> compatible = "xlnx,xuartps";
> reg = <0xE0001000 0x1000>;
> interrupts = <0 50 4>;
> - clock = <5000>;
> + clocks = <&uart_clk 0>;
Shouldn't this be <&uart_clk 1>?
> };
>
> slcr: slcr@f800 {
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 5/8] ARM: zynq: add COMMON_CLK support
On 10/31/2012 07:58 PM, Josh Cartwright wrote:
> [...]
> +#define PERIPH_CLK_CTRL_SRC(x) (periph_clk_parent_map[((x)&3)>>4])
> +#define PERIPH_CLK_CTRL_DIV(x) (((x)&0x3F00)>>8)
A few more spaces wouldn't hurt ;)
> [...]
> +static void __init zynq_periph_clk_setup(struct device_node *np)
> +{
> + struct zynq_periph_clk *periph;
> + const char *parent_names[3];
> + struct clk_init_data init;
> + struct clk *clk;
> + int err;
> + u32 reg;
> + int i;
> +
> + err = of_property_read_u32(np, "reg", ®);
> + WARN_ON(err);
Shouldn't the function abort if a error happens somewhere? Continuing here
will lead to undefined behavior. Same is probably true for the other WARN_ONs.
> +
> + periph = kzalloc(sizeof(*periph), GFP_KERNEL);
> + WARN_ON(!periph);
> +
> + periph->clk_ctrl = slcr_base + reg;
> + spin_lock_init(&periph->clkact_lock);
> +
> + init.name = np->name;
> + init.ops = &zynq_periph_clk_ops;
> + for (i = 0; i < ARRAY_SIZE(parent_names); i++)
> + parent_names[i] = of_clk_get_parent_name(np, i);
> + init.parent_names = parent_names;
> + init.num_parents = ARRAY_SIZE(parent_names);
> +
> + periph->hw.init = &init;
> +
> + clk = clk_register(NULL, &periph->hw);
> + WARN_ON(IS_ERR(clk));
> +
> + err = of_clk_add_provider(np, of_clk_src_simple_get, clk);
> + WARN_ON(err);
> +
> + for (i = 0; i < 2; i++) {
Not all of the peripheral clock generators have two output clocks. I think
it makes sense to use the number entries in clock-output-names here.
> + const char *name;
> +
> + err = of_property_read_string_index(np, "clock-output-names", i,
> + &name);
> + WARN_ON(err);
> +
> + periph->gates[i] = clk_register_gate(NULL, name, np->name, 0,
> + periph->clk_ctrl, i, 0,
> + &periph->clkact_lock);
> + WARN_ON(IS_ERR(periph->gates[i]));
> + }
> +
> + periph->onecell_data.clks = periph->gates;
> + periph->onecell_data.clk_num = i;
> +
> + err = of_clk_add_provider(np, of_clk_src_onecell_get,
> + &periph->onecell_data);
> + WARN_ON(err);
> +}
> [...]
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 5/8] ARM: zynq: add COMMON_CLK support
On 11/02/2012 02:38 PM, Josh Cartwright wrote:
> Thanks for the review.
>
> On Fri, Nov 02, 2012 at 10:33:44AM +0100, Lars-Peter Clausen wrote:
>> On 10/31/2012 07:58 PM, Josh Cartwright wrote:
>>> [...]
>>> +#define PERIPH_CLK_CTRL_SRC(x) (periph_clk_parent_map[((x)&3)>>4])
>>> +#define PERIPH_CLK_CTRL_DIV(x) (((x)&0x3F00)>>8)
>>
>> A few more spaces wouldn't hurt ;)
>
> Okay, sure.
>
>>> [...]
>>> +static void __init zynq_periph_clk_setup(struct device_node *np)
>>> +{
>>> + struct zynq_periph_clk *periph;
>>> + const char *parent_names[3];
>>> + struct clk_init_data init;
>>> + struct clk *clk;
>>> + int err;
>>> + u32 reg;
>>> + int i;
>>> +
>>> + err = of_property_read_u32(np, "reg", ®);
>>> + WARN_ON(err);
>>
>> Shouldn't the function abort if a error happens somewhere? Continuing here
>> will lead to undefined behavior. Same is probably true for the other
>> WARN_ONs.
>
> The way I see it is: the kernel is will be left in a bad state in the
> case of any failure, regardless of if we bail out or continue. AFAICT,
> there is no clean way to recover from a failure this early.
>
> Given that, it seems simpler (albeit marginally so) just to continue; so
> that's what I chose to do. I'm not opposed to bailing out, just not
> convinced it does anything for us.
>
The issue with this approach is that, while you get a warning, unexpected
seemingly unrelated side-effects may happen later on. E.g. if no reg
property for the clock is specified the reg variable will be uninitialized
and contain whatever was on the stack before. The clock will be registered
nonetheless and the boot process continues. Now if the clock is enabled a
bit in a random register will be modified, which could result in strange and
abnormal behavior, which can be very hard to track down.
Also if for example just one clock has its reg property missing the system
will continue to boot if we bail out here. It is just that the peripherals
using that clock won't work. Which will certainly be easier to diagnose than
random abnormal behavior.
>>> +
>>> + periph = kzalloc(sizeof(*periph), GFP_KERNEL);
>>> + WARN_ON(!periph);
>>> +
>>> + periph->clk_ctrl = slcr_base + reg;
>>> + spin_lock_init(&periph->clkact_lock);
>>> +
>>> + init.name = np->name;
>>> + init.ops = &zynq_periph_clk_ops;
>>> + for (i = 0; i < ARRAY_SIZE(parent_names); i++)
>>> + parent_names[i] = of_clk_get_parent_name(np, i);
>>> + init.parent_names = parent_names;
>>> + init.num_parents = ARRAY_SIZE(parent_names);
>>> +
>>> + periph->hw.init = &init;
>>> +
>>> + clk = clk_register(NULL, &periph->hw);
>>> + WARN_ON(IS_ERR(clk));
>>> +
>>> + err = of_clk_add_provider(np, of_clk_src_simple_get, clk);
>>> + WARN_ON(err);
>>> +
>>> + for (i = 0; i < 2; i++) {
>>
>> Not all of the peripheral clock generators have two output clocks. I think
>> it makes sense to use the number entries in clock-output-names here.
>
> Yes, I agree. I'll also update the bindings documentation.
>
> Thanks again,
> Josh
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: Querry on IIO/ADC DT based consumer device probe
On 11/08/2012 05:53 PM, Alban Bedel wrote:
> On Fri, 5 Oct 2012 14:37:01 +0530
> Naveen Krishna Ch
> wrote:
>
>> Hello All,
>>
>> I'm trying to add an ADC driver under IIO/ADC.
>> Machine is DT based so, passing the ADC device as tree node and
>> consumer devices (thermistors) as child nodes via DT.
>>
>> I don't find a frame work to parse the child nodes and probe them like
>> I2C does using of/of_i2c.c
>
> Here is my take at DT support in IIO, I wanted to submit that later on
> after some more test and cleanup but you can see if it help you.
>
> Alban
>
> From 15decde13239f09101673b08aa0bd7e67e970b3c Mon Sep 17 00:00:00 2001
> From: Alban Bedel
> Date: Thu, 18 Oct 2012 17:07:29 +0200
> Subject: [PATCH] IIO: Add basic DT/devm support for in kernel users
>
> Signed-off-by: Alban Bedel
> ---
> .../devicetree/bindings/iio/iio-channel.txt| 27
> drivers/iio/inkern.c | 154
>
> include/linux/iio/consumer.h | 63
> 3 files changed, 244 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/iio/iio-channel.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/iio-channel.txt
> b/Documentation/devicetree/bindings/iio/iio-channel.txt
> new file mode 100644
> index 000..dc894f4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/iio-channel.txt
> @@ -0,0 +1,27 @@
> +Specifying IIO channels for devices
> +===
> +
> +1) iio-channels property
> +
> +
> +Nodes that makes use of IIO channels should specify them using one or more
> +properties, each containing a iio-channels-list':
> +
> + iio-channel-list ::= [iio-channel-list]
> + single-iio-channel ::=
> + iio-channel-phandle : phandle to iio-channel controller node
> + iio-channel-specifier : Array of #iio-channel-cells specifying
> + specific IIO channel (controller specific)
I'd prefer something that is more in sync with what we have for other
subsystems which have a provider-consumer relationship, like for example the
clk and dma frameworks. Something like:
iio-channels = <&phandle1 &phandle2>;
iio-channel-names = "voltage", "current";
Also there is another major issue here. Devicetree is supposed to be
operating system independent, IIO on the other hand is a Linux specific
term. I'm not sure though yet what could be used instead. Maybe just
'io-...'. I've put the devicetree list on Cc.
> +
> +GPIO properties should be named "[-]iio-channels". Exact
IIO ;)
> +meaning of each gpios property must be documented in the device tree
> +binding for each device.
> +
> +2) iio device nodes
> +
> +
> +Every IIO device must have #iio-channel-cells contain the size of the
> +iio-channel-specifier.
> +
> +Currently #iio-channel-cells will always be 1 but this will most
> +propably change in the future.
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 6d5194f..5986ef5 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -10,6 +10,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include "iio_core.h"
> @@ -404,3 +405,156 @@ err_unlock:
> return ret;
> }
> EXPORT_SYMBOL_GPL(iio_get_channel_type);
> +
> +#ifdef CONFIG_OF
> +static int of_dev_node_match(struct device *dev, void *data)
> +{
> +return dev->parent ? dev->parent->of_node == data : 0;
> +}
> +
> +int __of_get_named_iio_channel(struct iio_channel* channel,
> +struct device_node *np,
> +const char *propname,
> +int index)
> +{
> + struct device *dev;
> + struct of_phandle_args ph;
> + struct iio_dev *indio_dev;
> + int ret, channel_id = -1;
> +
> + ret = of_parse_phandle_with_args(np, propname, "#iio-channel-cells",
> + index, &ph);
> + if (ret)
> + return ret;
> +
> + dev = bus_find_device(&iio_bus_type, NULL, ph.np,
> + of_dev_node_match);
> + if (!dev)
> + return -EPROBE_DEFER;
> +
> + if (ph.args_count > 0)
> + channel_id = ph.args[0];
> +
> + indio_dev = dev_to_iio_dev(dev);
> + if (channel_id < 0 || channel_id >= indio_dev->num_channels)
> + return -EINVAL;
> +
> + iio_device_get(indio_dev);
> + channel->indio_dev = indio_dev;
> + channel->channel = &indio_dev->channels[channel_id];
> +
> + return 0;
> +}
> +
> +/**
> + * of_get_iio_channel() - get a iio channel from a device tree property
> + * @np: Device node to get the channel from
> + * @propname: The property to read
> + * @index:Index of the channel
> + */
> +struct iio_channel* of_get_named_iio_channel(struct device_node *np,
> + const char *propname,
> +
Re: [PATCH v2 4/4] iio: Add OF support
On 02/03/2013 09:58 PM, Jonathan Cameron wrote: > On 02/03/2013 06:55 PM, Lars-Peter Clausen wrote: >> On 02/03/2013 06:30 PM, Tomasz Figa wrote: >>> On Sunday 03 of February 2013 09:01:07 Guenter Roeck wrote: >>>> On Sun, Feb 03, 2013 at 12:52:40PM +0100, Tomasz Figa wrote: >>>>> On Sunday 03 of February 2013 12:29:23 Lars-Peter Clausen wrote: >>>>>> On 02/03/2013 03:06 AM, Guenter Roeck wrote: >>>>>>> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote: >>>>>>>> Hi Guenter, >>>>>>>> >>>>>>>> Some comments inline. >>>>>>>> >>>>>>>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote: >>>>>>>>> Provide bindings and parse OF data during initialization. >>>>>>>>> >>>>>>>>> Signed-off-by: Guenter Roeck >>>>>>>>> --- >>>>>>>>> - Documentation update per feedback >>>>>>>>> - Dropped io-channel-output-names from the bindings document. >>>>>>>>> The >>>>>>>>> property is not used in the code, and it is not entirely clear >>>>>>>>> what >>>>>>>>> it >>>>>>>>> would be used for. If there is a need for it, we can add it back >>>>>>>>> in >>>>>>>>> later on. >>>>>>>>> - Don't export OF specific API calls >>>>>>>>> - For OF support, no longer depend on iio_map >>>>>>>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the >>>>>>>>> code >>>>>>>>> still builds if it is not selected. >>>>>>>>> - Change iio_channel_get to take device pointer as argument >>>>>>>>> instead >>>>>>>>> of >>>>>>>>> device name. Retain old API as of_iio_channel_get_sys. >>>>>>>>> - iio_channel_get now works for both OF and non-OF >>>>>>>>> configurations. >>>>>>>>> >>>>>>>>> .../devicetree/bindings/iio/iio-bindings.txt | 76 >>>>>>>>> >>>>>>>>> drivers/iio/inkern.c | 186 >>>>>>>>> >>>>>>>>> 2 files changed, 262 insertions(+) >>>>>>>>> >>>>>>>>> create mode 100644 >>>>>>>>> >>>>>>>>> Documentation/devicetree/bindings/iio/iio-bindings.txt >>>>>>>>> >>>>>>>>> diff --git >>>>>>>>> a/Documentation/devicetree/bindings/iio/iio-bindings.txt >>>>>>>>> b/Documentation/devicetree/bindings/iio/iio-bindings.txt new >>>>>>>>> file >>>>>>>>> mode >>>>>>>>> 100644 >>>>>>>>> index 000..58df5f6 >>>>>>>>> --- /dev/null >>>>>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt >>>>>>>>> @@ -0,0 +1,76 @@ >>>>>>>>> +This binding is a work-in-progress. It is derived from clock >>>>>>>>> bindings, >>>>>>>>> +and based on suggestions from Lars-Peter Clausen [1]. >>>>>>>>> + >>>>>>>>> +Sources of IIO channels can be represented by any node in the >>>>>>>>> device >>>>>>>>> +tree. Those nodes are designated as IIO providers. IIO >>>>>>>>> consumer >>>>>>>>> +nodes use a phandle and IIO specifier pair to connect IIO >>>>>>>>> provider >>>>>>>>> +outputs to IIO inputs. Similar to the gpio specifiers, an IIO >>>>>>>>> +specifier is an array of one or more cells identifying the IIO >>>>>>>>> +output on a device. The length of an IIO specifier is defined >>>>>>>>> by >>>>>>>>> the >>>>>>>>> +value of a #io-channel-cells property in the clock provider >>>>>>>>> node. >>>>>>>>>
Re: [PATCH v2 4/4] iio: Add OF support
On 02/04/2013 06:12 PM, Guenter Roeck wrote: > On Mon, Feb 04, 2013 at 12:14:52AM +0100, Tomasz Figa wrote: >> On Sunday 03 of February 2013 19:55:47 Lars-Peter Clausen wrote: >>> On 02/03/2013 06:30 PM, Tomasz Figa wrote: >>>> On Sunday 03 of February 2013 09:01:07 Guenter Roeck wrote: >>>>> On Sun, Feb 03, 2013 at 12:52:40PM +0100, Tomasz Figa wrote: >>>>>> On Sunday 03 of February 2013 12:29:23 Lars-Peter Clausen wrote: >>>>>>> On 02/03/2013 03:06 AM, Guenter Roeck wrote: >>>>>>>> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote: >>>>>>>>> Hi Guenter, >>>>>>>>> >>>>>>>>> Some comments inline. >>>>>>>>> >>>>>>>>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote: >>>>>>>>>> Provide bindings and parse OF data during initialization. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Guenter Roeck >>>>>>>>>> --- >>>>>>>>>> - Documentation update per feedback >>>>>>>>>> - Dropped io-channel-output-names from the bindings document. >>>>>>>>>> The >>>>>>>>>> property is not used in the code, and it is not entirely clear >>>>>>>>>> what >>>>>>>>>> it >>>>>>>>>> would be used for. If there is a need for it, we can add it back >>>>>>>>>> in >>>>>>>>>> later on. >>>>>>>>>> - Don't export OF specific API calls >>>>>>>>>> - For OF support, no longer depend on iio_map >>>>>>>>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the >>>>>>>>>> code >>>>>>>>>> still builds if it is not selected. >>>>>>>>>> - Change iio_channel_get to take device pointer as argument >>>>>>>>>> instead >>>>>>>>>> of >>>>>>>>>> device name. Retain old API as of_iio_channel_get_sys. >>>>>>>>>> - iio_channel_get now works for both OF and non-OF >>>>>>>>>> configurations. >>>>>>>>>> >>>>>>>>>> .../devicetree/bindings/iio/iio-bindings.txt | 76 >>>>>>>>>> >>>>>>>>>> drivers/iio/inkern.c | 186 >>>>>>>>>> >>>>>>>>>> 2 files changed, 262 insertions(+) >>>>>>>>>> >>>>>>>>>> create mode 100644 >>>>>>>>>> >>>>>>>>>> Documentation/devicetree/bindings/iio/iio-bindings.txt >>>>>>>>>> >>>>>>>>>> diff --git >>>>>>>>>> a/Documentation/devicetree/bindings/iio/iio-bindings.txt >>>>>>>>>> b/Documentation/devicetree/bindings/iio/iio-bindings.txt new >>>>>>>>>> file >>>>>>>>>> mode >>>>>>>>>> 100644 >>>>>>>>>> index 000..58df5f6 >>>>>>>>>> --- /dev/null >>>>>>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt >>>>>>>>>> @@ -0,0 +1,76 @@ >>>>>>>>>> +This binding is a work-in-progress. It is derived from clock >>>>>>>>>> bindings, >>>>>>>>>> +and based on suggestions from Lars-Peter Clausen [1]. >>>>>>>>>> + >>>>>>>>>> +Sources of IIO channels can be represented by any node in the >>>>>>>>>> device >>>>>>>>>> +tree. Those nodes are designated as IIO providers. IIO >>>>>>>>>> consumer >>>>>>>>>> +nodes use a phandle and IIO specifier pair to connect IIO >>>>>>>>>> provider >>>>>>>>>> +outputs to IIO inputs. Similar to the gpio specifiers, an IIO >>>>>>>>>> +specifier is an array of one or more cells identifying the IIO &g
Re: [alsa-devel] [PATCH 1/4 v6] ASoC: add .of_xlate_dai_name callback on struct snd_soc_dai_driver
On 02/14/2013 10:21 AM, Kuninori Morimoto wrote:
[...]
> +const char *snd_soc_of_get_port_dai_name(struct device_node *of_node,
> + const char *prop)
> +{
> + struct snd_soc_dai *dai;
> + struct of_phandle_args args;
> + const char *name;
> + int ret;
> +
> + ret = of_parse_phandle_with_args(of_node, prop,
> + "#sound-dai-cells", 0, &args);
> + if (ret)
> + return NULL;
> +
> + of_node_put(args.np);
You should keep the reference until after the link.
> +
> + list_for_each_entry(dai, &dai_list, list) {
> + if (dai->dev->of_node != args.np)
> + continue;
> +
> + if (dai->driver->of_xlate_dai_name) {
> + name = dai->driver->of_xlate_dai_name(dai, &args);
> + if (name)
> + return name;
> + }
Hm, this is not really a translate function, but rather a match function. It
iterates over all dais of the device and returns the name of the first one
which matches. But there is no translation going on. If the dai matches the
callback will always want to return dai->driver->name. Anything else doesn't
make much sense.
> + }
> +
> + return NULL;
> +}
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [alsa-devel] [PATCH 4/4 v6] ASoC: simple-card: add Device Tree support
On 02/14/2013 10:24 AM, Kuninori Morimoto wrote:
> Support for loading the simple-card module via devicetree.
> It requests cpu/codec information,
> and .of_xlate_dai_name support on each driver for probing.
>
> Signed-off-by: Kuninori Morimoto
> ---
> v5 -> v6
>
> - cpu/codec become sub-node
> - it is using new .of_xlate_dai_name based
>snd_soc_of_get_port_dai_name()
> - update Documentation
>
> .../devicetree/bindings/sound/simple-card.txt | 75 +
> sound/soc/generic/simple-card.c| 118
> +++-
> 2 files changed, 188 insertions(+), 5 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt
>
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt
> b/Documentation/devicetree/bindings/sound/simple-card.txt
> new file mode 100644
> index 000..86e0d9f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -0,0 +1,75 @@
> +Simple-Card:
> +
> +Required properties:
> +
> +- compatible : "simple-audio"
> +- simple-audio,card-name : simple-audio card name
> +- simple-audio,format: see below
> +- simple-audio,cpu : CPU sub-node, see below
> +- simple-audio,codec : CODEC sub-node, see below
> +
> +Optional properties:
> +
> +- simple-audio,system-clock-frequency: system clock rate if it is
> connected to both CPU/CODEC
> +- simple-audio,bitclock-inversion: bit clock inversion for both CPU/CODEC
> +- simple-audio,frame-inversion : frame inversion for both
> CPU/CODEC
> +
> +Required cpu/codec subnode properties:
> +
> +- simple-audio,dev : phandle and port for CPU/CODEC
> +- simple-audio,frame-master : frame master
> +- simple-audio,bitclock-master : bitclock master
> +#sound-dai-cells integer is required on simple-audio,dev phandle's node
Shouldn't the names of '#sound-dai-cells' and 'simple-audio,dev' kind of
match? E.g. '#sound-dai-cells' and 'sound-dai'. Maybe drop the sound, since
the a in dai kind of implies this.
[...]
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 6cf8355..e50e415 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -9,6 +9,7 @@
> * published by the Free Software Foundation.
> */
>
> +#include
> #include
> #include
> #include
> @@ -52,11 +53,108 @@ static int asoc_simple_card_dai_init(struct
> snd_soc_pcm_runtime *rtd)
> return 0;
> }
>
> +static struct device_node*
> +__asoc_simple_card_parse_of(struct device_node *np,
> + struct asoc_simple_dai *dai)
> +{
> + struct device_node *node;
> + char prop[128];
> +
> + /* get "simple-audio,dev = <&phandle port>" */
> + snprintf(prop, sizeof(prop), "simple-audio,dev");
Why do you need the extra buffer? Can you just pass, "simple-audio,dev"
directly to of_parse_phandle?
> + node = of_parse_phandle(np, prop, 0);
> + if (!node)
> + return NULL;
> +
> + of_node_put(node);
You shouldn't drop the reference until you are done processing it. Which in
this case is only after the device has been unregistered, since you pass the
node on to the ASoC core.
> +
> + /* get dai-name */
> + dai->name = snd_soc_of_get_port_dai_name(np, prop);
> +
> + /* get dai specific format */
> + dai->fmt = snd_soc_of_parse_daifmt(np, "simple-audio,");
> +
> + /* get "simple-audio,system-clock-frequency = " */
> + snprintf(prop, sizeof(prop), "simple-audio,system-clock-frequency");
> + of_property_read_u32(np, prop, &dai->sysclk);
> +
> + return node;
> +}
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [alsa-devel] [PATCH 3/4 v6] ASoC: fsi: enable .of_xlate_dai_name on struct snd_soc_dai_driver
On 02/20/2013 02:04 AM, Kuninori Morimoto wrote:
>
> Hi Stephen
>
>> Hmm. I think that the .of_xlate_dai_name should be something related to
>> the object (e.g. CODEC) that contains the DAIs, rather than the DAIs
>> themselves. The intent is to ask some object (e.g. CODEC) which of its
>> DAIs is represented by the DT DAI specifier, not to ask each DAI whether
>> it is represented by that particular DT DAI specifier.
>>
>> Aside from that, I'd expect the patch above to be something more like:
>>
>> const char *fsi_xlate_dai_name(struct snd_soc_dai *dai,
>> const struct of_phandle_args *args)
>> {
>> switch (args[0]) {
>> case 0:
>> return fsi_soc_dai[0].name;
>> case 1:
>> return fsi_soc_dai[1].name;
>> default:
>> return NULL;
>> }
>> }
>
> Thank you for pointing it.
> I use this style in next patch.
I don't think this makes any more sense than the current version. You still
iterate over all dai instances and call the xlate function for the first dai
where the _parent_ device's of_node matches. You also pass in the this dai
as the first parameter to the xlate function, which makes even less sense
since you can use it to lookup the correct name, which may either be the
dai's name or any of its siblings.
If you want to use the xlate paradigm you really need to keep track of the
cpu devices and make the xlate callback a property of the cpu device. Well
or use a match paradigm instead of xlate.
- Lars
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [alsa-devel] [PATCH 4/4 v6] ASoC: simple-card: add Device Tree support
On 02/20/2013 01:48 AM, Kuninori Morimoto wrote:
>
> Hi Lars
>
> Thank you for checking patch
>
>>> +- simple-audio,dev : phandle and port for CPU/CODEC
>>> +- simple-audio,frame-master: frame master
>>> +- simple-audio,bitclock-master : bitclock master
>>> +#sound-dai-cells integer is required on simple-audio,dev phandle's node
>>
>> Shouldn't the names of '#sound-dai-cells' and 'simple-audio,dev' kind of
>> match? E.g. '#sound-dai-cells' and 'sound-dai'. Maybe drop the sound, since
>> the a in dai kind of implies this.
>
> Thank you, but I would like to keep "simple-audio" name for it too.
> So, can I use
> simple-audio-dai
> #simple-audio-dai-cells
The requirement to be able to find the DAI via devicetree is not specific to
the simple-audio driver. I'd prefer if we could come up with a common
binding which can be used by multiple drivers. Since you also add the xlate
(or match) function to the sound core it makes sense - in my option - to
also have a generic phandle -> DAI look-up method in the core.
- Lars
>
>>> +static struct device_node*
>>> +__asoc_simple_card_parse_of(struct device_node *np,
>>> + struct asoc_simple_dai *dai)
>>> +{
>>> + struct device_node *node;
>>> + char prop[128];
>>> +
>>> + /* get "simple-audio,dev = <&phandle port>" */
>>> + snprintf(prop, sizeof(prop), "simple-audio,dev");
>>
>> Why do you need the extra buffer? Can you just pass, "simple-audio,dev"
>> directly to of_parse_phandle?
>
> Indeed, thank you.
>
>>> + node = of_parse_phandle(np, prop, 0);
>>> + if (!node)
>>> + return NULL;
>>> +
>>> + of_node_put(node);
>>
>> You shouldn't drop the reference until you are done processing it. Which in
>> this case is only after the device has been unregistered, since you pass the
>> node on to the ASoC core.
>
> I see, will fix
>
> Best regards
> ---
> Kuninori Morimoto
> ___
> Alsa-devel mailing list
> alsa-de...@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5] iio: Add OF support
On 02/20/2013 06:17 PM, Jonathan Cameron wrote: > > > Guenter Roeck wrote: > >> On Fri, Feb 08, 2013 at 08:30:48AM +, Jonathan Cameron wrote: >>> On 07/02/13 17:09, Guenter Roeck wrote: >>>> Provide bindings and parse OF data during initialization. >>>> >>>> Signed-off-by: Guenter Roeck >>> Looks fine to me. Will give it a few more days to see what others >>> have to say. >>> >> Hi Jonathan, >> >> Any further feedback on this ? If there is anything else I need to do >> to get it accepted, please let me know. > Was kind of hoping for a few ack/ reviewed by comments! Missed current merge > window so I will pick it up for start of next cycle and assume silence means > everyone else is happy! The latest version looked good to me, you can add my: Reviewed-by: Lars-Peter Clausen ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 1/5 v7][RFC] ASoC: add snd_soc_register_cpu()
On 02/28/2013 08:14 PM, Stephen Warren wrote: > On 02/27/2013 05:42 PM, Kuninori Morimoto wrote: >> >> Hi Stephen >> >>> On 02/25/2013 01:51 AM, Kuninori Morimoto wrote: Current ASoC has register function for platform/codec/dai/card, but doesn't have for cpu. It often produces confusion and fault on ASoC. This patch adds very basic register function for cpu >>> >>> This seems reasonable to me. >>> >>> I can't recall how much influence the existing CODEC objects have on the >>> various routing/matching decisions inside the ASoC core. While this >>> patch does register and unregister CPU objects, I wonder if it should >>> have more impact on any of the existing core code? Certainly I'd expect >>> the CPU objects to show up in ASoC's debugfs and any similar files... >> >> This patch doesn't have any impact to existing drivers. >> It just added new cpu list. > > Sure. My point was that now you've added the list, perhaps the core ASoC > code might want to make use of it in the same way it makes use of CODEC > objects, since it's logically equivalent. In my opinion it makes sense to get rid of the distinction between CPU objects and CODEC objects in this case and instead introduce some kind of DAI container object. I mean the distinction is mostly for historic reasons from a time where it was only possible to bind a the cpu_dai of a link to a CPU DAI and the codec_dai to a CODEC DAI. But these dais both can for example be bound CODEC DAIs. I think to have a common base here would allow us to do some cleanups to the ASoC core and also avoid the duplication with snd_soc_of_get_cpu_dai_name and snd_soc_of_get_codec_dai_name which was introduced in this series. - Lars ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: RFC: Zynq Clock Controller
On 03/06/2013 06:27 PM, Sören Brinkmann wrote:
> Hi Jan,
>
> what a small world. Good to hear from you.
>
> On Wed, Mar 06, 2013 at 12:51:21PM +0100, Jan Lübbe wrote:
>> Hi Sören,
>>
>> On Tue, 2013-03-05 at 12:04 -0800, Sören Brinkmann wrote:
>>> For this reasons, I'd like to propose moving Zynq into the same
>>> direction. I.e. adding a clock controller with the following DT
>>> description (details may change but the general idea should become
>>> clear):
>>> clkc: clkc {
>>> #clock-cells = <1>;
>>> compatible = "xlnx,ps7-clkc";
>>> ps_clk_frequency = <>; # board x-tal
>>> # optional props
>>> gem0_emio_clk_freq = <12500>;
>>> gem1_emio_clk_freq = <5000>;
>>> can_mio_clk_freq_xx = <1234>; # this is possible 54 times
>>> with xx = 00..53
>>> };
I definitely prefer the way it is right now in upstream, where we have one
dt node per clock. It is more descriptive and also more extensible. And you
also don't have to remember the clock index, and can use the phandle
directly instead.
>>
>> The clock controller should only contain properties for input frequency
>> (which can obviously not be calculated at run-time).
>>
>> Are the gem*, can* properties inputs? If they are actually outputs, the
>> corresponding frequencies should be requested by the clock consumers and
>> not hard-coded in DT.
> They are inputs. GEM and CAN have the option to be clocked through (E)MIO
> pins, i.e. some external clock input which cannot be derived from ps_clk like
> all other clocks. I plan to register a fixed rate, root clock for each of
> those properties, if present.
If it is a static external input clock use a "fixed-clock" dt node to
describe it. This also has the advantage that is is also possible to use a
non-static clock.
- Lars
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: RFC: Zynq Clock Controller
On 03/07/2013 08:11 PM, Sören Brinkmann wrote:
> On Thu, Mar 07, 2013 at 10:36:35AM +0100, Lars-Peter Clausen wrote:
>> On 03/06/2013 06:27 PM, Sören Brinkmann wrote:
>>> Hi Jan,
>>>
>>> what a small world. Good to hear from you.
>>>
>>> On Wed, Mar 06, 2013 at 12:51:21PM +0100, Jan Lübbe wrote:
>>>> Hi Sören,
>>>>
>>>> On Tue, 2013-03-05 at 12:04 -0800, Sören Brinkmann wrote:
>>>>> For this reasons, I'd like to propose moving Zynq into the same
>>>>> direction. I.e. adding a clock controller with the following DT
>>>>> description (details may change but the general idea should become
>>>>> clear):
>>>>> clkc: clkc {
>>>>> #clock-cells = <1>;
>>>>> compatible = "xlnx,ps7-clkc";
>>>>> ps_clk_frequency = <>;# board x-tal
>>>>> # optional props
>>>>> gem0_emio_clk_freq = <12500>;
>>>>> gem1_emio_clk_freq = <5000>;
>>>>> can_mio_clk_freq_xx = <1234>; # this is possible 54 times
>>>>> with xx = 00..53
>>>>> };
>>
>> I definitely prefer the way it is right now in upstream, where we have one
>> dt node per clock. It is more descriptive and also more extensible. And you
>> also don't have to remember the clock index, and can use the phandle
>> directly instead.
> The problems I see with that are:
> - with the clock controller we can model the clock tree as we need without
> changing the DT all the time
When do we need to change the clock tree? The clock tree is pretty much fixed
in hardware. And the DT describes the hardware, so I don't so a problem there.
> - w/o a clock controller there is no block which has knowledge of the whole
> clock tree (unless we parse the DT), and could conduct reparent operations
> and similar
The clk framework builds a representation of the clock tree. Each clock should
be able to handle re-parenting on it's own, without knowing about the other
clocks in the tree, the parent is selected by a field in the clocks register.
It doesn't even have to know who the parents are, the clk framework will take
care of all of this and just say, ok, switch your input clock to X, where X is
a simple integer number The clk framework will also take care of re-calculating
all the updates frequencies after re-parenting.
> - once the clock controller is properly defined the clock connection should
> be contained in a dtsi which never changes. So, regarding remembering
> outputs, I don't see a problem. I rather see issues with having a pile of
> clocks described in the DT. I have currently 44 outputs proposed, and to
> model the whole tree a lot of more clocks are used (I started modeling
> everything with the clock primitives and removed custom clock
> implementations, except for the PLLs). Having all those in the DT does not
> really help, IMHO.
Well, except if you want to use external clocks for ethernet or CAN. Also you
don't need to change the dtsi either if you have a separate node for each clock.
To avoid misunderstandings let me clarify that I don't want to have one node
per clock, but rather one node per clock block (or whatever they are called).
The combination of input select + divider + output gate. E.g. the UART clock
block with it's 3 inputs and two outputs.
Also the APER clocks should probably be one node with 24 outputs.
- Lars
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: RFC: Zynq Clock Controller
On 03/08/2013 12:25 AM, Sören Brinkmann wrote:
> On Thu, Mar 07, 2013 at 11:02:58PM +0100, Lars-Peter Clausen wrote:
>> On 03/07/2013 08:11 PM, Sören Brinkmann wrote:
>>> On Thu, Mar 07, 2013 at 10:36:35AM +0100, Lars-Peter Clausen wrote:
>>>> On 03/06/2013 06:27 PM, Sören Brinkmann wrote:
>>>>> Hi Jan,
>>>>>
>>>>> what a small world. Good to hear from you.
>>>>>
>>>>> On Wed, Mar 06, 2013 at 12:51:21PM +0100, Jan Lübbe wrote:
>>>>>> Hi Sören,
>>>>>>
>>>>>> On Tue, 2013-03-05 at 12:04 -0800, Sören Brinkmann wrote:
>>>>>>> For this reasons, I'd like to propose moving Zynq into the same
>>>>>>> direction. I.e. adding a clock controller with the following DT
>>>>>>> description (details may change but the general idea should become
>>>>>>> clear):
>>>>>>> clkc: clkc {
>>>>>>> #clock-cells = <1>;
>>>>>>> compatible = "xlnx,ps7-clkc";
>>>>>>> ps_clk_frequency = <>; # board x-tal
>>>>>>> # optional props
>>>>>>> gem0_emio_clk_freq = <12500>;
>>>>>>> gem1_emio_clk_freq = <5000>;
>>>>>>> can_mio_clk_freq_xx = <1234>; # this is possible 54
>>>>>>> times with xx = 00..53
>>>>>>> };
>>>>
>>>> I definitely prefer the way it is right now in upstream, where we have one
>>>> dt node per clock. It is more descriptive and also more extensible. And you
>>>> also don't have to remember the clock index, and can use the phandle
>>>> directly instead.
>>> The problems I see with that are:
>>> - with the clock controller we can model the clock tree as we need without
>>> changing the DT all the time
>>
>> When do we need to change the clock tree? The clock tree is pretty much fixed
>> in hardware. And the DT describes the hardware, so I don't so a problem
>> there.
>>
>>> - w/o a clock controller there is no block which has knowledge of the
>>> whole clock tree (unless we parse the DT), and could conduct reparent
>>> operations and similar
>>
>> The clk framework builds a representation of the clock tree. Each clock
>> should
>> be able to handle re-parenting on it's own, without knowing about the other
>> clocks in the tree, the parent is selected by a field in the clocks register.
>> It doesn't even have to know who the parents are, the clk framework will take
>> care of all of this and just say, ok, switch your input clock to X, where X
>> is
>> a simple integer number The clk framework will also take care of
>> re-calculating
>> all the updates frequencies after re-parenting.
> Nope, clk_set_parent() takes two 'struct *clk' as argument. So, you have to
> have those of the clock to change and all its parents. A device driver for
> example, which gets a clock through clk_get() does not have that information
> and should not, since it should not have to be aware of the SOC's clock
> hierarchy, IMHO.
>
Yes, you global clk_set_parent() method takes two clk structs. But the clock
framework takes care of all the magic of mapping that second clk struct to a
integer number, based on the clks parent list. So your set_parent callback in
your clk_ops takes as parameters your clk and the index of the parent.
>>
>>> - once the clock controller is properly defined the clock connection
>>> should be contained in a dtsi which never changes. So, regarding
>>> remembering outputs, I don't see a problem. I rather see issues with having
>>> a pile of clocks described in the DT. I have currently 44 outputs proposed,
>>> and to model the whole tree a lot of more clocks are used (I started
>>> modeling everything with the clock primitives and removed custom clock
>>> implementations, except for the PLLs). Having all those in the DT does not
>>> really help, IMHO.
>>
>> Well, except if you want to use external clocks for ethernet or CAN. Also you
>> don't need to change the dtsi either if you have a separate node for each
>> clock.
>>
>> To avoid misunderstandings let me clarify that I don't want to have one node
>> per clock, but rather one node per clock block (or whatever they are called).
>&
Re: RFC: Zynq Clock Controller
On 03/08/2013 06:38 PM, Sören Brinkmann wrote:
> On Fri, Mar 08, 2013 at 08:12:20AM +0100, Lars-Peter Clausen wrote:
>> On 03/08/2013 12:25 AM, Sören Brinkmann wrote:
>>> On Thu, Mar 07, 2013 at 11:02:58PM +0100, Lars-Peter Clausen wrote:
>>>> On 03/07/2013 08:11 PM, Sören Brinkmann wrote:
>>>>> On Thu, Mar 07, 2013 at 10:36:35AM +0100, Lars-Peter Clausen wrote:
>>>>>> On 03/06/2013 06:27 PM, Sören Brinkmann wrote:
>>>>>>> Hi Jan,
>>>>>>>
>>>>>>> what a small world. Good to hear from you.
>>>>>>>
>>>>>>> On Wed, Mar 06, 2013 at 12:51:21PM +0100, Jan Lübbe wrote:
>>>>>>>> Hi Sören,
>>>>>>>>
>>>>>>>> On Tue, 2013-03-05 at 12:04 -0800, Sören Brinkmann wrote:
>>>>>>>>> For this reasons, I'd like to propose moving Zynq into the same
>>>>>>>>> direction. I.e. adding a clock controller with the following DT
>>>>>>>>> description (details may change but the general idea should become
>>>>>>>>> clear):
>>>>>>>>> clkc: clkc {
>>>>>>>>> #clock-cells = <1>;
>>>>>>>>> compatible = "xlnx,ps7-clkc";
>>>>>>>>> ps_clk_frequency = <>;# board x-tal
>>>>>>>>> # optional props
>>>>>>>>> gem0_emio_clk_freq = <12500>;
>>>>>>>>> gem1_emio_clk_freq = <5000>;
>>>>>>>>> can_mio_clk_freq_xx = <1234>; # this is possible 54
>>>>>>>>> times with xx = 00..53
>>>>>>>>> };
>>>>>>
>>>>>> I definitely prefer the way it is right now in upstream, where we have
>>>>>> one
>>>>>> dt node per clock. It is more descriptive and also more extensible. And
>>>>>> you
>>>>>> also don't have to remember the clock index, and can use the phandle
>>>>>> directly instead.
>>>>> The problems I see with that are:
>>>>> - with the clock controller we can model the clock tree as we need
>>>>> without changing the DT all the time
>>>>
>>>> When do we need to change the clock tree? The clock tree is pretty much
>>>> fixed
>>>> in hardware. And the DT describes the hardware, so I don't so a problem
>>>> there.
>>>>
>>>>> - w/o a clock controller there is no block which has knowledge of the
>>>>> whole clock tree (unless we parse the DT), and could conduct reparent
>>>>> operations and similar
>>>>
>>>> The clk framework builds a representation of the clock tree. Each clock
>>>> should
>>>> be able to handle re-parenting on it's own, without knowing about the other
>>>> clocks in the tree, the parent is selected by a field in the clocks
>>>> register.
>>>> It doesn't even have to know who the parents are, the clk framework will
>>>> take
>>>> care of all of this and just say, ok, switch your input clock to X, where
>>>> X is
>>>> a simple integer number The clk framework will also take care of
>>>> re-calculating
>>>> all the updates frequencies after re-parenting.
>>> Nope, clk_set_parent() takes two 'struct *clk' as argument. So, you have to
>>> have those of the clock to change and all its parents. A device driver for
>>> example, which gets a clock through clk_get() does not have that
>>> information and should not, since it should not have to be aware of the
>>> SOC's clock hierarchy, IMHO.
>>>
>>
>> Yes, you global clk_set_parent() method takes two clk structs. But the clock
>> framework takes care of all the magic of mapping that second clk struct to a
>> integer number, based on the clks parent list. So your set_parent callback in
>> your clk_ops takes as parameters your clk and the index of the parent.
> Right, but who/what in your scheme is supposed to call the high level API
> function requiring knowledge about all those 'struct *clk' and their
> hierarchy? This is where I think a big block has the advantage since it has a
> holistic view on the
Re: [PATCH v3] clk: add si5351 i2c common clock driver
On 03/18/2013 11:43 AM, Sebastian Hesselbarth wrote:
> This patch adds a common clock driver for Silicon Labs Si5351a/b/c
> i2c programmable clock generators. Currently, the driver supports
> DT kernels only and VXCO feature of si5351b is not implemented. DT
> bindings selectively allow to overwrite stored Si5351 configuration
> which is very helpful for clock generators with empty eeprom
> configuration. Corresponding device tree binding documentation is
> also added.
>
> Signed-off-by: Sebastian Hesselbarth
Hi,
Couple of comments inside.
> ---
> Changes from v2:
> - add curly brackets to if-else-statements (Reported by Daniel Mack)
> - fix div-by-zero for clk6/clk7 (Reported by Daniel Mack)
> - fix parameter address calculation for clk6/clk7
>
> Changes from v1:
> - remove .is_enabled functions as they read from i2c
> (Reported by Daniel Mack)
> - add CLK_SET_RATE_PARENT on clkout reparent if clkout uses
> its own multisync
[...]
> ---
> .../devicetree/bindings/clock/silabs,si5351.txt| 114 ++
> .../devicetree/bindings/vendor-prefixes.txt|1 +
> drivers/clk/Kconfig|9 +
> drivers/clk/Makefile |1 +
> drivers/clk/clk-si5351.c | 1429
>
> drivers/clk/clk-si5351.h | 155 +++
> 6 files changed, 1709 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/silabs,si5351.txt
> create mode 100644 drivers/clk/clk-si5351.c
> create mode 100644 drivers/clk/clk-si5351.h
>
> diff --git a/Documentation/devicetree/bindings/clock/silabs,si5351.txt
> b/Documentation/devicetree/bindings/clock/silabs,si5351.txt
> new file mode 100644
> index 000..f73d2d2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/silabs,si5351.txt
> @@ -0,0 +1,114 @@
> +Binding for Silicon Labs Si5351a/b/c programmable i2c clock generator.
> +
> +Reference
> +[1] Si5351A/B/C Data Sheet
> +http://www.silabs.com/Support%20Documents/TechnicalDocs/Si5351.pdf
> +
> +The Si5351a/b/c are programmable i2c clock generators with upto 8 output
> +clocks. Si5351a also has a reduced pin-count package (MSOP10) where only
> +3 output clocks are accessible. The internal structure of the clock
> +generators can be found in [1].
> +
> +==I2C device node==
> +
> +Required properties:
> +- compatible: shall be one of "silabs,si5351{a,a-msop,b,c}".
> +- reg: i2c device address, shall be 0x60 or 0x61.
> +- #clock-cells: from common clock binding; shall be set to 1.
> +- clocks: from common clock binding; list of parent clock
> + handles, shall be xtal reference clock or xtal and clkin for
> + si5351c only.
> +- #address-cells: shall be set to 1.
> +- #size-cells: shall be set to 0.
> +
> +Optional properties:
> +- pll-source: pair of (number, source) for each pll. Allows
> + to overwrite clock source of pll A (number=0) or B (number=1).
> +
> +==Child nodes==
> +
> +Each of the clock outputs can be overwritten individually by
> +using a child node to the I2C device node. If a child node for a clock
> +output is not set, the eeprom configuration is not overwritten.
> +
> +Required child node properties:
> +- reg: number of clock output.
> +
> +Optional child node properties:
> +- clock-source: source clock of the output divider stage N, shall be
> + 0 = multisynth N
> + 1 = multisynth 0 for output clocks 0-3, else multisynth4
> + 2 = xtal
> + 3 = clkin (si5351c only)
> +- drive-strength: output drive strength in mA, shall be one of {2,4,6,8}.
> +- multisynth-source: source pll A(0) or B(1) of corresponding multisynth
> + divider.
> +- pll-master: boolean, multisynth can change pll frequency.
Custom properties need a vendor prefix.
[...]
> diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
> new file mode 100644
> index 000..38540e7
> --- /dev/null
> +++ b/drivers/clk/clk-si5351.c
> @@ -0,0 +1,1429 @@
[...]
> +
> +/*
> + * Si5351 vxco clock input (Si5351B only)
> + */
> +
> +static int si5351_vxco_prepare(struct clk_hw *hw)
> +{
> + struct si5351_hw_data *hwdata =
> + container_of(hw, struct si5351_hw_data, hw);
> +
> + dev_warn(&hwdata->drvdata->client->dev, "VXCO currently unsupported\n");
Wouldn't it be better to not add the vxco clock if it is not supported?
> +
> + return 0;
> +}
> +
> +static void si5351_vxco_unprepare(struct clk_hw *hw)
> +{
> +}
> +
> +static unsigned long si5351_vxco_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + return 0;
> +}
> +
> +static int si5351_vxco_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent)
> +{
> + return 0;
> +}
> +
> +static const struct clk_ops si5351_vxco_ops = {
> + .prepare = si5351_vxco_prepare,
> + .unprepare = si5351_vxco_unprepare,
> + .recalc_rate = si5351_vxco_recalc_rate,
> + .set_rate = si5351_vxco_set_rate,
>
Re: RFC v2: Zynq Clock Controller
On 03/21/2013 12:56 AM, Sören Brinkmann wrote:
> Hi,
>
> I spent some time working on this and incorporating feedback. Here's an
> updated proposal for a clock controller for Zynq:
>
> Required properties:
> - #clock-cells : Must be 1
> - compatible : "xlnx,ps7-clkc" (this may become 'xlnx,zynq-clkc'
> terminology differs a bit between Xilinx internal and mainline)
> - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
> (usually 33 MHz oscillators are used for Zynq platforms)
> - clock-output-names : List of strings used to name the clock outputs. Shall
> be a list of the outputs given below.
>
> Optional properties:
> - clocks : as described in the clock bindings
> - clock-names : as described in the clock bindings
>
> Clock inputs:
> The following strings are optional parameters to the 'clock-names' property in
> order to provide optional (E)MIO clock sources.
> - swdt_ext_clk
> - gem0_emio_clk
> - gem1_emio_clk
> - mio_clk_XX # with XX = 00..53
>
> Example:
> clkc: clkc {
> #clock-cells = <1>;
> compatible = "xlnx,ps7-clkc";
> ps-clk-frequency = <>;
The input frequency should be a clock as well.
> clock-output-names = "armpll", "ddrpll", "iopll",
> "cpu_6or4x", "cpu_3or2x", "cpu_2x", "cpu_1x", "ddr2x", "ddr3x", "dci",
> "lqspi", "smc", "pcap", "gem0", "gem1", "fclk0", "fclk1", "fclk2", "fclk3",
> "can0", "can1", "sdio0", "sdio1", "uart0", "uart1", "spi0", "spi1", "dma",
> "usb0_aper", "usb1_aper", "gem0_aper", "gem1_aper", "sdio0_aper",
> "sdio1_aper", "spi0_aper", "spi1_aper", "can0_aper", "can1_aper",
> "i2c0_aper", "i2c1_aper", "uart0_aper", "uart1_aper", "gpio_aper",
> "lqspi_aper", "smc_aper", "swdt", "dbg_trc", "dbg_apb"; /* long list...
> explanation below */
> /* optional props */
> clocks = <&clkc 16>, <&clk_foo>;
> clock-names = "gem1_emio_clk", "can_mio_clk_23";
> };
>
> With this revised bindings arbitrary upstream and downstream clock providers
> should be supported and it's also possible to loop back an output as input.
> The downside of supporting this is, that I don't see a way around explicitly
> listing the clock output names in the DT.
> The reason for this is, that a downstream clock provider should use
> of_clk_get_parent_name() to obtain its parent clock name. For a block with
> multiple outputs of_clk_get_parent_name() can return a valid clock name only
> when 'clock-output-names' is present.
> Probably the fclks are the only realistic use case to become parent of
> downstream clock providers, but I could imagine that e.g. a device driver
> like UART wants to use the CCF to model its internal clocks, hence it would
> require its parent clock name. Even though a device driver could use
> clk_get_parent() and __clk_get_name(), of_clk_get_parent_name() should
> probably work as well. I simply have a bad feeling about breaking
> of_clk_get_parent_name() for any clock.
> But after all, I'm open for finding a better solution for this.
>
>
> Similar, inputs for optional clock sources through (E)MIO pins can be defined
> as described in the clock bindings using the 'clocks' and 'clock-names'
> properties, with 'clock-names' being an arbitrary subset of the documented
> names. The actual parent clock names are internally resolved using
> of_clk_get_parent_name().
>
>
> Regarding this monolithic solution versus a finer granular split:
>
> On cost of more complex probing we would also have:
> - one clock driver covering the peripheral clocks
> - one for the CPU clock domain
> - one for the DDR clock domain
> - one for GEM
> - one for CAN
> - one for the APER clks
> - one for the PLLs
> - one for fclks
fclks are the same as peripheral clocks except for the gate bit, as far as I
can see.
> - probably one for the debug clocks (not sure whether we need those)
>
> Except for the peripheral one and probably the fclk, they would all be
> instantiated just once. So, there is not a lot of reuse going on.
PLLs are going to be instantiated multiple times as well.
> Fclks I would probably also rather put into one driver with four outputs
> instead of instantiating a single output driver multiple times. Same for PLLs.
>
> And then there is e.g. a mux for the system watchdog input which doesn't
> really fit anywhere and it would be pretty much ridiculous to have another
> clock driver just for that one and it would also become "hidden" in one of
> the others.
>
> In my opinion that's just not necessary. We would create a bunch of clock
> drivers including DT bindings for them, probing would become more complex and
> it doesn't really help with the probe/naming issue. So, I don't think it's
> beneficial to go down that road.
>
> The monolithic solution would need one custom driver for the PLLs, DT
> bindings wouldn't be required for it though. Eve
Re: [PATCH v3] clk: add si5351 i2c common clock driver
On 03/21/2013 10:32 PM, Sebastian Hesselbarth wrote: >>> + >>> +dev_info(&client->dev, "registered si5351 i2c client\n"); >>> + >> >> That's just noise, imagine every driver would print such a line, your bootlog >> would be scrolling for hours ;) I'd either remove it or make it dev_dbg > > Actually, I understand not to have it, but if you don't want it you can still > boot with "quiet", can't you? Having one single line that helps you see it has > been probed helps a lot. But, I don't have a strong opinion on that and can > make it dev_dbg. It is useful during development, but in my opinion only as long as not every other driver does this as well. If you want to check whether a device has been probed you can easily do this by checking the drivers sysfs node. - Lars ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: RFC v2: Zynq Clock Controller
Hi,
On 03/22/2013 11:41 PM, Sören Brinkmann wrote:
> Hi Lars,
>
> On Thu, Mar 21, 2013 at 07:32:52PM +0100, Lars-Peter Clausen wrote:
>> On 03/21/2013 12:56 AM, Sören Brinkmann wrote:
>>> Hi,
>>>
>>> I spent some time working on this and incorporating feedback. Here's an
>>> updated proposal for a clock controller for Zynq:
>>>
>>> Required properties:
>>> - #clock-cells : Must be 1
>>> - compatible : "xlnx,ps7-clkc" (this may become 'xlnx,zynq-clkc'
>>> terminology differs a bit between Xilinx internal and mainline)
>>> - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
>>> (usually 33 MHz oscillators are used for Zynq
>>> platforms)
>>> - clock-output-names : List of strings used to name the clock outputs.
>>> Shall be a list of the outputs given below.
>>>
>>> Optional properties:
>>> - clocks : as described in the clock bindings
>>> - clock-names : as described in the clock bindings
>>>
>>> Clock inputs:
>>> The following strings are optional parameters to the 'clock-names' property
>>> in
>>> order to provide optional (E)MIO clock sources.
>>> - swdt_ext_clk
>>> - gem0_emio_clk
>>> - gem1_emio_clk
>>> - mio_clk_XX # with XX = 00..53
>>>
>>> Example:
>>> clkc: clkc {
>>> #clock-cells = <1>;
>>> compatible = "xlnx,ps7-clkc";
>>> ps-clk-frequency = <>;
>>
>> The input frequency should be a clock as well.
> Again, monolithic vs split. I don't see a reason not to just internally
> call clk_register_fixed_rate(). That way its children do not have to
> cope with a variable name for the xtal.
> Also, with my proposal 'clocks' and 'clock-names' would be purely
> optional properties, only required if optional external inputs are
> present. Having the xtal defined externally would add mandatory entries for
> those props.
>
>>
>>> clock-output-names = "armpll", "ddrpll", "iopll",
>>> "cpu_6or4x", "cpu_3or2x", "cpu_2x", "cpu_1x", "ddr2x", "ddr3x", "dci",
>>> "lqspi", "smc", "pcap", "gem0", "gem1", "fclk0", "fclk1", "fclk2", "fclk3",
>>> "can0", "can1", "sdio0", "sdio1", "uart0", "uart1", "spi0", "spi1", "dma",
>>> "usb0_aper", "usb1_aper", "gem0_aper", "gem1_aper", "sdio0_aper",
>>> "sdio1_aper", "spi0_aper", "spi1_aper", "can0_aper", "can1_aper",
>>> "i2c0_aper", "i2c1_aper", "uart0_aper", "uart1_aper", "gpio_aper",
>>> "lqspi_aper", "smc_aper", "swdt", "dbg_trc", "dbg_apb"; /* long list...
>>> explanation below */
>>> /* optional props */
>>> clocks = <&clkc 16>, <&clk_foo>;
>>> clock-names = "gem1_emio_clk", "can_mio_clk_23";
>>> };
>>>
>>> With this revised bindings arbitrary upstream and downstream clock
>>> providers should be supported and it's also possible to loop back an output
>>> as input. The downside of supporting this is, that I don't see a way around
>>> explicitly listing the clock output names in the DT.
>>> The reason for this is, that a downstream clock provider should use
>>> of_clk_get_parent_name() to obtain its parent clock name. For a block with
>>> multiple outputs of_clk_get_parent_name() can return a valid clock name
>>> only when 'clock-output-names' is present.
>>> Probably the fclks are the only realistic use case to become parent of
>>> downstream clock providers, but I could imagine that e.g. a device driver
>>> like UART wants to use the CCF to model its internal clocks, hence it would
>>> require its parent clock name. Even though a device driver could use
>>> clk_get_parent() and __clk_get_name(), of_clk_get_parent_name() should
>>> probably work as well. I simply have a bad feeling about breaking
>>> of_clk_get_parent_name() for an
Re: RFC v2: Zynq Clock Controller
On 03/25/2013 06:08 PM, Sören Brinkmann wrote:
> Hi Lars,
>
> On Mon, Mar 25, 2013 at 03:46:35PM +0100, Lars-Peter Clausen wrote:
>> Hi,
>>
>> On 03/22/2013 11:41 PM, Sören Brinkmann wrote:
>>> Hi Lars,
>>>
>>> On Thu, Mar 21, 2013 at 07:32:52PM +0100, Lars-Peter Clausen wrote:
>>>> On 03/21/2013 12:56 AM, Sören Brinkmann wrote:
>>>>> Hi,
>>>>>
>>>>> I spent some time working on this and incorporating feedback. Here's an
>>>>> updated proposal for a clock controller for Zynq:
>>>>>
>>>>> Required properties:
>>>>> - #clock-cells : Must be 1
>>>>> - compatible : "xlnx,ps7-clkc" (this may become 'xlnx,zynq-clkc'
>>>>> terminology differs a bit between Xilinx internal and mainline)
>>>>> - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
>>>>> (usually 33 MHz oscillators are used for Zynq
>>>>> platforms)
>>>>> - clock-output-names : List of strings used to name the clock outputs.
>>>>> Shall be a list of the outputs given below.
>>>>>
>>>>> Optional properties:
>>>>> - clocks : as described in the clock bindings
>>>>> - clock-names : as described in the clock bindings
>>>>>
>>>>> Clock inputs:
>>>>> The following strings are optional parameters to the 'clock-names'
>>>>> property in
>>>>> order to provide optional (E)MIO clock sources.
>>>>> - swdt_ext_clk
>>>>> - gem0_emio_clk
>>>>> - gem1_emio_clk
>>>>> - mio_clk_XX # with XX = 00..53
>>>>>
>>>>> Example:
>>>>> clkc: clkc {
>>>>> #clock-cells = <1>;
>>>>> compatible = "xlnx,ps7-clkc";
>>>>> ps-clk-frequency = <>;
>>>>
>>>> The input frequency should be a clock as well.
>>> Again, monolithic vs split. I don't see a reason not to just internally
>>> call clk_register_fixed_rate(). That way its children do not have to
>>> cope with a variable name for the xtal.
>>> Also, with my proposal 'clocks' and 'clock-names' would be purely
>>> optional properties, only required if optional external inputs are
>>> present. Having the xtal defined externally would add mandatory entries for
>>> those props.
>>
>>
>>
>>>
>>>>
>>>>> clock-output-names = "armpll", "ddrpll", "iopll",
>>>>> "cpu_6or4x", "cpu_3or2x", "cpu_2x", "cpu_1x", "ddr2x", "ddr3x", "dci",
>>>>> "lqspi", "smc", "pcap", "gem0", "gem1", "fclk0", "fclk1", "fclk2",
>>>>> "fclk3", "can0", "can1", "sdio0", "sdio1", "uart0", "uart1", "spi0",
>>>>> "spi1", "dma", "usb0_aper", "usb1_aper", "gem0_aper", "gem1_aper",
>>>>> "sdio0_aper", "sdio1_aper", "spi0_aper", "spi1_aper", "can0_aper",
>>>>> "can1_aper", "i2c0_aper", "i2c1_aper", "uart0_aper", "uart1_aper",
>>>>> "gpio_aper", "lqspi_aper", "smc_aper", "swdt", "dbg_trc", "dbg_apb"; /*
>>>>> long list... explanation below */
>>>>> /* optional props */
>>>>> clocks = <&clkc 16>, <&clk_foo>;
>>>>> clock-names = "gem1_emio_clk", "can_mio_clk_23";
>>>>> };
>>>>>
>>>>> With this revised bindings arbitrary upstream and downstream clock
>>>>> providers should be supported and it's also possible to loop back an
>>>>> output as input. The downside of supporting this is, that I don't see a
>>>>> way around explicitly listing the clock output names in the DT.
>>>>> The reason for this is, that a downstream clock provider should use
>>>>> of_clk_get_parent_name() to obtain its parent clock name.
Re: RFC v2: Zynq Clock Controller
On 03/25/2013 06:59 PM, Sören Brinkmann wrote:
> Hi,
>
> On Mon, Mar 25, 2013 at 06:19:11PM +0100, Lars-Peter Clausen wrote:
>> On 03/25/2013 06:08 PM, Sören Brinkmann wrote:
>>> Hi Lars,
>>>
>>> On Mon, Mar 25, 2013 at 03:46:35PM +0100, Lars-Peter Clausen wrote:
>>>> Hi,
>>>>
>>>> On 03/22/2013 11:41 PM, Sören Brinkmann wrote:
>>>>> Hi Lars,
>>>>>
>>>>> On Thu, Mar 21, 2013 at 07:32:52PM +0100, Lars-Peter Clausen wrote:
>>>>>> On 03/21/2013 12:56 AM, Sören Brinkmann wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> I spent some time working on this and incorporating feedback. Here's an
>>>>>>> updated proposal for a clock controller for Zynq:
>>>>>>>
>>>>>>> Required properties:
>>>>>>> - #clock-cells : Must be 1
>>>>>>> - compatible : "xlnx,ps7-clkc" (this may become 'xlnx,zynq-clkc'
>>>>>>> terminology differs a bit between Xilinx internal and mainline)
>>>>>>> - ps-clk-frequency : Frequency of the oscillator providing ps_clk in HZ
>>>>>>> (usually 33 MHz oscillators are used for Zynq
>>>>>>> platforms)
>>>>>>> - clock-output-names : List of strings used to name the clock outputs.
>>>>>>> Shall be a list of the outputs given below.
>>>>>>>
>>>>>>> Optional properties:
>>>>>>> - clocks : as described in the clock bindings
>>>>>>> - clock-names : as described in the clock bindings
>>>>>>>
>>>>>>> Clock inputs:
>>>>>>> The following strings are optional parameters to the 'clock-names'
>>>>>>> property in
>>>>>>> order to provide optional (E)MIO clock sources.
>>>>>>> - swdt_ext_clk
>>>>>>> - gem0_emio_clk
>>>>>>> - gem1_emio_clk
>>>>>>> - mio_clk_XX # with XX = 00..53
>>>>>>>
>>>>>>> Example:
>>>>>>> clkc: clkc {
>>>>>>> #clock-cells = <1>;
>>>>>>> compatible = "xlnx,ps7-clkc";
>>>>>>> ps-clk-frequency = <>;
>>>>>>
>>>>>> The input frequency should be a clock as well.
>>>>> Again, monolithic vs split. I don't see a reason not to just internally
>>>>> call clk_register_fixed_rate(). That way its children do not have to
>>>>> cope with a variable name for the xtal.
>>>>> Also, with my proposal 'clocks' and 'clock-names' would be purely
>>>>> optional properties, only required if optional external inputs are
>>>>> present. Having the xtal defined externally would add mandatory entries
>>>>> for
>>>>> those props.
>>>>
>>>>
>>>>
>>>>>
>>>>>>
>>>>>>> clock-output-names = "armpll", "ddrpll", "iopll",
>>>>>>> "cpu_6or4x", "cpu_3or2x", "cpu_2x", "cpu_1x", "ddr2x", "ddr3x", "dci",
>>>>>>> "lqspi", "smc", "pcap", "gem0", "gem1", "fclk0", "fclk1", "fclk2",
>>>>>>> "fclk3", "can0", "can1", "sdio0", "sdio1", "uart0", "uart1", "spi0",
>>>>>>> "spi1", "dma", "usb0_aper", "usb1_aper", "gem0_aper", "gem1_aper",
>>>>>>> "sdio0_aper", "sdio1_aper", "spi0_aper", "spi1_aper", "can0_aper",
>>>>>>> "can1_aper", "i2c0_aper", "i2c1_aper", "uart0_aper", "uart1_aper",
>>>>>>> "gpio_aper", "lqspi_aper", "smc_aper", "swdt", "dbg_trc", "dbg_apb";
>>>>>>> /* long list... explanation below */
>>>>>>> /* optional props */
>>>>>>> clocks = <&clkc 16>, <&am
Re: [PATCH v2 1/4] iio: adc: Document the regulator/clocks for exynos-adc
On 03/27/2013 07:35 PM, Naveen Krishna Ch wrote:
> On 13 March 2013 13:39, Doug Anderson wrote:
>> The exynos ADC won't work without a regulator called "vdd" and a clock
>> called "adc". Document this fact in the device tree bindings.
>>
>> Signed-off-by: Doug Anderson
> Reviewed-by: Naveen Krishna Chatradhi
>
> Lars, any update on this patch set. This change is required.
Uhm, looks fine to me. I'm sure Jonathan will pick it up :)
>> ---
>> Changes in v2: None
>>
>> Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt | 8
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
>> b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
>> index f686378..96db940 100644
>> --- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
>> +++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
>> @@ -20,6 +20,9 @@ Required properties:
>> format is being dependent on which interrupt
>> controller
>> the Samsung device uses.
>> - #io-channel-cells = <1>; As ADC has multiple outputs
>> +- clocks From common clock binding: handle to adc clock.
>> +- clock-names From common clock binding: Shall be "adc".
>> +- vdd-supply VDD input supply.
>>
>> Note: child nodes can be added for auto probing from device tree.
>>
>> @@ -31,6 +34,11 @@ adc: adc@12D1 {
>> interrupts = <0 106 0>;
>> #io-channel-cells = <1>;
>> io-channel-ranges;
>> +
>> + clocks = <&clock 303>;
>> + clock-names = "adc";
>> +
>> + vdd-supply = <&buck5_reg>;
>> };
>>
>>
>> --
>> 1.8.1.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>
>
>
> --
> Shine bright,
> (: Nav :)
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver
On 06/22/2013 01:55 PM, Jonathan Cameron wrote: > On 06/20/2013 07:57 PM, Alexandre Belloni wrote: >> The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable >> gain and sampling rates. >> > Sorry, somewhat low on time today so only a quick review. > > 1) Missing userspace ABI documentation. Also, perhaps min_conversions is >a little vague? Not that I have a better idea! I really don't like the name min_conversions either. Isn't this effectively a decimation filter? - Lars ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver
On 06/22/2013 03:07 PM, Alexandre Belloni wrote: > On 22/06/2013 14:02, Lars-Peter Clausen wrote: >> On 06/22/2013 01:55 PM, Jonathan Cameron wrote: >>> On 06/20/2013 07:57 PM, Alexandre Belloni wrote: >>>> The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable >>>> gain and sampling rates. >>>> >>> Sorry, somewhat low on time today so only a quick review. >>> >>> 1) Missing userspace ABI documentation. Also, perhaps min_conversions is >>>a little vague? Not that I have a better idea! >> I really don't like the name min_conversions either. Isn't this effectively >> a decimation filter? > > Yeah, it could be seen like that but it is only relevant and only > happens when switching between channels. I'm open to any ideas. > I see. Is there anything about this in the datasheet on how many conversions you usually need? Is this really something you need to change at runtime or does moving this to platform data work? - Lars ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver
On 06/22/2013 03:28 PM, Alexandre Belloni wrote: > On 22/06/2013 15:20, Lars-Peter Clausen wrote: >> On 06/22/2013 03:07 PM, Alexandre Belloni wrote: >>> On 22/06/2013 14:02, Lars-Peter Clausen wrote: >>>> On 06/22/2013 01:55 PM, Jonathan Cameron wrote: >>>>> On 06/20/2013 07:57 PM, Alexandre Belloni wrote: >>>>>> The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable >>>>>> gain and sampling rates. >>>>>> >>>>> Sorry, somewhat low on time today so only a quick review. >>>>> >>>>> 1) Missing userspace ABI documentation. Also, perhaps min_conversions is >>>>>a little vague? Not that I have a better idea! >>>> I really don't like the name min_conversions either. Isn't this effectively >>>> a decimation filter? >>> Yeah, it could be seen like that but it is only relevant and only >>> happens when switching between channels. I'm open to any ideas. >>> >> I see. Is there anything about this in the datasheet on how many conversions >> you usually need? Is this really something you need to change at runtime or >> does moving this to platform data work? >> >> > > There is actually nothing in the datasheet. The default value (6 > conversions) was found experimentally. What I did was saturating the ADC > with the higher value on one channel and the lower value on the other > one and I tried to find when reading both channel sequentially was > resulting in a correct value. > > You may not need to change it at runtime. And that value mainly depend > on the precision versus speed balance you want to achieve. If you know > that the values on both channels will not be to far apart, then you may > not need to wait at all. > > Would you think that is something I should hide in the DT ? Or maybe I > can drop that knob for now and see if it is needed in the future. > It is always a good idea to be conservative when introducing new ABI, so if you think we can get away with hardcoding this in the driver I think that's a good idea. - Lars ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver
On 06/20/2013 08:57 PM, Alexandre Belloni wrote:
> The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
> gain and sampling rates.
>
> Signed-off-by: Alexandre Belloni
> Signed-off-by: Maxime Ripard
> ---
> .../bindings/iio/adc/nuvoton-nau7802.txt | 17 +
> drivers/iio/adc/Kconfig| 9 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/nau7802.c | 603
> +
> 4 files changed, 630 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
> create mode 100644 drivers/iio/adc/nau7802.c
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
> b/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
> new file mode 100644
> index 000..9bc4218
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
> @@ -0,0 +1,17 @@
> +* Nuvoton NAU7802 Analog to Digital Converter (ADC)use
> +
> +Required properties:
> + - compatible: Should be "nuvoton,nau7802"
> + - reg: Should contain the ADC I2C address
> +
> +Optional properties:
> + - nuvoton,vldo: Reference voltage in millivolts (integer)
> + - interrupts: IRQ line for the ADC. If not used the driver will use
> +polling.
> +
> +Example:
> +adc2: nau7802@2a {
> + compatible = "nuvoton,nau7802";
> + reg = <0x2a>;
> + nuvoton,vldo = <3000>;
We usually use the regulator framework for specifying the reference voltage.
> +};
[...]
> diff --git a/drivers/iio/adc/nau7802.c b/drivers/iio/adc/nau7802.c
> new file mode 100644
> index 000..e1b6981
> --- /dev/null
> +++ b/drivers/iio/adc/nau7802.c
> @@ -0,0 +1,603 @@
[...]
> +static int nau7802_set_gain(struct nau7802_state *st, int gain)
> +{
> + u8 data;
> + int ret;
> +
> + mutex_lock(&st->lock);
> + st->conversion_count = 0;
> +
> + data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_CTRL1);
> + if (data < 0)
> + goto nau7802_sysfs_set_gain_out;
ret will be uninitialized if the goto above is taken
> + ret = i2c_smbus_write_byte_data(st->client, NAU7802_REG_CTRL1,
> + (data & (~NAU7802_CTRL1_GAINS_BITS)) |
> + gain);
> +
> +nau7802_sysfs_set_gain_out:
> + mutex_unlock(&st->lock);
> +
> + return ret ? ret : 0;
> +}
[...]
> +static int nau7802_read_irq(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val)
> +{
> + struct nau7802_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + INIT_COMPLETION(st->value_ok);
> + enable_irq(st->client->irq);
Is it really necessary to enable/disable the IRQ or could you keep it
enabled all the time?
> +
> + nau7802_sync(st);
> +
> + /* read registers to ensure we flush everything */
> + ret = nau7802_read_conversion(st);
> + if (ret < 0)
> + goto read_chan_info_failure;
> +
> + /* Wait for a conversion to finish */
> + ret = wait_for_completion_interruptible_timeout(&st->value_ok,
> + msecs_to_jiffies(1000));
> + if (ret == 0)
> + ret = -ETIMEDOUT;
> +
> + if (ret < 0)
> + goto read_chan_info_failure;
> +
> + disable_irq(st->client->irq);
> +
> + *val = st->last_value;
> +
> + return IIO_VAL_INT;
> +
> +read_chan_info_failure:
> + disable_irq(st->client->irq);
> +
> + return ret;
> +}
[...]
[...]
> +static int nau7802_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct iio_dev *indio_dev;
> + struct nau7802_state *st;
> + struct device_node *np = client->dev.of_node;
> + int i, ret;
> + u8 data;
> + u32 tmp;
> +
> + if (!client->dev.of_node) {
> + dev_err(&client->dev, "No device tree node available.\n");
> + return -EINVAL;
> + }
Except for getting the vref the is no direct dependency on devicetree, if
you switch to the regulator framework for the vref this check can be removed.
[...]
> + /* Setup the ADC channels available on the board */
> + indio_dev->num_channels = 2;
ARRAY_SIZE(nau7802_chan_array)
> + indio_dev->channels = nau7802_chan_array;
> +
> + init_completion(&st->value_ok);
You need to initialize the completion before requesting the IRQ handler.
[...]
> +}
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver
On 06/24/2013 12:37 PM, Alexandre Belloni wrote:
> On 24/06/2013 08:41, Lars-Peter Clausen wrote:
>> On 06/20/2013 08:57 PM, Alexandre Belloni wrote:
>>> The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
>>> gain and sampling rates.
>>>
>>> Signed-off-by: Alexandre Belloni
>>> Signed-off-by: Maxime Ripard
>>> ---
>>> .../bindings/iio/adc/nuvoton-nau7802.txt | 17 +
>>> drivers/iio/adc/Kconfig| 9 +
>>> drivers/iio/adc/Makefile | 1 +
>>> drivers/iio/adc/nau7802.c | 603
>>> +
>>> 4 files changed, 630 insertions(+)
>>> create mode 100644
>>> Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
>>> create mode 100644 drivers/iio/adc/nau7802.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
>>> b/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
>>> new file mode 100644
>>> index 000..9bc4218
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
>>> @@ -0,0 +1,17 @@
>>> +* Nuvoton NAU7802 Analog to Digital Converter (ADC)use
>>> +
>>> +Required properties:
>>> + - compatible: Should be "nuvoton,nau7802"
>>> + - reg: Should contain the ADC I2C address
>>> +
>>> +Optional properties:
>>> + - nuvoton,vldo: Reference voltage in millivolts (integer)
>>> + - interrupts: IRQ line for the ADC. If not used the driver will use
>>> +polling.
>>> +
>>> +Example:
>>> +adc2: nau7802@2a {
>>> + compatible = "nuvoton,nau7802";
>>> + reg = <0x2a>;
>>> + nuvoton,vldo = <3000>;
>> We usually use the regulator framework for specifying the reference voltage.
>
> I followed what Jonathan said in his review of my first patch. Do we
> want to use the regulator framework to set the internal reference
> voltage of the ADC ? I agree that if you supply an external voltage, it
> will be necessary to use the regulator framework. Unfortunately, I can't
> test that here.
>
Ah, ok I missed that it is an internally generated voltage. It might makes
sense to add that to the properties documentation.
I guess ideally you'd also register a regulator for the internal regulator
and then use that. But I think that will unnecessarily complicate the code,
so I guess the current solution is fine.
There is one bug in probe though, if nuvoton,vldo is not set tmp will remain
uninitialized.
>>> +};
>> [...]
>>> diff --git a/drivers/iio/adc/nau7802.c b/drivers/iio/adc/nau7802.c
>>> new file mode 100644
>>> index 000..e1b6981
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/nau7802.c
>>> @@ -0,0 +1,603 @@
>> [...]
>>> +static int nau7802_set_gain(struct nau7802_state *st, int gain)
>>> +{
>>> + u8 data;
>>> + int ret;
>>> +
>>> + mutex_lock(&st->lock);
>>> + st->conversion_count = 0;
>>> +
>>> + data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_CTRL1);
>>> + if (data < 0)
>>> + goto nau7802_sysfs_set_gain_out;
>> ret will be uninitialized if the goto above is taken
>
> Right, bigger issue, data is u8 so it will never be negative. I'm fixing
> that !
>
>
>>> + ret = i2c_smbus_write_byte_data(st->client, NAU7802_REG_CTRL1,
>>> + (data & (~NAU7802_CTRL1_GAINS_BITS)) |
>>> + gain);
>>> +
>>> +nau7802_sysfs_set_gain_out:
>>> + mutex_unlock(&st->lock);
>>> +
>>> + return ret ? ret : 0;
>>> +}
>> [...]
>>> +static int nau7802_read_irq(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int *val)
>>> +{
>>> + struct nau7802_state *st = iio_priv(indio_dev);
>>> + int ret;
>>> +
>>> + INIT_COMPLETION(st->value_ok);
>>> + enable_irq(st->client->irq);
>> Is it really necessary to enable/disable the IRQ or could you keep it
>> enabled all the time?
>
> Fact is that the ADC doesn't really care if you are reading data or not
> so you will probably endd up in a situation were you will get 320 IRQ
> per second but not caring about the result. We have 3 ADCs on the board.
> so that amounts to 960 IRQ per second when we are only reading like once
> par second !
Ah, ok, makes sense.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 3/4] hwmon: Add a simple driver to read the MXS SoC temperature
On 06/27/2013 09:26 PM, Alexandre Belloni wrote: > Hi, > > On 27/06/2013 16:27, Guenter Roeck wrote: >> On Thu, Jun 27, 2013 at 11:17:32AM +0200, Maxime Ripard wrote: >>> On Wed, Jun 26, 2013 at 07:39:27AM -0700, Guenter Roeck wrote: On Wed, Jun 26, 2013 at 10:51:12AM +0200, Alexandre Belloni wrote: > The low resolution ADC of the mxs is able to read an internal temperature > sensor, expose that using hwmon. > > Signed-off-by: Alexandre Belloni > --- Wouldn't it make more sense to use iio-hwmon and improve it if necessary ? >>> Actually, I wonder if we should not just put the hwmon driver >>> capabilities directly into the mxs-lradc driver, just like it's already >>> been done in this driver for the touchscreen support. >>> >>> The probing of this hwmon driver doesn't really belong to the DT, it's >>> not really realistic to probe it from the machine definition, and it >>> really is the IP that is wired that way. >>> >> Merging iio-hwmon functionality into an adc driver seems just as bad >> (or even worse) as copying it into a new driver. >> >> If the lradc driver knows that the ADC channels are temperature sensors, it >> should register them with the iio subsystem as IIO_TEMP type. Then you should >> be able to use iio_hwmon as is. > > They are already registered as IIO_TEMP but only implement read_raw. Also, > > iio_hwmon_read_val() is using iio_read_channel_processed() and that will > basically only read one of the 2 channels. As I documented, you actually > need to read both channel 8 and channel 9 and then compute the value in > Kelvins. I'm not sure how you want me to do that in the current framework. What are these two channels actually measuring? Is the value of a single channel meaningful on it's own? If not it might make sense to update the IIO driver to just have one temperature channel. - Lars ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 3/4] hwmon: Add a simple driver to read the MXS SoC temperature
On 06/28/2013 04:50 PM, Alexandre Belloni wrote: > On 28/06/2013 16:18, Lars-Peter Clausen wrote: >> On 06/27/2013 09:26 PM, Alexandre Belloni wrote: >>> >>> They are already registered as IIO_TEMP but only implement read_raw. Also, >>> >>> iio_hwmon_read_val() is using iio_read_channel_processed() and that will >>> basically only read one of the 2 channels. As I documented, you actually >>> need to read both channel 8 and channel 9 and then compute the value in >>> Kelvins. I'm not sure how you want me to do that in the current framework. >> What are these two channels actually measuring? Is the value of a single >> channel meaningful on it's own? If not it might make sense to update the IIO >> driver to just have one temperature channel. > > It's not actually meaningful on its own. So, what you would do is expose > one iio channel for two ADC channels and do the computation in read_raw > ? or read_processed ? Then using iio-hwon to export it. ? > > Regards, > Yes, return channel9 - channel8 as the raw value for the temperature channel and provide proper scale and offset values, so that iio_read_channel_processed() will return the correct value. - Lars ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [alsa-devel] [PATCH] ASoC: Add PCM1681 codec driver.
On 06/26/2013 03:05 PM, Marek Belisko wrote:
[...]
> +static int pcm1681_deemph[] = { 44100, 48000, 32000 };
const
> +
> +static int pcm1681_set_deemph(struct snd_soc_codec *codec)
> +{
> + struct pcm1681_private *priv = snd_soc_codec_get_drvdata(codec);
> + int i = 0, val = -1, ret;
> +
> + if (priv->deemph)
> + for (i = 0; i < ARRAY_SIZE(pcm1681_deemph); i++)
> + if (pcm1681_deemph[i] == priv->rate)
> + val = i;
> +
> + /* enable choosen frequency */
> + if (val != -1)
> + regmap_update_bits(priv->regmap, PCM1681_DEEMPH_CONTROL,
> + PCM1681_DEEMPH_RATE_MASK, val);
So if the current sample rate doesn't match any of the available deempth rates
the current setting is kept. This doesn't seem right.
> +
> + /* enable/disable deemphasis functionality */
> + ret = regmap_update_bits(priv->regmap, PCM1681_DEEMPH_CONTROL,
> + PCM1681_DEEMPH_MASK, priv->deemph);
> +
> + return ret;
> +}
> +
[...]
> +
> +static int pcm1681_digital_mute(struct snd_soc_dai *dai, int mute)
> +{
> + struct snd_soc_codec *codec = dai->codec;
> + struct pcm1681_private *priv = snd_soc_codec_get_drvdata(codec);
> + int ret, val = 0;
> +
> + if (mute)
> + val = PCM1681_SOFT_MUTE_ALL;
> +
> + ret = regmap_write(priv->regmap, PCM1681_SOFT_MUTE, val);
How about just
return regmap_write(...)
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
[...]
> +static const DECLARE_TLV_DB_SCALE(pcm1681_dac_tlv, -6350, 50, 1);
> +
> +static const struct snd_kcontrol_new pcm1681_controls[] = {
> + SOC_DOUBLE_R_TLV("PCM1681 Channel 1/2 Playback Volume",
> + PCM1681_ATT_CONTROL(1), PCM1681_ATT_CONTROL(2), 0,
> + 0x7f, 0, pcm1681_dac_tlv),
> + SOC_DOUBLE_R_TLV("PCM1681 Channel 3/4 Playback Volume",
> + PCM1681_ATT_CONTROL(3), PCM1681_ATT_CONTROL(4), 0,
> + 0x7f, 0, pcm1681_dac_tlv),
> + SOC_DOUBLE_R_TLV("PCM1681 Channel 5/6 Playback Volume",
> + PCM1681_ATT_CONTROL(5), PCM1681_ATT_CONTROL(6), 0,
> + 0x7f, 0, pcm1681_dac_tlv),
> + SOC_DOUBLE_R_TLV("PCM1681 Channel 7/8 Playback Volume",
> + PCM1681_ATT_CONTROL(7), PCM1681_ATT_CONTROL(8), 0,
> + 0x7f, 0, pcm1681_dac_tlv),
> + SOC_SINGLE_BOOL_EXT("PCM1681 De-emphasis Switch", 0,
> + pcm1681_get_deemph, pcm1681_put_deemph),
> +};
The name of the codec should probably not be in the control names.
[...]
> +#ifdef CONFIG_OF
> +static const struct of_device_id pcm1681_dt_ids[] = {
> + { .compatible = "ti,pcm1681", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, pcm1681_dt_ids);
> +#endif
> +
> +static const struct regmap_config pcm1681_regmap = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = ARRAY_SIZE(pcm1681_reg_defaults),
max_register is the last register in the register map, so usually this would be
ARRAY_SIZE(...) + 1
> + .reg_defaults = pcm1681_reg_defaults,
> + .num_reg_defaults = ARRAY_SIZE(pcm1681_reg_defaults),
> + .writeable_reg = pcm1681_writeable_reg,
> + .readable_reg = pcm1681_accessible_reg,
> +};
[...]
> +
> +static struct snd_soc_codec_driver soc_codec_dev_pcm1681 = {
> + .probe = pcm1681_probe,
> + .remove = pcm1681_remove,
> + .controls = pcm1681_controls,
> + .num_controls = ARRAY_SIZE(pcm1681_controls),
> +};
> +
> +#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
Since the driver only supports I2C there is no need for this ifdef
[...]
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 4/4] iio: lps331ap: Add support for DT
On 07/02/2013 02:15 PM, Lukasz Czerwinski wrote:
> From: Jacek Anaszewski
>
> This patch adds DT support for the lps331ap barometer
> sensor.
>
> Signed-off-by: Jacek Anaszewski
> Signed-off-by: Kyungmin Park
> ---
> .../bindings/iio/pressure/st_pressure.txt | 41
>
> drivers/iio/pressure/st_pressure_i2c.c |9 +
> drivers/iio/pressure/st_pressure_spi.c |9 +
> 3 files changed, 59 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/iio/pressure/st_pressure.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/pressure/st_pressure.txt
> b/Documentation/devicetree/bindings/iio/pressure/st_pressure.txt
> new file mode 100644
> index 000..73a4b7d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/pressure/st_pressure.txt
> @@ -0,0 +1,41 @@
> +* STMicroelectronics LPS331AP barometer sensor
> +
> +Required properties:
> +
> + - compatible : should be "lps331ap"
Needs the vendor prefix.
> + - reg : the I2C address of the barometer
> +
> +Optional properties:
> +
> + - drdy-int-pin : redirect DRDY on pin INT1 (1) or pin INT2 (2) (u8)
> + - interrupt-parent : phandle to the interrupt map subnode
> + - interrupts : interrupt mapping for LPS331AP interrupt sources:
> + 2 sources: 0 - INT1, 1 - INT2
> + - irq-map : irq sub-node defining interrupt map
> + (all properties listed below are required):
> + - #interrupt-cells : should be 1
> + - #address-cells : should be 0
> + - #size-cells : should be 0
> + - interrupt-map : table of entries consisting of three child elements:
> + - unit_interrupt_specifier - 0 : INT1, 1 : INT2
> + - interrupt parent phandle
> + - parent unit interrupt specifier consisiting of two elements:
> + - index of the interrupt within the controller
> + - flags : should be 0
While this works I wonder why you choose such a complicated example for setting
up the IRQ? Why not just reference the IRQ directly using the interrupts
property?
> +
> +Example:
> +
> +lps331ap@5d {
> + compatible = "lps331ap";
> + reg = <0x5d>;
> + drdy-int-pin = /bits/ 8 <2>;
> + interrupt-parent = <&irq_map>;
> + interrupts = <0>, <1>;
> +
> + irq_map: irq-map {
> + #interrupt-cells = <1>;
> + #address-cells = <0>;
> + #size-cells = <0>;
> + interrupt-map = <0 &gpf0 5 0>;
> + };
> +};
> diff --git a/drivers/iio/pressure/st_pressure_i2c.c
> b/drivers/iio/pressure/st_pressure_i2c.c
> index 7cebcc7..a1ad3cf 100644
> --- a/drivers/iio/pressure/st_pressure_i2c.c
> +++ b/drivers/iio/pressure/st_pressure_i2c.c
> @@ -61,10 +61,19 @@ static const struct i2c_device_id st_press_id_table[] = {
> };
> MODULE_DEVICE_TABLE(i2c, st_press_id_table);
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id lps331ap_of_match[] = {
> + { .compatible = LPS331AP_PRESS_DEV_NAME, },
I think it is better to write the name out. The name the device binds to should
be fixed and not depend on some macro.
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, lps331ap_of_match);
> +#endif
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 1/1] Added Capella CM3218 Ambient Light Sensor IIO Driver.
On 07/04/2013 01:31 AM, Kevin Tsai wrote:
Maybe write at least a short commit message which states the features of the
chip.
> Signed-off-by: Kevin Tsai
> ---
> drivers/staging/iio/light/Kconfig | 10 +
> drivers/staging/iio/light/Makefile |1 +
> drivers/staging/iio/light/cm3218.c | 581
>
> 3 files changed, 592 insertions(+)
> create mode 100644 drivers/staging/iio/light/cm3218.c
>
> diff --git a/drivers/staging/iio/light/Kconfig
> b/drivers/staging/iio/light/Kconfig
> index ca8d6e6..647af0c 100644
> --- a/drivers/staging/iio/light/Kconfig
> +++ b/drivers/staging/iio/light/Kconfig
> @@ -40,4 +40,14 @@ config TSL2x7x
>tmd2672, tsl2772, tmd2772 devices.
>Provides iio_events and direct access via sysfs.
>
> +config SENSORS_CM3218
> +tristate "CM3218 Ambient Light Sensor"
> +depends on I2C
> +help
> + Say Y here if you have a Capella Micro CM3218 Ambient Light Sensor.
> +
> + To compile this driver as a module, choose M here. This module
> + will be called to 'cm3218'. It will access ALS data via iio sysfs.
> + This is recommended.
> +
Keep the entries in alphabetical order.
> endmenu
> diff --git a/drivers/staging/iio/light/Makefile
> b/drivers/staging/iio/light/Makefile
> index 9960fdf..63020ab 100644
> --- a/drivers/staging/iio/light/Makefile
> +++ b/drivers/staging/iio/light/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_SENSORS_ISL29018)+= isl29018.o
> obj-$(CONFIG_SENSORS_ISL29028) += isl29028.o
> obj-$(CONFIG_TSL2583)+= tsl2583.o
> obj-$(CONFIG_TSL2x7x)+= tsl2x7x_core.o
> +obj-$(CONFIG_SENSORS_CM3218)+= cm3218.o
Same here
> diff --git a/drivers/staging/iio/light/cm3218.c
> b/drivers/staging/iio/light/cm3218.c
> new file mode 100644
> index 000..9c2584d
> --- /dev/null
> +++ b/drivers/staging/iio/light/cm3218.c
> @@ -0,0 +1,581 @@
[...]
> +#include
> +#include
> +#include
> +#include
> +#include
delay.h seems to be unused
> +#include
> +#include
> +#include
[...]
> +
> +static int cm3218_write(struct i2c_client *client, u8 reg, u16 value)
> +{
> + u16 regval;
> + int ret;
> + struct cm3218_chip *chip = iio_priv(i2c_get_clientdata(client));
> +
> +#ifdef CM3218_DEBUG
> + dev_err(&client->dev,
> + "Write to device register 0x%02X with 0x%04X\n", reg, value);
> +#endif /* CM3218_DEBUG */
> + regval = cpu_to_le16(value);
> + ret = i2c_smbus_write_word_data(client, reg, regval);
This looks wrong, with this code the on-wire byteorder will differ between
big endian and little endian systems. Maybe you want
i2c_smbus_write_word_swapped?
But as Peter said, just use regmap for all IO.
> + if (ret) {
> + dev_err(&client->dev, "Write to device fails: 0x%x\n", ret);
> + } else {
> + /* Update cache */
> + if (reg < CM3218_MAX_CACHE_REGS)
> + chip->reg_cache[reg] = value;
> + }
> +
> + return ret;
[...]
> +
> +static int cm3218_detect(struct i2c_client *client,
> +struct i2c_board_info *info)
> +{
> + struct i2c_adapter *adapter = client->adapter;
> + const char *name = NULL;
> +
> + cm3218_read_ara(client);
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> + return -ENODEV;
> +
> + name = "cm3218";
> + strlcpy(info->type, name, I2C_NAME_SIZE);
> +
Always returning zero means the chip will bind to any I2C devices which
happen to use the same address. I don't think you actually need detection,
so just remove it.
> + return 0;
> +}
> +
> +static const struct i2c_device_id cm3218_id[] = {
> + {"cm3218", 0},
> + {}
> +};
> +
Nitpick: no need for the newline
> +MODULE_DEVICE_TABLE(i2c, cm3218_id);
> +
> +static const struct of_device_id cm3218_of_match[] = {
> + { .compatible = "invn,cm3218", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, cm3218_of_match);
> +
> +static struct i2c_driver cm3218_driver = {
> + .class = I2C_CLASS_HWMON,
> + .driver = {
> + .name = "cm3218",
> + .pm = CM3218_PM_OPS,
> + .owner = THIS_MODULE,
> + .of_match_table = cm3218_of_match,
> + },
Indention looks a bit strange here. Please use one tab per level.
> + .probe = cm3218_probe,
> + .remove = cm3218_remove,
> + .id_table = cm3218_id,
> + .detect = cm3218_detect,
> + .address_list = normal_i2c,
> +};
> +module_i2c_driver(cm3218_driver);
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCHv3 1/3] iio: Add Nuvoton NAU7802 ADC driver
On 06/24/2013 07:24 PM, Alexandre Belloni wrote: > The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable > gain and sampling rates. > > Signed-off-by: Alexandre Belloni > Signed-off-by: Maxime Ripard Reviewed-by: Lars-Peter Clausen One remark though. Multiline comments should be like /* * foo * bar */ not /* foo * bar */ But not need to resend the patch just for this - Lars ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [alsa-devel] [PATCH v2 1/5] sound: sam9x5_wm8731: machine driver for at91sam9x5 wm8731 boards
On 07/08/2013 03:29 PM, Richard Genoud wrote:
[...]
> +/*
> + * Logic for a wm8731 as connected on a at91sam9x5 based board.
> + */
> +static int at91sam9x5ek_wm8731_init(struct snd_soc_pcm_runtime *rtd)
> +{
[...]
> + codec_dai->driver->playback.rates &= SNDRV_PCM_RATE_8000 |
> + SNDRV_PCM_RATE_32000 |
> + SNDRV_PCM_RATE_48000 |
> + SNDRV_PCM_RATE_96000;
> + codec_dai->driver->capture.rates &= SNDRV_PCM_RATE_8000 |
> + SNDRV_PCM_RATE_32000 |
> + SNDRV_PCM_RATE_48000 |
> + SNDRV_PCM_RATE_96000;
That's not right. The driver structure is shared between all instances of
the codec, a single instance should not modify it. If you need to constrain
the list of supported rates use snd_pcm_hw_constraint_list().
> +
> + /* set the codec system clock for DAC and ADC */
> + ret = snd_soc_dai_set_sysclk(codec_dai, WM8731_SYSCLK_XTAL,
> + MCLK_RATE, SND_SOC_CLOCK_IN);
> + if (ret < 0) {
> + dev_err(dev, "ASoC: Failed to set WM8731 SYSCLK: %d\n", ret);
> + return ret;
> + }
> +
> + /* signal a DAPM event */
> + snd_soc_dapm_sync(dapm);
This should not be necessary.
> + return 0;
> +}
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH RFC 0/8] MPC512x DMA slave s/g support, OF DMA lookup
On 07/12/2013 05:26 PM, Gerhard Sittig wrote: [...] > Lars, there is a checkpatch warning about a line longer than 80 > characters in the common xlate part -- I didn't dare to change your > submission, and the part included here is verbatim from your patchwork > 2331091 (original submission) and 2555701 (resend), is there a followup > which you can provide or point us to? Well the line is 81 characters long, so I wouldn't worry to much about that. But we should probably remove the extern from all the function declarations in dma-of.h, since the extern is kind of implicit for functions declarations. - Lars ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] iio: add APDS9300 ambilent light sensor driver
On 07/10/2013 03:08 PM, Oleksandr Kravchenko wrote:
> From: Oleksandr Kravchenko
>
> This patch adds IIO driver for APDS9300 ambilent light sensor (ALS).
s/ambilent/ambient/
> http://www.avagotech.com/docs/AV02-1077EN
>
> The driver allows to read raw data from ADC registers or calculate
> lux value. It also can handle threshold inrerrupt.
s/inrerrupt/interrupt/
The patch looks very good in general, a couple of comment inline.
>
> Signed-off-by: Oleksandr Kravchenko
> ---
> .../devicetree/bindings/iio/light/apds9300.txt | 22 +
> drivers/iio/light/Kconfig | 10 +
> drivers/iio/light/Makefile |1 +
> drivers/iio/light/apds9300.c | 528
>
> 4 files changed, 561 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/light/apds9300.txt
> create mode 100644 drivers/iio/light/apds9300.c
>
> diff --git a/Documentation/devicetree/bindings/iio/light/apds9300.txt
> b/Documentation/devicetree/bindings/iio/light/apds9300.txt
> new file mode 100644
> index 000..d6f66c7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/apds9300.txt
> @@ -0,0 +1,22 @@
> +* Avago APDS9300 ambient light sensor
> +
> +http://www.avagotech.com/docs/AV02-1077EN
> +
> +Required properties:
> +
> + - compatible : should be "avago,apds9300"
You should also add the avago vendor prefix to
Documentation/devicetree/bindings/vendor-prefixes.txt. Preferably in a
separate patch.
> + - reg : the I2C address of the sensor
> +
> +Optional properties:
> +
> + - interrupt-parent : should be the phandle for the interrupt controller
> + - interrupts : interrupt mapping for GPIO IRQ
> +
> +Example:
> +
> +apds9300@39 {
> + compatible = "avago,apds9300";
> + reg = <0x39>;
> + interrupt-parent = <&gpio2>;
> + interrupts = <29 8>;
> +};
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 5ef1a39..08a6742 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -52,6 +52,16 @@ config VCNL4000
>To compile this driver as a module, choose M here: the
>module will be called vcnl4000.
>
> +config APDS9300
> + tristate "APDS9300 ambient light sensor"
> + depends on I2C
> + help
> + Say Y here if you want to build a driver for the Avago APDS9300
> + ambient light sensor.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called apds9300.
> +
Keeps this in alphabetical order
> config HID_SENSOR_ALS
> depends on HID_SENSOR_HUB
> select IIO_BUFFER
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 040d9c7..da58e12 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -6,4 +6,5 @@ obj-$(CONFIG_ADJD_S311) += adjd_s311.o
> obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o
> obj-$(CONFIG_SENSORS_TSL2563)+= tsl2563.o
> obj-$(CONFIG_VCNL4000) += vcnl4000.o
> +obj-$(CONFIG_APDS9300) += apds9300.o
Same here
> obj-$(CONFIG_HID_SENSOR_ALS) += hid-sensor-als.o
> diff --git a/drivers/iio/light/apds9300.c b/drivers/iio/light/apds9300.c
> new file mode 100644
> index 000..2275ecc
> --- /dev/null
> +++ b/drivers/iio/light/apds9300.c
> @@ -0,0 +1,528 @@
> +/*
> + * apds9300.c - IIO driver for Avago APDS9300 ambient light sensor
> + *
> + * Copyright 2013 Oleksandr Kravchenko
> + *
> + * This file is subject to the terms and conditions of version 2 of
> + * the GNU General Public License. See the file COPYING in the main
> + * directory of this archive for more details.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
No device driver should ever need to include irq.h
> +#include
> +#include
> +#include
> +
[...]
> +
> +static unsigned long als_calculate_lux(u16 ch0, u16 ch1)
> +{
> + unsigned long lux, tmp;
> + u64 tmp64;
> +
> + /* avoid division by zero */
> + if (ch0 == 0)
> + return 0;
> +
> + tmp = ch1 * 100 / ch0;
> + if (tmp <= 52) {
> + /*
> + * Variable tmp64 need to avoid overflow of this part of lux
> + * calculation formula.
> + */
If you want to avoid the overflow you have to do the math as 64bit math. As
it is right now it will do 32bit math and only store the result in a 64 bit
variable.
> + tmp64 = ch0 * lux_ratio[tmp] * 5930 / 1000;
> + lux = 3150 * ch0 - (unsigned long)tmp64;
> + }
> + else if (tmp <= 65)
> + lux = 2290 * ch0 - 2910 * ch1;
> + else if (tmp <= 80)
> + lux = 1570 * ch0 - 1800 * ch1;
> + else if (tmp <= 130)
> + lux = 338 * ch0 - 260 * ch1;
> + else
> + lux = 0;
> +
> + return lux / 10;
> +}
> +
[...]
> +static int als_get_adc_val(struct als_data *data, int adc_number)
>
Re: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
A couple of comments inline.
On 07/12/2013 09:18 AM, Oleksandr Kozaruk wrote:
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ab0767e6..87d699e 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -157,4 +157,12 @@ config VIPERBOARD_ADC
Say yes here to access the ADC part of the Nano River
Technologies Viperboard.
+config TWL6030_GPADC
+ tristate "TWL6030 GPADC (General Purpose A/D Convertor) Support"
+ depends on TWL4030_CORE
+ default n
+ help
+ Say yes here if you want support for the TWL6030 General Purpose
+ A/D Convertor.
+
Keep it in alphabetical order
endmenu
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0a825be..8b05633 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_MAX1363) += max1363.o
obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
+obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
Same here.
diff --git a/drivers/iio/adc/twl6030-gpadc.c b/drivers/iio/adc/twl6030-gpadc.c
new file mode 100644
index 000..6ceb789
--- /dev/null
+++ b/drivers/iio/adc/twl6030-gpadc.c
@@ -0,0 +1,1019 @@
[...]
+static u8 twl6032_channel_to_reg(int channel)
+{
+ return TWL6032_GPADC_GPCH0_LSB;
There is more than one channel, isn't there?
[...]
> +static int twl6030_gpadc_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + int *val, int *val2, long mask)
> +{
> + struct twl6030_gpadc_data *gpadc = iio_priv(indio_dev);
> + int ret = -EINVAL;
> +
> + mutex_lock(&gpadc->lock);
> +
> + ret = gpadc->pdata->start_conversion(chan->channel);
> + if (ret) {
> + dev_err(gpadc->dev, "failed to start conversion\n");
> + goto err;
> + }
> + /* wait for conversion to complete */
> + wait_for_completion_interruptible_timeout(&gpadc->irq_complete,
> + msecs_to_jiffies(5000));
wait_for_completion_interruptible_timeout() can return either a negative error
code if it was interrupted, 0 if it timed out, or a positive value if it was
successfully completed. You need to handle all three cases. Have a look at
other existing drivers to see how to do this properly.
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = twl6030_gpadc_get_raw(gpadc, chan->channel, val);
> + ret = ret ? -EIO : IIO_VAL_INT;
> + break;
> +
> + case IIO_CHAN_INFO_PROCESSED:
> + ret = twl6030_gpadc_get_processed(gpadc, chan->channel, val);
> + ret = ret ? -EIO : IIO_VAL_INT;
> + break;
> +
> + default:
> + break;
> + }
> +err:
> + mutex_unlock(&gpadc->lock);
> +
> + return ret;
> +}
+}
+
[...]
+static int twl6030_gpadc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct twl6030_gpadc_data *gpadc;
+ const struct twl6030_gpadc_platform_data *pdata;
+ const struct of_device_id *match;
+ struct iio_dev *indio_dev;
+ int irq;
+ int ret = 0;
+
+ match = of_match_device(of_match_ptr(of_twl6030_match_tbl), dev);
+ pdata = match ? match->data : dev->platform_data;
+
+ if (!pdata)
+ return -EINVAL;
+
+ indio_dev = iio_device_alloc(sizeof(struct twl6030_gpadc_data));
sizeof(*gpadc)
+ if (!indio_dev) {
+ dev_err(dev, "failed allocating iio device\n");
+ ret = -ENOMEM;
+ }
+
+ gpadc = iio_priv(indio_dev);
+
+ gpadc->twl6030_cal_tbl = devm_kzalloc(dev,
+ sizeof(struct twl6030_chnl_calib) *
+ pdata->nchannels, GFP_KERNEL);
+ if (!gpadc->twl6030_cal_tbl)
+ goto err;
+
+ gpadc->dev = dev;
+ gpadc->pdata = pdata;
+
+ platform_set_drvdata(pdev, indio_dev);
+ mutex_init(&gpadc->lock);
+ init_completion(&gpadc->irq_complete);
+
+ ret = pdata->calibrate(gpadc);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to read calibration registers\n");
+ goto err;
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "failed to get irq\n");
+ goto err;
+ }
+
+ ret = devm_request_threaded_irq(dev, irq, NULL,
+ twl6030_gpadc_irq_handler,
+ IRQF_ONESHOT, "twl6030_gpadc", gpadc);
You access memory in the interrupt handler which is freed before the interrupt
handler is freed.
+ if (ret) {
+ dev_dbg(&pdev->dev, "could not request irq\n");
+ goto err;
+ }
+
+ ret = twl6030_gpadc_enable_irq(TWL6030_GPADC_RT_SW1_EOC_MASK);
+ if (ret < 0) {
+ dev
Re: [PATCH RFC 5/8] dma: mpc512x: use symbolic specifiers for DMA channels
On 07/14/2013 10:50 AM, Arnd Bergmann wrote: > On Saturday 13 July 2013, Gerhard Sittig wrote: >> [ MPC8308 knowledge required, see below ] >> >> On Sat, Jul 13, 2013 at 09:17 +0200, Arnd Bergmann wrote: >>> >>> On Friday 12 July 2013, Gerhard Sittig wrote: +++ b/include/dt-bindings/dma/mpc512x-dma.h @@ -0,0 +1,21 @@ +/* + * This header file provides symbolic specifiers for DMA channels + * within the MPC512x SoC's DMA controller. Since requester lines + * directly map to channel numbers and no additional flexibility + * is involved, DMA channels can be considered directly associated + * with individual peripherals. + * + * This header file gets shared among DT bindings which provide + * hardware specs, and driver code which implements supporting logic. + */ + +#ifndef _DT_BINDINGS_DMA_MPC512x_DMA_H +#define _DT_BINDINGS_DMA_MPC512x_DMA_H + +#define MPC512x_DMACHAN_SCLPC 26 +#define MPC512x_DMACHAN_SDHC 30 +#define MPC512x_DMACHAN_MDDRC 32 + +#define MPC512x_DMACHAN_MAX64 + >>> >>> I think these should not be in the header and should not bve part of the >>> binding either. They are specific to an SoC that happens to be using this >>> DMA controller but would be completely different for any other SoC with >>> the same DMA engine. These belong into the dma descriptors of the slave >>> drivers and don't need symbolic names. >> >> Thank you for the feedback. >> >> OK, so not adding the dt-bindings header leads to no change in >> the DTS nodes, which in turn collapses 5/8 into something local >> to the .c driver source (introduce an enum and replace a few >> magic numbers with names), and obsoletes 4/8 as a prerequisite. >> This will further reduce the patch set's size. > > Actually I think you will need extra changes: The dma-engine driver > should not require knowledge of any channel-specific settings. > I did not notice you had them until you mentioned the above, but > from what I can tell, you need a few flags in the dma-specifier > to replace code like > > /* only start explicitly on MDDRC channel */ > - if (cid == 32) > + if (cid == MPC512x_DMACHAN_MDDRC) > mdesc->tcd->start = 1; > > with > > mdesc->tcd->start = dmaspec->explicit_start; > > or something along these lines, where dmaspec is a data structure > derived from the fields in the DT dma specifier of the child > node. > I think the MDDRC channel is used for mem-to-mem transfers. So there is no peripheral that is going to request it by handle. If the channel that is used for mem-to-mem transfers differs between different instances of the controller (e.g. on different SoCs) it should probably be a property of the dma controllers DT node. - Lars ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH RFC 1/8] powerpc: mpc512x_dma: add support for data transfers between memory and i/o memory
On 07/12/2013 05:26 PM, Gerhard Sittig wrote: [...] ] > + if (mchan->tcd_nunits) > + tcd->nbytes = mchan->tcd_nunits * 4; > + else > + tcd->nbytes = 64; Just wondering where does the magic 64 come from? ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
On 07/15/2013 01:09 PM, Kozaruk, Oleksandr wrote: [...] > >>> + ret = devm_request_threaded_irq(dev, irq, NULL, >>> + twl6030_gpadc_irq_handler, >>> + IRQF_ONESHOT, "twl6030_gpadc", gpadc); >> >> You access memory in the interrupt handler which is freed before the >> interrupt >> handler is freed. > Thanks for pointing this. devm_* will free memory for irq after the driver > is removed and memory for the device is freed. I took me awhile to understand > this. Is there going to be something like devm_iio_device_alloc? whould it be > helpfull? > Yes, I think it certainly makes sense to add a devm_iio_device_alloc(), care to send a patch? ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
On 07/15/2013 01:56 PM, Grygorii Strashko wrote:
> Hi All,
>
> I have a question regarding this patch and IIO in general
> - Does IIO provide sync mechanism with system wide suspend/resume or this
> should be handled by each driver itself?
>
> What if during system suspend iio_read_channel_raw() (or any other consumer
> API) will be called after gpadc driver have been suspended already? (I did
> some investigation and seems it's possible - Am I right?)
>
> If no, could "info_exist_lock" be reused for such purposes?
I think you can use the mlock for this purpose. Usually you'd have a flag in
your driver struct which you'd set to true in suspend and to false in
resume. And in the read_raw callback you'd check that flag before accessing
the hardware. If it turns out that this is a common pattern it probably
makes sense to add infrastructure for this to the IIO core. Something along
the lines of:
static int drv_suspend(...)
{
...
iio_device_set_suspended(iio_dev, true);
...
}
static int drv_suspend(...)
{
...
iio_device_set_suspended(iio_dev, false);
...
}
and then have the IIO core check that flag before calling the read_raw callback.
- Lars
>
> Regards,
> -grygorii
>
>
> On 07/12/2013 10:56 PM, Lars-Peter Clausen wrote:
>>
>> A couple of comments inline.
>>
>> On 07/12/2013 09:18 AM, Oleksandr Kozaruk wrote:
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index ab0767e6..87d699e 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -157,4 +157,12 @@ config VIPERBOARD_ADC
>>> Say yes here to access the ADC part of the Nano River
>>> Technologies Viperboard.
>>>
>>> +config TWL6030_GPADC
>>> +tristate "TWL6030 GPADC (General Purpose A/D Convertor) Support"
>>> +depends on TWL4030_CORE
>>> +default n
>>> +help
>>> + Say yes here if you want support for the TWL6030 General Purpose
>>> + A/D Convertor.
>>> +
>>
>> Keep it in alphabetical order
>>
>>> endmenu
>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>> index 0a825be..8b05633 100644
>>> --- a/drivers/iio/adc/Makefile
>>> +++ b/drivers/iio/adc/Makefile
>>> @@ -17,3 +17,4 @@ obj-$(CONFIG_MAX1363) += max1363.o
>>> obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>>> obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
>>> obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
>>> +obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
>>
>> Same here.
>>
>>> diff --git a/drivers/iio/adc/twl6030-gpadc.c
>>> b/drivers/iio/adc/twl6030-gpadc.c
>>> new file mode 100644
>>> index 000..6ceb789
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/twl6030-gpadc.c
>>> @@ -0,0 +1,1019 @@
>> [...]
>>> +static u8 twl6032_channel_to_reg(int channel)
>>> +{
>>> +return TWL6032_GPADC_GPCH0_LSB;
>>
>> There is more than one channel, isn't there?
>>
>> [...]
>> > +static int twl6030_gpadc_read_raw(struct iio_dev *indio_dev,
>> > + const struct iio_chan_spec *chan,
>> > + int *val, int *val2, long mask)
>> > +{
>> > +struct twl6030_gpadc_data *gpadc = iio_priv(indio_dev);
>> > +int ret = -EINVAL;
>> > +
>> > +mutex_lock(&gpadc->lock);
>> > +
>> > +ret = gpadc->pdata->start_conversion(chan->channel);
>> > +if (ret) {
>> > +dev_err(gpadc->dev, "failed to start conversion\n");
>> > +goto err;
>> > +}
>> > +/* wait for conversion to complete */
>> > +wait_for_completion_interruptible_timeout(&gpadc->irq_complete,
>> > +msecs_to_jiffies(5000));
>>
>> wait_for_completion_interruptible_timeout() can return either a negative
>> error code if it was interrupted, 0 if it timed out, or a positive value
>> if it was successfully completed. You need to handle all three cases.
>> Have a look at other existing drivers to see how to do this properly.
>>
>> > +
>> > +switch (mask) {
>> > +case IIO_CHAN_INFO_RAW:
>> > +ret = twl6030_gpadc_get_raw(gpadc, chan->channel, val);
>> > +ret = ret ? -EIO : IIO_VAL_INT;
>> > +break;
>> > +
>> > +case IIO_CHAN_INFO_PROCESSED:
>> >
Re: [PATCH] iio: add APDS9300 ambilent light sensor driver
On 07/15/2013 02:27 PM, Oleksandr Kravchenko wrote:
> Thank you for review! But I don't completely understand one of your comment:
>
>>> +static int als_probe(struct i2c_client *client, const struct i2c_device_id
>>> *id)
> [...]
>>> + if (client->irq) {
>>> + ret = devm_request_threaded_irq(&client->dev, client->irq,
>>> + NULL, als_interrupt_handler,
>>> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>>> + ALS_IRQ_NAME, indio_dev);
>>
>> This is a bit racy, you access memory in the irq handler that is freed
>> before the irq is freed.
>
> Do you mean than that indio_dev may be used in interrupt handler after
> iio_device_free(indio_dev) called in als_remove() function?
>
> If so, can I use disable_irq() in als_remove() before iio_device_free()
> to avoid this problem?
>
Just add a devm_iio_device_alloc() and use that, instead of trying to bodch
around the issue.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] iio: add APDS9300 ambilent light sensor driver
On 07/15/2013 04:54 PM, Oleksandr Kravchenko wrote:
> I can't to find devm_iio_device_alloc() in my kernel v3.11-rc1
>
It doesn't exist yet, but it should be too hard to implement one.
- Lars
> On Mon, Jul 15, 2013 at 3:35 PM, Lars-Peter Clausen wrote:
>> On 07/15/2013 02:27 PM, Oleksandr Kravchenko wrote:
>>> Thank you for review! But I don't completely understand one of your comment:
>>>
>>>>> +static int als_probe(struct i2c_client *client, const struct
>>>>> i2c_device_id *id)
>>> [...]
>>>>> + if (client->irq) {
>>>>> + ret = devm_request_threaded_irq(&client->dev, client->irq,
>>>>> + NULL, als_interrupt_handler,
>>>>> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>>>>> + ALS_IRQ_NAME, indio_dev);
>>>>
>>>> This is a bit racy, you access memory in the irq handler that is freed
>>>> before the irq is freed.
>>>
>>> Do you mean than that indio_dev may be used in interrupt handler after
>>> iio_device_free(indio_dev) called in als_remove() function?
>>>
>>> If so, can I use disable_irq() in als_remove() before iio_device_free()
>>> to avoid this problem?
>>>
>>
>> Just add a devm_iio_device_alloc() and use that, instead of trying to bodch
>> around the issue.
>>
>
>
>
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH RFC v2 2/5] dma: mpc512x: add support for peripheral transfers
On 07/14/2013 02:01 PM, Gerhard Sittig wrote:
> From: Alexander Popov
>
> introduce support for slave s/g transfer preparation and the associated
> device control callback in the MPC512x DMA controller driver, which adds
> support for data transfers between memory and peripheral I/O to the
> previously supported mem-to-mem transfers
>
> refuse to prepare chunked transfers (transfers with more than one part)
> as long as proper support for scatter/gather is lacking
>
> keep MPC8308 operational by always starting transfers from software,
> this SoC appears to not have request lines for flow control when
> peripherals are involved in transfers
I had a look at the current driver and it seems that any channel can be used
for memcpy operation not only the MDDRC channel. Since the dmaengine API
will just pick one of the currently free channels when performing a memcpy
operation I think this patch breaks memcpy operations. You probably need to
register two dma controllers, one for memcpy operations one for slave
operations, that way you can ensure that only the MDDRC channel is used for
memcpy operations.
>
> [ introduction of slave s/g preparation and device control ]
> Signed-off-by: Alexander Popov
> [ execute() start condition, mpc8308 compat, terminate all, s/g length check,
> reworded commit msg ]
> Signed-off-by: Gerhard Sittig
> ---
[...]
> +
> +static int mpc_dma_device_control(struct dma_chan *chan, enum dma_ctrl_cmd
> cmd,
> + unsigned long arg)
> +{
> + struct mpc_dma_chan *mchan;
> + struct mpc_dma *mdma;
> + struct dma_slave_config *cfg;
> +
> + mchan = dma_chan_to_mpc_dma_chan(chan);
> + switch (cmd) {
> + case DMA_TERMINATE_ALL:
> + /* disable channel requests */
> + mdma = dma_chan_to_mpc_dma(chan);
> + out_8(&mdma->regs->dmacerq, chan->chan_id);
> + list_splice_tail_init(&mchan->prepared, &mchan->free);
> + list_splice_tail_init(&mchan->queued, &mchan->free);
> + list_splice_tail_init(&mchan->active, &mchan->free);
This probably need locking.
> + return 0;
[...]
> +}
> +
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5 1/2] ARM: dts: twl: Add GPADC data to device tree
On 07/17/2013 04:33 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 17-07-2013 15:12, Oleksandr Kozaruk wrote:
>
>> GPADC is the general purpose ADC present on twl6030.
>> The dt data is interrupt used to trigger end of ADC
>> conversion.
>
>> Signed-off-by: Oleksandr Kozaruk
>> ---
>> arch/arm/boot/dts/twl6030.dtsi | 6 ++
>> 1 file changed, 6 insertions(+)
>
>> diff --git a/arch/arm/boot/dts/twl6030.dtsi b/arch/arm/boot/dts/twl6030.dtsi
>> index 2e3bd31..322aa8e 100644
>> --- a/arch/arm/boot/dts/twl6030.dtsi
>> +++ b/arch/arm/boot/dts/twl6030.dtsi
>> @@ -103,4 +103,10 @@
>> compatible = "ti,twl6030-pwmled";
>> #pwm-cells = <2>;
>> };
>> +
>> +adc: twl6030_gpadc {
>
>I was talking about the device name, not label. The "twl6030_gpadc" part.
The compatible property should also be: 'twl6030-gpadc' instead of
'twl6030_gpadc' and you need to add documentation for it.
- Lars
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
On 07/17/2013 03:45 PM, Oleksandr Kozaruk wrote:
> On Mon, Jul 15, 2013 at 01:33:53PM +0200, Lars-Peter Clausen wrote:
>> On 07/15/2013 01:09 PM, Kozaruk, Oleksandr wrote:
>> [...]
>> >
>> >>> + ret = devm_request_threaded_irq(dev, irq, NULL,
>> >>> + twl6030_gpadc_irq_handler,
>> >>> + IRQF_ONESHOT, "twl6030_gpadc", gpadc);
>> >>
>> >> You access memory in the interrupt handler which is freed before the
> interrupt
>> >> handler is freed.
>> > Thanks for pointing this. devm_* will free memory for irq after the driver
>> > is removed and memory for the device is freed. I took me awhile to
> understand
>> > this. Is there going to be something like devm_iio_device_alloc? whould
> it be helpfull?
>> >
>>
>> Yes, I think it certainly makes sense to add a devm_iio_device_alloc(), care
>> to send a patch?
>
> Anything like this? (of course it's not a patch)
>
No. I think you can for example use devm_regulator_get() as a template. But
instead of regulator_get() and regulator_put() use iio_device_alloc() and
iio_device_free().
> struct iio_dev *devm_iio_device_alloc(struct device *dev, int sizeof_priv)
> {
> struct iio_dev *indio_dev;
> size_t alloc_size;
>
> alloc_size = sizeof(struct iio_dev);
> if (sizeof_priv) {
> alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> alloc_size += sizeof_priv;
> }
> /* ensure 32-byte alignment of whole construct ? */
> alloc_size += IIO_ALIGN - 1;
>
> indio_dev = devm_kzalloc(dev, alloc_size, GFP_KERNEL);
> if (indio_dev) {
> indio_dev->dev.groups = indio_dev->groups;
> indio_dev->dev.type = &iio_device_type;
> indio_dev->dev.bus = &iio_bus_type;
> device_initialize(&indio_dev->dev);
> dev_set_drvdata(&indio_dev->dev, (void *)indio_dev);
> mutex_init(&indio_dev->mlock);
> mutex_init(&indio_dev->info_exist_lock);
> INIT_LIST_HEAD(&indio_dev->channel_attr_list);
>
> indio_dev->id = ida_simple_get(&iio_ida, 0, 0, GFP_KERNEL);
> if (indio_dev->id < 0) {
> /* cannot use a dev_err as the name isn't available */
> printk(KERN_ERR "Failed to get id\n");
> kfree(dev);
> return NULL;
> }
> dev_set_name(&indio_dev->dev, "iio:device%d", indio_dev->id);
> INIT_LIST_HEAD(&indio_dev->buffer_list);
> }
>
> return indio_dev;
> }
> EXPORT_SYMBOL(devm_iio_device_alloc);
>
> Regards,
> OK
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
On 07/17/2013 01:12 PM, Oleksandr Kozaruk wrote:
> The GPADC is general purpose ADC found on TWL6030, and TWL6032 PMIC,
> known also as Phoenix and PhoenixLite.
>
> The TWL6030 and TWL6032 have GPADC with 17 and 19 channels
> respectively. Some channels have current source and are used for
> measuring voltage drop on resistive load for detecting battery ID
> resistance, or measuring voltage drop on NTC resistors for external
> temperature measurements. Some channels measure voltage, (i.e. battery
> voltage), and have voltage dividers, thus, capable to scale voltage.
> Some channels are dedicated for measuring die temperature.
>
> Some channels are calibrated in 2 points, having offsets from ideal
> values kept in trim registers. This is used to correct measurements.
>
> The differences between GPADC in TWL6030 and TWL6032:
> - 10 bit vs 12 bit ADC;
> - 17 vs 19 channels;
> - channels have different purpose(i.e. battery voltage
> channel 8 vs channel 18);
> - trim values are interpreted differently.
>
> Based on the driver patched from Balaji TK, Graeme Gregory, Ambresh K,
> Girish S Ghongdemath.
>
> Signed-off-by: Balaji T K
> Signed-off-by: Graeme Gregory
> Signed-off-by: Oleksandr Kozaruk
Almost there (I hope). But please run scripts/checkpatch, make C=2 and make
coccicheck on your driver and fix all errors those tools give you before
submitting the driver upstream.
[...]
> +static int twl6030_gpadc_get_raw(struct twl6030_gpadc_data *gpadc,
> + int channel, int *res)
> +{
> + u8 reg = gpadc->pdata->channel_to_reg(channel);
> + __le16 val;
> + int raw_code;
> + int ret;
> +
> + ret = twl6030_gpadc_read(reg, (u8 *)&val);
> + if (ret) {
> + dev_dbg(gpadc->dev, "unable to read register 0x%X\n", reg);
> + return ret;
> + }
> +
> + raw_code = le16_to_cpup(&val);
raw_code = le16_to_cpu(val)
> + dev_dbg(gpadc->dev, "GPADC raw code: %d", raw_code);
> +
> + if (twl6030_channel_calibrated(gpadc->pdata, channel))
> + *res = twl6030_gpadc_make_correction(gpadc, channel, raw_code);
> + else
> + *res = raw_code;
> +
> + return ret;
> +}
> +
[...]
> +static int twl6030_gpadc_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + int *val, int *val2, long mask)
> +{
> + struct twl6030_gpadc_data *gpadc = iio_priv(indio_dev);
> + int ret = -EINVAL;
> + long timeout;
> +
> + mutex_lock(&gpadc->lock);
> +
> + ret = gpadc->pdata->start_conversion(chan->channel);
> + if (ret) {
> + dev_err(gpadc->dev, "failed to start conversion\n");
> + goto err;
> + }
> + /* wait for conversion to complete */
> + timeout = wait_for_completion_interruptible_timeout(
> + &gpadc->irq_complete, msecs_to_jiffies(5000));
> + if (!timeout)
> + return -ETIMEDOUT;
> + else if (timeout < 0)
> + return EINTR;
You still hold the mutex, try this instead:
ret = wait_for_
if (ret == 0)
ret = -ETIMEDOUT;
if (ret < 0)
goto err;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = twl6030_gpadc_get_raw(gpadc, chan->channel, val);
> + ret = ret ? -EIO : IIO_VAL_INT;
> + break;
> +
> + case IIO_CHAN_INFO_PROCESSED:
> + ret = twl6030_gpadc_get_processed(gpadc, chan->channel, val);
> + ret = ret ? -EIO : IIO_VAL_INT;
> + break;
> +
> + default:
> + break;
> + }
> +err:
> + mutex_unlock(&gpadc->lock);
> +
> + return ret;
> +}
[...]
> +
> +static int twl6032_calibration(struct twl6030_gpadc_data *gpadc)
> +{
> + int chn, d1 = 0, d2 = 0, temp;
> + u8 trim_regs[17];
> + int ret;
> +
> + ret = twl_i2c_read(TWL6030_MODULE_ID2, trim_regs + 1,
> + TWL6030_GPADC_TRIM1, 16);
> + if (ret < 0) {
> + dev_err(gpadc->dev, "calibration failed\n");
> + return ret;
> + }
> +
> + /*
> + * Loop to calculate the value needed for returning voltages from
> + * GPADC not values.
> + *
> + * gain is calculated to 3 decimal places fixed point.
> + */
> + for (chn = 0; chn < TWL6032_GPADC_MAX_CHANNELS; chn++) {
> +
> + switch (chn) {
> + case 0:
> + case 1:
> + case 2:
> + case 3:
> + case 4:
> + case 5:
> + case 6:
> + case 11:
> + case 12:
> + case 13:
> + case 14:
> + /* D1 */
> + d1 = (trim_regs[3] & 0x1F) << 2;
> + d1 |= (trim_regs[1] & 0x06) >> 1;
> + if (trim_regs[1] & 0x01)
> + d1 = -d1;
> +
> + /* D2 */
> +
Re: [PATCH v5 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
On 07/18/2013 10:36 AM, Oleksandr Kozaruk wrote:
> Hello Lars,
>
> On Wed, Jul 17, 2013 at 9:04 PM, Lars-Peter Clausen wrote:
>>> +static int twl6032_calibration(struct twl6030_gpadc_data *gpadc)
>>> +{
>>> + int chn, d1 = 0, d2 = 0, temp;
>>> + u8 trim_regs[17];
>>> + int ret;
>>> +
>>> + ret = twl_i2c_read(TWL6030_MODULE_ID2, trim_regs + 1,
>>> + TWL6030_GPADC_TRIM1, 16);
>>> + if (ret < 0) {
>>> + dev_err(gpadc->dev, "calibration failed\n");
>>> + return ret;
>>> + }
>>> +
>>> + /*
>>> + * Loop to calculate the value needed for returning voltages from
>>> + * GPADC not values.
>>> + *
>>> + * gain is calculated to 3 decimal places fixed point.
>>> + */
>>> + for (chn = 0; chn < TWL6032_GPADC_MAX_CHANNELS; chn++) {
>>> +
>>> + switch (chn) {
>>> + case 0:
>>> + case 1:
>>> + case 2:
>>> + case 3:
>>> + case 4:
>>> + case 5:
>>> + case 6:
>>> + case 11:
>>> + case 12:
>>> + case 13:
>>> + case 14:
>>> + /* D1 */
>>> + d1 = (trim_regs[3] & 0x1F) << 2;
>>> + d1 |= (trim_regs[1] & 0x06) >> 1;
>>> + if (trim_regs[1] & 0x01)
>>> + d1 = -d1;
>>> +
>>> + /* D2 */
>>> + d2 = (trim_regs[4] & 0x3F) << 2;
>>> + d2 |= (trim_regs[2] & 0x06) >> 1;
>>> + if (trim_regs[2] & 0x01)
>>> + d2 = -d2;
>>> + break;
>>> + case 8:
>>> + /* D1 */
>>> + temp = (trim_regs[3] & 0x1F) << 2;
>>> + temp |= (trim_regs[1] & 0x06) >> 1;
>>> + if (trim_regs[1] & 0x01)
>>> + temp = -temp;
>>> +
>>> + d1 = (trim_regs[8] & 0x18) << 1;
>>> + d1 |= (trim_regs[7] & 0x1E) >> 1;
>>> + if (trim_regs[7] & 0x01)
>>> + d1 = -d1;
>>> +
>>> + d1 += temp;
>>> +
>>> + /* D2 */
>>> + temp = (trim_regs[4] & 0x3F) << 2;
>>> + temp |= (trim_regs[2] & 0x06) >> 1;
>>> + if (trim_regs[2] & 0x01)
>>> + temp = -temp;
>>> +
>>> + d2 = (trim_regs[10] & 0x1F) << 2;
>>> + d2 |= (trim_regs[8] & 0x06) >> 1;
>>> + if (trim_regs[8] & 0x01)
>>> + d2 = -d2;
>>> +
>>> + d2 += temp;
>>> + break;
>>> + case 9:
>>> + /* D1 */
>>> + temp = (trim_regs[3] & 0x1F) << 2;
>>> + temp |= (trim_regs[1] & 0x06) >> 1;
>>> + if (trim_regs[1] & 0x01)
>>> + temp = -temp;
>>> +
>>> + d1 = (trim_regs[14] & 0x18) << 1;
>>> + d1 |= (trim_regs[12] & 0x1E) >> 1;
>>> + if (trim_regs[12] & 0x01)
>>> + d1 = -d1;
>>> +
>>> + d1 += temp;
>>> +
>>> + /* D2 */
>>> + temp = (trim_regs[4] & 0x3F) << 2;
>>> + temp |= (trim_regs[2] & 0x06) >> 1;
>>> + if (trim_regs[2] & 0x01)
>>> + temp = -temp;
>>> +
>>> + d2 = (trim_regs[16] & 0x1F) << 2;
>>> + d2 |= (trim_regs[14] & 0x06) >> 1;
>>> + if (trim_regs[14] & 0x01)
>>> + d2
Re: [PATCH 1/3] iio: core: implement devm_iio_device_alloc/devm_iio_device_free
On 07/18/2013 12:19 PM, Oleksandr Kravchenko wrote:
From: Grygorii Strashko
Add a resource managed devm_iio_device_alloc()/devm_iio_device_free()
to automatically clean up any allocations made by IIO drivers,
thus leading to simplified IIO drivers code.
In addition, this will allow IIO drivers to use other devm_*() API
(like devm_request_irq) and don't care about the race between
iio_device_free() and the release of resources by Device core
during driver removing.
Signed-off-by: Grygorii Strashko
[o.v.kravche...@globallogic.com: fix return value ib devm_iio_device_alloc
in case if devres_alloc failed, remove unused variable "rc"]
Signed-off-by: Oleksandr Kravchenko
Tested-by: Oleksandr Kravchenko
Very nice, thanks for taking care of this.
Reviewed-by: Lars-Peter Clausen
---
drivers/iio/industrialio-core.c | 47 +++
include/linux/iio/iio.h | 25 +
2 files changed, 72 insertions(+)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index e145931..d56d122 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -912,6 +912,53 @@ void iio_device_free(struct iio_dev *dev)
}
EXPORT_SYMBOL(iio_device_free);
+static void devm_iio_device_release(struct device *dev, void *res)
+{
+ iio_device_free(*(struct iio_dev **)res);
+}
+
+static int devm_iio_device_match(struct device *dev, void *res, void *data)
+{
+ struct iio_dev **r = res;
+ if (!r || !*r) {
+ WARN_ON(!r || !*r);
+ return 0;
+ }
+ return *r == data;
+}
+
+struct iio_dev *devm_iio_device_alloc(struct device *dev, int sizeof_priv)
+{
+ struct iio_dev **ptr, *iio_dev;
+
+ ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr),
+ GFP_KERNEL);
+ if (!ptr)
+ return NULL;
+
+ /* use raw alloc_dr for kmalloc caller tracing */
+ iio_dev = iio_device_alloc(sizeof_priv);
+ if (iio_dev) {
+ *ptr = iio_dev;
+ devres_add(dev, ptr);
+ } else {
+ devres_free(ptr);
+ }
+
+ return iio_dev;
+}
+EXPORT_SYMBOL_GPL(devm_iio_device_alloc);
+
+void devm_iio_device_free(struct device *dev, struct iio_dev *iio_dev)
+{
+ int rc;
+
+ rc = devres_release(dev, devm_iio_device_release,
+ devm_iio_device_match, iio_dev);
+ WARN_ON(rc);
+}
+EXPORT_SYMBOL_GPL(devm_iio_device_free);
+
/**
* iio_chrdev_open() - chrdev file open for buffer access and ioctls
**/
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 8d171f4..f1d99f6 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -532,6 +532,31 @@ static inline struct iio_dev *iio_priv_to_dev(void *priv)
void iio_device_free(struct iio_dev *indio_dev);
/**
+ * devm_iio_device_alloc - Resource-managed iio_device_alloc()
+ * @dev: Device to allocate iio_dev for
+ * @sizeof_priv: Space to allocate for private structure.
+ *
+ * Managed iio_device_alloc. iio_dev allocated with this function is
+ * automatically freed on driver detach.
+ *
+ * If an iio_dev allocated with this function needs to be freed separately,
+ * devm_iio_device_free() must be used.
+ *
+ * RETURNS:
+ * Pointer to allocated iio_dev on success, NULL on failure.
+ */
+struct iio_dev *devm_iio_device_alloc(struct device *dev, int sizeof_priv);
+
+/**
+ * devm_iio_device_free - Resource-managed iio_device_free()
+ * @dev: Device this iio_dev belongs to
+ * @indio_dev: the iio_dev associated with the device
+ *
+ * Free indio_dev allocated with devm_iio_device_alloc().
+ */
+void devm_iio_device_free(struct device *dev, struct iio_dev *iio_dev);
+
+/**
* iio_buffer_enabled() - helper function to test if the buffer is enabled
* @indio_dev:IIO device structure for device
**/
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [alsa-devel] [PATCH v2 00/11] ASoC: fsl-ssi: ac97-slave support
On 04/07/2013 09:25 PM, Markus Pargmann wrote: > Hi, > > This series adds DT support for phycore-ac97 using the fsl-ssi driver. In > version 2 I discarded most of the imx-ssi work and integrated the ac97-slave > support in fsl-ssi, including support for imx-pcm-fiq as alternative to dma. > There are some other changes to get phycore-ac97 working. The first two > patches > have notes about the detailed changes since version 1. The rest of the patches > is new and necessary for the ac97-slave support. > > Regards, > > Markus > Hi, The DMA PCM related changes won't apply anymore due to the recent dmaengine PCM cleanups. I've also been working on a more generic dmaengine PCM driver which has DT support and can be used by imx. The work in progress patches can be found here: https://github.com/lclausen-adi/linux-2.6/commits/asoc-dmaengine-cleanups - Lars > > Markus Pargmann (11): > ASoC: phycore-ac97: Add DT support > ASoC: imx-pcm-dma: Add support for DMA init by > ASoC: imx-pcm-fiq: Introduce pcm-fiq-params > ASoC: fsl-ssi: Add SACNT definitions > ASoC: fsl-ssi: Add support for imx-pcm-fiq > ASoC: fsl-ssi: Setup generic imx dma params > ARM: imx: Export ac97 reset functions > ASoC: fsl-ssi: imx ac97 support > ASoC: fsl: Kconfig: Use fsl-ssi for phycore-ac97 > ASoC: fsl: Move fsl-ssi binding doc to sound/ > ASoC: fsl: Update fsl-ssi binding doc > > Documentation/devicetree/bindings/powerpc/fsl/ssi.txt | 73 - > Documentation/devicetree/bindings/sound/fsl,ssi.txt | 13 > b/Documentation/devicetree/bindings/sound/fsl,ssi.txt | 73 + > b/Documentation/devicetree/bindings/sound/phytec,phycore-ac97.txt | 12 > b/arch/arm/mach-imx/mach-pca100.c |7 > b/arch/arm/mach-imx/mach-pcm043.c |7 > b/sound/soc/fsl/Kconfig |2 > b/sound/soc/fsl/fsl_ssi.c | 70 + > b/sound/soc/fsl/fsl_ssi.h |8 > b/sound/soc/fsl/imx-pcm-dma.c | 31 > b/sound/soc/fsl/imx-pcm-fiq.c | 14 > b/sound/soc/fsl/imx-pcm.h |6 > b/sound/soc/fsl/imx-ssi.c |7 > b/sound/soc/fsl/imx-ssi.h |1 > b/sound/soc/fsl/phycore-ac97.c| 185 > sound/soc/fsl/fsl_ssi.c | 379 > +++--- > sound/soc/fsl/imx-pcm.h |9 > 17 files changed, 664 insertions(+), 233 deletions(-) > > ___ > Alsa-devel mailing list > alsa-de...@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [alsa-devel] [PATCH v2 00/11] ASoC: fsl-ssi: ac97-slave support
On 04/07/2013 10:08 PM, Markus Pargmann wrote: > Hi, > > On Sun, Apr 07, 2013 at 09:39:31PM +0200, Lars-Peter Clausen wrote: >> On 04/07/2013 09:25 PM, Markus Pargmann wrote: >>> Hi, >>> >>> This series adds DT support for phycore-ac97 using the fsl-ssi driver. In >>> version 2 I discarded most of the imx-ssi work and integrated the ac97-slave >>> support in fsl-ssi, including support for imx-pcm-fiq as alternative to dma. >>> There are some other changes to get phycore-ac97 working. The first two >>> patches >>> have notes about the detailed changes since version 1. The rest of the >>> patches >>> is new and necessary for the ac97-slave support. >>> >>> Regards, >>> >>> Markus >>> >> >> Hi, >> >> The DMA PCM related changes won't apply anymore due to the recent dmaengine >> PCM cleanups. I've also been working on a more generic dmaengine PCM driver >> which has DT support and can be used by imx. The work in progress patches >> can be found here: >> https://github.com/lclausen-adi/linux-2.6/commits/asoc-dmaengine-cleanups >> >> - Lars > > Okay, I will rebase it for v3 onto your committed cleanups and will have > a look on your latest patches. > It might be a good idea to rebase it ontop of the generic dmaengine driver patches right away, since it will simplify things quite a bit. - Lars > Regards, > > Markus > >> >>> >>> Markus Pargmann (11): >>> ASoC: phycore-ac97: Add DT support >>> ASoC: imx-pcm-dma: Add support for DMA init by >>> ASoC: imx-pcm-fiq: Introduce pcm-fiq-params >>> ASoC: fsl-ssi: Add SACNT definitions >>> ASoC: fsl-ssi: Add support for imx-pcm-fiq >>> ASoC: fsl-ssi: Setup generic imx dma params >>> ARM: imx: Export ac97 reset functions >>> ASoC: fsl-ssi: imx ac97 support >>> ASoC: fsl: Kconfig: Use fsl-ssi for phycore-ac97 >>> ASoC: fsl: Move fsl-ssi binding doc to sound/ >>> ASoC: fsl: Update fsl-ssi binding doc >>> >>> Documentation/devicetree/bindings/powerpc/fsl/ssi.txt | 73 - >>> Documentation/devicetree/bindings/sound/fsl,ssi.txt | 13 >>> b/Documentation/devicetree/bindings/sound/fsl,ssi.txt | 73 + >>> b/Documentation/devicetree/bindings/sound/phytec,phycore-ac97.txt | 12 >>> b/arch/arm/mach-imx/mach-pca100.c |7 >>> b/arch/arm/mach-imx/mach-pcm043.c |7 >>> b/sound/soc/fsl/Kconfig |2 >>> b/sound/soc/fsl/fsl_ssi.c | 70 + >>> b/sound/soc/fsl/fsl_ssi.h |8 >>> b/sound/soc/fsl/imx-pcm-dma.c | 31 >>> b/sound/soc/fsl/imx-pcm-fiq.c | 14 >>> b/sound/soc/fsl/imx-pcm.h |6 >>> b/sound/soc/fsl/imx-ssi.c |7 >>> b/sound/soc/fsl/imx-ssi.h |1 >>> b/sound/soc/fsl/phycore-ac97.c| 185 >>> >>> sound/soc/fsl/fsl_ssi.c | 379 >>> +++--- >>> sound/soc/fsl/imx-pcm.h |9 >>> 17 files changed, 664 insertions(+), 233 deletions(-) >>> >>> ___ >>> Alsa-devel mailing list >>> alsa-de...@alsa-project.org >>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel >> >> > ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 1/2] dmaengine: mpc512x_dma: use generic DMA DT bindings
On 03/31/2013 06:17 PM, Anatolij Gustschin wrote:
> Add generic DMA bindings and register the DMA controller
> to DT DMA helpers.
>
> Signed-off-by: Anatolij Gustschin
> ---
> arch/powerpc/boot/dts/mpc5121.dtsi |5 ++-
> drivers/dma/mpc512x_dma.c | 63
> ++--
> 2 files changed, 64 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/boot/dts/mpc5121.dtsi
> b/arch/powerpc/boot/dts/mpc5121.dtsi
> index 723e292..d1fe070 100644
> --- a/arch/powerpc/boot/dts/mpc5121.dtsi
> +++ b/arch/powerpc/boot/dts/mpc5121.dtsi
> @@ -384,10 +384,13 @@
> interrupts = <40 0x8>;
> };
>
> - dma@14000 {
> + dma0: dma@14000 {
> compatible = "fsl,mpc5121-dma";
> reg = <0x14000 0x1800>;
> interrupts = <65 0x8>;
> + #dma-cells = <1>;
> + #dma-channels = <64>;
> + #dma-requests = <64>;
> };
> };
>
> diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
> index 2d95673..bc6c356 100644
> --- a/drivers/dma/mpc512x_dma.c
> +++ b/drivers/dma/mpc512x_dma.c
> @@ -40,6 +40,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> @@ -641,6 +642,44 @@ mpc_dma_prep_memcpy(struct dma_chan *chan, dma_addr_t
> dst, dma_addr_t src,
> return &mdesc->desc;
> }
>
> +struct mpc_dma_filter_args {
> + struct mpc_dma *mdma;
> + unsigned int chan_id;
> +};
> +
> +static bool mpc_dma_filter(struct dma_chan *chan, void *param)
> +{
> + struct mpc_dma_filter_args *fargs = param;
> +
> + if (chan->device != &fargs->mdma->dma)
> + return false;
> +
> + return (chan->chan_id == fargs->chan_id);
> +}
> +
> +static struct dma_chan *mpc_dma_xlate(struct of_phandle_args *dma_spec,
> + struct of_dma *ofdma)
> +{
> + int count = dma_spec->args_count;
> + struct mpc_dma *mdma = ofdma->of_dma_data;
> + struct mpc_dma_filter_args fargs;
> + dma_cap_mask_t cap;
> +
> + if (!mdma)
> + return NULL;
> +
> + if (count != 1)
> + return NULL;
> +
> + fargs.mdma = mdma;
> + fargs.chan_id = dma_spec->args[0];
> +
> + dma_cap_zero(cap);
> + dma_cap_set(DMA_SLAVE, cap);
> +
> + return dma_request_channel(cap, mpc_dma_filter, &fargs);
> +}
> +
This is more or less the same as the generic of_dma_xlate_by_chan_id
function I posted about two weeks ago:
https://patchwork.kernel.org/patch/2331091/
Vinod, can you take a look at that patch again? I'm not quite sure what you
meant by your question 'how will the client know which "id" to request?'.
- Lars
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [alsa-devel] [PATCH v2 00/11] ASoC: fsl-ssi: ac97-slave support
On 04/13/2013 03:32 PM, Markus Pargmann wrote: > On Sun, Apr 07, 2013 at 10:35:02PM +0200, Lars-Peter Clausen wrote: >> On 04/07/2013 10:08 PM, Markus Pargmann wrote: >>> Hi, >>> >>> On Sun, Apr 07, 2013 at 09:39:31PM +0200, Lars-Peter Clausen wrote: >>>> On 04/07/2013 09:25 PM, Markus Pargmann wrote: >>>>> Hi, >>>>> >>>>> This series adds DT support for phycore-ac97 using the fsl-ssi driver. In >>>>> version 2 I discarded most of the imx-ssi work and integrated the >>>>> ac97-slave >>>>> support in fsl-ssi, including support for imx-pcm-fiq as alternative to >>>>> dma. >>>>> There are some other changes to get phycore-ac97 working. The first two >>>>> patches >>>>> have notes about the detailed changes since version 1. The rest of the >>>>> patches >>>>> is new and necessary for the ac97-slave support. >>>>> >>>>> Regards, >>>>> >>>>> Markus >>>>> >>>> >>>> Hi, >>>> >>>> The DMA PCM related changes won't apply anymore due to the recent dmaengine >>>> PCM cleanups. I've also been working on a more generic dmaengine PCM driver >>>> which has DT support and can be used by imx. The work in progress patches >>>> can be found here: >>>> https://github.com/lclausen-adi/linux-2.6/commits/asoc-dmaengine-cleanups >>>> >>>> - Lars >>> >>> Okay, I will rebase it for v3 onto your committed cleanups and will have >>> a look on your latest patches. >>> >> >> It might be a good idea to rebase it ontop of the generic dmaengine driver >> patches right away, since it will simplify things quite a bit. > > I rebased the series onto your branch now. But there is a small problem > with the DMA handling. > > Currently you pass the device pointer into snd_dmaengine_pcm_register > and use it to request the dma channels. At least for imx-pcm-audio > driver, it would be useful to pass a second device just for the dma > requests. Otherwise imx-pcm-audio needs devicetree bindings, but I think > the dma properties belong to fsl-ssi. > I though about that as well, just initialize of_node of the newly allocated imx-pcm-audio platform device to that of the parent device. That should work nicely. - Lars ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 00/15] ASoC: fsl-ssi: ac97-slave support
On 04/14/2013 01:42 PM, Markus Pargmann wrote: > Hi, > > This series adds DT support for phycore-ac97. It is now based on the pcm dma > cleanups from Lars. Beside ac97 support, the series adds fsl-ssi imx-pcm-fiq > and generic DMA binding handling. > > @Lars: > There are 4 new patches at the beginning of the series that fix some problems > of your cleanups. I think they were in the changes you posted. If not, could > you > please add them to your branch? Hi, I had already fixed them localy. I'm in the process of polishing the last bits and pieces of my generic dmaengine PCM driver series right now and will submit it later today. - Lars ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] dma: of-dma: check OF pointer property before dereferencing it
On 04/15/2013 10:39 AM, Paolo Pisati wrote:
> Signed-off-by: Paolo Pisati
That should already be fixed in the DMA tree. See commit 7362f04c28 ("DMA:
OF: Check properties value before running be32_to_cpup() on it").
- Lars
> ---
> drivers/dma/of-dma.c |7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
> index 69d04d2..9628298 100644
> --- a/drivers/dma/of-dma.c
> +++ b/drivers/dma/of-dma.c
> @@ -91,6 +91,7 @@ int of_dma_controller_register(struct device_node *np,
> (struct of_phandle_args *, struct of_dma *),
> void *data)
> {
> + void*parent;
> struct of_dma *ofdma;
> int nbcells;
>
> @@ -103,8 +104,10 @@ int of_dma_controller_register(struct device_node *np,
> if (!ofdma)
> return -ENOMEM;
>
> - nbcells = be32_to_cpup(of_get_property(np, "#dma-cells", NULL));
> - if (!nbcells) {
> + parent = of_get_property(np, "#dma-cells", NULL);
> + if (parent)
> + nbcells = be32_to_cpup(parent);
> + if (!parent || !nbcells) {
> pr_err("%s: #dma-cells property is missing or invalid\n",
> __func__);
> kfree(ofdma);
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 00/15] ASoC: fsl-ssi: ac97-slave support
On 04/16/2013 02:25 AM, Timur Tabi wrote: > Lars-Peter Clausen wrote: >>> >@Lars: >>> >There are 4 new patches at the beginning of the series that fix some >>> problems >>> >of your cleanups. I think they were in the changes you posted. If not, >>> could you >>> >please add them to your branch? >> Hi, >> >> I had already fixed them localy. I'm in the process of polishing the last >> bits >> and pieces of my generic dmaengine PCM driver series right now and will >> submit >> it later today. > > So which repository should I apply these patches to? I want to make sure > they don't break PowerPC. > You can use https://github.com/lclausen-adi/linux-2.6/tree/asoc-dmaengine-cleanups, but make sure to skip the first 4 patches from this series. - Lars ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [alsa-devel] [PATCH v4 02/10] ASoC: imx-pcm-dma: DT support
On 04/18/2013 02:33 PM, Markus Pargmann wrote:
> This patch adds the possibility to pass a of_node as platform_data which
> is used by generic-pcm-dma to request a DMA slave channel.
>
> Signed-off-by: Markus Pargmann
> ---
> sound/soc/fsl/imx-pcm-dma.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/fsl/imx-pcm-dma.c b/sound/soc/fsl/imx-pcm-dma.c
> index c246fb5..8945d22 100644
> --- a/sound/soc/fsl/imx-pcm-dma.c
> +++ b/sound/soc/fsl/imx-pcm-dma.c
> @@ -62,9 +62,11 @@ static const struct snd_dmaengine_pcm_config
> imx_dmaengine_pcm_config = {
>
> int imx_pcm_dma_init(struct platform_device *pdev)
> {
> + if (pdev->dev.platform_data)
> + pdev->dev.of_node = pdev->dev.platform_data;
In my opinion it's better to use pdev->dev.parent->of_node here. In the ssi
driver you use platform_device_register_data, which will create a copy of
the of_node you pass in as platform data. I'm not quite sure how well this
will work. If you want to continue to use platform_data you should at least
change the code in the ssi driver to not make a copy of the of_node.
- Lars
> +
> return snd_dmaengine_pcm_register(&pdev->dev, &imx_dmaengine_pcm_config,
> SND_DMAENGINE_PCM_FLAG_NO_RESIDUE |
> - SND_DMAENGINE_PCM_FLAG_NO_DT |
> SND_DMAENGINE_PCM_FLAG_COMPAT);
> }
>
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH RESEND v5 1/2] dma: imx-dma: Add oftree support
Hi,
On 04/18/2013 03:21 PM, Markus Pargmann wrote:
[...]
> +static struct dma_chan *imxdma_xlate(struct of_phandle_args *dma_spec,
> + struct of_dma *ofdma)
> +{
> + int count = dma_spec->args_count;
> + struct imxdma_engine *imxdma = ofdma->of_dma_data;
> + struct imxdma_filter_data fdata = {
> + .imxdma = imxdma,
> + .request = *(unsigned *)&dma_spec->args[0],
This cast looks rather bogus and shouldn't be necessary.
> + };
> +
> + if (count != 1)
> + return NULL;
I think you need to check count before you access dma_spec->args[0]
> +
> + return dma_request_channel(imxdma->dma_device.cap_mask,
> + imxdma_filter_fn, &fdata);
> +}
> +
[...]
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 1/3] iio: Add Nuvoton NAU7802 ADC driver
Hi,
Some comments on top of what Jonathan said.
On 04/18/2013 05:38 PM, Alexandre Belloni wrote:
[...]
> diff --git a/drivers/iio/adc/nau7802.c b/drivers/iio/adc/nau7802.c
> new file mode 100644
> index 000..0148fd8
> --- /dev/null
> +++ b/drivers/iio/adc/nau7802.c
> @@ -0,0 +1,644 @@
[...]
> +static int nau7802_read_raw(struct iio_dev *idev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct nau7802_state *st = iio_priv(idev);
> + u8 data;
> + int ret;
> +
> + switch (mask) {
> +
[...]
> + case IIO_CHAN_INFO_SCALE:
> + ret = nau7802_i2c_read(st, NAU7802_REG_CTRL1, &data);
> + if (ret < 0)
> + return ret;
> +
> + *val = 0;
> + *val2 = (((u64)st->vref_mv) * 10ULL) >>
> + (chan->scan_type.realbits +
> + (data & NAU7802_CTRL1_GAINS_BITS));
If you use IIO_VAL_FRACTIONAL_LOG2 this becomes much more readable.
*val = st->vref_mv;
*val2 = chan->scan_type.realbits +
(data & NAU7802_CTRL1_GAINS_BITS);
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + break;
> + }
> + return -EINVAL;
> +}
[...]
> +static int nau7802_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct iio_dev *idev;
> + struct nau7802_state *st;
> + struct device_node *np = client->dev.of_node;
> + int ret;
> + u8 data;
> + u32 tmp;
> +
> + if (!client->dev.of_node) {
> + dev_err(&client->dev, "No device tree node available.\n");
> + return -EINVAL;
> + }
> +
> + /*
> + * If we have some interrupts declared, use them, if not, fall back
> + * on polling.
> + */
> + if (of_find_property(np, "interrupts", NULL)) {
> + if (client->irq <= 0) {
> + client->irq = irq_of_parse_and_map(np, 0);
> + if (client->irq <= 0)
> + return -EPROBE_DEFER;
> + }
> + } else
> + client->irq = 0;
This looks like something that could go into the of_i2c.c code.
> +
> + idev = iio_device_alloc(sizeof(struct nau7802_state));
> + if (idev == NULL)
> + return -ENOMEM;
> +
> + st = iio_priv(idev);
> +
> + i2c_set_clientdata(client, idev);
> +
> + idev->dev.parent = &client->dev;
> + idev->name = dev_name(&client->dev);
> + idev->modes = INDIO_DIRECT_MODE;
> + idev->info = &nau7802_info;
> +
> + st->client = client;
> +
> + /* Reset the device */
> + nau7802_i2c_write(st, NAU7802_REG_PUCTRL, NAU7802_PUCTRL_RR_BIT);
> +
> + /* Enter normal operation mode */
> + nau7802_i2c_write(st, NAU7802_REG_PUCTRL, NAU7802_PUCTRL_PUD_BIT);
> +
> + /*
> + * After about 200 usecs, the device should be ready and then
> + * the Power Up bit will be set to 1. If not, wait for it.
> + */
> + do {
> + udelay(200);
> + ret = nau7802_i2c_read(st, NAU7802_REG_PUCTRL, &data);
> + if (ret)
> + return -ENODEV;
> + } while (!(data & NAU7802_PUCTRL_PUR_BIT));
> +
> + of_property_read_u32(np, "nuvoton,vldo", &tmp);
> + st->vref_mv = tmp;
We've standardized on using the regulator framework for providing voltages.
You should use it here as well.
> +
> + data = NAU7802_PUCTRL_PUD_BIT | NAU7802_PUCTRL_PUA_BIT |
> + NAU7802_PUCTRL_CS_BIT;
> + if (tmp >= 2400)
> + data |= NAU7802_PUCTRL_AVDDS_BIT;
> +
> + nau7802_i2c_write(st, NAU7802_REG_PUCTRL, data);
> + nau7802_i2c_write(st, NAU7802_REG_ADC_CTRL, 0x30);
> +
> + if (tmp >= 2400) {
> + data = NAU7802_CTRL1_VLDO((4500 - tmp) / 300);
> + nau7802_i2c_write(st, NAU7802_REG_CTRL1, data);
> + }
> +
> + st->min_conversions = 6;
> +
> + /*
> + * The ADC fires continuously and we can't do anything about
> + * it. So we need to have the IRQ disabled by default, and we
> + * will enable them back when we will need them..
> + */
> + if (client->irq) {
> + irq_set_status_flags(client->irq, IRQ_NOAUTOEN);
No driver should be messing with a IRQs flags. Basically if you need irq.h
for your device driver its probably wrong. The proper solution is to
introduce a IRQF_NOAUTOEN.
> + ret = request_threaded_irq(client->irq,
> + NULL,
> + nau7802_eoc_trigger,
> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> + client->dev.driver->name,
> + idev);
> + if (ret) {
> + /*
> + * What may happen here is that our IRQ controller is
> + * not able
[PATCH] ASoC: Add ssm2518 support
This patch adds a ASoC CODEC driver for the SSM2516. The SSM2516 is a stereo
Class-D audio amplifier with an I2S interface for audio in and a built-in
dynamic range control processor.
Signed-off-by: Lars-Peter Clausen
---
.../devicetree/bindings/sound/ssm2518.txt | 20 +
include/linux/platform_data/ssm2518.h | 22 +
sound/soc/codecs/Kconfig | 4 +
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/ssm2518.c | 845 +
sound/soc/codecs/ssm2518.h | 20 +
6 files changed, 913 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/ssm2518.txt
create mode 100644 include/linux/platform_data/ssm2518.h
create mode 100644 sound/soc/codecs/ssm2518.c
create mode 100644 sound/soc/codecs/ssm2518.h
diff --git a/Documentation/devicetree/bindings/sound/ssm2518.txt
b/Documentation/devicetree/bindings/sound/ssm2518.txt
new file mode 100644
index 000..59381a7
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/ssm2518.txt
@@ -0,0 +1,20 @@
+SSM2518 audio amplifier
+
+This device supports I2C only.
+
+Required properties:
+ - compatible : Must be "adi,ssm2518"
+ - reg : the I2C address of the device. This will either be 0x34 (ADDR pin
low)
+ or 0x35 (ADDR pin high)
+
+Optional properties:
+ - gpios : GPIO connected to the nSD pin. If the property is not present it is
+ assumed that the nSD pin is hardwired to always on.
+
+Example:
+
+ ssm2518: ssm2518@34 {
+ compatible = "adi,ssm2518";
+ reg = <0x34>;
+ gpios = <&gpio 5 0>;
+ };
diff --git a/include/linux/platform_data/ssm2518.h
b/include/linux/platform_data/ssm2518.h
new file mode 100644
index 000..9a8e3ea
--- /dev/null
+++ b/include/linux/platform_data/ssm2518.h
@@ -0,0 +1,22 @@
+/*
+ * SSM2518 amplifier audio driver
+ *
+ * Copyright 2013 Analog Devices Inc.
+ * Author: Lars-Peter Clausen
+ *
+ * Licensed under the GPL-2.
+ */
+
+#ifndef __LINUX_PLATFORM_DATA_SSM2518_H__
+#define __LINUX_PLATFORM_DATA_SSM2518_H__
+
+/**
+ * struct ssm2518_platform_data - Platform data for the ssm2518 driver
+ * @enable_gpio: GPIO connected to the nSD pin. Set to -1 if the nSD pin is
+ *hardwired.
+ */
+struct ssm2518_platform_data {
+ int enable_gpio;
+};
+
+#endif
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 2f45f00..d76609a 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -60,6 +60,7 @@ config SND_SOC_ALL_CODECS
select SND_SOC_SI476X if MFD_SI476X_CORE
select SND_SOC_SN95031 if INTEL_SCU_IPC
select SND_SOC_SPDIF
+ select SND_SOC_SSM2518 if I2C
select SND_SOC_SSM2602 if SND_SOC_I2C_AND_SPI
select SND_SOC_STA32X if I2C
select SND_SOC_STA529 if I2C
@@ -313,6 +314,9 @@ config SND_SOC_SN95031
config SND_SOC_SPDIF
tristate
+config SND_SOC_SSM2518
+ tristate
+
config SND_SOC_SSM2602
tristate
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index dae0aa6..7204c24 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -52,6 +52,7 @@ snd-soc-si476x-objs := si476x.o
snd-soc-sn95031-objs := sn95031.o
snd-soc-spdif-tx-objs := spdif_transmitter.o
snd-soc-spdif-rx-objs := spdif_receiver.o
+snd-soc-ssm2518-objs := ssm2518.o
snd-soc-ssm2602-objs := ssm2602.o
snd-soc-sta32x-objs := sta32x.o
snd-soc-sta529-objs := sta529.o
@@ -176,6 +177,7 @@ obj-$(CONFIG_SND_SOC_SIGMADSP) += snd-soc-sigmadsp.o
obj-$(CONFIG_SND_SOC_SI476X) += snd-soc-si476x.o
obj-$(CONFIG_SND_SOC_SN95031) +=snd-soc-sn95031.o
obj-$(CONFIG_SND_SOC_SPDIF)+= snd-soc-spdif-rx.o snd-soc-spdif-tx.o
+obj-$(CONFIG_SND_SOC_SSM2518) += snd-soc-ssm2518.o
obj-$(CONFIG_SND_SOC_SSM2602) += snd-soc-ssm2602.o
obj-$(CONFIG_SND_SOC_STA32X) += snd-soc-sta32x.o
obj-$(CONFIG_SND_SOC_STA529) += snd-soc-sta529.o
diff --git a/sound/soc/codecs/ssm2518.c b/sound/soc/codecs/ssm2518.c
new file mode 100644
index 000..5874ba5
--- /dev/null
+++ b/sound/soc/codecs/ssm2518.c
@@ -0,0 +1,845 @@
+/*
+ * SSM2518 amplifier audio driver
+ *
+ * Copyright 2013 Analog Devices Inc.
+ * Author: Lars-Peter Clausen
+ *
+ * Licensed under the GPL-2.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "ssm2518.h"
+
+#define SSM2518_REG_POWER1 0x00
+#define SSM2518_REG_CLOCK 0x01
+#define SSM2518_REG_SAI_CTRL1 0x02
+#define SSM2518_REG_SAI_CTRL2 0x03
+#define SSM2518_REG_CHAN_MAP 0x04
+#define SSM2518_REG_LEFT_VOL 0x05
+#define SSM2518_REG_RIGHT_VOL 0x06
+#define SSM2518_REG_MUTE_CTRL 0x07
+#define SSM2518_REG_FAULT_CTRL 0x08
+#define SSM2518_REG_POWE
Re: [PATCH] ASoC: Add ssm2518 support
On 05/22/2013 07:57 PM, Mark Brown wrote:
> On Wed, May 22, 2013 at 07:00:13PM +0200, Lars-Peter Clausen wrote:
>> This patch adds a ASoC CODEC driver for the SSM2516. The SSM2516 is a stereo
>> Class-D audio amplifier with an I2S interface for audio in and a built-in
>> dynamic range control processor.
>
> I'll apply this but
I'll send a v2 with your comments fixed.
>
>> +if (slots == 0) {
>> +return regmap_update_bits(ssm2518->regmap,
>> +SSM2518_REG_SAI_CTRL1, SSM2518_SAI_CTRL1_SAI_MASK,
>> +SSM2518_SAI_CTRL1_SAI_I2S);
>> +}
>
> You've got quite a few single statement if () blocks with { } which
> shouldn't be there.
>
I prefer to only do this for single single-line statements.
[...]
>
>> +switch (freq) {
>> +case 2048000:
>
> Looks like the user can't select 0 for the SYSCLK, I'd expect that to be
> possible for systems that can reprogram the clock so that they can avoid
> having constraints set when they don't need them.
>
Makes sense.
>> +} else if (i2c->dev.of_node) {
>> +ssm2518->enable_gpio = of_get_gpio(i2c->dev.of_node, 0);
>> +if (ssm2518->enable_gpio == -EPROBE_DEFER)
>> +return -EPROBE_DEFER;
>
> Why are other errors being ignored here?
To make the property optional. But I think it might be better to only allow
-ENOENT and treat everything else as an error in this case.
Thanks,
- Lars
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
[PATCH v2] ASoC: Add ssm2518 support
This patch adds a ASoC CODEC driver for the SSM2516. The SSM2516 is a stereo
Class-D audio amplifier with an I2S interface for audio in and a built-in
dynamic range control processor.
Signed-off-by: Lars-Peter Clausen
---
Changes since v1:
* Power off the codec in the I2C probe function
* Allow a sysclk of 0 which won't but any constraints on the supported
sample rates. This can be used by systems which are able to change the
sysclk on demand.
* Remove {} around multi-line single statements
---
.../devicetree/bindings/sound/ssm2518.txt | 20 +
include/linux/platform_data/ssm2518.h | 22 +
sound/soc/codecs/Kconfig | 4 +
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/ssm2518.c | 856 +
sound/soc/codecs/ssm2518.h | 20 +
6 files changed, 924 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/ssm2518.txt
create mode 100644 include/linux/platform_data/ssm2518.h
create mode 100644 sound/soc/codecs/ssm2518.c
create mode 100644 sound/soc/codecs/ssm2518.h
diff --git a/Documentation/devicetree/bindings/sound/ssm2518.txt
b/Documentation/devicetree/bindings/sound/ssm2518.txt
new file mode 100644
index 000..59381a7
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/ssm2518.txt
@@ -0,0 +1,20 @@
+SSM2518 audio amplifier
+
+This device supports I2C only.
+
+Required properties:
+ - compatible : Must be "adi,ssm2518"
+ - reg : the I2C address of the device. This will either be 0x34 (ADDR pin
low)
+ or 0x35 (ADDR pin high)
+
+Optional properties:
+ - gpios : GPIO connected to the nSD pin. If the property is not present it is
+ assumed that the nSD pin is hardwired to always on.
+
+Example:
+
+ ssm2518: ssm2518@34 {
+ compatible = "adi,ssm2518";
+ reg = <0x34>;
+ gpios = <&gpio 5 0>;
+ };
diff --git a/include/linux/platform_data/ssm2518.h
b/include/linux/platform_data/ssm2518.h
new file mode 100644
index 000..9a8e3ea
--- /dev/null
+++ b/include/linux/platform_data/ssm2518.h
@@ -0,0 +1,22 @@
+/*
+ * SSM2518 amplifier audio driver
+ *
+ * Copyright 2013 Analog Devices Inc.
+ * Author: Lars-Peter Clausen
+ *
+ * Licensed under the GPL-2.
+ */
+
+#ifndef __LINUX_PLATFORM_DATA_SSM2518_H__
+#define __LINUX_PLATFORM_DATA_SSM2518_H__
+
+/**
+ * struct ssm2518_platform_data - Platform data for the ssm2518 driver
+ * @enable_gpio: GPIO connected to the nSD pin. Set to -1 if the nSD pin is
+ *hardwired.
+ */
+struct ssm2518_platform_data {
+ int enable_gpio;
+};
+
+#endif
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 2f45f00..d76609a 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -60,6 +60,7 @@ config SND_SOC_ALL_CODECS
select SND_SOC_SI476X if MFD_SI476X_CORE
select SND_SOC_SN95031 if INTEL_SCU_IPC
select SND_SOC_SPDIF
+ select SND_SOC_SSM2518 if I2C
select SND_SOC_SSM2602 if SND_SOC_I2C_AND_SPI
select SND_SOC_STA32X if I2C
select SND_SOC_STA529 if I2C
@@ -313,6 +314,9 @@ config SND_SOC_SN95031
config SND_SOC_SPDIF
tristate
+config SND_SOC_SSM2518
+ tristate
+
config SND_SOC_SSM2602
tristate
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index dae0aa6..7204c24 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -52,6 +52,7 @@ snd-soc-si476x-objs := si476x.o
snd-soc-sn95031-objs := sn95031.o
snd-soc-spdif-tx-objs := spdif_transmitter.o
snd-soc-spdif-rx-objs := spdif_receiver.o
+snd-soc-ssm2518-objs := ssm2518.o
snd-soc-ssm2602-objs := ssm2602.o
snd-soc-sta32x-objs := sta32x.o
snd-soc-sta529-objs := sta529.o
@@ -176,6 +177,7 @@ obj-$(CONFIG_SND_SOC_SIGMADSP) += snd-soc-sigmadsp.o
obj-$(CONFIG_SND_SOC_SI476X) += snd-soc-si476x.o
obj-$(CONFIG_SND_SOC_SN95031) +=snd-soc-sn95031.o
obj-$(CONFIG_SND_SOC_SPDIF)+= snd-soc-spdif-rx.o snd-soc-spdif-tx.o
+obj-$(CONFIG_SND_SOC_SSM2518) += snd-soc-ssm2518.o
obj-$(CONFIG_SND_SOC_SSM2602) += snd-soc-ssm2602.o
obj-$(CONFIG_SND_SOC_STA32X) += snd-soc-sta32x.o
obj-$(CONFIG_SND_SOC_STA529) += snd-soc-sta529.o
diff --git a/sound/soc/codecs/ssm2518.c b/sound/soc/codecs/ssm2518.c
new file mode 100644
index 000..3139a1b
--- /dev/null
+++ b/sound/soc/codecs/ssm2518.c
@@ -0,0 +1,856 @@
+/*
+ * SSM2518 amplifier audio driver
+ *
+ * Copyright 2013 Analog Devices Inc.
+ * Author: Lars-Peter Clausen
+ *
+ * Licensed under the GPL-2.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "ssm2518.h"
+
+#define SSM2518_REG_POWER1 0x00
+#define SSM2518_REG_CLOCK 0x01
+#
Re: [PATCH v2 02/10] pwm: Allow chips to support multiple PWMs.
On 02/06/2012 04:19 PM, Thierry Reding wrote: > This commit modifies the PWM core to support multiple PWMs per struct > pwm_chip. I think you should mention what motivates this change. > It achieves this in a similar way to how gpiolib works, by > allowing PWM ranges to be requested dynamically (pwm_chip.base == -1) or > starting at a given offset (pwm_chip.base >= 0). If we've learned one thing from gpiolib, I think it is that using a global index to identify a resource was a bad idea. > A chip specifies how > many PWMs it controls using the npwm member. Each of the functions in > the pwm_ops structure gets an additional argument that specified the PWM > number (it can be converted to a per-chip index by subtracting the > chip's base). > > The total maximum number of PWM devices is currently fixed to 64, but > can easily be made configurable via Kconfig. The code says 1024. > > The patch is incomplete in that it doesn't convert any existing drivers > that are now broken. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v4 01/10] PWM: add pwm framework support
On 03/14/2012 04:56 PM, Thierry Reding wrote:
> [...]
> +struct pwm_chip {
> + int pwm_id;
> + const char *label;
> + struct pwm_ops *ops;
Nothing major, but if you are going for another round it would be good to see
ops made const.
> +};
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
