* Re: [PATCH] iio: adc: add driver for the ti-adc084s021 chip
From: Mårten Lindahl @ 2017-04-30 12:42 UTC (permalink / raw)
To: Rob Herring
Cc: Mårten Lindahl, jic23-DgEjT+Ai2ygdnm+yROfE0A,
knaack.h-Mmb7MZpHnFY, lars-Qo5EllUWu/uELgA04lAiVw,
pmeerw-jW+XmwGofnusTnJN9+BGXg, linux-iio-u79uwXL29TY76Z2rM5mHXA,
mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170428135248.du6smmzktqldj4u4@rob-hp-laptop>
On Fri, 2017-04-28 at 08:52 -0500, Rob Herring wrote:
> On Fri, Apr 21, 2017 at 04:58:23PM +0200, Mårten Lindahl wrote:
> > From: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
> >
> > This adds support for the Texas Instruments ADC084S021 ADC chip.
> >
> > Signed-off-by: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
> > ---
> > .../devicetree/bindings/iio/adc/ti-adc084s021.txt | 25 ++
>
> It's preferred to put bindings in a separate patch.
I am sorry for that. I split this into its own patch in v2.
>
> > drivers/iio/adc/Kconfig | 12 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/ti-adc084s021.c | 342 +++++++++++++++++++++
> > 4 files changed, 380 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> > create mode 100644 drivers/iio/adc/ti-adc084s021.c
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> > new file mode 100644
> > index 0000000..921eb46
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> > @@ -0,0 +1,25 @@
> > +* Texas Instruments' ADC084S021
> > +
> > +Required properties:
> > + - compatible : Must be "ti,adc084s021"
> > + - reg : SPI chip select number for the device
> > + - vref-supply : The regulator supply for ADC reference voltage
> > + - spi-max-frequency : Definition as per Documentation/devicetree/bindings/spi/spi-bus.txt
> > +
> > +Optional properties:
> > + - spi-cpol : SPI inverse clock polarity, as per spi-bus bindings
> > + - spi-cpha : SPI shifted clock phase (CPHA), as per spi-bus bindings
> > + - spi-cs-high : SPI chip select active high, as per spi-bus bindings
>
> How are these optional? A given device should have specific properties
> required here.
No, they are not optional. I removed this section in v2 and updated the
required properties section.
>
> Also, no need to define them, "per spi-bus bindings" is enough.
Fixed in v2.
>
> Rob
Thanks,
Mårten
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] iio: adc: add driver for the ti-adc084s021 chip
From: Mårten Lindahl @ 2017-04-30 12:38 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Peter Meerwald-Stadler, Mårten Lindahl,
lars-Qo5EllUWu/uELgA04lAiVw, linux-iio-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <5dec6eec-c214-c14d-8dc9-ff298854fc1c-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On Sun, 2017-04-23 at 20:13 +0100, Jonathan Cameron wrote:
> On 21/04/17 21:19, Peter Meerwald-Stadler wrote:
> >
> >> From: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
> >
> > comments below
> A few more from me. Laptop battery gone flat so not so thorough on top few lines!
>
> Jonathan
Hi Jonathan! Thanks for the comments! Please see my reply below.
Thanks,
Mårten
> >
> >> This adds support for the Texas Instruments ADC084S021 ADC chip.
> >>
> >> Signed-off-by: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
> >> ---
> >> .../devicetree/bindings/iio/adc/ti-adc084s021.txt | 25 ++
> >> drivers/iio/adc/Kconfig | 12 +
> >> drivers/iio/adc/Makefile | 1 +
> >> drivers/iio/adc/ti-adc084s021.c | 342 +++++++++++++++++++++
> >> 4 files changed, 380 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> >> create mode 100644 drivers/iio/adc/ti-adc084s021.c
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> >> new file mode 100644
> >> index 0000000..921eb46
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> >> @@ -0,0 +1,25 @@
> >> +* Texas Instruments' ADC084S021
> >> +
> >> +Required properties:
> >> + - compatible : Must be "ti,adc084s021"
> >> + - reg : SPI chip select number for the device
> >> + - vref-supply : The regulator supply for ADC reference voltage
> >> + - spi-max-frequency : Definition as per Documentation/devicetree/bindings/spi/spi-bus.txt
> >> +
> >> +Optional properties:
> >> + - spi-cpol : SPI inverse clock polarity, as per spi-bus bindings
> >> + - spi-cpha : SPI shifted clock phase (CPHA), as per spi-bus bindings
> >> + - spi-cs-high : SPI chip select active high, as per spi-bus bindings
> >> +
> >> +
> >> +Example:
> >> +adc@0 {
> >> + compatible = "ti,adc084s021";
> >> + reg = <0>;
> >> + vref-supply = <&adc_vref>;
> >> + spi-cpol;
> >> + spi-cpha;
> >> + spi-cs-high;
> >> + spi-max-frequency = <16000000>;
> >> + pl022,com-mode = <0x2>; /* DMA */
> >
> > what is this?
> >
> >> +};
> >> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> >> index dedae7a..13141e5 100644
> >> --- a/drivers/iio/adc/Kconfig
> >> +++ b/drivers/iio/adc/Kconfig
> >> @@ -560,6 +560,18 @@ config TI_ADC0832
> >> This driver can also be built as a module. If so, the module will be
> >> called ti-adc0832.
> >>
> >> +config TI_ADC084S021
> >> + tristate "Texas Instruments ADC084S021"
> >> + depends on SPI
> >> + select IIO_BUFFER
> >> + select IIO_TRIGGERED_BUFFER
> >> + help
> >> + If you say yes here you get support for Texas Instruments ADC084S021
> >> + chips.
> >> +
> >> + This driver can also be built as a module. If so, the module will be
> >> + called ti-adc084s021.
> >> +
> >> config TI_ADC12138
> >> tristate "Texas Instruments ADC12130/ADC12132/ADC12138"
> >> depends on SPI
> >> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> >> index d001262..b1a6158 100644
> >> --- a/drivers/iio/adc/Makefile
> >> +++ b/drivers/iio/adc/Makefile
> >> @@ -51,6 +51,7 @@ obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
> >> obj-$(CONFIG_STM32_ADC) += stm32-adc.o
> >> obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
> >> obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
> >> +obj-$(CONFIG_TI_ADC084S021) += ti-adc084s021.o
> >> obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
> >> obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
> >> obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
> >> diff --git a/drivers/iio/adc/ti-adc084s021.c b/drivers/iio/adc/ti-adc084s021.c
> >> new file mode 100644
> >> index 0000000..4f33b91
> >> --- /dev/null
> >> +++ b/drivers/iio/adc/ti-adc084s021.c
> >> @@ -0,0 +1,342 @@
> >> +/**
> >> + * Copyright (C) 2017 Axis Communications AB
> >> + *
> >> + * Driver for Texas Instruments' ADC084S021 ADC chip.
> >> + * Datasheets can be found here:
> >> + * http://www.ti.com/lit/ds/symlink/adc084s021.pdf
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> >> + */
> >> +
> >> +#include <linux/err.h>
> >> +#include <linux/spi/spi.h>
> >> +#include <linux/module.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/iio/iio.h>
> >> +#include <linux/iio/buffer.h>
> >> +#include <linux/iio/events.h>
> >> +#include <linux/iio/triggered_buffer.h>
> >> +#include <linux/iio/trigger_consumer.h>
> >> +#include <linux/regulator/consumer.h>
> >> +
> >> +#define MODULE_NAME "adc084s021"
> >> +#define DRIVER_VERSION "1.0"
> >
> > is only used once at the very end...
> >
> >> +#define ADC_RESOLUTION 8
> Put it inline...
Fixed in v2.
> >> +#define ADC_N_CHANNELS 4
> Belongs inline. No additional info provided by having it here.
Fixed in v2.
> >
> > we want a consistent prefix, such as ADC084S021_
> >
> >> +
> >> +struct adc084s021_configuration {
> >> + const struct iio_chan_spec *channels;
> >> + u8 num_channels;
> >
> > no need for u8, perhaps unsigned?
> >
> >> +};
> >> +
> >> +struct adc084s021 {
> >> + struct spi_device *spi;
> >> + struct spi_message message;
> >> + struct spi_transfer spi_trans[2];
> >> + struct regulator *reg;
> >> + struct mutex lock;
> >> + /*
> >> + * DMA (thus cache coherency maintenance) requires the
> >> + * transfer buffers to live in their own cache lines.
> >> + */
> >> + union {
> >> + u8 tx_buf[2];
> >> + u8 rx_buf[2];
> >> + } ____cacheline_aligned;
> >> + u8 cur_adc_values[ADC_N_CHANNELS];
> >> +};
> >> +
> >> +/**
> >> + * Event triggered when value changes on a channel
> >> + */
> >> +static const struct iio_event_spec adc084s021_event = {
> >> + .type = IIO_EV_TYPE_CHANGE,
> >> + .dir = IIO_EV_DIR_NONE,
> >> +};
> Not the intent of that type of event at all.
I removed using events in v2.
> >> +
> >> +/**
> >> + * Channel specification
> >> + */
> >> +#define ADC084S021_VOLTAGE_CHANNEL(num) \
> >> + { \
> >> + .type = IIO_VOLTAGE, \
> >> + .channel = (num), \
> >> + .address = (num << 3), \
> >
> > parenthesis should be around (num)
> >
> >> + .indexed = 1, \
> >> + .scan_index = num, \
> >
> > parenthesis?
> >
> >> + .scan_type = { \
> >> + .sign = 'u', \
> >> + .realbits = 8, \
> >> + .storagebits = 32, \
> >> + .shift = 24 - ((num << 3)), \
> >
> > no need for (( )) around the expression, parenthesis should be around num
> >
> > the shift doesn't make sense, you are shifting in
> >
> > adc084s021_adc_conversion() already and return just 8 bits (so storagebits
> > should be 8)?
> >
> > you could/should use realbits = 8, storagebits = 16, shift = 4 and
> > endianness = IIO_BE if I read Figure 1 of the datasheet correctly
> >
> >> + }, \
> >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> >
> > likely this is missing _SCALE
> > IIO expects data to be returned in millivolt, see
> > Documentation/ABI/testing/sysfs-bus-iio
> >
> >> + .event_spec = &adc084s021_event, \
> >> + .num_event_specs = 1, \
> >
> > ARRAY_SIZE(&adc084s021_event) to be extensible?
> >
> >> + }
> >> +
> >> +static const struct iio_chan_spec adc084s021_channels[] = {
> >> + ADC084S021_VOLTAGE_CHANNEL(0),
> >> + ADC084S021_VOLTAGE_CHANNEL(1),
> >> + ADC084S021_VOLTAGE_CHANNEL(2),
> >> + ADC084S021_VOLTAGE_CHANNEL(3),
> >> + IIO_CHAN_SOFT_TIMESTAMP(4),
> >> +};
> >> +
> >> +static const struct adc084s021_configuration adc084s021_config[] = {
> >> + { adc084s021_channels, ARRAY_SIZE(adc084s021_channels) },
> >
> > for just one configuration, this is not really needed; so you plan/forsee
> > more chips being added soonish?
> >
> >> +};
> >> +
> >> +/**
> >> + * Read an ADC channel and return its value.
> >> + *
> >> + * @adc: The ADC SPI data.
> >> + * @channel: The IIO channel data structure.
> >> + */
> >> +static int adc084s021_adc_conversion(struct adc084s021 *adc,
> >> + struct iio_chan_spec const *channel)
> >> +{
> >> + u16 value;
> >
> > value should be u8, but is not really needed
> >
> >> + int ret;
> >> +
> >> + mutex_lock(&adc->lock);
> >> + adc->tx_buf[0] = channel->address;
> >> +
> >> + /* Do the transfer */
> >> + ret = spi_sync(adc->spi, &adc->message);
> >> +
> >
> > no newline here please
> >
> >> + if (ret < 0) {
> >> + mutex_unlock(&adc->lock);
> >> + return ret;
> >> + }
> >> +
> >> + value = (adc->rx_buf[0] << 4) | (adc->rx_buf[1] >> 4);
> >
> > I recommend using __be16 for rx_buf and
> > ret = (be16_to_cpu(adc->rx_buf) >> 4) & 0xff;
> >
> >> + mutex_unlock(&adc->lock);
> >> +
> >> + dev_dbg(&adc->spi->dev, "value 0x%02X on channel %d\n",
> >> + value, channel->channel);
> >> + return value;
> >> +}
> >> +
> >> +/**
> >> + * Make a readout of requested IIO channel info.
> >
> > no need to document this, it is found in every IIO driver...
> >
> >> + *
> >> + * @indio_dev: The industrial I/O device.
> >> + * @channel: The IIO channel data structure.
> >> + * @val: First element of value (integer).
> >> + * @val2: Second element of value (fractional).
> >> + * @mask: The info_mask to read.
> >> + */
> >> +static int adc084s021_read_raw(struct iio_dev *indio_dev,
> >> + struct iio_chan_spec const *channel, int *val,
> >> + int *val2, long mask)
> >> +{
> >> + struct adc084s021 *adc = iio_priv(indio_dev);
> >> + int retval;
> >
> > how about using ret everywhere?
> Agreed.
Fixed in v2.
> >
> >> +
> >> + switch (mask) {
> >> + case IIO_CHAN_INFO_RAW:
> >
> > probably want iio_device_claim_direct_mode() so to not interfere with
> > buffered accessBorderline case as all you will do is queue up additional spi transfers.
> At least after you have dropped the unusual events stuff.
>
> Arguably this will mean sysfs reads could make the buffered data flow
> less deterministic though so maybe on claiming direct mode (which
> prevents it running concurrently with buffered capture.
Fixed in v2. And no events anymore.
> >
> >> + retval = adc084s021_adc_conversion(adc, channel);
> >> + if (retval < 0)
> >> + return retval;
> >> +
> >> + *val = retval;
> >> + return IIO_VAL_INT;
> >> +
> >> + default:
> >> + return -EINVAL;
> >> + }
> >> +}
> >> +
> >> +/**
> >> + * Read enabled ADC channels and push data to the buffer.
> >> + *
> >> + * @irq: The interrupt number (not used).
> >> + * @pollfunc: Pointer to the poll func.
> >> + */
> >> +static irqreturn_t adc084s021_trigger_handler(int irq, void *pollfunc)
> >> +{
> >> + struct iio_poll_func *pf = pollfunc;
> >> + struct iio_dev *indio_dev = pf->indio_dev;
> >> + struct adc084s021 *adc = iio_priv(indio_dev);
> >> + u8 *data;
> >> + s64 timestamp;
> >> + int value, scan_index;
> >> +
> >> + data = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
> >
> > pre-allocate buffer with maximum size statically; allocation is
> > potentially expensive; don't forget space for the timestamp and padding...
> >
> >> + if (!data) {
> >> + iio_trigger_notify_done(indio_dev->trig);
> >> + return IRQ_NONE;
> >> + }
> >> +
> >> + timestamp = iio_get_time_ns(indio_dev);
> >> +
> >> + for_each_set_bit(scan_index, indio_dev->active_scan_mask,
> >> + indio_dev->masklength) {
> >> + const struct iio_chan_spec *channel =
> >> + &indio_dev->channels[scan_index];
> >> + value = adc084s021_adc_conversion(adc, channel);
> >
> > lock is taken and released for each channel, probably want to do it just
> > once?
> >
> >> + data[scan_index] = value;
> >> +
> >> + /*
> >> + * Compare read data to previous read. If it differs send
> >> + * event notification for affected channel.
> >> + */
> >> + if (adc->cur_adc_values[scan_index] != (u8)value) {
> >
> > cur_adc_values is not initialized (probably set to 0);
> > so on first read, should the notification be sent?
> ? This is 'unusual' to say the least. Why the events given the data
> is available via the buffer. If you really want to do this stuff, it
> ought to be in userspace.
>
> Are you trying to emulate the filtering input does?
I removed the check for previous value and sending events in v2.
> >
> >> + adc->cur_adc_values[scan_index] = (u8)value;
> >> + iio_push_event(indio_dev,
> >> + IIO_EVENT_CODE(IIO_VOLTAGE, 0,
> >> + IIO_NO_MOD, IIO_EV_DIR_NONE,
> >> + IIO_EV_TYPE_CHANGE,
> >> + channel->channel, 0, 0),
> >> + timestamp);
> >> + dev_dbg(&indio_dev->dev,
> >> + "new value on ch%d: 0x%02X (ts %llu)\n",
> >> + channel->channel, value, timestamp);
> >> + }
> >> + }
> >> +
> >> + iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
> >> + iio_trigger_notify_done(indio_dev->trig);
> >> + kfree(data);
> blank line here please.
Fixed in v2.
> >> + return IRQ_HANDLED;
> >> +}
> >> +
> >> +static const struct iio_info adc084s021_info = {
> >> + .read_raw = adc084s021_read_raw,
> >> + .driver_module = THIS_MODULE,
> >> +};
> >> +
> >> +/**
> >> + * Create and register ADC IIO device for SPI.
> >
> > comment is rather pointless
> >
> >> + */
> >> +static int adc084s021_probe(struct spi_device *spi)
> >> +{
> >> + struct iio_dev *indio_dev;
> >> + struct adc084s021 *adc;
> >> + int config = spi_get_device_id(spi)->driver_data;
> >> + int retval;
> >
> > ret maybe
> >
> >> +
> >> + /* Allocate an Industrial I/O device */
> >
> > obviously...
> :) Yeah, clear out any comments that don't tell us much.
Fixed in v2.
> >
> >> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> >> + if (!indio_dev) {
> >> + dev_err(&spi->dev, "Failed to allocate IIO device\n");
> >> + return -ENOMEM;
> >> + }
> >> +
> >> + adc = iio_priv(indio_dev);
> >> + adc->spi = spi;
> >> + spi->bits_per_word = ADC_RESOLUTION;
> This surprised me somewhat as I was expecting an odd value for this
> (and was going to ask if standard power of 2 options would work).
> If it had been inline, this would have required slightly less review
> time which I always like (particularly when crammed into an airline seat!)
Yes, I am using inline value in v2.
> >> +
> >> + /* Update the SPI device with config and connect the iio dev */
> >> + retval = spi_setup(spi);
> >> + if (retval) {
> >> + dev_err(&spi->dev, "Failed to update SPI device\n");
> >> + return retval;
> >> + }
> >> + spi_set_drvdata(spi, indio_dev);
> >> +
> >> + /* Initiate the Industrial I/O device */
> :>> + indio_dev->dev.parent = &spi->dev;
> >> + indio_dev->dev.of_node = spi->dev.of_node;
> >> + indio_dev->name = spi_get_device_id(spi)->name;
> >> + indio_dev->modes = INDIO_DIRECT_MODE;
> >> + indio_dev->info = &adc084s021_info;
> >> + indio_dev->channels = adc084s021_config[config].channels;
> >> + indio_dev->num_channels = adc084s021_config[config].num_channels;
> >
> > could do it directly as long there is just one configuration
> That would certainly be preferable unless V2 is going to show up
> with multiple options!
Fixed. No more options supported in v2.
> >
> >> +
> >> + /* Create SPI transfer for channel reads */
> >> + adc->spi_trans[0].tx_buf = &adc->tx_buf[0];
> >> + adc->spi_trans[0].len = 2;
> >> + adc->spi_trans[0].speed_hz = spi->max_speed_hz;
> >> + adc->spi_trans[0].bits_per_word = spi->bits_per_word;
> >> + adc->spi_trans[1].rx_buf = &adc->rx_buf[0];
> >> + adc->spi_trans[1].len = 2;
> >> + adc->spi_trans[1].speed_hz = spi->max_speed_hz;
> >> + adc->spi_trans[1].bits_per_word = spi->bits_per_word;
> >> +
> >> + /* Setup SPI message for channel reads */
> >> + spi_message_init(&adc->message);
> >> + spi_message_add_tail(&adc->spi_trans[0], &adc->message);
> >> + spi_message_add_tail(&adc->spi_trans[1], &adc->message);
> spi_init_with_transfers to save a bit of boilerplate.
Fixed in v2.
> >> +
> >> + adc->reg = devm_regulator_get(&spi->dev, "vref");
> >> + if (IS_ERR(adc->reg))
> >> + return PTR_ERR(adc->reg);
> >> +
> >> + retval = regulator_enable(adc->reg);
> >> + if (retval < 0)
> >> + return retval;
> Given we either have slow sysfs accesses or know we have buffered
> access on going. Have you considered enabling and disabling
> the regulator dynamically? The fact that this often makes sense is
> why Mark and co from the regulators side of things haven't provided
> a devm_regulator_enable... You would need a preenable and postdisable
> to make sure it was on for buffered access.
Yes, I am using preenable and postdisable functions for regulator in v2.
> >> +
> >> + mutex_init(&adc->lock);
> >> +
> >> + /* Setup triggered buffer with pollfunction */
> >> + retval = iio_triggered_buffer_setup(indio_dev, NULL,
> >
> > devm_()
> I disagree. It would change the ordering wrt to the regulator_enable.
> Now, obviously it won't actually matter, but it will make the code
> less obviously correct and for the small burden of extra unwind
> code I'd keep using the non devm version.
I did not change this in v2. So I kept the non devm_ functions.
> >
> >> + adc084s021_trigger_handler, NULL);
> >> + if (retval) {
> >> + dev_err(&spi->dev, "Failed to setup triggered buffer\n");
> >> + goto buffer_setup_failed;
> >> + }
> >> +
> >> + retval = iio_device_register(indio_dev);
> >> + if (retval) {
> >> + dev_err(&spi->dev, "Failed to register IIO device\n");
> Hmm. If we were to flesh out some error messages for the few
> cases where there is no info provided in iio_device_register we could
> probably clean out a fair bit of boiler plate reporting in drivers.
> Ah well, one for the future!
If you insist I will remove it :)
> >> + goto device_register_failed;
> >> + }
> >> +
> >> + dev_info(&spi->dev, "probed!\n");
> >
> > no logging please, just outputs clutter
> >
> >> + return 0;
> >> +
> >> +device_register_failed:
> >> + iio_triggered_buffer_cleanup(indio_dev);
> >> +buffer_setup_failed:
> >> + regulator_disable(adc->reg);
> >> + return retval;
> >> +}
> >> +
> >> +/**
> >> + * Unregister ADC IIO device for SPI.
> >> + */
> >> +static int adc084s021_remove(struct spi_device *spi)
> >> +{
> >> + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> >> + struct adc084s021 *adc = iio_priv(indio_dev);
> >> +
> >> + iio_device_unregister(indio_dev);
> >> + iio_triggered_buffer_cleanup(indio_dev);
> >> + regulator_disable(adc->reg);
> blank line here preferred (slightly!)
Fixed in v2.
> >> + return 0;
> >> +}
> >> +
> >> +static const struct of_device_id adc084s021_of_match[] = {
> >> + { .compatible = "ti,adc084s021", },
> >> + {},
> >> +};
> >> +
> >> +MODULE_DEVICE_TABLE(of, adc084s021_of_match);
> >> +
> >> +static const struct spi_device_id adc084s021_id[] = {
> >> + { MODULE_NAME, 0},
> >> + {}
> >> +};
> >> +
> >> +MODULE_DEVICE_TABLE(spi, adc084s021_id);
> >> +
> >> +static struct spi_driver adc084s021_driver = {
> >> + .driver = {
> >> + .name = MODULE_NAME,
> >> + .of_match_table = of_match_ptr(adc084s021_of_match),
> >> + },
> >> + .probe = adc084s021_probe,
> >> + .remove = adc084s021_remove,
> >> + .id_table = adc084s021_id,
> >> +};
> >> +
> Convention often says to not bother with a blank line here or before
> the MODULE_DEVICE_TABLE above. Gives a visual indication that these macros
> are being passed the structures.
> (really minor point!)
Fixed in v2.
> >> +module_spi_driver(adc084s021_driver);
> >> +
> >> +MODULE_AUTHOR("Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>");
> >> +MODULE_DESCRIPTION("Texas Instruments ADC084S021");
> >> +MODULE_LICENSE("GPL v2");
> >> +MODULE_VERSION(DRIVER_VERSION);
> >>
> >
>
^ permalink raw reply
* Re: [PATCH] regulator: Allow for asymmetric settling times
From: Mark Brown @ 2017-04-30 12:30 UTC (permalink / raw)
To: Laxman Dewangan
Cc: Matthias Kaehlcke, Liam Girdwood, Rob Herring, Mark Rutland,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Douglas Anderson, Brian Norris
In-Reply-To: <59044881.7090901-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]
On Sat, Apr 29, 2017 at 01:32:09PM +0530, Laxman Dewangan wrote:
> On Saturday 29 April 2017 05:36 AM, Matthias Kaehlcke wrote:
> > -- regulator-settling-time-us: Settling time, in microseconds, for voltage
> > - change if regulator have the constant time for any level voltage change.
> > - This is useful when regulator have exponential voltage change.
> > +- regulator-settling-time-up-us: Settling time, in microseconds, for voltage
> > + increase if the regulator needs a constant time to settle after voltage
> > + increases of any level. This is useful for regulators with exponential
> > + voltage changes.
> > +- regulator-settling-time-down-us: Settling time, in microseconds, for voltage
> > + decrease if the regulator needs a constant time to settle after voltage
> > + decreases of any level. This is useful for regulators with exponential
> > + voltage changes.
> Can we have regulator-settling-time-us also so if it is there then up/down
> same.
> If up/down different then separate properties can be used.
Removing the existing binding would also break existing DTs using it
which we obviously don't want. I don't see any reason to even deprecate
it, like Laxman says it's nice and convenient for people with symmetric
performance.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH] iio: adc: add driver for the ti-adc084s021 chip
From: Mårten Lindahl @ 2017-04-30 12:24 UTC (permalink / raw)
To: Peter Meerwald-Stadler
Cc: Mårten Lindahl, jic23-DgEjT+Ai2ygdnm+yROfE0A,
lars-Qo5EllUWu/uELgA04lAiVw, linux-iio-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <alpine.DEB.2.20.1704212149070.2470-M0QeDd4q1oXQbIPoIc8EuQ@public.gmane.org>
On Fri, 2017-04-21 at 22:19 +0200, Peter Meerwald-Stadler wrote:
> > From: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
>
> comments below
Hi! Thanks for the comments! Please see my reply below.
Thanks,
Mårten
>
> > This adds support for the Texas Instruments ADC084S021 ADC chip.
> >
> > Signed-off-by: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
> > ---
> > .../devicetree/bindings/iio/adc/ti-adc084s021.txt | 25 ++
> > drivers/iio/adc/Kconfig | 12 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/ti-adc084s021.c | 342 +++++++++++++++++++++
> > 4 files changed, 380 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> > create mode 100644 drivers/iio/adc/ti-adc084s021.c
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> > new file mode 100644
> > index 0000000..921eb46
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> > @@ -0,0 +1,25 @@
> > +* Texas Instruments' ADC084S021
> > +
> > +Required properties:
> > + - compatible : Must be "ti,adc084s021"
> > + - reg : SPI chip select number for the device
> > + - vref-supply : The regulator supply for ADC reference voltage
> > + - spi-max-frequency : Definition as per Documentation/devicetree/bindings/spi/spi-bus.txt
> > +
> > +Optional properties:
> > + - spi-cpol : SPI inverse clock polarity, as per spi-bus bindings
> > + - spi-cpha : SPI shifted clock phase (CPHA), as per spi-bus bindings
> > + - spi-cs-high : SPI chip select active high, as per spi-bus bindings
> > +
> > +
> > +Example:
> > +adc@0 {
> > + compatible = "ti,adc084s021";
> > + reg = <0>;
> > + vref-supply = <&adc_vref>;
> > + spi-cpol;
> > + spi-cpha;
> > + spi-cs-high;
> > + spi-max-frequency = <16000000>;
> > + pl022,com-mode = <0x2>; /* DMA */
>
> what is this?
It does not belong here. It is removed i v2. This dt-bindings part is
split into its own patch in v2.
>
> > +};
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index dedae7a..13141e5 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -560,6 +560,18 @@ config TI_ADC0832
> > This driver can also be built as a module. If so, the module will be
> > called ti-adc0832.
> >
> > +config TI_ADC084S021
> > + tristate "Texas Instruments ADC084S021"
> > + depends on SPI
> > + select IIO_BUFFER
> > + select IIO_TRIGGERED_BUFFER
> > + help
> > + If you say yes here you get support for Texas Instruments ADC084S021
> > + chips.
> > +
> > + This driver can also be built as a module. If so, the module will be
> > + called ti-adc084s021.
> > +
> > config TI_ADC12138
> > tristate "Texas Instruments ADC12130/ADC12132/ADC12138"
> > depends on SPI
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index d001262..b1a6158 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -51,6 +51,7 @@ obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
> > obj-$(CONFIG_STM32_ADC) += stm32-adc.o
> > obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
> > obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
> > +obj-$(CONFIG_TI_ADC084S021) += ti-adc084s021.o
> > obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
> > obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
> > obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
> > diff --git a/drivers/iio/adc/ti-adc084s021.c b/drivers/iio/adc/ti-adc084s021.c
> > new file mode 100644
> > index 0000000..4f33b91
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti-adc084s021.c
> > @@ -0,0 +1,342 @@
> > +/**
> > + * Copyright (C) 2017 Axis Communications AB
> > + *
> > + * Driver for Texas Instruments' ADC084S021 ADC chip.
> > + * Datasheets can be found here:
> > + * http://www.ti.com/lit/ds/symlink/adc084s021.pdf
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/module.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#define MODULE_NAME "adc084s021"
> > +#define DRIVER_VERSION "1.0"
>
> is only used once at the very end...
Removed in v2.
>
> > +#define ADC_RESOLUTION 8
> > +#define ADC_N_CHANNELS 4
>
> we want a consistent prefix, such as ADC084S021_
I use values inline i v2.
>
> > +
> > +struct adc084s021_configuration {
> > + const struct iio_chan_spec *channels;
> > + u8 num_channels;
>
> no need for u8, perhaps unsigned?
I removed this struct in v2 and use direct addressing.
>
> > +};
> > +
> > +struct adc084s021 {
> > + struct spi_device *spi;
> > + struct spi_message message;
> > + struct spi_transfer spi_trans[2];
> > + struct regulator *reg;
> > + struct mutex lock;
> > + /*
> > + * DMA (thus cache coherency maintenance) requires the
> > + * transfer buffers to live in their own cache lines.
> > + */
> > + union {
> > + u8 tx_buf[2];
> > + u8 rx_buf[2];
> > + } ____cacheline_aligned;
> > + u8 cur_adc_values[ADC_N_CHANNELS];
> > +};
> > +
> > +/**
> > + * Event triggered when value changes on a channel
> > + */
> > +static const struct iio_event_spec adc084s021_event = {
> > + .type = IIO_EV_TYPE_CHANGE,
> > + .dir = IIO_EV_DIR_NONE,
> > +};
> > +
> > +/**
> > + * Channel specification
> > + */
> > +#define ADC084S021_VOLTAGE_CHANNEL(num) \
> > + { \
> > + .type = IIO_VOLTAGE, \
> > + .channel = (num), \
> > + .address = (num << 3), \
>
> parenthesis should be around (num)
Fixed in v2.
>
> > + .indexed = 1, \
> > + .scan_index = num, \
>
> parenthesis?
Fixed in v2.
>
> > + .scan_type = { \
> > + .sign = 'u', \
> > + .realbits = 8, \
> > + .storagebits = 32, \
> > + .shift = 24 - ((num << 3)), \
>
> no need for (( )) around the expression, parenthesis should be around num
>
> the shift doesn't make sense, you are shifting in
>
> adc084s021_adc_conversion() already and return just 8 bits (so storagebits
> should be 8)?
>
> you could/should use realbits = 8, storagebits = 16, shift = 4 and
> endianness = IIO_BE if I read Figure 1 of the datasheet correctly
>
This macro has been fixed according to your suggestion (and the
datasheet) in v2.
> > + }, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>
> likely this is missing _SCALE
> IIO expects data to be returned in millivolt, see
> Documentation/ABI/testing/sysfs-bus-iio
>
Added IIO_CHAN_INFO_SCALE in v2.
> > + .event_spec = &adc084s021_event, \
> > + .num_event_specs = 1, \
>
> ARRAY_SIZE(&adc084s021_event) to be extensible?
Removed events in v2.
>
> > + }
> > +
> > +static const struct iio_chan_spec adc084s021_channels[] = {
> > + ADC084S021_VOLTAGE_CHANNEL(0),
> > + ADC084S021_VOLTAGE_CHANNEL(1),
> > + ADC084S021_VOLTAGE_CHANNEL(2),
> > + ADC084S021_VOLTAGE_CHANNEL(3),
> > + IIO_CHAN_SOFT_TIMESTAMP(4),
> > +};
> > +
> > +static const struct adc084s021_configuration adc084s021_config[] = {
> > + { adc084s021_channels, ARRAY_SIZE(adc084s021_channels) },
>
> for just one configuration, this is not really needed; so you plan/forsee
> more chips being added soonish?
No more than this chip supported by this driver, so I use direct
addressing in v2.
>
> > +};
> > +
> > +/**
> > + * Read an ADC channel and return its value.
> > + *
> > + * @adc: The ADC SPI data.
> > + * @channel: The IIO channel data structure.
> > + */
> > +static int adc084s021_adc_conversion(struct adc084s021 *adc,
> > + struct iio_chan_spec const *channel)
> > +{
> > + u16 value;
>
> value should be u8, but is not really needed
Fixed in v2.
>
> > + int ret;
> > +
> > + mutex_lock(&adc->lock);
> > + adc->tx_buf[0] = channel->address;
> > +
> > + /* Do the transfer */
> > + ret = spi_sync(adc->spi, &adc->message);
> > +
>
> no newline here please
Fixed in v2.
>
> > + if (ret < 0) {
> > + mutex_unlock(&adc->lock);
> > + return ret;
> > + }
> > +
> > + value = (adc->rx_buf[0] << 4) | (adc->rx_buf[1] >> 4);
>
> I recommend using __be16 for rx_buf and
> ret = (be16_to_cpu(adc->rx_buf) >> 4) & 0xff;
Fixed in v2.
>
> > + mutex_unlock(&adc->lock);
> > +
> > + dev_dbg(&adc->spi->dev, "value 0x%02X on channel %d\n",
> > + value, channel->channel);
> > + return value;
> > +}
> > +
> > +/**
> > + * Make a readout of requested IIO channel info.
>
> no need to document this, it is found in every IIO driver...
Removed documentation for standard driver functions in v2.
>
> > + *
> > + * @indio_dev: The industrial I/O device.
> > + * @channel: The IIO channel data structure.
> > + * @val: First element of value (integer).
> > + * @val2: Second element of value (fractional).
> > + * @mask: The info_mask to read.
> > + */
> > +static int adc084s021_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *channel, int *val,
> > + int *val2, long mask)
> > +{
> > + struct adc084s021 *adc = iio_priv(indio_dev);
> > + int retval;
>
> how about using ret everywhere?
Fixed in v2.
>
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
>
> probably want iio_device_claim_direct_mode() so to not interfere with
> buffered access
Fixed in v2.
>
> > + retval = adc084s021_adc_conversion(adc, channel);
> > + if (retval < 0)
> > + return retval;
> > +
> > + *val = retval;
> > + return IIO_VAL_INT;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +/**
> > + * Read enabled ADC channels and push data to the buffer.
> > + *
> > + * @irq: The interrupt number (not used).
> > + * @pollfunc: Pointer to the poll func.
> > + */
> > +static irqreturn_t adc084s021_trigger_handler(int irq, void *pollfunc)
> > +{
> > + struct iio_poll_func *pf = pollfunc;
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + struct adc084s021 *adc = iio_priv(indio_dev);
> > + u8 *data;
> > + s64 timestamp;
> > + int value, scan_index;
> > +
> > + data = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
>
> pre-allocate buffer with maximum size statically; allocation is
> potentially expensive; don't forget space for the timestamp and padding...
Fixed in v2.
>
> > + if (!data) {
> > + iio_trigger_notify_done(indio_dev->trig);
> > + return IRQ_NONE;
> > + }
> > +
> > + timestamp = iio_get_time_ns(indio_dev);
> > +
> > + for_each_set_bit(scan_index, indio_dev->active_scan_mask,
> > + indio_dev->masklength) {
> > + const struct iio_chan_spec *channel =
> > + &indio_dev->channels[scan_index];
> > + value = adc084s021_adc_conversion(adc, channel);
>
> lock is taken and released for each channel, probably want to do it just
> once?
Fixed in v2.
>
> > + data[scan_index] = value;
> > +
> > + /*
> > + * Compare read data to previous read. If it differs send
> > + * event notification for affected channel.
> > + */
> > + if (adc->cur_adc_values[scan_index] != (u8)value) {
>
> cur_adc_values is not initialized (probably set to 0);
> so on first read, should the notification be sent?
Events no longer used in v2, so no need for this check anymore.
>
> > + adc->cur_adc_values[scan_index] = (u8)value;
> > + iio_push_event(indio_dev,
> > + IIO_EVENT_CODE(IIO_VOLTAGE, 0,
> > + IIO_NO_MOD, IIO_EV_DIR_NONE,
> > + IIO_EV_TYPE_CHANGE,
> > + channel->channel, 0, 0),
> > + timestamp);
> > + dev_dbg(&indio_dev->dev,
> > + "new value on ch%d: 0x%02X (ts %llu)\n",
> > + channel->channel, value, timestamp);
> > + }
> > + }
> > +
> > + iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
> > + iio_trigger_notify_done(indio_dev->trig);
> > + kfree(data);
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static const struct iio_info adc084s021_info = {
> > + .read_raw = adc084s021_read_raw,
> > + .driver_module = THIS_MODULE,
> > +};
> > +
> > +/**
> > + * Create and register ADC IIO device for SPI.
>
> comment is rather pointless
Fixed in v2.
>
> > + */
> > +static int adc084s021_probe(struct spi_device *spi)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct adc084s021 *adc;
> > + int config = spi_get_device_id(spi)->driver_data;
> > + int retval;
>
> ret maybe
Using ret everywhere in v2.
>
> > +
> > + /* Allocate an Industrial I/O device */
>
> obviously...
:) Fixed in v2.
>
> > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> > + if (!indio_dev) {
> > + dev_err(&spi->dev, "Failed to allocate IIO device\n");
> > + return -ENOMEM;
> > + }
> > +
> > + adc = iio_priv(indio_dev);
> > + adc->spi = spi;
> > + spi->bits_per_word = ADC_RESOLUTION;
> > +
> > + /* Update the SPI device with config and connect the iio dev */
> > + retval = spi_setup(spi);
> > + if (retval) {
> > + dev_err(&spi->dev, "Failed to update SPI device\n");
> > + return retval;
> > + }
> > + spi_set_drvdata(spi, indio_dev);
> > +
> > + /* Initiate the Industrial I/O device */
> > + indio_dev->dev.parent = &spi->dev;
> > + indio_dev->dev.of_node = spi->dev.of_node;
> > + indio_dev->name = spi_get_device_id(spi)->name;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->info = &adc084s021_info;
> > + indio_dev->channels = adc084s021_config[config].channels;
> > + indio_dev->num_channels = adc084s021_config[config].num_channels;
>
> could do it directly as long there is just one configuration
Yes, fixed in v2.
>
> > +
> > + /* Create SPI transfer for channel reads */
> > + adc->spi_trans[0].tx_buf = &adc->tx_buf[0];
> > + adc->spi_trans[0].len = 2;
> > + adc->spi_trans[0].speed_hz = spi->max_speed_hz;
> > + adc->spi_trans[0].bits_per_word = spi->bits_per_word;
> > + adc->spi_trans[1].rx_buf = &adc->rx_buf[0];
> > + adc->spi_trans[1].len = 2;
> > + adc->spi_trans[1].speed_hz = spi->max_speed_hz;
> > + adc->spi_trans[1].bits_per_word = spi->bits_per_word;
> > +
> > + /* Setup SPI message for channel reads */
> > + spi_message_init(&adc->message);
> > + spi_message_add_tail(&adc->spi_trans[0], &adc->message);
> > + spi_message_add_tail(&adc->spi_trans[1], &adc->message);
> > +
> > + adc->reg = devm_regulator_get(&spi->dev, "vref");
> > + if (IS_ERR(adc->reg))
> > + return PTR_ERR(adc->reg);
> > +
> > + retval = regulator_enable(adc->reg);
> > + if (retval < 0)
> > + return retval;
> > +
> > + mutex_init(&adc->lock);
> > +
> > + /* Setup triggered buffer with pollfunction */
> > + retval = iio_triggered_buffer_setup(indio_dev, NULL,
>
> devm_()
I have not changed this yet in v2 since Jonathan has another opinion
about this.
>
> > + adc084s021_trigger_handler, NULL);
> > + if (retval) {
> > + dev_err(&spi->dev, "Failed to setup triggered buffer\n");
> > + goto buffer_setup_failed;
> > + }
> > +
> > + retval = iio_device_register(indio_dev);
> > + if (retval) {
> > + dev_err(&spi->dev, "Failed to register IIO device\n");
> > + goto device_register_failed;
> > + }
> > +
> > + dev_info(&spi->dev, "probed!\n");
>
> no logging please, just outputs clutter
Fixed in v2.
>
> > + return 0;
> > +
> > +device_register_failed:
> > + iio_triggered_buffer_cleanup(indio_dev);
> > +buffer_setup_failed:
> > + regulator_disable(adc->reg);
> > + return retval;
> > +}
> > +
> > +/**
> > + * Unregister ADC IIO device for SPI.
> > + */
> > +static int adc084s021_remove(struct spi_device *spi)
> > +{
> > + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> > + struct adc084s021 *adc = iio_priv(indio_dev);
> > +
> > + iio_device_unregister(indio_dev);
> > + iio_triggered_buffer_cleanup(indio_dev);
> > + regulator_disable(adc->reg);
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id adc084s021_of_match[] = {
> > + { .compatible = "ti,adc084s021", },
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, adc084s021_of_match);
> > +
> > +static const struct spi_device_id adc084s021_id[] = {
> > + { MODULE_NAME, 0},
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(spi, adc084s021_id);
> > +
> > +static struct spi_driver adc084s021_driver = {
> > + .driver = {
> > + .name = MODULE_NAME,
> > + .of_match_table = of_match_ptr(adc084s021_of_match),
> > + },
> > + .probe = adc084s021_probe,
> > + .remove = adc084s021_remove,
> > + .id_table = adc084s021_id,
> > +};
> > +
> > +module_spi_driver(adc084s021_driver);
> > +
> > +MODULE_AUTHOR("Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>");
> > +MODULE_DESCRIPTION("Texas Instruments ADC084S021");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_VERSION(DRIVER_VERSION);
> >
>
^ permalink raw reply
* Re: [PATCH v1 1/2] PCI: mediatek: Add Mediatek PCIe host controller support
From: kbuild test robot @ 2017-04-30 4:25 UTC (permalink / raw)
Cc: kbuild-all-JC7UmRfGjtg, Bjorn Helgaas, Rob Herring, Arnd Bergmann,
linux-pci-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Red Hung, Ryder Lee
In-Reply-To: <1493370634-7038-2-git-send-email-ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]
Hi Ryder,
[auto build test ERROR on pci/next]
[also build test ERROR on v4.11-rc8 next-20170428]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ryder-Lee/Add-PCIe-host-driver-support-for-Mediatek-SoCs/20170429-181028
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa
All errors (new ones prefixed by >>):
drivers/built-in.o: In function `pci_host_bridge_of_msi_domain':
>> (.text+0x2f154): undefined reference to `pci_fixup_irqs'
drivers/built-in.o: In function `mtk_pcie_probe':
pcie-mediatek.c:(.text+0x2f956): undefined reference to `pci_fixup_irqs'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 49161 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/4] of: remove *phandle properties from expanded device tree
From: kbuild test robot @ 2017-04-30 0:22 UTC (permalink / raw)
To: frowand.list
Cc: kbuild-all, Rob Herring, stephen.boyd, devicetree, linux-kernel
In-Reply-To: <1493110049-30165-2-git-send-email-frowand.list@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 8074 bytes --]
Hi Frank,
[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.11-rc8 next-20170428]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/frowand-list-gmail-com/of-remove-phandle-properties-from-expanded-device-tree/20170426-000149
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: s390-allmodconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=s390
All errors (new ones prefixed by >>):
In file included from include/linux/kobject.h:21:0,
from include/linux/device.h:17,
from include/linux/node.h:17,
from include/linux/cpu.h:16,
from drivers/of/base.c:25:
drivers/of/base.c: In function '__of_add_phandle_sysfs':
>> drivers/of/base.c:198:23: error: 'pp' undeclared (first use in this function)
sysfs_bin_attr_init(&pp->attr);
^
include/linux/sysfs.h:54:3: note: in definition of macro 'sysfs_attr_init'
(attr)->key = &__key; \
^~~~
drivers/of/base.c:198:2: note: in expansion of macro 'sysfs_bin_attr_init'
sysfs_bin_attr_init(&pp->attr);
^~~~~~~~~~~~~~~~~~~
drivers/of/base.c:198:23: note: each undeclared identifier is reported only once for each function it appears in
sysfs_bin_attr_init(&pp->attr);
^
include/linux/sysfs.h:54:3: note: in definition of macro 'sysfs_attr_init'
(attr)->key = &__key; \
^~~~
drivers/of/base.c:198:2: note: in expansion of macro 'sysfs_bin_attr_init'
sysfs_bin_attr_init(&pp->attr);
^~~~~~~~~~~~~~~~~~~
vim +/pp +198 drivers/of/base.c
19 */
20
21 #define pr_fmt(fmt) "OF: " fmt
22
23 #include <linux/console.h>
24 #include <linux/ctype.h>
> 25 #include <linux/cpu.h>
26 #include <linux/module.h>
27 #include <linux/of.h>
28 #include <linux/of_device.h>
29 #include <linux/of_graph.h>
30 #include <linux/spinlock.h>
31 #include <linux/slab.h>
32 #include <linux/string.h>
33 #include <linux/proc_fs.h>
34
35 #include "of_private.h"
36
37 LIST_HEAD(aliases_lookup);
38
39 struct device_node *of_root;
40 EXPORT_SYMBOL(of_root);
41 struct device_node *of_chosen;
42 struct device_node *of_aliases;
43 struct device_node *of_stdout;
44 static const char *of_stdout_options;
45
46 struct kset *of_kset;
47
48 /*
49 * Used to protect the of_aliases, to hold off addition of nodes to sysfs.
50 * This mutex must be held whenever modifications are being made to the
51 * device tree. The of_{attach,detach}_node() and
52 * of_{add,remove,update}_property() helpers make sure this happens.
53 */
54 DEFINE_MUTEX(of_mutex);
55
56 /* use when traversing tree through the child, sibling,
57 * or parent members of struct device_node.
58 */
59 DEFINE_RAW_SPINLOCK(devtree_lock);
60
61 int of_n_addr_cells(struct device_node *np)
62 {
63 const __be32 *ip;
64
65 do {
66 if (np->parent)
67 np = np->parent;
68 ip = of_get_property(np, "#address-cells", NULL);
69 if (ip)
70 return be32_to_cpup(ip);
71 } while (np->parent);
72 /* No #address-cells property for the root node */
73 return OF_ROOT_NODE_ADDR_CELLS_DEFAULT;
74 }
75 EXPORT_SYMBOL(of_n_addr_cells);
76
77 int of_n_size_cells(struct device_node *np)
78 {
79 const __be32 *ip;
80
81 do {
82 if (np->parent)
83 np = np->parent;
84 ip = of_get_property(np, "#size-cells", NULL);
85 if (ip)
86 return be32_to_cpup(ip);
87 } while (np->parent);
88 /* No #size-cells property for the root node */
89 return OF_ROOT_NODE_SIZE_CELLS_DEFAULT;
90 }
91 EXPORT_SYMBOL(of_n_size_cells);
92
93 #ifdef CONFIG_NUMA
94 int __weak of_node_to_nid(struct device_node *np)
95 {
96 return NUMA_NO_NODE;
97 }
98 #endif
99
100 #ifndef CONFIG_OF_DYNAMIC
101 static void of_node_release(struct kobject *kobj)
102 {
103 /* Without CONFIG_OF_DYNAMIC, no nodes gets freed */
104 }
105 #endif /* CONFIG_OF_DYNAMIC */
106
107 struct kobj_type of_node_ktype = {
108 .release = of_node_release,
109 };
110
111 static ssize_t of_node_property_read(struct file *filp, struct kobject *kobj,
112 struct bin_attribute *bin_attr, char *buf,
113 loff_t offset, size_t count)
114 {
115 struct property *pp = container_of(bin_attr, struct property, attr);
116 return memory_read_from_buffer(buf, count, &offset, pp->value, pp->length);
117 }
118
119 static ssize_t of_node_phandle_read(struct file *filp, struct kobject *kobj,
120 struct bin_attribute *bin_attr, char *buf,
121 loff_t offset, size_t count)
122 {
123 phandle phandle;
124 struct device_node *np;
125
126 np = container_of(bin_attr, struct device_node, attr_phandle);
127 phandle = cpu_to_be32(np->phandle);
128 return memory_read_from_buffer(buf, count, &offset, &phandle,
129 sizeof(phandle));
130 }
131
132 /* always return newly allocated name, caller must free after use */
133 static const char *safe_name(struct kobject *kobj, const char *orig_name)
134 {
135 const char *name = orig_name;
136 struct kernfs_node *kn;
137 int i = 0;
138
139 /* don't be a hero. After 16 tries give up */
140 while (i < 16 && (kn = sysfs_get_dirent(kobj->sd, name))) {
141 sysfs_put(kn);
142 if (name != orig_name)
143 kfree(name);
144 name = kasprintf(GFP_KERNEL, "%s#%i", orig_name, ++i);
145 }
146
147 if (name == orig_name) {
148 name = kstrdup(orig_name, GFP_KERNEL);
149 } else {
150 pr_warn("Duplicate name in %s, renamed to \"%s\"\n",
151 kobject_name(kobj), name);
152 }
153 return name;
154 }
155
156 int __of_add_property_sysfs(struct device_node *np, struct property *pp)
157 {
158 int rc;
159
160 /* Important: Don't leak passwords */
161 bool secure = strncmp(pp->name, "security-", 9) == 0;
162
163 if (!IS_ENABLED(CONFIG_SYSFS))
164 return 0;
165
166 if (!of_kset || !of_node_is_attached(np))
167 return 0;
168
169 sysfs_bin_attr_init(&pp->attr);
170 pp->attr.attr.name = safe_name(&np->kobj, pp->name);
171 pp->attr.attr.mode = secure ? S_IRUSR : S_IRUGO;
172 pp->attr.size = secure ? 0 : pp->length;
173 pp->attr.read = of_node_property_read;
174
175 rc = sysfs_create_bin_file(&np->kobj, &pp->attr);
176 WARN(rc, "error adding attribute %s to node %s\n", pp->name, np->full_name);
177 return rc;
178 }
179
180 /*
181 * In the imported device tree (fdt), phandle is a property. In the
182 * internal data structure it is instead stored in the struct device_node.
183 * Make phandle visible in sysfs as if it was a property.
184 */
185 int __of_add_phandle_sysfs(struct device_node *np)
186 {
187 int rc;
188
189 if (!IS_ENABLED(CONFIG_SYSFS))
190 return 0;
191
192 if (!of_kset || !of_node_is_attached(np))
193 return 0;
194
195 if (!np->phandle || np->phandle == 0xffffffff)
196 return 0;
197
> 198 sysfs_bin_attr_init(&pp->attr);
199 np->attr_phandle.attr.name = "phandle";
200 np->attr_phandle.attr.mode = 0444;
201 np->attr_phandle.size = sizeof(np->phandle);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45067 bytes --]
^ permalink raw reply
* Re: [PATCH v1 1/2] PCI: mediatek: Add Mediatek PCIe host controller support
From: kbuild test robot @ 2017-04-29 23:28 UTC (permalink / raw)
Cc: kbuild-all, Bjorn Helgaas, Rob Herring, Arnd Bergmann, linux-pci,
devicetree, linux-mediatek, linux-arm-kernel, linux-kernel,
Red Hung, Ryder Lee
In-Reply-To: <1493370634-7038-2-git-send-email-ryder.lee@mediatek.com>
[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]
Hi Ryder,
[auto build test ERROR on pci/next]
[also build test ERROR on v4.11-rc8 next-20170428]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ryder-Lee/Add-PCIe-host-driver-support-for-Mediatek-SoCs/20170429-181028
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: s390-allmodconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=s390
All errors (new ones prefixed by >>):
drivers/built-in.o: In function `mtk_pcie_probe':
>> drivers/pci/host/.tmp_gl_pcie-mediatek.o:(.text+0x5b68c): undefined reference to `pci_fixup_irqs'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45164 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver
From: Peter Rosin @ 2017-04-29 21:42 UTC (permalink / raw)
To: Philipp Zabel, linux-media-u79uwXL29TY76Z2rM5mHXA
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Steve Longerbeam, Sakari Ailus,
Pavel Machek, Rob Herring, Mark Rutland, Vladimir Zapolskiy,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, Sascha Hauer, Steve Longerbeam
In-Reply-To: <beb9f7c4-4959-1bb2-03e2-c5ccecbb8368-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
On 2017-04-29 23:29, Peter Rosin wrote:
> On 2017-04-28 16:13, Philipp Zabel wrote:
>> This driver can handle SoC internal and external video bus multiplexers,
>> controlled by mux controllers provided by the mux controller framework,
>> such as MMIO register bitfields or GPIOs. The subdevice passes through
>> the mbus configuration of the active input to the output side.
>>
>> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
>> ---
>> This has been last sent as part of the i.MX media series.
>>
>> Changes since https://patchwork.kernel.org/patch/9647869/:
>> - Split out the actual mux operation to be provided by the mux controller
>> framework [1]. GPIO and MMIO control can be provided by individual mux
>> controller drivers [2][3].
>> [1] https://patchwork.kernel.org/patch/9695837/
>> [2] https://patchwork.kernel.org/patch/9695839/
>> [3] https://patchwork.kernel.org/patch/9704509/
>> - Shortened 'video-multiplexer' to 'video-mux', replaced all instances of
>> vidsw with video_mux.
>> - Made the mux inactive by default, only activated by user interaction.
>> - Added CONFIG_OF and CONFIG_MULTIPLEXER dependencies.
>> - Reuse subdev.entity.num_pads instead of keeping our own count.
>> - Removed implicit link disabling. Instead, trying to enable a second
>> sink pad link yields -EBUSY.
>> - Merged _async_init into _probe.
>> - Removed superfluous pad index check from _set_format.
>> - Added is_source_pad helper to tell source and sink pads apart.
>> - Removed test for status property in endpoint nodes. Disable the remote
>> device or sever the endpoint link to disable a sink pad.
>> ---
>> drivers/media/platform/Kconfig | 6 +
>> drivers/media/platform/Makefile | 2 +
>> drivers/media/platform/video-mux.c | 341 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 349 insertions(+)
>> create mode 100644 drivers/media/platform/video-mux.c
>>
>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>> index c9106e105baba..b046a6d39fee5 100644
>> --- a/drivers/media/platform/Kconfig
>> +++ b/drivers/media/platform/Kconfig
>> @@ -74,6 +74,12 @@ config VIDEO_M32R_AR_M64278
>> To compile this driver as a module, choose M here: the
>> module will be called arv.
>>
>> +config VIDEO_MUX
>> + tristate "Video Multiplexer"
>> + depends on OF && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER && MULTIPLEXER
>> + help
>> + This driver provides support for N:1 video bus multiplexers.
>> +
>> config VIDEO_OMAP3
>> tristate "OMAP 3 Camera support"
>> depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
>> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
>> index 349ddf6a69da2..fd2735ca3ff75 100644
>> --- a/drivers/media/platform/Makefile
>> +++ b/drivers/media/platform/Makefile
>> @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU) += sh_veu.o
>>
>> obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE) += m2m-deinterlace.o
>>
>> +obj-$(CONFIG_VIDEO_MUX) += video-mux.o
>> +
>> obj-$(CONFIG_VIDEO_S3C_CAMIF) += s3c-camif/
>> obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) += exynos4-is/
>> obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG) += s5p-jpeg/
>> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
>> new file mode 100644
>> index 0000000000000..419541729f67e
>> --- /dev/null
>> +++ b/drivers/media/platform/video-mux.c
>> @@ -0,0 +1,341 @@
>> +/*
>> + * video stream multiplexer controlled via mux control
>> + *
>> + * Copyright (C) 2013 Pengutronix, Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> + * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>
> 2017?
>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version 2
>> + * of the License, or (at your option) any later version.
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/module.h>
>> +#include <linux/mux/consumer.h>
>> +#include <linux/of.h>
>> +#include <linux/of_graph.h>
>> +#include <linux/platform_device.h>
>> +#include <media/v4l2-async.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-subdev.h>
>> +#include <media/v4l2-of.h>
>> +
>> +struct video_mux {
>> + struct v4l2_subdev subdev;
>> + struct media_pad *pads;
>> + struct v4l2_mbus_framefmt *format_mbus;
>> + struct v4l2_of_endpoint *endpoint;
>> + struct mux_control *mux;
>> + int active;
>> +};
>> +
>> +static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
>> +{
>> + return container_of(sd, struct video_mux, subdev);
>> +}
>> +
>> +static inline bool is_source_pad(struct video_mux *vmux, unsigned int pad)
>> +{
>> + return pad == vmux->subdev.entity.num_pads - 1;
>> +}
>> +
>> +static int video_mux_link_setup(struct media_entity *entity,
>> + const struct media_pad *local,
>> + const struct media_pad *remote, u32 flags)
>> +{
>> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
>> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> + int ret;
>> +
>> + /*
>> + * The mux state is determined by the enabled sink pad link.
>> + * Enabling or disabling the source pad link has no effect.
>> + */
>> + if (is_source_pad(vmux, local->index))
>> + return 0;
>> +
>> + dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
>> + remote->entity->name, remote->index, local->entity->name,
>> + local->index, flags & MEDIA_LNK_FL_ENABLED);
>> +
>> + if (flags & MEDIA_LNK_FL_ENABLED) {
>> + if (vmux->active == local->index)
>
> Here, you shortcut the mux_control_select_trylock test and return "OK"
> based on a driver-local variable that is intended to keep track of mux
> ownership.
>
>> + return 0;
>> +
>> + if (vmux->active >= 0)
>
> Here too (and this check is not needed, the situation will be covered by
> the mux_control_try_select call).
>
>> + return -EBUSY;
>> +
>> + dev_dbg(sd->dev, "setting %d active\n", local->index);
>> + ret = mux_control_try_select(vmux->mux, local->index);
>> + if (ret < 0)
>> + return ret;
>> + vmux->active = local->index;
>> + } else {
>> + if (vmux->active != local->index)
>> + return 0;
>> +
>> + dev_dbg(sd->dev, "going inactive\n");
>> + mux_control_deselect(vmux->mux);
>
> But here you let go of the mux *before* you clear the driver-local
> ownership indicator. That looks suspicious. My guess is that this is
> "safe" because the upper layers has some serialization, but I don't
> know. Anyway, even if there is something saving you in the upper
> layers, it looks out of order and unneeded. I would have moved the
> below vmux->active = -1; statement up to before the above deselect.
>
> With that fixed, mux usage looks good to me, so you can add an Acked-
> by from me if you wish (goes for the bindings patch as well).
Ouch, that was a bit too soon. If there is *no* serialization in the
upper layers, this is *not* ok, even with my reordering. There must be
only one call to mux_control_deselect, and w/o serialization there
is a race where you might get multiple deselect calls when several
callers makes it through the active != index check before any of them
manages to set active = -1. That race must be taken care of!
Cheers,
peda
>> + vmux->active = -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static struct media_entity_operations video_mux_ops = {
>> + .link_setup = video_mux_link_setup,
>> + .link_validate = v4l2_subdev_link_validate,
>> +};
>> +
>> +static bool video_mux_endpoint_disabled(struct device_node *ep)
>> +{
>> + struct device_node *rpp = of_graph_get_remote_port_parent(ep);
>> +
>> + return !of_device_is_available(rpp);
>> +}
>> +
>> +static int video_mux_g_mbus_config(struct v4l2_subdev *sd,
>> + struct v4l2_mbus_config *cfg)
>> +{
>> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> + struct v4l2_of_endpoint *endpoint;
>> + struct media_pad *pad;
>> + int ret;
>> +
>> + if (vmux->active == -1) {
>> + dev_err(sd->dev, "no configuration for inactive mux\n");
>> + return -EINVAL;
>> + }
>> +
>> + /*
>> + * Retrieve media bus configuration from the entity connected to the
>> + * active input
>> + */
>> + pad = media_entity_remote_pad(&vmux->pads[vmux->active]);
>> + if (pad) {
>> + sd = media_entity_to_v4l2_subdev(pad->entity);
>> + ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
>> + if (ret == -ENOIOCTLCMD)
>> + pad = NULL;
>> + else if (ret < 0) {
>> + dev_err(sd->dev, "failed to get source configuration\n");
>> + return ret;
>> + }
>> + }
>> + if (!pad) {
>> + endpoint = &vmux->endpoint[vmux->active];
>> +
>> + /* Mirror the input side on the output side */
>> + cfg->type = endpoint->bus_type;
>> + if (cfg->type == V4L2_MBUS_PARALLEL ||
>> + cfg->type == V4L2_MBUS_BT656)
>> + cfg->flags = endpoint->bus.parallel.flags;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int video_mux_s_stream(struct v4l2_subdev *sd, int enable)
>> +{
>> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> + struct v4l2_subdev *upstream_sd;
>> + struct media_pad *pad;
>> +
>> + if (vmux->active == -1) {
>> + dev_err(sd->dev, "Can not start streaming on inactive mux\n");
>> + return -EINVAL;
>> + }
>> +
>> + pad = media_entity_remote_pad(&sd->entity.pads[vmux->active]);
>> + if (!pad) {
>> + dev_err(sd->dev, "Failed to find remote source pad\n");
>> + return -ENOLINK;
>> + }
>> +
>> + if (!is_media_entity_v4l2_subdev(pad->entity)) {
>> + dev_err(sd->dev, "Upstream entity is not a v4l2 subdev\n");
>> + return -ENODEV;
>> + }
>> +
>> + upstream_sd = media_entity_to_v4l2_subdev(pad->entity);
>> +
>> + return v4l2_subdev_call(upstream_sd, video, s_stream, enable);
>> +}
>> +
>> +static const struct v4l2_subdev_video_ops video_mux_subdev_video_ops = {
>> + .g_mbus_config = video_mux_g_mbus_config,
>> + .s_stream = video_mux_s_stream,
>> +};
>> +
>> +static struct v4l2_mbus_framefmt *
>> +__video_mux_get_pad_format(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_pad_config *cfg,
>> + unsigned int pad, u32 which)
>> +{
>> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> +
>> + switch (which) {
>> + case V4L2_SUBDEV_FORMAT_TRY:
>> + return v4l2_subdev_get_try_format(sd, cfg, pad);
>> + case V4L2_SUBDEV_FORMAT_ACTIVE:
>> + return &vmux->format_mbus[pad];
>> + default:
>> + return NULL;
>> + }
>> +}
>> +
>> +static int video_mux_get_format(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_format *sdformat)
>> +{
>> + sdformat->format = *__video_mux_get_pad_format(sd, cfg, sdformat->pad,
>> + sdformat->which);
>> + return 0;
>> +}
>> +
>> +static int video_mux_set_format(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_format *sdformat)
>> +{
>> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> + struct v4l2_mbus_framefmt *mbusformat;
>> +
>> + mbusformat = __video_mux_get_pad_format(sd, cfg, sdformat->pad,
>> + sdformat->which);
>> + if (!mbusformat)
>> + return -EINVAL;
>> +
>> + /* Source pad mirrors active sink pad, no limitations on sink pads */
>> + if (is_source_pad(vmux, sdformat->pad) && vmux->active >= 0)
>> + sdformat->format = vmux->format_mbus[vmux->active];
>> +
>> + *mbusformat = sdformat->format;
>> +
>> + return 0;
>> +}
>> +
>> +static struct v4l2_subdev_pad_ops video_mux_pad_ops = {
>> + .get_fmt = video_mux_get_format,
>> + .set_fmt = video_mux_set_format,
>> +};
>> +
>> +static struct v4l2_subdev_ops video_mux_subdev_ops = {
>> + .pad = &video_mux_pad_ops,
>> + .video = &video_mux_subdev_video_ops,
>> +};
>> +
>> +static int video_mux_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct device *dev = &pdev->dev;
>> + struct v4l2_of_endpoint endpoint;
>> + struct device_node *ep;
>> + struct video_mux *vmux;
>> + unsigned int num_pads = 0;
>> + int ret;
>> + int i;
>> +
>> + vmux = devm_kzalloc(dev, sizeof(*vmux), GFP_KERNEL);
>> + if (!vmux)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, vmux);
>> +
>> + v4l2_subdev_init(&vmux->subdev, &video_mux_subdev_ops);
>> + snprintf(vmux->subdev.name, sizeof(vmux->subdev.name), "%s", np->name);
>> + vmux->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> + vmux->subdev.dev = dev;
>> +
>> + /*
>> + * The largest numbered port is the output port. It determines
>> + * total number of pads.
>> + */
>> + for_each_endpoint_of_node(np, ep) {
>> + of_graph_parse_endpoint(ep, &endpoint.base);
>> + num_pads = max(num_pads, endpoint.base.port + 1);
>> + }
>> +
>> + if (num_pads < 2) {
>> + dev_err(dev, "Not enough ports %d\n", num_pads);
>> + return -EINVAL;
>> + }
>> +
>> + vmux->mux = devm_mux_control_get(dev, NULL);
>> + if (IS_ERR(vmux->mux)) {
>> + ret = PTR_ERR(vmux->mux);
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "Failed to get mux: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + vmux->active = -1;
>> + vmux->pads = devm_kzalloc(dev, sizeof(*vmux->pads) * num_pads,
>> + GFP_KERNEL);
>> + vmux->format_mbus = devm_kzalloc(dev, sizeof(*vmux->format_mbus) *
>> + num_pads, GFP_KERNEL);
>> + vmux->endpoint = devm_kzalloc(dev, sizeof(*vmux->endpoint) *
>> + (num_pads - 1), GFP_KERNEL);
>> +
>> + for (i = 0; i < num_pads - 1; i++)
>> + vmux->pads[i].flags = MEDIA_PAD_FL_SINK;
>> + vmux->pads[num_pads - 1].flags = MEDIA_PAD_FL_SOURCE;
>> +
>> + vmux->subdev.entity.function = MEDIA_ENT_F_VID_MUX;
>> + ret = media_entity_pads_init(&vmux->subdev.entity, num_pads,
>> + vmux->pads);
>> + if (ret < 0)
>> + return ret;
>> +
>> + vmux->subdev.entity.ops = &video_mux_ops;
>> +
>> + for_each_endpoint_of_node(np, ep) {
>> + v4l2_of_parse_endpoint(ep, &endpoint);
>> +
>> + if (video_mux_endpoint_disabled(ep)) {
>> + dev_dbg(dev, "port %d disabled\n", endpoint.base.port);
>> + continue;
>> + }
>> +
>> + vmux->endpoint[endpoint.base.port] = endpoint;
>> + }
>> +
>> + return v4l2_async_register_subdev(&vmux->subdev);
>> +}
>> +
>> +static int video_mux_remove(struct platform_device *pdev)
>> +{
>> + struct video_mux *vmux = platform_get_drvdata(pdev);
>> + struct v4l2_subdev *sd = &vmux->subdev;
>> +
>> + v4l2_async_unregister_subdev(sd);
>> + media_entity_cleanup(&sd->entity);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id video_mux_dt_ids[] = {
>> + { .compatible = "video-mux", },
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, video_mux_dt_ids);
>> +
>> +static struct platform_driver video_mux_driver = {
>> + .probe = video_mux_probe,
>> + .remove = video_mux_remove,
>> + .driver = {
>> + .of_match_table = video_mux_dt_ids,
>> + .name = "video-mux",
>> + },
>> +};
>> +
>> +module_platform_driver(video_mux_driver);
>> +
>> +MODULE_DESCRIPTION("video stream multiplexer");
>> +MODULE_AUTHOR("Sascha Hauer, Pengutronix");
>> +MODULE_AUTHOR("Philipp Zabel, Pengutronix");
>> +MODULE_LICENSE("GPL");
>>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver
From: Peter Rosin @ 2017-04-29 21:29 UTC (permalink / raw)
To: Philipp Zabel, linux-media-u79uwXL29TY76Z2rM5mHXA
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Steve Longerbeam, Sakari Ailus,
Pavel Machek, Rob Herring, Mark Rutland, Vladimir Zapolskiy,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, Sascha Hauer, Steve Longerbeam
In-Reply-To: <20170428141330.16187-2-p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
On 2017-04-28 16:13, Philipp Zabel wrote:
> This driver can handle SoC internal and external video bus multiplexers,
> controlled by mux controllers provided by the mux controller framework,
> such as MMIO register bitfields or GPIOs. The subdevice passes through
> the mbus configuration of the active input to the output side.
>
> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Steve Longerbeam <steve_longerbeam-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> ---
> This has been last sent as part of the i.MX media series.
>
> Changes since https://patchwork.kernel.org/patch/9647869/:
> - Split out the actual mux operation to be provided by the mux controller
> framework [1]. GPIO and MMIO control can be provided by individual mux
> controller drivers [2][3].
> [1] https://patchwork.kernel.org/patch/9695837/
> [2] https://patchwork.kernel.org/patch/9695839/
> [3] https://patchwork.kernel.org/patch/9704509/
> - Shortened 'video-multiplexer' to 'video-mux', replaced all instances of
> vidsw with video_mux.
> - Made the mux inactive by default, only activated by user interaction.
> - Added CONFIG_OF and CONFIG_MULTIPLEXER dependencies.
> - Reuse subdev.entity.num_pads instead of keeping our own count.
> - Removed implicit link disabling. Instead, trying to enable a second
> sink pad link yields -EBUSY.
> - Merged _async_init into _probe.
> - Removed superfluous pad index check from _set_format.
> - Added is_source_pad helper to tell source and sink pads apart.
> - Removed test for status property in endpoint nodes. Disable the remote
> device or sever the endpoint link to disable a sink pad.
> ---
> drivers/media/platform/Kconfig | 6 +
> drivers/media/platform/Makefile | 2 +
> drivers/media/platform/video-mux.c | 341 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 349 insertions(+)
> create mode 100644 drivers/media/platform/video-mux.c
>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index c9106e105baba..b046a6d39fee5 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -74,6 +74,12 @@ config VIDEO_M32R_AR_M64278
> To compile this driver as a module, choose M here: the
> module will be called arv.
>
> +config VIDEO_MUX
> + tristate "Video Multiplexer"
> + depends on OF && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER && MULTIPLEXER
> + help
> + This driver provides support for N:1 video bus multiplexers.
> +
> config VIDEO_OMAP3
> tristate "OMAP 3 Camera support"
> depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 349ddf6a69da2..fd2735ca3ff75 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU) += sh_veu.o
>
> obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE) += m2m-deinterlace.o
>
> +obj-$(CONFIG_VIDEO_MUX) += video-mux.o
> +
> obj-$(CONFIG_VIDEO_S3C_CAMIF) += s3c-camif/
> obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) += exynos4-is/
> obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG) += s5p-jpeg/
> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> new file mode 100644
> index 0000000000000..419541729f67e
> --- /dev/null
> +++ b/drivers/media/platform/video-mux.c
> @@ -0,0 +1,341 @@
> +/*
> + * video stream multiplexer controlled via mux control
> + *
> + * Copyright (C) 2013 Pengutronix, Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> + * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2017?
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/mux/consumer.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-of.h>
> +
> +struct video_mux {
> + struct v4l2_subdev subdev;
> + struct media_pad *pads;
> + struct v4l2_mbus_framefmt *format_mbus;
> + struct v4l2_of_endpoint *endpoint;
> + struct mux_control *mux;
> + int active;
> +};
> +
> +static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
> +{
> + return container_of(sd, struct video_mux, subdev);
> +}
> +
> +static inline bool is_source_pad(struct video_mux *vmux, unsigned int pad)
> +{
> + return pad == vmux->subdev.entity.num_pads - 1;
> +}
> +
> +static int video_mux_link_setup(struct media_entity *entity,
> + const struct media_pad *local,
> + const struct media_pad *remote, u32 flags)
> +{
> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> + int ret;
> +
> + /*
> + * The mux state is determined by the enabled sink pad link.
> + * Enabling or disabling the source pad link has no effect.
> + */
> + if (is_source_pad(vmux, local->index))
> + return 0;
> +
> + dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
> + remote->entity->name, remote->index, local->entity->name,
> + local->index, flags & MEDIA_LNK_FL_ENABLED);
> +
> + if (flags & MEDIA_LNK_FL_ENABLED) {
> + if (vmux->active == local->index)
Here, you shortcut the mux_control_select_trylock test and return "OK"
based on a driver-local variable that is intended to keep track of mux
ownership.
> + return 0;
> +
> + if (vmux->active >= 0)
Here too (and this check is not needed, the situation will be covered by
the mux_control_try_select call).
> + return -EBUSY;
> +
> + dev_dbg(sd->dev, "setting %d active\n", local->index);
> + ret = mux_control_try_select(vmux->mux, local->index);
> + if (ret < 0)
> + return ret;
> + vmux->active = local->index;
> + } else {
> + if (vmux->active != local->index)
> + return 0;
> +
> + dev_dbg(sd->dev, "going inactive\n");
> + mux_control_deselect(vmux->mux);
But here you let go of the mux *before* you clear the driver-local
ownership indicator. That looks suspicious. My guess is that this is
"safe" because the upper layers has some serialization, but I don't
know. Anyway, even if there is something saving you in the upper
layers, it looks out of order and unneeded. I would have moved the
below vmux->active = -1; statement up to before the above deselect.
With that fixed, mux usage looks good to me, so you can add an Acked-
by from me if you wish (goes for the bindings patch as well).
Cheers,
peda
> + vmux->active = -1;
> + }
> +
> + return 0;
> +}
> +
> +static struct media_entity_operations video_mux_ops = {
> + .link_setup = video_mux_link_setup,
> + .link_validate = v4l2_subdev_link_validate,
> +};
> +
> +static bool video_mux_endpoint_disabled(struct device_node *ep)
> +{
> + struct device_node *rpp = of_graph_get_remote_port_parent(ep);
> +
> + return !of_device_is_available(rpp);
> +}
> +
> +static int video_mux_g_mbus_config(struct v4l2_subdev *sd,
> + struct v4l2_mbus_config *cfg)
> +{
> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> + struct v4l2_of_endpoint *endpoint;
> + struct media_pad *pad;
> + int ret;
> +
> + if (vmux->active == -1) {
> + dev_err(sd->dev, "no configuration for inactive mux\n");
> + return -EINVAL;
> + }
> +
> + /*
> + * Retrieve media bus configuration from the entity connected to the
> + * active input
> + */
> + pad = media_entity_remote_pad(&vmux->pads[vmux->active]);
> + if (pad) {
> + sd = media_entity_to_v4l2_subdev(pad->entity);
> + ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
> + if (ret == -ENOIOCTLCMD)
> + pad = NULL;
> + else if (ret < 0) {
> + dev_err(sd->dev, "failed to get source configuration\n");
> + return ret;
> + }
> + }
> + if (!pad) {
> + endpoint = &vmux->endpoint[vmux->active];
> +
> + /* Mirror the input side on the output side */
> + cfg->type = endpoint->bus_type;
> + if (cfg->type == V4L2_MBUS_PARALLEL ||
> + cfg->type == V4L2_MBUS_BT656)
> + cfg->flags = endpoint->bus.parallel.flags;
> + }
> +
> + return 0;
> +}
> +
> +static int video_mux_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> + struct v4l2_subdev *upstream_sd;
> + struct media_pad *pad;
> +
> + if (vmux->active == -1) {
> + dev_err(sd->dev, "Can not start streaming on inactive mux\n");
> + return -EINVAL;
> + }
> +
> + pad = media_entity_remote_pad(&sd->entity.pads[vmux->active]);
> + if (!pad) {
> + dev_err(sd->dev, "Failed to find remote source pad\n");
> + return -ENOLINK;
> + }
> +
> + if (!is_media_entity_v4l2_subdev(pad->entity)) {
> + dev_err(sd->dev, "Upstream entity is not a v4l2 subdev\n");
> + return -ENODEV;
> + }
> +
> + upstream_sd = media_entity_to_v4l2_subdev(pad->entity);
> +
> + return v4l2_subdev_call(upstream_sd, video, s_stream, enable);
> +}
> +
> +static const struct v4l2_subdev_video_ops video_mux_subdev_video_ops = {
> + .g_mbus_config = video_mux_g_mbus_config,
> + .s_stream = video_mux_s_stream,
> +};
> +
> +static struct v4l2_mbus_framefmt *
> +__video_mux_get_pad_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + unsigned int pad, u32 which)
> +{
> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> +
> + switch (which) {
> + case V4L2_SUBDEV_FORMAT_TRY:
> + return v4l2_subdev_get_try_format(sd, cfg, pad);
> + case V4L2_SUBDEV_FORMAT_ACTIVE:
> + return &vmux->format_mbus[pad];
> + default:
> + return NULL;
> + }
> +}
> +
> +static int video_mux_get_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *sdformat)
> +{
> + sdformat->format = *__video_mux_get_pad_format(sd, cfg, sdformat->pad,
> + sdformat->which);
> + return 0;
> +}
> +
> +static int video_mux_set_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *sdformat)
> +{
> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> + struct v4l2_mbus_framefmt *mbusformat;
> +
> + mbusformat = __video_mux_get_pad_format(sd, cfg, sdformat->pad,
> + sdformat->which);
> + if (!mbusformat)
> + return -EINVAL;
> +
> + /* Source pad mirrors active sink pad, no limitations on sink pads */
> + if (is_source_pad(vmux, sdformat->pad) && vmux->active >= 0)
> + sdformat->format = vmux->format_mbus[vmux->active];
> +
> + *mbusformat = sdformat->format;
> +
> + return 0;
> +}
> +
> +static struct v4l2_subdev_pad_ops video_mux_pad_ops = {
> + .get_fmt = video_mux_get_format,
> + .set_fmt = video_mux_set_format,
> +};
> +
> +static struct v4l2_subdev_ops video_mux_subdev_ops = {
> + .pad = &video_mux_pad_ops,
> + .video = &video_mux_subdev_video_ops,
> +};
> +
> +static int video_mux_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct device *dev = &pdev->dev;
> + struct v4l2_of_endpoint endpoint;
> + struct device_node *ep;
> + struct video_mux *vmux;
> + unsigned int num_pads = 0;
> + int ret;
> + int i;
> +
> + vmux = devm_kzalloc(dev, sizeof(*vmux), GFP_KERNEL);
> + if (!vmux)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, vmux);
> +
> + v4l2_subdev_init(&vmux->subdev, &video_mux_subdev_ops);
> + snprintf(vmux->subdev.name, sizeof(vmux->subdev.name), "%s", np->name);
> + vmux->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + vmux->subdev.dev = dev;
> +
> + /*
> + * The largest numbered port is the output port. It determines
> + * total number of pads.
> + */
> + for_each_endpoint_of_node(np, ep) {
> + of_graph_parse_endpoint(ep, &endpoint.base);
> + num_pads = max(num_pads, endpoint.base.port + 1);
> + }
> +
> + if (num_pads < 2) {
> + dev_err(dev, "Not enough ports %d\n", num_pads);
> + return -EINVAL;
> + }
> +
> + vmux->mux = devm_mux_control_get(dev, NULL);
> + if (IS_ERR(vmux->mux)) {
> + ret = PTR_ERR(vmux->mux);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "Failed to get mux: %d\n", ret);
> + return ret;
> + }
> +
> + vmux->active = -1;
> + vmux->pads = devm_kzalloc(dev, sizeof(*vmux->pads) * num_pads,
> + GFP_KERNEL);
> + vmux->format_mbus = devm_kzalloc(dev, sizeof(*vmux->format_mbus) *
> + num_pads, GFP_KERNEL);
> + vmux->endpoint = devm_kzalloc(dev, sizeof(*vmux->endpoint) *
> + (num_pads - 1), GFP_KERNEL);
> +
> + for (i = 0; i < num_pads - 1; i++)
> + vmux->pads[i].flags = MEDIA_PAD_FL_SINK;
> + vmux->pads[num_pads - 1].flags = MEDIA_PAD_FL_SOURCE;
> +
> + vmux->subdev.entity.function = MEDIA_ENT_F_VID_MUX;
> + ret = media_entity_pads_init(&vmux->subdev.entity, num_pads,
> + vmux->pads);
> + if (ret < 0)
> + return ret;
> +
> + vmux->subdev.entity.ops = &video_mux_ops;
> +
> + for_each_endpoint_of_node(np, ep) {
> + v4l2_of_parse_endpoint(ep, &endpoint);
> +
> + if (video_mux_endpoint_disabled(ep)) {
> + dev_dbg(dev, "port %d disabled\n", endpoint.base.port);
> + continue;
> + }
> +
> + vmux->endpoint[endpoint.base.port] = endpoint;
> + }
> +
> + return v4l2_async_register_subdev(&vmux->subdev);
> +}
> +
> +static int video_mux_remove(struct platform_device *pdev)
> +{
> + struct video_mux *vmux = platform_get_drvdata(pdev);
> + struct v4l2_subdev *sd = &vmux->subdev;
> +
> + v4l2_async_unregister_subdev(sd);
> + media_entity_cleanup(&sd->entity);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id video_mux_dt_ids[] = {
> + { .compatible = "video-mux", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, video_mux_dt_ids);
> +
> +static struct platform_driver video_mux_driver = {
> + .probe = video_mux_probe,
> + .remove = video_mux_remove,
> + .driver = {
> + .of_match_table = video_mux_dt_ids,
> + .name = "video-mux",
> + },
> +};
> +
> +module_platform_driver(video_mux_driver);
> +
> +MODULE_DESCRIPTION("video stream multiplexer");
> +MODULE_AUTHOR("Sascha Hauer, Pengutronix");
> +MODULE_AUTHOR("Philipp Zabel, Pengutronix");
> +MODULE_LICENSE("GPL");
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC 1/3] dt-binding: soc: qcom: Add binding for RFSA
From: Bjorn Andersson @ 2017-04-29 20:02 UTC (permalink / raw)
To: Rob Herring
Cc: Andy Gross, David Brown, Frank Rowand, Mark Rutland,
linux-arm-msm, linux-soc, devicetree, linux-kernel
In-Reply-To: <20170428174239.x62opv5vzx2ce2wt@rob-hp-laptop>
On Fri 28 Apr 10:42 PDT 2017, Rob Herring wrote:
> On Sat, Apr 22, 2017 at 10:35:17AM -0700, Bjorn Andersson wrote:
> > This adds the binding for describing shared memory buffers for
> > implementing the remote filesystem protocol.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >
> > My initial attempt was to mimic the ramoops of just adding the compatible to
> > the reserved-memory node, but I have not been able to figure out a sane way of
> > getting hold of the base address in the case that the memory region is
> > described my a "size" only (done on some platforms).
>
> I prefer the ramoops way.
>
Me too :)
> > The problem is that we create the reserved_mem objects (and remove the
> > memblocks) while we're still operating on the flattened representation, so
> > without a phandle it doesn't seem like we have anything to perform the
> > comparison with later on.
>
> I'm not sure I follow.
>
In my first attempt I extended of_platform_default_populate_init() to
also probe my platform_driver and like ramoops acquired the values of
reg and memremap() these. This works fine.
But for some platforms the memory-region doesn't need a fixed location,
it's just required to be a consecutive chunk of physical ram. So I
replace the "reg = <>" with a "size = <>", this causes
__reserved_mem_alloc_size() to carve out a chunk of memory somewhere
and update the associated reserved_mem entry. The reserved_mem will be
populated with the phandle from the [flattened] of_node.
Later my platform_device is instantiated and I need to figure out the
base address that was picked earlier - so that I know which region to
memremap().
But as my "rfsa-node" stands on its own it will not have any "phandle",
so the reserved_mem->phandle will remain 0. Further more I only have the
unflattened of_node - so I can't just compare the address with the
flattened version previously used.
So the problem at hand is how to match my pdev->dev.of_node to an entry
in the reserved_mem array (in of_reserved_mem.c).
> >
> > .../devicetree/bindings/soc/qcom/qcom,rfsa.txt | 43 ++++++++++++++++++++++
> > 1 file changed, 43 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,rfsa.txt
> >
> > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,rfsa.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,rfsa.txt
> > new file mode 100644
> > index 000000000000..b4de0de74e46
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,rfsa.txt
> > @@ -0,0 +1,43 @@
> > +Qualcomm Remote File System Access binding
> > +
> > +This binding describes the Qualcomm RFSA, which serves the purpose of managing
> > +the shared memory region used for remote processors to access block device data
> > +using the Remote Filesystem protocol.
> > +
> > +- compatible:
> > + Usage: required
> > + Value type: <stringlist>
> > + Definition: must be:
> > + "qcom,rfsa"
>
> No versioning?
>
The binding is used to describe a chunk of memory and an associated
client-id of this memory, so I'm not sure if we need a version.
> > +
> > +- memory-region:
> > + Usage: required
> > + Value type: <prop-encoded-array>
> > + Definition: handle to memory reservation the associated rfsa region.
> > +
> > +- qcom,client-id:
> > + Usage: required
> > + Value type: <u32>
> > + Definition: identifier of the client to use this region for buffers.
>
> What determines these numbers?
>
The bigger picture of this is that the remote processors (e.g. modem)
needs access to block storage, so it sends request to a service on the
system with storage access (i.e the application processor) which will
read and write from the file system, storing blocks of data in the
rfsa-memory.
So the client-id is the hard coded identifier of each such remote system
requesting IO - each one with its own memory carveout.
In later platforms we need to configure the SMMU for each remote in
order to give them access to these cerveouts, so I don't see that its
possible to mash them together in one chunk and handle the client-id
thing only in the application.
> > +
> > += EXAMPLE
> > +The following example shows the RFSA setup for APQ8016, with the RFSA region
> > +for the Hexagon DSP (id #1) located at 0x86700000.
> > +
> > + reserved-memory {
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + ranges;
> > +
> > + rmtfs: rmtfs@86700000 {
>
> I think you should have a compatible here even with the 2 node approach.
>
I'm hoping we can figure out a way to fix the Linux implementation so
that we can describe it fully in one node.
> > + reg = <0x0 0x86700000 0x0 0xe0000>;
> > + no-map;
> > + };
> > + };
> > +
> > + hexagon-rfsa {
> > + compatible = "qcom,rfsa";
> > + memory-region = <&rmtfs>;
> > +
> > + qcom,client-id = <1>;
> > + };
> > --
> > 2.12.0
> >
Regards,
Bjorn
^ permalink raw reply
* [PATCH v2 5/5] mtd: nand: fsmc_nand: handle on-die ECC case
From: Thomas Petazzoni @ 2017-04-29 9:06 UTC (permalink / raw)
To: Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen,
David Woodhouse, Brian Norris,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Ian Campbell,
Pawel Moll, Mark Rutland, Kumar Gala
Cc: Thomas Petazzoni
In-Reply-To: <1493456806-18898-1-git-send-email-thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
This commit adjusts the fsmc_nand driver so that it accepts the
NAND_ECC_ON_DIE case. It simply does nothing in this case, since both
the ECC operations and OOB layout will be defined by the NAND chip code
rather than by the NAND controller code.
Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Reviewed-by: Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org>
---
drivers/mtd/nand/fsmc_nand.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
index cea50d2..5c058e8 100644
--- a/drivers/mtd/nand/fsmc_nand.c
+++ b/drivers/mtd/nand/fsmc_nand.c
@@ -986,6 +986,9 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
break;
}
+ case NAND_ECC_ON_DIE:
+ break;
+
default:
dev_err(&pdev->dev, "Unsupported ECC mode!\n");
goto err_probe;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v2 4/5] mtd: nand: add support for Micron on-die ECC
From: Thomas Petazzoni @ 2017-04-29 9:06 UTC (permalink / raw)
To: Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen,
David Woodhouse, Brian Norris,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Ian Campbell,
Pawel Moll, Mark Rutland, Kumar Gala
Cc: Thomas Petazzoni
In-Reply-To: <1493456806-18898-1-git-send-email-thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Now that the core NAND subsystem has support for on-die ECC, this commit
brings the necessary code to support on-die ECC on Micron NANDs.
In micron_nand_init(), we detect if the Micron NAND chip supports on-die
ECC mode, by checking a number of conditions:
- It must be an ONFI NAND
- It must be a SLC NAND
- Enabling *and* disabling on-die ECC must work
- The on-die ECC must be correcting 4 bits per 512 bytes of data. Some
Micron NAND chips have an on-die ECC able to correct 8 bits per 512
bytes of data, but they work slightly differently and therefore we
don't support them in this patch.
Then, if the on-die ECC cannot be disabled (some Micron NAND have on-die
ECC forcefully enabled), we bail out, as we don't support such
NANDs. Indeed, the implementation of raw_read()/raw_write() make the
assumption that on-die ECC can be disabled. Support for Micron NANDs
with on-die ECC forcefully enabled can easily be added, but in the
absence of such HW for testing, we preferred to simply bail out.
If the on-die ECC is supported, and requested in the Device Tree, then
it is indeed enabled, by using custom implementations of the
->read_page(), ->read_page_raw(), ->write_page() and ->write_page_raw()
operation to properly handle the on-die ECC.
In the non-raw functions, we need to enable the internal ECC engine
before issuing the NAND_CMD_READ0 or NAND_CMD_SEQIN commands, which is
why we set the NAND_ECC_CUSTOM_PAGE_ACCESS option at initialization
time (it asks the NAND core to let the NAND driver issue those
commands).
Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
drivers/mtd/nand/nand_micron.c | 215 +++++++++++++++++++++++++++++++++++++++++
include/linux/mtd/nand.h | 2 +
2 files changed, 217 insertions(+)
diff --git a/drivers/mtd/nand/nand_micron.c b/drivers/mtd/nand/nand_micron.c
index 8770110..0987f32 100644
--- a/drivers/mtd/nand/nand_micron.c
+++ b/drivers/mtd/nand/nand_micron.c
@@ -17,6 +17,12 @@
#include <linux/mtd/nand.h>
+/*
+ * Special Micron status bit that indicates when the block has been
+ * corrected by on-die ECC and should be rewritten
+ */
+#define NAND_STATUS_WRITE_RECOMMENDED BIT(3)
+
struct nand_onfi_vendor_micron {
u8 two_plane_read;
u8 read_cache;
@@ -66,9 +72,191 @@ static int micron_nand_onfi_init(struct nand_chip *chip)
return 0;
}
+static int micron_nand_on_die_ooblayout_ecc(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *oobregion)
+{
+ if (section >= 4)
+ return -ERANGE;
+
+ oobregion->offset = (section * 16) + 8;
+ oobregion->length = 8;
+
+ return 0;
+}
+
+static int micron_nand_on_die_ooblayout_free(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *oobregion)
+{
+ if (section >= 4)
+ return -ERANGE;
+
+ oobregion->offset = (section * 16) + 2;
+ oobregion->length = 6;
+
+ return 0;
+}
+
+static const struct mtd_ooblayout_ops micron_nand_on_die_ooblayout_ops = {
+ .ecc = micron_nand_on_die_ooblayout_ecc,
+ .free = micron_nand_on_die_ooblayout_free,
+};
+
+static int micron_nand_on_die_ecc_setup(struct nand_chip *chip, bool enable)
+{
+ u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = { 0, };
+
+ if (enable)
+ feature[0] |= ONFI_FEATURE_ON_DIE_ECC_EN;
+
+ return chip->onfi_set_features(nand_to_mtd(chip), chip,
+ ONFI_FEATURE_ON_DIE_ECC, feature);
+}
+
+static int
+micron_nand_read_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
+ uint8_t *buf, int oob_required,
+ int page)
+{
+ int status;
+ int max_bitflips = 0;
+
+ micron_nand_on_die_ecc_setup(chip, true);
+
+ chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
+ chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
+ status = chip->read_byte(mtd);
+ if (status & NAND_STATUS_FAIL)
+ mtd->ecc_stats.failed++;
+ /*
+ * The internal ECC doesn't tell us the number of bitflips
+ * that have been corrected, but tells us if it recommends to
+ * rewrite the block. If it's the case, then we pretend we had
+ * a number of bitflips equal to the ECC strength, which will
+ * hint the NAND core to rewrite the block.
+ */
+ else if (status & NAND_STATUS_WRITE_RECOMMENDED)
+ max_bitflips = chip->ecc.strength;
+
+ chip->cmdfunc(mtd, NAND_CMD_READ0, -1, -1);
+
+ nand_read_page_raw(mtd, chip, buf, oob_required, page);
+
+ micron_nand_on_die_ecc_setup(chip, false);
+
+ return max_bitflips;
+}
+
+static int
+micron_nand_write_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
+ const uint8_t *buf, int oob_required,
+ int page)
+{
+ micron_nand_on_die_ecc_setup(chip, true);
+
+ chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
+ nand_write_page_raw(mtd, chip, buf, oob_required, page);
+ chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
+
+ micron_nand_on_die_ecc_setup(chip, false);
+
+ return 0;
+}
+
+static int
+micron_nand_read_page_raw_on_die_ecc(struct mtd_info *mtd,
+ struct nand_chip *chip,
+ uint8_t *buf, int oob_required,
+ int page)
+{
+ chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
+ nand_read_page_raw(mtd, chip, buf, oob_required, page);
+
+ return 0;
+}
+
+static int
+micron_nand_write_page_raw_on_die_ecc(struct mtd_info *mtd,
+ struct nand_chip *chip,
+ const uint8_t *buf, int oob_required,
+ int page)
+{
+ chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
+ nand_write_page_raw(mtd, chip, buf, oob_required, page);
+ chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
+
+ return 0;
+}
+
+enum {
+ /* The NAND flash doesn't support on-die ECC */
+ MICRON_ON_DIE_UNSUPPORTED,
+
+ /*
+ * The NAND flash supports on-die ECC and it can be
+ * enabled/disabled by a set features command.
+ */
+ MICRON_ON_DIE_SUPPORTED,
+
+ /*
+ * The NAND flash supports on-die ECC, and it cannot be
+ * disabled.
+ */
+ MICRON_ON_DIE_MANDATORY,
+};
+
+/*
+ * Try to detect if the NAND support on-die ECC. To do this, we enable
+ * the feature, and read back if it has been enabled as expected. We
+ * also check if it can be disabled, because some Micron NANDs do not
+ * allow disabling the on-die ECC and we don't support such NANDs for
+ * now.
+ *
+ * This function also has the side effect of disabling on-die ECC if
+ * it had been left enabled by the firmware/bootloader.
+ */
+static int micron_supports_on_die_ecc(struct nand_chip *chip)
+{
+ u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = { 0, };
+ int ret;
+
+ if (chip->onfi_version == 0)
+ return MICRON_ON_DIE_UNSUPPORTED;
+
+ if (chip->bits_per_cell != 1)
+ return MICRON_ON_DIE_UNSUPPORTED;
+
+ ret = micron_nand_on_die_ecc_setup(chip, true);
+ if (ret)
+ return MICRON_ON_DIE_UNSUPPORTED;
+
+ chip->onfi_get_features(nand_to_mtd(chip), chip,
+ ONFI_FEATURE_ON_DIE_ECC, feature);
+ if ((feature[0] & ONFI_FEATURE_ON_DIE_ECC_EN) == 0)
+ return MICRON_ON_DIE_UNSUPPORTED;
+
+ ret = micron_nand_on_die_ecc_setup(chip, false);
+ if (ret)
+ return MICRON_ON_DIE_UNSUPPORTED;
+
+ chip->onfi_get_features(nand_to_mtd(chip), chip,
+ ONFI_FEATURE_ON_DIE_ECC, feature);
+ if (feature[0] & ONFI_FEATURE_ON_DIE_ECC_EN)
+ return MICRON_ON_DIE_MANDATORY;
+
+ /*
+ * Some Micron NANDs have an on-die ECC of 4/512, some other
+ * 8/512. We only support the former.
+ */
+ if (chip->onfi_params.ecc_bits != 4)
+ return MICRON_ON_DIE_UNSUPPORTED;
+
+ return MICRON_ON_DIE_SUPPORTED;
+}
+
static int micron_nand_init(struct nand_chip *chip)
{
struct mtd_info *mtd = nand_to_mtd(chip);
+ int ondie;
int ret;
ret = micron_nand_onfi_init(chip);
@@ -78,6 +266,33 @@ static int micron_nand_init(struct nand_chip *chip)
if (mtd->writesize == 2048)
chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
+ ondie = micron_supports_on_die_ecc(chip);
+
+ if (ondie == MICRON_ON_DIE_MANDATORY) {
+ pr_err("On-die ECC forcefully enabled, not supported\n");
+ return -EINVAL;
+ }
+
+ if (chip->ecc.mode == NAND_ECC_ON_DIE) {
+ if (ondie == MICRON_ON_DIE_UNSUPPORTED) {
+ pr_err("On-die ECC selected but not supported\n");
+ return -EINVAL;
+ }
+
+ chip->ecc.options = NAND_ECC_CUSTOM_PAGE_ACCESS;
+ chip->ecc.bytes = 32;
+ chip->ecc.strength = 4;
+ chip->ecc.algo = NAND_ECC_BCH;
+ chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
+ chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
+ chip->ecc.read_page_raw =
+ micron_nand_read_page_raw_on_die_ecc;
+ chip->ecc.write_page_raw =
+ micron_nand_write_page_raw_on_die_ecc;
+
+ mtd_set_ooblayout(mtd, µn_nand_on_die_ooblayout_ops);
+ }
+
return 0;
}
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 7a01d2e..f019916 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -258,6 +258,8 @@ struct nand_chip;
/* Vendor-specific feature address (Micron) */
#define ONFI_FEATURE_ADDR_READ_RETRY 0x89
+#define ONFI_FEATURE_ON_DIE_ECC 0x90
+#define ONFI_FEATURE_ON_DIE_ECC_EN BIT(3)
/* ONFI subfeature parameters length */
#define ONFI_SUBFEATURE_PARAM_LEN 4
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v2 3/5] mtd: nand: export nand_{read,write}_page_raw()
From: Thomas Petazzoni @ 2017-04-29 9:06 UTC (permalink / raw)
To: Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen,
David Woodhouse, Brian Norris,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Ian Campbell,
Pawel Moll, Mark Rutland, Kumar Gala
Cc: Thomas Petazzoni
In-Reply-To: <1493456806-18898-1-git-send-email-thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
The nand_read_page_raw() and nand_write_page_raw() functions might be
re-used by vendor-specific implementations of the read_page/write_page
functions. Instead of having vendor-specific code duplicate this code,
it is much better to export those functions and allow them to be
re-used.
Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Reviewed-by: Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org>
---
drivers/mtd/nand/nand_base.c | 10 ++++++----
include/linux/mtd/nand.h | 8 ++++++++
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index b639e88..3282738 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1456,14 +1456,15 @@ EXPORT_SYMBOL(nand_check_erased_ecc_chunk);
*
* Not for syndrome calculating ECC controllers, which use a special oob layout.
*/
-static int nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
- uint8_t *buf, int oob_required, int page)
+int nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+ uint8_t *buf, int oob_required, int page)
{
chip->read_buf(mtd, buf, mtd->writesize);
if (oob_required)
chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
return 0;
}
+EXPORT_SYMBOL(nand_read_page_raw);
/**
* nand_read_page_raw_syndrome - [INTERN] read raw page data without ecc
@@ -2401,8 +2402,8 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from,
*
* Not for syndrome calculating ECC controllers, which use a special oob layout.
*/
-static int nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
- const uint8_t *buf, int oob_required, int page)
+int nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+ const uint8_t *buf, int oob_required, int page)
{
chip->write_buf(mtd, buf, mtd->writesize);
if (oob_required)
@@ -2410,6 +2411,7 @@ static int nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
return 0;
}
+EXPORT_SYMBOL(nand_write_page_raw);
/**
* nand_write_page_raw_syndrome - [INTERN] raw page write function
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 6035220..7a01d2e 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -1259,6 +1259,14 @@ int nand_read_oob_std(struct mtd_info *mtd, struct nand_chip *chip, int page);
int nand_read_oob_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
int page);
+/* Default read_page_raw implementation */
+int nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+ uint8_t *buf, int oob_required, int page);
+
+/* Default write_page_raw implementation */
+int nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+ const uint8_t *buf, int oob_required, int page);
+
/* Reset and initialize a NAND device */
int nand_reset(struct nand_chip *chip, int chipnr);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v2 2/5] mtd: nand: add core support for on-die ECC
From: Thomas Petazzoni @ 2017-04-29 9:06 UTC (permalink / raw)
To: Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen,
David Woodhouse, Brian Norris,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Ian Campbell,
Pawel Moll, Mark Rutland, Kumar Gala
Cc: Thomas Petazzoni
In-Reply-To: <1493456806-18898-1-git-send-email-thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
A number of NAND flashes have a capability called "on-die ECC" where the
NAND chip itself is capable of detecting and correcting errors.
Linux already has support for using the ECC implementation of the NAND
controller, or a software based ECC implementation, but not for using
the ECC implementation of the NAND controller. However, such an
implementation is sometimes useful in situations where the NAND
controller provides ECC algorithms that are not strong enough for the
NAND chip used on the system. A typical case is a NAND chip that
requires a 4-bit ECC, while the NAND controller only provides a 1-bit
ECC algorithm.
This commit introduces the support for the NAND_ECC_ON_DIE ECC mode:
- Parsing of the "on-die" value for the "nand-ecc-mode" Device Tree
property
- Handling NAND_ECC_ON_DIE case in nand_scan_tail(). The idea is that
the vendor specific code for the NAND chip must implement
->read_page() and ->write_page(). It may optionally provide its own
->read_page_raw() and ->write_page_raw() as well. For OOB operation,
we assume the standard operations are good enough, but they can be
overridden by the vendor specific code if needed.
Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Reviewed-by: Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org>
---
drivers/mtd/nand/nand_base.c | 13 +++++++++++++
include/linux/mtd/nand.h | 1 +
2 files changed, 14 insertions(+)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index ed49a1d..b639e88 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4109,6 +4109,7 @@ static const char * const nand_ecc_modes[] = {
[NAND_ECC_HW] = "hw",
[NAND_ECC_HW_SYNDROME] = "hw_syndrome",
[NAND_ECC_HW_OOB_FIRST] = "hw_oob_first",
+ [NAND_ECC_ON_DIE] = "on-die",
};
static int of_get_nand_ecc_mode(struct device_node *np)
@@ -4649,6 +4650,18 @@ int nand_scan_tail(struct mtd_info *mtd)
}
break;
+ case NAND_ECC_ON_DIE:
+ if (!ecc->read_page || !ecc->write_page) {
+ WARN(1, "No ECC functions supplied; on-die ECC not possible\n");
+ ret = -EINVAL;
+ goto err_free;
+ }
+ if (!ecc->read_oob)
+ ecc->read_oob = nand_read_oob_std;
+ if (!ecc->write_oob)
+ ecc->write_oob = nand_write_oob_std;
+ break;
+
case NAND_ECC_NONE:
pr_warn("NAND_ECC_NONE selected by board driver. This is not recommended!\n");
ecc->read_page = nand_read_page_raw;
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 8f67b15..6035220 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -116,6 +116,7 @@ typedef enum {
NAND_ECC_HW,
NAND_ECC_HW_SYNDROME,
NAND_ECC_HW_OOB_FIRST,
+ NAND_ECC_ON_DIE,
} nand_ecc_modes_t;
enum nand_ecc_algo {
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v2 1/5] dt-bindings: mtd: document new "on-die" nand-ecc-mode
From: Thomas Petazzoni @ 2017-04-29 9:06 UTC (permalink / raw)
To: Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen,
David Woodhouse, Brian Norris,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Ian Campbell,
Pawel Moll, Mark Rutland, Kumar Gala
Cc: Thomas Petazzoni
In-Reply-To: <1493456806-18898-1-git-send-email-thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
A number of NAND chips support a feature called on-die ECC, where the
NAND chip itself is capable of doing error detection and correction. The
new "on-die" value for nand-ecc-mode indicates that we want this
functionality to be used.
Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Reviewed-by: Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org>
---
Documentation/devicetree/bindings/mtd/nand.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt
index b056016..133f381 100644
--- a/Documentation/devicetree/bindings/mtd/nand.txt
+++ b/Documentation/devicetree/bindings/mtd/nand.txt
@@ -21,7 +21,7 @@ Optional NAND chip properties:
- nand-ecc-mode : String, operation mode of the NAND ecc mode.
Supported values are: "none", "soft", "hw", "hw_syndrome",
- "hw_oob_first".
+ "hw_oob_first", "on-die".
Deprecated values:
"soft_bch": use "soft" and nand-ecc-algo instead
- nand-ecc-algo: string, algorithm of NAND ECC.
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v2 0/5] mtd: nand: add support for on-die ECC
From: Thomas Petazzoni @ 2017-04-29 9:06 UTC (permalink / raw)
To: Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen,
David Woodhouse, Brian Norris,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Ian Campbell,
Pawel Moll, Mark Rutland, Kumar Gala
Cc: Thomas Petazzoni
Hello,
This patch series adds support for on-die ECC, i.e ECC performed by
the NAND chip itself, as opposed to having the ECC calculated by the
NAND controller or in software.
It is useful in situations where the NAND chip has an ECC requirement
that is not met by the ECC capabilities of the NAND controller.
Patch 1 adjusts the NAND generic DT binding to add "on-die" as an
nand-ecc-mode.
Patch 2 adds the core support for on-die ECC, which is really simple
and minimal: all the work is really done in the vendor-specific NAND
code.
Patch 3 and 4 adds the on-die ECC implementation for Micron NANDs.
Patch 5 allows the FSMC NAND driver to use on-die ECC.
This series was tested on a SPEAr600 platform, with a Micron
MT29F1G08ABADAWP NAND. The mtd_nandbiterrs.ko test is passing
successfully, which shows that the on-die ECC is correcting bitflips
as expected.
This series is based on the current nand-next branch, as it depends on
the rework from Boris of the vendor-specific NAND code.
Many thanks to Boris from providing lots of useful feedback and
discussion during the development of this patch series.
Changes since v1:
- Rebased on the latest nand/next branch.
- Reworked the mechanism to detect if the Micron NAND suppors on-die
ECC. We realized that the READ_ID command only indicates if on-die
ECC is currently enabled or not, not whether it is supported by the
NAND. After discussion with Bean Huo from Micron we came up with a
logic to determine if the NAND has on-die ECC support, which PATCH
4/5 implements.
Note that we intentionally only support cases that we could
test. Therefore, we error out if the NAND has an on-die ECC that
cannot be disabled, and we error out if the NAND uses a 8 bits per
512 bytes on-die ECC, since it requires a slightly different
handling than the 4 bits per 512 bytes on-die ECC our NAND is
using.
- Added Acked-by from Rob Herring on PATCH 1/5 (DT binding)
- Added Acked-by from Richard Weinberger on all patches, except PATCH
4/5 since it was significantly changed.
Best regards,
Thomas
Thomas Petazzoni (5):
dt-bindings: mtd: document new "on-die" nand-ecc-mode
mtd: nand: add core support for on-die ECC
mtd: nand: export nand_{read,write}_page_raw()
mtd: nand: add support for Micron on-die ECC
mtd: nand: fsmc_nand: handle on-die ECC case
Documentation/devicetree/bindings/mtd/nand.txt | 2 +-
drivers/mtd/nand/fsmc_nand.c | 3 +
drivers/mtd/nand/nand_base.c | 23 ++-
drivers/mtd/nand/nand_micron.c | 215 +++++++++++++++++++++++++
include/linux/mtd/nand.h | 11 ++
5 files changed, 249 insertions(+), 5 deletions(-)
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] regulator: Allow for asymmetric settling times
From: Laxman Dewangan @ 2017-04-29 8:02 UTC (permalink / raw)
To: Matthias Kaehlcke, Liam Girdwood, Mark Brown, Rob Herring,
Mark Rutland
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Douglas Anderson, Brian Norris
In-Reply-To: <20170429000643.56407-1-mka-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
On Saturday 29 April 2017 05:36 AM, Matthias Kaehlcke wrote:
> Some regulators have different settling times for voltage increases and
> decreases. To avoid a time penalty on the faster transition extend the
> settling time property to allow for different settings for upward and
> downward transitions.
>
> Signed-off-by: Matthias Kaehlcke <mka-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> Dependencies (from broonie/regulator topic/settle):
> - regulator: DT: Add settling time property for non-linear voltage change
> - regulator: Add settling time for non-linear voltage transition
>
> Sorry for not bringing this up during the review of the 'settling time'
> patch, I just came across it when looking to revive a similar change I
> sent out some time ago (https://patchwork.kernel.org/patch/9332051/).
>
> Documentation/devicetree/bindings/regulator/regulator.txt | 11 ++++++++---
> drivers/regulator/core.c | 8 ++++++--
> drivers/regulator/of_regulator.c | 9 +++++++--
> include/linux/regulator/machine.h | 9 ++++++---
> 4 files changed, 27 insertions(+), 10 deletions(-)
I think DT change and code change go in different patches.
> diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
> index d18edb075e1c..f21fead1c802 100644
> --- a/Documentation/devicetree/bindings/regulator/regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/regulator.txt
> @@ -21,9 +21,14 @@ Optional properties:
> design requires. This property describes the total system ramp time
> required due to the combination of internal ramping of the regulator itself,
> and board design issues such as trace capacitance and load on the supply.
> -- regulator-settling-time-us: Settling time, in microseconds, for voltage
> - change if regulator have the constant time for any level voltage change.
> - This is useful when regulator have exponential voltage change.
> +- regulator-settling-time-up-us: Settling time, in microseconds, for voltage
> + increase if the regulator needs a constant time to settle after voltage
> + increases of any level. This is useful for regulators with exponential
> + voltage changes.
> +- regulator-settling-time-down-us: Settling time, in microseconds, for voltage
> + decrease if the regulator needs a constant time to settle after voltage
> + decreases of any level. This is useful for regulators with exponential
> + voltage changes.
Can we have regulator-settling-time-us also so if it is there then
up/down same.
If up/down different then separate properties can be used.
Also in driver, if up/dn are not provided and only
regulator-settling-time-us is provided then up/dn can take value from
this property.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] clk: qcom: ipq8074: fix platform_no_drv_owner.cocci warnings
From: kbuild test robot @ 2017-04-29 5:50 UTC (permalink / raw)
Cc: kbuild-all-JC7UmRfGjtg, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, mturquette-rdvid1DuHRBWk0Htik3J/w,
sboyd-sgV2jX0FEOL9JmXXK+q4OQ,
linus.walleij-QSEj5FYQhm4dnm+yROfE0A,
andy.gross-QSEj5FYQhm4dnm+yROfE0A,
david.brown-QSEj5FYQhm4dnm+yROfE0A, catalin.marinas-5wv7dgnIgG8,
will.deacon-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-gpio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-soc-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
sricharan-sgV2jX0FEOL9JmXXK+q4OQ, absahu-sgV2jX0FEOL9JmXXK+q4OQ,
sjaganat-sgV2jX0FEOL9JmXXK+q4OQ, Varadarajan Narayanan
In-Reply-To: <1493373403-23462-3-git-send-email-varada-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
drivers/clk/qcom/gcc-ipq8074.c:1014:3-8: No need to set .owner here. The core will do it.
Remove .owner field if calls are used which set it automatically
Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci
CC: Abhishek Sahu <absahu-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Signed-off-by: Fengguang Wu <fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
gcc-ipq8074.c | 1 -
1 file changed, 1 deletion(-)
--- a/drivers/clk/qcom/gcc-ipq8074.c
+++ b/drivers/clk/qcom/gcc-ipq8074.c
@@ -1011,7 +1011,6 @@ static struct platform_driver gcc_ipq807
.probe = gcc_ipq8074_probe,
.driver = {
.name = "qcom,gcc-ipq8074",
- .owner = THIS_MODULE,
.of_match_table = gcc_ipq8074_match_table,
},
};
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/5] clk: qcom: ipq8074: Add Global Clock Controller support
From: kbuild test robot @ 2017-04-29 5:50 UTC (permalink / raw)
Cc: kbuild-all, robh+dt, mark.rutland, mturquette, sboyd,
linus.walleij, andy.gross, david.brown, catalin.marinas,
will.deacon, devicetree, linux-kernel, linux-clk, linux-gpio,
linux-arm-msm, linux-soc, linux-arm-kernel, sricharan, absahu,
sjaganat, Varadarajan Narayanan
In-Reply-To: <1493373403-23462-3-git-send-email-varada@codeaurora.org>
Hi Abhishek,
[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.11-rc8 next-20170428]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Varadarajan-Narayanan/Add-minimal-boot-support-for-IPQ8074/20170429-130315
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
coccinelle warnings: (new ones prefixed by >>)
>> drivers/clk/qcom/gcc-ipq8074.c:1014:3-8: No need to set .owner here. The core will do it.
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: arm-ccn: Add bindings info for CCN-502 compatible string
From: Scott Branden @ 2017-04-29 4:07 UTC (permalink / raw)
To: Mark Rutland, Velibor Markovski
Cc: Rob Herring, Pawel Moll, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
BCM Kernel Feedback, nd-5wv7dgnIgG8
In-Reply-To: <20170214174833.GK23718@leverpostej>
Hi Mark/Pawel,
I think this patch series has been missed.
On 17-02-14 09:48 AM, Mark Rutland wrote:
> On Fri, Feb 10, 2017 at 12:42:47PM -0800, Velibor Markovski wrote:
>> Add CCN-502 to the list of supported devices by ARM CCN PMU driver.
>>
>> Signed-off-by: Velibor Markovski <velibor.markovski-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>
> Acked-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Acked-by: Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>
> I assume Pawel will take this along with the driver patch.
Could you please pick up this patch series?
>
> Thanks,
> Mark.
>
>> ---
>> Documentation/devicetree/bindings/arm/ccn.txt | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/ccn.txt b/Documentation/devicetree/bindings/arm/ccn.txt
>> index b100d38..2980145 100644
>> --- a/Documentation/devicetree/bindings/arm/ccn.txt
>> +++ b/Documentation/devicetree/bindings/arm/ccn.txt
>> @@ -3,6 +3,7 @@
>> Required properties:
>>
>> - compatible: (standard compatible string) should be one of:
>> + "arm,ccn-502"
>> "arm,ccn-504"
>> "arm,ccn-508"
>>
>> --
>> 1.9.1
>>
Thanks,
Scott
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] regulator: Allow for asymmetric settling times
From: Matthias Kaehlcke @ 2017-04-29 0:06 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, Laxman Dewangan, Rob Herring,
Mark Rutland
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Douglas Anderson, Brian Norris,
Matthias Kaehlcke
Some regulators have different settling times for voltage increases and
decreases. To avoid a time penalty on the faster transition extend the
settling time property to allow for different settings for upward and
downward transitions.
Signed-off-by: Matthias Kaehlcke <mka-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
Dependencies (from broonie/regulator topic/settle):
- regulator: DT: Add settling time property for non-linear voltage change
- regulator: Add settling time for non-linear voltage transition
Sorry for not bringing this up during the review of the 'settling time'
patch, I just came across it when looking to revive a similar change I
sent out some time ago (https://patchwork.kernel.org/patch/9332051/).
Documentation/devicetree/bindings/regulator/regulator.txt | 11 ++++++++---
drivers/regulator/core.c | 8 ++++++--
drivers/regulator/of_regulator.c | 9 +++++++--
include/linux/regulator/machine.h | 9 ++++++---
4 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
index d18edb075e1c..f21fead1c802 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -21,9 +21,14 @@ Optional properties:
design requires. This property describes the total system ramp time
required due to the combination of internal ramping of the regulator itself,
and board design issues such as trace capacitance and load on the supply.
-- regulator-settling-time-us: Settling time, in microseconds, for voltage
- change if regulator have the constant time for any level voltage change.
- This is useful when regulator have exponential voltage change.
+- regulator-settling-time-up-us: Settling time, in microseconds, for voltage
+ increase if the regulator needs a constant time to settle after voltage
+ increases of any level. This is useful for regulators with exponential
+ voltage changes.
+- regulator-settling-time-down-us: Settling time, in microseconds, for voltage
+ decrease if the regulator needs a constant time to settle after voltage
+ decreases of any level. This is useful for regulators with exponential
+ voltage changes.
- regulator-soft-start: Enable soft start so that voltage ramps slowly
- regulator-state-mem sub-root node for Suspend-to-RAM mode
: suspend to memory, the device goes to sleep, but all data stored in memory,
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 811096b23143..4df86c0da123 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2773,8 +2773,12 @@ static int _regulator_set_voltage_time(struct regulator_dev *rdev,
ramp_delay = rdev->constraints->ramp_delay;
else if (rdev->desc->ramp_delay)
ramp_delay = rdev->desc->ramp_delay;
- else if (rdev->constraints->settling_time)
- return rdev->constraints->settling_time;
+ else if (rdev->constraints->settling_time_up &&
+ (new_uV > old_uV))
+ return rdev->constraints->settling_time_up;
+ else if (rdev->constraints->settling_time_down &&
+ (new_uV < old_uV))
+ return rdev->constraints->settling_time_down;
if (ramp_delay == 0) {
rdev_dbg(rdev, "ramp_delay not set\n");
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 09d677d5d3f0..4d36c0e4c9b4 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -86,9 +86,14 @@ static void of_get_regulation_constraints(struct device_node *np,
constraints->ramp_disable = true;
}
- ret = of_property_read_u32(np, "regulator-settling-time-us", &pval);
+ ret = of_property_read_u32(np, "regulator-settling-time-up-us", &pval);
if (!ret)
- constraints->settling_time = pval;
+ constraints->settling_time_up = pval;
+
+ ret = of_property_read_u32(np, "regulator-settling-time-down-us",
+ &pval);
+ if (!ret)
+ constraints->settling_time_down = pval;
ret = of_property_read_u32(np, "regulator-enable-ramp-delay", &pval);
if (!ret)
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index 117699d1f7df..a92829e86b5d 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -108,8 +108,10 @@ struct regulator_state {
* @initial_state: Suspend state to set by default.
* @initial_mode: Mode to set at startup.
* @ramp_delay: Time to settle down after voltage change (unit: uV/us)
- * @settling_time: Time to settle down after voltage change when voltage
- * change is non-linear (unit: microseconds).
+ * @settling_time_up: Time to settle down after voltage increase when voltage
+ * change is non-linear (unit: microseconds).
+ * @settling_time_down : Time to settle down after voltage decrease when
+ * voltage change is non-linear (unit: microseconds).
* @active_discharge: Enable/disable active discharge. The enum
* regulator_active_discharge values are used for
* initialisation.
@@ -151,7 +153,8 @@ struct regulation_constraints {
unsigned int initial_mode;
unsigned int ramp_delay;
- unsigned int settling_time;
+ unsigned int settling_time_up;
+ unsigned int settling_time_down;
unsigned int enable_time;
unsigned int active_discharge;
--
2.13.0.rc0.306.g87b477812d-goog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH 1/2] wcn36xx: Pass used skb to ieee80211_tx_status()
From: Bjorn Andersson @ 2017-04-28 23:42 UTC (permalink / raw)
To: Johannes Berg
Cc: Eugene Krasnikov, Kalle Valo, Andy Gross, David Brown,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-soc-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
wcn36xx-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Nicolas Dechesne
In-Reply-To: <1493281332.2529.1.camel-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
On Thu 27 Apr 01:22 PDT 2017, Johannes Berg wrote:
>
> > @@ -371,7 +371,7 @@ static void reap_tx_dxes(struct wcn36xx *wcn,
> > struct wcn36xx_dxe_ch *ch)
> > info = IEEE80211_SKB_CB(ctl->skb);
> > if (!(info->flags &
> > IEEE80211_TX_CTL_REQ_TX_STATUS)) {
> > /* Keep frame until TX status comes
> > */
> > - ieee80211_free_txskb(wcn->hw, ctl-
> > >skb);
> > + ieee80211_tx_status(wcn->hw, ctl-
> > >skb);
> >
>
> I don't think this is a good idea.
Thanks for letting me know :)
> This code intentionally checked if TX status was requested, and if not
> then it doesn't go to the effort of building it.
>
What I'm finding puzzling is the fact that the only caller of
ieee80211_led_tx() is ieee80211_tx_status() and it seems like drivers,
such as ath10k, call this for each packet handled - but I'm likely
missing something.
> As it is with your patch, it'll go and report the TX status without any
> TX status information - which is handled in wcn36xx_dxe_tx_ack_ind()
> for those frames needing it.
>
Right, it doesn't sound desired. However, during normal operation I'm
not seeing IEEE80211_TX_CTL_REQ_TX_STATUS being set and as such
ieee80211_led_tx() is never called.
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v4 3/3] drm/vc4: Add specific compatible strings for Cygnus.
From: Eric Anholt @ 2017-04-28 22:42 UTC (permalink / raw)
To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Rob Herring,
Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Eric Anholt
In-Reply-To: <20170428224223.21904-1-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
Cygnus has V3D 2.6 instead of 2.1, and doesn't use the VC4 display
modules. The V3D can be uniquely identified by the IDENT[01]
registers, and there's nothing to key off of for the display change
other than the lack of DT nodes for the display components, but it's
convention to have new compatible strings anyway.
Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt | 4 ++--
drivers/gpu/drm/vc4/vc4_drv.c | 1 +
drivers/gpu/drm/vc4/vc4_v3d.c | 1 +
3 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
index bc1756f4f791..284e2b14cfbe 100644
--- a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
+++ b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
@@ -5,7 +5,7 @@ with HDMI output and the HVS (Hardware Video Scaler) for compositing
display planes.
Required properties for VC4:
-- compatible: Should be "brcm,bcm2835-vc4"
+- compatible: Should be "brcm,bcm2835-vc4" or "brcm,cygnus-vc4"
Required properties for Pixel Valve:
- compatible: Should be one of "brcm,bcm2835-pixelvalve0",
@@ -54,7 +54,7 @@ Required properties for VEC:
See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
Required properties for V3D:
-- compatible: Should be "brcm,bcm2835-v3d"
+- compatible: Should be "brcm,bcm2835-v3d" or "brcm,cygnus-v3d"
- reg: Physical base address and length of the V3D's registers
- interrupts: The interrupt number
See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 92fb9a41fe7c..754ce76d4b98 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -335,6 +335,7 @@ static int vc4_platform_drm_remove(struct platform_device *pdev)
static const struct of_device_id vc4_of_match[] = {
{ .compatible = "brcm,bcm2835-vc4", },
+ { .compatible = "brcm,cygnus-vc4", },
{},
};
MODULE_DEVICE_TABLE(of, vc4_of_match);
diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index 7500820e5cd5..c53afec34586 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -450,6 +450,7 @@ static int vc4_v3d_dev_remove(struct platform_device *pdev)
static const struct of_device_id vc4_v3d_dt_match[] = {
{ .compatible = "brcm,bcm2835-v3d" },
+ { .compatible = "brcm,cygnus-v3d" },
{ .compatible = "brcm,vc4-v3d" },
{}
};
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v4 2/3] drm/vc4: Don't try to initialize FBDEV if we're only bound to V3D.
From: Eric Anholt @ 2017-04-28 22:42 UTC (permalink / raw)
To: dri-devel, Rob Herring, Mark Rutland, devicetree; +Cc: linux-kernel
In-Reply-To: <20170428224223.21904-1-eric@anholt.net>
The FBDEV initialization would throw an error in dmesg, when we just
want to silently not initialize fbdev on a V3D-only VC4 instance.
Signed-off-by: Eric Anholt <eric@anholt.net>
---
drivers/gpu/drm/vc4/vc4_kms.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index ad7925a9e0ea..237a504f11f0 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -230,10 +230,12 @@ int vc4_kms_load(struct drm_device *dev)
drm_mode_config_reset(dev);
- vc4->fbdev = drm_fbdev_cma_init(dev, 32,
- dev->mode_config.num_connector);
- if (IS_ERR(vc4->fbdev))
- vc4->fbdev = NULL;
+ if (dev->mode_config.num_connector) {
+ vc4->fbdev = drm_fbdev_cma_init(dev, 32,
+ dev->mode_config.num_connector);
+ if (IS_ERR(vc4->fbdev))
+ vc4->fbdev = NULL;
+ }
drm_kms_helper_poll_init(dev);
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v4 1/3] drm/vc4: Turn the V3D clock on at runtime.
From: Eric Anholt @ 2017-04-28 22:42 UTC (permalink / raw)
To: dri-devel, Rob Herring, Mark Rutland, devicetree; +Cc: linux-kernel
For the Raspberry Pi's bindings, the power domain also implicitly
turns on the clock and deasserts reset, but for the new Cygnus port we
start representing the clock in the devicetree.
v2: Document the clock-names property, check for -ENOENT for no clock
in DT.
v3: Drop NULL checks around clk calls which embed NULL checks.
v4: Drop clk-names (feedback by Rob Herring)
Signed-off-by: Eric Anholt <eric@anholt.net>
---
.../devicetree/bindings/display/brcm,bcm-vc4.txt | 3 +++
drivers/gpu/drm/vc4/vc4_drv.h | 1 +
drivers/gpu/drm/vc4/vc4_v3d.c | 31 +++++++++++++++++++++-
3 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
index ca02d3e4db91..bc1756f4f791 100644
--- a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
+++ b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
@@ -59,6 +59,9 @@ Required properties for V3D:
- interrupts: The interrupt number
See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
+Optional properties for V3D:
+- clocks: The clock the unit runs on
+
Required properties for DSI:
- compatible: Should be "brcm,bcm2835-dsi0" or "brcm,bcm2835-dsi1"
- reg: Physical base address and length of the DSI block's registers
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index b0967e2f7e88..92eb7d811bf2 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -200,6 +200,7 @@ struct vc4_v3d {
struct vc4_dev *vc4;
struct platform_device *pdev;
void __iomem *regs;
+ struct clk *clk;
};
struct vc4_hvs {
diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index a88078d7c9d1..7500820e5cd5 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -16,6 +16,7 @@
* this program. If not, see <http://www.gnu.org/licenses/>.
*/
+#include "linux/clk.h"
#include "linux/component.h"
#include "linux/pm_runtime.h"
#include "vc4_drv.h"
@@ -305,6 +306,8 @@ static int vc4_v3d_runtime_suspend(struct device *dev)
drm_gem_object_put_unlocked(&vc4->bin_bo->base.base);
vc4->bin_bo = NULL;
+ clk_disable_unprepare(v3d->clk);
+
return 0;
}
@@ -318,6 +321,10 @@ static int vc4_v3d_runtime_resume(struct device *dev)
if (ret)
return ret;
+ ret = clk_prepare_enable(v3d->clk);
+ if (ret != 0)
+ return ret;
+
vc4_v3d_init_hw(vc4->dev);
vc4_irq_postinstall(vc4->dev);
@@ -348,15 +355,37 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)
vc4->v3d = v3d;
v3d->vc4 = vc4;
+ v3d->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(v3d->clk)) {
+ int ret = PTR_ERR(v3d->clk);
+
+ if (ret == -ENOENT) {
+ /* bcm2835 didn't have a clock reference in the DT. */
+ ret = 0;
+ v3d->clk = NULL;
+ } else {
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "Failed to get V3D clock: %d\n",
+ ret);
+ return ret;
+ }
+ }
+
if (V3D_READ(V3D_IDENT0) != V3D_EXPECTED_IDENT0) {
DRM_ERROR("V3D_IDENT0 read 0x%08x instead of 0x%08x\n",
V3D_READ(V3D_IDENT0), V3D_EXPECTED_IDENT0);
return -EINVAL;
}
+ ret = clk_prepare_enable(v3d->clk);
+ if (ret != 0)
+ return ret;
+
ret = vc4_allocate_bin_bo(drm);
- if (ret)
+ if (ret) {
+ clk_disable_unprepare(v3d->clk);
return ret;
+ }
/* Reset the binner overflow address/size at setup, to be sure
* we don't reuse an old one.
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox