* Re: [PATCH v2 1/4] iio: chemical: scd30: add core driver
From: Tomasz Duszynski @ 2020-06-01 12:10 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Tomasz Duszynski, linux-iio, linux-kernel, devicetree, robh+dt,
andy.shevchenko, pmeerw
In-Reply-To: <20200531192152.GC27246@arch>
On Sun, May 31, 2020 at 09:21:52PM +0200, Tomasz Duszynski wrote:
> On Sun, May 31, 2020 at 10:58:40AM +0100, Jonathan Cameron wrote:
> > On Sat, 30 May 2020 23:36:27 +0200
> > Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:
> >
> > > Add Sensirion SCD30 carbon dioxide core driver.
> > >
> > > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> >
> > Hi Tomasz
> >
> > A few things inline. Includes the alignment issue on
> > x86_32 that I fell into whilst trying to fix timestamp
> > alignment issues. Suggested resolution inline for that.
> >
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > > Documentation/ABI/testing/sysfs-bus-iio-scd30 | 20 +
> > > MAINTAINERS | 6 +
> > > drivers/iio/chemical/Kconfig | 11 +
> > > drivers/iio/chemical/Makefile | 1 +
> > > drivers/iio/chemical/scd30.h | 75 ++
> > > drivers/iio/chemical/scd30_core.c | 764 ++++++++++++++++++
> > > 6 files changed, 877 insertions(+)
> > > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > create mode 100644 drivers/iio/chemical/scd30.h
> > > create mode 100644 drivers/iio/chemical/scd30_core.c
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-scd30 b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > new file mode 100644
> > > index 000000000000..a05b1d28e94a
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > @@ -0,0 +1,20 @@
> > > +What: /sys/bus/iio/devices/iio:deviceX/calibration
> > > +Date: June 2020
> > > +KernelVersion: 5.8
> > > +Contact: linux-iio@vger.kernel.org
> > > +Description:
> > > + Contaminants build-up in the measurement chamber or optical
> > > + elements deterioration leads to sensor drift.
> > > +
> > > + One can compensate for sensor drift by using either automatic
> > > + self calibration (asc) or forced recalibration (frc). If used
> > > + at once one will overwrite the other.
> > > +
> > > + Writing 1 or 0 to this attribute will respectively activate or
> > > + deactivate asc.
> > > +
> > > + Picking value from the range [400 1 2000] and writing it to the
> > > + sensor will set frc.
> > Seems to me like this would be more intuitive as two separate parameters
> > perhaps:
> > calibration_auto_enable
> > calibration_forced_value
> > ?
> >
>
> Fine.
>
> > > +
> > > + Upon reading current asc status and frc value are returned
> > > + respectively.
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 60ed2963efaa..41a509cca6f1 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -15137,6 +15137,12 @@ S: Maintained
> > > F: drivers/misc/phantom.c
> > > F: include/uapi/linux/phantom.h
> > >
> > > +SENSIRION SCD30 CARBON DIOXIDE SENSOR DRIVER
> > > +M: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > > +S: Maintained
> > > +F: drivers/iio/chemical/scd30.h
> > > +F: drivers/iio/chemical/scd30_core.c
> > > +
> > > SENSIRION SPS30 AIR POLLUTION SENSOR DRIVER
> > > M: Tomasz Duszynski <tduszyns@gmail.com>
> > > S: Maintained
> > > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> > > index 7f21afd73b1c..99e852b67e55 100644
> > > --- a/drivers/iio/chemical/Kconfig
> > > +++ b/drivers/iio/chemical/Kconfig
> > > @@ -85,6 +85,17 @@ config PMS7003
> > > To compile this driver as a module, choose M here: the module will
> > > be called pms7003.
> > >
> > > +config SCD30_CORE
> > > + tristate "SCD30 carbon dioxide sensor driver"
> > > + select IIO_BUFFER
> > > + select IIO_TRIGGERED_BUFFER
> > > + help
> > > + Say Y here to build support for the Sensirion SCD30 sensor with carbon
> > > + dioxide, relative humidity and temperature sensing capabilities.
> > > +
> > > + To compile this driver as a module, choose M here: the module will
> > > + be called scd30_core.
> > > +
> > > config SENSIRION_SGP30
> > > tristate "Sensirion SGPxx gas sensors"
> > > depends on I2C
> > > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> > > index aba4167db745..c9804b041ecd 100644
> > > --- a/drivers/iio/chemical/Makefile
> > > +++ b/drivers/iio/chemical/Makefile
> > > @@ -12,6 +12,7 @@ obj-$(CONFIG_BME680_SPI) += bme680_spi.o
> > > obj-$(CONFIG_CCS811) += ccs811.o
> > > obj-$(CONFIG_IAQCORE) += ams-iaq-core.o
> > > obj-$(CONFIG_PMS7003) += pms7003.o
> > > +obj-$(CONFIG_SCD30_CORE) += scd30_core.o
> > > obj-$(CONFIG_SENSIRION_SGP30) += sgp30.o
> > > obj-$(CONFIG_SPS30) += sps30.o
> > > obj-$(CONFIG_VZ89X) += vz89x.o
> > > diff --git a/drivers/iio/chemical/scd30.h b/drivers/iio/chemical/scd30.h
> > > new file mode 100644
> > > index 000000000000..9b25f7423142
> > > --- /dev/null
> > > +++ b/drivers/iio/chemical/scd30.h
> > > @@ -0,0 +1,75 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef _SCD30_H
> > > +#define _SCD30_H
> > > +
> > > +#include <linux/completion.h>
> > > +#include <linux/device.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/pm.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/types.h>
> > > +
> > > +struct scd30_state;
> > > +
> > > +enum scd30_cmd {
> > > + /* start continuous measurement with pressure compensation */
> > > + CMD_START_MEAS,
> > > + /* stop continuous measurement */
> > > + CMD_STOP_MEAS,
> > > + /* set/get measurement interval */
> > > + CMD_MEAS_INTERVAL,
> > > + /* check whether new measurement is ready */
> > > + CMD_MEAS_READY,
> > > + /* get measurement */
> > > + CMD_READ_MEAS,
> > > + /* turn on/off automatic self calibration */
> > > + CMD_ASC,
> > > + /* set/get forced recalibration value */
> > > + CMD_FRC,
> > > + /* set/get temperature offset */
> > > + CMD_TEMP_OFFSET,
> > > + /* get firmware version */
> > > + CMD_FW_VERSION,
> > > + /* reset sensor */
> > > + CMD_RESET,
> > > + /*
> > > + * Command for altitude compensation was omitted intentionally because
> > > + * the same can be achieved by means of CMD_START_MEAS which takes
> > > + * pressure above the sea level as an argument.
> > > + */
> > > +};
> > > +
> > > +#define SCD30_MEAS_COUNT 3
> > > +
> > > +typedef int (*scd30_command_t)(struct scd30_state *state, enum scd30_cmd cmd,
> > > + u16 arg, void *response, int size);
> > > +
> > > +struct scd30_state {
> > > + /* serialize access to the device */
> > > + struct mutex lock;
> > > + struct device *dev;
> > > + struct regulator *vdd;
> > > + struct completion meas_ready;
> > > + void *priv;
> > > + int irq;
> > > + /*
> > > + * no way to retrieve current ambient pressure compensation value from
> > > + * the sensor so keep one around
> > > + */
> > > + u16 pressure_comp;
> > > + u16 meas_interval;
> > > + int meas[SCD30_MEAS_COUNT];
> > > +
> > > + scd30_command_t command;
> > > +};
> > > +
> > > +int scd30_suspend(struct device *dev);
> > > +int scd30_resume(struct device *dev);
> > > +
> > > +static __maybe_unused SIMPLE_DEV_PM_OPS(scd30_pm_ops, scd30_suspend,
> > > + scd30_resume);
> > > +
> > > +int scd30_probe(struct device *dev, int irq, const char *name, void *priv,
> > > + scd30_command_t command);
> > > +
> > > +#endif
> > > diff --git a/drivers/iio/chemical/scd30_core.c b/drivers/iio/chemical/scd30_core.c
> > > new file mode 100644
> > > index 000000000000..3b7d0a7ea7ae
> > > --- /dev/null
> > > +++ b/drivers/iio/chemical/scd30_core.c
> > > @@ -0,0 +1,764 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Sensirion SCD30 carbon dioxide sensor core driver
> > > + *
> > > + * Copyright (c) 2020 Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > > + */
> > > +#include <linux/bits.h>
> > > +#include <linux/completion.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/errno.h>
> > > +#include <linux/export.h>
> > > +#include <linux/iio/buffer.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>
> > > +#include <linux/iio/trigger.h>
> > > +#include <linux/iio/trigger_consumer.h>
> > > +#include <linux/iio/triggered_buffer.h>
> > > +#include <linux/iio/types.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/irqreturn.h>
> > > +#include <linux/jiffies.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/string.h>
> > > +#include <linux/sysfs.h>
> > > +#include <linux/types.h>
> > > +#include <asm/byteorder.h>
> > > +
> > > +#include "scd30.h"
> > > +
> > > +#define SCD30_PRESSURE_COMP_MIN_MBAR 700
> > > +#define SCD30_PRESSURE_COMP_MAX_MBAR 1400
> > > +#define SCD30_PRESSURE_COMP_DEFAULT 1013
> > > +#define SCD30_MEAS_INTERVAL_MIN_S 2
> > > +#define SCD30_MEAS_INTERVAL_MAX_S 1800
> > > +#define SCD30_MEAS_INTERVAL_DEFAULT SCD30_MEAS_INTERVAL_MIN_S
> > > +#define SCD30_FRC_MIN_PPM 400
> > > +#define SCD30_FRC_MAX_PPM 2000
> > > +#define SCD30_TEMP_OFFSET_MAX 655360
> > > +#define SCD30_EXTRA_TIMEOUT_PER_S 250
> > > +
> > > +enum {
> > > + SCD30_CONC,
> > > + SCD30_TEMP,
> > > + SCD30_HR,
> > > +};
> > > +
> > > +static int scd30_command_write(struct scd30_state *state, enum scd30_cmd cmd,
> > > + u16 arg)
> > > +{
> > > + return state->command(state, cmd, arg, NULL, 0);
> > > +}
> > > +
> > > +static int scd30_command_read(struct scd30_state *state, enum scd30_cmd cmd,
> > > + u16 *val)
> > > +{
> > > + int ret;
> > > +
> > > + ret = state->command(state, cmd, 0, val, sizeof(*val));
> > > + *val = be16_to_cpup((__be16 *)val);
> >
> > Please use a local variable for the __be16 as it makes thing more readable
> > and easier to check for endian problems.
> >
>
> Okay.
>
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int scd30_reset(struct scd30_state *state)
> > > +{
> > > + int ret;
> > > + u16 val;
> > > +
> > > + ret = scd30_command_write(state, CMD_RESET, 0);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /* sensor boots up within 2 secs */
> > > + msleep(2000);
> > > + /*
> > > + * Power-on-reset causes sensor to produce some glitch on i2c bus and
> > > + * some controllers end up in error state. Try to recover by placing
> > > + * any data on the bus.
> > > + */
> > > + scd30_command_read(state, CMD_MEAS_READY, &val);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/* simplified float to fixed point conversion with a scaling factor of 0.01 */
> > > +static int scd30_float_to_fp(int float32)
> > > +{
> > > + int fraction, shift,
> > > + mantissa = float32 & GENMASK(22, 0),
> > > + sign = float32 & BIT(31) ? -1 : 1,
> > > + exp = (float32 & ~BIT(31)) >> 23;
> > > +
> > > + /* special case 0 */
> > > + if (!exp && !mantissa)
> > > + return 0;
> > > +
> > > + exp -= 127;
> > > + if (exp < 0) {
> > > + exp = -exp;
> > > + /* return values ranging from 1 to 99 */
> > > + return sign * ((((BIT(23) + mantissa) * 100) >> 23) >> exp);
> > > + }
> > > +
> > > + /* return values starting at 100 */
> > > + shift = 23 - exp;
> > > + float32 = BIT(exp) + (mantissa >> shift);
> > > + fraction = mantissa & GENMASK(shift - 1, 0);
> > > +
> > > + return sign * (float32 * 100 + ((fraction * 100) >> shift));
> > > +}
> > > +
> > > +static int scd30_read_meas(struct scd30_state *state)
> > > +{
> > > + int i, ret;
> > > +
> > > + ret = state->command(state, CMD_READ_MEAS, 0, state->meas,
> > > + sizeof(state->meas));
> > > + if (ret)
> > > + return ret;
> > > +
> > > + be32_to_cpu_array(state->meas, state->meas, ARRAY_SIZE(state->meas));
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(state->meas); i++)
> > > + state->meas[i] = scd30_float_to_fp(state->meas[i]);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int scd30_wait_meas_irq(struct scd30_state *state)
> > > +{
> > > + int ret, timeout;
> > > +
> > > + timeout = state->meas_interval * (1000 + SCD30_EXTRA_TIMEOUT_PER_S);
> > > + timeout = msecs_to_jiffies(timeout);
> > > + reinit_completion(&state->meas_ready);
> > > + enable_irq(state->irq);
> > > + ret = wait_for_completion_interruptible_timeout(&state->meas_ready,
> > > + timeout);
> > > + if (ret > 0)
> > > + ret = 0;
> > > + else if (!ret)
> > > + ret = -ETIMEDOUT;
> > > +
> > > + disable_irq(state->irq);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int scd30_wait_meas_poll(struct scd30_state *state)
> > > +{
> > > + int timeout = state->meas_interval * SCD30_EXTRA_TIMEOUT_PER_S;
> > > + int tries = 5;
> > > +
> > > + do {
> > > + int ret;
> > > + u16 val;
> > > +
> > > + ret = scd30_command_read(state, CMD_MEAS_READY, &val);
> > > + if (ret)
> > > + return -EIO;
> > > +
> > > + /* new measurement available */
> > > + if (val)
> > > + break;
> > > +
> > > + msleep_interruptible(timeout);
> > > + } while (--tries);
> > > +
> > > + return tries ? 0 : -ETIMEDOUT;
> > > +}
> > > +
> > > +static int scd30_read_poll(struct scd30_state *state)
> > > +{
> > > + int ret;
> > > +
> > > + ret = scd30_wait_meas_poll(state);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return scd30_read_meas(state);
> > > +}
> > > +
> > > +static int scd30_read(struct scd30_state *state)
> > > +{
> > > + if (state->irq > 0)
> > > + return scd30_wait_meas_irq(state);
> > > +
> > > + return scd30_read_poll(state);
> > > +}
> > > +
> > > +static int scd30_read_raw(struct iio_dev *indio_dev,
> > > + struct iio_chan_spec const *chan,
> > > + int *val, int *val2, long mask)
> > > +{
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + int ret, meas[SCD30_MEAS_COUNT];
> > > +
> > > + switch (mask) {
> > > + case IIO_CHAN_INFO_RAW:
> > > + case IIO_CHAN_INFO_PROCESSED:
> > > + ret = iio_device_claim_direct_mode(indio_dev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + mutex_lock(&state->lock);
> > > + ret = scd30_read(state);
> > > + memcpy(meas, state->meas, SCD30_MEAS_COUNT * sizeof(*meas));
> >
> > The local copy seems a bit excessive. This isn't likely to be a particularly
> > fast path so perhaps skip the copy but hold the locks until we are
> > done with the buffer?
> >
>
> Okay.
>
> > > + mutex_unlock(&state->lock);
> > > + iio_device_release_direct_mode(indio_dev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + switch (chan->type) {
> > > + case IIO_CONCENTRATION:
> > > + *val = meas[chan->address] / 1000000;
> > > + *val2 = meas[chan->address] % 1000000;
> > > +
> > > + return IIO_VAL_INT_PLUS_MICRO;
> > > + case IIO_TEMP:
> > > + case IIO_HUMIDITYRELATIVE:
> > > + *val = meas[chan->address] * 10;
> > > +
> > > + return IIO_VAL_INT;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > + case IIO_CHAN_INFO_SCALE:
> > > + switch (chan->type) {
> > > + case IIO_CONCENTRATION:
> > > + *val = 0;
> > > + *val2 = 1;
> > > +
> > > + return IIO_VAL_INT_PLUS_MICRO;
> > > + case IIO_TEMP:
> > > + case IIO_HUMIDITYRELATIVE:
> > > + *val = 10;
> > > +
> > > + return IIO_VAL_INT;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > + case IIO_CHAN_INFO_SAMP_FREQ:
> > > + *val = 0;
> > > + *val2 = 0;
> > > +
> > > + mutex_lock(&state->lock);
> > > + ret = scd30_command_read(state, CMD_MEAS_INTERVAL, (u16 *)val2);
> >
> > See below. I'll assume you'll fix all of these.
> >
> > > + mutex_unlock(&state->lock);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + *val2 = 1000000000 / *val2;
> > > +
> > > + return IIO_VAL_INT_PLUS_NANO;
> > > + case IIO_CHAN_INFO_CALIBSCALE:
> > > + mutex_lock(&state->lock);
> > > + *val = state->pressure_comp / 10;
> > > + *val2 = (state->pressure_comp % 10) * 100000;
> > > + mutex_unlock(&state->lock);
> > > +
> > > + return IIO_VAL_INT_PLUS_MICRO;
> > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > + *val = 0;
> > > + mutex_lock(&state->lock);
> > > + ret = scd30_command_read(state, CMD_TEMP_OFFSET, (u16 *)val);
> >
> > Reading a u16 directly into a int is not a good idea. What you get will
> > depend on the endianness of the machine.
> >
>
> Right, that would obviously break on BE. Sometimes trying to keep number
> of local variables low simply stops paying off :).
>
> > Use an intermediate variable of the right size.
> >
> > > + mutex_unlock(&state->lock);
> > > +
> > > + return IIO_VAL_INT;
> > > + }
> > > +
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static int scd30_write_raw(struct iio_dev *indio_dev,
> > > + struct iio_chan_spec const *chan,
> > > + int val, int val2, long mask)
> > > +{
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + int ret = -EINVAL;
> > > +
> > > + mutex_lock(&state->lock);
> > > + switch (mask) {
> > > + case IIO_CHAN_INFO_SAMP_FREQ:
> > > + if (val)
> > > + break;
> > > +
> > > + val = 1000000000 / val2;
> > > + if (val < SCD30_MEAS_INTERVAL_MIN_S ||
> > > + val > SCD30_MEAS_INTERVAL_MAX_S)
> > > + break;
> > > +
> > > + ret = scd30_command_write(state, CMD_MEAS_INTERVAL, val);
> > > + if (ret)
> > > + break;
> > > +
> > > + state->meas_interval = val;
> > > + break;
> > > + case IIO_CHAN_INFO_CALIBSCALE:
> > > + val = (val * 1000000 + val2) / 100000;
> > > + if (val < SCD30_PRESSURE_COMP_MIN_MBAR ||
> > > + val > SCD30_PRESSURE_COMP_MAX_MBAR)
> > > + break;
> > > +
> > > + ret = scd30_command_write(state, CMD_START_MEAS, val);
> > > + if (ret)
> > > + break;
> > > +
> > > + state->pressure_comp = val;
> > > + break;
> > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > + if (val < 0 || val > SCD30_TEMP_OFFSET_MAX)
> > > + break;
> > > + /*
> > > + * Manufacturer does not explicitly specify min/max sensible
> > > + * values hence check is omitted for simplicity.
> > > + */
> > > + ret = scd30_command_write(state, CMD_TEMP_OFFSET / 10, val);
> > > + }
> > > + mutex_unlock(&state->lock);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int scd30_write_raw_get_fmt(struct iio_dev *indio_dev,
> > > + struct iio_chan_spec const *chan, long mask)
> > > +{
> > > + switch (mask) {
> > > + case IIO_CHAN_INFO_RAW:
> > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > + return IIO_VAL_INT;
> > > + case IIO_CHAN_INFO_CALIBSCALE:
> > > + return IIO_VAL_INT_PLUS_MICRO;
> > > + case IIO_CHAN_INFO_SAMP_FREQ:
> > > + return IIO_VAL_INT_PLUS_NANO;
> > > + }
> > > +
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static const int scd30_pressure_calibscale_available[] = {
> > > + SCD30_PRESSURE_COMP_MIN_MBAR / 10, 0,
> > > + 0, 100000,
> > > + SCD30_PRESSURE_COMP_MAX_MBAR / 10, 0,
> > > +};
> > > +
> > > +static const int scd30_temp_calibbias_available[] = {
> > > + 0, 10, SCD30_TEMP_OFFSET_MAX,
> > > +};
> > > +
> > > +static int scd30_read_avail(struct iio_dev *indio_dev,
> > > + struct iio_chan_spec const *chan, const int **vals,
> > > + int *type, int *length, long mask)
> > > +{
> > > + switch (mask) {
> > > + case IIO_CHAN_INFO_CALIBSCALE:
> > > + *vals = scd30_pressure_calibscale_available;
> > > + *type = IIO_VAL_INT_PLUS_MICRO;
> > > +
> > > + return IIO_AVAIL_RANGE;
> > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > + *vals = scd30_temp_calibbias_available;
> > > + *type = IIO_VAL_INT;
> > > +
> > > + return IIO_AVAIL_RANGE;
> > > + }
> > > +
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static ssize_t sampling_frequency_available_show(struct device *dev,
> > > + struct device_attribute *attr,
> > > + char *buf)
> > > +{
> > > + int i = SCD30_MEAS_INTERVAL_MIN_S;
> > > + ssize_t len = 0;
> > > +
> > > + do {
> > > + len += scnprintf(buf + len, PAGE_SIZE - len, "0.%09u ",
> > > + 1000000000 / i);
> > > + /*
> > > + * Not all values fit PAGE_SIZE buffer hence print every 6th
> > > + * (each frequency differs by 6s in time domain from the
> > > + * adjecent). Unlisted but valid ones are still accepted.
> >
> > adjacent
> >
> > Hmm. Maybe we need to think about some description for inverse of linear
> > cases as they are likely to be fairly common.
> > This will work in meantime.
> >
> > > + */
> > > + i += 6;
> > > + } while (i <= SCD30_MEAS_INTERVAL_MAX_S);
> > > +
> > > + buf[len - 1] = '\n';
> > > +
> > > + return len;
> > > +}
> > > +
> > > +static ssize_t calibration_show(struct device *dev,
> > > + struct device_attribute *attr, char *buf)
> > > +{
> > > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + u16 asc, frc;
> > > + int ret;
> > > +
> > > + mutex_lock(&state->lock);
> > > + ret = scd30_command_read(state, CMD_ASC, &asc);
> > > + if (ret)
> > > + goto out;
> > > +
> > > + ret = scd30_command_read(state, CMD_FRC, &frc);
> > > +out:
> > > + mutex_unlock(&state->lock);
> > > +
> > > + return ret ?: sprintf(buf, "%d %d\n", asc, frc);
> > > +}
> > > +
> > > +static ssize_t calibration_store(struct device *dev,
> > > + struct device_attribute *attr, const char *buf,
> > > + size_t len)
> > > +{
> > > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + int ret;
> > > + u16 val;
> >
> > As commented above, this interface doesn't win on the
> > obvious front so needs a rethink!
> >
> > > +
> > > + ret = kstrtou16(buf, 0, &val);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + mutex_lock(&state->lock);
> > > + if (val == 0 || val == 1)
> > > + ret = scd30_command_write(state, CMD_ASC, val);
> > > + else if (val >= SCD30_FRC_MIN_PPM && val <= SCD30_FRC_MAX_PPM)
> > > + ret = scd30_command_write(state, CMD_FRC, val);
> > > + else
> > > + ret = -EINVAL;
> > > + mutex_unlock(&state->lock);
> > > +
> > > + return ret ?: len;
> > > +}
> > > +
> > > +static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
> > > +static IIO_DEVICE_ATTR_RW(calibration, 0);
> > > +
> > > +static struct attribute *scd30_attrs[] = {
> > > + &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> > > + &iio_dev_attr_calibration.dev_attr.attr,
> > > + NULL
> > > +};
> > > +
> > > +static const struct attribute_group scd30_attr_group = {
> > > + .attrs = scd30_attrs,
> > > +};
> > > +
> > > +static const struct iio_info scd30_info = {
> > > + .attrs = &scd30_attr_group,
> > > + .read_raw = scd30_read_raw,
> > > + .write_raw = scd30_write_raw,
> > > + .write_raw_get_fmt = scd30_write_raw_get_fmt,
> > > + .read_avail = scd30_read_avail,
> > > +};
> > > +
> > > +#define SCD30_CHAN_SCAN_TYPE(_sign, _realbits) .scan_type = { \
> > > + .sign = _sign, \
> > > + .realbits = _realbits, \
> > > + .storagebits = 32, \
> > > + .endianness = IIO_CPU, \
> > > +}
> > > +
> > > +static const struct iio_chan_spec scd30_channels[] = {
> > > + {
> > > + .type = IIO_PRESSURE,
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE),
> > > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_CALIBSCALE),
> > > + .scan_index = -1,
> > > + },
> > > + {
> > > + .type = IIO_CONCENTRATION,
> > > + .channel2 = IIO_MOD_CO2,
> > > + .address = SCD30_CONC,
> > > + .scan_index = SCD30_CONC,
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > > + BIT(IIO_CHAN_INFO_SCALE),
> > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > > + .modified = 1,
> > > +
> > > + SCD30_CHAN_SCAN_TYPE('u', 16),
> > > + },
> > > + {
> > > + .type = IIO_TEMP,
> > > + .address = SCD30_TEMP,
> > > + .scan_index = SCD30_TEMP,
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > > + BIT(IIO_CHAN_INFO_CALIBBIAS) |
> > > + BIT(IIO_CHAN_INFO_SCALE),
> >
> > Combination of processed and scale is unusual. Normally scale provides
> > a conversion factor or a _RAW reading.
>
> Right that's pointless. Scales were for raw measurements inside buffer.
> Somehow I failed to realize that only co2 concentration is raw.
>
One more thing occurred to me here. I just looked at CONCENTRATION_RAW
description and is states that this should return *percentage* reading.
Then after scaling what we should be left with?
Or perhaps scale should return just 1.0 for completeness if we want to
live with percentages.
Though in case where percentage reading is fractional then passing
through buffers will not work. Or am I missing something?
On the other hand if abi said nothing about percentages one would just
push whatever raw reading sensor outputs and provide scaling info to
userspace.
> >
> > I 'think' these units are otherwise fine (milli degrees centigrade)
> >
> >
> > > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_CALIBBIAS),
> > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > > +
> > > + SCD30_CHAN_SCAN_TYPE('s', 14),
> > > + },
> > > + {
> > > + .type = IIO_HUMIDITYRELATIVE,
> > > + .address = SCD30_HR,
> > > + .scan_index = SCD30_HR,
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > > + BIT(IIO_CHAN_INFO_SCALE),
> >
> > As above. Not normal to see scale and processed.
> >
> > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > > +
> > > + SCD30_CHAN_SCAN_TYPE('u', 14),
> > > + },
> > > + IIO_CHAN_SOFT_TIMESTAMP(3),
> > > +};
> > > +
> > > +int __maybe_unused scd30_suspend(struct device *dev)
> > > +{
> > > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + int ret;
> > > +
> > > + ret = scd30_command_write(state, CMD_STOP_MEAS, 0);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return regulator_disable(state->vdd);
> > > +}
> > > +EXPORT_SYMBOL(scd30_suspend);
> > > +
> > > +int __maybe_unused scd30_resume(struct device *dev)
> > > +{
> > > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + int ret;
> > > +
> > > + ret = regulator_enable(state->vdd);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return scd30_command_write(state, CMD_START_MEAS, state->pressure_comp);
> > > +}
> > > +EXPORT_SYMBOL(scd30_resume);
> > > +
> > > +static void scd30_stop_meas(void *data)
> > > +{
> > > + struct scd30_state *state = data;
> > > +
> > > + scd30_command_write(state, CMD_STOP_MEAS, 0);
> > > +}
> > > +
> > > +static void scd30_disable_regulator(void *data)
> > > +{
> > > + struct scd30_state *state = data;
> > > +
> > > + regulator_disable(state->vdd);
> > > +}
> > > +
> > > +static irqreturn_t scd30_irq_handler(int irq, void *priv)
> > > +{
> > > + struct iio_dev *indio_dev = priv;
> > > +
> > > + if (iio_buffer_enabled(indio_dev)) {
> >
> > There is a potential quirk here. It's possible that
> > this device is using a different trigger, but another device
> > is registered to use this one. If that happens this check
> > will be a bit counter intuitive.
> >
> > As such you might want to provide the validate callback so
> > that this device is the only device allowed to use it's
> > own trigger.
> >
>
> Right. This needs to be fixed.
>
> > > + iio_trigger_poll(indio_dev->trig);
> > > +
> > > + return IRQ_HANDLED;
> > > + }
> > > +
> > > + return IRQ_WAKE_THREAD;
> > > +}
> > > +
> > > +static irqreturn_t scd30_irq_thread_handler(int irq, void *priv)
> > > +{
> > > + struct iio_dev *indio_dev = priv;
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + int ret;
> > > +
> > > + ret = scd30_read_meas(state);
> > > + if (ret)
> > > + goto out;
> > > +
> > > + complete_all(&state->meas_ready);
> > > +out:
> > > + return IRQ_HANDLED;
> > > +}
> > > +
> > > +static irqreturn_t scd30_trigger_handler(int irq, void *p)
> > > +{
> > > + struct iio_poll_func *pf = p;
> > > + struct iio_dev *indio_dev = pf->indio_dev;
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + struct {
> > > + int data[SCD30_MEAS_COUNT];
> > > + u64 ts;
> >
> > Turns out I was wrong when suggesting this approach for drivers. On x86_32
> > this will result in there not being any padding between the
> > data and the timestamp (and in IIO rule of naturally aligned there
> > needs to be 4 bytes there). Result is that this structure is
> > too short. (thanks btw to Andy who pointed out this issue!)
> >
> > So, to force that my current preference is.
> >
> > struct {
> > int data[SCD30_MEAS_COUNT];
> > s64 ts __aligned(8);
> > } scan;
> >
>
> Ah, so x86_32 aligns s64 to 4 bytes.
>
> > However, given we do have a hole in the structure there is
> > a kernel data leak. So either you need to zero it here,
> > or move it into the iio_priv() structure. Doing that
> > will ensure it is zeroed at allocation.
> >
> > > + } scan;
> > > + int ret;
> > > +
> > > + mutex_lock(&state->lock);
> > > + if (!iio_trigger_using_own(indio_dev))
> > > + ret = scd30_read_poll(state);
> > > + else
> > > + ret = scd30_read_meas(state);
> > > + memcpy(scan.data, state->meas, sizeof(state->meas));
> > > + mutex_unlock(&state->lock);
> > > + if (ret)
> > > + goto out;
> > > +
> > > + iio_push_to_buffers_with_timestamp(indio_dev, &scan,
> > > + iio_get_time_ns(indio_dev));
> > > +out:
> > > + iio_trigger_notify_done(indio_dev->trig);
> > > + return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int scd30_set_trigger_state(struct iio_trigger *trig, bool state)
> > > +{
> > > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > > + struct scd30_state *st = iio_priv(indio_dev);
> > > +
> > > + if (state)
> > > + enable_irq(st->irq);
> > > + else
> > > + disable_irq(st->irq);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct iio_trigger_ops scd30_trigger_ops = {
> > > + .set_trigger_state = scd30_set_trigger_state,
> > > +};
> > > +
> > > +static int scd30_setup_trigger(struct iio_dev *indio_dev)
> > > +{
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + struct device *dev = indio_dev->dev.parent;
> > > + struct iio_trigger *trig;
> > > + int ret;
> > > +
> > > + trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
> > > + indio_dev->id);
> > > + if (!trig) {
> > > + dev_err(dev, "failed to allocate trigger\n");
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + trig->dev.parent = dev;
> > > + trig->ops = &scd30_trigger_ops;
> > > + iio_trigger_set_drvdata(trig, indio_dev);
> > > +
> > > + ret = devm_iio_trigger_register(dev, trig);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + indio_dev->trig = iio_trigger_get(trig);
> > > +
> > > + ret = devm_request_threaded_irq(dev, state->irq, scd30_irq_handler,
> > > + scd30_irq_thread_handler,
> > > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > > + indio_dev->name, indio_dev);
> > > + if (ret)
> > > + dev_err(dev, "failed to request irq\n");
> > > +
> > > + disable_irq(state->irq);
> >
> > Given there is a gap between the request above and this disable, this
> > disable needs a comment explaining why it is here.
> >
> > I'm assuming it's an optimization?
> >
>
> Interrupt is enabled just before taking measurement to grab the fresh
> data and disabled afterwards. Not disabling it here would produce fat warning
> about unbalanced irqs.
>
> And that is because sensor takes measurements continuously. On demand
> mode, even though possible, doesn't work reliably. Sensor (at least the one
> sitting on my desk) needs way too much time to wakeup and grab measurement which
> makes the whole point of adjustable sampling frequency pointless :).
>
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +int scd30_probe(struct device *dev, int irq, const char *name, void *priv,
> > > + scd30_command_t command)
> > > +{
> > > + static const unsigned long scd30_scan_masks[] = { 0x07, 0x00 };
> > > + struct scd30_state *state;
> > > + struct iio_dev *indio_dev;
> > > + int ret;
> > > + u16 val;
> > > +
> > > + indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
> > > + if (!indio_dev)
> > > + return -ENOMEM;
> > > +
> > > + state = iio_priv(indio_dev);
> > > + state->dev = dev;
> >
> > Doesn't seem to be used.
> >
> > > + state->priv = priv;
> >
> > What's this for? At least at first glance I can't find it being used
> > anywhere.
> >
> > > + state->irq = irq;
> > > + state->pressure_comp = SCD30_PRESSURE_COMP_DEFAULT;
> > > + state->meas_interval = SCD30_MEAS_INTERVAL_DEFAULT;
> > > + state->command = command;
> > > + mutex_init(&state->lock);
> > > + init_completion(&state->meas_ready);
> > > +
> > > + dev_set_drvdata(dev, indio_dev);
> > > +
> > > + indio_dev->dev.parent = dev;
> >
> > Side note that there is a series moving this into the core under revision at
> > the moment. Hopefully I'll remember to fix this up when applying your patch
> > if that one has gone in ahead of it.
> >
> > > + indio_dev->info = &scd30_info;
> > > + indio_dev->name = name;
> > > + indio_dev->channels = scd30_channels;
> > > + indio_dev->num_channels = ARRAY_SIZE(scd30_channels);
> > > + indio_dev->modes = INDIO_DIRECT_MODE;
> > > + indio_dev->available_scan_masks = scd30_scan_masks;
> > > +
> > > + state->vdd = devm_regulator_get(dev, "vdd");
> > > + if (IS_ERR(state->vdd)) {
> > > + if (PTR_ERR(state->vdd) == -EPROBE_DEFER)
> > > + return -EPROBE_DEFER;
> > > +
> > > + dev_err(dev, "failed to get regulator\n");
> > > + return PTR_ERR(state->vdd);
> > > + }
> > > +
> > > + ret = regulator_enable(state->vdd);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = devm_add_action_or_reset(dev, scd30_disable_regulator, state);
> > > + if (ret)
> > > + return ret;
> > > +
> >
> > A comment here on why it makes sense to register this here. What
> > started mesurement? It seems that happens well below here so
> > we should really call this after that start all.
> >
>
> Sensor after being powered up starts in mode it was left in.
> Chances are it was continuous mode and we want to shut it down.
>
> > > + ret = devm_add_action_or_reset(dev, scd30_stop_meas, state);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = scd30_reset(state);
> > > + if (ret) {
> > > + dev_err(dev, "failed to reset device: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + if (state->irq > 0) {
> > > + ret = scd30_setup_trigger(indio_dev);
> > > + if (ret) {
> > > + dev_err(dev, "failed to setup trigger: %d\n", ret);
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > > + scd30_trigger_handler, NULL);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = scd30_command_read(state, CMD_FW_VERSION, &val);
> > > + if (ret) {
> > > + dev_err(dev, "failed to read firmware version: %d\n", ret);
> > > + return ret;
> > > + }
> > > + dev_info(dev, "firmware version: %d.%d\n", val >> 8, (char)val);
> > > +
> > > + ret = scd30_command_write(state, CMD_MEAS_INTERVAL,
> > > + state->meas_interval);
> > > + if (ret) {
> > > + dev_err(dev, "failed to set measurement interval: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + ret = scd30_command_write(state, CMD_START_MEAS, state->pressure_comp);
> > > + if (ret) {
> > > + dev_err(dev, "failed to start measurement: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + return devm_iio_device_register(dev, indio_dev);
> > > +}
> > > +EXPORT_SYMBOL(scd30_probe);
> > > +
> > > +MODULE_AUTHOR("Tomasz Duszynski <tomasz.duszynski@octakon.com>");
> > > +MODULE_DESCRIPTION("Sensirion SCD30 carbon dioxide sensor core driver");
> > > +MODULE_LICENSE("GPL v2");
> > > --
> > > 2.26.2
> > >
> >
^ permalink raw reply
* Re: [PATCH v8 0/5] support reserving crashkernel above 4G on arm64 kdump
From: Prabhakar Kushwaha @ 2020-06-01 12:02 UTC (permalink / raw)
To: Chen Zhou
Cc: Thomas Gleixner, mingo, Catalin Marinas, Will Deacon, dyoung, bhe,
robh+dt, John Donnelly, arnd, devicetree, Linux Doc Mailing List,
kexec mailing list, Linux Kernel Mailing List, horms, guohanjun,
Prabhakar Kushwaha, linux-arm-kernel
In-Reply-To: <20200521093805.64398-1-chenzhou10@huawei.com>
Hi Chen,
On Thu, May 21, 2020 at 3:05 PM Chen Zhou <chenzhou10@huawei.com> wrote:
>
> This patch series enable reserving crashkernel above 4G in arm64.
>
> There are following issues in arm64 kdump:
> 1. We use crashkernel=X to reserve crashkernel below 4G, which will fail
> when there is no enough low memory.
> 2. Currently, crashkernel=Y@X can be used to reserve crashkernel above 4G,
> in this case, if swiotlb or DMA buffers are required, crash dump kernel
> will boot failure because there is no low memory available for allocation.
>
> To solve these issues, introduce crashkernel=X,low to reserve specified
> size low memory.
> Crashkernel=X tries to reserve memory for the crash dump kernel under
> 4G. If crashkernel=Y,low is specified simultaneously, reserve spcified
> size low memory for crash kdump kernel devices firstly and then reserve
> memory above 4G.
>
> When crashkernel is reserved above 4G in memory, that is, crashkernel=X,low
> is specified simultaneously, kernel should reserve specified size low memory
> for crash dump kernel devices. So there may be two crash kernel regions, one
> is below 4G, the other is above 4G.
> In order to distinct from the high region and make no effect to the use of
> kexec-tools, rename the low region as "Crash kernel (low)", and add DT property
> "linux,low-memory-range" to crash dump kernel's dtb to pass the low region.
>
> Besides, we need to modify kexec-tools:
> arm64: kdump: add another DT property to crash dump kernel's dtb(see [1])
>
> The previous changes and discussions can be retrieved from:
>
> Changes since [v7]
> - Move x86 CRASH_ALIGN to 2M
> Suggested by Dave and do some test, move x86 CRASH_ALIGN to 2M.
> - Update Documentation/devicetree/bindings/chosen.txt
> Add corresponding documentation to Documentation/devicetree/bindings/chosen.txt suggested by Arnd.
> - Add Tested-by from Jhon and pk
>
> Changes since [v6]
> - Fix build errors reported by kbuild test robot.
>
> Changes since [v5]
> - Move reserve_crashkernel_low() into kernel/crash_core.c.
> - Delete crashkernel=X,high.
> - Modify crashkernel=X,low.
> If crashkernel=X,low is specified simultaneously, reserve spcified size low
> memory for crash kdump kernel devices firstly and then reserve memory above 4G.
> In addition, rename crashk_low_res as "Crash kernel (low)" for arm64, and then
> pass to crash dump kernel by DT property "linux,low-memory-range".
> - Update Documentation/admin-guide/kdump/kdump.rst.
>
> Changes since [v4]
> - Reimplement memblock_cap_memory_ranges for multiple ranges by Mike.
>
> Changes since [v3]
> - Add memblock_cap_memory_ranges back for multiple ranges.
> - Fix some compiling warnings.
>
> Changes since [v2]
> - Split patch "arm64: kdump: support reserving crashkernel above 4G" as
> two. Put "move reserve_crashkernel_low() into kexec_core.c" in a separate
> patch.
>
> Changes since [v1]:
> - Move common reserve_crashkernel_low() code into kernel/kexec_core.c.
> - Remove memblock_cap_memory_ranges() i added in v1 and implement that
> in fdt_enforce_memory_region().
> There are at most two crash kernel regions, for two crash kernel regions
> case, we cap the memory range [min(regs[*].start), max(regs[*].end)]
> and then remove the memory range in the middle.
>
> [1]: http://lists.infradead.org/pipermail/kexec/2020-May/025128.html
> [v1]: https://lkml.org/lkml/2019/4/2/1174
> [v2]: https://lkml.org/lkml/2019/4/9/86
> [v3]: https://lkml.org/lkml/2019/4/9/306
> [v4]: https://lkml.org/lkml/2019/4/15/273
> [v5]: https://lkml.org/lkml/2019/5/6/1360
> [v6]: https://lkml.org/lkml/2019/8/30/142
> [v7]: https://lkml.org/lkml/2019/12/23/411
>
> Chen Zhou (5):
> x86: kdump: move reserve_crashkernel_low() into crash_core.c
> arm64: kdump: reserve crashkenel above 4G for crash dump kernel
> arm64: kdump: add memory for devices by DT property, low-memory-range
> kdump: update Documentation about crashkernel on arm64
> dt-bindings: chosen: Document linux,low-memory-range for arm64 kdump
>
We are getting "warn_alloc" [1] warning during boot of kdump kernel
with bootargs as [2] of primary kernel.
This error observed on ThunderX2 ARM64 platform.
It is observed with latest upstream tag (v5.7-rc3) with this patch set
and https://lists.infradead.org/pipermail/kexec/2020-May/025128.html
Also **without** this patch-set
"https://www.spinics.net/lists/arm-kernel/msg806882.html"
This issue comes whenever crashkernel memory is reserved after 0xc000_0000.
More details discussed earlier in
https://www.spinics.net/lists/arm-kernel/msg806882.html without any
solution
This patch-set is expected to solve similar kind of issue.
i.e. low memory is only targeted for DMA, swiotlb; So above mentioned
observation should be considered/fixed. .
--pk
[1]
[ 30.366695] DMI: Cavium Inc. Saber/Saber, BIOS
TX2-FW-Release-3.1-build_01-2803-g74253a541a mm/dd/yyyy
[ 30.367696] NET: Registered protocol family 16
[ 30.369973] swapper/0: page allocation failure: order:6,
mode:0x1(GFP_DMA), nodemask=(null),cpuset=/,mems_allowed=0
[ 30.369980] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.7.0-rc3+ #121
[ 30.369981] Hardware name: Cavium Inc. Saber/Saber, BIOS
TX2-FW-Release-3.1-build_01-2803-g74253a541a mm/dd/yyyy
[ 30.369984] Call trace:
[ 30.369989] dump_backtrace+0x0/0x1f8
[ 30.369991] show_stack+0x20/0x30
[ 30.369997] dump_stack+0xc0/0x10c
[ 30.370001] warn_alloc+0x10c/0x178
[ 30.370004] __alloc_pages_slowpath.constprop.111+0xb10/0xb50
[ 30.370006] __alloc_pages_nodemask+0x2b4/0x300
[ 30.370008] alloc_page_interleave+0x24/0x98
[ 30.370011] alloc_pages_current+0xe4/0x108
[ 30.370017] dma_atomic_pool_init+0x44/0x1a4
[ 30.370020] do_one_initcall+0x54/0x228
[ 30.370027] kernel_init_freeable+0x228/0x2cc
[ 30.370031] kernel_init+0x1c/0x110
[ 30.370034] ret_from_fork+0x10/0x18
[ 30.370036] Mem-Info:
[ 30.370064] active_anon:0 inactive_anon:0 isolated_anon:0
[ 30.370064] active_file:0 inactive_file:0 isolated_file:0
[ 30.370064] unevictable:0 dirty:0 writeback:0 unstable:0
[ 30.370064] slab_reclaimable:34 slab_unreclaimable:4438
[ 30.370064] mapped:0 shmem:0 pagetables:14 bounce:0
[ 30.370064] free:1537719 free_pcp:219 free_cma:0
[ 30.370070] Node 0 active_anon:0kB inactive_anon:0kB
active_file:0kB inactive_file:0kB unevictable:0kB isolated(anon):0kB
isolated(file):0kB mapped:0kB dirty:0kB writeback:0kB shmem:0kB
shmem_thp: 0kB shmem_pmdmapped: 0kB anon_thp: 0kB writeback_tmp:0kB
unstable:0kB all_unreclaimable? no
[ 30.370073] Node 1 active_anon:0kB inactive_anon:0kB
active_file:0kB inactive_file:0kB unevictable:0kB isolated(anon):0kB
isolated(file):0kB mapped:0kB dirty:0kB writeback:0kB shmem:0kB
shmem_thp: 0kB shmem_pmdmapped: 0kB anon_thp: 0kB writeback_tmp:0kB
unstable:0kB all_unreclaimable? no
[ 30.370079] Node 0 DMA free:0kB min:0kB low:0kB high:0kB
reserved_highatomic:0KB active_anon:0kB inactive_anon:0kB
active_file:0kB inactive_file:0kB unevictable:0kB writepending:0kB
present:128kB managed:0kB mlocked:0kB kernel_stack:0kB pagetables:0kB
bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB
[ 30.370084] lowmem_reserve[]: 0 250 6063 6063
[ 30.370090] Node 0 DMA32 free:256000kB min:408kB low:664kB
high:920kB reserved_highatomic:0KB active_anon:0kB inactive_anon:0kB
active_file:0kB inactive_file:0kB unevictable:0kB writepending:0kB
present:269700kB managed:256000kB mlocked:0kB kernel_stack:0kB
pagetables:0kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB
[ 30.370094] lowmem_reserve[]: 0 0 5813 5813
[ 30.370100] Node 0 Normal free:5894876kB min:9552kB low:15504kB
high:21456kB reserved_highatomic:0KB active_anon:0kB inactive_anon:0kB
active_file:0kB inactive_file:0kB unevictable:0kB writepending:0kB
present:8388608kB managed:5953112kB mlocked:0kB kernel_stack:21672kB
pagetables:56kB bounce:0kB free_pcp:876kB local_pcp:176kB free_cma:0kB
[ 30.370104] lowmem_reserve[]: 0 0 0 0
[ 30.370107] Node 0 DMA: 0*4kB 0*8kB 0*16kB 0*32kB 0*64kB 0*128kB
0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB = 0kB
[ 30.370113] Node 0 DMA32: 0*4kB 0*8kB 0*16kB 0*32kB 0*64kB 0*128kB
0*256kB 0*512kB 0*1024kB 1*2048kB (M) 62*4096kB (M) = 256000kB
[ 30.370119] Node 0 Normal: 2*4kB (M) 3*8kB (ME) 2*16kB (UE) 3*32kB
(UM) 1*64kB (U) 2*128kB (M) 2*256kB (ME) 3*512kB (ME) 3*1024kB (ME)
3*2048kB (UME) 1436*4096kB (M) = 5893600kB
[ 30.370129] Node 0 hugepages_total=0 hugepages_free=0
hugepages_surp=0 hugepages_size=1048576kB
[ 30.370130] 0 total pagecache pages
[ 30.370132] 0 pages in swap cache
[ 30.370134] Swap cache stats: add 0, delete 0, find 0/0
[ 30.370135] Free swap = 0kB
[ 30.370136] Total swap = 0kB
[ 30.370137] 2164609 pages RAM
[ 30.370139] 0 pages HighMem/MovableOnly
[ 30.370140] 612331 pages reserved
[ 30.370141] 0 pages hwpoisoned
[ 30.370143] DMA: failed to allocate 256 KiB pool for atomic
coherent allocation
[2]
root@localhost$ dmesg | grep crash
[ 0.000000] Reserving 250MB of low memory at 3724MB for crashkernel
(System low RAM: 2029MB)
[ 0.000000] crashkernel reserved: 0x0000000e00000000 -
0x0000001000000000 (8192 MB)
[ 0.000000] Kernel command line:
BOOT_IMAGE=(hd11,gpt2)/vmlinuz-5.7.0-rc3+
root=UUID=e5c34f86-6727-4668-81f9-f41433555df6 ro crashkernel=250M,low
crashkernel=8G nowatchdog console=ttyAMA0 default_hugepagesz=1G
hugepagesz=1G hugepages=2
[ 44.019393] crashkernel=8G
^ permalink raw reply
* Re: [PATCH 04/12] dt-bindings: irqchip: ti, sci-intr: Update bindings to drop the usage of gic as parent
From: Lokesh Vutla @ 2020-06-01 11:36 UTC (permalink / raw)
To: Marc Zyngier
Cc: Nishanth Menon, Rob Herring, Grygorii Strashko, Peter Ujfalusi,
Device Tree Mailing List, Sekhar Nori, Tero Kristo,
Santosh Shilimkar, Thomas Gleixner, Linux ARM Mailing List
In-Reply-To: <4ea8c6110a16900220a65f1d44145146@kernel.org>
Hi Marc,
On 29/05/20 3:48 pm, Marc Zyngier wrote:
> On 2020-05-29 11:14, Lokesh Vutla wrote:
>> Hi Rob,
>>
>> On 29/05/20 3:44 am, Rob Herring wrote:
>>> On Wed, May 20, 2020 at 06:14:46PM +0530, Lokesh Vutla wrote:
>>>> Drop the firmware related dt-bindings and use the hardware specified
>>>> interrupt numbers within Interrupt Router. This ensures interrupt router
>>>> DT node need not assume any interrupt parent type.
>>>
>>> I didn't like this binding to begin with, but now you're breaking
>>> compatibility.
>>
>> Yes, I do agree that this change is breaking backward compatibility. But IMHO,
>> this does cleanup of firmware specific properties from DT. Since this is not
>> deployed out yet in the wild market, I took the leverage of breaking backward
>> compatibility. Before accepting these changes from firmware team, I did
>> discuss[0] with Marc on this topic.
>
> And I assume that should anyone complain about the kernel being broken
> because they have an old firmware, you'll be OK with the patches being
> reverted, right?
I am assuming there is no one to complain as there is no product available yet
with upstream kernel. Internally everyone is aware of the changes. So, yes I
would agree with you to revert the changes if someone really needs it. :)
Thanks and regards,
Lokesh
^ permalink raw reply
* Re: [PATCH v2 1/4] iio: chemical: scd30: add core driver
From: Tomasz Duszynski @ 2020-06-01 11:30 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Tomasz Duszynski, Jonathan Cameron, linux-iio, linux-kernel,
devicetree, robh+dt, andy.shevchenko, pmeerw
In-Reply-To: <20200601113604.00002d70@Huawei.com>
On Mon, Jun 01, 2020 at 11:36:04AM +0100, Jonathan Cameron wrote:
> On Sun, 31 May 2020 21:21:52 +0200
> Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:
>
> > On Sun, May 31, 2020 at 10:58:40AM +0100, Jonathan Cameron wrote:
> > > On Sat, 30 May 2020 23:36:27 +0200
> > > Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:
> > >
> > > > Add Sensirion SCD30 carbon dioxide core driver.
> > > >
> > > > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > >
> > > Hi Tomasz
> > >
> > > A few things inline. Includes the alignment issue on
> > > x86_32 that I fell into whilst trying to fix timestamp
> > > alignment issues. Suggested resolution inline for that.
> > >
> > > Thanks,
> > >
> > > Jonathan
> > >
> > > > ---
> > > > Documentation/ABI/testing/sysfs-bus-iio-scd30 | 20 +
> > > > MAINTAINERS | 6 +
> > > > drivers/iio/chemical/Kconfig | 11 +
> > > > drivers/iio/chemical/Makefile | 1 +
> > > > drivers/iio/chemical/scd30.h | 75 ++
> > > > drivers/iio/chemical/scd30_core.c | 764 ++++++++++++++++++
> > > > 6 files changed, 877 insertions(+)
> > > > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > > create mode 100644 drivers/iio/chemical/scd30.h
> > > > create mode 100644 drivers/iio/chemical/scd30_core.c
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-scd30 b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > > new file mode 100644
> > > > index 000000000000..a05b1d28e94a
> > > > --- /dev/null
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > > @@ -0,0 +1,20 @@
> > > > +What: /sys/bus/iio/devices/iio:deviceX/calibration
> > > > +Date: June 2020
> > > > +KernelVersion: 5.8
> > > > +Contact: linux-iio@vger.kernel.org
> > > > +Description:
> > > > + Contaminants build-up in the measurement chamber or optical
> > > > + elements deterioration leads to sensor drift.
> > > > +
> > > > + One can compensate for sensor drift by using either automatic
> > > > + self calibration (asc) or forced recalibration (frc). If used
> > > > + at once one will overwrite the other.
> > > > +
> > > > + Writing 1 or 0 to this attribute will respectively activate or
> > > > + deactivate asc.
> > > > +
> > > > + Picking value from the range [400 1 2000] and writing it to the
> > > > + sensor will set frc.
> > > Seems to me like this would be more intuitive as two separate parameters
> > > perhaps:
> > > calibration_auto_enable
> > > calibration_forced_value
> > > ?
> > >
> >
> > Fine.
> >
> > > > +
> > > > + Upon reading current asc status and frc value are returned
> > > > + respectively.
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 60ed2963efaa..41a509cca6f1 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -15137,6 +15137,12 @@ S: Maintained
> > > > F: drivers/misc/phantom.c
> > > > F: include/uapi/linux/phantom.h
> > > >
> > > > +SENSIRION SCD30 CARBON DIOXIDE SENSOR DRIVER
> > > > +M: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > > > +S: Maintained
> > > > +F: drivers/iio/chemical/scd30.h
> > > > +F: drivers/iio/chemical/scd30_core.c
> > > > +
> > > > SENSIRION SPS30 AIR POLLUTION SENSOR DRIVER
> > > > M: Tomasz Duszynski <tduszyns@gmail.com>
> > > > S: Maintained
> > > > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> > > > index 7f21afd73b1c..99e852b67e55 100644
> > > > --- a/drivers/iio/chemical/Kconfig
> > > > +++ b/drivers/iio/chemical/Kconfig
> > > > @@ -85,6 +85,17 @@ config PMS7003
> > > > To compile this driver as a module, choose M here: the module will
> > > > be called pms7003.
> > > >
> > > > +config SCD30_CORE
> > > > + tristate "SCD30 carbon dioxide sensor driver"
> > > > + select IIO_BUFFER
> > > > + select IIO_TRIGGERED_BUFFER
> > > > + help
> > > > + Say Y here to build support for the Sensirion SCD30 sensor with carbon
> > > > + dioxide, relative humidity and temperature sensing capabilities.
> > > > +
> > > > + To compile this driver as a module, choose M here: the module will
> > > > + be called scd30_core.
> > > > +
> > > > config SENSIRION_SGP30
> > > > tristate "Sensirion SGPxx gas sensors"
> > > > depends on I2C
> > > > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> > > > index aba4167db745..c9804b041ecd 100644
> > > > --- a/drivers/iio/chemical/Makefile
> > > > +++ b/drivers/iio/chemical/Makefile
> > > > @@ -12,6 +12,7 @@ obj-$(CONFIG_BME680_SPI) += bme680_spi.o
> > > > obj-$(CONFIG_CCS811) += ccs811.o
> > > > obj-$(CONFIG_IAQCORE) += ams-iaq-core.o
> > > > obj-$(CONFIG_PMS7003) += pms7003.o
> > > > +obj-$(CONFIG_SCD30_CORE) += scd30_core.o
> > > > obj-$(CONFIG_SENSIRION_SGP30) += sgp30.o
> > > > obj-$(CONFIG_SPS30) += sps30.o
> > > > obj-$(CONFIG_VZ89X) += vz89x.o
> > > > diff --git a/drivers/iio/chemical/scd30.h b/drivers/iio/chemical/scd30.h
> > > > new file mode 100644
> > > > index 000000000000..9b25f7423142
> > > > --- /dev/null
> > > > +++ b/drivers/iio/chemical/scd30.h
> > > > @@ -0,0 +1,75 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +#ifndef _SCD30_H
> > > > +#define _SCD30_H
> > > > +
> > > > +#include <linux/completion.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/mutex.h>
> > > > +#include <linux/pm.h>
> > > > +#include <linux/regulator/consumer.h>
> > > > +#include <linux/types.h>
> > > > +
> > > > +struct scd30_state;
> > > > +
> > > > +enum scd30_cmd {
> > > > + /* start continuous measurement with pressure compensation */
> > > > + CMD_START_MEAS,
> > > > + /* stop continuous measurement */
> > > > + CMD_STOP_MEAS,
> > > > + /* set/get measurement interval */
> > > > + CMD_MEAS_INTERVAL,
> > > > + /* check whether new measurement is ready */
> > > > + CMD_MEAS_READY,
> > > > + /* get measurement */
> > > > + CMD_READ_MEAS,
> > > > + /* turn on/off automatic self calibration */
> > > > + CMD_ASC,
> > > > + /* set/get forced recalibration value */
> > > > + CMD_FRC,
> > > > + /* set/get temperature offset */
> > > > + CMD_TEMP_OFFSET,
> > > > + /* get firmware version */
> > > > + CMD_FW_VERSION,
> > > > + /* reset sensor */
> > > > + CMD_RESET,
> > > > + /*
> > > > + * Command for altitude compensation was omitted intentionally because
> > > > + * the same can be achieved by means of CMD_START_MEAS which takes
> > > > + * pressure above the sea level as an argument.
> > > > + */
> > > > +};
> > > > +
> > > > +#define SCD30_MEAS_COUNT 3
> > > > +
> > > > +typedef int (*scd30_command_t)(struct scd30_state *state, enum scd30_cmd cmd,
> > > > + u16 arg, void *response, int size);
> > > > +
> > > > +struct scd30_state {
> > > > + /* serialize access to the device */
> > > > + struct mutex lock;
> > > > + struct device *dev;
> > > > + struct regulator *vdd;
> > > > + struct completion meas_ready;
> > > > + void *priv;
> > > > + int irq;
> > > > + /*
> > > > + * no way to retrieve current ambient pressure compensation value from
> > > > + * the sensor so keep one around
> > > > + */
> > > > + u16 pressure_comp;
> > > > + u16 meas_interval;
> > > > + int meas[SCD30_MEAS_COUNT];
> > > > +
> > > > + scd30_command_t command;
> > > > +};
> > > > +
> > > > +int scd30_suspend(struct device *dev);
> > > > +int scd30_resume(struct device *dev);
> > > > +
> > > > +static __maybe_unused SIMPLE_DEV_PM_OPS(scd30_pm_ops, scd30_suspend,
> > > > + scd30_resume);
> > > > +
> > > > +int scd30_probe(struct device *dev, int irq, const char *name, void *priv,
> > > > + scd30_command_t command);
> > > > +
> > > > +#endif
> > > > diff --git a/drivers/iio/chemical/scd30_core.c b/drivers/iio/chemical/scd30_core.c
> > > > new file mode 100644
> > > > index 000000000000..3b7d0a7ea7ae
> > > > --- /dev/null
> > > > +++ b/drivers/iio/chemical/scd30_core.c
> > > > @@ -0,0 +1,764 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Sensirion SCD30 carbon dioxide sensor core driver
> > > > + *
> > > > + * Copyright (c) 2020 Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > > > + */
> > > > +#include <linux/bits.h>
> > > > +#include <linux/completion.h>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/errno.h>
> > > > +#include <linux/export.h>
> > > > +#include <linux/iio/buffer.h>
> > > > +#include <linux/iio/iio.h>
> > > > +#include <linux/iio/sysfs.h>
> > > > +#include <linux/iio/trigger.h>
> > > > +#include <linux/iio/trigger_consumer.h>
> > > > +#include <linux/iio/triggered_buffer.h>
> > > > +#include <linux/iio/types.h>
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/irqreturn.h>
> > > > +#include <linux/jiffies.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/mutex.h>
> > > > +#include <linux/regulator/consumer.h>
> > > > +#include <linux/string.h>
> > > > +#include <linux/sysfs.h>
> > > > +#include <linux/types.h>
> > > > +#include <asm/byteorder.h>
> > > > +
> > > > +#include "scd30.h"
> > > > +
> > > > +#define SCD30_PRESSURE_COMP_MIN_MBAR 700
> > > > +#define SCD30_PRESSURE_COMP_MAX_MBAR 1400
> > > > +#define SCD30_PRESSURE_COMP_DEFAULT 1013
> > > > +#define SCD30_MEAS_INTERVAL_MIN_S 2
> > > > +#define SCD30_MEAS_INTERVAL_MAX_S 1800
> > > > +#define SCD30_MEAS_INTERVAL_DEFAULT SCD30_MEAS_INTERVAL_MIN_S
> > > > +#define SCD30_FRC_MIN_PPM 400
> > > > +#define SCD30_FRC_MAX_PPM 2000
> > > > +#define SCD30_TEMP_OFFSET_MAX 655360
> > > > +#define SCD30_EXTRA_TIMEOUT_PER_S 250
> > > > +
> > > > +enum {
> > > > + SCD30_CONC,
> > > > + SCD30_TEMP,
> > > > + SCD30_HR,
> > > > +};
> > > > +
> > > > +static int scd30_command_write(struct scd30_state *state, enum scd30_cmd cmd,
> > > > + u16 arg)
> > > > +{
> > > > + return state->command(state, cmd, arg, NULL, 0);
> > > > +}
> > > > +
> > > > +static int scd30_command_read(struct scd30_state *state, enum scd30_cmd cmd,
> > > > + u16 *val)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + ret = state->command(state, cmd, 0, val, sizeof(*val));
> > > > + *val = be16_to_cpup((__be16 *)val);
> > >
> > > Please use a local variable for the __be16 as it makes thing more readable
> > > and easier to check for endian problems.
> > >
> >
> > Okay.
> >
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int scd30_reset(struct scd30_state *state)
> > > > +{
> > > > + int ret;
> > > > + u16 val;
> > > > +
> > > > + ret = scd30_command_write(state, CMD_RESET, 0);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + /* sensor boots up within 2 secs */
> > > > + msleep(2000);
> > > > + /*
> > > > + * Power-on-reset causes sensor to produce some glitch on i2c bus and
> > > > + * some controllers end up in error state. Try to recover by placing
> > > > + * any data on the bus.
> > > > + */
> > > > + scd30_command_read(state, CMD_MEAS_READY, &val);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +/* simplified float to fixed point conversion with a scaling factor of 0.01 */
> > > > +static int scd30_float_to_fp(int float32)
> > > > +{
> > > > + int fraction, shift,
> > > > + mantissa = float32 & GENMASK(22, 0),
> > > > + sign = float32 & BIT(31) ? -1 : 1,
> > > > + exp = (float32 & ~BIT(31)) >> 23;
> > > > +
> > > > + /* special case 0 */
> > > > + if (!exp && !mantissa)
> > > > + return 0;
> > > > +
> > > > + exp -= 127;
> > > > + if (exp < 0) {
> > > > + exp = -exp;
> > > > + /* return values ranging from 1 to 99 */
> > > > + return sign * ((((BIT(23) + mantissa) * 100) >> 23) >> exp);
> > > > + }
> > > > +
> > > > + /* return values starting at 100 */
> > > > + shift = 23 - exp;
> > > > + float32 = BIT(exp) + (mantissa >> shift);
> > > > + fraction = mantissa & GENMASK(shift - 1, 0);
> > > > +
> > > > + return sign * (float32 * 100 + ((fraction * 100) >> shift));
> > > > +}
> > > > +
> > > > +static int scd30_read_meas(struct scd30_state *state)
> > > > +{
> > > > + int i, ret;
> > > > +
> > > > + ret = state->command(state, CMD_READ_MEAS, 0, state->meas,
> > > > + sizeof(state->meas));
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + be32_to_cpu_array(state->meas, state->meas, ARRAY_SIZE(state->meas));
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(state->meas); i++)
> > > > + state->meas[i] = scd30_float_to_fp(state->meas[i]);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int scd30_wait_meas_irq(struct scd30_state *state)
> > > > +{
> > > > + int ret, timeout;
> > > > +
> > > > + timeout = state->meas_interval * (1000 + SCD30_EXTRA_TIMEOUT_PER_S);
> > > > + timeout = msecs_to_jiffies(timeout);
> > > > + reinit_completion(&state->meas_ready);
> > > > + enable_irq(state->irq);
> > > > + ret = wait_for_completion_interruptible_timeout(&state->meas_ready,
> > > > + timeout);
> > > > + if (ret > 0)
> > > > + ret = 0;
> > > > + else if (!ret)
> > > > + ret = -ETIMEDOUT;
> > > > +
> > > > + disable_irq(state->irq);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int scd30_wait_meas_poll(struct scd30_state *state)
> > > > +{
> > > > + int timeout = state->meas_interval * SCD30_EXTRA_TIMEOUT_PER_S;
> > > > + int tries = 5;
> > > > +
> > > > + do {
> > > > + int ret;
> > > > + u16 val;
> > > > +
> > > > + ret = scd30_command_read(state, CMD_MEAS_READY, &val);
> > > > + if (ret)
> > > > + return -EIO;
> > > > +
> > > > + /* new measurement available */
> > > > + if (val)
> > > > + break;
> > > > +
> > > > + msleep_interruptible(timeout);
> > > > + } while (--tries);
> > > > +
> > > > + return tries ? 0 : -ETIMEDOUT;
> > > > +}
> > > > +
> > > > +static int scd30_read_poll(struct scd30_state *state)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + ret = scd30_wait_meas_poll(state);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + return scd30_read_meas(state);
> > > > +}
> > > > +
> > > > +static int scd30_read(struct scd30_state *state)
> > > > +{
> > > > + if (state->irq > 0)
> > > > + return scd30_wait_meas_irq(state);
> > > > +
> > > > + return scd30_read_poll(state);
> > > > +}
> > > > +
> > > > +static int scd30_read_raw(struct iio_dev *indio_dev,
> > > > + struct iio_chan_spec const *chan,
> > > > + int *val, int *val2, long mask)
> > > > +{
> > > > + struct scd30_state *state = iio_priv(indio_dev);
> > > > + int ret, meas[SCD30_MEAS_COUNT];
> > > > +
> > > > + switch (mask) {
> > > > + case IIO_CHAN_INFO_RAW:
> > > > + case IIO_CHAN_INFO_PROCESSED:
> > > > + ret = iio_device_claim_direct_mode(indio_dev);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + mutex_lock(&state->lock);
> > > > + ret = scd30_read(state);
> > > > + memcpy(meas, state->meas, SCD30_MEAS_COUNT * sizeof(*meas));
> > >
> > > The local copy seems a bit excessive. This isn't likely to be a particularly
> > > fast path so perhaps skip the copy but hold the locks until we are
> > > done with the buffer?
> > >
> >
> > Okay.
> >
> > > > + mutex_unlock(&state->lock);
> > > > + iio_device_release_direct_mode(indio_dev);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + switch (chan->type) {
> > > > + case IIO_CONCENTRATION:
> > > > + *val = meas[chan->address] / 1000000;
> > > > + *val2 = meas[chan->address] % 1000000;
> > > > +
> > > > + return IIO_VAL_INT_PLUS_MICRO;
> > > > + case IIO_TEMP:
> > > > + case IIO_HUMIDITYRELATIVE:
> > > > + *val = meas[chan->address] * 10;
> > > > +
> > > > + return IIO_VAL_INT;
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > + case IIO_CHAN_INFO_SCALE:
> > > > + switch (chan->type) {
> > > > + case IIO_CONCENTRATION:
> > > > + *val = 0;
> > > > + *val2 = 1;
> > > > +
> > > > + return IIO_VAL_INT_PLUS_MICRO;
> > > > + case IIO_TEMP:
> > > > + case IIO_HUMIDITYRELATIVE:
> > > > + *val = 10;
> > > > +
> > > > + return IIO_VAL_INT;
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > + case IIO_CHAN_INFO_SAMP_FREQ:
> > > > + *val = 0;
> > > > + *val2 = 0;
> > > > +
> > > > + mutex_lock(&state->lock);
> > > > + ret = scd30_command_read(state, CMD_MEAS_INTERVAL, (u16 *)val2);
> > >
> > > See below. I'll assume you'll fix all of these.
> > >
> > > > + mutex_unlock(&state->lock);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + *val2 = 1000000000 / *val2;
> > > > +
> > > > + return IIO_VAL_INT_PLUS_NANO;
> > > > + case IIO_CHAN_INFO_CALIBSCALE:
> > > > + mutex_lock(&state->lock);
> > > > + *val = state->pressure_comp / 10;
> > > > + *val2 = (state->pressure_comp % 10) * 100000;
> > > > + mutex_unlock(&state->lock);
> > > > +
> > > > + return IIO_VAL_INT_PLUS_MICRO;
> > > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > > + *val = 0;
> > > > + mutex_lock(&state->lock);
> > > > + ret = scd30_command_read(state, CMD_TEMP_OFFSET, (u16 *)val);
> > >
> > > Reading a u16 directly into a int is not a good idea. What you get will
> > > depend on the endianness of the machine.
> > >
> >
> > Right, that would obviously break on BE. Sometimes trying to keep number
> > of local variables low simply stops paying off :).
> >
> > > Use an intermediate variable of the right size.
> > >
> > > > + mutex_unlock(&state->lock);
> > > > +
> > > > + return IIO_VAL_INT;
> > > > + }
> > > > +
> > > > + return -EINVAL;
> > > > +}
> > > > +
> > > > +static int scd30_write_raw(struct iio_dev *indio_dev,
> > > > + struct iio_chan_spec const *chan,
> > > > + int val, int val2, long mask)
> > > > +{
> > > > + struct scd30_state *state = iio_priv(indio_dev);
> > > > + int ret = -EINVAL;
> > > > +
> > > > + mutex_lock(&state->lock);
> > > > + switch (mask) {
> > > > + case IIO_CHAN_INFO_SAMP_FREQ:
> > > > + if (val)
> > > > + break;
> > > > +
> > > > + val = 1000000000 / val2;
> > > > + if (val < SCD30_MEAS_INTERVAL_MIN_S ||
> > > > + val > SCD30_MEAS_INTERVAL_MAX_S)
> > > > + break;
> > > > +
> > > > + ret = scd30_command_write(state, CMD_MEAS_INTERVAL, val);
> > > > + if (ret)
> > > > + break;
> > > > +
> > > > + state->meas_interval = val;
> > > > + break;
> > > > + case IIO_CHAN_INFO_CALIBSCALE:
> > > > + val = (val * 1000000 + val2) / 100000;
> > > > + if (val < SCD30_PRESSURE_COMP_MIN_MBAR ||
> > > > + val > SCD30_PRESSURE_COMP_MAX_MBAR)
> > > > + break;
> > > > +
> > > > + ret = scd30_command_write(state, CMD_START_MEAS, val);
> > > > + if (ret)
> > > > + break;
> > > > +
> > > > + state->pressure_comp = val;
> > > > + break;
> > > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > > + if (val < 0 || val > SCD30_TEMP_OFFSET_MAX)
> > > > + break;
> > > > + /*
> > > > + * Manufacturer does not explicitly specify min/max sensible
> > > > + * values hence check is omitted for simplicity.
> > > > + */
> > > > + ret = scd30_command_write(state, CMD_TEMP_OFFSET / 10, val);
> > > > + }
> > > > + mutex_unlock(&state->lock);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int scd30_write_raw_get_fmt(struct iio_dev *indio_dev,
> > > > + struct iio_chan_spec const *chan, long mask)
> > > > +{
> > > > + switch (mask) {
> > > > + case IIO_CHAN_INFO_RAW:
> > > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > > + return IIO_VAL_INT;
> > > > + case IIO_CHAN_INFO_CALIBSCALE:
> > > > + return IIO_VAL_INT_PLUS_MICRO;
> > > > + case IIO_CHAN_INFO_SAMP_FREQ:
> > > > + return IIO_VAL_INT_PLUS_NANO;
> > > > + }
> > > > +
> > > > + return -EINVAL;
> > > > +}
> > > > +
> > > > +static const int scd30_pressure_calibscale_available[] = {
> > > > + SCD30_PRESSURE_COMP_MIN_MBAR / 10, 0,
> > > > + 0, 100000,
> > > > + SCD30_PRESSURE_COMP_MAX_MBAR / 10, 0,
> > > > +};
> > > > +
> > > > +static const int scd30_temp_calibbias_available[] = {
> > > > + 0, 10, SCD30_TEMP_OFFSET_MAX,
> > > > +};
> > > > +
> > > > +static int scd30_read_avail(struct iio_dev *indio_dev,
> > > > + struct iio_chan_spec const *chan, const int **vals,
> > > > + int *type, int *length, long mask)
> > > > +{
> > > > + switch (mask) {
> > > > + case IIO_CHAN_INFO_CALIBSCALE:
> > > > + *vals = scd30_pressure_calibscale_available;
> > > > + *type = IIO_VAL_INT_PLUS_MICRO;
> > > > +
> > > > + return IIO_AVAIL_RANGE;
> > > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > > + *vals = scd30_temp_calibbias_available;
> > > > + *type = IIO_VAL_INT;
> > > > +
> > > > + return IIO_AVAIL_RANGE;
> > > > + }
> > > > +
> > > > + return -EINVAL;
> > > > +}
> > > > +
> > > > +static ssize_t sampling_frequency_available_show(struct device *dev,
> > > > + struct device_attribute *attr,
> > > > + char *buf)
> > > > +{
> > > > + int i = SCD30_MEAS_INTERVAL_MIN_S;
> > > > + ssize_t len = 0;
> > > > +
> > > > + do {
> > > > + len += scnprintf(buf + len, PAGE_SIZE - len, "0.%09u ",
> > > > + 1000000000 / i);
> > > > + /*
> > > > + * Not all values fit PAGE_SIZE buffer hence print every 6th
> > > > + * (each frequency differs by 6s in time domain from the
> > > > + * adjecent). Unlisted but valid ones are still accepted.
> > >
> > > adjacent
> > >
> > > Hmm. Maybe we need to think about some description for inverse of linear
> > > cases as they are likely to be fairly common.
> > > This will work in meantime.
> > >
> > > > + */
> > > > + i += 6;
> > > > + } while (i <= SCD30_MEAS_INTERVAL_MAX_S);
> > > > +
> > > > + buf[len - 1] = '\n';
> > > > +
> > > > + return len;
> > > > +}
> > > > +
> > > > +static ssize_t calibration_show(struct device *dev,
> > > > + struct device_attribute *attr, char *buf)
> > > > +{
> > > > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > > + struct scd30_state *state = iio_priv(indio_dev);
> > > > + u16 asc, frc;
> > > > + int ret;
> > > > +
> > > > + mutex_lock(&state->lock);
> > > > + ret = scd30_command_read(state, CMD_ASC, &asc);
> > > > + if (ret)
> > > > + goto out;
> > > > +
> > > > + ret = scd30_command_read(state, CMD_FRC, &frc);
> > > > +out:
> > > > + mutex_unlock(&state->lock);
> > > > +
> > > > + return ret ?: sprintf(buf, "%d %d\n", asc, frc);
> > > > +}
> > > > +
> > > > +static ssize_t calibration_store(struct device *dev,
> > > > + struct device_attribute *attr, const char *buf,
> > > > + size_t len)
> > > > +{
> > > > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > > + struct scd30_state *state = iio_priv(indio_dev);
> > > > + int ret;
> > > > + u16 val;
> > >
> > > As commented above, this interface doesn't win on the
> > > obvious front so needs a rethink!
> > >
> > > > +
> > > > + ret = kstrtou16(buf, 0, &val);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + mutex_lock(&state->lock);
> > > > + if (val == 0 || val == 1)
> > > > + ret = scd30_command_write(state, CMD_ASC, val);
> > > > + else if (val >= SCD30_FRC_MIN_PPM && val <= SCD30_FRC_MAX_PPM)
> > > > + ret = scd30_command_write(state, CMD_FRC, val);
> > > > + else
> > > > + ret = -EINVAL;
> > > > + mutex_unlock(&state->lock);
> > > > +
> > > > + return ret ?: len;
> > > > +}
> > > > +
> > > > +static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
> > > > +static IIO_DEVICE_ATTR_RW(calibration, 0);
> > > > +
> > > > +static struct attribute *scd30_attrs[] = {
> > > > + &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> > > > + &iio_dev_attr_calibration.dev_attr.attr,
> > > > + NULL
> > > > +};
> > > > +
> > > > +static const struct attribute_group scd30_attr_group = {
> > > > + .attrs = scd30_attrs,
> > > > +};
> > > > +
> > > > +static const struct iio_info scd30_info = {
> > > > + .attrs = &scd30_attr_group,
> > > > + .read_raw = scd30_read_raw,
> > > > + .write_raw = scd30_write_raw,
> > > > + .write_raw_get_fmt = scd30_write_raw_get_fmt,
> > > > + .read_avail = scd30_read_avail,
> > > > +};
> > > > +
> > > > +#define SCD30_CHAN_SCAN_TYPE(_sign, _realbits) .scan_type = { \
> > > > + .sign = _sign, \
> > > > + .realbits = _realbits, \
> > > > + .storagebits = 32, \
> > > > + .endianness = IIO_CPU, \
> > > > +}
> > > > +
> > > > +static const struct iio_chan_spec scd30_channels[] = {
> > > > + {
> > > > + .type = IIO_PRESSURE,
> > > > + .info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE),
> > > > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_CALIBSCALE),
> > > > + .scan_index = -1,
> > > > + },
> > > > + {
> > > > + .type = IIO_CONCENTRATION,
> > > > + .channel2 = IIO_MOD_CO2,
> > > > + .address = SCD30_CONC,
> > > > + .scan_index = SCD30_CONC,
> > > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > > > + BIT(IIO_CHAN_INFO_SCALE),
> > > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > > > + .modified = 1,
> > > > +
> > > > + SCD30_CHAN_SCAN_TYPE('u', 16),
> > > > + },
> > > > + {
> > > > + .type = IIO_TEMP,
> > > > + .address = SCD30_TEMP,
> > > > + .scan_index = SCD30_TEMP,
> > > > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > > > + BIT(IIO_CHAN_INFO_CALIBBIAS) |
> > > > + BIT(IIO_CHAN_INFO_SCALE),
> > >
> > > Combination of processed and scale is unusual. Normally scale provides
> > > a conversion factor or a _RAW reading.
> >
> > Right that's pointless. Scales were for raw measurements inside buffer.
> > Somehow I failed to realize that only co2 concentration is raw.
> >
> > >
> > > I 'think' these units are otherwise fine (milli degrees centigrade)
> > >
> > >
> > > > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_CALIBBIAS),
> > > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > > > +
> > > > + SCD30_CHAN_SCAN_TYPE('s', 14),
> > > > + },
> > > > + {
> > > > + .type = IIO_HUMIDITYRELATIVE,
> > > > + .address = SCD30_HR,
> > > > + .scan_index = SCD30_HR,
> > > > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > > > + BIT(IIO_CHAN_INFO_SCALE),
> > >
> > > As above. Not normal to see scale and processed.
> > >
> > > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > > > +
> > > > + SCD30_CHAN_SCAN_TYPE('u', 14),
> > > > + },
> > > > + IIO_CHAN_SOFT_TIMESTAMP(3),
> > > > +};
> > > > +
> > > > +int __maybe_unused scd30_suspend(struct device *dev)
> > > > +{
> > > > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > > > + struct scd30_state *state = iio_priv(indio_dev);
> > > > + int ret;
> > > > +
> > > > + ret = scd30_command_write(state, CMD_STOP_MEAS, 0);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + return regulator_disable(state->vdd);
> > > > +}
> > > > +EXPORT_SYMBOL(scd30_suspend);
> > > > +
> > > > +int __maybe_unused scd30_resume(struct device *dev)
> > > > +{
> > > > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > > > + struct scd30_state *state = iio_priv(indio_dev);
> > > > + int ret;
> > > > +
> > > > + ret = regulator_enable(state->vdd);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + return scd30_command_write(state, CMD_START_MEAS, state->pressure_comp);
> > > > +}
> > > > +EXPORT_SYMBOL(scd30_resume);
> > > > +
> > > > +static void scd30_stop_meas(void *data)
> > > > +{
> > > > + struct scd30_state *state = data;
> > > > +
> > > > + scd30_command_write(state, CMD_STOP_MEAS, 0);
> > > > +}
> > > > +
> > > > +static void scd30_disable_regulator(void *data)
> > > > +{
> > > > + struct scd30_state *state = data;
> > > > +
> > > > + regulator_disable(state->vdd);
> > > > +}
> > > > +
> > > > +static irqreturn_t scd30_irq_handler(int irq, void *priv)
> > > > +{
> > > > + struct iio_dev *indio_dev = priv;
> > > > +
> > > > + if (iio_buffer_enabled(indio_dev)) {
> > >
> > > There is a potential quirk here. It's possible that
> > > this device is using a different trigger, but another device
> > > is registered to use this one. If that happens this check
> > > will be a bit counter intuitive.
> > >
> > > As such you might want to provide the validate callback so
> > > that this device is the only device allowed to use it's
> > > own trigger.
> > >
> >
> > Right. This needs to be fixed.
> >
> > > > + iio_trigger_poll(indio_dev->trig);
> > > > +
> > > > + return IRQ_HANDLED;
> > > > + }
> > > > +
> > > > + return IRQ_WAKE_THREAD;
> > > > +}
> > > > +
> > > > +static irqreturn_t scd30_irq_thread_handler(int irq, void *priv)
> > > > +{
> > > > + struct iio_dev *indio_dev = priv;
> > > > + struct scd30_state *state = iio_priv(indio_dev);
> > > > + int ret;
> > > > +
> > > > + ret = scd30_read_meas(state);
> > > > + if (ret)
> > > > + goto out;
> > > > +
> > > > + complete_all(&state->meas_ready);
> > > > +out:
> > > > + return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +static irqreturn_t scd30_trigger_handler(int irq, void *p)
> > > > +{
> > > > + struct iio_poll_func *pf = p;
> > > > + struct iio_dev *indio_dev = pf->indio_dev;
> > > > + struct scd30_state *state = iio_priv(indio_dev);
> > > > + struct {
> > > > + int data[SCD30_MEAS_COUNT];
> > > > + u64 ts;
> > >
> > > Turns out I was wrong when suggesting this approach for drivers. On x86_32
> > > this will result in there not being any padding between the
> > > data and the timestamp (and in IIO rule of naturally aligned there
> > > needs to be 4 bytes there). Result is that this structure is
> > > too short. (thanks btw to Andy who pointed out this issue!)
> > >
> > > So, to force that my current preference is.
> > >
> > > struct {
> > > int data[SCD30_MEAS_COUNT];
> > > s64 ts __aligned(8);
> > > } scan;
> > >
> >
> > Ah, so x86_32 aligns s64 to 4 bytes.
>
> yup
>
> >
> > > However, given we do have a hole in the structure there is
> > > a kernel data leak. So either you need to zero it here,
> > > or move it into the iio_priv() structure. Doing that
> > > will ensure it is zeroed at allocation.
> > >
> > > > + } scan;
> > > > + int ret;
> > > > +
> > > > + mutex_lock(&state->lock);
> > > > + if (!iio_trigger_using_own(indio_dev))
> > > > + ret = scd30_read_poll(state);
> > > > + else
> > > > + ret = scd30_read_meas(state);
> > > > + memcpy(scan.data, state->meas, sizeof(state->meas));
> > > > + mutex_unlock(&state->lock);
> > > > + if (ret)
> > > > + goto out;
> > > > +
> > > > + iio_push_to_buffers_with_timestamp(indio_dev, &scan,
> > > > + iio_get_time_ns(indio_dev));
> > > > +out:
> > > > + iio_trigger_notify_done(indio_dev->trig);
> > > > + return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +static int scd30_set_trigger_state(struct iio_trigger *trig, bool state)
> > > > +{
> > > > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > > > + struct scd30_state *st = iio_priv(indio_dev);
> > > > +
> > > > + if (state)
> > > > + enable_irq(st->irq);
> > > > + else
> > > > + disable_irq(st->irq);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static const struct iio_trigger_ops scd30_trigger_ops = {
> > > > + .set_trigger_state = scd30_set_trigger_state,
> > > > +};
> > > > +
> > > > +static int scd30_setup_trigger(struct iio_dev *indio_dev)
> > > > +{
> > > > + struct scd30_state *state = iio_priv(indio_dev);
> > > > + struct device *dev = indio_dev->dev.parent;
> > > > + struct iio_trigger *trig;
> > > > + int ret;
> > > > +
> > > > + trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
> > > > + indio_dev->id);
> > > > + if (!trig) {
> > > > + dev_err(dev, "failed to allocate trigger\n");
> > > > + return -ENOMEM;
> > > > + }
> > > > +
> > > > + trig->dev.parent = dev;
> > > > + trig->ops = &scd30_trigger_ops;
> > > > + iio_trigger_set_drvdata(trig, indio_dev);
> > > > +
> > > > + ret = devm_iio_trigger_register(dev, trig);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + indio_dev->trig = iio_trigger_get(trig);
> > > > +
> > > > + ret = devm_request_threaded_irq(dev, state->irq, scd30_irq_handler,
> > > > + scd30_irq_thread_handler,
> > > > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > > > + indio_dev->name, indio_dev);
> > > > + if (ret)
> > > > + dev_err(dev, "failed to request irq\n");
> > > > +
> > > > + disable_irq(state->irq);
> > >
> > > Given there is a gap between the request above and this disable, this
> > > disable needs a comment explaining why it is here.
> > >
> > > I'm assuming it's an optimization?
> > >
> >
> > Interrupt is enabled just before taking measurement to grab the fresh
> > data and disabled afterwards. Not disabling it here would produce fat warning
> > about unbalanced irqs.
> >
> > And that is because sensor takes measurements continuously. On demand
> > mode, even though possible, doesn't work reliably. Sensor (at least the one
> > sitting on my desk) needs way too much time to wakeup and grab measurement which
> > makes the whole point of adjustable sampling frequency pointless :).
> >
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +int scd30_probe(struct device *dev, int irq, const char *name, void *priv,
> > > > + scd30_command_t command)
> > > > +{
> > > > + static const unsigned long scd30_scan_masks[] = { 0x07, 0x00 };
> > > > + struct scd30_state *state;
> > > > + struct iio_dev *indio_dev;
> > > > + int ret;
> > > > + u16 val;
> > > > +
> > > > + indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
> > > > + if (!indio_dev)
> > > > + return -ENOMEM;
> > > > +
> > > > + state = iio_priv(indio_dev);
> > > > + state->dev = dev;
> > >
> > > Doesn't seem to be used.
> > >
> > > > + state->priv = priv;
> > >
> > > What's this for? At least at first glance I can't find it being used
> > > anywhere.
> > >
> > > > + state->irq = irq;
> > > > + state->pressure_comp = SCD30_PRESSURE_COMP_DEFAULT;
> > > > + state->meas_interval = SCD30_MEAS_INTERVAL_DEFAULT;
> > > > + state->command = command;
> > > > + mutex_init(&state->lock);
> > > > + init_completion(&state->meas_ready);
> > > > +
> > > > + dev_set_drvdata(dev, indio_dev);
> > > > +
> > > > + indio_dev->dev.parent = dev;
> > >
> > > Side note that there is a series moving this into the core under revision at
> > > the moment. Hopefully I'll remember to fix this up when applying your patch
> > > if that one has gone in ahead of it.
> > >
> > > > + indio_dev->info = &scd30_info;
> > > > + indio_dev->name = name;
> > > > + indio_dev->channels = scd30_channels;
> > > > + indio_dev->num_channels = ARRAY_SIZE(scd30_channels);
> > > > + indio_dev->modes = INDIO_DIRECT_MODE;
> > > > + indio_dev->available_scan_masks = scd30_scan_masks;
> > > > +
> > > > + state->vdd = devm_regulator_get(dev, "vdd");
> > > > + if (IS_ERR(state->vdd)) {
> > > > + if (PTR_ERR(state->vdd) == -EPROBE_DEFER)
> > > > + return -EPROBE_DEFER;
> > > > +
> > > > + dev_err(dev, "failed to get regulator\n");
> > > > + return PTR_ERR(state->vdd);
> > > > + }
> > > > +
> > > > + ret = regulator_enable(state->vdd);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = devm_add_action_or_reset(dev, scd30_disable_regulator, state);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > >
> > > A comment here on why it makes sense to register this here. What
> > > started mesurement? It seems that happens well below here so
> > > we should really call this after that start all.
> > >
> >
> > Sensor after being powered up starts in mode it was left in.
> > Chances are it was continuous mode and we want to shut it down.
>
> That's fine. The question is why 'here' as opposed to after the below where you
> put it into continuous mode.
>
Let's suppose sensor got energized and started measuring. Then without
registering action which stops measurement we jump to device reset etc.
Now if reset failed for whatever reason (same applies to everything
below reset) devm will gracefully unwind previous actions but sensor
will continue doing his job. But there's no point. Better to save some
milliaps for later.
In case we have real regulator then there's no real issue because
power gets cut off during cleanup.
Quite often though there's only a dummy one which does nothing useful
except making regulator framework happy.
So my thinking here is that we're slightly better off registering
scd30_stop_meas() action earlier to prevent such scenario from happening.
> >
> > > > + ret = devm_add_action_or_reset(dev, scd30_stop_meas, state);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = scd30_reset(state);
> > > > + if (ret) {
> > > > + dev_err(dev, "failed to reset device: %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + if (state->irq > 0) {
> > > > + ret = scd30_setup_trigger(indio_dev);
> > > > + if (ret) {
> > > > + dev_err(dev, "failed to setup trigger: %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > + }
> > > > +
> > > > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > > > + scd30_trigger_handler, NULL);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = scd30_command_read(state, CMD_FW_VERSION, &val);
> > > > + if (ret) {
> > > > + dev_err(dev, "failed to read firmware version: %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > + dev_info(dev, "firmware version: %d.%d\n", val >> 8, (char)val);
> > > > +
> > > > + ret = scd30_command_write(state, CMD_MEAS_INTERVAL,
> > > > + state->meas_interval);
> > > > + if (ret) {
> > > > + dev_err(dev, "failed to set measurement interval: %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = scd30_command_write(state, CMD_START_MEAS, state->pressure_comp);
> > > > + if (ret) {
> > > > + dev_err(dev, "failed to start measurement: %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return devm_iio_device_register(dev, indio_dev);
> > > > +}
> > > > +EXPORT_SYMBOL(scd30_probe);
> > > > +
> > > > +MODULE_AUTHOR("Tomasz Duszynski <tomasz.duszynski@octakon.com>");
> > > > +MODULE_DESCRIPTION("Sensirion SCD30 carbon dioxide sensor core driver");
> > > > +MODULE_LICENSE("GPL v2");
> > > > --
> > > > 2.26.2
> > > >
> > >
>
>
^ permalink raw reply
* [v2] drm/msm: add shutdown support for display platform_driver
From: Krishna Manikandan @ 2020-06-01 11:03 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, freedreno, devicetree
Cc: Krishna Manikandan, linux-kernel, robdclark, seanpaul, hoegsberg,
kalyan_t, nganji, mka
Define shutdown callback for display drm driver,
so as to disable all the CRTCS when shutdown
notification is received by the driver.
This change will turn off the timing engine so
that no display transactions are requested
while mmu translations are getting disabled
during reboot sequence.
Signed-off-by: Krishna Manikandan <mkrishn@codeaurora.org>
Changes in v2:
- Remove NULL check from msm_pdev_shutdown (Stephen Boyd)
- Change commit text to reflect when this issue
was uncovered (Sai Prakash Ranjan)
---
drivers/gpu/drm/msm/msm_drv.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index e4b750b..94e3963 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1322,6 +1322,13 @@ static int msm_pdev_remove(struct platform_device *pdev)
return 0;
}
+static void msm_pdev_shutdown(struct platform_device *pdev)
+{
+ struct drm_device *drm = platform_get_drvdata(pdev);
+
+ drm_atomic_helper_shutdown(drm);
+}
+
static const struct of_device_id dt_match[] = {
{ .compatible = "qcom,mdp4", .data = (void *)KMS_MDP4 },
{ .compatible = "qcom,mdss", .data = (void *)KMS_MDP5 },
@@ -1334,6 +1341,7 @@ static int msm_pdev_remove(struct platform_device *pdev)
static struct platform_driver msm_platform_driver = {
.probe = msm_pdev_probe,
.remove = msm_pdev_remove,
+ .shutdown = msm_pdev_shutdown,
.driver = {
.name = "msm",
.of_match_table = dt_match,
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] arm: allwinner: a20: Add Drejo DS167 initial support
From: Maxime Ripard @ 2020-06-01 10:49 UTC (permalink / raw)
To: stulluk; +Cc: robh+dt, wens, devicetree, linux-kernel, linux-sunxi
In-Reply-To: <20200531110538.30153-1-stulluk@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 12482 bytes --]
Hi,
Please make sure that all the proper recipients are in Cc. The
linux-arm-kernel ML especially is missing, get_maintainers will give you
the list.
Also, the prefix of your commit log is supposed to be
ARM: dts: sunxi: ...
On Sun, May 31, 2020 at 02:05:38PM +0300, stulluk@gmail.com wrote:
> From: Sertac TULLUK <stulluk@gmail.com>
>
> Drejo DS167 is an Allwinner A20 based IoT device,
> which support
> - Allwinner A20 Cortex-A7
> - Mali-400MP2 GPU
> - AXP209 PMIC
> - 1GB DDR3 RAM
> - 8GB eMMC
> - 10/100M Ethernet
> - SATA
> - HDMI
> - 10.1inch and 7.0inch LVDS LCD and Touch screens
> - CSI: OV5640 sensor
> - USB OTG
> - 2x USB2.0
> - built-in KNX Transceiver
> - 3x Dry Contact Input
> - 3x Relay output
> - IR RX/TX
> - UART3
> - SPI1
> - RTC Battery
> - 8x GPIO
> - Analogue + Digital Microphone
> - PAM8620 speaker Amplifier
> - 12V DC power supply
> - 3.7V Battery Operable
>
> Signed-off-by: Sertac TULLUK <stulluk@gmail.com>
> ---
> arch/arm/boot/dts/Makefile | 2 +
> .../boot/dts/sun7i-a20-drejo-ds167-emmc.dts | 69 +++++
> arch/arm/boot/dts/sun7i-a20-drejo-ds167.dts | 288 ++++++++++++++++++
> 3 files changed, 359 insertions(+)
> create mode 100644 arch/arm/boot/dts/sun7i-a20-drejo-ds167-emmc.dts
> create mode 100644 arch/arm/boot/dts/sun7i-a20-drejo-ds167.dts
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 3823090d07e7..d81e685dee38 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1097,6 +1097,8 @@ dtb-$(CONFIG_MACH_SUN7I) += \
> sun7i-a20-bananapro.dtb \
> sun7i-a20-cubieboard2.dtb \
> sun7i-a20-cubietruck.dtb \
> + sun7i-a20-drejo-ds167.dtb \
> + sun7i-a20-drejo-ds167-emmc.dtb \
You indented with spaces here, instead of a tab.
> sun7i-a20-hummingbird.dtb \
> sun7i-a20-itead-ibox.dtb \
> sun7i-a20-i12-tvbox.dtb \
> diff --git a/arch/arm/boot/dts/sun7i-a20-drejo-ds167-emmc.dts b/arch/arm/boot/dts/sun7i-a20-drejo-ds167-emmc.dts
> new file mode 100644
> index 000000000000..b6147eb013b0
> --- /dev/null
> +++ b/arch/arm/boot/dts/sun7i-a20-drejo-ds167-emmc.dts
> @@ -0,0 +1,69 @@
> +/*
> + * Copyright 2020 Sertac TULLUK
> + *
> + * Sertac TULLUK <stulluk@gmail.com>
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + * a) This file is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * This file is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * Or, alternatively,
> + *
> + * b) Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use,
> + * copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following
> + * conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
You should use an SPDX tag here
> +#include "sun7i-a20-drejo-ds167.dts"
Is there any reason not to have the emmc into the main DTS?
> +/ {
> + model = "drejo DS167-eMMC";
Drejo should have an uppercase?
> + compatible = "drejo,sun7i-a20-drejo-ds167-emmc", "allwinner,sun7i-a20";
This compatible should be documented.
Also, this can be something like drejo,ds167 (the vendor should be
documented too )
> +
> + mmc2_pwrseq: pwrseq {
> + compatible = "mmc-pwrseq-emmc";
> + reset-gpios = <&pio 2 24 GPIO_ACTIVE_LOW>;
> + };
> +};
> +
> +&mmc2 {
> + vmmc-supply = <®_vcc3v3>;
> + bus-width = <4>;
> + non-removable;
> + mmc-pwrseq = <&mmc2_pwrseq>;
> + status = "okay";
> +
> + emmc: emmc@0 {
> + reg = <0>;
> + compatible = "mmc-card";
> + broken-hpi;
> + };
> +};
> diff --git a/arch/arm/boot/dts/sun7i-a20-drejo-ds167.dts b/arch/arm/boot/dts/sun7i-a20-drejo-ds167.dts
> new file mode 100644
> index 000000000000..79db92f88251
> --- /dev/null
> +++ b/arch/arm/boot/dts/sun7i-a20-drejo-ds167.dts
> @@ -0,0 +1,288 @@
> +/*
> + * Copyright 2020 Sertac TULLUK
> + *
> + * Sertac TULLUK <stulluk@gmail.com>
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + * a) This file is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * This file is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * Or, alternatively,
> + *
> + * b) Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use,
> + * copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following
> + * conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +/dts-v1/;
> +#include "sun7i-a20.dtsi"
> +#include "sunxi-common-regulators.dtsi"
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +
> +/ {
> + model = "drejo DS167";
> + compatible = "drejo,sun7i-a20-drejo-ds167", "allwinner,sun7i-a20";
> +
> + aliases {
> + serial0 = &uart0;
> + serial1 = &uart3;
> +
> + spi0 = &spi1;
> + spi1 = &spi2;
> + };
> +
> + chosen {
> + stdout-path = "serial0:115200n8";
> + };
> +
> + hdmi-connector {
> + compatible = "hdmi-connector";
> + type = "a";
> +
> + port {
> + hdmi_con_in: endpoint {
> + remote-endpoint = <&hdmi_out_con>;
> + };
> + };
> + };
> +
> + leds {
> + compatible = "gpio-leds";
> + pinctrl-names = "default";
> + pinctrl-0 = <&led_pins_ds167>;
You don't need a pinctrl node for the GPIOs
> +
> + red {
> + label = "ds167-status";
This isn't the proper red name, it should be something like
ds167:red:status
> + gpios = <&pio 8 9 GPIO_ACTIVE_LOW>; /* PI9 STATUS LED NEAR A20 SERTAC */
> + default-state = "on";
> + };
> + };
> +};
> +
> +&ahci {
> + target-supply = <®_ahci_5v>;
> + status = "okay";
> +};
> +
> +&codec {
> + status = "okay";
> +};
> +
> +&cpu0 {
> + cpu-supply = <®_dcdc2>;
> +};
> +
> +&de {
> + status = "okay";
> +};
> +
> +&ehci0 {
> + status = "okay";
> +};
> +
> +&ehci1 {
> + status = "okay";
> +};
> +
> +&gmac {
> + pinctrl-names = "default";
> + pinctrl-0 = <&gmac_mii_pins>, <&gmac_txerr>;
> + phy-handle = <&phy1>;
> + phy-mode = "mii";
> + status = "okay";
> +};
> +
> +&hdmi {
> + status = "okay";
> +};
> +
> +&hdmi_out {
> + hdmi_out_con: endpoint {
> + remote-endpoint = <&hdmi_con_in>;
> + };
> +};
> +
> +&i2c0 {
> + status = "okay";
> +
> + axp209: pmic@34 {
> + reg = <0x34>;
> + interrupt-parent = <&nmi_intc>;
> + interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> + };
> +};
> +
> +&i2c2 {
> + status = "okay";
> +};
> +
> +&lradc {
> + vref-supply = <®_vcc3v0>;
> + status = "okay";
> +};
> +
> +&gmac_mdio {
> + phy1: ethernet-phy@1 {
> + reg = <1>;
> + };
> +};
> +
> +&mmc0 {
> + vmmc-supply = <®_vcc3v3>;
> + bus-width = <4>;
> + cd-gpios = <&pio 7 1 GPIO_ACTIVE_LOW>; /* PH1 */
> + status = "okay";
> +};
> +
> +&ohci0 {
> + status = "okay";
> +};
> +
> +&ohci1 {
> + status = "okay";
> +};
> +
> +&otg_sram {
> + status = "okay";
> +};
> +
> +&pio {
> + gmac_txerr: gmac-txerr-pin {
> + pins = "PA17";
> + function = "gmac";
> + };
> +
> + led_pins_ds167: led-pins {
> + pins = "PI9"; /* Status led, on schematic, this is LED_EN */
> + function = "gpio_out";
> + drive-strength = <20>;
> + };
> +};
> +
> +&pwm {
> + pinctrl-names = "default";
> + pinctrl-0 = <&pwm0_pin>, <&pwm1_pin>;
> + status = "okay";
> +};
> +
> +#include "axp209.dtsi"
> +
> +&ac_power_supply {
> + status = "okay";
> +};
> +
> +&battery_power_supply {
> + status = "okay";
> +};
> +
> +®_dcdc2 {
> + regulator-always-on;
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1400000>;
> + regulator-name = "vdd-cpu";
> +};
> +
> +®_dcdc3 {
> + regulator-always-on;
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1400000>;
> + regulator-name = "vdd-int-dll";
> +};
> +
> +®_ldo2 {
> + regulator-always-on;
> + regulator-min-microvolt = <3000000>;
> + regulator-max-microvolt = <3000000>;
> + regulator-name = "avcc";
> +};
> +
> +®_ahci_5v {
> + status = "okay";
> +};
> +
> +®_usb0_vbus {
> + status = "okay";
> +};
> +
> +®_usb1_vbus {
> + status = "okay";
> +};
> +
> +®_usb2_vbus {
> + status = "okay";
> +};
> +
> +&spi1 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&spi1_pi_pins>,
> + <&spi1_cs0_pi_pin>;
> + status = "okay";
> +};
> +
> +&spi2 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&spi2_pc_pins>,
> + <&spi2_cs0_pc_pin>;
> + status = "okay";
> +};
> +
> +&uart0 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart0_pb_pins>;
> + status = "okay";
> +};
> +
> +&uart3 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart3_pg_pins>;
> + status = "okay";
> +};
> +
> +&usb_otg {
> + dr_mode = "otg";
> + status = "okay";
> +};
> +
> +&usb_power_supply {
> + status = "okay";
> +};
> +
> +&usbphy {
> + usb0_id_det-gpios = <&pio 7 4 (GPIO_ACTIVE_HIGH | GPIO_PULL_UP)>; /* PH4 */
> + usb0_vbus_det-gpios = <&pio 7 5 (GPIO_ACTIVE_HIGH | GPIO_PULL_DOWN)>; /* PH5 */
> + usb0_vbus-supply = <®_usb0_vbus>;
> + usb1_vbus-supply = <®_usb1_vbus>;
> + usb2_vbus-supply = <®_usb2_vbus>;
> + status = "okay";
> +};
Looks good otherwise, thanks!
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* [PATCH] dt-bindings: mailbox: Convert imx mu to json-schema
From: Anson Huang @ 2020-06-01 10:37 UTC (permalink / raw)
To: robh+dt, linux, peng.fan, jaswinder.singh, hongxing.zhu,
aisheng.dong, devicetree, linux-kernel
Cc: Linux-imx
Convert the i.MX MU binding to DT schema format using json-schema
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
.../devicetree/bindings/mailbox/fsl,mu.txt | 58 --------------
.../devicetree/bindings/mailbox/fsl,mu.yaml | 89 ++++++++++++++++++++++
2 files changed, 89 insertions(+), 58 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/mailbox/fsl,mu.txt
create mode 100644 Documentation/devicetree/bindings/mailbox/fsl,mu.yaml
diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
deleted file mode 100644
index 26b7a88..0000000
--- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
+++ /dev/null
@@ -1,58 +0,0 @@
-NXP i.MX Messaging Unit (MU)
---------------------------------------------------------------------
-
-The Messaging Unit module enables two processors within the SoC to
-communicate and coordinate by passing messages (e.g. data, status
-and control) through the MU interface. The MU also provides the ability
-for one processor to signal the other processor using interrupts.
-
-Because the MU manages the messaging between processors, the MU uses
-different clocks (from each side of the different peripheral buses).
-Therefore, the MU must synchronize the accesses from one side to the
-other. The MU accomplishes synchronization using two sets of matching
-registers (Processor A-facing, Processor B-facing).
-
-Messaging Unit Device Node:
-=============================
-
-Required properties:
--------------------
-- compatible : should be "fsl,<chip>-mu", the supported chips include
- imx6sx, imx7s, imx8qxp, imx8qm.
- The "fsl,imx6sx-mu" compatible is seen as generic and should
- be included together with SoC specific compatible.
- There is a version 1.0 MU on imx7ulp, use "fsl,imx7ulp-mu"
- compatible to support it.
- To communicate with i.MX8 SCU, "fsl,imx8-mu-scu" could be
- used for fast IPC
-- reg : Should contain the registers location and length
-- interrupts : Interrupt number. The interrupt specifier format depends
- on the interrupt controller parent.
-- #mbox-cells: Must be 2.
- <&phandle type channel>
- phandle : Label name of controller
- type : Channel type
- channel : Channel number
-
- This MU support 4 type of unidirectional channels, each type
- has 4 channels. A total of 16 channels. Following types are
- supported:
- 0 - TX channel with 32bit transmit register and IRQ transmit
- acknowledgment support.
- 1 - RX channel with 32bit receive register and IRQ support
- 2 - TX doorbell channel. Without own register and no ACK support.
- 3 - RX doorbell channel.
-
-Optional properties:
--------------------
-- clocks : phandle to the input clock.
-- fsl,mu-side-b : Should be set for side B MU.
-
-Examples:
---------
-lsio_mu0: mailbox@5d1b0000 {
- compatible = "fsl,imx8qxp-mu", "fsl,imx6sx-mu";
- reg = <0x0 0x5d1b0000 0x0 0x10000>;
- interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
- #mbox-cells = <2>;
-};
diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.yaml b/Documentation/devicetree/bindings/mailbox/fsl,mu.yaml
new file mode 100644
index 0000000..93db996
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.yaml
@@ -0,0 +1,89 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/fsl,mu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP i.MX Messaging Unit (MU)
+
+maintainers:
+ - Dong Aisheng <aisheng.dong@nxp.com>
+
+description: |
+ The Messaging Unit module enables two processors within the SoC to
+ communicate and coordinate by passing messages (e.g. data, status
+ and control) through the MU interface. The MU also provides the ability
+ for one processor to signal the other processor using interrupts.
+
+ Because the MU manages the messaging between processors, the MU uses
+ different clocks (from each side of the different peripheral buses).
+ Therefore, the MU must synchronize the accesses from one side to the
+ other. The MU accomplishes synchronization using two sets of matching
+ registers (Processor A-facing, Processor B-facing).
+
+properties:
+ compatible:
+ oneOf:
+ - const: fsl,imx6sx-mu
+ - const: fsl,imx7ulp-mu
+ - const: fsl,imx8-mu-scu
+ - items:
+ - enum:
+ - fsl,imx7s-mu
+ - fsl,imx8mq-mu
+ - fsl,imx8mm-mu
+ - fsl,imx8mn-mu
+ - fsl,imx8mp-mu
+ - fsl,imx8qxp-mu
+ - const: fsl,imx6sx-mu
+ - description: To communicate with i.MX8 SCU with fast IPC
+ items:
+ - const: fsl,imx8qxp-mu
+ - const: fsl,imx8-mu-scu
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ "#mbox-cells":
+ description: |
+ <&phandle type channel>
+ phandle : Label name of controller
+ type : Channel type
+ channel : Channel number
+
+ This MU support 4 type of unidirectional channels, each type
+ has 4 channels. A total of 16 channels. Following types are
+ supported:
+ 0 - TX channel with 32bit transmit register and IRQ transmit
+ acknowledgment support.
+ 1 - RX channel with 32bit receive register and IRQ support
+ 2 - TX doorbell channel. Without own register and no ACK support.
+ 3 - RX doorbell channel.
+ const: 2
+
+ clocks:
+ maxItems: 1
+
+ fsl,mu-side-b:
+ description: boolean, if present, means it is for side B MU.
+ type: boolean
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - "#mbox-cells"
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+ mailbox@5d1b0000 {
+ compatible = "fsl,imx8qxp-mu", "fsl,imx6sx-mu";
+ reg = <0x0 0x5d1b0000 0x0 0x10000>;
+ interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+ #mbox-cells = <2>;
+ };
--
2.7.4
^ permalink raw reply related
* Re: [PATCHv1 00/19] Improve SBS battery support
From: Marek Szyprowski @ 2020-06-01 10:40 UTC (permalink / raw)
To: Sebastian Reichel, Sebastian Reichel, Rob Herring,
Greg Kroah-Hartman, Rafael J . Wysocki
Cc: linux-pm, devicetree, linux-kernel, kernel,
'Linux Samsung SOC'
In-Reply-To: <20200513185615.508236-1-sebastian.reichel@collabora.com>
Hi Sebastian,
On 13.05.2020 20:55, Sebastian Reichel wrote:
> This patchset improves support for SBS compliant batteries. Due to
> the changes, the battery now exposes 32 power supply properties and
> (un)plugging it generates a backtrace containing the following message
> without the first patch in this series:
>
> ---------------------------
> WARNING: CPU: 0 PID: 20 at lib/kobject_uevent.c:659 add_uevent_var+0xd4/0x104
> add_uevent_var: too many keys
> ---------------------------
>
> For references this is what an SBS battery status looks like after
> the patch series has been applied:
>
> cat /sys/class/power_supply/sbs-0-000b/uevent
> POWER_SUPPLY_NAME=sbs-0-000b
> POWER_SUPPLY_TYPE=Battery
> POWER_SUPPLY_STATUS=Discharging
> POWER_SUPPLY_CAPACITY_LEVEL=Normal
> POWER_SUPPLY_HEALTH=Good
> POWER_SUPPLY_PRESENT=1
> POWER_SUPPLY_TECHNOLOGY=Li-ion
> POWER_SUPPLY_CYCLE_COUNT=12
> POWER_SUPPLY_VOLTAGE_NOW=11441000
> POWER_SUPPLY_CURRENT_NOW=-26000
> POWER_SUPPLY_CURRENT_AVG=-24000
> POWER_SUPPLY_CAPACITY=76
> POWER_SUPPLY_CAPACITY_ERROR_MARGIN=1
> POWER_SUPPLY_TEMP=198
> POWER_SUPPLY_TIME_TO_EMPTY_AVG=438600
> POWER_SUPPLY_TIME_TO_FULL_AVG=3932100
> POWER_SUPPLY_SERIAL_NUMBER=0000
> POWER_SUPPLY_VOLTAGE_MIN_DESIGN=10800000
> POWER_SUPPLY_VOLTAGE_MAX_DESIGN=10800000
> POWER_SUPPLY_ENERGY_NOW=31090000
> POWER_SUPPLY_ENERGY_FULL=42450000
> POWER_SUPPLY_ENERGY_FULL_DESIGN=41040000
> POWER_SUPPLY_CHARGE_NOW=2924000
> POWER_SUPPLY_CHARGE_FULL=3898000
> POWER_SUPPLY_CHARGE_FULL_DESIGN=3800000
> POWER_SUPPLY_CONSTANT_CHARGE_CURRENT_MAX=3000000
> POWER_SUPPLY_CONSTANT_CHARGE_VOLTAGE_MAX=12300000
> POWER_SUPPLY_MANUFACTURE_YEAR=2017
> POWER_SUPPLY_MANUFACTURE_MONTH=7
> POWER_SUPPLY_MANUFACTURE_DAY=3
> POWER_SUPPLY_MANUFACTURER=UR18650A
> POWER_SUPPLY_MODEL_NAME=GEHC
This patch landed in linux-next dated 20200529. Sadly it causes a
regression on Samsung Exynos-based Chromebooks (Exynos5250 Snow,
Exynos5420 Peach-Pi and Exynos5800 Peach-Pit). System boots to
userspace, but then, when udev populates /dev, booting hangs:
[ 4.435167] VFS: Mounted root (ext4 filesystem) readonly on device
179:51.
[ 4.457477] devtmpfs: mounted
[ 4.460235] Freeing unused kernel memory: 1024K
[ 4.464022] Run /sbin/init as init process
INIT: version 2.88 booting
[info] Using makefile-style concurrent boot in runlevel S.
[ 5.102096] random: crng init done
[....] Starting the hotplug events dispatcher: systemd-udevdstarting
version 236
[ ok .
[....] Synthesizing the initial hotplug events...[ ok done.
[....] Waiting for /dev to be fully populated...[ 34.409914]
TPS65090_RAILSDCDC1: disabling
[ 34.412977] TPS65090_RAILSDCDC2: disabling
[ 34.417021] TPS65090_RAILSDCDC3: disabling
[ 34.423848] TPS65090_RAILSLDO1: disabling
[ 34.429068] TPS65090_RAILSLDO2: disabling
Bisect between v5.7-rc1 and next-20200529 pointed me to the first bad
commit: [c4b12a2f3f3de670f6be5e96092a2cab0b877f1a] power: supply:
sbs-battery: simplify read_read_string_data. However reverting it in
linux-next doesn't fix the issue, so the next commits are also relevant
to this issue.
Let me know how can I help debugging it.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
* Re: [RFC PATCH V4 4/4] platform: mtk-isp: Add Mediatek FD driver
From: Jerry-ch Chen @ 2020-06-01 10:37 UTC (permalink / raw)
To: Tomasz Figa
Cc: Hans Verkuil, Hans Verkuil, Laurent Pinchart, Matthias Brugger,
Mauro Carvalho Chehab, Pi-Hsun Shih, yuzhao, zwisler,
moderated list:ARM/Mediatek SoC support,
list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>,,
Sean Cheng (鄭昇弘), Sj Huang,
Christie Yu (游雅惠),
Frederic Chen (陳俊元),
Jungo Lin (林明俊), Linux Media Mailing List,
srv_heupstream, linux-devicetree
In-Reply-To: <CAAFQd5BBfapVv_3cwGte=p=6G8QXZQP=-ciZ8NBZZeSBGrHmCA@mail.gmail.com>
On Fri, 2020-05-29 at 14:59 +0200, Tomasz Figa wrote:
> On Fri, May 29, 2020 at 2:26 PM Jerry-ch Chen
> <Jerry-ch.Chen@mediatek.com> wrote:
> >
> > Hi Tomasz,
> >
> > I Appreciate your review comments, here's the reply.
> >
> > On Mon, 2020-05-25 at 14:24 +0200, Tomasz Figa wrote:
> > > r
> > >
> > > On Fri, May 22, 2020 at 4:11 PM Jerry-ch Chen
> > > <Jerry-ch.Chen@mediatek.com> wrote:
> > > >
> > > > Hi Tomasz,
> > > >
> > > > On Thu, 2020-05-21 at 18:28 +0000, Tomasz Figa wrote:
> > > > > Hi Jerry,
> > > > >
> > > > > On Wed, Dec 04, 2019 at 08:47:32PM +0800, Jerry-ch Chen wrote:
> [snip]
> > > Isn't still a need to clamp() width and height to min/max, though?
> > Yes, I'll add them back.
> >
> > This function will be refined as :
> >
> > static void mtk_fd_fill_pixfmt_mp(struct v4l2_pix_format_mplane *dfmt,
> > u32 pixfmt)
> > {
> > v4l2_fill_pixfmt_mp(dfmt, pixfmt, dfmt->width, dfmt->height);
> >
> > dfmt->field = V4L2_FIELD_NONE;
> > dfmt->colorspace = V4L2_COLORSPACE_BT2020;
> > dfmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > dfmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> > dfmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(dfmt->colorspace);
> >
> > /* Keep user setting as possible */
> > dfmt->width = clamp(dfmt->width,
> > MTK_FD_OUTPUT_MIN_WIDTH,
> > MTK_FD_OUTPUT_MAX_WIDTH);
> > dfmt->height = clamp(dfmt->height,
> > MTK_FD_OUTPUT_MIN_HEIGHT,
> > MTK_FD_OUTPUT_MAX_HEIGHT);
>
> Note that this would cause the other fields of dfmt to be inconsistent
> with width and height. The correct way to do this would be to first
> clamp and then call v4l2_fill_pixfmt_mp().
>
Ok, I will fix it.
Thanks and Best regards,
Jerry
> Best regards,
> Tomasz
^ permalink raw reply
* Re: [PATCH v2 1/4] iio: chemical: scd30: add core driver
From: Jonathan Cameron @ 2020-06-01 10:36 UTC (permalink / raw)
To: Tomasz Duszynski
Cc: Jonathan Cameron, linux-iio, linux-kernel, devicetree, robh+dt,
andy.shevchenko, pmeerw
In-Reply-To: <20200531192152.GC27246@arch>
On Sun, 31 May 2020 21:21:52 +0200
Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:
> On Sun, May 31, 2020 at 10:58:40AM +0100, Jonathan Cameron wrote:
> > On Sat, 30 May 2020 23:36:27 +0200
> > Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:
> >
> > > Add Sensirion SCD30 carbon dioxide core driver.
> > >
> > > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> >
> > Hi Tomasz
> >
> > A few things inline. Includes the alignment issue on
> > x86_32 that I fell into whilst trying to fix timestamp
> > alignment issues. Suggested resolution inline for that.
> >
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > > Documentation/ABI/testing/sysfs-bus-iio-scd30 | 20 +
> > > MAINTAINERS | 6 +
> > > drivers/iio/chemical/Kconfig | 11 +
> > > drivers/iio/chemical/Makefile | 1 +
> > > drivers/iio/chemical/scd30.h | 75 ++
> > > drivers/iio/chemical/scd30_core.c | 764 ++++++++++++++++++
> > > 6 files changed, 877 insertions(+)
> > > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > create mode 100644 drivers/iio/chemical/scd30.h
> > > create mode 100644 drivers/iio/chemical/scd30_core.c
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-scd30 b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > new file mode 100644
> > > index 000000000000..a05b1d28e94a
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > @@ -0,0 +1,20 @@
> > > +What: /sys/bus/iio/devices/iio:deviceX/calibration
> > > +Date: June 2020
> > > +KernelVersion: 5.8
> > > +Contact: linux-iio@vger.kernel.org
> > > +Description:
> > > + Contaminants build-up in the measurement chamber or optical
> > > + elements deterioration leads to sensor drift.
> > > +
> > > + One can compensate for sensor drift by using either automatic
> > > + self calibration (asc) or forced recalibration (frc). If used
> > > + at once one will overwrite the other.
> > > +
> > > + Writing 1 or 0 to this attribute will respectively activate or
> > > + deactivate asc.
> > > +
> > > + Picking value from the range [400 1 2000] and writing it to the
> > > + sensor will set frc.
> > Seems to me like this would be more intuitive as two separate parameters
> > perhaps:
> > calibration_auto_enable
> > calibration_forced_value
> > ?
> >
>
> Fine.
>
> > > +
> > > + Upon reading current asc status and frc value are returned
> > > + respectively.
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 60ed2963efaa..41a509cca6f1 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -15137,6 +15137,12 @@ S: Maintained
> > > F: drivers/misc/phantom.c
> > > F: include/uapi/linux/phantom.h
> > >
> > > +SENSIRION SCD30 CARBON DIOXIDE SENSOR DRIVER
> > > +M: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > > +S: Maintained
> > > +F: drivers/iio/chemical/scd30.h
> > > +F: drivers/iio/chemical/scd30_core.c
> > > +
> > > SENSIRION SPS30 AIR POLLUTION SENSOR DRIVER
> > > M: Tomasz Duszynski <tduszyns@gmail.com>
> > > S: Maintained
> > > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> > > index 7f21afd73b1c..99e852b67e55 100644
> > > --- a/drivers/iio/chemical/Kconfig
> > > +++ b/drivers/iio/chemical/Kconfig
> > > @@ -85,6 +85,17 @@ config PMS7003
> > > To compile this driver as a module, choose M here: the module will
> > > be called pms7003.
> > >
> > > +config SCD30_CORE
> > > + tristate "SCD30 carbon dioxide sensor driver"
> > > + select IIO_BUFFER
> > > + select IIO_TRIGGERED_BUFFER
> > > + help
> > > + Say Y here to build support for the Sensirion SCD30 sensor with carbon
> > > + dioxide, relative humidity and temperature sensing capabilities.
> > > +
> > > + To compile this driver as a module, choose M here: the module will
> > > + be called scd30_core.
> > > +
> > > config SENSIRION_SGP30
> > > tristate "Sensirion SGPxx gas sensors"
> > > depends on I2C
> > > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> > > index aba4167db745..c9804b041ecd 100644
> > > --- a/drivers/iio/chemical/Makefile
> > > +++ b/drivers/iio/chemical/Makefile
> > > @@ -12,6 +12,7 @@ obj-$(CONFIG_BME680_SPI) += bme680_spi.o
> > > obj-$(CONFIG_CCS811) += ccs811.o
> > > obj-$(CONFIG_IAQCORE) += ams-iaq-core.o
> > > obj-$(CONFIG_PMS7003) += pms7003.o
> > > +obj-$(CONFIG_SCD30_CORE) += scd30_core.o
> > > obj-$(CONFIG_SENSIRION_SGP30) += sgp30.o
> > > obj-$(CONFIG_SPS30) += sps30.o
> > > obj-$(CONFIG_VZ89X) += vz89x.o
> > > diff --git a/drivers/iio/chemical/scd30.h b/drivers/iio/chemical/scd30.h
> > > new file mode 100644
> > > index 000000000000..9b25f7423142
> > > --- /dev/null
> > > +++ b/drivers/iio/chemical/scd30.h
> > > @@ -0,0 +1,75 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef _SCD30_H
> > > +#define _SCD30_H
> > > +
> > > +#include <linux/completion.h>
> > > +#include <linux/device.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/pm.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/types.h>
> > > +
> > > +struct scd30_state;
> > > +
> > > +enum scd30_cmd {
> > > + /* start continuous measurement with pressure compensation */
> > > + CMD_START_MEAS,
> > > + /* stop continuous measurement */
> > > + CMD_STOP_MEAS,
> > > + /* set/get measurement interval */
> > > + CMD_MEAS_INTERVAL,
> > > + /* check whether new measurement is ready */
> > > + CMD_MEAS_READY,
> > > + /* get measurement */
> > > + CMD_READ_MEAS,
> > > + /* turn on/off automatic self calibration */
> > > + CMD_ASC,
> > > + /* set/get forced recalibration value */
> > > + CMD_FRC,
> > > + /* set/get temperature offset */
> > > + CMD_TEMP_OFFSET,
> > > + /* get firmware version */
> > > + CMD_FW_VERSION,
> > > + /* reset sensor */
> > > + CMD_RESET,
> > > + /*
> > > + * Command for altitude compensation was omitted intentionally because
> > > + * the same can be achieved by means of CMD_START_MEAS which takes
> > > + * pressure above the sea level as an argument.
> > > + */
> > > +};
> > > +
> > > +#define SCD30_MEAS_COUNT 3
> > > +
> > > +typedef int (*scd30_command_t)(struct scd30_state *state, enum scd30_cmd cmd,
> > > + u16 arg, void *response, int size);
> > > +
> > > +struct scd30_state {
> > > + /* serialize access to the device */
> > > + struct mutex lock;
> > > + struct device *dev;
> > > + struct regulator *vdd;
> > > + struct completion meas_ready;
> > > + void *priv;
> > > + int irq;
> > > + /*
> > > + * no way to retrieve current ambient pressure compensation value from
> > > + * the sensor so keep one around
> > > + */
> > > + u16 pressure_comp;
> > > + u16 meas_interval;
> > > + int meas[SCD30_MEAS_COUNT];
> > > +
> > > + scd30_command_t command;
> > > +};
> > > +
> > > +int scd30_suspend(struct device *dev);
> > > +int scd30_resume(struct device *dev);
> > > +
> > > +static __maybe_unused SIMPLE_DEV_PM_OPS(scd30_pm_ops, scd30_suspend,
> > > + scd30_resume);
> > > +
> > > +int scd30_probe(struct device *dev, int irq, const char *name, void *priv,
> > > + scd30_command_t command);
> > > +
> > > +#endif
> > > diff --git a/drivers/iio/chemical/scd30_core.c b/drivers/iio/chemical/scd30_core.c
> > > new file mode 100644
> > > index 000000000000..3b7d0a7ea7ae
> > > --- /dev/null
> > > +++ b/drivers/iio/chemical/scd30_core.c
> > > @@ -0,0 +1,764 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Sensirion SCD30 carbon dioxide sensor core driver
> > > + *
> > > + * Copyright (c) 2020 Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > > + */
> > > +#include <linux/bits.h>
> > > +#include <linux/completion.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/errno.h>
> > > +#include <linux/export.h>
> > > +#include <linux/iio/buffer.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>
> > > +#include <linux/iio/trigger.h>
> > > +#include <linux/iio/trigger_consumer.h>
> > > +#include <linux/iio/triggered_buffer.h>
> > > +#include <linux/iio/types.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/irqreturn.h>
> > > +#include <linux/jiffies.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/string.h>
> > > +#include <linux/sysfs.h>
> > > +#include <linux/types.h>
> > > +#include <asm/byteorder.h>
> > > +
> > > +#include "scd30.h"
> > > +
> > > +#define SCD30_PRESSURE_COMP_MIN_MBAR 700
> > > +#define SCD30_PRESSURE_COMP_MAX_MBAR 1400
> > > +#define SCD30_PRESSURE_COMP_DEFAULT 1013
> > > +#define SCD30_MEAS_INTERVAL_MIN_S 2
> > > +#define SCD30_MEAS_INTERVAL_MAX_S 1800
> > > +#define SCD30_MEAS_INTERVAL_DEFAULT SCD30_MEAS_INTERVAL_MIN_S
> > > +#define SCD30_FRC_MIN_PPM 400
> > > +#define SCD30_FRC_MAX_PPM 2000
> > > +#define SCD30_TEMP_OFFSET_MAX 655360
> > > +#define SCD30_EXTRA_TIMEOUT_PER_S 250
> > > +
> > > +enum {
> > > + SCD30_CONC,
> > > + SCD30_TEMP,
> > > + SCD30_HR,
> > > +};
> > > +
> > > +static int scd30_command_write(struct scd30_state *state, enum scd30_cmd cmd,
> > > + u16 arg)
> > > +{
> > > + return state->command(state, cmd, arg, NULL, 0);
> > > +}
> > > +
> > > +static int scd30_command_read(struct scd30_state *state, enum scd30_cmd cmd,
> > > + u16 *val)
> > > +{
> > > + int ret;
> > > +
> > > + ret = state->command(state, cmd, 0, val, sizeof(*val));
> > > + *val = be16_to_cpup((__be16 *)val);
> >
> > Please use a local variable for the __be16 as it makes thing more readable
> > and easier to check for endian problems.
> >
>
> Okay.
>
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int scd30_reset(struct scd30_state *state)
> > > +{
> > > + int ret;
> > > + u16 val;
> > > +
> > > + ret = scd30_command_write(state, CMD_RESET, 0);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /* sensor boots up within 2 secs */
> > > + msleep(2000);
> > > + /*
> > > + * Power-on-reset causes sensor to produce some glitch on i2c bus and
> > > + * some controllers end up in error state. Try to recover by placing
> > > + * any data on the bus.
> > > + */
> > > + scd30_command_read(state, CMD_MEAS_READY, &val);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/* simplified float to fixed point conversion with a scaling factor of 0.01 */
> > > +static int scd30_float_to_fp(int float32)
> > > +{
> > > + int fraction, shift,
> > > + mantissa = float32 & GENMASK(22, 0),
> > > + sign = float32 & BIT(31) ? -1 : 1,
> > > + exp = (float32 & ~BIT(31)) >> 23;
> > > +
> > > + /* special case 0 */
> > > + if (!exp && !mantissa)
> > > + return 0;
> > > +
> > > + exp -= 127;
> > > + if (exp < 0) {
> > > + exp = -exp;
> > > + /* return values ranging from 1 to 99 */
> > > + return sign * ((((BIT(23) + mantissa) * 100) >> 23) >> exp);
> > > + }
> > > +
> > > + /* return values starting at 100 */
> > > + shift = 23 - exp;
> > > + float32 = BIT(exp) + (mantissa >> shift);
> > > + fraction = mantissa & GENMASK(shift - 1, 0);
> > > +
> > > + return sign * (float32 * 100 + ((fraction * 100) >> shift));
> > > +}
> > > +
> > > +static int scd30_read_meas(struct scd30_state *state)
> > > +{
> > > + int i, ret;
> > > +
> > > + ret = state->command(state, CMD_READ_MEAS, 0, state->meas,
> > > + sizeof(state->meas));
> > > + if (ret)
> > > + return ret;
> > > +
> > > + be32_to_cpu_array(state->meas, state->meas, ARRAY_SIZE(state->meas));
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(state->meas); i++)
> > > + state->meas[i] = scd30_float_to_fp(state->meas[i]);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int scd30_wait_meas_irq(struct scd30_state *state)
> > > +{
> > > + int ret, timeout;
> > > +
> > > + timeout = state->meas_interval * (1000 + SCD30_EXTRA_TIMEOUT_PER_S);
> > > + timeout = msecs_to_jiffies(timeout);
> > > + reinit_completion(&state->meas_ready);
> > > + enable_irq(state->irq);
> > > + ret = wait_for_completion_interruptible_timeout(&state->meas_ready,
> > > + timeout);
> > > + if (ret > 0)
> > > + ret = 0;
> > > + else if (!ret)
> > > + ret = -ETIMEDOUT;
> > > +
> > > + disable_irq(state->irq);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int scd30_wait_meas_poll(struct scd30_state *state)
> > > +{
> > > + int timeout = state->meas_interval * SCD30_EXTRA_TIMEOUT_PER_S;
> > > + int tries = 5;
> > > +
> > > + do {
> > > + int ret;
> > > + u16 val;
> > > +
> > > + ret = scd30_command_read(state, CMD_MEAS_READY, &val);
> > > + if (ret)
> > > + return -EIO;
> > > +
> > > + /* new measurement available */
> > > + if (val)
> > > + break;
> > > +
> > > + msleep_interruptible(timeout);
> > > + } while (--tries);
> > > +
> > > + return tries ? 0 : -ETIMEDOUT;
> > > +}
> > > +
> > > +static int scd30_read_poll(struct scd30_state *state)
> > > +{
> > > + int ret;
> > > +
> > > + ret = scd30_wait_meas_poll(state);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return scd30_read_meas(state);
> > > +}
> > > +
> > > +static int scd30_read(struct scd30_state *state)
> > > +{
> > > + if (state->irq > 0)
> > > + return scd30_wait_meas_irq(state);
> > > +
> > > + return scd30_read_poll(state);
> > > +}
> > > +
> > > +static int scd30_read_raw(struct iio_dev *indio_dev,
> > > + struct iio_chan_spec const *chan,
> > > + int *val, int *val2, long mask)
> > > +{
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + int ret, meas[SCD30_MEAS_COUNT];
> > > +
> > > + switch (mask) {
> > > + case IIO_CHAN_INFO_RAW:
> > > + case IIO_CHAN_INFO_PROCESSED:
> > > + ret = iio_device_claim_direct_mode(indio_dev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + mutex_lock(&state->lock);
> > > + ret = scd30_read(state);
> > > + memcpy(meas, state->meas, SCD30_MEAS_COUNT * sizeof(*meas));
> >
> > The local copy seems a bit excessive. This isn't likely to be a particularly
> > fast path so perhaps skip the copy but hold the locks until we are
> > done with the buffer?
> >
>
> Okay.
>
> > > + mutex_unlock(&state->lock);
> > > + iio_device_release_direct_mode(indio_dev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + switch (chan->type) {
> > > + case IIO_CONCENTRATION:
> > > + *val = meas[chan->address] / 1000000;
> > > + *val2 = meas[chan->address] % 1000000;
> > > +
> > > + return IIO_VAL_INT_PLUS_MICRO;
> > > + case IIO_TEMP:
> > > + case IIO_HUMIDITYRELATIVE:
> > > + *val = meas[chan->address] * 10;
> > > +
> > > + return IIO_VAL_INT;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > + case IIO_CHAN_INFO_SCALE:
> > > + switch (chan->type) {
> > > + case IIO_CONCENTRATION:
> > > + *val = 0;
> > > + *val2 = 1;
> > > +
> > > + return IIO_VAL_INT_PLUS_MICRO;
> > > + case IIO_TEMP:
> > > + case IIO_HUMIDITYRELATIVE:
> > > + *val = 10;
> > > +
> > > + return IIO_VAL_INT;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > + case IIO_CHAN_INFO_SAMP_FREQ:
> > > + *val = 0;
> > > + *val2 = 0;
> > > +
> > > + mutex_lock(&state->lock);
> > > + ret = scd30_command_read(state, CMD_MEAS_INTERVAL, (u16 *)val2);
> >
> > See below. I'll assume you'll fix all of these.
> >
> > > + mutex_unlock(&state->lock);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + *val2 = 1000000000 / *val2;
> > > +
> > > + return IIO_VAL_INT_PLUS_NANO;
> > > + case IIO_CHAN_INFO_CALIBSCALE:
> > > + mutex_lock(&state->lock);
> > > + *val = state->pressure_comp / 10;
> > > + *val2 = (state->pressure_comp % 10) * 100000;
> > > + mutex_unlock(&state->lock);
> > > +
> > > + return IIO_VAL_INT_PLUS_MICRO;
> > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > + *val = 0;
> > > + mutex_lock(&state->lock);
> > > + ret = scd30_command_read(state, CMD_TEMP_OFFSET, (u16 *)val);
> >
> > Reading a u16 directly into a int is not a good idea. What you get will
> > depend on the endianness of the machine.
> >
>
> Right, that would obviously break on BE. Sometimes trying to keep number
> of local variables low simply stops paying off :).
>
> > Use an intermediate variable of the right size.
> >
> > > + mutex_unlock(&state->lock);
> > > +
> > > + return IIO_VAL_INT;
> > > + }
> > > +
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static int scd30_write_raw(struct iio_dev *indio_dev,
> > > + struct iio_chan_spec const *chan,
> > > + int val, int val2, long mask)
> > > +{
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + int ret = -EINVAL;
> > > +
> > > + mutex_lock(&state->lock);
> > > + switch (mask) {
> > > + case IIO_CHAN_INFO_SAMP_FREQ:
> > > + if (val)
> > > + break;
> > > +
> > > + val = 1000000000 / val2;
> > > + if (val < SCD30_MEAS_INTERVAL_MIN_S ||
> > > + val > SCD30_MEAS_INTERVAL_MAX_S)
> > > + break;
> > > +
> > > + ret = scd30_command_write(state, CMD_MEAS_INTERVAL, val);
> > > + if (ret)
> > > + break;
> > > +
> > > + state->meas_interval = val;
> > > + break;
> > > + case IIO_CHAN_INFO_CALIBSCALE:
> > > + val = (val * 1000000 + val2) / 100000;
> > > + if (val < SCD30_PRESSURE_COMP_MIN_MBAR ||
> > > + val > SCD30_PRESSURE_COMP_MAX_MBAR)
> > > + break;
> > > +
> > > + ret = scd30_command_write(state, CMD_START_MEAS, val);
> > > + if (ret)
> > > + break;
> > > +
> > > + state->pressure_comp = val;
> > > + break;
> > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > + if (val < 0 || val > SCD30_TEMP_OFFSET_MAX)
> > > + break;
> > > + /*
> > > + * Manufacturer does not explicitly specify min/max sensible
> > > + * values hence check is omitted for simplicity.
> > > + */
> > > + ret = scd30_command_write(state, CMD_TEMP_OFFSET / 10, val);
> > > + }
> > > + mutex_unlock(&state->lock);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int scd30_write_raw_get_fmt(struct iio_dev *indio_dev,
> > > + struct iio_chan_spec const *chan, long mask)
> > > +{
> > > + switch (mask) {
> > > + case IIO_CHAN_INFO_RAW:
> > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > + return IIO_VAL_INT;
> > > + case IIO_CHAN_INFO_CALIBSCALE:
> > > + return IIO_VAL_INT_PLUS_MICRO;
> > > + case IIO_CHAN_INFO_SAMP_FREQ:
> > > + return IIO_VAL_INT_PLUS_NANO;
> > > + }
> > > +
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static const int scd30_pressure_calibscale_available[] = {
> > > + SCD30_PRESSURE_COMP_MIN_MBAR / 10, 0,
> > > + 0, 100000,
> > > + SCD30_PRESSURE_COMP_MAX_MBAR / 10, 0,
> > > +};
> > > +
> > > +static const int scd30_temp_calibbias_available[] = {
> > > + 0, 10, SCD30_TEMP_OFFSET_MAX,
> > > +};
> > > +
> > > +static int scd30_read_avail(struct iio_dev *indio_dev,
> > > + struct iio_chan_spec const *chan, const int **vals,
> > > + int *type, int *length, long mask)
> > > +{
> > > + switch (mask) {
> > > + case IIO_CHAN_INFO_CALIBSCALE:
> > > + *vals = scd30_pressure_calibscale_available;
> > > + *type = IIO_VAL_INT_PLUS_MICRO;
> > > +
> > > + return IIO_AVAIL_RANGE;
> > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > + *vals = scd30_temp_calibbias_available;
> > > + *type = IIO_VAL_INT;
> > > +
> > > + return IIO_AVAIL_RANGE;
> > > + }
> > > +
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static ssize_t sampling_frequency_available_show(struct device *dev,
> > > + struct device_attribute *attr,
> > > + char *buf)
> > > +{
> > > + int i = SCD30_MEAS_INTERVAL_MIN_S;
> > > + ssize_t len = 0;
> > > +
> > > + do {
> > > + len += scnprintf(buf + len, PAGE_SIZE - len, "0.%09u ",
> > > + 1000000000 / i);
> > > + /*
> > > + * Not all values fit PAGE_SIZE buffer hence print every 6th
> > > + * (each frequency differs by 6s in time domain from the
> > > + * adjecent). Unlisted but valid ones are still accepted.
> >
> > adjacent
> >
> > Hmm. Maybe we need to think about some description for inverse of linear
> > cases as they are likely to be fairly common.
> > This will work in meantime.
> >
> > > + */
> > > + i += 6;
> > > + } while (i <= SCD30_MEAS_INTERVAL_MAX_S);
> > > +
> > > + buf[len - 1] = '\n';
> > > +
> > > + return len;
> > > +}
> > > +
> > > +static ssize_t calibration_show(struct device *dev,
> > > + struct device_attribute *attr, char *buf)
> > > +{
> > > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + u16 asc, frc;
> > > + int ret;
> > > +
> > > + mutex_lock(&state->lock);
> > > + ret = scd30_command_read(state, CMD_ASC, &asc);
> > > + if (ret)
> > > + goto out;
> > > +
> > > + ret = scd30_command_read(state, CMD_FRC, &frc);
> > > +out:
> > > + mutex_unlock(&state->lock);
> > > +
> > > + return ret ?: sprintf(buf, "%d %d\n", asc, frc);
> > > +}
> > > +
> > > +static ssize_t calibration_store(struct device *dev,
> > > + struct device_attribute *attr, const char *buf,
> > > + size_t len)
> > > +{
> > > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + int ret;
> > > + u16 val;
> >
> > As commented above, this interface doesn't win on the
> > obvious front so needs a rethink!
> >
> > > +
> > > + ret = kstrtou16(buf, 0, &val);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + mutex_lock(&state->lock);
> > > + if (val == 0 || val == 1)
> > > + ret = scd30_command_write(state, CMD_ASC, val);
> > > + else if (val >= SCD30_FRC_MIN_PPM && val <= SCD30_FRC_MAX_PPM)
> > > + ret = scd30_command_write(state, CMD_FRC, val);
> > > + else
> > > + ret = -EINVAL;
> > > + mutex_unlock(&state->lock);
> > > +
> > > + return ret ?: len;
> > > +}
> > > +
> > > +static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
> > > +static IIO_DEVICE_ATTR_RW(calibration, 0);
> > > +
> > > +static struct attribute *scd30_attrs[] = {
> > > + &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> > > + &iio_dev_attr_calibration.dev_attr.attr,
> > > + NULL
> > > +};
> > > +
> > > +static const struct attribute_group scd30_attr_group = {
> > > + .attrs = scd30_attrs,
> > > +};
> > > +
> > > +static const struct iio_info scd30_info = {
> > > + .attrs = &scd30_attr_group,
> > > + .read_raw = scd30_read_raw,
> > > + .write_raw = scd30_write_raw,
> > > + .write_raw_get_fmt = scd30_write_raw_get_fmt,
> > > + .read_avail = scd30_read_avail,
> > > +};
> > > +
> > > +#define SCD30_CHAN_SCAN_TYPE(_sign, _realbits) .scan_type = { \
> > > + .sign = _sign, \
> > > + .realbits = _realbits, \
> > > + .storagebits = 32, \
> > > + .endianness = IIO_CPU, \
> > > +}
> > > +
> > > +static const struct iio_chan_spec scd30_channels[] = {
> > > + {
> > > + .type = IIO_PRESSURE,
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE),
> > > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_CALIBSCALE),
> > > + .scan_index = -1,
> > > + },
> > > + {
> > > + .type = IIO_CONCENTRATION,
> > > + .channel2 = IIO_MOD_CO2,
> > > + .address = SCD30_CONC,
> > > + .scan_index = SCD30_CONC,
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > > + BIT(IIO_CHAN_INFO_SCALE),
> > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > > + .modified = 1,
> > > +
> > > + SCD30_CHAN_SCAN_TYPE('u', 16),
> > > + },
> > > + {
> > > + .type = IIO_TEMP,
> > > + .address = SCD30_TEMP,
> > > + .scan_index = SCD30_TEMP,
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > > + BIT(IIO_CHAN_INFO_CALIBBIAS) |
> > > + BIT(IIO_CHAN_INFO_SCALE),
> >
> > Combination of processed and scale is unusual. Normally scale provides
> > a conversion factor or a _RAW reading.
>
> Right that's pointless. Scales were for raw measurements inside buffer.
> Somehow I failed to realize that only co2 concentration is raw.
>
> >
> > I 'think' these units are otherwise fine (milli degrees centigrade)
> >
> >
> > > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_CALIBBIAS),
> > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > > +
> > > + SCD30_CHAN_SCAN_TYPE('s', 14),
> > > + },
> > > + {
> > > + .type = IIO_HUMIDITYRELATIVE,
> > > + .address = SCD30_HR,
> > > + .scan_index = SCD30_HR,
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > > + BIT(IIO_CHAN_INFO_SCALE),
> >
> > As above. Not normal to see scale and processed.
> >
> > > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > > +
> > > + SCD30_CHAN_SCAN_TYPE('u', 14),
> > > + },
> > > + IIO_CHAN_SOFT_TIMESTAMP(3),
> > > +};
> > > +
> > > +int __maybe_unused scd30_suspend(struct device *dev)
> > > +{
> > > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + int ret;
> > > +
> > > + ret = scd30_command_write(state, CMD_STOP_MEAS, 0);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return regulator_disable(state->vdd);
> > > +}
> > > +EXPORT_SYMBOL(scd30_suspend);
> > > +
> > > +int __maybe_unused scd30_resume(struct device *dev)
> > > +{
> > > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + int ret;
> > > +
> > > + ret = regulator_enable(state->vdd);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return scd30_command_write(state, CMD_START_MEAS, state->pressure_comp);
> > > +}
> > > +EXPORT_SYMBOL(scd30_resume);
> > > +
> > > +static void scd30_stop_meas(void *data)
> > > +{
> > > + struct scd30_state *state = data;
> > > +
> > > + scd30_command_write(state, CMD_STOP_MEAS, 0);
> > > +}
> > > +
> > > +static void scd30_disable_regulator(void *data)
> > > +{
> > > + struct scd30_state *state = data;
> > > +
> > > + regulator_disable(state->vdd);
> > > +}
> > > +
> > > +static irqreturn_t scd30_irq_handler(int irq, void *priv)
> > > +{
> > > + struct iio_dev *indio_dev = priv;
> > > +
> > > + if (iio_buffer_enabled(indio_dev)) {
> >
> > There is a potential quirk here. It's possible that
> > this device is using a different trigger, but another device
> > is registered to use this one. If that happens this check
> > will be a bit counter intuitive.
> >
> > As such you might want to provide the validate callback so
> > that this device is the only device allowed to use it's
> > own trigger.
> >
>
> Right. This needs to be fixed.
>
> > > + iio_trigger_poll(indio_dev->trig);
> > > +
> > > + return IRQ_HANDLED;
> > > + }
> > > +
> > > + return IRQ_WAKE_THREAD;
> > > +}
> > > +
> > > +static irqreturn_t scd30_irq_thread_handler(int irq, void *priv)
> > > +{
> > > + struct iio_dev *indio_dev = priv;
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + int ret;
> > > +
> > > + ret = scd30_read_meas(state);
> > > + if (ret)
> > > + goto out;
> > > +
> > > + complete_all(&state->meas_ready);
> > > +out:
> > > + return IRQ_HANDLED;
> > > +}
> > > +
> > > +static irqreturn_t scd30_trigger_handler(int irq, void *p)
> > > +{
> > > + struct iio_poll_func *pf = p;
> > > + struct iio_dev *indio_dev = pf->indio_dev;
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + struct {
> > > + int data[SCD30_MEAS_COUNT];
> > > + u64 ts;
> >
> > Turns out I was wrong when suggesting this approach for drivers. On x86_32
> > this will result in there not being any padding between the
> > data and the timestamp (and in IIO rule of naturally aligned there
> > needs to be 4 bytes there). Result is that this structure is
> > too short. (thanks btw to Andy who pointed out this issue!)
> >
> > So, to force that my current preference is.
> >
> > struct {
> > int data[SCD30_MEAS_COUNT];
> > s64 ts __aligned(8);
> > } scan;
> >
>
> Ah, so x86_32 aligns s64 to 4 bytes.
yup
>
> > However, given we do have a hole in the structure there is
> > a kernel data leak. So either you need to zero it here,
> > or move it into the iio_priv() structure. Doing that
> > will ensure it is zeroed at allocation.
> >
> > > + } scan;
> > > + int ret;
> > > +
> > > + mutex_lock(&state->lock);
> > > + if (!iio_trigger_using_own(indio_dev))
> > > + ret = scd30_read_poll(state);
> > > + else
> > > + ret = scd30_read_meas(state);
> > > + memcpy(scan.data, state->meas, sizeof(state->meas));
> > > + mutex_unlock(&state->lock);
> > > + if (ret)
> > > + goto out;
> > > +
> > > + iio_push_to_buffers_with_timestamp(indio_dev, &scan,
> > > + iio_get_time_ns(indio_dev));
> > > +out:
> > > + iio_trigger_notify_done(indio_dev->trig);
> > > + return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int scd30_set_trigger_state(struct iio_trigger *trig, bool state)
> > > +{
> > > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > > + struct scd30_state *st = iio_priv(indio_dev);
> > > +
> > > + if (state)
> > > + enable_irq(st->irq);
> > > + else
> > > + disable_irq(st->irq);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct iio_trigger_ops scd30_trigger_ops = {
> > > + .set_trigger_state = scd30_set_trigger_state,
> > > +};
> > > +
> > > +static int scd30_setup_trigger(struct iio_dev *indio_dev)
> > > +{
> > > + struct scd30_state *state = iio_priv(indio_dev);
> > > + struct device *dev = indio_dev->dev.parent;
> > > + struct iio_trigger *trig;
> > > + int ret;
> > > +
> > > + trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
> > > + indio_dev->id);
> > > + if (!trig) {
> > > + dev_err(dev, "failed to allocate trigger\n");
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + trig->dev.parent = dev;
> > > + trig->ops = &scd30_trigger_ops;
> > > + iio_trigger_set_drvdata(trig, indio_dev);
> > > +
> > > + ret = devm_iio_trigger_register(dev, trig);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + indio_dev->trig = iio_trigger_get(trig);
> > > +
> > > + ret = devm_request_threaded_irq(dev, state->irq, scd30_irq_handler,
> > > + scd30_irq_thread_handler,
> > > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > > + indio_dev->name, indio_dev);
> > > + if (ret)
> > > + dev_err(dev, "failed to request irq\n");
> > > +
> > > + disable_irq(state->irq);
> >
> > Given there is a gap between the request above and this disable, this
> > disable needs a comment explaining why it is here.
> >
> > I'm assuming it's an optimization?
> >
>
> Interrupt is enabled just before taking measurement to grab the fresh
> data and disabled afterwards. Not disabling it here would produce fat warning
> about unbalanced irqs.
>
> And that is because sensor takes measurements continuously. On demand
> mode, even though possible, doesn't work reliably. Sensor (at least the one
> sitting on my desk) needs way too much time to wakeup and grab measurement which
> makes the whole point of adjustable sampling frequency pointless :).
>
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +int scd30_probe(struct device *dev, int irq, const char *name, void *priv,
> > > + scd30_command_t command)
> > > +{
> > > + static const unsigned long scd30_scan_masks[] = { 0x07, 0x00 };
> > > + struct scd30_state *state;
> > > + struct iio_dev *indio_dev;
> > > + int ret;
> > > + u16 val;
> > > +
> > > + indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
> > > + if (!indio_dev)
> > > + return -ENOMEM;
> > > +
> > > + state = iio_priv(indio_dev);
> > > + state->dev = dev;
> >
> > Doesn't seem to be used.
> >
> > > + state->priv = priv;
> >
> > What's this for? At least at first glance I can't find it being used
> > anywhere.
> >
> > > + state->irq = irq;
> > > + state->pressure_comp = SCD30_PRESSURE_COMP_DEFAULT;
> > > + state->meas_interval = SCD30_MEAS_INTERVAL_DEFAULT;
> > > + state->command = command;
> > > + mutex_init(&state->lock);
> > > + init_completion(&state->meas_ready);
> > > +
> > > + dev_set_drvdata(dev, indio_dev);
> > > +
> > > + indio_dev->dev.parent = dev;
> >
> > Side note that there is a series moving this into the core under revision at
> > the moment. Hopefully I'll remember to fix this up when applying your patch
> > if that one has gone in ahead of it.
> >
> > > + indio_dev->info = &scd30_info;
> > > + indio_dev->name = name;
> > > + indio_dev->channels = scd30_channels;
> > > + indio_dev->num_channels = ARRAY_SIZE(scd30_channels);
> > > + indio_dev->modes = INDIO_DIRECT_MODE;
> > > + indio_dev->available_scan_masks = scd30_scan_masks;
> > > +
> > > + state->vdd = devm_regulator_get(dev, "vdd");
> > > + if (IS_ERR(state->vdd)) {
> > > + if (PTR_ERR(state->vdd) == -EPROBE_DEFER)
> > > + return -EPROBE_DEFER;
> > > +
> > > + dev_err(dev, "failed to get regulator\n");
> > > + return PTR_ERR(state->vdd);
> > > + }
> > > +
> > > + ret = regulator_enable(state->vdd);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = devm_add_action_or_reset(dev, scd30_disable_regulator, state);
> > > + if (ret)
> > > + return ret;
> > > +
> >
> > A comment here on why it makes sense to register this here. What
> > started mesurement? It seems that happens well below here so
> > we should really call this after that start all.
> >
>
> Sensor after being powered up starts in mode it was left in.
> Chances are it was continuous mode and we want to shut it down.
That's fine. The question is why 'here' as opposed to after the below where you
put it into continuous mode.
>
> > > + ret = devm_add_action_or_reset(dev, scd30_stop_meas, state);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = scd30_reset(state);
> > > + if (ret) {
> > > + dev_err(dev, "failed to reset device: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + if (state->irq > 0) {
> > > + ret = scd30_setup_trigger(indio_dev);
> > > + if (ret) {
> > > + dev_err(dev, "failed to setup trigger: %d\n", ret);
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > > + scd30_trigger_handler, NULL);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = scd30_command_read(state, CMD_FW_VERSION, &val);
> > > + if (ret) {
> > > + dev_err(dev, "failed to read firmware version: %d\n", ret);
> > > + return ret;
> > > + }
> > > + dev_info(dev, "firmware version: %d.%d\n", val >> 8, (char)val);
> > > +
> > > + ret = scd30_command_write(state, CMD_MEAS_INTERVAL,
> > > + state->meas_interval);
> > > + if (ret) {
> > > + dev_err(dev, "failed to set measurement interval: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + ret = scd30_command_write(state, CMD_START_MEAS, state->pressure_comp);
> > > + if (ret) {
> > > + dev_err(dev, "failed to start measurement: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + return devm_iio_device_register(dev, indio_dev);
> > > +}
> > > +EXPORT_SYMBOL(scd30_probe);
> > > +
> > > +MODULE_AUTHOR("Tomasz Duszynski <tomasz.duszynski@octakon.com>");
> > > +MODULE_DESCRIPTION("Sensirion SCD30 carbon dioxide sensor core driver");
> > > +MODULE_LICENSE("GPL v2");
> > > --
> > > 2.26.2
> > >
> >
^ permalink raw reply
* Re: [PATCH v3 04/20] arm64: dts: arm: vexpress: Move fixed devices out of bus node
From: André Przywara @ 2020-06-01 10:14 UTC (permalink / raw)
To: Guenter Roeck, Rob Herring, Sudeep Holla
Cc: Liviu Dudau, Lorenzo Pieralisi, Mark Rutland, devicetree,
linux-arm-kernel
In-Reply-To: <48afb8bb-a22a-54df-7751-55b7b84c3c88@arm.com>
On 28/05/2020 14:30, André Przywara wrote:
Hi,
> On 28/05/2020 03:48, Guenter Roeck wrote:
>
> Hi Guenter,
>
>> On Wed, May 13, 2020 at 11:30:00AM +0100, Andre Przywara wrote:
>>> The devicetree compiler complains when DT nodes without a reg property
>>> live inside a (simple) bus node:
>>> Warning (simple_bus_reg): Node /bus@8000000/motherboard-bus/refclk32khz
>>> missing or empty reg/ranges property
>>>
>>> Move the fixed clocks, the fixed regulator, the leds and the config bus
>>> subtree to the root node, since they do not depend on any busses.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>
>> This patch results in tracebacks when booting the vexpress-a15 machine
>> with vexpress-v2p-ca15-tc1 devicetree file in qemu. Reverting it as well
>> as the subsequent patches affecting the same file (to avoid revert
>> conflicts) fixes the problem.
>
> Many thanks for the heads up! I was able to reproduce it here. On the
> first glance it looks like the UART is probed before the clocks now,
> because the traversal of the changed DT leads to a different probe
> order. I will look into how to fix this.
Turned out to be a bit more complicated:
The arm,vexpress,config-bus driver walks up the device tree to find a
arm,vexpress,site property [1]. With this patch the first parent node
with that property it finds is now the root node, with the wrong site ID
(0xf instead of 0x0). So it queries the wrong clocks (those IDs are
actually reserved there), and QEMU reports back "0", consequently [2].
Finding a clock frequency in the range of [0, 0] won't get very far.
Possible solutions are:
1) Just keep the mcc and its children at where it is in mainline right
now, so *partly* reverting this patch. This has the problem of still
producing a dtc warning, so kind of defeats the purpose of this patch.
2) Add a "arm,vexpress,site = <0>;" line to the "mcc" node itself.
Works, but looks somewhat dodgy, as the mcc node should really be a
child of the motherboard node, and we should not hack around this.
3) Dig deeper and fix the DT in a way that makes dtc happy. Might
involve (dummy?) ranges or reg properties. My gut feeling is that
arm,vexpress-sysreg,func should really have been "reg" in the first
place, but that's too late to change now, anyway.
I will post 2) as a fix if 3) turns out to be not feasible.
Cheers,
Andre
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/bus/vexpress-config.c#n46
[2]
https://git.qemu.org/?p=qemu.git;a=blob;f=hw/arm/vexpress.c;hb=HEAD#l404
>
> Cheers,
> Andre
>
>>
>> Guenter
>>
>> ---
>> [ 12.744248] ------------[ cut here ]------------
>> [ 12.744562] WARNING: CPU: 0 PID: 20 at drivers/tty/serial/serial_core.c:471 uart_get_baud_rate+0x100/0x154
>> [ 12.744607] Modules linked in:
>> [ 12.744785] CPU: 0 PID: 20 Comm: kworker/0:1 Not tainted 5.7.0-rc7-next-20200526 #1
>> [ 12.744818] Hardware name: ARM-Versatile Express
>> [ 12.745021] Workqueue: events amba_deferred_retry_func
>> [ 12.745155] [<c0312484>] (unwind_backtrace) from [<c030c490>] (show_stack+0x10/0x14)
>> [ 12.745206] [<c030c490>] (show_stack) from [<c0880f04>] (dump_stack+0xc8/0xdc)
>> [ 12.745239] [<c0880f04>] (dump_stack) from [<c0346e44>] (__warn+0xdc/0xf4)
>> [ 12.745270] [<c0346e44>] (__warn) from [<c0346f0c>] (warn_slowpath_fmt+0xb0/0xb8)
>> [ 12.745302] [<c0346f0c>] (warn_slowpath_fmt) from [<c0a6b16c>] (uart_get_baud_rate+0x100/0x154)
>> [ 12.745336] [<c0a6b16c>] (uart_get_baud_rate) from [<c0a7f5ac>] (pl011_set_termios+0x48/0x32c)
>> [ 12.745367] [<c0a7f5ac>] (pl011_set_termios) from [<c0a6bbbc>] (uart_set_options+0x124/0x164)
>> [ 12.745404] [<c0a6bbbc>] (uart_set_options) from [<c1b8c804>] (pl011_console_setup+0x214/0x230)
>> [ 12.745438] [<c1b8c804>] (pl011_console_setup) from [<c03ab0d8>] (try_enable_new_console+0x98/0x138)
>> [ 12.745469] [<c03ab0d8>] (try_enable_new_console) from [<c03acc64>] (register_console+0xe8/0x304)
>> [ 12.745499] [<c03acc64>] (register_console) from [<c0a6c88c>] (uart_add_one_port+0x4c0/0x504)
>> [ 12.745529] [<c0a6c88c>] (uart_add_one_port) from [<c0a80404>] (pl011_register_port+0x5c/0xac)
>> [ 12.745568] [<c0a80404>] (pl011_register_port) from [<c097f5a0>] (amba_probe+0x9c/0x110)
>> [ 12.745602] [<c097f5a0>] (amba_probe) from [<c0b57e84>] (really_probe+0x218/0x348)
>> [ 12.745632] [<c0b57e84>] (really_probe) from [<c0b580c0>] (driver_probe_device+0x5c/0xb4)
>> [ 12.745662] [<c0b580c0>] (driver_probe_device) from [<c0b55ff4>] (bus_for_each_drv+0x58/0xb8)
>> [ 12.745692] [<c0b55ff4>] (bus_for_each_drv) from [<c0b57bf8>] (__device_attach+0xd4/0x140)
>> [ 12.745721] [<c0b57bf8>] (__device_attach) from [<c0b56eb0>] (bus_probe_device+0x88/0x90)
>> [ 12.745751] [<c0b56eb0>] (bus_probe_device) from [<c0b53234>] (device_add+0x3d4/0x6e8)
>> [ 12.745782] [<c0b53234>] (device_add) from [<c097f664>] (amba_device_try_add+0x50/0x2d4)
>> [ 12.745812] [<c097f664>] (amba_device_try_add) from [<c097f924>] (amba_deferred_retry+0x3c/0x98)
>> [ 12.745847] [<c097f924>] (amba_deferred_retry) from [<c097f988>] (amba_deferred_retry_func+0x8/0x40)
>> [ 12.745881] [<c097f988>] (amba_deferred_retry_func) from [<c0365b6c>] (process_one_work+0x2b8/0x6e8)
>> [ 12.745912] [<c0365b6c>] (process_one_work) from [<c0365fe0>] (worker_thread+0x44/0x540)
>> [ 12.745942] [<c0365fe0>] (worker_thread) from [<c036d810>] (kthread+0x16c/0x178)
>> [ 12.745973] [<c036d810>] (kthread) from [<c03001a8>] (ret_from_fork+0x14/0x2c)
>> [ 12.746041] Exception stack(0xc73abfb0 to 0xc73abff8)
>> [ 12.746181] bfa0: 00000000 00000000 00000000 00000000
>> [ 12.746302] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> [ 12.746397] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
>> [ 12.746651] ---[ end trace 2a3f61da56bd8a49 ]---
>>
>> ---
>> # bad: [b0523c7b1c9d0edcd6c0fe6d2cb558a9ad5c60a8] Add linux-next specific files for 20200526
>> # good: [9cb1fd0efd195590b828b9b865421ad345a4a145] Linux 5.7-rc7
>> git bisect start 'next-20200526' 'v5.7-rc7'
>> # bad: [0c7351ad83670964e48cb9a098ad732c1ecbf804] Merge remote-tracking branch 'crypto/master'
>> git bisect bad 0c7351ad83670964e48cb9a098ad732c1ecbf804
>> # bad: [42e11d9b4682229fa7187d129758b8c382f8cd5d] Merge remote-tracking branch 'jc_docs/docs-next'
>> git bisect bad 42e11d9b4682229fa7187d129758b8c382f8cd5d
>> # bad: [ab6f501559e9efa687c711a781243cf6651a82d3] Merge remote-tracking branch 'm68k/for-next'
>> git bisect bad ab6f501559e9efa687c711a781243cf6651a82d3
>> # bad: [44aaa516ca63b3ab2da8ae81e9c6a58656e6acb5] Merge branch 'arm/drivers' into for-next
>> git bisect bad 44aaa516ca63b3ab2da8ae81e9c6a58656e6acb5
>> # good: [1cb00f8c3b36e6ae026fb58d1cd2ccd78b81aa9f] Merge tag 'qcom-arm64-for-5.8' of git://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux into arm/dt
>> git bisect good 1cb00f8c3b36e6ae026fb58d1cd2ccd78b81aa9f
>> # bad: [ed0c25932fbfafdfe37e9633dee21770d3c5a306] Merge branch 'arm/defconfig' into for-next
>> git bisect bad ed0c25932fbfafdfe37e9633dee21770d3c5a306
>> # bad: [9eddc06a3bc79402f50176703237ed045ae77b16] Merge branch 'mmp/fixes' into arm/dt
>> git bisect bad 9eddc06a3bc79402f50176703237ed045ae77b16
>> # bad: [87b990ab62722a8a3cb0691107971ab1bd7bddb5] Merge tag 'mvebu-dt64-5.8-1' of git://git.infradead.org/linux-mvebu into arm/dt
>> git bisect bad 87b990ab62722a8a3cb0691107971ab1bd7bddb5
>> # bad: [94cc3f1baabac5e5c4dcc6c2f070353f8315d0ee] arm64: dts: juno: Fix SCPI shared mem node name
>> git bisect bad 94cc3f1baabac5e5c4dcc6c2f070353f8315d0ee
>> # bad: [a78aee9e434932a500db36cc6d88daeff3745e9f] arm64: dts: juno: Fix GIC child nodes
>> git bisect bad a78aee9e434932a500db36cc6d88daeff3745e9f
>> # bad: [feebdc3f7950d7e44e914e821f6c04e58e292c74] arm64: dts: fvp: Move fixed clocks out of bus node
>> git bisect bad feebdc3f7950d7e44e914e821f6c04e58e292c74
>> # good: [849bfc3dfc13cde6ec04fbcf32af553ded9f7ec3] arm64: dts: fvp: Move fixed devices out of bus node
>> git bisect good 849bfc3dfc13cde6ec04fbcf32af553ded9f7ec3
>> # bad: [d9258898ad49cbb46caffe23af0d4f0b766e67a2] arm64: dts: vexpress: Move fixed devices out of bus node
>> git bisect bad d9258898ad49cbb46caffe23af0d4f0b766e67a2
>> # first bad commit: [d9258898ad49cbb46caffe23af0d4f0b766e67a2] arm64: dts: vexpress: Move fixed devices out of bus node
>>
>
^ permalink raw reply
* Re: [RFC PATCH v5 3/6] PM / devfreq: exynos-bus: Add registration of interconnect child device
From: Sylwester Nawrocki @ 2020-06-01 10:04 UTC (permalink / raw)
To: Chanwoo Choi
Cc: Georgi Djakov, Chanwoo Choi, Krzysztof Kozlowski,
Artur Świgoń, MyungJoo Ham, inki.dae, Seung-Woo Kim,
Bartlomiej Zolnierkiewicz, Marek Szyprowski, linux-kernel,
linux-samsung-soc, dri-devel, linux-arm-kernel, Rob Herring,
devicetree
In-Reply-To: <CAGTfZH1KC=jpQ5GXNtEf1cn7+WqXJdqbbVKmpxr8Snh4GEy8bA@mail.gmail.com>
Cc: Rob, devicetree ML
On 31.05.2020 01:57, Chanwoo Choi wrote:
> On Sat, May 30, 2020 at 1:33 AM Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>>
>> This patch adds registration of a child platform device for the exynos
>> interconnect driver. It is assumed that the interconnect provider will
>> only be needed when #interconnect-cells property is present in the bus
>> DT node, hence the child device will be created only when such a property
>> is present.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>
>> Changes for v5:
>> - new patch.
>> ---
>> drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>> index 8fa8eb5..856e37d 100644
>> --- a/drivers/devfreq/exynos-bus.c
>> +++ b/drivers/devfreq/exynos-bus.c
>> @@ -24,6 +24,7 @@
>>
>> struct exynos_bus {
>> struct device *dev;
>> + struct platform_device *icc_pdev;
>>
>> struct devfreq *devfreq;
>> struct devfreq_event_dev **edev;
>> @@ -156,6 +157,8 @@ static void exynos_bus_exit(struct device *dev)
>> if (ret < 0)
>> dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>
>> + platform_device_unregister(bus->icc_pdev);
>> +
>> dev_pm_opp_of_remove_table(dev);
>> clk_disable_unprepare(bus->clk);
>> if (bus->opp_table) {
>> @@ -168,6 +171,8 @@ static void exynos_bus_passive_exit(struct device *dev)
>> {
>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>
>> + platform_device_unregister(bus->icc_pdev);
>> +
>> dev_pm_opp_of_remove_table(dev);
>> clk_disable_unprepare(bus->clk);
>> }
>> @@ -431,6 +436,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>> if (ret < 0)
>> goto err;
>>
>> + /* Create child platform device for the interconnect provider */
>> + if (of_get_property(dev->of_node, "#interconnect-cells", NULL)) {
>> + bus->icc_pdev = platform_device_register_data(
>> + dev, "exynos-generic-icc",
>> + PLATFORM_DEVID_AUTO, NULL, 0);
>> +
>> + if (IS_ERR(bus->icc_pdev)) {
>> + ret = PTR_ERR(bus->icc_pdev);
>> + goto err;
>> + }
>> + }
>> +
>> max_state = bus->devfreq->profile->max_state;
>> min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
>> max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
>> --
>> 2.7.4
>>
>
> It looks like very similar like the registering the interconnect
> device of imx-bus.c
> and I already reviewed and agreed this approach.
>
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
>
> nitpick: IMHO, I think that 'exynos-icc' is proper and simple without
> 'generic' word.
> If we need to add new icc compatible int the future, we will add
> 'exynosXXXX-icc' new compatible.
> But, I'm not forcing it. just opinion. Anyway, I agree this approach.
Thanks for review. I will change the name to exynos-icc in next version,
as I commented at other patch, it is not part of any DT binding,
it is just for device/driver matching between devfreq and interconnect.
--
Thanks,
Sylwester
^ permalink raw reply
* Re: [RFC PATCH v5 2/6] interconnect: Add generic interconnect driver for Exynos SoCs
From: Sylwester Nawrocki @ 2020-06-01 9:57 UTC (permalink / raw)
To: Chanwoo Choi
Cc: Georgi Djakov, Chanwoo Choi, Krzysztof Kozlowski,
Artur Świgoń, MyungJoo Ham, inki.dae, Seung-Woo Kim,
Bartlomiej Zolnierkiewicz, Marek Szyprowski, linux-kernel,
linux-samsung-soc, dri-devel, linux-arm-kernel, Rob Herring,
devicetree
In-Reply-To: <CAGTfZH3zvgOtME0U2hKjtqO49e0-Nw6MRxhw+z6Mfio-=FUwcQ@mail.gmail.com>
Cc: Rob, devicetree ML
On 31.05.2020 02:13, Chanwoo Choi wrote:
> On Sat, May 30, 2020 at 1:34 AM Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>>
>> This patch adds a generic interconnect driver for Exynos SoCs in order
>> to provide interconnect functionality for each "samsung,exynos-bus"
>> compatible device.
>>
>> The SoC topology is a graph (or more specifically, a tree) and its
>> edges are specified using the 'samsung,interconnect-parent' in the
>> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
>> propagated to ensure that the parent is probed before its children.
>>
>> Each bus is now an interconnect provider and an interconnect node as
>> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
>> registers itself as a node. Node IDs are not hardcoded but rather
>> assigned dynamically at runtime. This approach allows for using this
>> driver with various Exynos SoCs.
>>
>> Frequencies requested via the interconnect API for a given node are
>> propagated to devfreq using dev_pm_qos_update_request(). Please note
>> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
>> case all interconnect API functions are no-op.
>>
>> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>
>> Changes for v5:
>> - adjust to renamed exynos,interconnect-parent-node property,
>> - use automatically generated platform device id as the interconect
>> node id instead of a now unavailable devfreq->id field,
>> - add icc_ prefix to some variables to make the code more self-commenting,
>> - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
>> - adjust to exynos,interconnect-parent-node property rename to
>> samsung,interconnect-parent,
>> - converted to a separate platform driver in drivers/interconnect.
>> ---
>> drivers/interconnect/Kconfig | 1 +
>> drivers/interconnect/Makefile | 1 +
>> drivers/interconnect/exynos/Kconfig | 6 ++
>> drivers/interconnect/exynos/Makefile | 4 +
>> drivers/interconnect/exynos/exynos.c | 185 +++++++++++++++++++++++++++++++++++
>> 5 files changed, 197 insertions(+)
>> create mode 100644 drivers/interconnect/exynos/Kconfig
>> create mode 100644 drivers/interconnect/exynos/Makefile
>> create mode 100644 drivers/interconnect/exynos/exynos.c
>>
>> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
>> index 5b7204e..eca6eda 100644
>> --- a/drivers/interconnect/Kconfig
>> +++ b/drivers/interconnect/Kconfig
>> @@ -11,6 +11,7 @@ menuconfig INTERCONNECT
>>
>> if INTERCONNECT
>>
>> +source "drivers/interconnect/exynos/Kconfig"
>> source "drivers/interconnect/imx/Kconfig"
>> source "drivers/interconnect/qcom/Kconfig"
>>
>> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
>> index 4825c28..2ba1de6 100644
>> --- a/drivers/interconnect/Makefile
>> +++ b/drivers/interconnect/Makefile
>> @@ -4,5 +4,6 @@ CFLAGS_core.o := -I$(src)
>> icc-core-objs := core.o
>>
>> obj-$(CONFIG_INTERCONNECT) += icc-core.o
>> +obj-$(CONFIG_INTERCONNECT_EXYNOS) += exynos/
>> obj-$(CONFIG_INTERCONNECT_IMX) += imx/
>> obj-$(CONFIG_INTERCONNECT_QCOM) += qcom/
>> diff --git a/drivers/interconnect/exynos/Kconfig b/drivers/interconnect/exynos/Kconfig
>> new file mode 100644
>> index 0000000..e51e52e
>> --- /dev/null
>> +++ b/drivers/interconnect/exynos/Kconfig
>> @@ -0,0 +1,6 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +config INTERCONNECT_EXYNOS
>> + tristate "Exynos generic interconnect driver"
>> + depends on ARCH_EXYNOS || COMPILE_TEST
>> + help
>> + Generic interconnect driver for Exynos SoCs.
>> diff --git a/drivers/interconnect/exynos/Makefile b/drivers/interconnect/exynos/Makefile
>> new file mode 100644
>> index 0000000..e19d1df
>> --- /dev/null
>> +++ b/drivers/interconnect/exynos/Makefile
>> @@ -0,0 +1,4 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +exynos-interconnect-objs := exynos.o
>> +
>> +obj-$(CONFIG_INTERCONNECT_EXYNOS) += exynos-interconnect.o
>> diff --git a/drivers/interconnect/exynos/exynos.c b/drivers/interconnect/exynos/exynos.c
>> new file mode 100644
>> index 0000000..8278194
>> --- /dev/null
>> +++ b/drivers/interconnect/exynos/exynos.c
>> @@ -0,0 +1,185 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Exynos generic interconnect provider driver
>> + *
>> + * Copyright (c) 2020 Samsung Electronics Co., Ltd.
>> + *
>> + * Authors: Artur Świgoń <a.swigon@samsung.com>
>> + * Sylwester Nawrocki <s.nawrocki@samsung.com>
>> + */
>> +#include <linux/device.h>
>> +#include <linux/interconnect-provider.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_qos.h>
>> +
>> +#define kbps_to_khz(x) ((x) / 8)
>> +
>> +struct exynos_icc_priv {
>> + struct device *dev;
>> +
>> + /* One interconnect node per provider */
>> + struct icc_provider provider;
>> + struct icc_node *node;
>> +
>> + struct dev_pm_qos_request qos_req;
>> +};
>> +
>> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
>> +{
>> + struct of_phandle_args args;
>> + int num, ret;
>> +
>> + num = of_count_phandle_with_args(np, "samsung,interconnect-parent",
>> + "#interconnect-cells");
>> + if (num != 1)
>> + return NULL; /* parent nodes are optional */
>> +
>> + ret = of_parse_phandle_with_args(np, "samsung,interconnect-parent",
>> + "#interconnect-cells", 0, &args);
>> + if (ret < 0)
>> + return ERR_PTR(ret);
>> +
>> + of_node_put(args.np);
>> +
>> + return of_icc_get_from_provider(&args);
>> +}
>> +
>> +
>> +static int exynos_generic_icc_set(struct icc_node *src, struct icc_node *dst)
>> +{
>> + struct exynos_icc_priv *src_priv = src->data, *dst_priv = dst->data;
>> + s32 src_freq = kbps_to_khz(max(src->avg_bw, src->peak_bw));
>> + s32 dst_freq = kbps_to_khz(max(dst->avg_bw, dst->peak_bw));
>> + int ret;
>> +
>> + ret = dev_pm_qos_update_request(&src_priv->qos_req, src_freq);
>> + if (ret < 0) {
>> + dev_err(src_priv->dev, "failed to update PM QoS of %s\n",
>> + src->name);
>> + return ret;
>> + }
>> +
>> + ret = dev_pm_qos_update_request(&dst_priv->qos_req, dst_freq);
>> + if (ret < 0) {
>> + dev_err(dst_priv->dev, "failed to update PM QoS of %s\n",
>> + dst->name);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static struct icc_node *exynos_generic_icc_xlate(struct of_phandle_args *spec,
>> + void *data)
>> +{
>> + struct exynos_icc_priv *priv = data;
>> +
>> + if (spec->np != priv->dev->parent->of_node)
>> + return ERR_PTR(-EINVAL);
>> +
>> + return priv->node;
>> +}
>> +
>> +static int exynos_generic_icc_remove(struct platform_device *pdev)
>> +{
>> + struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
>> + struct icc_node *parent_node, *node = priv->node;
>> +
>> + parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
>> + if (parent_node && !IS_ERR(parent_node))
>> + icc_link_destroy(node, parent_node);
>> +
>> + icc_nodes_remove(&priv->provider);
>> + icc_provider_del(&priv->provider);
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_generic_icc_probe(struct platform_device *pdev)
>> +{
>> + struct device *bus_dev = pdev->dev.parent;
>> + struct exynos_icc_priv *priv;
>> + struct icc_provider *provider;
>> + struct icc_node *icc_node, *icc_parent_node;
>> + int ret;
>> +
>> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + priv->dev = &pdev->dev;
>> + platform_set_drvdata(pdev, priv);
>> +
>> + provider = &priv->provider;
>> +
>> + provider->set = exynos_generic_icc_set;
>> + provider->aggregate = icc_std_aggregate;
>> + provider->xlate = exynos_generic_icc_xlate;
>> + provider->dev = bus_dev;
>> + provider->inter_set = true;
>> + provider->data = priv;
>> +
>> + ret = icc_provider_add(provider);
>> + if (ret < 0)
>> + return ret;
>> +
>> + icc_node = icc_node_create(pdev->id);
>> + if (IS_ERR(icc_node)) {
>> + ret = PTR_ERR(icc_node);
>> + goto err_prov_del;
>> + }
>> +
>> + priv->node = icc_node;
>> + icc_node->name = bus_dev->of_node->name;
>> + icc_node->data = priv;
>> + icc_node_add(icc_node, provider);
>> +
>> + icc_parent_node = exynos_icc_get_parent(bus_dev->of_node);
>> + if (IS_ERR(icc_parent_node)) {
>> + ret = PTR_ERR(icc_parent_node);
>> + goto err_node_del;
>> + }
>> + if (icc_parent_node) {
>> + ret = icc_link_create(icc_node, icc_parent_node->id);
>> + if (ret < 0)
>> + goto err_node_del;
>> + }
>> +
>> + /*
>> + * Register a PM QoS request for the bus device for which also devfreq
>> + * functionality is registered.
>> + */
>> + ret = dev_pm_qos_add_request(bus_dev, &priv->qos_req,
>> + DEV_PM_QOS_MIN_FREQUENCY, 0);
>> + if (ret < 0)
>> + goto err_link_destroy;
>> +
>> + return 0;
>> +
>> +err_link_destroy:
>> + if (icc_parent_node)
>> + icc_link_destroy(icc_node, icc_parent_node);
>> +err_node_del:
>> + icc_nodes_remove(provider);
>> +err_prov_del:
>> + icc_provider_del(provider);
>> +
>> + return ret;
>> +}
>> +
>> +static struct platform_driver exynos_generic_icc_driver = {
>> + .driver = {
>> + .name = "exynos-generic-icc",
>> + },
>> + .probe = exynos_generic_icc_probe,
>> + .remove = exynos_generic_icc_remove,
>> +};
>> +module_platform_driver(exynos_generic_icc_driver);
>> +
>> +MODULE_DESCRIPTION("Exynos generic interconnect driver");
>> +MODULE_AUTHOR("Artur Świgoń <a.swigon@samsung.com>");
>> +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:exynos-generic-icc");
>> --
>> 2.7.4
>>
>
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>
> As I commented, How about changing the compatible name 'exynos-icc'
> without 'generic'?
> The 'exynos-icc' doesn't have the any specific version of Exynos SoC,
> it think that it already contain the 'generic' meaning for Exynos SoC.
Sure, I can change it to "exynos-icc". However please note it is just
a name for the driver and its related virtual platform (sub)device that
is created in the devfreq driver, which matches on the "samsung,exynos-bus"
compatible. Of course we could add any specific DT compatible to this driver
in future if needed.
--
Thanks,
Sylwester
^ permalink raw reply
* [RFC PATCH 2/3] firmware: Add support for PSA FF-A transport for VM partitions
From: Sudeep Holla @ 2020-06-01 9:45 UTC (permalink / raw)
To: Will Deacon, devicetree, linux-arm-kernel
Cc: Sudeep Holla, linux-kernel, Marc Zyngier
In-Reply-To: <20200601094512.50509-1-sudeep.holla@arm.com>
Initial support for PSA FF-A interface providing APIs for non-secure VM
partitions.
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
drivers/firmware/Kconfig | 1 +
drivers/firmware/Makefile | 1 +
drivers/firmware/arm_psa_ffa/Kconfig | 15 ++
drivers/firmware/arm_psa_ffa/Makefile | 2 +
drivers/firmware/arm_psa_ffa/driver.c | 250 ++++++++++++++++++++++++++
include/linux/arm_psa_ffa.h | 42 +++++
6 files changed, 311 insertions(+)
create mode 100644 drivers/firmware/arm_psa_ffa/Kconfig
create mode 100644 drivers/firmware/arm_psa_ffa/Makefile
create mode 100644 drivers/firmware/arm_psa_ffa/driver.c
create mode 100644 include/linux/arm_psa_ffa.h
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 4843e94713a4..1ee421974cba 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -295,6 +295,7 @@ config TURRIS_MOX_RWTM
other manufacturing data and also utilize the Entropy Bit Generator
for hardware random number generation.
+source "drivers/firmware/arm_psa_ffa/Kconfig"
source "drivers/firmware/broadcom/Kconfig"
source "drivers/firmware/google/Kconfig"
source "drivers/firmware/efi/Kconfig"
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 99510be9f5ed..1c0b5f130d7d 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_TI_SCI_PROTOCOL) += ti_sci.o
obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
obj-$(CONFIG_TURRIS_MOX_RWTM) += turris-mox-rwtm.o
+obj-y += arm_psa_ffa/
obj-$(CONFIG_ARM_SCMI_PROTOCOL) += arm_scmi/
obj-y += broadcom/
obj-y += meson/
diff --git a/drivers/firmware/arm_psa_ffa/Kconfig b/drivers/firmware/arm_psa_ffa/Kconfig
new file mode 100644
index 000000000000..ba699ec68ec4
--- /dev/null
+++ b/drivers/firmware/arm_psa_ffa/Kconfig
@@ -0,0 +1,15 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config ARM_PSA_FFA_TRANSPORT
+ tristate "Arm Platform Security Architecture Firmware Framework for Armv8-A"
+ depends on ARM64 && HAVE_ARM_SMCCC_DISCOVERY
+ help
+ This Firmware Framework(FF) of the Platform Security Architecture
+ (PSA) for Arm A-profile processors describes interfaces that
+ standardize communication between the various software images which
+ includes communication between images in the Secure world and
+ Normal world. It also leverages the virtualization extension to
+ isolate software images provided by an ecosystem of vendors from
+ each other.
+
+ This driver provides interface for all the client drivers making
+ use of the features offered by ARM PSA-FF-A.
diff --git a/drivers/firmware/arm_psa_ffa/Makefile b/drivers/firmware/arm_psa_ffa/Makefile
new file mode 100644
index 000000000000..ac0455ff71a4
--- /dev/null
+++ b/drivers/firmware/arm_psa_ffa/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_ARM_PSA_FFA_TRANSPORT) += driver.o
diff --git a/drivers/firmware/arm_psa_ffa/driver.c b/drivers/firmware/arm_psa_ffa/driver.c
new file mode 100644
index 000000000000..700bd5850746
--- /dev/null
+++ b/drivers/firmware/arm_psa_ffa/driver.c
@@ -0,0 +1,250 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Arm Platform Security Architecture(PSA) Firmware Framework for ARMv8-A(FFA)
+ * interface driver
+ *
+ * The Arm PSA FFA specification[1] describes a software architecture to
+ * leverages the virtualization extension to isolate software images
+ * provided by an ecosystem of vendors from each other and describes
+ * interfaces that standardize communication between the various software
+ * images including communication between images in the Secure world and
+ * Normal world. Any Hypervisor could use the PSA FFA interfaces to enable
+ * communication between VMs it manages.
+ *
+ * The Hypervisor a.k.a Partition managers in FFA terminology can assign
+ * system resources(Memory regions, Devices, CPU cycles) to the partitions
+ * and manage isolation amongst them.
+ *
+ * [1] https://developer.arm.com/docs/den0077/latest
+ *
+ * Copyright (C) 2020 Arm Ltd.
+ */
+
+#define pr_fmt(fmt) "ARM PSA FF-A: " fmt
+
+#include <linux/arm_psa_ffa.h>
+#include <linux/arm-smccc.h>
+#include <linux/bitfield.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/types.h>
+
+#define PSA_FFA_SMC(calling_convention, func_num) \
+ ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, (calling_convention), \
+ ARM_SMCCC_OWNER_STANDARD, (func_num))
+
+#define PSA_FFA_SMC_32(func_num) PSA_FFA_SMC(ARM_SMCCC_SMC_32, (func_num))
+#define PSA_FFA_SMC_64(func_num) PSA_FFA_SMC(ARM_SMCCC_SMC_64, (func_num))
+
+#define PSA_FFA_ERROR PSA_FFA_SMC_32(0x60)
+#define PSA_FFA_SUCCESS PSA_FFA_SMC_32(0x61)
+#define PSA_FFA_INTERRUPT PSA_FFA_SMC_32(0x62)
+#define PSA_FFA_VERSION PSA_FFA_SMC_32(0x63)
+#define PSA_FFA_FEATURES PSA_FFA_SMC_32(0x64)
+#define PSA_FFA_PARTITION_INFO_GET PSA_FFA_SMC_32(0x68)
+#define PSA_FFA_ID_GET PSA_FFA_SMC_32(0x69)
+
+/* PSA_FFA error codes. */
+#define PSA_FFA_RET_SUCCESS (0)
+#define PSA_FFA_RET_NOT_SUPPORTED (-1)
+#define PSA_FFA_RET_INVALID_PARAMETERS (-2)
+#define PSA_FFA_RET_NO_MEMORY (-3)
+#define PSA_FFA_RET_BUSY (-4)
+#define PSA_FFA_RET_INTERRUPTED (-5)
+#define PSA_FFA_RET_DENIED (-6)
+#define PSA_FFA_RET_RETRY (-7)
+#define PSA_FFA_RET_ABORTED (-8)
+
+#define MAJOR_VERSION_MASK GENMASK(30, 16)
+#define MINOR_VERSION_MASK GENMASK(15, 0)
+#define MAJOR_VERSION(x) (u16)(FIELD_GET(MAJOR_VERSION_MASK, (x)))
+#define MINOR_VERSION(x) (u16)(FIELD_GET(MINOR_VERSION_MASK, (x)))
+#define PACK_VERSION_INFO(major, minor) \
+ (FIELD_PREP(MAJOR_VERSION_MASK, (major)) | \
+ FIELD_PREP(MINOR_VERSION_MASK, (minor)))
+#define PSA_FFA_VERSION_1_0 PACK_VERSION_INFO(1, 0)
+#define PSA_FFA_MIN_VERSION PSA_FFA_VERSION_1_0
+#define PSA_FFA_DRIVER_VERSION PSA_FFA_VERSION_1_0
+
+#define SENDER_ID_MASK GENMASK(31, 16)
+#define RECEIVER_ID_MASK GENMASK(15, 0)
+#define SENDER_ID(x) (u16)(FIELD_GET(SENDER_ID_MASK, (x)))
+#define RECEIVER_ID(x) (u16)(FIELD_GET(RECEIVER_ID_MASK, (x)))
+#define PACK_TARGET_INFO(s, r) \
+ (FIELD_PREP(SENDER_ID_MASK, (s)) | FIELD_PREP(RECEIVER_ID_MASK, (r)))
+
+typedef struct arm_smccc_res
+(arm_psa_ffa_fn)(unsigned long, unsigned long, unsigned long, unsigned long,
+ unsigned long, unsigned long, unsigned long, unsigned long);
+static arm_psa_ffa_fn *invoke_arm_psa_ffa_fn;
+
+static struct device *psa_ffa_dev;
+
+static const int psa_ffa_linux_errmap[] = {
+ /* better than switch case as long as return value is continuous */
+ 0, /* PSA_FFA_RET_SUCCESS */
+ -EOPNOTSUPP, /* PSA_FFA_RET_NOT_SUPPORTED */
+ -EINVAL, /* PSA_FFA_RET_INVALID_PARAMETERS */
+ -ENOMEM, /* PSA_FFA_RET_NO_MEMORY */
+ -EBUSY, /* PSA_FFA_RET_BUSY */
+ -EINTR, /* PSA_FFA_RET_INTERRUPTED */
+ -EACCES, /* PSA_FFA_RET_DENIED */
+ -EAGAIN, /* PSA_FFA_RET_RETRY */
+ -ECANCELED, /* PSA_FFA_RET_ABORTED */
+};
+
+static inline int psa_ffa_to_linux_errno(int errno)
+{
+ if (errno < PSA_FFA_RET_SUCCESS && errno >= PSA_FFA_RET_ABORTED)
+ return psa_ffa_linux_errmap[-errno];
+ return -EINVAL;
+}
+
+struct arm_smccc_res
+__arm_psa_ffa_fn_smc(unsigned long function_id,unsigned long arg0,
+ unsigned long arg1, unsigned long arg2,
+ unsigned long arg3, unsigned long arg4,
+ unsigned long arg5, unsigned long arg6)
+{
+ struct arm_smccc_res res;
+
+ arm_smccc_smc(function_id, arg0, arg1, arg2, arg3, arg4, arg5, arg6,
+ &res);
+
+ return res;
+}
+
+struct arm_smccc_res
+__arm_psa_ffa_fn_hvc(unsigned long function_id,unsigned long arg0,
+ unsigned long arg1, unsigned long arg2,
+ unsigned long arg3, unsigned long arg4,
+ unsigned long arg5, unsigned long arg6)
+{
+ struct arm_smccc_res res;
+
+ arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4, arg5, arg6,
+ &res);
+
+ return res;
+}
+
+static u16 psa_ffa_id_get(void)
+{
+ struct arm_smccc_res id;
+
+ id = invoke_arm_psa_ffa_fn(PSA_FFA_ID_GET, 0, 0, 0, 0, 0, 0, 0);
+
+ if (id.a0 == PSA_FFA_ERROR)
+ return psa_ffa_to_linux_errno((int)id.a2);
+ else
+ return id.a2 & 0xffff;
+}
+
+static int psa_ffa_partition_info_get(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3,
+ struct psa_ffa_partition_info **buffer)
+{
+ struct arm_smccc_res partition_info;
+
+ partition_info = invoke_arm_psa_ffa_fn(PSA_FFA_PARTITION_INFO_GET,
+ uuid0, uuid1, uuid2, uuid3,
+ 0, 0, 0);
+
+ if (partition_info.a0 == PSA_FFA_ERROR)
+ return psa_ffa_to_linux_errno((int)partition_info.a2);
+
+ /* TODO: read data from RX buffers */
+ return partition_info.a2;
+}
+
+static struct arm_psa_ffa_handle drv_handle = {
+ .id_get = psa_ffa_id_get,
+ .partition_info_get = psa_ffa_partition_info_get,
+};
+
+struct arm_psa_ffa_handle *arm_psa_ffa_handle_get(struct device *dev)
+{
+ struct arm_psa_ffa_handle *handle = NULL;
+
+ if (dev->parent == psa_ffa_dev)
+ handle = &drv_handle;
+
+ return handle;
+}
+
+EXPORT_SYMBOL_GPL(arm_psa_ffa_handle_get);
+
+static int psa_ffa_version_check(void)
+{
+ struct arm_smccc_res version;
+
+ version = invoke_arm_psa_ffa_fn(PSA_FFA_VERSION, PSA_FFA_DRIVER_VERSION,
+ 0, 0, 0, 0, 0, 0);
+
+ if (version.a0 == PSA_FFA_RET_NOT_SUPPORTED) {
+ pr_info("PSA_FFA_VERSION returned not supported\n");
+ return -ENOTSUPP;
+ }
+
+ if (version.a0 < PSA_FFA_MIN_VERSION ||
+ version.a0 > PSA_FFA_DRIVER_VERSION) {
+ pr_err("Incompatible version %d.%d found\n",
+ MAJOR_VERSION(version.a0), MINOR_VERSION(version.a0));
+ return -EINVAL;
+ }
+
+ pr_info("Found version %d.%d found\n", MAJOR_VERSION(version.a0),
+ MINOR_VERSION(version.a0));
+ return 0;
+}
+
+static int psa_ffa_probe(struct platform_device *pdev)
+{
+ int ret;
+ enum arm_smccc_conduit conduit;
+
+ if (arm_smccc_get_version() < ARM_SMCCC_VERSION_1_2)
+ return 0;
+
+ conduit = arm_smccc_1_1_get_conduit();
+ if (conduit == SMCCC_CONDUIT_NONE) {
+ pr_err("%s: invalid SMCCC conduit\n", __func__);
+ return -EOPNOTSUPP;
+ }
+
+ if (conduit == SMCCC_CONDUIT_SMC)
+ invoke_arm_psa_ffa_fn = __arm_psa_ffa_fn_smc;
+ else
+ invoke_arm_psa_ffa_fn = __arm_psa_ffa_fn_hvc;
+
+ ret = psa_ffa_version_check();
+ if (ret)
+ return ret;
+
+ psa_ffa_dev = &pdev->dev;
+
+ return devm_of_platform_populate(psa_ffa_dev);
+}
+
+static const struct of_device_id psa_ffa_of_match[] = {
+ {.compatible = "arm,psa-ffa"},
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, psa_ffa_of_match);
+
+static struct platform_driver psa_ffa_driver = {
+ .driver = {
+ .name = "arm-psa-ffa",
+ .of_match_table = psa_ffa_of_match,
+ },
+ .probe = psa_ffa_probe,
+};
+
+module_platform_driver(psa_ffa_driver);
+
+MODULE_ALIAS("platform: arm-psa-ffa");
+MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
+MODULE_DESCRIPTION("Arm PSA FF-A interface driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/arm_psa_ffa.h b/include/linux/arm_psa_ffa.h
new file mode 100644
index 000000000000..03a4ff559fa3
--- /dev/null
+++ b/include/linux/arm_psa_ffa.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2019, 2020 Arm Ltd.
+ */
+
+#ifndef __LINUX_ARM_PSA_FFA_H
+#define __LINUX_ARM_PSA_FFA_H
+
+#include <linux/device.h>
+#include <linux/types.h>
+
+struct psa_ffa_partition_info {
+ /* The ID of the VM the information is about */
+ uint16_t id;
+ /* The number of execution contexts implemented by the partition */
+ uint16_t execution_context;
+ /* The Partition's properties, e.g. supported messaging methods */
+ uint32_t partition_properties;
+};
+
+
+/**
+ * struct psa_ffa_ops - represents the various PSA_FFA protocol operations
+ * available for an endpoint.
+ */
+struct arm_psa_ffa_handle {
+ u16 (*id_get)(void);
+ int (*partition_info_get)(u32 uuid0, u32 uuid1, u32 uuid2, u32 uuid3,
+ struct psa_ffa_partition_info**);
+};
+
+#if IS_REACHABLE(CONFIG_ARM_PSA_FFA_TRANSPORT)
+struct arm_psa_ffa_handle *arm_psa_ffa_handle_get(struct device *dev);
+#else
+static inline
+struct arm_psa_ffa_handle * arm_psa_ffa_handle_get(struct device *dev)
+{
+ return NULL;
+}
+#endif
+
+#endif /*__LINUX_ARM_PSA_FFA_H*/
--
2.17.1
^ permalink raw reply related
* [RFC PATCH 3/3] firmware: Add example PSA FF-A non-secure VM partition
From: Sudeep Holla @ 2020-06-01 9:45 UTC (permalink / raw)
To: Will Deacon, devicetree, linux-arm-kernel
Cc: Sudeep Holla, linux-kernel, Marc Zyngier
In-Reply-To: <20200601094512.50509-1-sudeep.holla@arm.com>
This is just an example non-secure VM partition to show how to create
the device and use the PSA FF-A interface APIs.
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
drivers/firmware/arm_psa_ffa/Kconfig | 7 +++
drivers/firmware/arm_psa_ffa/Makefile | 1 +
drivers/firmware/arm_psa_ffa/partition.c | 71 ++++++++++++++++++++++++
3 files changed, 79 insertions(+)
create mode 100644 drivers/firmware/arm_psa_ffa/partition.c
diff --git a/drivers/firmware/arm_psa_ffa/Kconfig b/drivers/firmware/arm_psa_ffa/Kconfig
index ba699ec68ec4..34ad61e79234 100644
--- a/drivers/firmware/arm_psa_ffa/Kconfig
+++ b/drivers/firmware/arm_psa_ffa/Kconfig
@@ -13,3 +13,10 @@ config ARM_PSA_FFA_TRANSPORT
This driver provides interface for all the client drivers making
use of the features offered by ARM PSA-FF-A.
+
+config ARM_PSA_FFA_PARTITION
+ tristate "Arm PSA FF-A compliant partition"
+ depends on ARM_PSA_FFA_TRANSPORT
+ help
+ This driver provides example for ARM PSA-FF-A client driver
+ making use of the interfaces offered by ARM PSA-FF-A driver.
diff --git a/drivers/firmware/arm_psa_ffa/Makefile b/drivers/firmware/arm_psa_ffa/Makefile
index ac0455ff71a4..8eb03898baf7 100644
--- a/drivers/firmware/arm_psa_ffa/Makefile
+++ b/drivers/firmware/arm_psa_ffa/Makefile
@@ -1,2 +1,3 @@
# SPDX-License-Identifier: GPL-2.0-only
obj-$(CONFIG_ARM_PSA_FFA_TRANSPORT) += driver.o
+obj-$(CONFIG_ARM_PSA_FFA_PARTITION) += partition.o
diff --git a/drivers/firmware/arm_psa_ffa/partition.c b/drivers/firmware/arm_psa_ffa/partition.c
new file mode 100644
index 000000000000..8549f8d61454
--- /dev/null
+++ b/drivers/firmware/arm_psa_ffa/partition.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Arm PSA FFA example partition driver
+ *
+ * Copyright (C) 2020 Arm Ltd.
+ */
+
+#include <linux/arm_psa_ffa.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/uuid.h>
+
+static int psa_ffa_partition_probe(struct platform_device *pdev)
+{
+ u16 vm_id;
+ uuid_t uuid;
+ const char *uuid_str;
+ u32 uuid0_4[4];
+ struct device *dev = &pdev->dev;
+ const struct device_node *np = dev->of_node;
+ struct arm_psa_ffa_handle *handle;
+ struct psa_ffa_partition_info **buffer;
+
+ handle = arm_psa_ffa_handle_get(dev);
+ if (!handle)
+ return -ENODEV;
+
+ if (of_property_read_string(np, "uuid", &uuid_str)) {
+ dev_err(dev, "failed to parse \"uuid\" property in '%pOF'\n", np);
+ return -ENODEV;
+ }
+
+ if (uuid_parse(uuid_str, &uuid)) {
+ dev_err(dev, "invalid \"uuid\" property (%s)\n", uuid_str);
+ return -ENODEV;
+ }
+
+ export_uuid((u8 *)uuid0_4, &uuid);
+
+ vm_id = handle->id_get();
+
+ handle->partition_info_get(uuid0_4[0], uuid0_4[1], uuid0_4[2],
+ uuid0_4[3], buffer);
+
+ return 0;
+}
+
+static const struct of_device_id psa_ffa_partition_of_match[] = {
+ {.compatible = "arm,psa-ffa-partition"},
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, psa_ffa_partition_of_match);
+
+static struct platform_driver psa_ffa_partition_driver = {
+ .driver = {
+ .name = "psa-ffa-partition",
+ .of_match_table = psa_ffa_partition_of_match,
+ },
+ .probe = psa_ffa_partition_probe,
+};
+
+module_platform_driver(psa_ffa_partition_driver);
+
+MODULE_ALIAS("platform: arm-psa-ffa");
+MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
+MODULE_DESCRIPTION("Arm PSA FF-A example partition driver");
+MODULE_LICENSE("GPL v2");
--
2.17.1
^ permalink raw reply related
* [RFC PATCH 1/3] dt-bindings: Add ARM PSA FF binding for non-secure VM partitions
From: Sudeep Holla @ 2020-06-01 9:45 UTC (permalink / raw)
To: Will Deacon, devicetree, linux-arm-kernel
Cc: Sudeep Holla, linux-kernel, Marc Zyngier
In-Reply-To: <20200601094512.50509-1-sudeep.holla@arm.com>
Add devicetree bindings for a Arm PSA FF-A compliant non-secure partition
at virtual interface(VMs).
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
.../devicetree/bindings/arm/arm,psa-ffa.txt | 47 +++++++++++++++++++
1 file changed, 47 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/arm,psa-ffa.txt
diff --git a/Documentation/devicetree/bindings/arm/arm,psa-ffa.txt b/Documentation/devicetree/bindings/arm/arm,psa-ffa.txt
new file mode 100644
index 000000000000..ee543fb5b397
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/arm,psa-ffa.txt
@@ -0,0 +1,47 @@
+Arm Platform Security Architecture(PSA) Firmware Framework(FF) for Armv8-A
+--------------------------------------------------------------------------
+
+This binding is intended to define the interface the firmware framework
+implementing the Non-secure partitions/endpoints(mostly VMs) as described
+in ARM document ARM DEN 0077A ("Arm Platform Security Architecture
+Firmware Framework for Arm v8-A") [0]
+
+In the case of a Non-secure virtual FF-A instance, the hypervisor
+(e.g. Linux KVM) can use this binding to create and launch VM partitions.
+
+The SMCCC conduit available in the VM partition itself is used and hence
+there is no explicit binding to specify the conduit used for PSA FFA
+interface.
+
+Required properties:
+
+- compatible : Should be one of:
+ "arm,psa-ffa"
+
+- One or more child nodes, each describing an PSA FFA partition using the
+ following required properties:
+
+ - compatible: Should be one of:
+ "arm,psa-ffa-partition"
+
+ - uuid : The 128-bit UUID [2] of the service implemented by this partition,
+ represented as a string.
+
+[0] https://developer.arm.com/docs/den0077/latest
+[1] https://tools.ietf.org/html/rfc4122
+
+Example:
+
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ firmware {
+ psa-ffa {
+ compatible = "arm,psa-ffa";
+
+ partition0: psa_ffa_partition {
+ compatible = "arm,psa-ffa-partition";
+ uuid = "12345678-9abc-def0-1234-56789abcdef0";
+ };
+ };
+ };
--
2.17.1
^ permalink raw reply related
* [RFC PATCH 0/3] firmware: Add support for PSA FF-A interface
From: Sudeep Holla @ 2020-06-01 9:45 UTC (permalink / raw)
To: Will Deacon, devicetree, linux-arm-kernel
Cc: Sudeep Holla, linux-kernel, Marc Zyngier
Hi All,
Sorry for posting in the middle of merge window and I must have done
this last week itself. This is not the driver I had thought about posting
last week. After I started cleaning up and looking at Will's KVM prototype[1]
for PSA FF-A (previously known as SPCI), I got more doubts on alignment
and dropped huge chunk of interface APIs in the driver in order to keep
it simple, and get aligned more with that prototype and avoid scanning
lots of code unnecessary.
Here are few things to clarify:
1. DT bindings
---------------
I was initially against adding bindings for Tx/Rx buffers for
partitions. As per the spec, an endpoint could allocate the
buffer pair and use the FFA_RXTX_MAP interface to map it with the
Hypervisor(KVM here). However looking at the prototype and also
I remember you mentioning that it is not possible to manage buffers
in that way. Please confirm if you plan to add the buffer details
fetcthing them through ioctls in KVM and adding them to VM DT nodes
in KVM userspace. I will update the bindings accordingly.
2. Driver
---------
a. Support for multiple partitions in a VM
------------------------------------------
I am not sure if there is need for supporting multiple partitions
within a VM. It should be possible to do so as I expect to create
device for each partition entry under arm-psa-ffa devicetree node.
However, I don't want to assume something that will never be a
usecase. However I don't think this will change must of the
abstraction as we need to keep the interface API implementation
separate to support different partitions on various platforms.
b. SMCCC interface
------------------
This is something I messed up completely while trying to add
support for SMCCC v1.2. It now supports x0-x17 as parameter
registers(input) and return registers(output). I started simple
with x0-x7 as both input and output as PSA FF-A needs that at
most. But extending to x0-x17 then became with messy in my
implementation. That's the reason I dropped it completely
here and thought of checking it first.
Do we need to extend the optimisations that were done to handle
ARCH_WORKAROUND_{1,2}. Or should be just use a version with x0-x7
as both input and ouput. Hyper-V guys need full x0-x17 support.
I need some guidance as what is the approach preferred ?
3. Partitions
-------------
I am not sure if we have a full define partition that we plan to
push upstream. Without one, we can have a sample/example partition
to test all the interface APIs, but is that fine with respect to
what we want upstream ? Any other thoughts that helps to test the
driver ?
Sorry for long email and too many questions, but I thought it is easier
this way to begin with than throwing huge code implementing loads of APIs
with no users(expect example partition) especially that I am posting this
during merge window.
Sudeep Holla (3):
dt-bindings: Add ARM PSA FF binding for non-secure VM partitions
firmware: Add support for PSA FF-A transport for VM partitions
firmware: Add example PSA FF-A non-secure VM partition
.../devicetree/bindings/arm/arm,psa-ffa.txt | 47 ++++
drivers/firmware/Kconfig | 1 +
drivers/firmware/Makefile | 1 +
drivers/firmware/arm_psa_ffa/Kconfig | 22 ++
drivers/firmware/arm_psa_ffa/Makefile | 3 +
drivers/firmware/arm_psa_ffa/driver.c | 250 ++++++++++++++++++
drivers/firmware/arm_psa_ffa/partition.c | 71 +++++
include/linux/arm_psa_ffa.h | 42 +++
8 files changed, 437 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/arm,psa-ffa.txt
create mode 100644 drivers/firmware/arm_psa_ffa/Kconfig
create mode 100644 drivers/firmware/arm_psa_ffa/Makefile
create mode 100644 drivers/firmware/arm_psa_ffa/driver.c
create mode 100644 drivers/firmware/arm_psa_ffa/partition.c
create mode 100644 include/linux/arm_psa_ffa.h
--
2.17.1
^ permalink raw reply
* RE: [PATCH V3 2/3] arm64: dts: imx8m: add mu node
From: Aisheng Dong @ 2020-06-01 9:44 UTC (permalink / raw)
To: Peng Fan, shawnguo@kernel.org, Fabio Estevam,
kernel@pengutronix.de, robh+dt@kernel.org, sboyd@kernel.org,
linux@rempel-privat.de, jaswinder.singh@linaro.org
Cc: linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, dl-linux-imx, Leonard Crestez,
Daniel Baluta, l.stach@pengutronix.de, devicetree@vger.kernel.org,
linux-clk@vger.kernel.org
In-Reply-To: <1590999602-29482-3-git-send-email-peng.fan@nxp.com>
> From: Peng Fan <peng.fan@nxp.com>
> Sent: Monday, June 1, 2020 4:20 PM
>
> Add mu node to let A53 could communicate with M Core.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
Regards
Aisheng
^ permalink raw reply
* Re: [RFC PATCH v5 1/6] dt-bindings: exynos-bus: Add documentation for interconnect properties
From: Sylwester Nawrocki @ 2020-06-01 9:40 UTC (permalink / raw)
To: Chanwoo Choi
Cc: Georgi Djakov, Chanwoo Choi, Krzysztof Kozlowski,
Artur Świgoń, MyungJoo Ham, inki.dae, Seung-Woo Kim,
Bartlomiej Zolnierkiewicz, Marek Szyprowski, linux-kernel,
linux-samsung-soc, dri-devel, linux-arm-kernel, Rob Herring,
devicetree
In-Reply-To: <CAGTfZH1yM0KRaEF5VTs2juTm+yrK9VqQZxWjdNf_ffjGHWPLsg@mail.gmail.com>
Hi Chanwoo,
On 31.05.2020 02:01, Chanwoo Choi wrote:
> On Sat, May 30, 2020 at 1:32 AM Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>>
>> Add documentation for new optional properties in the exynos bus nodes:
>> samsung,interconnect-parent, #interconnect-cells.
>> These properties allow to specify the SoC interconnect structure which
>> then allows the interconnect consumer devices to request specific
>> bandwidth requirements.
>>
>> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> ---
>> Changes for v5:
>> - exynos,interconnect-parent-node renamed to samsung,interconnect-parent
>> ---
>> Documentation/devicetree/bindings/devfreq/exynos-bus.txt | 15 +++++++++++++--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>> index e71f752..e0d2daa 100644
>> --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>> @@ -51,6 +51,11 @@ Optional properties only for parent bus device:
>> - exynos,saturation-ratio: the percentage value which is used to calibrate
>> the performance count against total cycle count.
>>
>> +Optional properties for interconnect functionality (QoS frequency constraints):
>> +- samsung,interconnect-parent: phandle to the parent interconnect node; for
>> + passive devices should point to same node as the exynos,parent-bus property.
>> +- #interconnect-cells: should be 0
>> +
>> Detailed correlation between sub-blocks and power line according to Exynos SoC:
>> - In case of Exynos3250, there are two power line as following:
>> VDD_MIF |--- DMC
>> @@ -185,8 +190,9 @@ Example1:
>> ----------------------------------------------------------
>>
>> Example2 :
>> - The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi
>> - is listed below:
>> + The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi is
>> + listed below. An interconnect path "bus_lcd0 -- bus_leftbus -- bus_dmc"
>> + is defined for demonstration purposes.
>>
>> bus_dmc: bus_dmc {
>> compatible = "samsung,exynos-bus";
>> @@ -376,12 +382,15 @@ Example2 :
>> &bus_dmc {
>> devfreq-events = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>;
>> vdd-supply = <&buck1_reg>; /* VDD_MIF */
>> + #interconnect-cells = <0>;
>> status = "okay";
>> };
>>
>> &bus_leftbus {
>> devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
>> vdd-supply = <&buck3_reg>;
>> + samsung,interconnect-parent = <&bus_dmc>;
>> + #interconnect-cells = <0>;
>> status = "okay";
>> };
>>
>> @@ -392,6 +401,8 @@ Example2 :
>>
>> &bus_lcd0 {
>> devfreq = <&bus_leftbus>;
>> + samsung,interconnect-parent = <&bus_leftbus>;
>> + #interconnect-cells = <0>;
>> status = "okay";
>> };
>>
>> --
>> 2.7.4
>>
>
> If you add the usage example like the mixer device of patch5 to this
> dt-binding document,
> I think it is very beneficial and more helpful for user of
> exynos-bus/exynos-generic-icc.
>
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
Thanks for review. I will make sure the example includes a consumer
in next version. Will also mention ../interconnect/interconnect.txt
in description of the #interconnect-cells property.
--
Regards,
Sylwester
^ permalink raw reply
* Re: [PATCH v25 03/16] dt: bindings: lp50xx: Introduce the lp50xx family of RGB drivers
From: Jacek Anaszewski @ 2020-06-01 9:34 UTC (permalink / raw)
To: Pavel Machek, Dan Murphy; +Cc: robh, devicetree, linux-leds, linux-kernel
In-Reply-To: <20200531190625.GA30537@duo.ucw.cz>
Hi Pavel and Dan,
On 5/31/20 9:06 PM, Pavel Machek wrote:
> Hi!
>
>>>> + There can only be one instance of the ti,led-bank
>>>> + property for each device node. This is a required node is the LED
>>>> + modules are to be backed.
>>> I don't understand the second sentence. Pretty sure it is not valid
>>> english.
>>
>>
>> If I make these changes is this still viable for 5.8 or would you then go
>> into 5.9?
>
> It really depends if we get -rc8 or not, and if you'll need to do any
> changes to C code or not...
I think that we need to simmer such a big extension of the LED
subsystem for a whole cycle in linux-next, especially taking into
account addition of new sysfs interface, that is bit quirky.
Effectively 5.8 seems to not have been viable since few weeks.
--
Best regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support
From: Srinivas Kandagatla @ 2020-06-01 9:24 UTC (permalink / raw)
To: Doug Anderson
Cc: Ravi Kumar Bokka (Temp), Rob Herring, LKML,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rajendra Nayak, Sai Prakash Ranjan, dhavalp, mturney, sparate,
c_rbokka, mkurumel
In-Reply-To: <CAD=FV=UCwUO5aKrUj7e+v6Bpkh_O+wuSXD5tJHdGOfaVTL0t1w@mail.gmail.com>
On 26/05/2020 23:31, Doug Anderson wrote:
> Hi,
>
> On Fri, May 22, 2020 at 4:18 AM Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>>
>> On 21/05/2020 22:28, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Thu, May 21, 2020 at 8:56 AM Srinivas Kandagatla
>>> <srinivas.kandagatla@linaro.org> wrote:
>>>>
>>>> On 21/05/2020 16:10, Doug Anderson wrote:
>>>>>> On 20/05/2020 23:48, Doug Anderson wrote:
>>>>>>>> Is this only applicable for corrected address space?
>>>>>>> I guess I was proposing a two dts-node / two drive approach here.
>>>>>>>
>>>>>>> dts node #1:just covers the memory range for accessing the FEC-corrected data
>>>>>>> driver #1: read-only and reads the FEC-corrected data
>>>>>>>
>>>>>>> dts node #2: covers the memory range that's_not_ the FEC-corrected
>>>>>>> memory range.
>>>>>>> driver #2: read-write. reading reads uncorrected data
>>>>>>>
>>>>>>> Does that seem sane?
>>>>>> I see your point but it does not make sense to have two node for same thing.
>>>>> OK, so that sounds as if we want to go with the proposal where we
>>>>> "deprecate the old driver and/or bindings and say that there really
>>>>> should just be one node and one driver".
>>>>>
>>>>> Would this be acceptable to you?
>>>>>
>>>>> 1. Officially mark the old bindings as deprecated.
>>>>
>>>> Possibly Yes for some reasons below!
>>>>
>>>>>
>>>>> 2. Leave the old driver there to support the old deprecated bindings,
>>>>> at least until everyone can be transferred over. There seem to be
>>>>> quite a few existing users of "qcom,qfprom" and we're supposed to make
>>>>> an attempt at keeping the old device trees working, at least for a
>>>>> little while. Once everyone is transferred over we could decide to
>>>>> delete the old driver.
>>>> we could consider "qcom,qfrom" to be only passing corrected address
>>>> space. Till we transition users to new bindings!
>>>>
>>>>>
>>>> Yes.
>>>>
>>>>> 3. We will have a totally new driver here.
>>>> No, we should still be using the same driver. But the exiting driver
>>>> seems to incorrect and is need of fixing.
>>>>
>>>> Having a look at the memory map for old SoCs like msm8996 and msm8916
>>>> shows that memory map that was passed to qfprom driver is corrected
>>>> address space. Writes will not obviously work!
>>>>
>>>> This should also be true with sdm845 or sc7180
>>>>
>>>> That needs to be fixed first!
>>>
>>> OK, so to summarize:
>>>
>>
>>> 1. We will have one driver: "drivers/nvmem/qfprom.c"
>>
>> Yes, we should one driver for this because we are dealing with exactly
>> same IP.
>>
>>>
>>> 2. If the driver detects that its reg is pointing to the corrected
>>> address space then it should operate in read-only mode. Maybe it can
>>> do this based on the compatible string being just "qcom,qfprom" or
>>> maybe it can do this based on the size of the "reg".
>>
>> I found out that there is a version register at offset of 0x6000 which
>> can give MAJOR, MINOR and STEP numbers.
>>
>> So we could still potentially continue using "qcom,qfprom"
>
> OK, sounds good. I think it's still good practice to include both the
> SoC specific and the generic. Even if the driver never does anything
> with the SoC-specific compatible string it doesn't hurt to have it
> there. Thus, we'd want:
>
> compatible = "qcom,msm8996-qfprom", "qcom,qfprom"
>
> The driver itself would never need to refer to the SoC-specific name
> but that does give us more flexibility.
>
>
>> The address space can be split into 3 resources, which is inline with
>> Specs as well
>>
>> 1. Raw address space ("raw")
>> 2. Configuration address space ("conf" or "core")
>> 3. Corrected address space ("corrected")
>
> Sure, this is OK with me then. Originally Ravi had 3 ranges but then
> he was (in the driver) treating it as one range. As long as the
> driver truly treats it as 3 ranges I have no problem doing it like
> this.
>
> In general, over the years, there has been a push to keep
> implementation details out of the dts and rely more on the "of_match"
> table to add SoC-specific details. This becomes really important when
> 1 year down the road you realize that you need one more random
> property or address range to fix some bug and then you need to figure
> out how to try to keep old "dts" files compatible. It's not a
> hard-and-fast rule, though.
Am not 100% sure if "qcom,fuse-blow-frequency" is something integration
specific or SoC Specific, My idea was that this will give more
flexibility in future. As adding new SoC Support does not need driver
changes.
Having said that, Am okay either way!
Incase we go compatible way, I would like to see compatible strings
having proper IP versions to have ip version rather than SoC names.
Having SoC names in compatible string means both driver and bindings
need update for every new SoC which can be overhead very soon!
Rob can help review once we have v2 bindings out!
>
>
>> Exiting qfprom entries or read-only qfprom will have "corrected"
>> address space which can be the only resource provided by device tree
>> entries.
>> Other two entries("raw" and "conf") are optional.
>>
>> qfprom: qfprom@780000 {
>> compatible = "qcom,qfprom";
>> reg = <0 0x00780000 0 0x8ff>,
>> <0 0x00782000 0 0x100>,
>> <0 0x00784000 0 0x8ff>;
>> reg-names = "raw", "conf", "corrected";
>>
>> vcc-supply = <&vreg_xyz>;
>>
>> clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
>> clock-names = "secclk";
>>
>> assigned-clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
>> assigned-clock-rates = <19200000>;
>>
>> qcom,fuse-blow-frequency = <4800000>
>>
>> #address-cells = <1>;
>> #size-cells = <1>;
>>
>> qusb2p_hstx_trim: hstx-trim-primary@25b {
>> reg = <0x25b 0x1>;
>> bits = <1 3>;
>> };
>> };
>>
>> Regarding clk rate setting, the default rate can be set using
>> assigned-clock-rates property, however the blow frequency can go as new
>> binding.
>> regarding voltage range for regulator, it should come as part of board
>> specific voltage regulator node. In worst case we can discuss on adding
>> new bindings for allowing specific range.
>
> I'd up to you (and Rob H, who probably will wait for the next rev of
> the binding before chiming in) but the "qcom,fuse-blow-frequency" is
> the type of property that feels better in the driver and achieved from
> the of_match table. Then you don't need to worry about adding it to
> the bindings. Voltage (if needed) would be similar, but I would hope
> we don't need it.
>
>
>> for Older SoCs: we still continue to use old style with just one
>> resource corresponding to corrected by default.
>>
>> qfprom: qfprom@784000 {
>> compatible = "qcom,qfprom";
>> reg = <0 0x00784000 0 0x8ff>;
>> #address-cells = <1>;
>> #size-cells = <1>;
>>
>> qusb2p_hstx_trim: hstx-trim-primary@1eb {
>> reg = <0x1eb 0x1>;
>> bits = <1 4>;
>> };
>>
>> qusb2s_hstx_trim: hstx-trim-secondary@1eb {
>> reg = <0x1eb 0x2>;
>> bits = <6 4>;
>> };
>> };
>>
>>
>> I see the patch as adding write support to qfprom, rather than adding
>> new driver or new SoC support.
>>
>> This in summary should give us good direction for this patch!
>>
>> Correct me if I miss understood something here!
>
> Sounds sane to me.
Cool! lets see how v2 will endup like!
--srini
>
>
> -Doug
>
^ permalink raw reply
* Re: [Linux-stm32] [PATCH v8 08/10] drm: stm: dw-mipi-dsi: let the bridge handle the HW version check
From: Adrian Ratiu @ 2020-06-01 9:15 UTC (permalink / raw)
To: Philippe CORNU, Adrian Ratiu,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
linux-rockchip@lists.infradead.org, Laurent Pinchart
Cc: Jernej Skrabec, Heiko Stuebner, Adrian Pop, Jonas Karlman,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
Andrzej Hajda, linux-imx@nxp.com, kernel@collabora.com,
linux-stm32@st-md-mailman.stormreply.com, Arnaud Ferraris,
Yannick FERTRE, Benjamin GAIGNARD
In-Reply-To: <4acc09e8-0610-01f6-b18d-3ffc390c45a3@st.com>
On Fri, 29 May 2020, Philippe CORNU <philippe.cornu@st.com> wrote:
> Hi Adrian, and thank you very much for the patchset. Thank you
> also for having tested it on STM32F769 and STM32MP1. Sorry for
> the late response, Yannick and I will review it as soon as
> possible and we will keep you posted. Note: Do not hesitate to
> put us in copy for the next version (philippe.cornu@st.com,
> yannick.fertre@st.com) Regards, Philippe :-)
Hi Philippe,
Thank you very much for your previous and future STM testing,
really appreciate it! I've CC'd Yannick until now but I'll also CC
you sure :)
It's been over a month since I posted v8 and I was just gearing up
to address all feedback, rebase & retest to prepare v9 but I'll
wait a little longer, no problem, it's no rush.
Have an awesome day,
Adrian
>
>
> On 4/27/20 10:19 AM, Adrian Ratiu wrote:
>> The stm mipi-dsi platform driver added a version test in
>> commit fa6251a747b7 ("drm/stm: dsi: check hardware version")
>> so that HW revisions other than v1.3x get rejected. The rockchip
>> driver had no such check and just assumed register layouts are
>> v1.3x compatible.
>>
>> Having such tests was a good idea because only v130/v131 layouts
>> were supported at the time, however since adding multiple layout
>> support in the bridge, the version is automatically checked for
>> all drivers, compatible layouts get picked and unsupported HW is
>> automatically rejected by the bridge, so there's no use keeping
>> the test in the stm driver.
>>
>> The main reason prompting this change is that the stm driver
>> test immediately disabled the peripheral clock after reading
>> the version, making the bridge read version 0x0 immediately
>> after in its own probe(), so we move the clock disabling after
>> the bridge does the version test.
>>
>> Tested on STM32F769 and STM32MP1.
>>
>> Cc: linux-stm32@st-md-mailman.stormreply.com
>> Reported-by: Adrian Pop <pop.adrian61@gmail.com>
>> Tested-by: Adrian Pop <pop.adrian61@gmail.com>
>> Tested-by: Arnaud Ferraris <arnaud.ferraris@collabora.com>
>> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
>> ---
>> New in v6.
>> ---
>> drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 12 +++---------
>> 1 file changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
>> index 2e1f2664495d0..7218e405d7e2b 100644
>> --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
>> +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
>> @@ -402,15 +402,6 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
>> goto err_dsi_probe;
>> }
>>
>> - dsi->hw_version = dsi_read(dsi, DSI_VERSION) & VERSION;
>> - clk_disable_unprepare(pclk);
>> -
>> - if (dsi->hw_version != HWVER_130 && dsi->hw_version != HWVER_131) {
>> - ret = -ENODEV;
>> - DRM_ERROR("bad dsi hardware version\n");
>> - goto err_dsi_probe;
>> - }
>> -
>> dw_mipi_dsi_stm_plat_data.base = dsi->base;
>> dw_mipi_dsi_stm_plat_data.priv_data = dsi;
>>
>> @@ -423,6 +414,9 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
>> goto err_dsi_probe;
>> }
>>
>> + dsi->hw_version = dsi_read(dsi, DSI_VERSION) & VERSION;
>> + clk_disable_unprepare(pclk);
>> +
>> return 0;
>>
>> err_dsi_probe:
>>
^ permalink raw reply
* Re: [PATCH v6 07/11] i2c: designware: Discard Cherry Trail model flag
From: Jarkko Nikula @ 2020-06-01 8:54 UTC (permalink / raw)
To: Andy Shevchenko, Serge Semin
Cc: Wolfram Sang, Mika Westerberg, Serge Semin, Alexey Malahov,
Thomas Bogendoerfer, Rob Herring, linux-mips, devicetree,
linux-i2c, linux-kernel
In-Reply-To: <20200528100635.GH1634618@smile.fi.intel.com>
On 5/28/20 1:06 PM, Andy Shevchenko wrote:
> On Thu, May 28, 2020 at 12:33:17PM +0300, Serge Semin wrote:
>> A PM workaround activated by the flag MODEL_CHERRYTRAIL has been removed
>> since commit 9cbeeca05049 ("i2c: designware: Remove Cherry Trail PMIC I2C
>> bus pm_disabled workaround"), but the flag most likely by mistake has been
>> left in the Dw I2C drivers. Let's remove it. Since MODEL_MSCC_OCELOT is
>> the only model-flag left, redefine it to be 0x100 so setting a very first
>> bit in the MODEL_MASK bits range.
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Conditionally, in case Wolfram and Jarkko are fine with shuffling model
> defines, which I consider an unneeded churn.
>
>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
>> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
I completely agree with Andy and more over, I didn't ack this version.
This was the version I acked:
https://www.spinics.net/lists/linux-i2c/msg45492.html
So in general please drop the received tags in case you decide to modify
patch since reviewers may not agree anymore with it.
That said, I'm fine with that minor code and commit log change. And
don't want to have yet another patchset version because it. Four
patchset versions during 2 days is a bit too much... Usual
recommendation is to wait for 1 week before posting a new version
especially if patchset is under active discussion.
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
^ permalink raw reply
* [PATCH v1 6/6] drm: panel-simple: add LOGIC Technologies panels
From: Sam Ravnborg @ 2020-06-01 8:33 UTC (permalink / raw)
To: dri-devel, devicetree
Cc: Sam Ravnborg, Søren Andersen, Thierry Reding,
Ville Syrjälä, Douglas Anderson, Bjorn Andersson,
Sebastian Reichel
In-Reply-To: <20200601083309.712606-1-sam@ravnborg.org>
Add support for the following panels from LOGIC Technologies, Inc:
- LTTD800480070-L2RT
- LTTD800480070-L6WH-RT
The LTTD800480 L2RT is discontinued, but it may be used
by existing products.
Signed-off-by: Søren Andersen <san@skov.dk>
Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Søren Andersen <san@skov.dk>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
drivers/gpu/drm/panel/panel-simple.c | 70 ++++++++++++++++++++++++++++
1 file changed, 70 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 25c96639631f..70d54164b1ae 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -2428,6 +2428,70 @@ static const struct panel_desc logictechno_lt170410_2whc = {
.connector_type = DRM_MODE_CONNECTOR_LVDS,
};
+static const struct drm_display_mode logictechno_lttd800480070_l2rt_mode = {
+ .clock = 33000,
+ .hdisplay = 800,
+ .hsync_start = 800 + 112,
+ .hsync_end = 800 + 112 + 3,
+ .htotal = 800 + 112 + 3 + 85,
+ .vdisplay = 480,
+ .vsync_start = 480 + 38,
+ .vsync_end = 480 + 38 + 3,
+ .vtotal = 480 + 38 + 3 + 29,
+ .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
+};
+
+static const struct panel_desc logictechno_lttd800480070_l2rt = {
+ .modes = &logictechno_lttd800480070_l2rt_mode,
+ .num_modes = 1,
+ .bpc = 8,
+ .size = {
+ .width = 154,
+ .height = 86,
+ },
+ .delay = {
+ .prepare = 45,
+ .enable = 100,
+ .disable = 100,
+ .unprepare = 45
+ },
+ .bus_format = MEDIA_BUS_FMT_RGB888_1X24,
+ .bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+ .connector_type = DRM_MODE_CONNECTOR_DPI,
+};
+
+static const struct drm_display_mode logictechno_lttd800480070_l6wh_rt_mode = {
+ .clock = 33000,
+ .hdisplay = 800,
+ .hsync_start = 800 + 154,
+ .hsync_end = 800 + 154 + 3,
+ .htotal = 800 + 154 + 3 + 43,
+ .vdisplay = 480,
+ .vsync_start = 480 + 47,
+ .vsync_end = 480 + 47 + 3,
+ .vtotal = 480 + 47 + 3 + 20,
+ .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
+};
+
+static const struct panel_desc logictechno_lttd800480070_l6wh_rt = {
+ .modes = &logictechno_lttd800480070_l6wh_rt_mode,
+ .num_modes = 1,
+ .bpc = 8,
+ .size = {
+ .width = 154,
+ .height = 86,
+ },
+ .delay = {
+ .prepare = 45,
+ .enable = 100,
+ .disable = 100,
+ .unprepare = 45
+ },
+ .bus_format = MEDIA_BUS_FMT_RGB888_1X24,
+ .bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+ .connector_type = DRM_MODE_CONNECTOR_DPI,
+};
+
static const struct drm_display_mode mitsubishi_aa070mc01_mode = {
.clock = 30400,
.hdisplay = 800,
@@ -3841,6 +3905,12 @@ static const struct of_device_id platform_of_match[] = {
}, {
.compatible = "logictechno,lt170410-2whc",
.data = &logictechno_lt170410_2whc,
+ }, {
+ .compatible = "logictechno,lttd800480070-l2rt",
+ .data = &logictechno_lttd800480070_l2rt,
+ }, {
+ .compatible = "logictechno,lttd800480070-l6wh-rt",
+ .data = &logictechno_lttd800480070_l6wh_rt,
}, {
.compatible = "mitsubishi,aa070mc01-ca1",
.data = &mitsubishi_aa070mc01,
--
2.25.1
^ permalink raw reply related
* [PATCH v1 5/6] dt-bindings: panel: add LOGIC Technologies panels
From: Sam Ravnborg @ 2020-06-01 8:33 UTC (permalink / raw)
To: dri-devel, devicetree
Cc: Sam Ravnborg, Søren Andersen, Thierry Reding,
Ville Syrjälä, Douglas Anderson, Bjorn Andersson,
Sebastian Reichel
In-Reply-To: <20200601083309.712606-1-sam@ravnborg.org>
Add support for the following panels from LOGIC Technologies, Inc:
- LTTD800480070-L2RT
- LTTD800480070-L6WH-RT
The LTTD800480 L2RT is discontinued, but it may be used in
existing products.
Both panels are dumb panels that matches the panel-simple binding.
Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Søren Andersen <san@skov.dk>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
.../devicetree/bindings/display/panel/panel-simple.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
index 6fe0ac86696d..a4910d4af96b 100644
--- a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
+++ b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
@@ -183,6 +183,10 @@ properties:
- logictechno,lt161010-2nhr
# Logic Technologies LT170410-2WHC 10.1" 1280x800 IPS TFT Cap Touch Mod.
- logictechno,lt170410-2whc
+ # LOGIC Technologies Inc. LTTD800480070-L2RT 7" (800x480) TFT LCD panel
+ - logictechno,lttd800480070-l2rt
+ # LOGIC Technologies Inc. LTTD800480070-L6WH-RT 7" (800x480) TFT LCD panel
+ - logictechno,lttd800480070-l6wh-rt
# Mitsubishi "AA070MC01 7.0" WVGA TFT LCD panel
- mitsubishi,aa070mc01-ca1
# NEC LCD Technologies, Ltd. 12.1" WXGA (1280x800) LVDS TFT LCD panel
--
2.25.1
^ 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