* [PATCH v6 8/9] counter: stm32-lptimer: add counter device
From: William Breathitt Gray @ 2018-05-16 17:52 UTC (permalink / raw)
To: jic23
Cc: benjamin.gaignard, fabrice.gasnier, linux-iio, linux-kernel,
devicetree, linux-arm-kernel, William Breathitt Gray
In-Reply-To: <cover.1526487615.git.vilhelm.gray@gmail.com>
From: Fabrice Gasnier <fabrice.gasnier@st.com>
Add support for new counter device to stm32-lptimer.
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
.../{iio => }/counter/stm32-lptimer-cnt.txt | 0
.../devicetree/bindings/mfd/stm32-lptimer.txt | 2 +-
drivers/counter/Kconfig | 10 +
drivers/counter/Makefile | 1 +
drivers/counter/stm32-lptimer-cnt.c | 722 ++++++++++++++++++
drivers/iio/counter/Kconfig | 9 -
drivers/iio/counter/Makefile | 2 -
drivers/iio/counter/stm32-lptimer-cnt.c | 382 ---------
8 files changed, 734 insertions(+), 394 deletions(-)
rename Documentation/devicetree/bindings/{iio => }/counter/stm32-lptimer-cnt.txt (100%)
create mode 100644 drivers/counter/stm32-lptimer-cnt.c
delete mode 100644 drivers/iio/counter/stm32-lptimer-cnt.c
diff --git a/Documentation/devicetree/bindings/iio/counter/stm32-lptimer-cnt.txt b/Documentation/devicetree/bindings/counter/stm32-lptimer-cnt.txt
similarity index 100%
rename from Documentation/devicetree/bindings/iio/counter/stm32-lptimer-cnt.txt
rename to Documentation/devicetree/bindings/counter/stm32-lptimer-cnt.txt
diff --git a/Documentation/devicetree/bindings/mfd/stm32-lptimer.txt b/Documentation/devicetree/bindings/mfd/stm32-lptimer.txt
index 2a9ff29db9c9..fb54e4dad5b3 100644
--- a/Documentation/devicetree/bindings/mfd/stm32-lptimer.txt
+++ b/Documentation/devicetree/bindings/mfd/stm32-lptimer.txt
@@ -16,7 +16,7 @@ Required properties:
Optional subnodes:
- pwm: See ../pwm/pwm-stm32-lp.txt
-- counter: See ../iio/timer/stm32-lptimer-cnt.txt
+- counter: See ../counter/stm32-lptimer-cnt.txt
- trigger: See ../iio/timer/stm32-lptimer-trigger.txt
Example:
diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index 90b698aa52cf..196d210d4e19 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig
@@ -46,4 +46,14 @@ config STM32_TIMER_CNT
To compile this driver as a module, choose M here: the
module will be called stm32-timer-cnt.
+config STM32_LPTIMER_CNT
+ tristate "STM32 LP Timer encoder counter driver"
+ depends on (MFD_STM32_LPTIMER || COMPILE_TEST) && IIO
+ help
+ Select this option to enable STM32 Low-Power Timer quadrature encoder
+ and counter driver.
+
+ To compile this driver as a module, choose M here: the
+ module will be called stm32-lptimer-cnt.
+
endif # COUNTER
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index ff024259fb46..8ff19b32022b 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -9,3 +9,4 @@ counter-y := generic-counter.o
obj-$(CONFIG_104_QUAD_8) += 104-quad-8.o
obj-$(CONFIG_STM32_TIMER_CNT) += stm32-timer-cnt.o
+obj-$(CONFIG_STM32_LPTIMER_CNT) += stm32-lptimer-cnt.o
diff --git a/drivers/counter/stm32-lptimer-cnt.c b/drivers/counter/stm32-lptimer-cnt.c
new file mode 100644
index 000000000000..c2c0b19320cf
--- /dev/null
+++ b/drivers/counter/stm32-lptimer-cnt.c
@@ -0,0 +1,722 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * STM32 Low-Power Timer Encoder and Counter driver
+ *
+ * Copyright (C) STMicroelectronics 2017
+ *
+ * Author: Fabrice Gasnier <fabrice.gasnier@st.com>
+ *
+ * Inspired by 104-quad-8 and stm32-timer-trigger drivers.
+ *
+ */
+
+#include <linux/bitfield.h>
+#include <linux/counter.h>
+#include <linux/iio/iio.h>
+#include <linux/mfd/stm32-lptimer.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+struct stm32_lptim_cnt {
+ struct counter_device counter;
+ struct device *dev;
+ struct regmap *regmap;
+ struct clk *clk;
+ u32 ceiling;
+ u32 polarity;
+ u32 quadrature_mode;
+};
+
+static int stm32_lptim_is_enabled(struct stm32_lptim_cnt *priv)
+{
+ u32 val;
+ int ret;
+
+ ret = regmap_read(priv->regmap, STM32_LPTIM_CR, &val);
+ if (ret)
+ return ret;
+
+ return FIELD_GET(STM32_LPTIM_ENABLE, val);
+}
+
+static int stm32_lptim_set_enable_state(struct stm32_lptim_cnt *priv,
+ int enable)
+{
+ int ret;
+ u32 val;
+
+ val = FIELD_PREP(STM32_LPTIM_ENABLE, enable);
+ ret = regmap_write(priv->regmap, STM32_LPTIM_CR, val);
+ if (ret)
+ return ret;
+
+ if (!enable) {
+ clk_disable(priv->clk);
+ return 0;
+ }
+
+ /* LP timer must be enabled before writing CMP & ARR */
+ ret = regmap_write(priv->regmap, STM32_LPTIM_ARR, priv->ceiling);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(priv->regmap, STM32_LPTIM_CMP, 0);
+ if (ret)
+ return ret;
+
+ /* ensure CMP & ARR registers are properly written */
+ ret = regmap_read_poll_timeout(priv->regmap, STM32_LPTIM_ISR, val,
+ (val & STM32_LPTIM_CMPOK_ARROK),
+ 100, 1000);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(priv->regmap, STM32_LPTIM_ICR,
+ STM32_LPTIM_CMPOKCF_ARROKCF);
+ if (ret)
+ return ret;
+
+ ret = clk_enable(priv->clk);
+ if (ret) {
+ regmap_write(priv->regmap, STM32_LPTIM_CR, 0);
+ return ret;
+ }
+
+ /* Start LP timer in continuous mode */
+ return regmap_update_bits(priv->regmap, STM32_LPTIM_CR,
+ STM32_LPTIM_CNTSTRT, STM32_LPTIM_CNTSTRT);
+}
+
+static int stm32_lptim_setup(struct stm32_lptim_cnt *priv, int enable)
+{
+ u32 mask = STM32_LPTIM_ENC | STM32_LPTIM_COUNTMODE |
+ STM32_LPTIM_CKPOL | STM32_LPTIM_PRESC;
+ u32 val;
+
+ /* Setup LP timer encoder/counter and polarity, without prescaler */
+ if (priv->quadrature_mode)
+ val = enable ? STM32_LPTIM_ENC : 0;
+ else
+ val = enable ? STM32_LPTIM_COUNTMODE : 0;
+ val |= FIELD_PREP(STM32_LPTIM_CKPOL, enable ? priv->polarity : 0);
+
+ return regmap_update_bits(priv->regmap, STM32_LPTIM_CFGR, mask, val);
+}
+
+static int stm32_lptim_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_ENABLE:
+ if (val < 0 || val > 1)
+ return -EINVAL;
+
+ /* Check nobody uses the timer, or already disabled/enabled */
+ ret = stm32_lptim_is_enabled(priv);
+ if ((ret < 0) || (!ret && !val))
+ return ret;
+ if (val && ret)
+ return -EBUSY;
+
+ ret = stm32_lptim_setup(priv, val);
+ if (ret)
+ return ret;
+ return stm32_lptim_set_enable_state(priv, val);
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int stm32_lptim_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
+ u32 dat;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = regmap_read(priv->regmap, STM32_LPTIM_CNT, &dat);
+ if (ret)
+ return ret;
+ *val = dat;
+ return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_ENABLE:
+ ret = stm32_lptim_is_enabled(priv);
+ if (ret < 0)
+ return ret;
+ *val = ret;
+ return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_SCALE:
+ /* Non-quadrature mode: scale = 1 */
+ *val = 1;
+ *val2 = 0;
+ if (priv->quadrature_mode) {
+ /*
+ * Quadrature encoder mode:
+ * - both edges, quarter cycle, scale is 0.25
+ * - either rising/falling edge scale is 0.5
+ */
+ if (priv->polarity > 1)
+ *val2 = 2;
+ else
+ *val2 = 1;
+ }
+ return IIO_VAL_FRACTIONAL_LOG2;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info stm32_lptim_cnt_iio_info = {
+ .read_raw = stm32_lptim_read_raw,
+ .write_raw = stm32_lptim_write_raw,
+};
+
+static const char *const stm32_lptim_quadrature_modes[] = {
+ "non-quadrature",
+ "quadrature",
+};
+
+static int stm32_lptim_get_quadrature_mode(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
+
+ return priv->quadrature_mode;
+}
+
+static int stm32_lptim_set_quadrature_mode(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ unsigned int type)
+{
+ struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
+
+ if (stm32_lptim_is_enabled(priv))
+ return -EBUSY;
+
+ priv->quadrature_mode = type;
+
+ return 0;
+}
+
+static const struct iio_enum stm32_lptim_quadrature_mode_en = {
+ .items = stm32_lptim_quadrature_modes,
+ .num_items = ARRAY_SIZE(stm32_lptim_quadrature_modes),
+ .get = stm32_lptim_get_quadrature_mode,
+ .set = stm32_lptim_set_quadrature_mode,
+};
+
+static const char * const stm32_lptim_cnt_polarity[] = {
+ "rising-edge", "falling-edge", "both-edges",
+};
+
+static int stm32_lptim_cnt_get_polarity(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
+
+ return priv->polarity;
+}
+
+static int stm32_lptim_cnt_set_polarity(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ unsigned int type)
+{
+ struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
+
+ if (stm32_lptim_is_enabled(priv))
+ return -EBUSY;
+
+ priv->polarity = type;
+
+ return 0;
+}
+
+static const struct iio_enum stm32_lptim_cnt_polarity_en = {
+ .items = stm32_lptim_cnt_polarity,
+ .num_items = ARRAY_SIZE(stm32_lptim_cnt_polarity),
+ .get = stm32_lptim_cnt_get_polarity,
+ .set = stm32_lptim_cnt_set_polarity,
+};
+
+static ssize_t stm32_lptim_cnt_get_ceiling(struct stm32_lptim_cnt *priv,
+ char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "%u\n", priv->ceiling);
+}
+
+static ssize_t stm32_lptim_cnt_set_ceiling(struct stm32_lptim_cnt *priv,
+ const char *buf, size_t len)
+{
+ int ret;
+
+ if (stm32_lptim_is_enabled(priv))
+ return -EBUSY;
+
+ ret = kstrtouint(buf, 0, &priv->ceiling);
+ if (ret)
+ return ret;
+
+ if (priv->ceiling > STM32_LPTIM_MAX_ARR)
+ return -EINVAL;
+
+ return len;
+}
+
+static ssize_t stm32_lptim_cnt_get_preset_iio(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ char *buf)
+{
+ struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
+
+ return stm32_lptim_cnt_get_ceiling(priv, buf);
+}
+
+static ssize_t stm32_lptim_cnt_set_preset_iio(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ const char *buf, size_t len)
+{
+ struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
+
+ return stm32_lptim_cnt_set_ceiling(priv, buf, len);
+}
+
+/* LP timer with encoder */
+static const struct iio_chan_spec_ext_info stm32_lptim_enc_ext_info[] = {
+ {
+ .name = "preset",
+ .shared = IIO_SEPARATE,
+ .read = stm32_lptim_cnt_get_preset_iio,
+ .write = stm32_lptim_cnt_set_preset_iio,
+ },
+ IIO_ENUM("polarity", IIO_SEPARATE, &stm32_lptim_cnt_polarity_en),
+ IIO_ENUM_AVAILABLE("polarity", &stm32_lptim_cnt_polarity_en),
+ IIO_ENUM("quadrature_mode", IIO_SEPARATE,
+ &stm32_lptim_quadrature_mode_en),
+ IIO_ENUM_AVAILABLE("quadrature_mode", &stm32_lptim_quadrature_mode_en),
+ {}
+};
+
+static const struct iio_chan_spec stm32_lptim_enc_channels = {
+ .type = IIO_COUNT,
+ .channel = 0,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_ENABLE) |
+ BIT(IIO_CHAN_INFO_SCALE),
+ .ext_info = stm32_lptim_enc_ext_info,
+ .indexed = 1,
+};
+
+/* LP timer without encoder (counter only) */
+static const struct iio_chan_spec_ext_info stm32_lptim_cnt_ext_info[] = {
+ {
+ .name = "preset",
+ .shared = IIO_SEPARATE,
+ .read = stm32_lptim_cnt_get_preset_iio,
+ .write = stm32_lptim_cnt_set_preset_iio,
+ },
+ IIO_ENUM("polarity", IIO_SEPARATE, &stm32_lptim_cnt_polarity_en),
+ IIO_ENUM_AVAILABLE("polarity", &stm32_lptim_cnt_polarity_en),
+ {}
+};
+
+static const struct iio_chan_spec stm32_lptim_cnt_channels = {
+ .type = IIO_COUNT,
+ .channel = 0,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_ENABLE) |
+ BIT(IIO_CHAN_INFO_SCALE),
+ .ext_info = stm32_lptim_cnt_ext_info,
+ .indexed = 1,
+};
+
+/**
+ * stm32_lptim_cnt_function - enumerates stm32 LPTimer counter & encoder modes
+ * @STM32_LPTIM_COUNTER_INCREASE: up count on IN1 rising, falling or both edges
+ * @STM32_LPTIM_ENCODER_RISING: count on rising edge (IN1 & IN2 quadrature)
+ * @STM32_LPTIM_ENCODER_FALLING: count on falling edge (IN1 & IN2 quadrature)
+ * @STM32_LPTIM_ENCODER_BOTH_EDGE: count on both edges (IN1 & IN2 quadrature)
+ */
+enum stm32_lptim_cnt_function {
+ STM32_LPTIM_COUNTER_INCREASE,
+ STM32_LPTIM_ENCODER_RISING,
+ STM32_LPTIM_ENCODER_FALLING,
+ STM32_LPTIM_ENCODER_BOTH_EDGE,
+};
+
+static enum count_function stm32_lptim_cnt_functions[] = {
+ [STM32_LPTIM_COUNTER_INCREASE] = COUNT_FUNCTION_INCREASE,
+ [STM32_LPTIM_ENCODER_RISING] = COUNT_FUNCTION_QUADRATURE_X2_RISING,
+ [STM32_LPTIM_ENCODER_FALLING] = COUNT_FUNCTION_QUADRATURE_X2_FALLING,
+ [STM32_LPTIM_ENCODER_BOTH_EDGE] = COUNT_FUNCTION_QUADRATURE_X4,
+};
+
+enum stm32_lptim_synapse_action {
+ STM32_LPTIM_SYNAPSE_ACTION_RISING_EDGE,
+ STM32_LPTIM_SYNAPSE_ACTION_FALLING_EDGE,
+ STM32_LPTIM_SYNAPSE_ACTION_BOTH_EDGES,
+ STM32_LPTIM_SYNAPSE_ACTION_NONE,
+};
+
+static enum synapse_action stm32_lptim_cnt_synapse_actions[] = {
+ /* Index must match with stm32_lptim_cnt_polarity[] (priv->polarity) */
+ [STM32_LPTIM_SYNAPSE_ACTION_RISING_EDGE] = SYNAPSE_ACTION_RISING_EDGE,
+ [STM32_LPTIM_SYNAPSE_ACTION_FALLING_EDGE] = SYNAPSE_ACTION_FALLING_EDGE,
+ [STM32_LPTIM_SYNAPSE_ACTION_BOTH_EDGES] = SYNAPSE_ACTION_BOTH_EDGES,
+ [STM32_LPTIM_SYNAPSE_ACTION_NONE] = SYNAPSE_ACTION_NONE,
+};
+
+static int stm32_lptim_cnt_read(struct counter_device *counter,
+ struct counter_count *count,
+ struct count_read_value *val)
+{
+ struct stm32_lptim_cnt *const priv = counter->priv;
+ u32 cnt;
+ int ret;
+
+ ret = regmap_read(priv->regmap, STM32_LPTIM_CNT, &cnt);
+ if (ret)
+ return ret;
+
+ set_count_read_value(val, COUNT_POSITION_UNSIGNED, &cnt);
+
+ return 0;
+}
+
+static int stm32_lptim_cnt_function_get(struct counter_device *counter,
+ struct counter_count *count,
+ size_t *function)
+{
+ struct stm32_lptim_cnt *const priv = counter->priv;
+
+ if (!priv->quadrature_mode) {
+ *function = STM32_LPTIM_COUNTER_INCREASE;
+ return 0;
+ }
+
+ switch (priv->polarity) {
+ case STM32_LPTIM_SYNAPSE_ACTION_RISING_EDGE:
+ *function = STM32_LPTIM_ENCODER_RISING;
+ return 0;
+ case STM32_LPTIM_SYNAPSE_ACTION_FALLING_EDGE:
+ *function = STM32_LPTIM_ENCODER_FALLING;
+ return 0;
+ case STM32_LPTIM_SYNAPSE_ACTION_BOTH_EDGES:
+ *function = STM32_LPTIM_ENCODER_BOTH_EDGE;
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static int stm32_lptim_cnt_function_set(struct counter_device *counter,
+ struct counter_count *count,
+ size_t function)
+{
+ struct stm32_lptim_cnt *const priv = counter->priv;
+
+ if (stm32_lptim_is_enabled(priv))
+ return -EBUSY;
+
+ switch (function) {
+ case STM32_LPTIM_COUNTER_INCREASE:
+ priv->quadrature_mode = 0;
+ return 0;
+ case STM32_LPTIM_ENCODER_RISING:
+ priv->quadrature_mode = 1;
+ priv->polarity = STM32_LPTIM_SYNAPSE_ACTION_RISING_EDGE;
+ return 0;
+ case STM32_LPTIM_ENCODER_FALLING:
+ priv->quadrature_mode = 1;
+ priv->polarity = STM32_LPTIM_SYNAPSE_ACTION_FALLING_EDGE;
+ return 0;
+ case STM32_LPTIM_ENCODER_BOTH_EDGE:
+ priv->quadrature_mode = 1;
+ priv->polarity = STM32_LPTIM_SYNAPSE_ACTION_BOTH_EDGES;
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static ssize_t stm32_lptim_cnt_enable_read(struct counter_device *counter,
+ struct counter_count *count,
+ void *private, char *buf)
+{
+ struct stm32_lptim_cnt *const priv = counter->priv;
+ int ret;
+
+ ret = stm32_lptim_is_enabled(priv);
+ if (ret < 0)
+ return ret;
+
+ return scnprintf(buf, PAGE_SIZE, "%u\n", ret);
+}
+
+static ssize_t stm32_lptim_cnt_enable_write(struct counter_device *counter,
+ struct counter_count *count,
+ void *private,
+ const char *buf, size_t len)
+{
+ struct stm32_lptim_cnt *const priv = counter->priv;
+ bool enable;
+ int ret;
+
+ ret = kstrtobool(buf, &enable);
+ if (ret)
+ return ret;
+
+ /* Check nobody uses the timer, or already disabled/enabled */
+ ret = stm32_lptim_is_enabled(priv);
+ if ((ret < 0) || (!ret && !enable))
+ return ret;
+ if (enable && ret)
+ return -EBUSY;
+
+ ret = stm32_lptim_setup(priv, enable);
+ if (ret)
+ return ret;
+
+ ret = stm32_lptim_set_enable_state(priv, enable);
+ if (ret)
+ return ret;
+
+ return len;
+}
+
+static ssize_t stm32_lptim_cnt_ceiling_read(struct counter_device *counter,
+ struct counter_count *count,
+ void *private, char *buf)
+{
+ struct stm32_lptim_cnt *const priv = counter->priv;
+
+ return stm32_lptim_cnt_get_ceiling(priv, buf);
+}
+
+static ssize_t stm32_lptim_cnt_ceiling_write(struct counter_device *counter,
+ struct counter_count *count,
+ void *private,
+ const char *buf, size_t len)
+{
+ struct stm32_lptim_cnt *const priv = counter->priv;
+
+ return stm32_lptim_cnt_set_ceiling(priv, buf, len);
+}
+
+static const struct counter_count_ext stm32_lptim_cnt_ext[] = {
+ {
+ .name = "enable",
+ .read = stm32_lptim_cnt_enable_read,
+ .write = stm32_lptim_cnt_enable_write
+ },
+ {
+ .name = "ceiling",
+ .read = stm32_lptim_cnt_ceiling_read,
+ .write = stm32_lptim_cnt_ceiling_write
+ },
+};
+
+static int stm32_lptim_cnt_action_get(struct counter_device *counter,
+ struct counter_count *count,
+ struct counter_synapse *synapse,
+ size_t *action)
+{
+ struct stm32_lptim_cnt *const priv = counter->priv;
+ size_t function;
+ int err;
+
+ err = stm32_lptim_cnt_function_get(counter, count, &function);
+ if (err)
+ return err;
+
+ switch (function) {
+ case STM32_LPTIM_COUNTER_INCREASE:
+ /* LP Timer acts as up-counter on input 1 */
+ if (synapse->signal->id == count->synapses[0].signal->id)
+ *action = priv->polarity;
+ else
+ *action = STM32_LPTIM_SYNAPSE_ACTION_NONE;
+ return 0;
+ case STM32_LPTIM_ENCODER_RISING:
+ case STM32_LPTIM_ENCODER_FALLING:
+ case STM32_LPTIM_ENCODER_BOTH_EDGE:
+ *action = priv->polarity;
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static int stm32_lptim_cnt_action_set(struct counter_device *counter,
+ struct counter_count *count,
+ struct counter_synapse *synapse,
+ size_t action)
+{
+ struct stm32_lptim_cnt *const priv = counter->priv;
+ size_t function;
+ int err;
+
+ if (stm32_lptim_is_enabled(priv))
+ return -EBUSY;
+
+ err = stm32_lptim_cnt_function_get(counter, count, &function);
+ if (err)
+ return err;
+
+ /* only set polarity when in counter mode (on input 1) */
+ if (function == STM32_LPTIM_COUNTER_INCREASE
+ && synapse->signal->id == count->synapses[0].signal->id) {
+ switch (action) {
+ case STM32_LPTIM_SYNAPSE_ACTION_RISING_EDGE:
+ case STM32_LPTIM_SYNAPSE_ACTION_FALLING_EDGE:
+ case STM32_LPTIM_SYNAPSE_ACTION_BOTH_EDGES:
+ priv->polarity = action;
+ return 0;
+ }
+ }
+
+ return -EINVAL;
+}
+
+static const struct counter_ops stm32_lptim_cnt_ops = {
+ .count_read = stm32_lptim_cnt_read,
+ .function_get = stm32_lptim_cnt_function_get,
+ .function_set = stm32_lptim_cnt_function_set,
+ .action_get = stm32_lptim_cnt_action_get,
+ .action_set = stm32_lptim_cnt_action_set,
+};
+
+static struct counter_signal stm32_lptim_cnt_signals[] = {
+ {
+ .id = 0,
+ .name = "Channel 1 Quadrature A"
+ },
+ {
+ .id = 1,
+ .name = "Channel 1 Quadrature B"
+ }
+};
+
+static struct counter_synapse stm32_lptim_cnt_synapses[] = {
+ {
+ .actions_list = stm32_lptim_cnt_synapse_actions,
+ .num_actions = ARRAY_SIZE(stm32_lptim_cnt_synapse_actions),
+ .signal = &stm32_lptim_cnt_signals[0]
+ },
+ {
+ .actions_list = stm32_lptim_cnt_synapse_actions,
+ .num_actions = ARRAY_SIZE(stm32_lptim_cnt_synapse_actions),
+ .signal = &stm32_lptim_cnt_signals[1]
+ }
+};
+
+/* LP timer with encoder */
+static struct counter_count stm32_lptim_enc_counts = {
+ .id = 0,
+ .name = "LPTimer Count",
+ .functions_list = stm32_lptim_cnt_functions,
+ .num_functions = ARRAY_SIZE(stm32_lptim_cnt_functions),
+ .synapses = stm32_lptim_cnt_synapses,
+ .num_synapses = ARRAY_SIZE(stm32_lptim_cnt_synapses),
+ .ext = stm32_lptim_cnt_ext,
+ .num_ext = ARRAY_SIZE(stm32_lptim_cnt_ext)
+};
+
+/* LP timer without encoder (counter only) */
+static struct counter_count stm32_lptim_in1_counts = {
+ .id = 0,
+ .name = "LPTimer Count",
+ .functions_list = stm32_lptim_cnt_functions,
+ .num_functions = 1,
+ .synapses = stm32_lptim_cnt_synapses,
+ .num_synapses = 1,
+ .ext = stm32_lptim_cnt_ext,
+ .num_ext = ARRAY_SIZE(stm32_lptim_cnt_ext)
+};
+
+static int stm32_lptim_cnt_probe(struct platform_device *pdev)
+{
+ struct stm32_lptimer *ddata = dev_get_drvdata(pdev->dev.parent);
+ struct stm32_lptim_cnt *priv;
+ struct iio_dev *indio_dev;
+ int ret;
+
+ if (IS_ERR_OR_NULL(ddata))
+ return -EINVAL;
+
+ indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ priv = iio_priv(indio_dev);
+ priv->dev = &pdev->dev;
+ priv->regmap = ddata->regmap;
+ priv->clk = ddata->clk;
+ priv->ceiling = STM32_LPTIM_MAX_ARR;
+
+ /* Initialize IIO device */
+ indio_dev->name = dev_name(&pdev->dev);
+ indio_dev->dev.parent = &pdev->dev;
+ indio_dev->dev.of_node = pdev->dev.of_node;
+ indio_dev->info = &stm32_lptim_cnt_iio_info;
+ if (ddata->has_encoder)
+ indio_dev->channels = &stm32_lptim_enc_channels;
+ else
+ indio_dev->channels = &stm32_lptim_cnt_channels;
+ indio_dev->num_channels = 1;
+
+ /* Initialize Counter device */
+ priv->counter.name = dev_name(&pdev->dev);
+ priv->counter.parent = &pdev->dev;
+ priv->counter.ops = &stm32_lptim_cnt_ops;
+ if (ddata->has_encoder) {
+ priv->counter.counts = &stm32_lptim_enc_counts;
+ priv->counter.num_signals = ARRAY_SIZE(stm32_lptim_cnt_signals);
+ } else {
+ priv->counter.counts = &stm32_lptim_in1_counts;
+ priv->counter.num_signals = 1;
+ }
+ priv->counter.num_counts = 1;
+ priv->counter.signals = stm32_lptim_cnt_signals;
+ priv->counter.priv = priv;
+
+ platform_set_drvdata(pdev, priv);
+
+ ret = devm_iio_device_register(&pdev->dev, indio_dev);
+ if (ret)
+ return ret;
+
+ return devm_counter_register(&pdev->dev, &priv->counter);
+}
+
+static const struct of_device_id stm32_lptim_cnt_of_match[] = {
+ { .compatible = "st,stm32-lptimer-counter", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, stm32_lptim_cnt_of_match);
+
+static struct platform_driver stm32_lptim_cnt_driver = {
+ .probe = stm32_lptim_cnt_probe,
+ .driver = {
+ .name = "stm32-lptimer-counter",
+ .of_match_table = stm32_lptim_cnt_of_match,
+ },
+};
+module_platform_driver(stm32_lptim_cnt_driver);
+
+MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@st.com>");
+MODULE_ALIAS("platform:stm32-lptimer-counter");
+MODULE_DESCRIPTION("STMicroelectronics STM32 LPTIM counter driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/iio/counter/Kconfig b/drivers/iio/counter/Kconfig
index eeb358122cbe..95a7a0df6cac 100644
--- a/drivers/iio/counter/Kconfig
+++ b/drivers/iio/counter/Kconfig
@@ -5,13 +5,4 @@
menu "Counters"
-config STM32_LPTIMER_CNT
- tristate "STM32 LP Timer encoder counter driver"
- depends on MFD_STM32_LPTIMER || COMPILE_TEST
- help
- Select this option to enable STM32 Low-Power Timer quadrature encoder
- and counter driver.
-
- To compile this driver as a module, choose M here: the
- module will be called stm32-lptimer-cnt.
endmenu
diff --git a/drivers/iio/counter/Makefile b/drivers/iio/counter/Makefile
index 93933ba49280..8fd3d954775a 100644
--- a/drivers/iio/counter/Makefile
+++ b/drivers/iio/counter/Makefile
@@ -3,5 +3,3 @@
#
# When adding new entries keep the list in alphabetical order
-
-obj-$(CONFIG_STM32_LPTIMER_CNT) += stm32-lptimer-cnt.o
diff --git a/drivers/iio/counter/stm32-lptimer-cnt.c b/drivers/iio/counter/stm32-lptimer-cnt.c
deleted file mode 100644
index 42fb8ba67090..000000000000
--- a/drivers/iio/counter/stm32-lptimer-cnt.c
+++ /dev/null
@@ -1,382 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * STM32 Low-Power Timer Encoder and Counter driver
- *
- * Copyright (C) STMicroelectronics 2017
- *
- * Author: Fabrice Gasnier <fabrice.gasnier@st.com>
- *
- * Inspired by 104-quad-8 and stm32-timer-trigger drivers.
- *
- */
-
-#include <linux/bitfield.h>
-#include <linux/iio/iio.h>
-#include <linux/mfd/stm32-lptimer.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
-
-struct stm32_lptim_cnt {
- struct device *dev;
- struct regmap *regmap;
- struct clk *clk;
- u32 preset;
- u32 polarity;
- u32 quadrature_mode;
-};
-
-static int stm32_lptim_is_enabled(struct stm32_lptim_cnt *priv)
-{
- u32 val;
- int ret;
-
- ret = regmap_read(priv->regmap, STM32_LPTIM_CR, &val);
- if (ret)
- return ret;
-
- return FIELD_GET(STM32_LPTIM_ENABLE, val);
-}
-
-static int stm32_lptim_set_enable_state(struct stm32_lptim_cnt *priv,
- int enable)
-{
- int ret;
- u32 val;
-
- val = FIELD_PREP(STM32_LPTIM_ENABLE, enable);
- ret = regmap_write(priv->regmap, STM32_LPTIM_CR, val);
- if (ret)
- return ret;
-
- if (!enable) {
- clk_disable(priv->clk);
- return 0;
- }
-
- /* LP timer must be enabled before writing CMP & ARR */
- ret = regmap_write(priv->regmap, STM32_LPTIM_ARR, priv->preset);
- if (ret)
- return ret;
-
- ret = regmap_write(priv->regmap, STM32_LPTIM_CMP, 0);
- if (ret)
- return ret;
-
- /* ensure CMP & ARR registers are properly written */
- ret = regmap_read_poll_timeout(priv->regmap, STM32_LPTIM_ISR, val,
- (val & STM32_LPTIM_CMPOK_ARROK),
- 100, 1000);
- if (ret)
- return ret;
-
- ret = regmap_write(priv->regmap, STM32_LPTIM_ICR,
- STM32_LPTIM_CMPOKCF_ARROKCF);
- if (ret)
- return ret;
-
- ret = clk_enable(priv->clk);
- if (ret) {
- regmap_write(priv->regmap, STM32_LPTIM_CR, 0);
- return ret;
- }
-
- /* Start LP timer in continuous mode */
- return regmap_update_bits(priv->regmap, STM32_LPTIM_CR,
- STM32_LPTIM_CNTSTRT, STM32_LPTIM_CNTSTRT);
-}
-
-static int stm32_lptim_setup(struct stm32_lptim_cnt *priv, int enable)
-{
- u32 mask = STM32_LPTIM_ENC | STM32_LPTIM_COUNTMODE |
- STM32_LPTIM_CKPOL | STM32_LPTIM_PRESC;
- u32 val;
-
- /* Setup LP timer encoder/counter and polarity, without prescaler */
- if (priv->quadrature_mode)
- val = enable ? STM32_LPTIM_ENC : 0;
- else
- val = enable ? STM32_LPTIM_COUNTMODE : 0;
- val |= FIELD_PREP(STM32_LPTIM_CKPOL, enable ? priv->polarity : 0);
-
- return regmap_update_bits(priv->regmap, STM32_LPTIM_CFGR, mask, val);
-}
-
-static int stm32_lptim_write_raw(struct iio_dev *indio_dev,
- struct iio_chan_spec const *chan,
- int val, int val2, long mask)
-{
- struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
- int ret;
-
- switch (mask) {
- case IIO_CHAN_INFO_ENABLE:
- if (val < 0 || val > 1)
- return -EINVAL;
-
- /* Check nobody uses the timer, or already disabled/enabled */
- ret = stm32_lptim_is_enabled(priv);
- if ((ret < 0) || (!ret && !val))
- return ret;
- if (val && ret)
- return -EBUSY;
-
- ret = stm32_lptim_setup(priv, val);
- if (ret)
- return ret;
- return stm32_lptim_set_enable_state(priv, val);
-
- default:
- return -EINVAL;
- }
-}
-
-static int stm32_lptim_read_raw(struct iio_dev *indio_dev,
- struct iio_chan_spec const *chan,
- int *val, int *val2, long mask)
-{
- struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
- u32 dat;
- int ret;
-
- switch (mask) {
- case IIO_CHAN_INFO_RAW:
- ret = regmap_read(priv->regmap, STM32_LPTIM_CNT, &dat);
- if (ret)
- return ret;
- *val = dat;
- return IIO_VAL_INT;
-
- case IIO_CHAN_INFO_ENABLE:
- ret = stm32_lptim_is_enabled(priv);
- if (ret < 0)
- return ret;
- *val = ret;
- return IIO_VAL_INT;
-
- case IIO_CHAN_INFO_SCALE:
- /* Non-quadrature mode: scale = 1 */
- *val = 1;
- *val2 = 0;
- if (priv->quadrature_mode) {
- /*
- * Quadrature encoder mode:
- * - both edges, quarter cycle, scale is 0.25
- * - either rising/falling edge scale is 0.5
- */
- if (priv->polarity > 1)
- *val2 = 2;
- else
- *val2 = 1;
- }
- return IIO_VAL_FRACTIONAL_LOG2;
-
- default:
- return -EINVAL;
- }
-}
-
-static const struct iio_info stm32_lptim_cnt_iio_info = {
- .read_raw = stm32_lptim_read_raw,
- .write_raw = stm32_lptim_write_raw,
-};
-
-static const char *const stm32_lptim_quadrature_modes[] = {
- "non-quadrature",
- "quadrature",
-};
-
-static int stm32_lptim_get_quadrature_mode(struct iio_dev *indio_dev,
- const struct iio_chan_spec *chan)
-{
- struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
-
- return priv->quadrature_mode;
-}
-
-static int stm32_lptim_set_quadrature_mode(struct iio_dev *indio_dev,
- const struct iio_chan_spec *chan,
- unsigned int type)
-{
- struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
-
- if (stm32_lptim_is_enabled(priv))
- return -EBUSY;
-
- priv->quadrature_mode = type;
-
- return 0;
-}
-
-static const struct iio_enum stm32_lptim_quadrature_mode_en = {
- .items = stm32_lptim_quadrature_modes,
- .num_items = ARRAY_SIZE(stm32_lptim_quadrature_modes),
- .get = stm32_lptim_get_quadrature_mode,
- .set = stm32_lptim_set_quadrature_mode,
-};
-
-static const char * const stm32_lptim_cnt_polarity[] = {
- "rising-edge", "falling-edge", "both-edges",
-};
-
-static int stm32_lptim_cnt_get_polarity(struct iio_dev *indio_dev,
- const struct iio_chan_spec *chan)
-{
- struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
-
- return priv->polarity;
-}
-
-static int stm32_lptim_cnt_set_polarity(struct iio_dev *indio_dev,
- const struct iio_chan_spec *chan,
- unsigned int type)
-{
- struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
-
- if (stm32_lptim_is_enabled(priv))
- return -EBUSY;
-
- priv->polarity = type;
-
- return 0;
-}
-
-static const struct iio_enum stm32_lptim_cnt_polarity_en = {
- .items = stm32_lptim_cnt_polarity,
- .num_items = ARRAY_SIZE(stm32_lptim_cnt_polarity),
- .get = stm32_lptim_cnt_get_polarity,
- .set = stm32_lptim_cnt_set_polarity,
-};
-
-static ssize_t stm32_lptim_cnt_get_preset(struct iio_dev *indio_dev,
- uintptr_t private,
- const struct iio_chan_spec *chan,
- char *buf)
-{
- struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
-
- return snprintf(buf, PAGE_SIZE, "%u\n", priv->preset);
-}
-
-static ssize_t stm32_lptim_cnt_set_preset(struct iio_dev *indio_dev,
- uintptr_t private,
- const struct iio_chan_spec *chan,
- const char *buf, size_t len)
-{
- struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
- int ret;
-
- if (stm32_lptim_is_enabled(priv))
- return -EBUSY;
-
- ret = kstrtouint(buf, 0, &priv->preset);
- if (ret)
- return ret;
-
- if (priv->preset > STM32_LPTIM_MAX_ARR)
- return -EINVAL;
-
- return len;
-}
-
-/* LP timer with encoder */
-static const struct iio_chan_spec_ext_info stm32_lptim_enc_ext_info[] = {
- {
- .name = "preset",
- .shared = IIO_SEPARATE,
- .read = stm32_lptim_cnt_get_preset,
- .write = stm32_lptim_cnt_set_preset,
- },
- IIO_ENUM("polarity", IIO_SEPARATE, &stm32_lptim_cnt_polarity_en),
- IIO_ENUM_AVAILABLE("polarity", &stm32_lptim_cnt_polarity_en),
- IIO_ENUM("quadrature_mode", IIO_SEPARATE,
- &stm32_lptim_quadrature_mode_en),
- IIO_ENUM_AVAILABLE("quadrature_mode", &stm32_lptim_quadrature_mode_en),
- {}
-};
-
-static const struct iio_chan_spec stm32_lptim_enc_channels = {
- .type = IIO_COUNT,
- .channel = 0,
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
- BIT(IIO_CHAN_INFO_ENABLE) |
- BIT(IIO_CHAN_INFO_SCALE),
- .ext_info = stm32_lptim_enc_ext_info,
- .indexed = 1,
-};
-
-/* LP timer without encoder (counter only) */
-static const struct iio_chan_spec_ext_info stm32_lptim_cnt_ext_info[] = {
- {
- .name = "preset",
- .shared = IIO_SEPARATE,
- .read = stm32_lptim_cnt_get_preset,
- .write = stm32_lptim_cnt_set_preset,
- },
- IIO_ENUM("polarity", IIO_SEPARATE, &stm32_lptim_cnt_polarity_en),
- IIO_ENUM_AVAILABLE("polarity", &stm32_lptim_cnt_polarity_en),
- {}
-};
-
-static const struct iio_chan_spec stm32_lptim_cnt_channels = {
- .type = IIO_COUNT,
- .channel = 0,
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
- BIT(IIO_CHAN_INFO_ENABLE) |
- BIT(IIO_CHAN_INFO_SCALE),
- .ext_info = stm32_lptim_cnt_ext_info,
- .indexed = 1,
-};
-
-static int stm32_lptim_cnt_probe(struct platform_device *pdev)
-{
- struct stm32_lptimer *ddata = dev_get_drvdata(pdev->dev.parent);
- struct stm32_lptim_cnt *priv;
- struct iio_dev *indio_dev;
-
- if (IS_ERR_OR_NULL(ddata))
- return -EINVAL;
-
- indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
- if (!indio_dev)
- return -ENOMEM;
-
- priv = iio_priv(indio_dev);
- priv->dev = &pdev->dev;
- priv->regmap = ddata->regmap;
- priv->clk = ddata->clk;
- priv->preset = STM32_LPTIM_MAX_ARR;
-
- indio_dev->name = dev_name(&pdev->dev);
- indio_dev->dev.parent = &pdev->dev;
- indio_dev->dev.of_node = pdev->dev.of_node;
- indio_dev->info = &stm32_lptim_cnt_iio_info;
- if (ddata->has_encoder)
- indio_dev->channels = &stm32_lptim_enc_channels;
- else
- indio_dev->channels = &stm32_lptim_cnt_channels;
- indio_dev->num_channels = 1;
-
- platform_set_drvdata(pdev, priv);
-
- return devm_iio_device_register(&pdev->dev, indio_dev);
-}
-
-static const struct of_device_id stm32_lptim_cnt_of_match[] = {
- { .compatible = "st,stm32-lptimer-counter", },
- {},
-};
-MODULE_DEVICE_TABLE(of, stm32_lptim_cnt_of_match);
-
-static struct platform_driver stm32_lptim_cnt_driver = {
- .probe = stm32_lptim_cnt_probe,
- .driver = {
- .name = "stm32-lptimer-counter",
- .of_match_table = stm32_lptim_cnt_of_match,
- },
-};
-module_platform_driver(stm32_lptim_cnt_driver);
-
-MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@st.com>");
-MODULE_ALIAS("platform:stm32-lptimer-counter");
-MODULE_DESCRIPTION("STMicroelectronics STM32 LPTIM counter driver");
-MODULE_LICENSE("GPL v2");
--
2.17.0
^ permalink raw reply related
* [PATCH v6 9/9] iio: counter: Remove IIO counter subdirectory
From: William Breathitt Gray @ 2018-05-16 17:52 UTC (permalink / raw)
To: jic23
Cc: benjamin.gaignard, fabrice.gasnier, linux-iio, linux-kernel,
devicetree, linux-arm-kernel, William Breathitt Gray
In-Reply-To: <cover.1526487615.git.vilhelm.gray@gmail.com>
This patch removes the IIO counter subdirectory which is now superceded
by the Counter subsystem. Deprecation warnings are added to the
documentation of the relevant IIO counter sysfs attributes.
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
Documentation/ABI/testing/sysfs-bus-iio | 8 ++++++++
.../ABI/testing/sysfs-bus-iio-counter-104-quad-8 | 16 ++++++++++++++++
drivers/iio/Kconfig | 1 -
drivers/iio/Makefile | 1 -
drivers/iio/counter/Kconfig | 8 --------
drivers/iio/counter/Makefile | 5 -----
6 files changed, 24 insertions(+), 15 deletions(-)
delete mode 100644 drivers/iio/counter/Kconfig
delete mode 100644 drivers/iio/counter/Makefile
diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 731146c3b138..6115d97b075e 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1637,6 +1637,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_countY_raw
KernelVersion: 4.10
Contact: linux-iio@vger.kernel.org
Description:
+ This interface is deprecated; please use the Counter subsystem.
+
Raw counter device counts from channel Y. For quadrature
counters, multiplication by an available [Y]_scale results in
the counts of a single quadrature signal phase from channel Y.
@@ -1645,6 +1647,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_indexY_raw
KernelVersion: 4.10
Contact: linux-iio@vger.kernel.org
Description:
+ This interface is deprecated; please use the Counter subsystem.
+
Raw counter device index value from channel Y. This attribute
provides an absolute positional reference (e.g. a pulse once per
revolution) which may be used to home positional systems as
@@ -1654,6 +1658,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_count_count_direction_available
KernelVersion: 4.12
Contact: linux-iio@vger.kernel.org
Description:
+ This interface is deprecated; please use the Counter subsystem.
+
A list of possible counting directions which are:
- "up" : counter device is increasing.
- "down": counter device is decreasing.
@@ -1662,4 +1668,6 @@ What: /sys/bus/iio/devices/iio:deviceX/in_countY_count_direction
KernelVersion: 4.12
Contact: linux-iio@vger.kernel.org
Description:
+ This interface is deprecated; please use the Counter subsystem.
+
Raw counter device counters direction for channel Y.
diff --git a/Documentation/ABI/testing/sysfs-bus-iio-counter-104-quad-8 b/Documentation/ABI/testing/sysfs-bus-iio-counter-104-quad-8
index 7fac2c268d9a..bac3d0d48b7b 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio-counter-104-quad-8
+++ b/Documentation/ABI/testing/sysfs-bus-iio-counter-104-quad-8
@@ -6,6 +6,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_index_synchronous_mode_available
KernelVersion: 4.10
Contact: linux-iio@vger.kernel.org
Description:
+ This interface is deprecated; please use the Counter subsystem.
+
Discrete set of available values for the respective counter
configuration are listed in this file.
@@ -13,6 +15,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_countY_count_mode
KernelVersion: 4.10
Contact: linux-iio@vger.kernel.org
Description:
+ This interface is deprecated; please use the Counter subsystem.
+
Count mode for channel Y. Four count modes are available:
normal, range limit, non-recycle, and modulo-n. The preset value
for channel Y is used by the count mode where required.
@@ -47,6 +51,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_countY_noise_error
KernelVersion: 4.10
Contact: linux-iio@vger.kernel.org
Description:
+ This interface is deprecated; please use the Counter subsystem.
+
Read-only attribute that indicates whether excessive noise is
present at the channel Y count inputs in quadrature clock mode;
irrelevant in non-quadrature clock mode.
@@ -55,6 +61,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_countY_preset
KernelVersion: 4.10
Contact: linux-iio@vger.kernel.org
Description:
+ This interface is deprecated; please use the Counter subsystem.
+
If the counter device supports preset registers, the preset
count for channel Y is provided by this attribute.
@@ -62,6 +70,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_countY_quadrature_mode
KernelVersion: 4.10
Contact: linux-iio@vger.kernel.org
Description:
+ This interface is deprecated; please use the Counter subsystem.
+
Configure channel Y counter for non-quadrature or quadrature
clock mode. Selecting non-quadrature clock mode will disable
synchronous load mode. In quadrature clock mode, the channel Y
@@ -83,6 +93,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_countY_set_to_preset_on_index
KernelVersion: 4.10
Contact: linux-iio@vger.kernel.org
Description:
+ This interface is deprecated; please use the Counter subsystem.
+
Whether to set channel Y counter with channel Y preset value
when channel Y index input is active, or continuously count.
Valid attribute values are boolean.
@@ -91,6 +103,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_indexY_index_polarity
KernelVersion: 4.10
Contact: linux-iio@vger.kernel.org
Description:
+ This interface is deprecated; please use the Counter subsystem.
+
Active level of channel Y index input; irrelevant in
non-synchronous load mode.
@@ -98,6 +112,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_indexY_synchronous_mode
KernelVersion: 4.10
Contact: linux-iio@vger.kernel.org
Description:
+ This interface is deprecated; please use the Counter subsystem.
+
Configure channel Y counter for non-synchronous or synchronous
load mode. Synchronous load mode cannot be selected in
non-quadrature clock mode.
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index d69e85a8bdc3..1152efad91a1 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -74,7 +74,6 @@ source "drivers/iio/afe/Kconfig"
source "drivers/iio/amplifiers/Kconfig"
source "drivers/iio/chemical/Kconfig"
source "drivers/iio/common/Kconfig"
-source "drivers/iio/counter/Kconfig"
source "drivers/iio/dac/Kconfig"
source "drivers/iio/dummy/Kconfig"
source "drivers/iio/frequency/Kconfig"
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index d8cba9c229c0..7bdd31f1b88f 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -20,7 +20,6 @@ obj-y += amplifiers/
obj-y += buffer/
obj-y += chemical/
obj-y += common/
-obj-y += counter/
obj-y += dac/
obj-y += dummy/
obj-y += gyro/
diff --git a/drivers/iio/counter/Kconfig b/drivers/iio/counter/Kconfig
deleted file mode 100644
index 95a7a0df6cac..000000000000
--- a/drivers/iio/counter/Kconfig
+++ /dev/null
@@ -1,8 +0,0 @@
-#
-# Counter devices
-#
-# When adding new entries keep the list in alphabetical order
-
-menu "Counters"
-
-endmenu
diff --git a/drivers/iio/counter/Makefile b/drivers/iio/counter/Makefile
deleted file mode 100644
index 8fd3d954775a..000000000000
--- a/drivers/iio/counter/Makefile
+++ /dev/null
@@ -1,5 +0,0 @@
-#
-# Makefile for IIO counter devices
-#
-
-# When adding new entries keep the list in alphabetical order
--
2.17.0
^ permalink raw reply related
* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
From: Guenter Roeck @ 2018-05-16 18:05 UTC (permalink / raw)
To: Robin Murphy
Cc: Stefan Wahren, Mark Rutland, Jean Delvare, Scott Branden,
devicetree, Ray Jui, Phil Elwell, Eric Anholt, Rob Herring,
bcm-kernel-feedback-list, linux-rpi-kernel, Florian Fainelli,
linux-hwmon, linux-arm-kernel, Noralf Trønnes
In-Reply-To: <b053c458-e50c-d4db-3a18-699327f021f3@arm.com>
On Wed, May 16, 2018 at 02:51:49PM +0100, Robin Murphy wrote:
> Hi Stefan,
>
> On 16/05/18 14:37, Stefan Wahren wrote:
> >Currently there is no easy way to detect under-voltage conditions on a remote
> >Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> >via mailbox interface. The handling based on Noralf's modifications to the
> >downstream firmware driver. In case of an under-voltage condition only an entry
> >is written to the kernel log.
> >
> >CC: "Noralf Trønnes" <noralf@tronnes.org>
> >Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> >---
> > drivers/hwmon/Kconfig | 10 ++
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 218 insertions(+)
> > create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
> >
> >diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> >index 768aed5..7f935cf 100644
> >--- a/drivers/hwmon/Kconfig
> >+++ b/drivers/hwmon/Kconfig
> >@@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> > This driver can also be built as a module. If so, the module
> > will be called pwm-fan.
> >+config SENSORS_RASPBERRYPI_HWMON
> >+ tristate "Raspberry Pi voltage monitor"
> >+ depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
>
> Since RASPBERRYPI_FIRMWARE already implies ARCH_BCM2835 (via BCM2835_MBOX),
> this is just a very roundabout way to say:
>
> depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
>
That would permit SENSORS_RASPBERRYPI_HWMON=y combined with
RASPBERRYPI_FIRMWARE=m, which AFAICS would result in a build error
because include/soc/bcm2835/raspberrypi-firmware.h uses IS_ENABLED()
and not IS_REACHABLE().
Guenter
> Robin.
>
> >+ help
> >+ If you say yes here you get support for voltage sensor on the
> >+ Raspberry Pi.
> >+
> >+ This driver can also be built as a module. If so, the module
> >+ will be called raspberrypi-hwmon.
> >+
> > config SENSORS_SHT15
> > tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> > depends on GPIOLIB || COMPILE_TEST
> >diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> >index e7d52a3..a929770 100644
> >--- a/drivers/hwmon/Makefile
> >+++ b/drivers/hwmon/Makefile
> >@@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> > obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> >+obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> > obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> > obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> > obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> >diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> >new file mode 100644
> >index 0000000..2003f6c
> >--- /dev/null
> >+++ b/drivers/hwmon/raspberrypi-hwmon.c
> >@@ -0,0 +1,207 @@
> >+// SPDX-License-Identifier: GPL-2.0+
> >+/*
> >+ * Raspberry Pi voltage sensor driver
> >+ *
> >+ * Based on firmware/raspberrypi.c by Noralf Trønnes
> >+ *
> >+ * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> >+ */
> >+#include <linux/device.h>
> >+#include <linux/err.h>
> >+#include <linux/hwmon.h>
> >+#include <linux/hwmon-sysfs.h>
> >+#include <linux/module.h>
> >+#include <linux/of_device.h>
> >+#include <linux/platform_device.h>
> >+#include <linux/slab.h>
> >+#include <linux/workqueue.h>
> >+#include <soc/bcm2835/raspberrypi-firmware.h>
> >+
> >+#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> >+
> >+struct rpi_hwmon_data {
> >+ struct device *hwmon_dev;
> >+ struct rpi_firmware *fw;
> >+ u32 last_throttled;
> >+ struct delayed_work get_values_poll_work;
> >+};
> >+
> >+static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> >+{
> >+ u32 new_uv, old_uv, value;
> >+ int ret;
> >+
> >+ /* Clear sticky bits */
> >+ value = 0xffff;
> >+
> >+ ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> >+ &value, sizeof(value));
> >+ if (ret) {
> >+ dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> >+ __func__, ret);
> >+ return;
> >+ }
> >+
> >+ new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> >+ old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> >+ data->last_throttled = value;
> >+
> >+ if (new_uv == old_uv)
> >+ return;
> >+
> >+ if (new_uv)
> >+ dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> >+ value);
> >+ else
> >+ dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> >+ value);
> >+
> >+ sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> >+}
> >+
> >+static void get_values_poll(struct work_struct *work)
> >+{
> >+ struct rpi_hwmon_data *data;
> >+
> >+ data = container_of(work, struct rpi_hwmon_data,
> >+ get_values_poll_work.work);
> >+
> >+ rpi_firmware_get_throttled(data);
> >+
> >+ /*
> >+ * We can't run faster than the sticky shift (100ms) since we get
> >+ * flipping in the sticky bits that are cleared.
> >+ */
> >+ schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> >+}
> >+
> >+static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> >+ u32 attr, int channel, long *val)
> >+{
> >+ struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> >+
> >+ if (type != hwmon_in)
> >+ return -EOPNOTSUPP;
> >+
> >+ if (attr != hwmon_in_lcrit_alarm)
> >+ return -EOPNOTSUPP;
> >+
> >+ if (channel)
> >+ return -EOPNOTSUPP;
> >+
> >+ *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> >+ return 0;
> >+}
> >+
> >+static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> >+ u32 attr, int channel)
> >+{
> >+ if (type != hwmon_in)
> >+ return 0;
> >+
> >+ if (attr != hwmon_in_lcrit_alarm)
> >+ return 0;
> >+
> >+ if (channel)
> >+ return 0;
> >+
> >+ return 0444;
> >+}
> >+
> >+static const u32 rpi_in_config[] = {
> >+ HWMON_I_LCRIT_ALARM,
> >+ 0
> >+};
> >+
> >+static const struct hwmon_channel_info rpi_in = {
> >+ .type = hwmon_in,
> >+ .config = rpi_in_config,
> >+};
> >+
> >+static const struct hwmon_channel_info *rpi_info[] = {
> >+ &rpi_in,
> >+ NULL
> >+};
> >+
> >+static const struct hwmon_ops rpi_hwmon_ops = {
> >+ .is_visible = rpi_is_visible,
> >+ .read = rpi_read,
> >+};
> >+
> >+static const struct hwmon_chip_info rpi_chip_info = {
> >+ .ops = &rpi_hwmon_ops,
> >+ .info = rpi_info,
> >+};
> >+
> >+static int rpi_hwmon_probe(struct platform_device *pdev)
> >+{
> >+ struct device *dev = &pdev->dev;
> >+ struct device_node *fw_node;
> >+ struct rpi_hwmon_data *data;
> >+ int ret;
> >+
> >+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >+ if (!data)
> >+ return -ENOMEM;
> >+
> >+ fw_node = of_get_parent(dev->of_node);
> >+ if (!fw_node) {
> >+ dev_err(dev, "Missing firmware node\n");
> >+ return -ENOENT;
> >+ }
> >+
> >+ data->fw = rpi_firmware_get(fw_node);
> >+ of_node_put(fw_node);
> >+ if (!data->fw)
> >+ return -EPROBE_DEFER;
> >+
> >+ ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> >+ &data->last_throttled,
> >+ sizeof(data->last_throttled));
> >+ if (ret) {
> >+ dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
> >+ return -EOPNOTSUPP;
> >+ }
> >+
> >+ data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> >+ data,
> >+ &rpi_chip_info,
> >+ NULL);
> >+
> >+ INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> >+ platform_set_drvdata(pdev, data);
> >+
> >+ if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> >+ schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> >+
> >+ return PTR_ERR_OR_ZERO(data->hwmon_dev);
> >+}
> >+
> >+static int rpi_hwmon_remove(struct platform_device *pdev)
> >+{
> >+ struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> >+
> >+ cancel_delayed_work_sync(&data->get_values_poll_work);
> >+
> >+ return 0;
> >+}
> >+
> >+static const struct of_device_id rpi_hwmon_of_match[] = {
> >+ { .compatible = "raspberrypi,bcm2835-hwmon", },
> >+ { /* sentinel */},
> >+};
> >+MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> >+
> >+static struct platform_driver rpi_hwmon_driver = {
> >+ .probe = rpi_hwmon_probe,
> >+ .remove = rpi_hwmon_remove,
> >+ .driver = {
> >+ .name = "raspberrypi-hwmon",
> >+ .of_match_table = rpi_hwmon_of_match,
> >+ },
> >+};
> >+module_platform_driver(rpi_hwmon_driver);
> >+
> >+MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> >+MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> >+MODULE_LICENSE("GPL v2");
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 1/2] dt-bindings: Documentation for qcom, llcc
From: Stephen Boyd @ 2018-05-16 18:08 UTC (permalink / raw)
To: rishabhb
Cc: devicetree, linux-arm-kernel, linux-arm-msm, linux-kernel,
linux-arm, tsoni, ckadabi, evgreen, robh
In-Reply-To: <385198cbb91c4a36ad758997916ad271@codeaurora.org>
Quoting rishabhb@codeaurora.org (2018-05-16 10:33:14)
> On 2018-05-16 10:03, Stephen Boyd wrote:
> > Quoting Rishabh Bhatnagar (2018-05-08 13:22:00)
>
> >> +
> >> +- max-slices:
> >> + usage: required
> >> + Value Type: <u32>
> >> + Definition: Number of cache slices supported by hardware
> >> +
> >> +Example:
> >> +
> >> + llcc: qcom,llcc@1100000 {
> >
> > cache-controller@1100000 ?
> >
> We have tried to use consistent naming convention as in llcc_*
> everywhere.
> Using cache-controller will mix and match the naming convention. Also in
> the documentation it is explained what llcc is and its full form.
>
DT prefers standard node names as opposed to vendor specific node names.
Isn't it a cache controller? I fail to see why this can't be done.
^ permalink raw reply
* Re: [patch v21 4/4] Documentation: jtag: Add ABI documentation
From: Randy Dunlap @ 2018-05-16 18:16 UTC (permalink / raw)
To: Oleksandr Shamray, gregkh, arnd
Cc: linux-kernel, linux-arm-kernel, devicetree, openbmc, joel, jiri,
tklauser, linux-serial, vadimp, system-sw-low-level, robh+dt,
openocd-devel-owner, linux-api, davem, mchehab
In-Reply-To: <1526394095-5069-5-git-send-email-oleksandrs@mellanox.com>
On 05/15/2018 07:21 AM, Oleksandr Shamray wrote:
> Added document that describe the ABI for JTAG class drivrer
>
> ---
> Documentation/ABI/testing/jtag-dev | 27 +++++++++
> Documentation/jtag/overview | 28 +++++++++
> Documentation/jtag/transactions | 109 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 164 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/ABI/testing/jtag-dev
> create mode 100644 Documentation/jtag/overview
> create mode 100644 Documentation/jtag/transactions
>
> diff --git a/Documentation/jtag/overview b/Documentation/jtag/overview
> new file mode 100644
> index 0000000..a86f188
> --- /dev/null
> +++ b/Documentation/jtag/overview
> @@ -0,0 +1,28 @@
> +Linux kernel JTAG support
> +=========================
> +
> +The JTAG is an industry standard for verifying hardware
JTAG is an industry standard for verifying hardware.
> +JTAG provides access to many logic signals of a complex integrated circuit,
> +including the device pins.
> +
> +A JTAG interface is a special interface added to a chip.
> +Depending on the version of JTAG, two, four, or five pins are added.
> +
> +The connector pins are:
> + TDI (Test Data In)
> + TDO (Test Data Out)
> + TCK (Test Clock)
> + TMS (Test Mode Select)
> + TRST (Test Reset) optional.
> +
> +JTAG interface is designed to have two parts - basic core driver and
> +hardware specific driver. The basic driver introduces a general interface
> +which is not dependent of specific hardware. It provides communication
> +between user space and hardware specific driver.
> +Each JTAG device is represented as a char device from (jtag0, jtag1, ...).
> +Access to a JTAG device is performed through IOCTL calls.
> +
> +Call flow example:
> +User: open -> /dev/jatgX
> +User: ioctl -> /dev/jtagX -> JTAG core driver -> JTAG hw specific driver
JTAG hardware specific driver
> +User: close -> /dev/jatgX
> diff --git a/Documentation/jtag/transactions b/Documentation/jtag/transactions
> new file mode 100644
> index 0000000..6a857c8
> --- /dev/null
> +++ b/Documentation/jtag/transactions
> @@ -0,0 +1,109 @@
> +The JTAG API
> +=============
> +
> +JTAG master devices can be accessed through a character misc-device.
> +Each JTAG master interface can be accessed by using /dev/jtagN
/dev/jtagN.
> +
> +JTAG system calls set:
> +- SIR (Scan Instruction Register, IEEE 1149.1 Instruction Register scan);
> +- SDR (Scan Data Register, IEEE 1149.1 Data Register scan);
> +- RUNTEST (Forces the IEEE 1149.1 bus to a run state for a specified
> +number of clocks.
> +
> +open(), close()
> +-------
> +open() opens JTAG device. Only one open operation per JTAG device
> +can be performed. Two or more open for one device will return error
will return error.
> +
> +Open/Close device:
> +- open("/dev/jtag0", O_RDWR);
> +- close(jtag_fd');
close(jtag_fd);
> +
> +ioctl()
> +-------
> +All access operations to JTAG devices performed through ioctl interface.
> +The IOCTL interface supports this requests:
these requests:
> + JTAG_IOCRUNTEST - Force JTAG state machine to RUN_TEST/IDLE state
> + JTAG_SIOCFREQ - Set JTAG TCK frequency
> + JTAG_GIOCFREQ - Get JTAG TCK frequency
> + JTAG_IOCXFER - send JTAG data Xfer
> + JTAG_GIOCSTATUS - get current JTAG TAP status
> + JTAG_SIOCMODE - set JTAG mode flags.
> +
> +JTAG_SIOCFREQ, JTAG_GIOCFREQ
> +------
> +Set/Get JTAG clock speed:
> +
> + unsigned int jtag_fd;
> + ioctl(jtag_fd, JTAG_SIOCFREQ, &frq);
> + ioctl(jtag_fd, JTAG_GIOCFREQ, &frq);
> +
> +JTAG_IOCRUNTEST
> +------
> +Force JTAG state machine to RUN_TEST/IDLE state
> +
> +struct jtag_run_test_idle {
> + __u8 reset;
> + __u8 endstate;
> + __u8 tck;
> +};
> +
> +reset: 0 - run IDLE/PAUSE from current state
> + 1 - go through TEST_LOGIC/RESET state before IDLE/PAUSE
> +endstate: completion flag
> +tck: clock counter
> +
> +Example:
> + struct jtag_run_test_idle runtest;
> +
> + runtest.endstate = JTAG_STATE_IDLE;
> + runtest.reset = 0;
> + runtest.tck = data_p->tck;
> + usleep(25 * 1000);
> + ioctl(jtag_fd, JTAG_IOCRUNTEST, &runtest);
> +
> +JTAG_IOCXFER
> +------
> +Send SDR/SIR transaction
> +
> +struct jtag_xfer {
> + __u8 type;
> + __u8 direction;
> + __u8 endstate;
> + __u8 padding;
> + __u32 length;
> + __u64 tdio;
> +};
> +
> +type: transfer type - JTAG_SIR_XFER/JTAG_SDR_XFER
> +direction: xfer direction - JTAG_SIR_XFER/JTAG_SDR_XFER,
> +length: xfer data len in bits
> +tdio : xfer data array
> +endstate: xfer end state after transaction finish
> + can be: JTAG_STATE_IDLE/JTAG_STATE_PAUSEIR/JTAG_STATE_PAUSEDR
> +
> +Example:
> + struct jtag_xfer xfer;
> + static char buf[64];
> + static unsigned int buf_len = 0;
> + [...]
> + xfer.type = JTAG_SDR_XFER;
> + xfer.tdio = (__u64)buf;
> + xfer.length = buf_len;
> + xfer.endstate = JTAG_STATE_IDLE;
> +
> + if (is_read)
> + xfer.direction = JTAG_READ_XFER;
> + else
> + xfer.direction = JTAG_WRITE_XFER;
> +
> + ioctl(jtag_fd, JTAG_IOCXFER, &xfer);
> +
> +JTAG_SIOCMODE
> +------
> +If hw driver can support different running modes you can change it.
If hardware driver
> +
> +Example:
> + unsigned int mode;
> + mode = JTAG_XFER_HW_MODE;
> + ioctl(jtag_fd, JTAG_SIOCMODE, &mode);
>
--
~Randy
^ permalink raw reply
* Re: [PATCH net-next v2 0/2] of: mdio: Fall back to mdiobus_register() with NULL device_node
From: David Miller @ 2018-05-16 18:21 UTC (permalink / raw)
To: f.fainelli
Cc: netdev, andrew, vivien.didelot, nicolas.ferre, fugang.duan,
sergei.shtylyov, peppe.cavallaro, alexandre.torgue, joabreu,
grygorii.strashko, woojung.huh, UNGLinuxDriver, robh+dt,
frowand.list, antoine.tenart, Tobias.Jordan, rmk+kernel,
geert+renesas, thomas.petazzoni, niklas.soderlund+renesas,
horms+renesas, muvarov, nsekhar, linux-kernel, linux-renesas-soc,
linux-omap, linux-usb, devicetree
In-Reply-To: <20180515235619.27773-1-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue, 15 May 2018 16:56:17 -0700
> This patch series updates of_mdiobus_register() such that when the device_node
> argument is NULL, it calls mdiobus_register() directly. This is consistent with
> the behavior of of_mdiobus_register() when CONFIG_OF=n.
>
> I only converted the most obvious drivers, there are others that have a much
> less obvious behavior and specifically attempt to deal with CONFIG_ACPI.
>
> Changes in v2:
>
> - fixed build error in davincin_mdio.c (Grygorii)
> - reworked first patch a bit: commit message, subject and removed useless
> code comment
Based upon Andrew's response to Geert's feedback, I'm applying this series.
Thanks.
^ permalink raw reply
* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
From: Guenter Roeck @ 2018-05-16 18:21 UTC (permalink / raw)
To: Stefan Wahren
Cc: Mark Rutland, devicetree, Jean Delvare, Scott Branden, Ray Jui,
Phil Elwell, Eric Anholt, Rob Herring, bcm-kernel-feedback-list,
linux-rpi-kernel, Florian Fainelli, linux-hwmon, linux-arm-kernel,
Noralf Trønnes
In-Reply-To: <1526477827-10859-4-git-send-email-stefan.wahren@i2se.com>
On Wed, May 16, 2018 at 03:37:04PM +0200, Stefan Wahren wrote:
> Currently there is no easy way to detect under-voltage conditions on a remote
> Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> via mailbox interface. The handling based on Noralf's modifications to the
> downstream firmware driver. In case of an under-voltage condition only an entry
> is written to the kernel log.
>
My major concern is how this is displayed with the 'sensors' command.
Can you test and report ?
Of course, it would be much better if the firmware would also report
the actual voltage, but I guess we can't have everything.
More comments inline.
Thanks,
Guenter
> CC: "Noralf Trønnes" <noralf@tronnes.org>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
> drivers/hwmon/Kconfig | 10 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
Please also provide Documentation/hwmon/raspberrypi-hwmon.
> 3 files changed, 218 insertions(+)
> create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 768aed5..7f935cf 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> This driver can also be built as a module. If so, the module
> will be called pwm-fan.
>
> +config SENSORS_RASPBERRYPI_HWMON
> + tristate "Raspberry Pi voltage monitor"
> + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> + help
> + If you say yes here you get support for voltage sensor on the
> + Raspberry Pi.
> +
> + This driver can also be built as a module. If so, the module
> + will be called raspberrypi-hwmon.
> +
> config SENSORS_SHT15
> tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> depends on GPIOLIB || COMPILE_TEST
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e7d52a3..a929770 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> +obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> new file mode 100644
> index 0000000..2003f6c
> --- /dev/null
> +++ b/drivers/hwmon/raspberrypi-hwmon.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Raspberry Pi voltage sensor driver
> + *
> + * Based on firmware/raspberrypi.c by Noralf Trønnes
> + *
> + * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> + */
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
Unnecessary include
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +#include <soc/bcm2835/raspberrypi-firmware.h>
> +
> +#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> +
> +struct rpi_hwmon_data {
> + struct device *hwmon_dev;
> + struct rpi_firmware *fw;
> + u32 last_throttled;
> + struct delayed_work get_values_poll_work;
> +};
> +
> +static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> +{
> + u32 new_uv, old_uv, value;
> + int ret;
> +
> + /* Clear sticky bits */
Please make more explicit that this is a request/command sent to the firmware.
> + value = 0xffff;
> +
> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> + &value, sizeof(value));
> + if (ret) {
> + dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> + __func__, ret);
The function name seems unnecessary.
> + return;
> + }
> +
> + new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> + old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> + data->last_throttled = value;
> +
> + if (new_uv == old_uv)
> + return;
> +
> + if (new_uv)
> + dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> + value);
> + else
> + dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> + value);
What value do those hex values provide to the user ?
> +
> + sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> +}
> +
> +static void get_values_poll(struct work_struct *work)
> +{
> + struct rpi_hwmon_data *data;
> +
> + data = container_of(work, struct rpi_hwmon_data,
> + get_values_poll_work.work);
> +
> + rpi_firmware_get_throttled(data);
> +
> + /*
> + * We can't run faster than the sticky shift (100ms) since we get
> + * flipping in the sticky bits that are cleared.
> + */
> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> +}
> +
> +static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> +
> + if (type != hwmon_in)
> + return -EOPNOTSUPP;
> +
> + if (attr != hwmon_in_lcrit_alarm)
> + return -EOPNOTSUPP;
> +
> + if (channel)
> + return -EOPNOTSUPP;
> +
There is only one channel, one attribute, and one type supported.
As such, the checks are unnecessary.
> + *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> + return 0;
> +}
> +
> +static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + if (type != hwmon_in)
> + return 0;
> +
> + if (attr != hwmon_in_lcrit_alarm)
> + return 0;
> +
> + if (channel)
> + return 0;
Same as above. In the list below, there is not a single conditional attribute.
Given that, the checks should be unnecessary, and it should be sufficient to
just return 0444.
> +
> + return 0444;
> +}
> +
> +static const u32 rpi_in_config[] = {
> + HWMON_I_LCRIT_ALARM,
> + 0
> +};
> +
> +static const struct hwmon_channel_info rpi_in = {
> + .type = hwmon_in,
> + .config = rpi_in_config,
> +};
> +
> +static const struct hwmon_channel_info *rpi_info[] = {
> + &rpi_in,
> + NULL
> +};
> +
> +static const struct hwmon_ops rpi_hwmon_ops = {
> + .is_visible = rpi_is_visible,
> + .read = rpi_read,
> +};
> +
> +static const struct hwmon_chip_info rpi_chip_info = {
> + .ops = &rpi_hwmon_ops,
> + .info = rpi_info,
> +};
> +
> +static int rpi_hwmon_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *fw_node;
> + struct rpi_hwmon_data *data;
> + int ret;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + fw_node = of_get_parent(dev->of_node);
> + if (!fw_node) {
> + dev_err(dev, "Missing firmware node\n");
> + return -ENOENT;
> + }
> +
> + data->fw = rpi_firmware_get(fw_node);
> + of_node_put(fw_node);
> + if (!data->fw)
> + return -EPROBE_DEFER;
> +
> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> + &data->last_throttled,
> + sizeof(data->last_throttled));
> + if (ret) {
> + dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
If this is an error -> dev_err().
> + return -EOPNOTSUPP;
or return -ENODEV.
> + }
> +
> + data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> + data,
> + &rpi_chip_info,
> + NULL);
> +
> + INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> + platform_set_drvdata(pdev, data);
> +
> + if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> +
> + return PTR_ERR_OR_ZERO(data->hwmon_dev);
> +}
> +
> +static int rpi_hwmon_remove(struct platform_device *pdev)
> +{
> + struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> +
> + cancel_delayed_work_sync(&data->get_values_poll_work);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id rpi_hwmon_of_match[] = {
> + { .compatible = "raspberrypi,bcm2835-hwmon", },
> + { /* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> +
> +static struct platform_driver rpi_hwmon_driver = {
> + .probe = rpi_hwmon_probe,
> + .remove = rpi_hwmon_remove,
> + .driver = {
> + .name = "raspberrypi-hwmon",
> + .of_match_table = rpi_hwmon_of_match,
> + },
> +};
> +module_platform_driver(rpi_hwmon_driver);
> +
> +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> +MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
From: Robin Murphy @ 2018-05-16 18:23 UTC (permalink / raw)
To: Guenter Roeck
Cc: Stefan Wahren, Mark Rutland, Jean Delvare, Scott Branden,
devicetree, Ray Jui, Phil Elwell, Eric Anholt, Rob Herring,
bcm-kernel-feedback-list, linux-rpi-kernel, Florian Fainelli,
linux-hwmon, linux-arm-kernel, Noralf Trønnes
In-Reply-To: <20180516180549.GA22705@roeck-us.net>
On 16/05/18 19:05, Guenter Roeck wrote:
> On Wed, May 16, 2018 at 02:51:49PM +0100, Robin Murphy wrote:
>> Hi Stefan,
>>
>> On 16/05/18 14:37, Stefan Wahren wrote:
>>> Currently there is no easy way to detect under-voltage conditions on a remote
>>> Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
>>> via mailbox interface. The handling based on Noralf's modifications to the
>>> downstream firmware driver. In case of an under-voltage condition only an entry
>>> is written to the kernel log.
>>>
>>> CC: "Noralf Trønnes" <noralf@tronnes.org>
>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>> ---
>>> drivers/hwmon/Kconfig | 10 ++
>>> drivers/hwmon/Makefile | 1 +
>>> drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 218 insertions(+)
>>> create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
>>>
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index 768aed5..7f935cf 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
>>> This driver can also be built as a module. If so, the module
>>> will be called pwm-fan.
>>> +config SENSORS_RASPBERRYPI_HWMON
>>> + tristate "Raspberry Pi voltage monitor"
>>> + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
>>
>> Since RASPBERRYPI_FIRMWARE already implies ARCH_BCM2835 (via BCM2835_MBOX),
>> this is just a very roundabout way to say:
>>
>> depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
>>
> That would permit SENSORS_RASPBERRYPI_HWMON=y combined with
> RASPBERRYPI_FIRMWARE=m, which AFAICS would result in a build error
> because include/soc/bcm2835/raspberrypi-firmware.h uses IS_ENABLED()
> and not IS_REACHABLE().
But that's only possible when COMPILE_TEST=y, where in any case there
are stub definitions for the #else case in that header which should
still be enough to build with, right? (and if not, that's probably its
own bug)
Nobody's expecting COMPILE_TEST configs to actually boot and work
perfectly, are they?
Robin.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH net-next v2 0/2] of: mdio: Fall back to mdiobus_register() with NULL device_node
From: Geert Uytterhoeven @ 2018-05-16 18:32 UTC (permalink / raw)
To: David Miller
Cc: Florian Fainelli, netdev, Andrew Lunn, Vivien Didelot,
Nicolas Ferre, Fugang Duan, Sergei Shtylyov, Giuseppe Cavallaro,
Alexandre Torgue, Jose Abreu, Grygorii Strashko, Woojung Huh,
Microchip Linux Driver Support, Rob Herring, Frank Rowand,
Antoine Ténart, Tobias Jordan
In-Reply-To: <20180516.142102.1650229402143291732.davem@davemloft.net>
Hi David,
On Wed, May 16, 2018 at 8:21 PM, David Miller <davem@davemloft.net> wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Tue, 15 May 2018 16:56:17 -0700
>
>> This patch series updates of_mdiobus_register() such that when the device_node
>> argument is NULL, it calls mdiobus_register() directly. This is consistent with
>> the behavior of of_mdiobus_register() when CONFIG_OF=n.
>>
>> I only converted the most obvious drivers, there are others that have a much
>> less obvious behavior and specifically attempt to deal with CONFIG_ACPI.
>>
>> Changes in v2:
>>
>> - fixed build error in davincin_mdio.c (Grygorii)
>> - reworked first patch a bit: commit message, subject and removed useless
>> code comment
>
> Based upon Andrew's response to Geert's feedback, I'm applying this series.
Thanks, his feedback made perfect sense.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH/RFT v3 0/3] thermal: add support for r8a77995
From: Niklas Söderlund @ 2018-05-16 19:08 UTC (permalink / raw)
To: Ulrich Hecht
Cc: jacopo mondi, Yoshihiro Kaneko, Linux-Renesas, Zhang Rui,
Eduardo Valentin, Rob Herring, Linux PM list, devicetree
In-Reply-To: <CAO3366w6jfL4W5i_ezoL0s6_1Rrc0RssHyc0p2zhKQcicpzNqQ@mail.gmail.com>
Hi Ulrich,
On 2018-05-16 15:07:01 +0200, Ulrich Hecht wrote:
> On Wed, Apr 11, 2018 at 11:01 AM, jacopo mondi <jacopo@jmondi.org> wrote:
> > Hello Kaneko-san,
> >
> > On Tue, Apr 03, 2018 at 09:43:02PM +0900, Yoshihiro Kaneko wrote:
> >> This series adds thermal support for r8a77995.
> >> R-Car D3 (r8a77995) have a thermal sensor module which is similar to Gen2.
> >> Therefore this series adds r8a77995 support to rcar_thermal driver not
> >> rcar_gen3_thermal driver.
> >
> > I tested this on D3 Draak.
> >
> > I generated load expecting the detected temperature to rise.
> >
> > It took a while, and I only see a slight increase of the temperature
> > reported by the 'temp' attribute.
>
> Pointing a heat gun at the SoC, I managed to get the temperature up to
> 80000, and it went back to 40000 when I removed it. I'd say this
> works.
I like your style! I contemplated using a hairdryer when testing some
Gen3 thermal work but decided against it. Good too see others are not as
weak minded as my self :-)
>
> Tested-By: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
>
> CU
> Uli
--
Regards,
Niklas S�derlund
^ permalink raw reply
* Re: [PATCH v6 2/3] gpio: pca953x: define masks for addressing common and extended registers
From: Andy Shevchenko @ 2018-05-16 19:50 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Kumar Gala, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
Linus Walleij, Alexandre Courbot, devicetree,
open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
Discussions about the Letux Kernel, kernel
In-Reply-To: <314df7f911fda8f5584cc4384a26074848a2673b.1526490085.git.hns@goldelico.com>
On Wed, May 16, 2018 at 8:01 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
> These mask bits are to be used to map the extended register
> addreseses (which are defined for an unsupported 8-bit pcal chip)
> to 16 and 24 bit chips (pcal6524).
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
> drivers/gpio/gpio-pca953x.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 2b667166e855..c0eb679e60d4 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -56,6 +56,10 @@
> #define PCAL6524_DEBOUNCE 0x2d
>
> #define PCA_GPIO_MASK 0x00FF
> +
> +#define PCAL_GPIO_MASK GENMASK(4, 0)
> +#define PCAL_PINCTRL_MASK (~PCAL_GPIO_MASK)
> +
I give second thought about it, and think
either plain values, or second converted to its own explicit GENMASK
would be better.
(most confusing part to me is unknowness of the side of PINCTRL part
in the mask)
> #define PCA_INT 0x0100
> #define PCA_PCAL 0x0200
> #define PCA_LATCH_INT (PCA_PCAL | PCA_INT)
> --
> 2.12.2
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH 0/3] Add R8A77980 GEther support
From: Sergei Shtylyov @ 2018-05-16 19:52 UTC (permalink / raw)
To: netdev, devicetree, David S. Miller, Rob Herring
Cc: Mark Rutland, linux-renesas-soc
Hello!
Here's a set of 3 patches against DaveM's 'net-next.git' repo. They (gradually)
add R8A77980 GEther support to the 'sh_eth' driver, starting with couple new
register bits/values introduced with this chip, and ending with adding a new
'struct sh_eth_cpu_data' instance connected to the new DT "compatible" prop
value...
[1/1] sh_eth: add RGMII support
[2/3] sh_eth: add EDMR.NBST support
[3/3] sh_eth: add R8A77980 support
MBR, Sergei
^ permalink raw reply
* Re: [PATCH v6 0/3] pcal6524 extensions and fixes for pca953x driver
From: Andy Shevchenko @ 2018-05-16 19:53 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Kumar Gala, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
Linus Walleij, Alexandre Courbot, devicetree,
open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
Discussions about the Letux Kernel, kernel
In-Reply-To: <cover.1526490085.git.hns@goldelico.com>
On Wed, May 16, 2018 at 8:01 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
> V6:
> * added proper attribution to the formula used for fixing the
> pcal6524 register address (changes commit message only)
> * add back missing first patch from V2 that defines the
> PCA_LATCH_INT constant
> * removed patches already merged
>
Thanks for an update.
I think we still need to address the constant representation in patch 2.
For the rest take my
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 2018-04-28 18:33:42: V5:
> * fix wrong split up between patches 1/7 and 2/7.
>
> 2018-04-26 19:35:07: V4:
> * introduced PCA_LATCH_INT constant to make of_table more
> readable (suggested by Andy Shevchenko)
> * converted all register constants to hex in a separate
> patch (suggested by Andy Shevchenko)
> * separated additional pcal953x and pcal6524 register
> definitions into separate patches (suggested by Andy Shevchenko)
> * made special pcal6524 address adjustment more readable
> (suggested by Andy Shevchenko)
> * moved gpio-controller and interrupt-controller to the
> "required" section (reviewed by Rob Herring)
>
> 2018-04-10 18:07:07: V3:
> * add Reported-by: and Reviewed-by:
> * fix wording for bindings description and example
> * convert all register offsets to hex
> * omit the LEVEL-IRQ RFC/hack commit
>
> 2018-04-04 21:00:27: V2:
> * added PCA_PCAL flags if matched through of-table
> * fix address calculation for extended PCAL6524 registers
> * hack to map LEVEL_LOW to EDGE_FALLING to be able to
> test in combination with ts3a227e driver
> * improve description of bindings for optional vcc-supply
> and interrupt-controller;
>
> 2018-03-10 09:32:53: no initial description
>
> H. Nikolaus Schaller (3):
> gpio: pca953x: set the PCA_PCAL flag also when matching by DT
> gpio: pca953x: define masks for addressing common and extended
> registers
> gpio: pca953x: fix address calculation for pcal6524
>
> drivers/gpio/gpio-pca953x.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> --
> 2.12.2
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v6 2/3] gpio: pca953x: define masks for addressing common and extended registers
From: H. Nikolaus Schaller @ 2018-05-16 19:57 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Kumar Gala, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
Linus Walleij, Alexandre Courbot, devicetree,
open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
Discussions about the Letux Kernel, kernel
In-Reply-To: <CAHp75VcadghXsqu66NmrfbEPpgqusEUuzSkrE8PSJjAV0avntQ@mail.gmail.com>
> Am 16.05.2018 um 21:50 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
>
> On Wed, May 16, 2018 at 8:01 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>> These mask bits are to be used to map the extended register
>> addreseses (which are defined for an unsupported 8-bit pcal chip)
>> to 16 and 24 bit chips (pcal6524).
>>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/gpio/gpio-pca953x.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
>> index 2b667166e855..c0eb679e60d4 100644
>> --- a/drivers/gpio/gpio-pca953x.c
>> +++ b/drivers/gpio/gpio-pca953x.c
>> @@ -56,6 +56,10 @@
>> #define PCAL6524_DEBOUNCE 0x2d
>>
>> #define PCA_GPIO_MASK 0x00FF
>> +
>> +#define PCAL_GPIO_MASK GENMASK(4, 0)
>> +#define PCAL_PINCTRL_MASK (~PCAL_GPIO_MASK)
>> +
>
> I give second thought about it, and think
> either plain values, or second converted to its own explicit GENMASK
> would be better.
>
> (most confusing part to me is unknowness of the side of PINCTRL part
> in the mask)
I see.
Then, I'd also prefer plain values.
If ok, I can send a v7 tomorrow.
BR,
Nikolaus
^ permalink raw reply
* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
From: Stefan Wahren @ 2018-05-16 19:59 UTC (permalink / raw)
To: Guenter Roeck
Cc: Mark Rutland, devicetree, Jean Delvare, Florian Fainelli,
Scott Branden, Ray Jui, Phil Elwell, Rob Herring, Eric Anholt,
Noralf Trønnes, bcm-kernel-feedback-list, linux-rpi-kernel,
linux-hwmon, linux-arm-kernel
In-Reply-To: <20180516182144.GB22705@roeck-us.net>
Hi Guenter,
> Guenter Roeck <linux@roeck-us.net> hat am 16. Mai 2018 um 20:21 geschrieben:
>
>
> On Wed, May 16, 2018 at 03:37:04PM +0200, Stefan Wahren wrote:
> > Currently there is no easy way to detect under-voltage conditions on a remote
> > Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> > via mailbox interface. The handling based on Noralf's modifications to the
> > downstream firmware driver. In case of an under-voltage condition only an entry
> > is written to the kernel log.
> >
>
> My major concern is how this is displayed with the 'sensors' command.
> Can you test and report ?
I get the following output:
rpi_volt-isa-0000
Adapter: ISA adapter
in0: N/A
>
> Of course, it would be much better if the firmware would also report
> the actual voltage, but I guess we can't have everything.
I think this isn't possible because the hardware only provide a binary value (GPIO).
>
> More comments inline.
>
> Thanks,
> Guenter
>
> > CC: "Noralf Trønnes" <noralf@tronnes.org>
> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > ---
> > drivers/hwmon/Kconfig | 10 ++
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
>
> Please also provide Documentation/hwmon/raspberrypi-hwmon.
>
> > 3 files changed, 218 insertions(+)
> > create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 768aed5..7f935cf 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> > This driver can also be built as a module. If so, the module
> > will be called pwm-fan.
> >
> > +config SENSORS_RASPBERRYPI_HWMON
> > + tristate "Raspberry Pi voltage monitor"
> > + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> > + help
> > + If you say yes here you get support for voltage sensor on the
> > + Raspberry Pi.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called raspberrypi-hwmon.
> > +
> > config SENSORS_SHT15
> > tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> > depends on GPIOLIB || COMPILE_TEST
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index e7d52a3..a929770 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> > obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> > +obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> > obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> > obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> > obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> > diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> > new file mode 100644
> > index 0000000..2003f6c
> > --- /dev/null
> > +++ b/drivers/hwmon/raspberrypi-hwmon.c
> > @@ -0,0 +1,207 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Raspberry Pi voltage sensor driver
> > + *
> > + * Based on firmware/raspberrypi.c by Noralf Trønnes
> > + *
> > + * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> > + */
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
>
> Unnecessary include
>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/workqueue.h>
> > +#include <soc/bcm2835/raspberrypi-firmware.h>
> > +
> > +#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> > +
> > +struct rpi_hwmon_data {
> > + struct device *hwmon_dev;
> > + struct rpi_firmware *fw;
> > + u32 last_throttled;
> > + struct delayed_work get_values_poll_work;
> > +};
> > +
> > +static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> > +{
> > + u32 new_uv, old_uv, value;
> > + int ret;
> > +
> > + /* Clear sticky bits */
>
> Please make more explicit that this is a request/command sent to the firmware.
>
> > + value = 0xffff;
> > +
> > + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> > + &value, sizeof(value));
> > + if (ret) {
> > + dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> > + __func__, ret);
>
> The function name seems unnecessary.
>
> > + return;
> > + }
> > +
> > + new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> > + old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> > + data->last_throttled = value;
> > +
> > + if (new_uv == old_uv)
> > + return;
> > +
> > + if (new_uv)
> > + dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> > + value);
> > + else
> > + dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> > + value);
>
> What value do those hex values provide to the user ?
The actual definition of the bits can be found in the commit log of patch #1. But this isn't very helpful for an end user.
>
> > +
> > + sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> > +}
> > +
> > +static void get_values_poll(struct work_struct *work)
> > +{
> > + struct rpi_hwmon_data *data;
> > +
> > + data = container_of(work, struct rpi_hwmon_data,
> > + get_values_poll_work.work);
> > +
> > + rpi_firmware_get_throttled(data);
> > +
> > + /*
> > + * We can't run faster than the sticky shift (100ms) since we get
> > + * flipping in the sticky bits that are cleared.
> > + */
> > + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> > +}
> > +
> > +static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, long *val)
> > +{
> > + struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> > +
> > + if (type != hwmon_in)
> > + return -EOPNOTSUPP;
> > +
> > + if (attr != hwmon_in_lcrit_alarm)
> > + return -EOPNOTSUPP;
> > +
> > + if (channel)
> > + return -EOPNOTSUPP;
> > +
> There is only one channel, one attribute, and one type supported.
> As such, the checks are unnecessary.
>
> > + *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> > + return 0;
> > +}
> > +
> > +static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> > + u32 attr, int channel)
> > +{
> > + if (type != hwmon_in)
> > + return 0;
> > +
> > + if (attr != hwmon_in_lcrit_alarm)
> > + return 0;
> > +
> > + if (channel)
> > + return 0;
>
> Same as above. In the list below, there is not a single conditional attribute.
> Given that, the checks should be unnecessary, and it should be sufficient to
> just return 0444.
>
> > +
> > + return 0444;
> > +}
> > +
> > +static const u32 rpi_in_config[] = {
> > + HWMON_I_LCRIT_ALARM,
> > + 0
> > +};
> > +
> > +static const struct hwmon_channel_info rpi_in = {
> > + .type = hwmon_in,
> > + .config = rpi_in_config,
> > +};
> > +
> > +static const struct hwmon_channel_info *rpi_info[] = {
> > + &rpi_in,
> > + NULL
> > +};
> > +
> > +static const struct hwmon_ops rpi_hwmon_ops = {
> > + .is_visible = rpi_is_visible,
> > + .read = rpi_read,
> > +};
> > +
> > +static const struct hwmon_chip_info rpi_chip_info = {
> > + .ops = &rpi_hwmon_ops,
> > + .info = rpi_info,
> > +};
> > +
> > +static int rpi_hwmon_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_node *fw_node;
> > + struct rpi_hwmon_data *data;
> > + int ret;
> > +
> > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + fw_node = of_get_parent(dev->of_node);
> > + if (!fw_node) {
> > + dev_err(dev, "Missing firmware node\n");
> > + return -ENOENT;
> > + }
> > +
> > + data->fw = rpi_firmware_get(fw_node);
> > + of_node_put(fw_node);
> > + if (!data->fw)
> > + return -EPROBE_DEFER;
> > +
> > + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> > + &data->last_throttled,
> > + sizeof(data->last_throttled));
> > + if (ret) {
> > + dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
>
> If this is an error -> dev_err().
I wasn't sure. If the firmware is too old, we cannot provide this feature and it's a waste of resources to load this driver. On the other side i don't want to confuse people this is something bad.
Stefan
>
> > + return -EOPNOTSUPP;
>
> or return -ENODEV.
>
> > + }
> > +
> > + data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> > + data,
> > + &rpi_chip_info,
> > + NULL);
> > +
> > + INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> > + platform_set_drvdata(pdev, data);
> > +
> > + if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> > + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> > +
> > + return PTR_ERR_OR_ZERO(data->hwmon_dev);
> > +}
> > +
> > +static int rpi_hwmon_remove(struct platform_device *pdev)
> > +{
> > + struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> > +
> > + cancel_delayed_work_sync(&data->get_values_poll_work);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id rpi_hwmon_of_match[] = {
> > + { .compatible = "raspberrypi,bcm2835-hwmon", },
> > + { /* sentinel */},
> > +};
> > +MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> > +
> > +static struct platform_driver rpi_hwmon_driver = {
> > + .probe = rpi_hwmon_probe,
> > + .remove = rpi_hwmon_remove,
> > + .driver = {
> > + .name = "raspberrypi-hwmon",
> > + .of_match_table = rpi_hwmon_of_match,
> > + },
> > +};
> > +module_platform_driver(rpi_hwmon_driver);
> > +
> > +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> > +MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 3/3] sh_eth: add R8A77980 support
From: Sergei Shtylyov @ 2018-05-16 20:00 UTC (permalink / raw)
To: netdev, devicetree, David S. Miller, Rob Herring
Cc: Mark Rutland, linux-renesas-soc
In-Reply-To: <087c91a3-a451-6de7-5e0f-a835f8cc98f1@cogentembedded.com>
Finally, add support for the DT probing of the R-Car V3H (AKA R8A77980) --
it's the only R-Car gen3 SoC having the GEther controller -- others have
only EtherAVB...
Based on the original (and large) patch by Vladimir Barinov.
Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
Documentation/devicetree/bindings/net/sh_eth.txt | 1
drivers/net/ethernet/renesas/sh_eth.c | 44 +++++++++++++++++++++++
2 files changed, 45 insertions(+)
Index: net-next/Documentation/devicetree/bindings/net/sh_eth.txt
===================================================================
--- net-next.orig/Documentation/devicetree/bindings/net/sh_eth.txt
+++ net-next/Documentation/devicetree/bindings/net/sh_eth.txt
@@ -14,6 +14,7 @@ Required properties:
"renesas,ether-r8a7791" if the device is a part of R8A7791 SoC.
"renesas,ether-r8a7793" if the device is a part of R8A7793 SoC.
"renesas,ether-r8a7794" if the device is a part of R8A7794 SoC.
+ "renesas,gether-r8a77980" if the device is a part of R8A77980 SoC.
"renesas,ether-r7s72100" if the device is a part of R7S72100 SoC.
"renesas,rcar-gen1-ether" for a generic R-Car Gen1 device.
"renesas,rcar-gen2-ether" for a generic R-Car Gen2 or RZ/G1
Index: net-next/drivers/net/ethernet/renesas/sh_eth.c
===================================================================
--- net-next.orig/drivers/net/ethernet/renesas/sh_eth.c
+++ net-next/drivers/net/ethernet/renesas/sh_eth.c
@@ -753,6 +753,49 @@ static struct sh_eth_cpu_data rcar_gen2_
.rmiimode = 1,
.magic = 1,
};
+
+/* R8A77980 */
+static struct sh_eth_cpu_data r8a77980_data = {
+ .soft_reset = sh_eth_soft_reset_gether,
+
+ .set_duplex = sh_eth_set_duplex,
+ .set_rate = sh_eth_set_rate_gether,
+
+ .register_type = SH_ETH_REG_GIGABIT,
+
+ .edtrr_trns = EDTRR_TRNS_GETHER,
+ .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD | ECSR_MPD,
+ .ecsipr_value = ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP |
+ ECSIPR_MPDIP,
+ .eesipr_value = EESIPR_RFCOFIP | EESIPR_ECIIP |
+ EESIPR_FTCIP | EESIPR_TDEIP | EESIPR_TFUFIP |
+ EESIPR_FRIP | EESIPR_RDEIP | EESIPR_RFOFIP |
+ EESIPR_RMAFIP | EESIPR_RRFIP |
+ EESIPR_RTLFIP | EESIPR_RTSFIP |
+ EESIPR_PREIP | EESIPR_CERFIP,
+
+ .tx_check = EESR_FTC | EESR_CD | EESR_RTO,
+ .eesr_err_check = EESR_TWB1 | EESR_TWB | EESR_TABT | EESR_RABT |
+ EESR_RFE | EESR_RDE | EESR_RFRMER |
+ EESR_TFE | EESR_TDE | EESR_ECI,
+ .fdr_value = 0x0000070f,
+
+ .apr = 1,
+ .mpr = 1,
+ .tpauser = 1,
+ .bculr = 1,
+ .hw_swap = 1,
+ .nbst = 1,
+ .rpadir = 1,
+ .rpadir_value = 2 << 16,
+ .no_trimd = 1,
+ .no_ade = 1,
+ .xdfar_rw = 1,
+ .hw_checksum = 1,
+ .select_mii = 1,
+ .magic = 1,
+ .cexcr = 1,
+};
#endif /* CONFIG_OF */
static void sh_eth_set_rate_sh7724(struct net_device *ndev)
@@ -3134,6 +3177,7 @@ static const struct of_device_id sh_eth_
{ .compatible = "renesas,ether-r8a7791", .data = &rcar_gen2_data },
{ .compatible = "renesas,ether-r8a7793", .data = &rcar_gen2_data },
{ .compatible = "renesas,ether-r8a7794", .data = &rcar_gen2_data },
+ { .compatible = "renesas,gether-r8a77980", .data = &r8a77980_data },
{ .compatible = "renesas,ether-r7s72100", .data = &r7s72100_data },
{ .compatible = "renesas,rcar-gen1-ether", .data = &rcar_gen1_data },
{ .compatible = "renesas,rcar-gen2-ether", .data = &rcar_gen2_data },
^ permalink raw reply
* Re: [PATCH v6 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver
From: Jacek Anaszewski @ 2018-05-16 20:10 UTC (permalink / raw)
To: Dan Murphy, robh+dt, mark.rutland, pavel
Cc: devicetree, linux-kernel, linux-leds
In-Reply-To: <c72494c9-e513-f3d0-3098-83f47aa9367c@ti.com>
Dan,
On 05/15/2018 11:29 PM, Dan Murphy wrote:
> Jacek
>
> On 05/15/2018 04:13 PM, Jacek Anaszewski wrote:
>> Hi Dan,
>>
>> Thanks for the update.
>>
>> On 05/15/2018 05:43 PM, Dan Murphy wrote:
>>> Introduce the device tree bindings for the lm3601x
>>> family of LED torch, flash and IR drivers.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>> ---
>>>
>>> v6 - Removed multiple led child nodes, fixed example to display micro ranges
>>> for corresponding child nodes and added led-sources to define the current driver -
>>> https://patchwork.kernel.org/patch/10392121/
>>>
>>> v5 - No changes - https://patchwork.kernel.org/patch/10391743/
>>> v4 - Added " " around "=", changed strobe to flash on label, removed "support and
>>> register" comment and change ir lable to ir:torch - See v2 patchworks for comments
>>> v3 - Removed wildcard compatible - https://patchwork.kernel.org/patch/10386241/
>>> v2 - No changes - https://patchwork.kernel.org/patch/10384587/
>>>
>>> .../devicetree/bindings/leds/leds-lm3601x.txt | 47 +++++++++++++++++++
>>> 1 file changed, 47 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3601x.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3601x.txt b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt
>>> new file mode 100644
>>> index 000000000000..27930a89e9a5
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt
>>> @@ -0,0 +1,47 @@
>>> +* Texas Instruments - lm3601x Single-LED Flash Driver
>>> +
>>> +The LM3601X are ultra-small LED flash drivers that
>>> +provide a high level of adjustability.
>>> +
>>> +Required properties:
>>> + - compatible : Can be one of the following
>>> + "ti,lm36010"
>>> + "ti,lm36011"
>>> + - reg : I2C slave address
>>> + - #address-cells : 1
>>> + - #size-cells : 0
>>> +
>>> +Required child properties:
>>> + - reg : 0
>>> + - led-sources: 0 - Indicates a IR mode
>>> + 1 - Indicates a Torch (white LED) mode
>>
>> You don't need led-sources at all. Please use reg as in
>> the previous version. led-sources is useful for describing
>> more complex arrangements. Here reg will do.
>
> OK. Thought we would keep consistent with the max IC.
>
>>
>>> +
>>> +Required properties for flash LED child nodes:
>>> + See Documentation/devicetree/bindings/leds/common.txt
>>> + - flash-max-microamp : Range from 11mA -> 1.5A
>>> + - flash-max-timeout-us : Range from 40ms -> 1600ms
>>> + - led-max-microamp : Range from 2.4mA -> 376mA
>>
>> Please give also a step in the format like below
>> (taken from max77693):
>>
>> Valid values: 62500 - 1000000, step by 62500 (rounded down)
>
> I would do this but the step is not linear. The step is 40ms from 40 -> 400.
> after that the step goes to 200ms from 400->1600.
>
> same with the current ranges.
If you're going to apply Andy's formula in the driver, then you can
present it also here. Anyway, please avoid using non-standard "->"
symbol in favor of "to" if you're using "from".
Or even better I propose to list allowed values - there are not
so many of them.
--
Best regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [PATCH v6 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver
From: Dan Murphy @ 2018-05-16 20:14 UTC (permalink / raw)
To: Jacek Anaszewski, robh+dt, mark.rutland, pavel
Cc: devicetree, linux-kernel, linux-leds
In-Reply-To: <03934462-13ef-59c0-aa27-01a003bf7baa@gmail.com>
On 05/16/2018 03:10 PM, Jacek Anaszewski wrote:
> Dan,
>
> On 05/15/2018 11:29 PM, Dan Murphy wrote:
>> Jacek
>>
>> On 05/15/2018 04:13 PM, Jacek Anaszewski wrote:
>>> Hi Dan,
>>>
>>> Thanks for the update.
>>>
>>> On 05/15/2018 05:43 PM, Dan Murphy wrote:
>>>> Introduce the device tree bindings for the lm3601x
>>>> family of LED torch, flash and IR drivers.
>>>>
>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>> ---
>>>>
>>>> v6 - Removed multiple led child nodes, fixed example to display micro ranges
>>>> for corresponding child nodes and added led-sources to define the current driver -
>>>> https://patchwork.kernel.org/patch/10392121/
>>>>
>>>> v5 - No changes - https://patchwork.kernel.org/patch/10391743/
>>>> v4 - Added " " around "=", changed strobe to flash on label, removed "support and
>>>> register" comment and change ir lable to ir:torch - See v2 patchworks for comments
>>>> v3 - Removed wildcard compatible - https://patchwork.kernel.org/patch/10386241/
>>>> v2 - No changes - https://patchwork.kernel.org/patch/10384587/
>>>>
>>>> .../devicetree/bindings/leds/leds-lm3601x.txt | 47 +++++++++++++++++++
>>>> 1 file changed, 47 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3601x.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3601x.txt b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt
>>>> new file mode 100644
>>>> index 000000000000..27930a89e9a5
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt
>>>> @@ -0,0 +1,47 @@
>>>> +* Texas Instruments - lm3601x Single-LED Flash Driver
>>>> +
>>>> +The LM3601X are ultra-small LED flash drivers that
>>>> +provide a high level of adjustability.
>>>> +
>>>> +Required properties:
>>>> + - compatible : Can be one of the following
>>>> + "ti,lm36010"
>>>> + "ti,lm36011"
>>>> + - reg : I2C slave address
>>>> + - #address-cells : 1
>>>> + - #size-cells : 0
>>>> +
>>>> +Required child properties:
>>>> + - reg : 0
>>>> + - led-sources: 0 - Indicates a IR mode
>>>> + 1 - Indicates a Torch (white LED) mode
>>>
>>> You don't need led-sources at all. Please use reg as in
>>> the previous version. led-sources is useful for describing
>>> more complex arrangements. Here reg will do.
>>
>> OK. Thought we would keep consistent with the max IC.
>>
>>>
>>>> +
>>>> +Required properties for flash LED child nodes:
>>>> + See Documentation/devicetree/bindings/leds/common.txt
>>>> + - flash-max-microamp : Range from 11mA -> 1.5A
>>>> + - flash-max-timeout-us : Range from 40ms -> 1600ms
>>>> + - led-max-microamp : Range from 2.4mA -> 376mA
>>>
>>> Please give also a step in the format like below
>>> (taken from max77693):
>>>
>>> Valid values: 62500 - 1000000, step by 62500 (rounded down)
>>
>> I would do this but the step is not linear. The step is 40ms from 40 -> 400.
>> after that the step goes to 200ms from 400->1600.
>>
>> same with the current ranges.
>
> If you're going to apply Andy's formula in the driver, then you can
> present it also here. Anyway, please avoid using non-standard "->"
> symbol in favor of "to" if you're using "from".
>
OK I will. I have not decided if I am going to apply the formula or not.
I am still trying to process all the comments and determine what is preference and
what is beneficial.
> Or even better I propose to list allowed values - there are not
> so many of them.
>
I will figure that out based on what I do with the driver. Been busy doing internal work today
Dan
--
------------------
Dan Murphy
^ permalink raw reply
* Re: [PATCH net-next v2 2/2] drivers: net: Remove device_node checks with of_mdiobus_register()
From: Sergei Shtylyov @ 2018-05-16 20:20 UTC (permalink / raw)
To: Florian Fainelli, netdev
Cc: Andrew Lunn, Vivien Didelot, David S. Miller, Nicolas Ferre,
Fugang Duan, Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
Grygorii Strashko, Woojung Huh, Microchip Linux Driver Support,
Rob Herring, Frank Rowand, Antoine Tenart, Tobias Jordan,
Russell King, Geert Uytterhoeven, Thomas Petazzoni <tho>
In-Reply-To: <20180515235619.27773-3-f.fainelli@gmail.com>
Hello!
On 05/16/2018 02:56 AM, Florian Fainelli wrote:
> A number of drivers have the following pattern:
>
> if (np)
> of_mdiobus_register()
> else
> mdiobus_register()
>
> which the implementation of of_mdiobus_register() now takes care of.
> Remove that pattern in drivers that strictly adhere to it.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
[...]
> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> index ac621f44237a..02e8982519ce 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -450,12 +450,8 @@ static int bcm_sf2_mdio_register(struct dsa_switch *ds)
> priv->slave_mii_bus->parent = ds->dev->parent;
> priv->slave_mii_bus->phy_mask = ~priv->indir_phy_mask;
>
> - if (dn)
> - err = of_mdiobus_register(priv->slave_mii_bus, dn);
> - else
> - err = mdiobus_register(priv->slave_mii_bus);
> -
> - if (err)
> + err = of_mdiobus_register(priv->slave_mii_bus, dn);
> + if (err && dn)
of_node_put() checks for NULL.
> of_node_put(dn);
>
> return err;
[...]
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index d4604bc8eb5b..f3e43db0d6cb 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2052,13 +2052,9 @@ static int fec_enet_mii_init(struct platform_device *pdev)
> fep->mii_bus->parent = &pdev->dev;
>
> node = of_get_child_by_name(pdev->dev.of_node, "mdio");
> - if (node) {
> - err = of_mdiobus_register(fep->mii_bus, node);
> + err = of_mdiobus_register(fep->mii_bus, node);
> + if (node)
> of_node_put(node);
Same comment here.
[...]
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 5970d9e5ddf1..8dd41e08a6c6 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -3025,15 +3025,10 @@ static int sh_mdio_init(struct sh_eth_private *mdp,
> pdev->name, pdev->id);
>
> /* register MDIO bus */
> - if (dev->of_node) {
> - ret = of_mdiobus_register(mdp->mii_bus, dev->of_node);
> - } else {
> - if (pd->phy_irq > 0)
> - mdp->mii_bus->irq[pd->phy] = pd->phy_irq;
> -
> - ret = mdiobus_register(mdp->mii_bus);
> - }
> + if (pd->phy_irq > 0)
> + mdp->mii_bus->irq[pd->phy] = pd->phy_irq;
>
> + ret = of_mdiobus_register(mdp->mii_bus, dev->of_node);
> if (ret)
> goto out_free_bus;
>
This part is:
Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
[...]
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 91761436709a..8dff87ec6d99 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -1843,12 +1843,9 @@ static int lan78xx_mdio_init(struct lan78xx_net *dev)
> }
>
> node = of_get_child_by_name(dev->udev->dev.of_node, "mdio");
> - if (node) {
> - ret = of_mdiobus_register(dev->mdiobus, node);
> + ret = of_mdiobus_register(dev->mdiobus, node);
> + if (node)
> of_node_put(node);
of_node_put() checks for NULL, again...
MBR, Sergei
^ permalink raw reply
* Re: [PATCH v5 4/4] rtc: ds1307: add frequency_test_enable sysfs attribute to check tick on m41txx
From: Andy Shevchenko @ 2018-05-16 20:22 UTC (permalink / raw)
To: Giulio Benetti
Cc: Alessandro Zummo, alexandre.belloni, Rob Herring, Mark Rutland,
linux-rtc, devicetree, Linux Kernel Mailing List
In-Reply-To: <20180516103251.74707-4-giulio.benetti@micronovasrl.com>
On Wed, May 16, 2018 at 1:32 PM, Giulio Benetti
<giulio.benetti@micronovasrl.com> wrote:
> On m41txx you can enable open-drain OUT pin to check if offset is ok.
> Enabling OUT pin with frequency_test_enable attribute, OUT pin will tick
> 512 times faster than 1s tick base.
>
> Enable or Disable FT bit on CONTROL register if freq_test is 1 or 0.
>
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> +static ssize_t frequency_test_enable_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ds1307 *ds1307 = dev_get_drvdata(dev);
> + unsigned long freq_test = 0;
> + int retval;
> +
> + retval = kstrtoul(buf, 10, &freq_test);
> + if ((retval < 0) || (retval > 1)) {
kstrtobool() then?
> + dev_err(dev, "Failed to store RTC Frequency Test attribute\n");
> + return -EINVAL;
...and do not shadow actual error code.
> + }
> +
> + regmap_update_bits(ds1307->regmap, M41TXX_REG_CONTROL, M41TXX_BIT_FT,
> + freq_test ? M41TXX_BIT_FT : 0);
> +
> + return retval ? retval : count;
Does the condition make any sense?
> +}
> +static ssize_t frequency_test_enable_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + int freq_test_en = 0;
> + if (ctrl_reg & M41TXX_BIT_FT)
> + freq_test_en = true;
> + else
> + freq_test_en = false;
> +
> + return sprintf(buf, "%d\n", freq_test_en);
So, is it boolean or integer? This code makes it confusing a lot.
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 1/3] rtc: stm32: rework register management to prepare other version of RTC
From: Alexandre Belloni @ 2018-05-16 20:25 UTC (permalink / raw)
To: Amelie Delaunay
Cc: Alessandro Zummo, Rob Herring, Mark Rutland, Maxime Coquelin,
Alexandre Torgue, linux-rtc, devicetree, linux-arm-kernel,
linux-kernel
In-Reply-To: <1525880770-22263-2-git-send-email-amelie.delaunay@st.com>
Hi,
On 09/05/2018 17:46:08+0200, Amelie Delaunay wrote:
> static void stm32_rtc_wpr_unlock(struct stm32_rtc *rtc)
> {
> - writel_relaxed(RTC_WPR_1ST_KEY, rtc->base + STM32_RTC_WPR);
> - writel_relaxed(RTC_WPR_2ND_KEY, rtc->base + STM32_RTC_WPR);
> + struct stm32_rtc_registers regs = rtc->data->regs;
regs should probably be a pointer to ensure that no copy is made. I've
actually checked and it doesn't make a difference because gcc is smart
enough to not make the copy.
> +
> + writel_relaxed(RTC_WPR_1ST_KEY, rtc->base + regs.wpr);
> + writel_relaxed(RTC_WPR_2ND_KEY, rtc->base + regs.wpr);
> }
>
...
> static irqreturn_t stm32_rtc_alarm_irq(int irq, void *dev_id)
> {
> struct stm32_rtc *rtc = (struct stm32_rtc *)dev_id;
> - unsigned int isr, cr;
> + struct stm32_rtc_registers regs = rtc->data->regs;
> + struct stm32_rtc_events evts = rtc->data->events;
Ditto for evts.
> + unsigned int status, cr;
>
> mutex_lock(&rtc->rtc_dev->ops_lock);
>
> - isr = readl_relaxed(rtc->base + STM32_RTC_ISR);
> - cr = readl_relaxed(rtc->base + STM32_RTC_CR);
> + status = readl_relaxed(rtc->base + regs.isr);
> + cr = readl_relaxed(rtc->base + regs.cr);
>
> - if ((isr & STM32_RTC_ISR_ALRAF) &&
> + if ((status & evts.alra) &&
> (cr & STM32_RTC_CR_ALRAIE)) {
> /* Alarm A flag - Alarm interrupt */
> dev_dbg(&rtc->rtc_dev->dev, "Alarm occurred\n");
...
> @@ -641,7 +710,7 @@ static int stm32_rtc_probe(struct platform_device *pdev)
>
> /*
> * After a system reset, RTC_ISR.INITS flag can be read to check if
> - * the calendar has been initalized or not. INITS flag is reset by a
> + * the calendar has been initialized or not. INITS flag is reset by a
> * power-on reset (no vbat, no power-supply). It is not reset if
> * rtc_ck parent clock has changed (so RTC prescalers need to be
> * changed). That's why we cannot rely on this flag to know if RTC
> @@ -666,7 +735,7 @@ static int stm32_rtc_probe(struct platform_device *pdev)
> "alarm won't be able to wake up the system");
>
> rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, pdev->name,
> - &stm32_rtc_ops, THIS_MODULE);
> + &stm32_rtc_ops, THIS_MODULE);
> if (IS_ERR(rtc->rtc_dev)) {
> ret = PTR_ERR(rtc->rtc_dev);
> dev_err(&pdev->dev, "rtc device registration failed, err=%d\n",
Those two changes should go into a separate cleanup patch.
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v2 3/3] rtc: stm32: add stm32mp1 rtc support
From: Alexandre Belloni @ 2018-05-16 20:32 UTC (permalink / raw)
To: Amelie Delaunay
Cc: Alessandro Zummo, Rob Herring, Mark Rutland, Maxime Coquelin,
Alexandre Torgue, linux-rtc, devicetree, linux-arm-kernel,
linux-kernel
In-Reply-To: <1525880770-22263-4-git-send-email-amelie.delaunay@st.com>
On 09/05/2018 17:46:10+0200, Amelie Delaunay wrote:
> struct stm32_rtc_registers {
> @@ -86,6 +98,9 @@ struct stm32_rtc_registers {
> u8 prer;
> u8 alrmar;
> u8 wpr;
> + u8 sr;
> + u8 scr;
> + u16 verr;
All those offsets should probably be u16 or u32...
> + if (regs.verr != UNDEF_REG) {
...else, this is not working, as reported by kbuild
> + u32 ver = readl_relaxed(rtc->base + regs.verr);
> +
> + dev_info(&pdev->dev, "registered rev:%d.%d\n",
> + (ver >> STM32_RTC_VERR_MAJREV_SHIFT) & 0xF,
> + (ver >> STM32_RTC_VERR_MINREV_SHIFT) & 0xF);
> + }
> +
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v2 01/40] iommu: Introduce Shared Virtual Addressing API
From: Jacob Pan @ 2018-05-16 20:41 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: kvm-u79uwXL29TY76Z2rM5mHXA, linux-pci-u79uwXL29TY76Z2rM5mHXA,
xuzaibo-hv44wF8Li93QT0dZR+AlfA, will.deacon-5wv7dgnIgG8,
okaya-sgV2jX0FEOL9JmXXK+q4OQ, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
ashok.raj-ral2JQCrhuEAvxtiuMwx3w, bharatku-gjFFaj9aHVfQT0dZR+AlfA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA, rfranz-YGCgFSpz5w/QT0dZR+AlfA,
devicetree-u79uwXL29TY76Z2rM5mHXA, rgummal-gjFFaj9aHVfQT0dZR+AlfA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
dwmw2-wEGCiKHe2LqWVfeAwA7xHQ,
ilias.apalodimas-QSEj5FYQhm4dnm+yROfE0A,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
christian.koenig-5C7GfCeVMHo
In-Reply-To: <20180511190641.23008-2-jean-philippe.brucker-5wv7dgnIgG8@public.gmane.org>
On Fri, 11 May 2018 20:06:02 +0100
Jean-Philippe Brucker <jean-philippe.brucker-5wv7dgnIgG8@public.gmane.org> wrote:
> Shared Virtual Addressing (SVA) provides a way for device drivers to
> bind process address spaces to devices. This requires the IOMMU to
> support page table format and features compatible with the CPUs, and
> usually requires the system to support I/O Page Faults (IOPF) and
> Process Address Space ID (PASID). When all of these are available,
> DMA can access virtual addresses of a process. A PASID is allocated
> for each process, and the device driver programs it into the device
> in an implementation-specific way.
>
> Add a new API for sharing process page tables with devices. Introduce
> two IOMMU operations, sva_device_init() and sva_device_shutdown(),
> that prepare the IOMMU driver for SVA. For example allocate PASID
> tables and fault queues. Subsequent patches will implement the bind()
> and unbind() operations.
>
> Support for I/O Page Faults will be added in a later patch using a new
> feature bit (IOMMU_SVA_FEAT_IOPF). With the current API users must pin
> down all shared mappings. Other feature bits that may be added in the
> future are IOMMU_SVA_FEAT_PRIVATE, to support private PASID address
> spaces, and IOMMU_SVA_FEAT_NO_PASID, to bind the whole device address
> space to a process.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker-5wv7dgnIgG8@public.gmane.org>
>
> ---
> v1->v2:
> * Add sva_param structure to iommu_param
> * CONFIG option is only selectable by IOMMU drivers
> ---
> drivers/iommu/Kconfig | 4 ++
> drivers/iommu/Makefile | 1 +
> drivers/iommu/iommu-sva.c | 110
> ++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h |
> 32 +++++++++++ 4 files changed, 147 insertions(+)
> create mode 100644 drivers/iommu/iommu-sva.c
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 7564237f788d..cca8e06903c7 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -74,6 +74,10 @@ config IOMMU_DMA
> select IOMMU_IOVA
> select NEED_SG_DMA_LENGTH
>
> +config IOMMU_SVA
> + bool
> + select IOMMU_API
> +
> config FSL_PAMU
> bool "Freescale IOMMU support"
> depends on PCI
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 1fb695854809..1dbcc89ebe4c 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_IOMMU_API) += iommu.o
> obj-$(CONFIG_IOMMU_API) += iommu-traces.o
> obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
> obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
> +obj-$(CONFIG_IOMMU_SVA) += iommu-sva.o
> obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
> obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
> obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> new file mode 100644
> index 000000000000..8b4afb7c63ae
> --- /dev/null
> +++ b/drivers/iommu/iommu-sva.c
> @@ -0,0 +1,110 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Manage PASIDs and bind process address spaces to devices.
> + *
> + * Copyright (C) 2018 ARM Ltd.
> + */
> +
> +#include <linux/iommu.h>
> +#include <linux/slab.h>
> +
> +/**
> + * iommu_sva_device_init() - Initialize Shared Virtual Addressing
> for a device
> + * @dev: the device
> + * @features: bitmask of features that need to be initialized
> + * @max_pasid: max PASID value supported by the device
> + *
> + * Users of the bind()/unbind() API must call this function to
> initialize all
> + * features required for SVA.
> + *
> + * The device must support multiple address spaces (e.g. PCI PASID).
> By default
> + * the PASID allocated during bind() is limited by the IOMMU
> capacity, and by
> + * the device PASID width defined in the PCI capability or in the
> firmware
> + * description. Setting @max_pasid to a non-zero value smaller than
> this limit
> + * overrides it.
> + *
seems the min_pasid never gets used. do you really need it?
> + * The device should not be performing any DMA while this function
> is running,
> + * otherwise the behavior is undefined.
> + *
> + * Return 0 if initialization succeeded, or an error.
> + */
> +int iommu_sva_device_init(struct device *dev, unsigned long features,
> + unsigned int max_pasid)
> +{
> + int ret;
> + struct iommu_sva_param *param;
> + struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> +
> + if (!domain || !domain->ops->sva_device_init)
> + return -ENODEV;
> +
> + if (features)
> + return -EINVAL;
should it be !features?
> +
> + param = kzalloc(sizeof(*param), GFP_KERNEL);
> + if (!param)
> + return -ENOMEM;
> +
> + param->features = features;
> + param->max_pasid = max_pasid;
> +
> + /*
> + * IOMMU driver updates the limits depending on the IOMMU
> and device
> + * capabilities.
> + */
> + ret = domain->ops->sva_device_init(dev, param);
> + if (ret)
> + goto err_free_param;
> +
> + mutex_lock(&dev->iommu_param->lock);
> + if (dev->iommu_param->sva_param)
> + ret = -EEXIST;
> + else
> + dev->iommu_param->sva_param = param;
> + mutex_unlock(&dev->iommu_param->lock);
> + if (ret)
> + goto err_device_shutdown;
> +
> + return 0;
> +
> +err_device_shutdown:
> + if (domain->ops->sva_device_shutdown)
> + domain->ops->sva_device_shutdown(dev, param);
> +
> +err_free_param:
> + kfree(param);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(iommu_sva_device_init);
> +
> +/**
> + * iommu_sva_device_shutdown() - Shutdown Shared Virtual Addressing
> for a device
> + * @dev: the device
> + *
> + * Disable SVA. Device driver should ensure that the device isn't
> performing any
> + * DMA while this function is running.
> + */
> +int iommu_sva_device_shutdown(struct device *dev)
> +{
> + struct iommu_sva_param *param;
> + struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> +
> + if (!domain)
> + return -ENODEV;
> +
> + mutex_lock(&dev->iommu_param->lock);
> + param = dev->iommu_param->sva_param;
> + dev->iommu_param->sva_param = NULL;
> + mutex_unlock(&dev->iommu_param->lock);
> + if (!param)
> + return -ENODEV;
> +
> + if (domain->ops->sva_device_shutdown)
> + domain->ops->sva_device_shutdown(dev, param);
seems a little mismatch here, do you need pass the param. I don't think
there is anything else model specific iommu driver need to do for the
param.
> +
> + kfree(param);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_sva_device_shutdown);
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 0933f726d2e6..2efe7738bedb 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -212,6 +212,12 @@ struct page_response_msg {
> u64 private_data;
> };
>
> +struct iommu_sva_param {
> + unsigned long features;
> + unsigned int min_pasid;
> + unsigned int max_pasid;
> +};
> +
> /**
> * struct iommu_ops - iommu ops and capabilities
> * @capable: check capability
> @@ -219,6 +225,8 @@ struct page_response_msg {
> * @domain_free: free iommu domain
> * @attach_dev: attach device to an iommu domain
> * @detach_dev: detach device from an iommu domain
> + * @sva_device_init: initialize Shared Virtual Adressing for a device
> + * @sva_device_shutdown: shutdown Shared Virtual Adressing for a
> device
> * @map: map a physically contiguous memory region to an iommu domain
> * @unmap: unmap a physically contiguous memory region from an iommu
> domain
> * @map_sg: map a scatter-gather list of physically contiguous
> memory chunks @@ -256,6 +264,10 @@ struct iommu_ops {
>
> int (*attach_dev)(struct iommu_domain *domain, struct device
> *dev); void (*detach_dev)(struct iommu_domain *domain, struct device
> *dev);
> + int (*sva_device_init)(struct device *dev,
> + struct iommu_sva_param *param);
> + void (*sva_device_shutdown)(struct device *dev,
> + struct iommu_sva_param *param);
> int (*map)(struct iommu_domain *domain, unsigned long iova,
> phys_addr_t paddr, size_t size, int prot);
> size_t (*unmap)(struct iommu_domain *domain, unsigned long
> iova, @@ -413,6 +425,7 @@ struct iommu_fault_param {
> * struct iommu_param - collection of per-device IOMMU data
> *
> * @fault_param: IOMMU detected device fault reporting data
> + * @sva_param: SVA parameters
> *
> * TODO: migrate other per device data pointers under
> iommu_dev_data, e.g.
> * struct iommu_group *iommu_group;
> @@ -421,6 +434,7 @@ struct iommu_fault_param {
> struct iommu_param {
> struct mutex lock;
> struct iommu_fault_param *fault_param;
> + struct iommu_sva_param *sva_param;
> };
>
> int iommu_device_register(struct iommu_device *iommu);
> @@ -920,4 +934,22 @@ static inline int iommu_sva_invalidate(struct
> iommu_domain *domain,
> #endif /* CONFIG_IOMMU_API */
>
> +#ifdef CONFIG_IOMMU_SVA
> +extern int iommu_sva_device_init(struct device *dev, unsigned long
> features,
> + unsigned int max_pasid);
> +extern int iommu_sva_device_shutdown(struct device *dev);
> +#else /* CONFIG_IOMMU_SVA */
> +static inline int iommu_sva_device_init(struct device *dev,
> + unsigned long features,
> + unsigned int max_pasid)
> +{
> + return -ENODEV;
> +}
> +
> +static inline int iommu_sva_device_shutdown(struct device *dev)
> +{
> + return -ENODEV;
> +}
> +#endif /* CONFIG_IOMMU_SVA */
> +
> #endif /* __LINUX_IOMMU_H */
[Jacob Pan]
^ permalink raw reply
* Re: [PATCH v6 2/2] leds: lm3601x: Introduce the lm3601x LED driver
From: Jacek Anaszewski @ 2018-05-16 20:43 UTC (permalink / raw)
To: Dan Murphy, robh+dt, mark.rutland, pavel
Cc: devicetree, linux-kernel, linux-leds
In-Reply-To: <ab12334c-99ec-648c-8917-9dd5f316fd97@ti.com>
Dan,
On 05/15/2018 11:50 PM, Dan Murphy wrote:
> Jacek
>
> On 05/15/2018 04:24 PM, Jacek Anaszewski wrote:
>> Dan,
>>
>> Thanks for the update. Please refer to my comments below.
>>
>> On 05/15/2018 05:43 PM, Dan Murphy wrote:
>>> Introduce the family of LED devices that can
>>> drive a torch, strobe or IR LED.
>>>
>>> The LED driver can be configured with a strobe
>>> timer to execute a strobe flash. The IR LED
>>> brightness is controlled via the torch brightness
>>> register.
>>>
>>> The data sheet for each the LM36010 and LM36011
>>> LED drivers can be found here:
>>> http://www.ti.com/product/LM36010
>>> http://www.ti.com/product/LM36011
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>> ---
>>>
>>> v6 - This driver has been heavily modified from v5. There is no longer reading
>>> of individual child nodes. There are too many changes to list here see -
>>> https://patchwork.kernel.org/patch/10392123/
>>>
>>> v5 - Fixed magic numbers, change reg cache type, added of_put_node to release
>>> the dt node ref, and I did not change the remove function to leave the LED in its
>>> state on driver removal - https://patchwork.kernel.org/patch/10391741/
>>> v4 - Fixed Cocci issue using ARRAY_SIZE - https://patchwork.kernel.org/patch/10389259/
>>> v3 - removed wildcard dt compatible, fixed copyright, fixed struct doc, removed
>>> RO registers from default, added regmap volatile for FLAGS_REG, updated regmap cache type,
>>> fixed unlock and extra semi colon in strobe_set, removed unnecessary out label
>>> in led register and fixed checking of the ret in brightness_set - https://patchwork.kernel.org/patch/10386243/
>>> v2 - Fixed kbuild issue and removed unused cdev_strobe - https://patchwork.kernel.org/patch/10384585/
>>>
>>> drivers/leds/Kconfig | 9 +
>>> drivers/leds/Makefile | 1 +
>>> drivers/leds/leds-lm3601x.c | 595 ++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 605 insertions(+)
>>> create mode 100644 drivers/leds/leds-lm3601x.c
>>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index 2c896c0e69e1..50ae536f343f 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -145,6 +145,15 @@ config LEDS_LM3692X
>>> This option enables support for the TI LM3692x family
>>> of white LED string drivers used for backlighting.
>>> +config LEDS_LM3601X
>>> + tristate "LED support for LM3601x Chips"
>>> + depends on LEDS_CLASS && I2C && OF
>>> + depends on LEDS_CLASS_FLASH
>>> + select REGMAP_I2C
>>> + help
>>> + This option enables support for the TI LM3601x family
>>> + of flash, torch and indicator classes.
>>> +
>>> config LEDS_LOCOMO
>>> tristate "LED Support for Locomo device"
>>> depends on LEDS_CLASS
>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>> index 91eca81cae82..b79807fe1b67 100644
>>> --- a/drivers/leds/Makefile
>>> +++ b/drivers/leds/Makefile
>>> @@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_MLXREG) += leds-mlxreg.o
>>> obj-$(CONFIG_LEDS_NIC78BX) += leds-nic78bx.o
>>> obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o
>>> obj-$(CONFIG_LEDS_LM3692X) += leds-lm3692x.o
>>> +obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o
>>> # LED SPI Drivers
>>> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
>>> diff --git a/drivers/leds/leds-lm3601x.c b/drivers/leds/leds-lm3601x.c
>>> new file mode 100644
>>> index 000000000000..fa87da5d5159
>>> --- /dev/null
>>> +++ b/drivers/leds/leds-lm3601x.c
>>> @@ -0,0 +1,595 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +// Flash and torch driver for Texas Instruments LM3601X LED
>>> +// Flash driver chip family
>>> +// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
>>> +
>>> +#include <linux/delay.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/leds.h>
>>> +#include <linux/led-class-flash.h>
>>> +#include <linux/module.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/slab.h>
>>> +#include <uapi/linux/uleds.h>
>>> +
>>> +#define LM3601X_LED_TORCH 0x0
>>> +#define LM3601X_LED_IR 0x1
>>> +
>>> +/* Registers */
>>> +#define LM3601X_ENABLE_REG 0x01
>>> +#define LM3601X_CFG_REG 0x02
>>> +#define LM3601X_LED_FLASH_REG 0x03
>>> +#define LM3601X_LED_TORCH_REG 0x04
>>> +#define LM3601X_FLAGS_REG 0x05
>>> +#define LM3601X_DEV_ID_REG 0x06
>>> +
>>> +#define LM3601X_SW_RESET BIT(7)
>>> +
>>> +/* Enable Mode bits */
>>> +#define LM3601X_MODE_STANDBY 0x00
>>> +#define LM3601X_MODE_IR_DRV BIT(0)
>>> +#define LM3601X_MODE_TORCH BIT(1)
>>> +#define LM3601X_MODE_STROBE (BIT(0) | BIT(1))
>>> +#define LM3601X_STRB_EN BIT(2)
>>> +#define LM3601X_STRB_LVL_TRIG ~BIT(3)
>>> +#define LM3601X_STRB_EDGE_TRIG BIT(3)
>>> +#define LM3601X_IVFM_EN BIT(4)
>>> +
>>> +#define LM36010_BOOST_LIMIT_19 ~BIT(5)
>>> +#define LM36010_BOOST_LIMIT_28 BIT(5)
>>> +#define LM36010_BOOST_FREQ_2MHZ ~BIT(6)
>>> +#define LM36010_BOOST_FREQ_4MHZ BIT(6)
>>> +#define LM36010_BOOST_MODE_NORM ~BIT(7)
>>> +#define LM36010_BOOST_MODE_PASS BIT(7)
>>> +
>>> +/* Flag Mask */
>>> +#define LM3601X_FLASH_TIME_OUT BIT(0)
>>> +#define LM3601X_UVLO_FAULT BIT(1)
>>> +#define LM3601X_THERM_SHUTDOWN BIT(2)
>>> +#define LM3601X_THERM_CURR BIT(3)
>>> +#define LM36010_CURR_LIMIT BIT(4)
>>> +#define LM3601X_SHORT_FAULT BIT(5)
>>> +#define LM3601X_IVFM_TRIP BIT(6)
>>> +#define LM36010_OVP_FAULT BIT(7)
>>> +
>>> +#define LM3601X_MIN_TORCH_I_UA 2400
>>> +#define LM3601X_MIN_STROBE_I_MA 11
>>> +
>>> +#define LM3601X_TIMEOUT_MASK 0x1e
>>> +#define LM3601X_ENABLE_MASK 0x03
>>> +
>>> +enum lm3601x_type {
>>> + CHIP_LM36010 = 0,
>>> + CHIP_LM36011,
>>> +};
>>> +
>>> +/**
>>> + * struct lm3601x_max_timeouts -
>>> + * @timeout: timeout value in ms
>>> + * @regval: the value of the register to write
>>> + */
>>> +struct lm3601x_max_timeouts {
>>> + int timeout;
>>> + int reg_val;
>>> +};
>>> +
>>> +/**
>>> + * struct lm3601x_led -
>>> + * @lock: Lock for reading/writing the device
>>> + * @regmap: Devices register map
>>> + * @client: Pointer to the I2C client
>>> + * @led_node: DT device node for the led
>>> + * @cdev_torch: led class device pointer for the torch
>>> + * @cdev_ir: led class device pointer for infrared
>>> + * @fled_cdev: flash led class device pointer
>>> + * @led_name: LED label for the Torch or IR LED
>>> + * @strobe: LED label for the strobe
>>> + * @last_flag: last known flags register value
>>> + * @strobe_timeout: the timeout for the strobe
>>> + * @torch_current_max: maximum current for the torch
>>> + * @strobe_current_max: maximum current for the strobe
>>> + * @max_strobe_timeout: maximum timeout for the strobe
>>> + * @led_mode: The mode to enable either IR or Torch
>>> + */
>>> +struct lm3601x_led {
>>> + struct mutex lock;
>>> + struct regmap *regmap;
>>> + struct i2c_client *client;
>>> +
>>> + struct device_node *led_node;
>>> +
>>> + struct led_classdev cdev_torch;
>>> + struct led_classdev cdev_ir;
>>> +
>>> + struct led_classdev_flash fled_cdev;
>>> +
>>> + char led_name[LED_MAX_NAME_SIZE];
>>> + char strobe[LED_MAX_NAME_SIZE];
>>> +
>>> + unsigned int last_flag;
>>> + unsigned int strobe_timeout;
>>> +
>>> + u32 torch_current_max;
>>> + u32 strobe_current_max;
>>> + u32 max_strobe_timeout;
>>> +
>>> + int led_mode;
>>> +};
>>> +
>>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = {
>>> + { 40000, 0x00 },
>>> + { 80000, 0x01 },
>>> + { 120000, 0x02 },
>>> + { 160000, 0x03 },
>>> + { 200000, 0x04 },
>>> + { 240000, 0x05 },
>>> + { 280000, 0x06 },
>>> + { 320000, 0x07 },
>>> + { 360000, 0x08 },
>>> + { 400000, 0x09 },
>>> + { 600000, 0x0a },
>>> + { 800000, 0x0b },
>>> + { 1000000, 0x0c },
>>> + { 1200000, 0x0d },
>>> + { 1400000, 0x0e },
>>> + { 1600000, 0x0f },
>>> +};
>>> +
>>> +static const struct reg_default lm3601x_regmap_defs[] = {
>>> + { LM3601X_ENABLE_REG, 0x20 },
>>> + { LM3601X_CFG_REG, 0x15 },
>>> + { LM3601X_LED_FLASH_REG, 0x00 },
>>> + { LM3601X_LED_TORCH_REG, 0x00 },
>>> +};
>>> +
>>> +static bool lm3601x_volatile_reg(struct device *dev, unsigned int reg)
>>> +{
>>> + switch (reg) {
>>> + case LM3601X_FLAGS_REG:
>>> + return true;
>>> + default:
>>> + return false;
>>> + }
>>> +}
>>> +
>>> +static const struct regmap_config lm3601x_regmap = {
>>> + .reg_bits = 8,
>>> + .val_bits = 8,
>>> +
>>> + .max_register = LM3601X_DEV_ID_REG,
>>> + .reg_defaults = lm3601x_regmap_defs,
>>> + .num_reg_defaults = ARRAY_SIZE(lm3601x_regmap_defs),
>>> + .cache_type = REGCACHE_RBTREE,
>>> + .volatile_reg = lm3601x_volatile_reg,
>>> +};
>>> +
>>> +static struct lm3601x_led *fled_cdev_to_led(
>>> + struct led_classdev_flash *fled_cdev)
>>> +{
>>> + return container_of(fled_cdev, struct lm3601x_led, fled_cdev);
>>> +}
>>> +
>>> +static int lm3601x_read_faults(struct lm3601x_led *led)
>>> +{
>>> + int flags_val;
>>> + int ret;
>>> +
>>> + ret = regmap_read(led->regmap, LM3601X_FLAGS_REG, &flags_val);
>>> + if (ret < 0)
>>> + return -EIO;
>>> +
>>> + led->last_flag = 0;
>>> +
>>> + if (flags_val & LM36010_OVP_FAULT)
>>> + led->last_flag |= LED_FAULT_OVER_VOLTAGE;
>>> +
>>> + if (flags_val & (LM3601X_THERM_SHUTDOWN | LM3601X_THERM_CURR))
>>> + led->last_flag |= LED_FAULT_OVER_TEMPERATURE;
>>> +
>>> + if (flags_val & LM3601X_SHORT_FAULT)
>>> + led->last_flag |= LED_FAULT_SHORT_CIRCUIT;
>>> +
>>> + if (flags_val & LM36010_CURR_LIMIT)
>>> + led->last_flag |= LED_FAULT_OVER_CURRENT;
>>> +
>>> + if (flags_val & LM3601X_UVLO_FAULT)
>>> + led->last_flag |= LED_FAULT_UNDER_VOLTAGE;
>>> +
>>> + if (flags_val & LM3601X_IVFM_TRIP)
>>> + led->last_flag |= LED_FAULT_INPUT_VOLTAGE;
>>> +
>>> + if (flags_val & LM3601X_THERM_SHUTDOWN)
>>> + led->last_flag |= LED_FAULT_LED_OVER_TEMPERATURE;
>>> +
>>> + return led->last_flag;
>>> +}
>>> +
>>> +static int lm3601x_brightness_set(struct led_classdev *cdev,
>>> + enum led_brightness brightness)
>>> +{
>>> + struct lm3601x_led *led =
>>> + container_of(cdev, struct lm3601x_led, cdev_torch);
>>> + u8 brightness_val;
>>> + int ret, led_mode_val;
>>> +
>>> + mutex_lock(&led->lock);
>>> +
>>> + ret = lm3601x_read_faults(led);
>>> + if (ret < 0)
>>> + goto out;
>>> +
>>> + if (led->led_mode == LM3601X_LED_TORCH)
>>> + led_mode_val = LM3601X_MODE_TORCH;
>>> + else
>>> + led_mode_val = LM3601X_MODE_IR_DRV;
>>> +
>>> + if (brightness == LED_OFF) {
>>> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
>>> + led_mode_val, LED_OFF);
>>> + goto out;
>>> + }
>>> +
>>> + if (brightness == LED_ON)
>>> + brightness_val = LED_ON;
>>> + else
>>> + brightness_val = (brightness/2);
>>> +
>>> + ret = regmap_write(led->regmap, LM3601X_LED_TORCH_REG, brightness_val);
>>> + if (ret < 0)
>>> + goto out;
>>> +
>>> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
>>> + led_mode_val,
>>> + led_mode_val);
>>> +
>>> +out:
>>> + mutex_unlock(&led->lock);
>>> + return ret;
>>> +}
>>> +
>>> +static int lm3601x_strobe_set(struct led_classdev_flash *fled_cdev,
>>> + bool state)
>>> +{
>>> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
>>> + int ret;
>>> + int current_timeout;
>>> + int reg_count;
>>> + int i;
>>> + int timeout_reg_val = 0;
>>> +
>>> + mutex_lock(&led->lock);
>>> +
>>> + ret = regmap_read(led->regmap, LM3601X_CFG_REG, ¤t_timeout);
>>> + if (ret < 0)
>>> + goto out;
>>> +
>>> + reg_count = ARRAY_SIZE(strobe_timeouts);
>>> + for (i = 0; i < reg_count; i++) {
>>> + if (led->strobe_timeout > strobe_timeouts[i].timeout)
>>> + continue;
>>> +
>>> + if (led->strobe_timeout <= strobe_timeouts[i].timeout) {
>>> + timeout_reg_val = (strobe_timeouts[i].reg_val << 1);
>>> + break;
>>> + }
>>> +
>>> + ret = -EINVAL;
>>> + goto out;
>>> + }
>>> +
>>> + if (led->strobe_timeout != current_timeout)
>>> + ret = regmap_update_bits(led->regmap, LM3601X_CFG_REG,
>>> + LM3601X_TIMEOUT_MASK, timeout_reg_val);
>>> +
>>> + if (state)
>>> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
>>> + LM3601X_MODE_STROBE,
>>> + LM3601X_MODE_STROBE);
>>> + else
>>> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
>>> + LM3601X_MODE_STROBE, LED_OFF);
>>> +
>>> + ret = lm3601x_read_faults(led);
>>> +out:
>>> + mutex_unlock(&led->lock);
>>> + return ret;
>>> +}
>>> +
>>> +static int lm3601x_strobe_brightness_set(struct led_classdev *cdev,
>>> + enum led_brightness brightness)
>>> +{
>>> + struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev);
>>> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
>>> + int ret;
>>> + u8 brightness_val;
>>> +
>>> + mutex_lock(&led->lock);
>>> + ret = lm3601x_read_faults(led);
>>> + if (ret < 0)
>>> + goto out;
>>> +
>>> + if (brightness == LED_OFF) {
>>> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
>>> + LM3601X_MODE_STROBE, LED_OFF);
>>> + goto out;
>>> + }
>>> +
>>> + if (brightness == LED_ON)
>>> + brightness_val = LED_ON;
>>> + else
>>> + brightness_val = (brightness/2);
>>> +
>>> + ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val);
>>> +
>>> +out:
>>> + mutex_unlock(&led->lock);
>>> + return ret;
>>> +}
>>> +
>>> +static int lm3601x_strobe_timeout_set(struct led_classdev_flash *fled_cdev,
>>> + u32 timeout)
>>> +{
>>> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
>>> + int ret = 0;
>>> +
>>> + mutex_lock(&led->lock);
>>> +
>>> + led->strobe_timeout = timeout;
>>> +
>>> + mutex_unlock(&led->lock);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int lm3601x_strobe_get(struct led_classdev_flash *fled_cdev,
>>> + bool *state)
>>> +{
>>> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
>>> + int ret;
>>> + int strobe_state;
>>> +
>>> + mutex_lock(&led->lock);
>>> +
>>> + ret = regmap_read(led->regmap, LM3601X_ENABLE_REG, &strobe_state);
>>> + if (ret < 0)
>>> + goto out;
>>> +
>>> + *state = strobe_state & LM3601X_MODE_STROBE;
>>> +
>>> +out:
>>> + mutex_unlock(&led->lock);
>>> + return ret;
>>> +}
>>> +
>>> +static int lm3601x_strobe_fault_get(struct led_classdev_flash *fled_cdev,
>>> + u32 *fault)
>>> +{
>>> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
>>> +
>>> + lm3601x_read_faults(led);
>>> +
>>> + *fault = led->last_flag;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct led_flash_ops strobe_ops = {
>>> + .strobe_set = lm3601x_strobe_set,
>>> + .strobe_get = lm3601x_strobe_get,
>>> + .timeout_set = lm3601x_strobe_timeout_set,
>>> + .fault_get = lm3601x_strobe_fault_get,
>>
>> You're still missing flash_brightness_set here.
>
> OK not sure how this is working then.
flash_brightness_set sets the brightness that is to be applied
upon execution of strobe_set.
>
>>
>>> +};
>>> +
>>> +static int lm3601x_register_leds(struct lm3601x_led *led)
>>> +{
>>> + struct led_classdev_flash *fled_cdev;
>>> + struct led_classdev *led_cdev;
>>> + int err = -ENODEV;
>>> +
>>> + led->cdev_torch.name = led->led_name;
>>> + led->cdev_torch.max_brightness = LED_FULL;
>>
>> This should be led->torch_current_max converted to brightness levels.
>> Please compare how leds-max77693 and leds-as3645a do that.
>
> I will look into it.
>
>>
>>> + led->cdev_torch.brightness_set_blocking = lm3601x_brightness_set;
>>> + err = devm_led_classdev_register(&led->client->dev,
>>> + &led->cdev_torch);
>>> + if (err < 0)
>>> + return err;
>>
>> You shouldn't register two LED class devices for a single DT child
>> node, and honestly I don't know why you would have to do it.
>
> See below
>
>>
>>> +
>>> + fled_cdev = &led->fled_cdev;
>>> + fled_cdev->ops = &strobe_ops;
>>> +
>>> + led_cdev = &fled_cdev->led_cdev;
>>> + led_cdev->name = led->strobe;
>>> + led_cdev->max_brightness = LED_FUL
>>> + led_cdev->brightness_set_blocking = lm3601x_strobe_brightness_set;
>>> + led_cdev->flags |= LED_DEV_CAP_FLASH;
>>> +
>>> + err = led_classdev_flash_register(&led->client->dev,
>>> + fled_cdev);
>>
>> Please look into the implementation of led_classdev_flash_register()
>> and compare other LED flash class drivers. What prevents you from
>> applying the same approach?
>
> I did actually follow the same approach here as the as3645a driver.
> I used this driver since I have a data sheet and can read the code.
You might have been misled by the indicator LED which is indeed
a separate one, but it doesn't have a counterpart in your driver.
But see the snippets from that driver:
This is for setting indicator brightness:
------------------------------------------
iled_cdev->brightness_set_blocking = as3645a_set_indicator_brightness;
This is for setting torch brightness:
-------------------------------------
fled_cdev->brightness_set_blocking = as3645a_set_assist_brightness;
This is for setting flash brightness:
-------------------------------------
static const struct led_flash_ops as3645a_led_flash_ops = {
.flash_brightness_set = as3645a_set_flash_brightness,
> This is a copy, paste, slight modify from that as3645a driver as3645a_led_class_setup function.
>
> I used the devm register function above as a variation.
>
> I will have to dig further into it.
>
>>
>> To put it concisely: LED flash class wrapper was designed to allow
>> for operating on a single LED in two modes: torch and flash.
>> Torch brightness is governed by brightness file and flash brightness
>> is governed by flash_brightness file. The difference between the two
>> modes is that torch mode is activated by writing value greater than 0
>> to the brightness file and flash mode is activated by writing 1 to
>> the strobe_set file.
>>
>> In other words flash_brightness overwrites the (torch) brightness
>> for the time equal to the flash_timeout or until 0 is written
>> to the strobe_set file.
>
> I will look at it again
>
>>
>>> + return err;
>>> +}
>>> +
>>> +static void lm3601x_init_flash_timeout(struct lm3601x_led *led)
>>> +{
>>> + struct led_flash_setting *setting;
>>> +
>>> + setting = &led->fled_cdev.timeout;
>>> + setting->min = strobe_timeouts[0].timeout;
>>> + setting->max = led->max_strobe_timeout;
>>> + setting->step = 40;
>>> + setting->val = led->max_strobe_timeout;
>>> +}
>>> +
>>> +static int lm3601x_parse_node(struct lm3601x_led *led,
>>> + struct device_node *node)
>>> +{
>>> + struct device_node *child_node;
>>> + const char *name;
>>> + char *mode_name;
>>> + int ret = -ENODEV;
>>> +
>>> + for_each_available_child_of_node(node, child_node) {
>>> + led->led_node = of_node_get(child_node);
>>> + if (!led->led_node) {
>>> + dev_err(&led->client->dev,
>>> + "No LED Child node\n");
>>> +
>>> + goto out_err;
>>> + }
>>> +
>>> + ret = of_property_read_u32(led->led_node, "led-sources",
>>> + &led->led_mode);
>>> + if (ret) {
>>> + dev_err(&led->client->dev,
>>> + "led-sources DT property missing\n");
>>> + goto out_err;
>>> + }
>>> +
>>> + if (led->led_mode < LM3601X_LED_TORCH ||
>>> + led->led_mode > LM3601X_LED_IR) {
>>> + dev_warn(&led->client->dev,
>>> + "Invalid led mode requested\n");
>>> +
>>> + goto out_err;
>>> +
>>> + }
>>> + }
>>> +
>>> + if (led->led_mode == LM3601X_LED_TORCH) {
>>> + ret = of_property_read_string(led->led_node, "label", &name);
>>> + if (!ret)
>>> + snprintf(led->led_name, sizeof(led->led_name),
>>> + "%s:%s", led->led_node->name, name);
>>
>> First segment of a LED class device name should be "devicename".
>
> Hmm. I remember this conversation from my other drivers.
> I just picked the wrong dt node name.
>
> I will fix it.
>
>>
>>> + else
>>> + snprintf(led->led_name, sizeof(led->led_name),
>>> + "%s:torch", led->led_node->name);
>>> +
>>> + ret = of_property_read_u32(led->led_node, "led-max-microamp",
>>> + &led->torch_current_max);
>>> + if (ret < 0) {
>>> + dev_warn(&led->client->dev,
>>> + "led-max-microamp DT property missing\n");
>>> +
>>> + goto out_err;
>>> + }
>>> +
>>> + mode_name = "torch";
>>> +
>>> + } else if (led->led_mode == LM3601X_LED_IR) {
>>> + ret = of_property_read_string(led->led_node, "label", &name);
>>> + if (!ret)
>>> + snprintf(led->led_name, sizeof(led->led_name),
>>> + "%s:%s", led->led_node->name, name);
>>> + else
>>> + snprintf(led->led_name, sizeof(led->led_name),
>>> + "%s::infrared", led->led_node->name);
>>
>> Ditto.
>
> Ack
>
>>
>>> +
>>> + mode_name = "ir";
>>> +
>>> + } else {
>>> + dev_warn(&led->client->dev,
>>> + "No LED mode is selected exiting probe\n");
>>> +
>>> + goto out_err;
>>> + }
>>> +
>>> + /* Flash mode is available in IR or Torch mode so read the DT */
>>> + snprintf(led->strobe, sizeof(led->strobe),
>>> + "%s:%s:strobe", led->led_node->name, mode_name);
>>
>> strobe is not a proper LED function name (it is not a noun).
>> Please use "flash" instead.
>
> Strobe can be used as a noun to define an electronic camera flash. But this is
> really a US English definition.
I've checked in Cambridge Dictionary and I was in fact wrong - it can be
either a verb or a noun.
> I can change it to flash
We're using in few places the expression "to strobe the flash", so
let's use "flash" consistently to describe the LED function.
>>> +
>>> + ret = of_property_read_u32(led->led_node,
>>> + "flash-max-microamp",
>>> + &led->strobe_current_max);
>>> + if (ret < 0) {
>>> + dev_warn(&led->client->dev,
>>> + "flash-max-microamp DT property missing\n");
>>> + goto out_err;
>>> + }
>>> +
>>> + ret = of_property_read_u32(led->led_node,
>>> + "flash-max-timeout-us",
>>> + &led->max_strobe_timeout);
>>> + if (ret < 0) {
>>> + dev_warn(&led->client->dev,
>>> + "flash-max-timeout-us DT property missing\n");
>>> +
>>> + goto out_err;
>>> + }
>>> +
>>> + lm3601x_init_flash_timeout(led);
>>> +
>>> +out_err:
>>> + of_node_put(led->led_node);
>>> + return ret;
>>> +}
>>> +
>>> +static int lm3601x_probe(struct i2c_client *client,
>>> + const struct i2c_device_id *id)
>>> +{
>>> + struct lm3601x_led *led;
>>> + int err;
>>> +
>>> + led = devm_kzalloc(&client->dev,
>>> + sizeof(struct lm3601x_led), GFP_KERNEL);
>>> + if (!led)
>>> + return -ENOMEM;
>>> +
>>> + err = lm3601x_parse_node(led, client->dev.of_node);
>>> + if (err < 0)
>>> + return -ENODEV;
>>> +
>>> + led->client = client;
>>> + led->regmap = devm_regmap_init_i2c(client, &lm3601x_regmap);
>>> + if (IS_ERR(led->regmap)) {
>>> + err = PTR_ERR(led->regmap);
>>> + dev_err(&client->dev,
>>> + "Failed to allocate register map: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + mutex_init(&led->lock);
>>> + i2c_set_clientdata(client, led);
>>> + err = lm3601x_register_leds(led);
>>> +
>>> + return err;
>>> +}
>>> +
>>> +static int lm3601x_remove(struct i2c_client *client)
>>> +{
>>> + struct lm3601x_led *led = i2c_get_clientdata(client);
>>> +
>>> + regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
>>> + LM3601X_ENABLE_MASK,
>>> + LM3601X_MODE_STANDBY);
>>
>> You need also mutex_destroy(&led->lock) here.
>
> OK.
>
>>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct i2c_device_id lm3601x_id[] = {
>>> + { "LM36010", CHIP_LM36010 },
>>> + { "LM36011", CHIP_LM36011 },
>>> + { },
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, lm3601x_id);
>>> +
>>> +static const struct of_device_id of_lm3601x_leds_match[] = {
>>> + { .compatible = "ti,lm36010", },
>>> + { .compatible = "ti,lm36011", },
>>> + {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, of_lm3601x_leds_match);
>>> +
>>> +static struct i2c_driver lm3601x_i2c_driver = {
>>> + .driver = {
>>> + .name = "lm3601x",
>>> + .of_match_table = of_lm3601x_leds_match,
>>> + },
>>> + .probe = lm3601x_probe,
>>> + .remove = lm3601x_remove,
>>> + .id_table = lm3601x_id,
>>> +};
>>> +module_i2c_driver(lm3601x_i2c_driver);
>>> +
>>> +MODULE_DESCRIPTION("Texas Instruments Flash Lighting driver for LM3601X");
>>> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [PATCH v6 2/2] leds: lm3601x: Introduce the lm3601x LED driver
From: Dan Murphy @ 2018-05-16 20:43 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Rob Herring, Mark Rutland, Jacek Anaszewski, Pavel Machek,
devicetree, Linux Kernel Mailing List, Linux LED Subsystem
In-Reply-To: <CAHp75Vcz08zrs3bgMFcvYwRrN_A43D=Nvt5cFDQf8m9whB=yhA@mail.gmail.com>
Andy
The first response is to Jacek the rest are addressed to you
On 05/15/2018 05:24 PM, Andy Shevchenko wrote:
> On Wed, May 16, 2018 at 1:08 AM, Dan Murphy <dmurphy@ti.com> wrote:
>> On 05/15/2018 04:56 PM, Andy Shevchenko wrote:
>>> On Tue, May 15, 2018 at 6:43 PM, Dan Murphy <dmurphy@ti.com> wrote:
>
>>>> + depends on LEDS_CLASS && I2C && OF
>>>
>>> What is OF specific in this driver?
>>
>> as3645a_led_class_setup has a "of" dependency
>
> So what? Is it called from this driver or?
>
Jacek
Do you have a preference about the use of "of" calls over the device_property calls?
I know Andy is pushing this for IoT and gave a good presentation on it last year on
common mistakes. Not sure this is a "common mistake" but more of a overall Linux compatibility issue
between x86 and ARM using two different configuration methods.
Since these lighting drivers are agnostic to the processor maybe we should adopt that philosophy.
I have no problem being the example driver on that.
>
>>>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = {
>>>> + { 40000, 0x00 },
>>>> + { 80000, 0x01 },
>>>> + { 120000, 0x02 },
>>>> + { 160000, 0x03 },
>>>> + { 200000, 0x04 },
>>>> + { 240000, 0x05 },
>>>> + { 280000, 0x06 },
>>>> + { 320000, 0x07 },
>>>> + { 360000, 0x08 },
>>>> + { 400000, 0x09 },
>>>> + { 600000, 0x0a },
>>>> + { 800000, 0x0b },
>>>> + { 1000000, 0x0c },
>>>> + { 1200000, 0x0d },
>>>> + { 1400000, 0x0e },
>>>> + { 1600000, 0x0f },
>>>
>>> Huh?!
>>
>> Please give comments that actually mean something other wise I will opt to ignore them.
>
> I did below.
Sort of. "So what?" and "Huh" are not really a technical comments. I can tell from your LVEE presentation that
you have a passion for making the kernel better. That is awesome but if we try to make the code perfect everytime no code will
ever get merged. And we are human and we will miss coding issues and make mistakes.
>
>>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
>>
>> Not sure what equation you are trying to point out here. But if you are trying to apply
>> a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout
>> step is not linear.
>
> Yeah, I know people are more than often too lazy to think.
Well this is why I put the table in. It is not laziness on my part as I try to add clarity for
our newbies or productization engineers, translation/lookup tables are not as bad as you think.
I am fully aware that this bloats the data section of the image and has negative impacts
on IoT small memory foot print devices.
>
> if (x < 9)
> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
> else
> strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC;
>
Not sure if this will produce the register value I am looking for. I have to run through the
algorithim. The idea is to take in the timeout and get the reg value to program.
If it yields the expected output, or with a modified equation, then I can pull it in.
>>>> + brightness_val = (brightness/2);
>>>
>>> Spaces.
>>
>> Not sure what this means checkpatch was clean
>
> Even besides missed whispaces it has redundant parens.
>
> checkpatch is not a silver bullet to get your code clean and nice.
>
True but it is the tool many maintainers use (sometimes) to ensure clean and nice.
I am still not see the extra spaces here but I do see the unneeded parens.
>>> This is return led_...();
>>
>> That is a preference. It does not have to be that way.
>
> What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit.
>
Again for non-IoT cases this would be fine but if we are striving to meet IoT size
requirements I do understand your concern.
>>>> + ret = of_property_read_string(led->led_node, "label", &name);
>>>
>>> device_property_...();
>>
>> It can be if the maintainer is requesting this.
>
> Jacek, if you need rationale behind this comment it's here: the driver
> has nothing DT specific and getting rid of OF centric programming
> allows to reuse the driver on non-DT platforms w/o touching a source
> code.
>
See below
>> Is the trend to move to these functions?
>
> See above.
See below
>
>> Most drivers use the "of" calls.
>
> So what?
>
I am deferring this to Jacek. Because this should be an overall coding change for all LED
drivers that are new. Like I pointed out I don't mind adding/changing the code as long as
all LED drivers will have that same mandate moving forward.
If the maintainers agree we can get this driver to be the example for the use of device_property calls.
>
>>>> + if (!ret)
>>>
>>> if (ret) sounds more natural. And better just to split
OK
>>>
>>>> + snprintf(led->led_name, sizeof(led->led_name),
>>>> + "%s:%s", led->led_node->name, name);
>>>> + else
>>>> + snprintf(led->led_name, sizeof(led->led_name),
>>>> + "%s:torch", led->led_node->name);
>>>
>>> const char *tmp;
>>>
>>> ret = device_property_read_...(&tmp);
>>> if (ret)
>>> tmp = ...
>>> sprintf(...);
>
> No comments on this?
No. This would depend on the disposition of the device_property calls.
If we go that way then yes this will probably change. But we need to have an
if..else as the label property is option in the DT documentation and we have to
make a label if one does not exist.
>
>>>> + led = devm_kzalloc(&client->dev,
>>>> + sizeof(struct lm3601x_led), GFP_KERNEL);
>>>
>>> sizeof(*led) and one line in the result
>
> And this?
Either one should yield the same allocation. So I can change it.
>
>
>>>> + { },
>>>
>>> Terminators better w/o comma.
>>
>> Looking at other drivers adding comma's on the sentinel is accepted. See the as3645a driver
>
> So what?
Not a compile issue here. I have been asked by maintainers/reviewers to add the comma on the sentinnel not just
struct definition but also for enums.
This is a maintainer decision I defer to them for guidance.
>
> Terminator at compile time even better.
>
>>>> + {},
>>>
>>> Ditto.
>>
>> See above
>
> See above.
>
See above
--
------------------
Dan Murphy
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox