* [PATCH v2 1/3] dt-bindings: panel: Add LG SW43408 MIPI-DSI panel
From: Dmitry Baryshkov @ 2024-03-30 15:00 UTC (permalink / raw)
To: Sumit Semwal, Caleb Connolly, Neil Armstrong, Jessica Zhang,
Sam Ravnborg, David Airlie, Daniel Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: dri-devel, devicetree, linux-kernel, linux-arm-msm, Vinod Koul,
Caleb Connolly
In-Reply-To: <20240330-lg-sw43408-panel-v2-0-293a58717b38@linaro.org>
From: Sumit Semwal <sumit.semwal@linaro.org>
LG SW43408 is 1080x2160, 4-lane MIPI-DSI panel present on Google Pixel 3
phones.
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
[caleb: convert to yaml]
Signed-off-by: Caleb Connolly <caleb@connolly.tech>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
.../bindings/display/panel/lg,sw43408.yaml | 62 ++++++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/panel/lg,sw43408.yaml b/Documentation/devicetree/bindings/display/panel/lg,sw43408.yaml
new file mode 100644
index 000000000000..1e08648f5bc7
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/lg,sw43408.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/lg,sw43408.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LG SW43408 1080x2160 DSI panel
+
+maintainers:
+ - Caleb Connolly <caleb.connolly@linaro.org>
+
+description:
+ This panel is used on the Pixel 3, it is a 60hz OLED panel which
+ required DSC (Display Stream Compression) and has rounded corners.
+
+allOf:
+ - $ref: panel-common.yaml#
+
+properties:
+ compatible:
+ items:
+ - const: lg,sw43408
+
+ reg: true
+ port: true
+ vddi-supply: true
+ vpnl-supply: true
+ reset-gpios: true
+
+required:
+ - compatible
+ - vddi-supply
+ - vpnl-supply
+ - reset-gpios
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ dsi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ panel@0 {
+ compatible = "lg,sw43408";
+ reg = <0>;
+
+ vddi-supply = <&vreg_l14a_1p88>;
+ vpnl-supply = <&vreg_l28a_3p0>;
+
+ reset-gpios = <&tlmm 6 GPIO_ACTIVE_LOW>;
+
+ port {
+ endpoint {
+ remote-endpoint = <&mdss_dsi0_out>;
+ };
+ };
+ };
+ };
+...
--
2.39.2
^ permalink raw reply related
* [PATCH v2 2/3] drm/mipi-dsi: add mipi_dsi_compression_mode_raw()
From: Dmitry Baryshkov @ 2024-03-30 15:00 UTC (permalink / raw)
To: Sumit Semwal, Caleb Connolly, Neil Armstrong, Jessica Zhang,
Sam Ravnborg, David Airlie, Daniel Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: dri-devel, devicetree, linux-kernel, linux-arm-msm
In-Reply-To: <20240330-lg-sw43408-panel-v2-0-293a58717b38@linaro.org>
The LG SW43408 panel requires sending non-standard data as a part of the
MIPI_DSI_COMPRESSION_MODE packet. Rather than hacking existing
mipi_dsi_compression_mode() add mipi_dsi_compression_mode_raw(), which
accepts raw data buffer and length.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/drm_mipi_dsi.c | 34 ++++++++++++++++++++++++++--------
include/drm/drm_mipi_dsi.h | 1 +
2 files changed, 27 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index ef6e416522f8..f340d1e0a9a5 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -645,29 +645,47 @@ int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
EXPORT_SYMBOL(mipi_dsi_set_maximum_return_packet_size);
/**
- * mipi_dsi_compression_mode() - enable/disable DSC on the peripheral
+ * mipi_dsi_compression_mode_raw() - control DSC on the peripheral
* @dsi: DSI peripheral device
- * @enable: Whether to enable or disable the DSC
+ * @data: data to be sent to the device
+ * @len: size of the data buffer
*
- * Enable or disable Display Stream Compression on the peripheral using the
+ * Control the Display Stream Compression on the peripheral using the
* default Picture Parameter Set and VESA DSC 1.1 algorithm.
*
* Return: 0 on success or a negative error code on failure.
*/
-ssize_t mipi_dsi_compression_mode(struct mipi_dsi_device *dsi, bool enable)
+ssize_t mipi_dsi_compression_mode_raw(struct mipi_dsi_device *dsi, void *data, size_t len)
{
- /* Note: Needs updating for non-default PPS or algorithm */
- u8 tx[2] = { enable << 0, 0 };
struct mipi_dsi_msg msg = {
.channel = dsi->channel,
.type = MIPI_DSI_COMPRESSION_MODE,
- .tx_len = sizeof(tx),
- .tx_buf = tx,
+ .tx_len = len,
+ .tx_buf = data,
};
int ret = mipi_dsi_device_transfer(dsi, &msg);
return (ret < 0) ? ret : 0;
}
+EXPORT_SYMBOL(mipi_dsi_compression_mode_raw);
+
+/**
+ * mipi_dsi_compression_mode() - enable/disable DSC on the peripheral
+ * @dsi: DSI peripheral device
+ * @enable: Whether to enable or disable the DSC
+ *
+ * Enable or disable Display Stream Compression on the peripheral using the
+ * default Picture Parameter Set and VESA DSC 1.1 algorithm.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+ssize_t mipi_dsi_compression_mode(struct mipi_dsi_device *dsi, bool enable)
+{
+ /* Note: Needs updating for non-default PPS or algorithm */
+ u8 tx[2] = { enable << 0, 0 };
+
+ return mipi_dsi_compression_mode_raw(dsi, tx, sizeof(tx));
+}
EXPORT_SYMBOL(mipi_dsi_compression_mode);
/**
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index c0aec0d4d664..321d2b019687 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -242,6 +242,7 @@ int mipi_dsi_turn_on_peripheral(struct mipi_dsi_device *dsi);
int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
u16 value);
ssize_t mipi_dsi_compression_mode(struct mipi_dsi_device *dsi, bool enable);
+ssize_t mipi_dsi_compression_mode_raw(struct mipi_dsi_device *dsi, void *data, size_t len);
ssize_t mipi_dsi_picture_parameter_set(struct mipi_dsi_device *dsi,
const struct drm_dsc_picture_parameter_set *pps);
--
2.39.2
^ permalink raw reply related
* [PATCH v2 3/3] drm: panel: Add LG sw43408 panel driver
From: Dmitry Baryshkov @ 2024-03-30 15:00 UTC (permalink / raw)
To: Sumit Semwal, Caleb Connolly, Neil Armstrong, Jessica Zhang,
Sam Ravnborg, David Airlie, Daniel Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: dri-devel, devicetree, linux-kernel, linux-arm-msm, Vinod Koul,
Caleb Connolly
In-Reply-To: <20240330-lg-sw43408-panel-v2-0-293a58717b38@linaro.org>
From: Sumit Semwal <sumit.semwal@linaro.org>
LG SW43408 is 1080x2160, 4-lane MIPI-DSI panel, used in some Pixel3
phones.
Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
[vinod: Add DSC support]
Signed-off-by: Vinod Koul <vkoul@kernel.org>
[caleb: cleanup and support turning off the panel]
Signed-off-by: Caleb Connolly <caleb@connolly.tech>
[DB: partially rewrote the driver and fixed DSC programming]
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
MAINTAINERS | 8 +
drivers/gpu/drm/panel/Kconfig | 11 ++
drivers/gpu/drm/panel/Makefile | 1 +
drivers/gpu/drm/panel/panel-lg-sw43408.c | 321 +++++++++++++++++++++++++++++++
4 files changed, 341 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 4b511a55101c..f4cf7ee97376 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6755,6 +6755,14 @@ S: Maintained
F: Documentation/devicetree/bindings/display/panel/jadard,jd9365da-h3.yaml
F: drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c
+DRM DRIVER FOR LG SW43408 PANELS
+M: Sumit Semwal <sumit.semwal@linaro.org>
+M: Caleb Connolly <caleb.connolly@linaro.org>
+S: Maintained
+T: git git://anongit.freedesktop.org/drm/drm-misc
+F: Documentation/devicetree/bindings/display/panel/lg,sw43408.yaml
+F: drivers/gpu/drm/panel/panel-lg-sw43408.c
+
DRM DRIVER FOR LOGICVC DISPLAY CONTROLLER
M: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
S: Supported
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index d037b3b8b999..f94c702735cb 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -335,6 +335,17 @@ config DRM_PANEL_LG_LG4573
Say Y here if you want to enable support for LG4573 RGB panel.
To compile this driver as a module, choose M here.
+config DRM_PANEL_LG_SW43408
+ tristate "LG SW43408 panel"
+ depends on OF
+ depends on DRM_MIPI_DSI
+ depends on BACKLIGHT_CLASS_DEVICE
+ help
+ Say Y here if you want to enable support for LG sw43408 panel.
+ The panel has a 1080x2160 resolution and uses
+ 24 bit RGB per pixel. It provides a MIPI DSI interface to
+ the host and has a built-in LED backlight.
+
config DRM_PANEL_MAGNACHIP_D53E6EA8966
tristate "Magnachip D53E6EA8966 DSI panel"
depends on OF && SPI
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index f156d7fa0bcc..a75687d13caf 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK050H3146W) += panel-leadtek-ltk050h3146w.o
obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o
obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o
obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
+obj-$(CONFIG_DRM_PANEL_LG_SW43408) += panel-lg-sw43408.o
obj-$(CONFIG_DRM_PANEL_MAGNACHIP_D53E6EA8966) += panel-magnachip-d53e6ea8966.o
obj-$(CONFIG_DRM_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o
obj-$(CONFIG_DRM_PANEL_NEWVISION_NV3051D) += panel-newvision-nv3051d.o
diff --git a/drivers/gpu/drm/panel/panel-lg-sw43408.c b/drivers/gpu/drm/panel/panel-lg-sw43408.c
new file mode 100644
index 000000000000..6c244b9642f1
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-lg-sw43408.c
@@ -0,0 +1,321 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019-2024 Linaro Ltd
+ * Author: Sumit Semwal <sumit.semwal@linaro.org>
+ * Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+
+#include <video/mipi_display.h>
+
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/display/drm_dsc.h>
+#include <drm/display/drm_dsc_helper.h>
+
+#define NUM_SUPPLIES 2
+
+struct sw43408_panel {
+ struct drm_panel base;
+ struct mipi_dsi_device *link;
+
+ const struct drm_display_mode *mode;
+
+ struct regulator_bulk_data supplies[NUM_SUPPLIES];
+
+ struct gpio_desc *reset_gpio;
+
+ struct drm_dsc_config dsc;
+};
+
+static inline struct sw43408_panel *to_panel_info(struct drm_panel *panel)
+{
+ return container_of(panel, struct sw43408_panel, base);
+}
+
+static int sw43408_unprepare(struct drm_panel *panel)
+{
+ struct sw43408_panel *ctx = to_panel_info(panel);
+ int ret;
+
+ ret = mipi_dsi_dcs_set_display_off(ctx->link);
+ if (ret < 0)
+ dev_err(panel->dev, "set_display_off cmd failed ret = %d\n", ret);
+
+ ret = mipi_dsi_dcs_enter_sleep_mode(ctx->link);
+ if (ret < 0)
+ dev_err(panel->dev, "enter_sleep cmd failed ret = %d\n", ret);
+
+ msleep(100);
+
+ gpiod_set_value(ctx->reset_gpio, 1);
+
+ return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+}
+
+static int sw43408_program(struct drm_panel *panel)
+{
+ struct sw43408_panel *ctx = to_panel_info(panel);
+ struct drm_dsc_picture_parameter_set pps;
+ /* Note; this uses a non-standard argument format */
+ u8 dsc_en[] = { 0x11 };
+
+ mipi_dsi_dcs_write_seq(ctx->link, MIPI_DCS_SET_GAMMA_CURVE, 0x02);
+
+ mipi_dsi_dcs_set_tear_on(ctx->link, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
+
+ mipi_dsi_dcs_write_seq(ctx->link, 0x53, 0x0c, 0x30);
+ mipi_dsi_dcs_write_seq(ctx->link, 0x55, 0x00, 0x70, 0xdf, 0x00, 0x70, 0xdf);
+ mipi_dsi_dcs_write_seq(ctx->link, 0xf7, 0x01, 0x49, 0x0c);
+
+ mipi_dsi_dcs_exit_sleep_mode(ctx->link);
+
+ msleep(135);
+
+ mipi_dsi_compression_mode_raw(ctx->link, dsc_en, sizeof(dsc_en));
+
+ mipi_dsi_dcs_write_seq(ctx->link, 0xb0, 0xac);
+ mipi_dsi_dcs_write_seq(ctx->link, 0xe5,
+ 0x00, 0x3a, 0x00, 0x3a, 0x00, 0x0e, 0x10);
+ mipi_dsi_dcs_write_seq(ctx->link, 0xb5,
+ 0x75, 0x60, 0x2d, 0x5d, 0x80, 0x00, 0x0a, 0x0b,
+ 0x00, 0x05, 0x0b, 0x00, 0x80, 0x0d, 0x0e, 0x40,
+ 0x00, 0x0c, 0x00, 0x16, 0x00, 0xb8, 0x00, 0x80,
+ 0x0d, 0x0e, 0x40, 0x00, 0x0c, 0x00, 0x16, 0x00,
+ 0xb8, 0x00, 0x81, 0x00, 0x03, 0x03, 0x03, 0x01,
+ 0x01);
+ msleep(85);
+ mipi_dsi_dcs_write_seq(ctx->link, 0xcd,
+ 0x00, 0x00, 0x00, 0x19, 0x19, 0x19, 0x19, 0x19,
+ 0x19, 0x19, 0x19, 0x19, 0x19, 0x19, 0x19, 0x19,
+ 0x16, 0x16);
+ mipi_dsi_dcs_write_seq(ctx->link, 0xcb, 0x80, 0x5c, 0x07, 0x03, 0x28);
+ mipi_dsi_dcs_write_seq(ctx->link, 0xc0, 0x02, 0x02, 0x0f);
+ mipi_dsi_dcs_write_seq(ctx->link, 0x55, 0x04, 0x61, 0xdb, 0x04, 0x70, 0xdb);
+ mipi_dsi_dcs_write_seq(ctx->link, 0xb0, 0xca);
+
+ mipi_dsi_dcs_set_display_on(ctx->link);
+
+ msleep(50);
+
+ ctx->link->mode_flags &= ~MIPI_DSI_MODE_LPM;
+
+ drm_dsc_pps_payload_pack(&pps, ctx->link->dsc);
+ mipi_dsi_picture_parameter_set(ctx->link, &pps);
+
+ ctx->link->mode_flags |= MIPI_DSI_MODE_LPM;
+
+ return 0;
+}
+
+static int sw43408_prepare(struct drm_panel *panel)
+{
+ struct sw43408_panel *ctx = to_panel_info(panel);
+ int ret;
+
+ ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+ if (ret < 0)
+ return ret;
+
+ usleep_range(5000, 6000);
+
+ gpiod_set_value(ctx->reset_gpio, 0);
+ usleep_range(9000, 10000);
+ gpiod_set_value(ctx->reset_gpio, 1);
+ usleep_range(1000, 2000);
+ gpiod_set_value(ctx->reset_gpio, 0);
+ usleep_range(9000, 10000);
+
+ ret = sw43408_program(panel);
+ if (ret)
+ goto poweroff;
+
+ return 0;
+
+poweroff:
+ gpiod_set_value(ctx->reset_gpio, 1);
+ regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+ return ret;
+}
+
+static int sw43408_get_modes(struct drm_panel *panel,
+ struct drm_connector *connector)
+{
+ struct sw43408_panel *ctx = to_panel_info(panel);
+
+ return drm_connector_helper_get_modes_fixed(connector, ctx->mode);
+}
+
+static int sw43408_backlight_update_status(struct backlight_device *bl)
+{
+ struct mipi_dsi_device *dsi = bl_get_data(bl);
+ uint16_t brightness = backlight_get_brightness(bl);
+
+ return mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);
+}
+
+const struct backlight_ops sw43408_backlight_ops = {
+ .update_status = sw43408_backlight_update_status,
+};
+
+static int sw43408_backlight_init(struct sw43408_panel *ctx)
+{
+ struct device *dev = &ctx->link->dev;
+ const struct backlight_properties props = {
+ .type = BACKLIGHT_PLATFORM,
+ .brightness = 255,
+ .max_brightness = 255,
+ };
+
+ ctx->base.backlight = devm_backlight_device_register(dev, dev_name(dev), dev,
+ ctx->link,
+ &sw43408_backlight_ops,
+ &props);
+
+ if (IS_ERR(ctx->base.backlight))
+ return dev_err_probe(dev, PTR_ERR(ctx->base.backlight),
+ "Failed to create backlight\n");
+
+ return 0;
+}
+
+static const struct drm_panel_funcs sw43408_funcs = {
+ .unprepare = sw43408_unprepare,
+ .prepare = sw43408_prepare,
+ .get_modes = sw43408_get_modes,
+};
+
+static const struct drm_display_mode sw43408_default_mode = {
+ .clock = 152340,
+
+ .hdisplay = 1080,
+ .hsync_start = 1080 + 20,
+ .hsync_end = 1080 + 20 + 32,
+ .htotal = 1080 + 20 + 32 + 20,
+
+ .vdisplay = 2160,
+ .vsync_start = 2160 + 20,
+ .vsync_end = 2160 + 20 + 4,
+ .vtotal = 2160 + 20 + 4 + 20,
+
+ .width_mm = 62,
+ .height_mm = 124,
+
+ .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
+};
+
+static const struct of_device_id sw43408_of_match[] = {
+ { .compatible = "lg,sw43408", .data = &sw43408_default_mode },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sw43408_of_match);
+
+static int sw43408_add(struct sw43408_panel *ctx)
+{
+ struct device *dev = &ctx->link->dev;
+ int ret;
+
+ ctx->supplies[0].supply = "vddi"; /* 1.88 V */
+ ctx->supplies[0].init_load_uA = 62000;
+ ctx->supplies[1].supply = "vpnl"; /* 3.0 V */
+ ctx->supplies[1].init_load_uA = 857000;
+
+ ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ctx->supplies),
+ ctx->supplies);
+ if (ret < 0)
+ return ret;
+
+ ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(ctx->reset_gpio)) {
+ dev_err(dev, "cannot get reset gpio %ld\n",
+ PTR_ERR(ctx->reset_gpio));
+ return PTR_ERR(ctx->reset_gpio);
+ }
+
+ ret = sw43408_backlight_init(ctx);
+ if (ret < 0)
+ return ret;
+
+ ctx->base.prepare_prev_first = true;
+
+ drm_panel_init(&ctx->base, dev, &sw43408_funcs, DRM_MODE_CONNECTOR_DSI);
+
+ drm_panel_add(&ctx->base);
+ return ret;
+}
+
+static int sw43408_probe(struct mipi_dsi_device *dsi)
+{
+ struct sw43408_panel *ctx;
+ int ret;
+
+ ctx = devm_kzalloc(&dsi->dev, sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ ctx->mode = of_device_get_match_data(&dsi->dev);
+ dsi->mode_flags = MIPI_DSI_MODE_LPM;
+ dsi->format = MIPI_DSI_FMT_RGB888;
+ dsi->lanes = 4;
+
+ ctx->link = dsi;
+ mipi_dsi_set_drvdata(dsi, ctx);
+
+ ret = sw43408_add(ctx);
+ if (ret < 0)
+ return ret;
+
+ /* The panel is DSC panel only, set the dsc params */
+ ctx->dsc.dsc_version_major = 0x1;
+ ctx->dsc.dsc_version_minor = 0x1;
+
+ /* slice_count * slice_width == width */
+ ctx->dsc.slice_height = 16;
+ ctx->dsc.slice_width = 540;
+ ctx->dsc.slice_count = 2;
+ ctx->dsc.bits_per_component = 8;
+ ctx->dsc.bits_per_pixel = 8 << 4;
+ ctx->dsc.block_pred_enable = true;
+
+ dsi->dsc = &ctx->dsc;
+
+ return mipi_dsi_attach(dsi);
+}
+
+static void sw43408_remove(struct mipi_dsi_device *dsi)
+{
+ struct sw43408_panel *ctx = mipi_dsi_get_drvdata(dsi);
+ int ret;
+
+ ret = sw43408_unprepare(&ctx->base);
+ if (ret < 0)
+ dev_err(&dsi->dev, "failed to unprepare panel: %d\n",
+ ret);
+
+ ret = mipi_dsi_detach(dsi);
+ if (ret < 0)
+ dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
+
+ drm_panel_remove(&ctx->base);
+}
+
+static struct mipi_dsi_driver sw43408_driver = {
+ .driver = {
+ .name = "panel-lg-sw43408",
+ .of_match_table = sw43408_of_match,
+ },
+ .probe = sw43408_probe,
+ .remove = sw43408_remove,
+};
+module_mipi_dsi_driver(sw43408_driver);
+
+MODULE_AUTHOR("Sumit Semwal <sumit.semwal@linaro.org>");
+MODULE_DESCRIPTION("LG SW436408 MIPI-DSI LED panel");
+MODULE_LICENSE("GPL");
--
2.39.2
^ permalink raw reply related
* Re: [PATCH 0/2] arm64: dts: fsl-lx2162a-som: add description for rtc
From: Shawn Guo @ 2024-03-30 15:07 UTC (permalink / raw)
To: Josua Mayer
Cc: Shawn Guo, Li Yang, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-arm-kernel, devicetree, linux-kernel
In-Reply-To: <20240312-lx2162-rtc-v1-0-1f4cd431b1cf@solid-run.com>
On Tue, Mar 12, 2024 at 08:56:53PM +0100, Josua Mayer wrote:
> Josua Mayer (2):
> arm64: dts: fsl-lx2162a-som: add description for rtc
> arm64: dts: fsl-lx2162a-clearfog: add alias for i2c bus iic6
Applied both, thanks!
^ permalink raw reply
* [PATCH v5 1/2] dt-bindings: backlight: Add Texas Instruments LM3509
From: Patrick Gansterer @ 2024-03-30 14:59 UTC (permalink / raw)
To: dri-devel, linux-leds, devicetree, linux-kernel, linux-fbdev
Cc: Patrick Gansterer, Lee Jones, Daniel Thompson, Jingoo Han,
Pavel Machek, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Helge Deller, Sam Ravnborg, Krzysztof Kozlowski
In-Reply-To: <20240330145931.729116-1-paroga@paroga.com>
Add Device Tree bindings for Texas Instruments LM3509 - a
High Efficiency Boost for White LED's and/or OLED Displays
Signed-off-by: Patrick Gansterer <paroga@paroga.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
---
.../bindings/leds/backlight/ti,lm3509.yaml | 139 ++++++++++++++++++
1 file changed, 139 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/backlight/ti,lm3509.yaml
diff --git a/Documentation/devicetree/bindings/leds/backlight/ti,lm3509.yaml b/Documentation/devicetree/bindings/leds/backlight/ti,lm3509.yaml
new file mode 100644
index 000000000000..b67f67648852
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/ti,lm3509.yaml
@@ -0,0 +1,139 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/backlight/ti,lm3509.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI LM3509 High Efficiency Boost for White LED's and/or OLED Displays
+
+maintainers:
+ - Patrick Gansterer <paroga@paroga.com>
+
+description:
+ The LM3509 current mode boost converter offers two separate outputs.
+ https://www.ti.com/product/LM3509
+
+properties:
+ compatible:
+ const: ti,lm3509
+
+ reg:
+ maxItems: 1
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+ reset-gpios:
+ maxItems: 1
+
+ ti,brightness-rate-of-change-us:
+ description: Brightness Rate of Change in microseconds.
+ enum: [51, 13000, 26000, 52000]
+
+ ti,oled-mode:
+ description: Enable OLED mode.
+ type: boolean
+
+patternProperties:
+ "^led@[01]$":
+ type: object
+ description: Properties for a string of connected LEDs.
+
+ allOf:
+ - $ref: common.yaml#
+
+ properties:
+ reg:
+ description:
+ The control register that is used to program the two current sinks.
+ The LM3509 has two registers (BMAIN and BSUB) and are represented
+ as 0 or 1 in this property. The two current sinks can be controlled
+ independently with both registers, or register BMAIN can be
+ configured to control both sinks with the led-sources property.
+ minimum: 0
+ maximum: 1
+
+ label: true
+
+ led-sources:
+ allOf:
+ - minItems: 1
+ maxItems: 2
+ items:
+ minimum: 0
+ maximum: 1
+
+ default-brightness:
+ minimum: 0
+ maximum: 31
+ default: 18
+
+ max-brightness:
+ minimum: 0
+ maximum: 31
+ default: 31
+
+ required:
+ - reg
+
+ additionalProperties: false
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ backlight@36 {
+ compatible = "ti,lm3509";
+ reg = <0x36>;
+ reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
+
+ ti,oled-mode;
+ ti,brightness-rate-of-change-us = <52000>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ led-sources = <0 1>;
+ label = "lcd-backlight";
+ default-brightness = <12>;
+ max-brightness = <31>;
+ };
+ };
+ };
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ backlight@36 {
+ compatible = "ti,lm3509";
+ reg = <0x36>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ default-brightness = <12>;
+ };
+
+ led@1 {
+ reg = <1>;
+ default-brightness = <15>;
+ };
+ };
+ };
--
2.44.0
^ permalink raw reply related
* Re: [PATCH v5 1/7] iio: accel: adxl345: Make data_range obsolete
From: Jonathan Cameron @ 2024-03-30 15:18 UTC (permalink / raw)
To: Lothar Rubusch
Cc: lars, Michael.Hennerich, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-iio, devicetree, linux-kernel, eraretuya
In-Reply-To: <CAFXKEHaPcbRMVJTWW0Atg37jqb97JBdPoSmm3gAZEO1Q=SZm9w@mail.gmail.com>
On Fri, 29 Mar 2024 01:03:29 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> On Thu, Mar 28, 2024 at 2:37 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Wed, 27 Mar 2024 22:03:14 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > > Replace write() data_format by regmap_update_bits(), because bus specific
> > > pre-configuration may have happened before on the same register. For
> > > further updates to the data_format register then bus pre-configuration
> > > needs to be masked out.
> > >
> > > Remove the data_range field from the struct adxl345_data, because it is
> > > not used anymore.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > > drivers/iio/accel/adxl345_core.c | 18 ++++++++++++------
> > > 1 file changed, 12 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index 8bd30a23e..35df5e372 100644
> > > --- a/drivers/iio/accel/adxl345_core.c
> > > +++ b/drivers/iio/accel/adxl345_core.c
> > > @@ -37,7 +37,15 @@
> > > #define ADXL345_POWER_CTL_MEASURE BIT(3)
> > > #define ADXL345_POWER_CTL_STANDBY 0x00
> > >
> > > +#define ADXL345_DATA_FORMAT_RANGE GENMASK(1, 0) /* Set the g range */
> > > +#define ADXL345_DATA_FORMAT_JUSTIFY BIT(2) /* Left-justified (MSB) mode */
> > > #define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> > > +#define ADXL345_DATA_FORMAT_SELF_TEST BIT(7) /* Enable a self test */
> > > +#define ADXL345_DATA_FORMAT_MSK (ADXL345_DATA_FORMAT_RANGE | \
> > > + ADXL345_DATA_FORMAT_JUSTIFY | \
> > > + ADXL345_DATA_FORMAT_FULL_RES | \
> > > + ADXL345_DATA_FORMAT_SELF_TEST)
> > This needs renaming. It's not a mask of everything in the register, or
> > even just of everything related to format.
> >
> > Actually I'd just not have this definition. Use the build up value
> > from all the submasks at the call site. Then we are just making it clear
> > only a subset of fields are being cleared.
> >
> I understand this solution was not very useful. I'm not sure, I
> understood you correctly. Please have a look into v6 if this matches
> your comment.
> Now, I remove the mask, instead I use a local variable in core's
> probe() for the update mask. I added a comment. Nevertheless, I keep
> the used flags for FORMAT_DATA. Does this go into the direction of
> using the build up value from the submasks at the call site?
>
The new code looks good to me. A local variable doesn't carry the
same implication of global applicability as the define did.
Thanks,
J
> > Jonathan
> >
> > > +
> > > #define ADXL345_DATA_FORMAT_2G 0
> > > #define ADXL345_DATA_FORMAT_4G 1
> > > #define ADXL345_DATA_FORMAT_8G 2
> > > @@ -48,7 +56,6 @@
> > > struct adxl345_data {
> > > const struct adxl345_chip_info *info;
> > > struct regmap *regmap;
> > > - u8 data_range;
> > > };
> > >
> > > #define ADXL345_CHANNEL(index, axis) { \
> > > @@ -218,15 +225,14 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
> > >
> > > data = iio_priv(indio_dev);
> > > data->regmap = regmap;
> > > - /* Enable full-resolution mode */
> > > - data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
> > > data->info = device_get_match_data(dev);
> > > if (!data->info)
> > > return -ENODEV;
> > >
> > > - ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
> > > - data->data_range);
> > > - if (ret < 0)
> > > + /* Enable full-resolution mode */
> > > + ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
> > > + ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
> > > + if (ret)
> > > return dev_err_probe(dev, ret, "Failed to set data range\n");
> > >
> > > indio_dev->name = data->info->name;
> >
^ permalink raw reply
* [PATCH v5 2/2] backlight: Add new lm3509 backlight driver
From: Patrick Gansterer @ 2024-03-30 14:59 UTC (permalink / raw)
To: dri-devel, linux-leds, devicetree, linux-kernel, linux-fbdev
Cc: Patrick Gansterer, Lee Jones, Daniel Thompson, Jingoo Han,
Pavel Machek, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Helge Deller, Sam Ravnborg
In-Reply-To: <20240330145931.729116-1-paroga@paroga.com>
This is a general driver for LM3509 backlight chip of TI.
LM3509 is High Efficiency Boost for White LEDs and/or OLED Displays with
Dual Current Sinks. This driver supports OLED/White LED select, brightness
control and sub/main control.
The datasheet can be found at http://www.ti.com/product/lm3509.
Signed-off-by: Patrick Gansterer <paroga@paroga.com>
---
drivers/video/backlight/Kconfig | 7 +
drivers/video/backlight/Makefile | 1 +
drivers/video/backlight/lm3509_bl.c | 340 ++++++++++++++++++++++++++++
3 files changed, 348 insertions(+)
create mode 100644 drivers/video/backlight/lm3509_bl.c
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 230bca07b09d..3614a5d29c71 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -373,6 +373,13 @@ config BACKLIGHT_AAT2870
If you have a AnalogicTech AAT2870 say Y to enable the
backlight driver.
+config BACKLIGHT_LM3509
+ tristate "Backlight Driver for LM3509"
+ depends on I2C
+ select REGMAP_I2C
+ help
+ This supports TI LM3509 Backlight Driver
+
config BACKLIGHT_LM3630A
tristate "Backlight Driver for LM3630A"
depends on I2C && PWM
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index 8d2cb252042d..8fc98f760a8a 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_BACKLIGHT_IPAQ_MICRO) += ipaq_micro_bl.o
obj-$(CONFIG_BACKLIGHT_KTD253) += ktd253-backlight.o
obj-$(CONFIG_BACKLIGHT_KTD2801) += ktd2801-backlight.o
obj-$(CONFIG_BACKLIGHT_KTZ8866) += ktz8866.o
+obj-$(CONFIG_BACKLIGHT_LM3509) += lm3509_bl.o
obj-$(CONFIG_BACKLIGHT_LM3533) += lm3533_bl.o
obj-$(CONFIG_BACKLIGHT_LM3630A) += lm3630a_bl.o
obj-$(CONFIG_BACKLIGHT_LM3639) += lm3639_bl.o
diff --git a/drivers/video/backlight/lm3509_bl.c b/drivers/video/backlight/lm3509_bl.c
new file mode 100644
index 000000000000..ab57f79ffe23
--- /dev/null
+++ b/drivers/video/backlight/lm3509_bl.c
@@ -0,0 +1,340 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#define LM3509_NAME "lm3509_bl"
+
+#define LM3509_SINK_MAIN 0
+#define LM3509_SINK_SUB 1
+#define LM3509_NUM_SINKS 2
+
+#define LM3509_DEF_BRIGHTNESS 0x12
+#define LM3509_MAX_BRIGHTNESS 0x1F
+
+#define REG_GP 0x10
+#define REG_BMAIN 0xA0
+#define REG_BSUB 0xB0
+#define REG_MAX 0xFF
+
+enum {
+ REG_GP_ENM_BIT = 0,
+ REG_GP_ENS_BIT,
+ REG_GP_UNI_BIT,
+ REG_GP_RMP0_BIT,
+ REG_GP_RMP1_BIT,
+ REG_GP_OLED_BIT,
+};
+
+struct lm3509_bl {
+ struct regmap *regmap;
+ struct backlight_device *bl_main;
+ struct backlight_device *bl_sub;
+ struct gpio_desc *reset_gpio;
+};
+
+struct lm3509_bl_led_data {
+ const char *label;
+ int led_sources;
+ u32 brightness;
+ u32 max_brightness;
+};
+
+static void lm3509_reset(struct lm3509_bl *data)
+{
+ if (data->reset_gpio) {
+ gpiod_set_value(data->reset_gpio, 1);
+ udelay(1);
+ gpiod_set_value(data->reset_gpio, 0);
+ udelay(10);
+ }
+}
+
+static int lm3509_update_status(struct backlight_device *bl,
+ unsigned int en_mask, unsigned int br_reg)
+{
+ struct lm3509_bl *data = bl_get_data(bl);
+ int ret;
+ bool en;
+
+ ret = regmap_write(data->regmap, br_reg, backlight_get_brightness(bl));
+ if (ret < 0)
+ return ret;
+
+ en = !backlight_is_blank(bl);
+ return regmap_update_bits(data->regmap, REG_GP, en_mask,
+ en ? en_mask : 0);
+}
+
+static int lm3509_main_update_status(struct backlight_device *bl)
+{
+ return lm3509_update_status(bl, BIT(REG_GP_ENM_BIT), REG_BMAIN);
+}
+
+static const struct backlight_ops lm3509_main_ops = {
+ .options = BL_CORE_SUSPENDRESUME,
+ .update_status = lm3509_main_update_status,
+};
+
+static int lm3509_sub_update_status(struct backlight_device *bl)
+{
+ return lm3509_update_status(bl, BIT(REG_GP_ENS_BIT), REG_BSUB);
+}
+
+static const struct backlight_ops lm3509_sub_ops = {
+ .options = BL_CORE_SUSPENDRESUME,
+ .update_status = lm3509_sub_update_status,
+};
+
+static struct backlight_device *
+lm3509_backlight_register(struct device *dev, const char *name_suffix,
+ struct lm3509_bl *data,
+ const struct backlight_ops *ops,
+ const struct lm3509_bl_led_data *led_data)
+
+{
+ struct backlight_device *bd;
+ struct backlight_properties props;
+ const char *label = led_data->label;
+ char name[64];
+
+ memset(&props, 0, sizeof(props));
+ props.type = BACKLIGHT_RAW;
+ props.brightness = led_data->brightness;
+ props.max_brightness = led_data->max_brightness;
+ props.scale = BACKLIGHT_SCALE_NON_LINEAR;
+
+ if (!label) {
+ snprintf(name, sizeof(name), "lm3509-%s-%s", dev_name(dev),
+ name_suffix);
+ label = name;
+ }
+
+ bd = devm_backlight_device_register(dev, label, dev, data, ops, &props);
+ if (bd)
+ backlight_update_status(bd);
+
+ return bd;
+}
+
+static const struct regmap_config lm3509_regmap = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = REG_MAX,
+};
+
+static int lm3509_parse_led_sources(struct device_node *node,
+ int default_led_sources)
+{
+ u32 sources[LM3509_NUM_SINKS];
+ int ret, num_sources, i;
+
+ num_sources = of_property_count_u32_elems(node, "led-sources");
+ if (num_sources < 0)
+ return default_led_sources;
+ else if (num_sources > ARRAY_SIZE(sources))
+ return -EINVAL;
+
+ ret = of_property_read_u32_array(node, "led-sources", sources,
+ num_sources);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < num_sources; i++) {
+ if (sources[i] >= LM3509_NUM_SINKS)
+ return -EINVAL;
+
+ ret |= BIT(sources[i]);
+ }
+
+ return ret;
+}
+
+static int lm3509_parse_dt_node(struct device *dev,
+ struct lm3509_bl_led_data *led_data)
+{
+ struct device_node *child;
+ int seen_led_sources = 0;
+
+ for_each_child_of_node(dev->of_node, child) {
+ struct lm3509_bl_led_data *ld;
+ int ret;
+ u32 reg;
+ int valid_led_sources;
+
+ ret = of_property_read_u32(child, "reg", ®);
+ if (ret < 0)
+ return ret;
+ if (reg >= LM3509_NUM_SINKS)
+ return -EINVAL;
+ ld = &led_data[reg];
+
+ ld->led_sources = lm3509_parse_led_sources(child, BIT(reg));
+ if (ld->led_sources < 0)
+ return ld->led_sources;
+
+ if (reg == 0)
+ valid_led_sources = BIT(LM3509_SINK_MAIN) |
+ BIT(LM3509_SINK_SUB);
+ else
+ valid_led_sources = BIT(LM3509_SINK_SUB);
+
+ if (ld->led_sources != (ld->led_sources & valid_led_sources))
+ return -EINVAL;
+
+ if (seen_led_sources & ld->led_sources)
+ return -EINVAL;
+
+ seen_led_sources |= ld->led_sources;
+
+ ld->label = NULL;
+ of_property_read_string(child, "label", &ld->label);
+
+ ld->max_brightness = LM3509_MAX_BRIGHTNESS;
+ of_property_read_u32(child, "max-brightness",
+ &ld->max_brightness);
+ ld->max_brightness =
+ min_t(u32, ld->max_brightness, LM3509_MAX_BRIGHTNESS);
+
+ ld->brightness = LM3509_DEF_BRIGHTNESS;
+ of_property_read_u32(child, "default-brightness",
+ &ld->brightness);
+ ld->brightness = min_t(u32, ld->brightness, ld->max_brightness);
+ }
+
+ return 0;
+}
+
+static int lm3509_probe(struct i2c_client *client)
+{
+ struct lm3509_bl *data;
+ struct device *dev = &client->dev;
+ int ret;
+ bool oled_mode = false;
+ unsigned int reg_gp_val = 0;
+ struct lm3509_bl_led_data led_data[LM3509_NUM_SINKS];
+ u32 rate_of_change = 0;
+
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+ dev_err(dev, "i2c functionality check failed\n");
+ return -EOPNOTSUPP;
+ }
+
+ data = devm_kzalloc(dev, sizeof(struct lm3509_bl), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->regmap = devm_regmap_init_i2c(client, &lm3509_regmap);
+ if (IS_ERR(data->regmap))
+ return PTR_ERR(data->regmap);
+ i2c_set_clientdata(client, data);
+
+ data->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(data->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(data->reset_gpio),
+ "Failed to get 'reset' gpio\n");
+
+ lm3509_reset(data);
+
+ memset(led_data, 0, sizeof(led_data));
+ ret = lm3509_parse_dt_node(dev, led_data);
+ if (ret)
+ return ret;
+
+ oled_mode = of_property_read_bool(dev->of_node, "ti,oled-mode");
+
+ if (!of_property_read_u32(dev->of_node,
+ "ti,brightness-rate-of-change-us",
+ &rate_of_change)) {
+ switch (rate_of_change) {
+ case 51:
+ reg_gp_val = 0;
+ break;
+ case 13000:
+ reg_gp_val = BIT(REG_GP_RMP1_BIT);
+ break;
+ case 26000:
+ reg_gp_val = BIT(REG_GP_RMP0_BIT);
+ break;
+ case 52000:
+ reg_gp_val = BIT(REG_GP_RMP0_BIT) |
+ BIT(REG_GP_RMP1_BIT);
+ break;
+ default:
+ dev_warn(dev, "invalid rate of change %u\n",
+ rate_of_change);
+ break;
+ }
+ }
+
+ if (led_data[0].led_sources ==
+ (BIT(LM3509_SINK_MAIN) | BIT(LM3509_SINK_SUB)))
+ reg_gp_val |= BIT(REG_GP_UNI_BIT);
+ if (oled_mode)
+ reg_gp_val |= BIT(REG_GP_OLED_BIT);
+
+ ret = regmap_write(data->regmap, REG_GP, reg_gp_val);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "failed to write register\n");
+
+ if (led_data[0].led_sources) {
+ data->bl_main = lm3509_backlight_register(
+ dev, "main", data, &lm3509_main_ops, &led_data[0]);
+ if (IS_ERR(data->bl_main)) {
+ return dev_err_probe(
+ dev, PTR_ERR(data->bl_main),
+ "failed to register main backlight\n");
+ }
+ }
+
+ if (led_data[1].led_sources) {
+ data->bl_sub = lm3509_backlight_register(
+ dev, "sub", data, &lm3509_sub_ops, &led_data[1]);
+ if (IS_ERR(data->bl_sub)) {
+ return dev_err_probe(
+ dev, PTR_ERR(data->bl_sub),
+ "failed to register secondary backlight\n");
+ }
+ }
+
+ return 0;
+}
+
+static void lm3509_remove(struct i2c_client *client)
+{
+ struct lm3509_bl *data = i2c_get_clientdata(client);
+
+ regmap_write(data->regmap, REG_GP, 0x00);
+}
+
+static const struct i2c_device_id lm3509_id[] = { { LM3509_NAME, 0 }, {} };
+
+MODULE_DEVICE_TABLE(i2c, lm3509_id);
+
+static const struct of_device_id lm3509_match_table[] = {
+ {
+ .compatible = "ti,lm3509",
+ },
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, lm3509_match_table);
+
+static struct i2c_driver lm3509_i2c_driver = {
+ .driver = {
+ .name = LM3509_NAME,
+ .of_match_table = lm3509_match_table,
+ },
+ .probe = lm3509_probe,
+ .remove = lm3509_remove,
+ .id_table = lm3509_id,
+};
+
+module_i2c_driver(lm3509_i2c_driver);
+
+MODULE_DESCRIPTION("Texas Instruments Backlight driver for LM3509");
+MODULE_AUTHOR("Patrick Gansterer <paroga@paroga.com>");
+MODULE_LICENSE("GPL");
--
2.44.0
^ permalink raw reply related
* [PATCH v5 0/2] backlight: Add new lm3509 backlight driver
From: Patrick Gansterer @ 2024-03-30 14:59 UTC (permalink / raw)
To: dri-devel, linux-leds, devicetree, linux-kernel, linux-fbdev
Cc: Patrick Gansterer, Lee Jones, Daniel Thompson, Jingoo Han,
Pavel Machek, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Helge Deller, Sam Ravnborg
This is a general driver for LM3509 backlight chip of TI.
LM3509 is High Efficiency Boost for White LEDs and/or OLED Displays with
Dual Current Sinks. This driver supports OLED/White LED select, brightness
control and sub/main control.
The datasheet can be found at http://www.ti.com/product/lm3509.
---
Changes in v5:
Renamed lm3509_bl_led_pdata to lm3509_bl_led_data
Set backlight_properties.scale to BACKLIGHT_SCALE_NON_LINEAR
Add dev_err_probe() for first write to a register
Use dev_err_probe() instead of dev_err()
v4: https://lore.kernel.org/all/20240310135344.3455294-1-paroga@paroga.com/
Changes in v4:
Use backlight_*() to access backlight_device
Do not set backlight_properties.power
v3: https://lore.kernel.org/all/20240309132521.1290173-1-paroga@paroga.com/
Changes in v3:
Improved device tree bindings documentation
v2: https://lore.kernel.org/all/20240308215617.1729664-1-paroga@paroga.com/
Changes in v2:
Add device tree nodes for each output
Addressed multiple smaller review comments
v1: https://lore.kernel.org/all/20240302212757.1871164-1-paroga@paroga.com/
Patrick Gansterer (2):
dt-bindings: backlight: Add Texas Instruments LM3509
backlight: Add new lm3509 backlight driver
.../bindings/leds/backlight/ti,lm3509.yaml | 139 +++++++
drivers/video/backlight/Kconfig | 7 +
drivers/video/backlight/Makefile | 1 +
drivers/video/backlight/lm3509_bl.c | 340 ++++++++++++++++++
4 files changed, 487 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/backlight/ti,lm3509.yaml
create mode 100644 drivers/video/backlight/lm3509_bl.c
--
2.44.0
^ permalink raw reply
* Re: [PATCH v5 7/7] iio: accel: adxl345: Add spi-3wire option
From: Jonathan Cameron @ 2024-03-30 15:24 UTC (permalink / raw)
To: Lothar Rubusch
Cc: lars, Michael.Hennerich, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-iio, devicetree, linux-kernel, eraretuya
In-Reply-To: <CAFXKEHZva5AE=sinx_i0Gec2FbB4ZfyEu8mWg52omzGBvr5uXw@mail.gmail.com>
On Fri, 29 Mar 2024 01:33:01 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> On Thu, Mar 28, 2024 at 2:39 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Wed, 27 Mar 2024 22:03:20 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > > Add a setup function implementation to the spi module to enable spi-3wire
> > > as option when specified in the device-tree.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > > drivers/iio/accel/adxl345.h | 2 ++
> > > drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> > > 2 files changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > > index 4ea9341d4..e6bc3591c 100644
> > > --- a/drivers/iio/accel/adxl345.h
> > > +++ b/drivers/iio/accel/adxl345.h
> > > @@ -30,6 +30,8 @@
> > > #define ADXL345_POWER_CTL_MEASURE BIT(3)
> > > #define ADXL345_POWER_CTL_STANDBY 0x00
> > >
> > > +#define ADXL345_DATA_FORMAT_SPI_3WIRE BIT(6) /* 3-wire SPI mode */
> > > +
> > > #define ADXL345_DATA_FORMAT_RANGE GENMASK(1, 0) /* Set the g range */
> > > #define ADXL345_DATA_FORMAT_JUSTIFY BIT(2) /* Left-justified (MSB) mode */
> > > #define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> > > diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> > > index 1c0513bd3..f145d5c1d 100644
> > > --- a/drivers/iio/accel/adxl345_spi.c
> > > +++ b/drivers/iio/accel/adxl345_spi.c
> > > @@ -20,6 +20,16 @@ static const struct regmap_config adxl345_spi_regmap_config = {
> > > .read_flag_mask = BIT(7) | BIT(6),
> > > };
> > >
> > > +static int adxl345_spi_setup(struct device *dev, struct regmap *regmap)
> > > +{
> > > + struct spi_device *spi = container_of(dev, struct spi_device, dev);
> > > +
> > > + if (spi->mode & SPI_3WIRE)
> > > + return regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
> > > + ADXL345_DATA_FORMAT_SPI_3WIRE);
> > Your earlier patch carefully (I think) left one or two fields alone, then
> > this write just comes in and changes them. In particular INT_INVERT.
> >
> Why do you refer here to INT_INVERT? In this code above I try to set
> SPI to 3 lines. Since this is a SPI configuration, i.e. bus specific,
> it happens in adxl345_spi.c. Passing this function to the bus
> independent adxl345_core.c file allows to configure the bus first.
> Therefore, I'm using the update function in core masking out the SPI
> filag.
Ah. Ok. It was only intended to mask out the SPI3-wire bit, not the
other bits that you also masked out. I thought intent was to leave
them untouched for some reason. Given they don't matter in the driver
at the moment (no interrupt support) then no problem.
>
> My reason why I try to keep INT_INVERT out is different. There is
> another driver for the same part in the kernel:
> ./drivers/input/misc/adxl34x.c - This is a input driver, using the
> interrupts of the adxl345 for the input device implementation. I
> assumed, that in the iio implementation there won't be interrupt
> handling for the adx345, since it is not an input device. Does this
> make sense?
No. You can't use these two drivers at the same time. They will almost
certainly trample over each other in actually reading channels etc.
Their is some legacy of old drivers in input from a long time back.
Given this driver clearly doesn't have full functionality yet in IIO there
and the different userspace ABI, we've just left the input driver alone.
>
> > If it doesn't makes sense to write it there, either write that bit
> > every time here, or leave it alone every time. Not decide on whether
> > to write the bit based on SPI_3WIRE or not. As far as I know they
> > are unconnected features.
> >
> I think I did not understand. Could you please specify a bit more?
> When spi-3wire is configured in the DT it has to be parsed and handled
> in the bus specific part, i.e. the adxl345_spi.c Therefore I configure
> SPI_3WIRE there. I don't want to place SPI specific code in the core
> file.
My confusion was that you were deliberately not touching the other unused
flags. In reality you are touching the but only if you enable 3wire.
I would write them register to 0 in the !3wire case so all other values
are the same in both paths.
>
> > > + return 0;
> > > +}
> > > +
> > > static int adxl345_spi_probe(struct spi_device *spi)
> > > {
> > > struct regmap *regmap;
> > > @@ -33,7 +43,7 @@ static int adxl345_spi_probe(struct spi_device *spi)
> > > if (IS_ERR(regmap))
> > > return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
> > >
> > > - return adxl345_core_probe(&spi->dev, regmap, NULL);
> > > + return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
> > > }
> > >
> > > static const struct adxl345_chip_info adxl345_spi_info = {
> >
^ permalink raw reply
* Re: [PATCH v6 7/7] iio: accel: adxl345: Add spi-3wire option
From: Jonathan Cameron @ 2024-03-30 15:29 UTC (permalink / raw)
To: Lothar Rubusch
Cc: lars, Michael.Hennerich, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-iio, devicetree, linux-kernel, eraretuya
In-Reply-To: <20240329004030.16153-8-l.rubusch@gmail.com>
On Fri, 29 Mar 2024 00:40:30 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> Add a setup function implementation to the spi module to enable spi-3wire
> when specified in the device-tree.
>
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
> drivers/iio/accel/adxl345.h | 1 +
> drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index e859c01d4..3d5c8719d 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -31,6 +31,7 @@
> #define ADXL345_DATA_FORMAT_RANGE GENMASK(1, 0) /* Set the g range */
> #define ADXL345_DATA_FORMAT_JUSTIFY BIT(2) /* Left-justified (MSB) mode */
> #define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> +#define ADXL345_DATA_FORMAT_SPI_3WIRE BIT(6) /* 3-wire SPI mode */
> #define ADXL345_DATA_FORMAT_SELF_TEST BIT(7) /* Enable a self test */
>
> #define ADXL345_DATA_FORMAT_2G 0
> diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> index 1c0513bd3..f145d5c1d 100644
> --- a/drivers/iio/accel/adxl345_spi.c
> +++ b/drivers/iio/accel/adxl345_spi.c
> @@ -20,6 +20,16 @@ static const struct regmap_config adxl345_spi_regmap_config = {
> .read_flag_mask = BIT(7) | BIT(6),
> };
>
> +static int adxl345_spi_setup(struct device *dev, struct regmap *regmap)
> +{
> + struct spi_device *spi = container_of(dev, struct spi_device, dev);
> +
> + if (spi->mode & SPI_3WIRE)
> + return regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
> + ADXL345_DATA_FORMAT_SPI_3WIRE);
My only remaining comment on this patch set is to add equivalent of
else
return regmap_write(regmap, ADXL345_REG_DATA_FORMAT, 0);
If the hardware had some sort of software reset, that was used,
this wouldn't be needed as the status of those other bits would be known.
If we leave them alone in the non 3wire path we may in the future have
subtle issues because some other code left this in an odd state and
we clear those other bits only for 3wire mode.
Jonathan
> + return 0;
> +}
> +
> static int adxl345_spi_probe(struct spi_device *spi)
> {
> struct regmap *regmap;
> @@ -33,7 +43,7 @@ static int adxl345_spi_probe(struct spi_device *spi)
> if (IS_ERR(regmap))
> return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
>
> - return adxl345_core_probe(&spi->dev, regmap, NULL);
> + return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
> }
>
> static const struct adxl345_chip_info adxl345_spi_info = {
^ permalink raw reply
* Re: [PATCH 3/3] arm64: dts: rockchip: Remove UART2 from RGB30
From: Chris Morgan @ 2024-03-30 15:34 UTC (permalink / raw)
To: Ahmad Fatoum
Cc: Chris Morgan, linux-rockchip, linux-clk, devicetree, sboyd,
mturquette, heiko, conor+dt, krzysztof.kozlowski+dt, robh+dt
In-Reply-To: <7c7138f8-b0b7-46c3-b7b2-84e01467f368@pengutronix.de>
On Sat, Mar 30, 2024 at 02:13:05PM +0100, Ahmad Fatoum wrote:
> Hello Chris,
>
> On 18.10.23 17:33, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan@hotmail.com>
> >
> > The Powkiddy RGB30 has no onboard UART header, so remove the reference
> > to it in the device tree. This was left on by mistake in the initial
> > commit.
>
> Do you know if the UART is perhaps available over testpoints?
There is not one as best I can tell on either the RGB30 or RK2023. The
Powkiddy X55 does have UART, however. I was able to exploit the fact
that the RGB30 is extremely similar to all of the Anbernic devices
(such as the RG353 series) for the purposes of low-level development.
Once I got a network connection I performed the rest of development
over SSH, but prior to that I just developed on a different device.
Thank you,
Chris.
>
> If yes, having a DT-overlay upstream enabling it along with documentation could be useful.
> If not, how do you do low-level debugging on the RBG30 in absence of the serial console?
>
> Thanks,
> Ahmad
>
> >
> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > ---
> > arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rgb30.dts | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rgb30.dts b/arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rgb30.dts
> > index 3ebc21608213..1ead3c5c24b3 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rgb30.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rgb30.dts
> > @@ -64,6 +64,10 @@ simple-audio-card,cpu {
> >
> > /delete-node/ &adc_keys;
> >
> > +&chosen {
> > + /delete-property/ stdout-path;
> > +};
> > +
> > &cru {
> > assigned-clocks = <&pmucru CLK_RTC_32K>, <&cru PLL_GPLL>,
> > <&pmucru PLL_PPLL>, <&cru PLL_VPLL>;
> > @@ -149,4 +153,9 @@ rk817_charger: charger {
> > };
> > };
> >
> > +/* There is no UART header visible on the board for this device. */
> > +&uart2 {
> > + status = "disabled";
> > +};
> > +
> > /delete-node/ &vibrator;
>
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>
^ permalink raw reply
* Re: [PATCH 2/2] dt-bindings: iio: imu: add icm42688 inside inv_icm42600
From: Jonathan Cameron @ 2024-03-30 16:10 UTC (permalink / raw)
To: Conor Dooley
Cc: inv.git-commit, robh, krzysztof.kozlowski+dt, conor+dt, lars,
linux-iio, devicetree, Jean-Baptiste Maneyrol
In-Reply-To: <20240329-fifth-earpiece-78daf4d943ce@spud>
On Fri, 29 Mar 2024 15:49:26 +0000
Conor Dooley <conor@kernel.org> wrote:
> On Fri, Mar 29, 2024 at 03:15:35PM +0000, inv.git-commit@tdk.com wrote:
> > From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> >
> > Add bindings for ICM-42688-P chip.
> >
> > Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
>
> My initial thought was that you're missing a sign-off, but is
> "inv.git-commit@tdk.com" some system you have to bypass corporate email
> garbage?
Common enough setup, as long as the From: line matches the sign-off, git will
ignore the email address used to send it when the patch is applied.
>
> > ---
> > .../devicetree/bindings/iio/imu/invensense,icm42600.yaml | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml b/Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml
> > index 7cd05bcbee31..152aec8d82ff 100644
> > --- a/Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml
> > +++ b/Documentation/devicetree/bindings/iio/imu/invensense,icm42600.yaml
> > @@ -31,6 +31,7 @@ properties:
> > - invensense,icm42602
> > - invensense,icm42605
> > - invensense,icm42622
> > + - invensense,icm42688
>
> Can you add this in alphanumerical order please?
>
> Also, this patch should be the first in the series.
Agreed on both, otherwise this series LGTM
Jonathan
>
> Thanks,
> Conor.
>
> > - invensense,icm42631
> >
> > reg:
> > --
> > 2.34.1
> >
^ permalink raw reply
* Re: [PATCH 3/3] dt-bindings: adc: Document XuanTie TH1520 ADC
From: Jonathan Cameron @ 2024-03-30 16:14 UTC (permalink / raw)
To: wefu
Cc: jszhang, guoren, conor, robh, krzysztof.kozlowski+dt,
paul.walmsley, palmer, aou, lars, andriy.shevchenko, nuno.sa,
marcelo.schmitt, bigunclemax, marius.cristea, fr0st61te,
okan.sahin, marcus.folkesson, schnelle, lee, mike.looijmans,
linux-riscv, devicetree, linux-kernel, linux-iio
In-Reply-To: <20240329200241.4122000-4-wefu@redhat.com>
On Sat, 30 Mar 2024 04:01:26 +0800
wefu@redhat.com wrote:
> From: Wei Fu <wefu@redhat.com>
>
> Document devicetree bindings for the XuanTie TH1520 AP sub-system ADC.
>
> Signed-off-by: Wei Fu <wefu@redhat.com>
Hi.
Few comments inline.
Given Fugang Duan is listed as a maintainer I'd also like an ack ideally
from them. BTW your +CC list seems a bit random.
Thanks,
Jonathan
> ---
> .../bindings/iio/adc/thead,th1520.yaml | 52 +++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml b/Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml
> new file mode 100644
> index 000000000000..80890ce62810
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/thead,th1520.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/thead,th1520.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: XuanTie TH1520 Analog to Digital Converter (ADC)
> +
> +maintainers:
> + - Fugang Duan <duanfugang.dfg@linux.alibaba.com>
> + - Wei Fu <wefu@redhat.com>
> +
> +description: |
> + 12-Bit Analog to Digital Converter (ADC) on XuanTie TH1520
> +
> +properties:
> + compatible:
> + const: thead,th1520
See the example - doesn't match. Should be -adc here as well
as this is just part of the SoC.
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + maxItems: 1
> +
> + clock-names:
> + const: adc
No objection to having names, but if there is only 1 why is
it required?
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - clock-names
> + - status
Status doesn't need to be here
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + adc: adc@0xfffff51000 {
> + compatible = "thead,th1520-adc";
As the bot pointed out, spaces not tabs for these - Please run the suggested
tests in that email before sending a v2.
> + reg = <0xff 0xfff51000 0x0 0x1000>;
> + interrupts = <61 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&aonsys_clk>;
> + clock-names = "adc";
> + /* ADC pin is proprietary,no need to config pinctrl */
> + status = "disabled";
No need for a status entry in the example.
> + };
^ permalink raw reply
* Re: [PATCH v2 3/3] arm64: dts: freescale: Add device tree for Emcraft Systems NavQ+ Kit
From: Andrew Lunn @ 2024-03-30 16:46 UTC (permalink / raw)
To: Gilles Talis
Cc: devicetree, imx, linux-arm-kernel, conor+dt,
krzysztof.kozlowski+dt, robh, shawnguo, festevam, alex
In-Reply-To: <20240330133410.41408-4-gilles.talis@gmail.com>
> +&eqos {
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_eqos>;
> + phy-mode = "rgmii-id";
> + phy-handle = <ðphy0>;
> + status = "okay";
> +
> + mdio {
> + compatible = "snps,dwmac-mdio";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ethphy0: ethernet-phy@0 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + reg = <0>;
> + reset-gpios = <&gpio4 22 GPIO_ACTIVE_LOW>;
> + reset-assert-us = <1000>;
> + reset-deassert-us = <10000>;
> + qca,disable-smarteee;
> + qca,disable-hibernation-mode;
> + };
> + };
> +};
Thanks for removing the regulator.
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v2 3/3] arm64: dts: freescale: Add device tree for Emcraft Systems NavQ+ Kit
From: Fabio Estevam @ 2024-03-30 17:03 UTC (permalink / raw)
To: Gilles Talis
Cc: devicetree, imx, linux-arm-kernel, conor+dt,
krzysztof.kozlowski+dt, robh, shawnguo, alex, andrew
In-Reply-To: <20240330133410.41408-4-gilles.talis@gmail.com>
Hi Gilles,
On Sat, Mar 30, 2024 at 10:34 AM Gilles Talis <gilles.talis@gmail.com> wrote:
>
> The Emcraft Systems NavQ+ kit is a mobile robotics platform
> based on NXP i.MX8 MPlus SoC.
>
> The following interfaces and devices are enabled:
> - eMMC
> - Gigabit Ethernet
> - RTC
> - SD-Card
> - UART console
>
> Signed-off-by: Gilles Talis <gilles.talis@gmail.com>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
^ permalink raw reply
* Re: [RFC PATCH 1/2] spi: dt-bindings: add Siflower Quad SPI controller
From: Krzysztof Kozlowski @ 2024-03-30 17:42 UTC (permalink / raw)
To: Qingfang Deng, Mark Brown, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Qingfang Deng, linux-spi, devicetree, linux-kernel
In-Reply-To: <20240329015147.1481349-1-dqfext@gmail.com>
On 29/03/2024 02:51, Qingfang Deng wrote:
> From: Qingfang Deng <qingfang.deng@siflower.com.cn>
>
> Add YAML devicetree bindings for Siflower Quad SPI controller.
Describe the hardware. What is this Siflower?
>
> Signed-off-by: Qingfang Deng <qingfang.deng@siflower.com.cn>
> ---
> .../bindings/spi/siflower,qspi.yaml | 54 +++++++++++++++++++
> 1 file changed, 54 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/spi/siflower,qspi.yaml
>
> diff --git a/Documentation/devicetree/bindings/spi/siflower,qspi.yaml b/Documentation/devicetree/bindings/spi/siflower,qspi.yaml
> new file mode 100644
> index 000000000000..c2dbe82affc2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/siflower,qspi.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/siflower,qspi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Siflower Quad Serial Peripheral Interface (QSPI)
> +
> +maintainers:
> + - Qingfang Deng <qingfang.deng@siflower.com.cn>
> +
> +allOf:
> + - $ref: spi-controller.yaml#
> +
> +properties:
> + compatible:
> + const: siflower,qspi
Except that this was not tested, aren't you adding it for some SoC? If
so, then you miss here SoC part.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [RFC PATCH v2 1/2] spi: dt-bindings: add Siflower Quad SPI controller
From: Krzysztof Kozlowski @ 2024-03-30 17:43 UTC (permalink / raw)
To: Qingfang Deng, Mark Brown, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Qingfang Deng, linux-spi, devicetree, linux-kernel
In-Reply-To: <20240329054657.1602450-1-dqfext@gmail.com>
On 29/03/2024 06:46, Qingfang Deng wrote:
> From: Qingfang Deng <qingfang.deng@siflower.com.cn>
>
> Add YAML devicetree bindings for Siflower Quad SPI controller.
Not much improved.
Do not attach (thread) your patchsets to some other threads (unrelated
or older versions). This buries them deep in the mailbox and might
interfere with applying entire sets.
Allow people to actually review your code - give them some time, like
24h between postings.
>
> Signed-off-by: Qingfang Deng <qingfang.deng@siflower.com.cn>
> ---
> v2: fix dt_binding_check reported errors
>
> .../bindings/spi/siflower,qspi.yaml | 54 +++++++++++++++++++
> 1 file changed, 54 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/spi/siflower,qspi.yaml
>
> diff --git a/Documentation/devicetree/bindings/spi/siflower,qspi.yaml b/Documentation/devicetree/bindings/spi/siflower,qspi.yaml
> new file mode 100644
> index 000000000000..15ce25a2176a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/siflower,qspi.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/siflower,qspi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Siflower Quad Serial Peripheral Interface (QSPI)
> +
> +maintainers:
> + - Qingfang Deng <qingfang.deng@siflower.com.cn>
> +
> +allOf:
> + - $ref: spi-controller.yaml#
> +
> +properties:
> + compatible:
> + const: siflower,qspi
> +
My previous comments stay valid. Respond to them.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/3] dt-bindings: panel: Add LG SW43408 MIPI-DSI panel
From: Krzysztof Kozlowski @ 2024-03-30 18:10 UTC (permalink / raw)
To: Dmitry Baryshkov, Sumit Semwal, Caleb Connolly, Neil Armstrong,
Jessica Zhang, Sam Ravnborg, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: dri-devel, devicetree, linux-kernel, linux-arm-msm, Vinod Koul,
Caleb Connolly
In-Reply-To: <20240330-lg-sw43408-panel-v2-1-293a58717b38@linaro.org>
On 30/03/2024 16:00, Dmitry Baryshkov wrote:
> From: Sumit Semwal <sumit.semwal@linaro.org>
>
> LG SW43408 is 1080x2160, 4-lane MIPI-DSI panel present on Google Pixel 3
> phones.
>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
> [caleb: convert to yaml]
> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: arm: sunxi: document Tanix TX1 name
From: Krzysztof Kozlowski @ 2024-03-30 18:11 UTC (permalink / raw)
To: Andre Przywara, Chen-Yu Tsai, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jernej Skrabec, Samuel Holland
Cc: devicetree, linux-sunxi
In-Reply-To: <20240330013243.17943-2-andre.przywara@arm.com>
On 30/03/2024 02:32, Andre Przywara wrote:
> The Tanix TX1 is a tiny TV box with the Allwinner H313 SoC, a lower bin
> version of the Allwinner H616. It comes with no SD card slot or Ethernet
> port.
>
> Add the board/SoC compatible string pair to the list of known boards.
> Since the H313 does not look different from a software point of view,
> we keep the H616 compatible string.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 net-next 1/2] dt-bindings: net: airoha,en8811h: Add en8811h
From: Krzysztof Kozlowski @ 2024-03-30 18:13 UTC (permalink / raw)
To: Eric Woudstra, Rob Herring
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Krzysztof Kozlowski, Conor Dooley, Andrew Lunn, Heiner Kallweit,
Russell King, Matthias Brugger, AngeloGioacchino Del Regno,
Frank Wunderlich, Daniel Golle, Lucien Jheng, Zhi-Jun You, netdev,
devicetree
In-Reply-To: <9ea90a1a-37b2-4baf-94af-2b89276a625d@gmail.com>
On 26/03/2024 21:41, Eric Woudstra wrote:
> Hi Rob,
>
> On 3/26/24 20:29, Rob Herring wrote:
>> On Tue, Mar 26, 2024 at 05:23:04PM +0100, Eric Woudstra wrote:
>>> Add the Airoha EN8811H 2.5 Gigabit PHY.
>>>
>>> The en8811h phy can be set with serdes polarity reversed on rx and/or tx.
>>>
>>> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
>>
>> Did you change something or forget to add Krzysztof's Reviewed-by?
>
> Nothing has changed in this commit. I was wondering if I should do this,
> so I should have added the Reviewed-by Krzysztof.
>
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.
https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
Please carefully read above guideline. Entire.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v6 1/3] dt-bindings: dmaengine: Add dma multiplexer for CV18XX/SG200X series SoC
From: Krzysztof Kozlowski @ 2024-03-30 18:17 UTC (permalink / raw)
To: Inochi Amaoto, Vinod Koul, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Chen Wang, Paul Walmsley, Palmer Dabbelt, Albert Ou
Cc: Jisheng Zhang, Liu Gui, Jingbao Qiu, dlan, dmaengine, devicetree,
linux-kernel, linux-riscv
In-Reply-To: <IA1PR20MB4953DDD8E89675650E0E2529BB3A2@IA1PR20MB4953.namprd20.prod.outlook.com>
On 29/03/2024 03:04, Inochi Amaoto wrote:
> The DMA IP of Sophgo CV18XX/SG200X is based on a DW AXI CORE, with
> an additional channel remap register located in the top system control
> area. The DMA channel is exclusive to each core.
>
> In addition, the DMA multiplexer is a subdevice of system controller,
> so this binding only contains necessary properties for the multiplexer
> itself.
>
> Add the dmamux binding for CV18XX/SG200X series SoC.
>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 1/3] drivers/iio/adc: Add XuanTie TH1520 ADC driver
From: Jonathan Cameron @ 2024-03-30 18:17 UTC (permalink / raw)
To: wefu
Cc: jszhang, guoren, conor, robh, krzysztof.kozlowski+dt,
paul.walmsley, palmer, aou, lars, andriy.shevchenko, nuno.sa,
marcelo.schmitt, bigunclemax, marius.cristea, fr0st61te,
okan.sahin, marcus.folkesson, schnelle, lee, mike.looijmans,
linux-riscv, devicetree, linux-kernel, linux-iio
In-Reply-To: <20240329200241.4122000-2-wefu@redhat.com>
On Sat, 30 Mar 2024 04:01:24 +0800
wefu@redhat.com wrote:
> From: Wei Fu <wefu@redhat.com>
>
> Signed-off-by: Wei Fu <wefu@redhat.com>
Hi Wei Fu,
Comments inline.
Main thing is that there is quite a bit of code here to support a more
feature rich driver, but with parts missing. Rip all that out until those
features are added in future patches.
Jonathan
> ---
> drivers/iio/adc/Kconfig | 13 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/xuantie-th1520-adc.c | 572 +++++++++++++++++++++++++++
> drivers/iio/adc/xuantie-th1520-adc.h | 193 +++++++++
> 4 files changed, 779 insertions(+)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 0d9282fa67f5..9e37ba2a877a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1515,4 +1515,17 @@ config XILINX_AMS
> The driver can also be built as a module. If so, the module will be called
> xilinx-ams.
>
> +config XUANTIE_TH1520_ADC
> + tristate "XuanTie TH1520 ADC driver"
> + depends on OF
No need for that dependency. In theory you could use this with ACPI PRP0001
for example.
> + depends on HAS_IOMEM
> + depends on ARCH_THEAD
|| COMPILE_TEST
to get us build coverage as I don't immediately see anything arch specific.
> + default y
Don't set the default (i.e. rely on default default of no.)
> + help
> + Say yes here to support for XuanTie TH1520 MPW analog-to-digital
> + converter.
> +
> + This driver can also be built as a module. If so, the module will be
> + called xuantie-th1520-adc.
> +
> endmenu
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index b3c434722364..820e2a136b33 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -135,4 +135,5 @@ obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
> xilinx-xadc-y := xilinx-xadc-core.o xilinx-xadc-events.o
> obj-$(CONFIG_XILINX_XADC) += xilinx-xadc.o
> obj-$(CONFIG_XILINX_AMS) += xilinx-ams.o
> +obj-$(CONFIG_XUANTIE_TH1520_ADC) += xuantie-th1520-adc.o
> obj-$(CONFIG_SD_ADC_MODULATOR) += sd_adc_modulator.o
> diff --git a/drivers/iio/adc/xuantie-th1520-adc.c b/drivers/iio/adc/xuantie-th1520-adc.c
> new file mode 100644
> index 000000000000..df0452c2abe7
> --- /dev/null
> +++ b/drivers/iio/adc/xuantie-th1520-adc.c
> @@ -0,0 +1,572 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * XuanTie TH1520 ADC driver
> + *
> + * Copyright (C) 2021-2024 Alibaba Group Holding Limited.
> + * Fugang Duan <duanfugang.dfg@linux.alibaba.com>
> + *
Blank line here not useful so drop it.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
As below. Unlikely these are needed in a driver.
> +#include <linux/regulator/consumer.h>
> +#include <linux/of_platform.h>
Doubt you need that. mod_devicetable.h perhaps?
> +#include <linux/err.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
Why?
> +#include <linux/iio/sysfs.h>
> +
> +#include "xuantie-th1520-adc.h"
> +
> +static inline void th1520_adc_cfg_init(struct th1520_adc *info)
> +{
> + struct th1520_adc_feature *adc_feature = &info->adc_feature;
> +
> + /* set default Configuration for ADC controller */
> + adc_feature->selres_sel = TH1520_ADC_SELRES_12BIT;
> + adc_feature->input_mode = TH1520_ADC_SINGLE_ENDED_INPUTS;
> + adc_feature->vol_ref = TH1520_ADC_VOL_VREF;
> + adc_feature->offset_mode = TH1520_ADC_OFFSET_DIS;
> + adc_feature->conv_mode = TH1520_ADC_MODE_SINGLE;
> + adc_feature->clk_sel = TH1520_ADC_FCLK_TYP_1M;
> +
> + adc_feature->int_actual = TH1520_ADC_ACTUAL_ALL;
> + adc_feature->int_detal = TH1520_ADC_DETAL_ALL;
> +
> + info->ch0_offmeas = 0;
> + info->ch1_offmeas = 0;
Bring these features in when you add support to the driver. Until then
stick just using the default values inline as it simplifies the code.
> +}
> +
> +static void th1520_adc_reg_set(struct th1520_adc *info)
> +{
> + u32 phy_cfg = 0;
> + u32 op_ctrl = 0;
> + struct th1520_adc_feature *adc_feature = &info->adc_feature;
> +
> + /* phy_cfg */
That's obvious from the code, so I'd drop the comment.
> + switch (adc_feature->selres_sel) {
> + case TH1520_ADC_SELRES_6BIT:
Use numbers here. You could just use maths for the conversion as well
to simplify things further. However I've suggested elsewhere that you just
always use 12bit in which case this complexity isn't needed.
> + phy_cfg |= TH1520_ADC_PHY_CFG_SELRES_6BIT;
> + break;
> + case TH1520_ADC_SELRES_8BIT:
> + phy_cfg |= TH1520_ADC_PHY_CFG_SELRES_8BIT;
> + break;
> + case TH1520_ADC_SELRES_10BIT:
> + phy_cfg |= TH1520_ADC_PHY_CFG_SELRES_10BIT;
> + break;
> + case TH1520_ADC_SELRES_12BIT:
> + phy_cfg |= TH1520_ADC_PHY_CFG_SELRES_12BIT;
> + break;
> + default:
It's an enum, so we shouldn't get here with anything else and can drop the default.
> + break;
> + }
> +
> + switch (adc_feature->input_mode) {
> + case TH1520_ADC_SINGLE_ENDED_INPUTS:
> + phy_cfg |= TH1520_ADC_PHY_CFG_SELDIFF_SINGLE_ENDED_INPUTS;
> + break;
> + case TH1520_ADC_DIFFERENTIAL_INPUTS:
> + phy_cfg |= TH1520_ADC_PHY_CFG_SELDIFF_DIFFERENTIAL_INPUTS;
> + break;
> + default:
Not need as all values covered.
> + break;
> + }
> +
> + switch (adc_feature->vol_ref) {
> + case TH1520_ADC_VOL_VREF:
> + phy_cfg |= TH1520_ADC_PHY_CFG_SELBG_EXTERNAL |
> + TH1520_ADC_PHY_CFG_SELREF_EXT;
> + break;
> + case TH1520_ADC_VOL_INTE:
> + phy_cfg |= TH1520_ADC_PHY_CFG_SELBG_INTERNAL |
> + TH1520_ADC_PHY_CFG_SELREF_INTERNAL;
> + break;
> + default:
Probably also not needed.
> + break;
> + }
> +
> + /* op_ctrl */
> + switch (adc_feature->conv_mode) {
> + case TH1520_ADC_MODE_SINGLE:
> + op_ctrl |= TH1520_ADC_OP_CTRL_MODE_SINGLE;
> + break;
> + case TH1520_ADC_MODE_CONTINOUS:
> + op_ctrl |= TH1520_ADC_OP_CTRL_MODE_CONTINOUS;
> + break;
> + default:
As above.
> + break;
> + }
> +
> + writel(phy_cfg, info->regs + TH1520_ADC_PHY_CFG);
I'd keep the writes with the register setup, so move this up above
filling in op_ctrl value.
> + writel(op_ctrl, info->regs + TH1520_ADC_OP_CTRL);
> +
> + writel(TH1520_ADC_PHY_ENCTR, info->regs + TH1520_ADC_PHY_TEST);
> +
> + /* disable the irq */
> + writel(0xff, info->regs + TH1520_ADC_INT_CTRL1);
> + writel(0xff, info->regs + TH1520_ADC_INT_CTRL2);
> +
> + if (adc_feature->conv_mode == TH1520_ADC_MODE_CONTINOUS)
> + writel(TH1520_ADC_PHY_CTRL_ENADC_EN,
> + info->regs + TH1520_ADC_PHY_CTRL);
> +}
> +
> +static void th1520_adc_fclk_set(struct th1520_adc *info)
> +{
> + int fclk_ctrl = 0;
> + int start_time = 0;
> + int sample_time = 0;
> + struct th1520_adc_feature *adc_feature = &info->adc_feature;
> +
> + switch (adc_feature->clk_sel) {
> + case TH1520_ADC_FCLK_TYP_1M:
> + fclk_ctrl = TH1520_ADC_FCLK_CTRL_TYP_1M;
> + start_time = TH1520_ADC_START_TIME_TYP_1M;
> + if (adc_feature->selres_sel == TH1520_ADC_SELRES_6BIT)
Use numbers. Also maybe just convert from resolution to register value
using maths rather than an if/else stack. Otherwise, you could just have
an array mapping from register value to real value and search it.
> + sample_time = TH1520_ADC_SAMPLE_TIME_TYP_6BIT;
> + else if (adc_feature->selres_sel == TH1520_ADC_SELRES_8BIT)
> + sample_time = TH1520_ADC_SAMPLE_TIME_TYP_8BIT;
> + else if (adc_feature->selres_sel == TH1520_ADC_SELRES_10BIT)
> + sample_time = TH1520_ADC_SAMPLE_TIME_TYP_10BIT;
> + else if (adc_feature->selres_sel == TH1520_ADC_SELRES_12BIT)
> + sample_time = TH1520_ADC_SAMPLE_TIME_TYP_12BIT;
> + else {
> + pr_err("[%s,%d]invalid selres select\n",
> + __func__, __LINE__);
> + return;
> + }
> + break;
> + default:
Comment on why nothing to do here.
> + break;
> + }
> + writel(fclk_ctrl, info->regs + TH1520_ADC_FCLK_CTRL);
> + writel(start_time, info->regs + TH1520_ADC_START_TIME);
> + writel(sample_time, info->regs + TH1520_ADC_SAMPLE_TIME);
> +}
> +
> +static void th1520_adc_hw_init(struct th1520_adc *info)
> +{
> + th1520_adc_reg_set(info);
> + th1520_adc_fclk_set(info);
> +}
> +
> +static const struct iio_chan_spec th1520_adc_iio_channels[] = {
> + TH1520_ADC_CHAN(0, IIO_VOLTAGE),
> + TH1520_ADC_CHAN(1, IIO_VOLTAGE),
> + TH1520_ADC_CHAN(2, IIO_VOLTAGE),
> + TH1520_ADC_CHAN(3, IIO_VOLTAGE),
> + TH1520_ADC_CHAN(4, IIO_VOLTAGE),
> + TH1520_ADC_CHAN(5, IIO_VOLTAGE),
> + TH1520_ADC_CHAN(6, IIO_VOLTAGE),
> + TH1520_ADC_CHAN(7, IIO_VOLTAGE),
> + /* sentinel */
? There isn't a sentinel so drop the comment.
> +};
> +
> +static irqreturn_t th1520_adc_isr(int irq, void *dev_id)
> +{
> + struct iio_dev *indio_dev = dev_id;
> + struct th1520_adc *info = iio_priv(indio_dev);
> + /* TBD */
? Shouldn't see a TBD in a submitted driver.
> + complete(&info->completion);
> + return IRQ_HANDLED;
> +}
> +
> +static int th1520_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + int tmp;
> + long ret;
> + struct th1520_adc *info = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
Could use guard() here though the saving in this case is monor.
> + mutex_lock(&info->mlock);
> +
> + if (info->adc_feature.conv_mode == TH1520_ADC_MODE_SINGLE) {
At the moment there is no control over this mode, so I would drop
the alternative path until you add such control. Also drop even tracking
what that mode is set to as it won't change.
> + uint ievent;
> + uint vld_flag;
> + uint phy_ctrl;
> + uint real_chan;
> + uint op_ctrl = 0;
> + uint single_retry = TH1520_ADC_FIFO_DATA_SIZE;
> +
> + op_ctrl = readl(info->regs + TH1520_ADC_OP_CTRL);
> + op_ctrl &= ~TH1520_ADC_OP_CTRL_CH_EN_ALL;
> + op_ctrl |= (BIT(chan->channel + TH1520_ADC_OP_CTRL_CH_EN_0) &
> + TH1520_ADC_OP_CTRL_CH_EN_ALL);
FIELD_PREP()
> + writel(op_ctrl, info->regs + TH1520_ADC_OP_CTRL);
This first block is common to both paths. I'd factor out as much as possible
> +
> + writel(TH1520_ADC_PHY_CTRL_ENADC_EN,
> + info->regs + TH1520_ADC_PHY_CTRL);
> +
> + vld_flag = TH1520_ADC_SAMPLE_DATA_CH0_VLD;
> +
> + while (single_retry--) {
Add a common to justify the retries (particularly why this number of them).
This deep indentation suggests to me that some of this code should be factored
out into a separate function.
> + writel(TH1520_ADC_OP_SINGLE_START_EN,
> + info->regs + TH1520_ADC_OP_SINGLE_START);
> + /* wait the sampling result */
> + ret = readl_poll_timeout(info->regs +
> + TH1520_ADC_SAMPLE_DATA,
> + ievent,
> + ievent & vld_flag, 100,
> + TH1520_ADC_TIMEOUT);
> + if (ret)
> + pr_err("wait the sampling timeout\n");
Given you are going to retry, shouldn't see an error message here.
> +
> + real_chan =
> + (ievent & TH1520_ADC_SAMPLE_DATA_CH0_NUMBER) >>
> + TH1520_ADC_SAMPLE_DATA_CH0_NUMBER_OFF;
FIELD_GET()
> + if (real_chan == chan->channel)
> + break;
> + }
> +
> + phy_ctrl = readl(info->regs + TH1520_ADC_PHY_CTRL);
> + phy_ctrl &= ~TH1520_ADC_PHY_CTRL_ENADC_EN;
> + writel(phy_ctrl, info->regs + TH1520_ADC_PHY_CTRL);
> +
> + /* read the sampling data */
> + *val = (ievent & TH1520_ADC_SAMPLE_DATA_CH0) >>
> + TH1520_ADC_SAMPLE_DATA_CH0_OFF;
FIELD_GET()
> + } else {
> + uint ievent;
> + uint vld_flag;
> + uint op_single;
> + uint op_ctrl = 0;
> +
> + op_ctrl = readl(info->regs + TH1520_ADC_OP_CTRL);
> + op_ctrl &= ~TH1520_ADC_OP_CTRL_CH_EN_ALL;
> + op_ctrl |= (BIT(chan->channel + TH1520_ADC_OP_CTRL_CH_EN_0) &
> + TH1520_ADC_OP_CTRL_CH_EN_ALL);
> + writel(op_ctrl, info->regs + TH1520_ADC_OP_CTRL);
> +
> + op_single = readl(info->regs +
> + TH1520_ADC_OP_SINGLE_START);
> + op_single &= ~TH1520_ADC_OP_SINGLE_START_EN;
> + writel(op_single,
> + info->regs + TH1520_ADC_OP_SINGLE_START);
> +
> + vld_flag = TH1520_ADC_SAMPLE_DATA_CH0_VLD |
> + TH1520_ADC_SAMPLE_DATA_CH1_VLD;
> +
> + /* wait the sampling result */
> + ret = readl_poll_timeout(info->regs +
> + TH1520_ADC_SAMPLE_DATA,
> + ievent, ievent & vld_flag, 10,
> + TH1520_ADC_TIMEOUT);
> + if (ret)
> + pr_err("wait the sampling timeout\n");
> +
> + /* read the sampling data */
> + tmp = readl(info->regs + TH1520_ADC_SAMPLE_DATA);
> + if (tmp & TH1520_ADC_SAMPLE_DATA_CH0_VLD)
> + *val = (tmp & TH1520_ADC_SAMPLE_DATA_CH0) >>
> + TH1520_ADC_SAMPLE_DATA_CH0_OFF;
> + else
> + *val = (tmp & TH1520_ADC_SAMPLE_DATA_CH1) >>
> + TH1520_ADC_SAMPLE_DATA_CH1_OFF;
> + }
> +
> + mutex_unlock(&info->mlock);
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + *val = info->vref_uv / 1000;
> + *val2 = info->adc_feature.selres_sel;
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = info->current_clk;
> + *val2 = 0;
> + return IIO_VAL_INT;
> +
> + default:
> + break;
return -EINVAL; then drop the one below.
> + }
> +
> + return -EINVAL;
> +}
> +
> +static ssize_t th1520_adc_res_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + size_t bufpos = 0, count = 5;
> + struct platform_device *pdev = to_platform_device(dev);
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct th1520_adc *info = iio_priv(indio_dev);
> +
> + snprintf(buf + bufpos, count - bufpos, "%.*x: ", 4,
> + info->adc_feature.selres_sel);
> + bufpos += 4;
> + buf[bufpos++] = '\n';
> +
> + return bufpos;
> +}
> +
> +static ssize_t th1520_adc_res_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size)
> +{
> + unsigned long res;
> + char *start = (char *)buf;
> + struct platform_device *pdev = to_platform_device(dev);
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct th1520_adc *info = iio_priv(indio_dev);
> +
> + if (kstrtoul(start, 0, &res))
> + return -EINVAL;
> +
> + switch (res) {
> + case TH1520_ADC_SELRES_6BIT:
> + case TH1520_ADC_SELRES_8BIT:
> + case TH1520_ADC_SELRES_10BIT:
> + case TH1520_ADC_SELRES_12BIT:
> + info->adc_feature.selres_sel = res;
> + th1520_adc_reset(info);
> + th1520_adc_hw_init(info);
> + break;
> + default:
> + dev_err(dev, "not support res\n");
> + return -EINVAL;
> + }
> +
> + return size;
> +}
> +
> +static DEVICE_ATTR_RW(th1520_adc_res);
Custom ABI needs documentation. We very rarely allow direct control of resolution
as it normally also affects scale which is standard ABI.
Also, for drivers only providing polled reads, the saving in time of using
low res readouts it negligible so just use 12bit ever time.
> +
> +static int th1520_adc_probe(struct platform_device *pdev)
> +{
> + int irq;
> + int ret;
> + struct resource *mem;
> + struct th1520_adc *info;
> + struct iio_dev *indio_dev;
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev,
> + sizeof(struct th1520_adc));
> + if (!indio_dev) {
> + dev_err(&pdev->dev, "Failed allocating iio device\n");
> + return -ENOMEM;
dev_err_probe() here as well.
> + }
> +
> + info = iio_priv(indio_dev);
> + info->dev = &pdev->dev;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return irq;
> +
> + ret = devm_request_irq(info->dev, irq, th1520_adc_isr, 0,
> + dev_name(&pdev->dev), indio_dev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq);
> + return ret;
Use return dev_err_probe() for errors in probe() routines and functions that
are only called during probe. It better handles deferred probing and also
gives shorter code in general.
> + }
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + info->regs = devm_ioremap_resource(&pdev->dev, mem);
> + if (IS_ERR(info->regs))
> + return PTR_ERR(info->regs);
> +
> + info->clk = devm_clk_get(&pdev->dev, "adc");
> + if (IS_ERR(info->clk)) {
> + dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
> + PTR_ERR(info->clk));
> + return PTR_ERR(info->clk);
> + }
> +
> + info->vref = devm_regulator_get(&pdev->dev, "vref");
> + if (IS_ERR(info->vref))
> + return PTR_ERR(info->vref);
> +
> + ret = regulator_enable(info->vref);
> + if (ret)
> + return ret;
> +
> + info->vref_uv = regulator_get_voltage(info->vref);
There is a patch set on list that will replace this boiler plate with a single
call to enable the regulator and get it's voltage, but we can make use of that
once/if that series is accepted.
> +
> + platform_set_drvdata(pdev, indio_dev);
> +
> + mutex_init(&info->mlock);
> + init_completion(&info->completion);
> +
> + indio_dev->name = dev_name(&pdev->dev);
> + indio_dev->dev.parent = &pdev->dev;
> + indio_dev->dev.of_node = pdev->dev.of_node;
Both parent and of_node are set in the IIO core. I doubt you need to override
them.
> + indio_dev->info = &th1520_adc_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = th1520_adc_iio_channels;
> + indio_dev->num_channels = ARRAY_SIZE(th1520_adc_iio_channels);
> +
> + ret = clk_prepare_enable(info->clk);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Could not prepare or enable the clock.\n");
> + goto error_adc_clk_enable;
> + }
> +
> + th1520_adc_cfg_init(info);
> + th1520_adc_reset(info);
> + th1520_adc_hw_init(info);
> +
> + ret = iio_device_register(indio_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Couldn't register the device.\n");
> + goto error_iio_device_register;
> + }
> +
> + ret = sysfs_create_file(&pdev->dev.kobj, &dev_attr_th1520_adc_res.attr);
This is very much the wrong way to do sysfs attributes in general, but I doubt
this will end up in the final driver anyway so I won't go into how this should
be done. Adding a file after registration is racey with userspace.
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to create adc debug sysfs.\n");
> + goto error_iio_device_register;
> + }
> +
> + dev_info(&pdev->dev, "XuanTie TH1520 adc registered.\n");
Too noisy. There are loads of ways of finding that out that don't need a message
in the kernel log. If you really want something then dev_dbg() at most.
> + return 0;
> +
> +error_iio_device_register:
> + clk_disable_unprepare(info->clk);
> +error_adc_clk_enable:
> + regulator_disable(info->vref);
> +
> + return ret;
> +}
> +
> +static int th1520_adc_remove(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct th1520_adc *info = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
> + regulator_disable(info->vref);
> + clk_disable_unprepare(info->clk);
> +
> + return 0;
> +}
> +static SIMPLE_DEV_PM_OPS(th1520_adc_pm_ops,
IIRC this is deprecated use.
DEFINE_SIMPLE_DEV_PM_OPS()
> + th1520_adc_suspend, th1520_adc_resume);
> +
> +static struct platform_driver th1520_adc_driver = {
> + .probe = th1520_adc_probe,
> + .remove = th1520_adc_remove,
> + .driver = {
> + .name = DRIVER_NAME,
Put the string here and don't use a define.
> + .of_match_table = th1520_adc_match,
> + .pm = &th1520_adc_pm_ops,
with the above, pm_sleep_ptr() around this and then if you build
without PM support the unused code will be removed by the compiler.
> + },
> +};
> +module_platform_driver(th1520_adc_driver);
> +
> +MODULE_AUTHOR("fugang.duan <duanfugang.dfg@linux.alibaba.com>");
> +MODULE_DESCRIPTION("XuanTie TH1520 ADC driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/iio/adc/xuantie-th1520-adc.h b/drivers/iio/adc/xuantie-th1520-adc.h
> new file mode 100644
> index 000000000000..c38d17fc6bbe
> --- /dev/null
> +++ b/drivers/iio/adc/xuantie-th1520-adc.h
> @@ -0,0 +1,193 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * XuanTie TH1520 ADC driver
> + *
> + * Copyright (C) 2021-2024 Alibaba Group Holding Limited.
> + * Fugang Duan <duanfugang.dfg@linux.alibaba.com>
> + *
> + */
> +
> +#include <linux/bitops.h>
> +
> +/* This will be the driver name the kernel reports */
> +#define DRIVER_NAME "xuantie-th1520-adc"
No obvious reason for a header - push this down into the C file.
> +
> +/* ADC registers */
> +#define TH1520_ADC_PHY_CFG 0x00
> +#define TH1520_ADC_PHY_CTRL 0x04
> +#define TH1520_ADC_PHY_TEST 0x08
> +#define TH1520_ADC_OP_CTRL 0x0C
> +#define TH1520_ADC_OP_SINGLE_START 0x10
> +#define TH1520_ADC_FCLK_CTRL 0x14
> +#define TH1520_ADC_START_TIME 0x18
> +#define TH1520_ADC_SAMPLE_TIME 0x1C
> +#define TH1520_ADC_SAMPLE_DATA 0x20
> +#define TH1520_ADC_INT_CTRL1 0x50
> +#define TH1520_ADC_INT_CTRL2 0x54
> +#define TH1520_ADC_INT_STATUS 0x58
> +#define TH1520_ADC_INT_ACTUAL_VALUE_CH0 0x60
> +#define TH1520_ADC_INT_ACTUAL_VALUE_CH1 0x64
> +#define TH1520_ADC_INT_DELTA_VALUE_CH0 0x90
> +#define TH1520_ADC_INT_DELTA_VALUE_CH1 0x94
> +
> +/* Configuration register field define */
> +#define TH1520_ADC_PHY_CFG_SELRES_6BIT (0x0)
> +#define TH1520_ADC_PHY_CFG_SELRES_8BIT (0x1)
> +#define TH1520_ADC_PHY_CFG_SELRES_10BIT (0x2)
> +#define TH1520_ADC_PHY_CFG_SELRES_12BIT (0x3)
> +#define TH1520_ADC_PHY_CFG_SELDIFF_SINGLE_ENDED_INPUTS (0x0 << 4)
> +#define TH1520_ADC_PHY_CFG_SELDIFF_DIFFERENTIAL_INPUTS (0x1 << 4)
> +#define TH1520_ADC_PHY_CFG_SELBG_INTERNAL (0x1 << 8)
> +#define TH1520_ADC_PHY_CFG_SELBG_EXTERNAL (0x0 << 8)
> +#define TH1520_ADC_PHY_CFG_SELREF_INTERNAL (0x1 << 12)
> +#define TH1520_ADC_PHY_CFG_SELREF_EXT (0x0 << 12)
> +
> +/* PHY CTRL register field define */
> +#define TH1520_ADC_PHY_CTRL_ENOFFSET_EN (0x1 << 12)
BIT()
> +#define TH1520_ADC_PHY_CTRL_ENOFFMEAS_EN (0x1 << 8)
> +#define TH1520_ADC_PHY_CTRL_RST_EN (0x1 << 4)
> +#define TH1520_ADC_PHY_CTRL_ENADC_EN (0x1 << 0)
> +
> +/* ADC OP ctrl field define */
> +#define TH1520_ADC_OP_CTRL_CH_EN_ALL GENMASK(19, 12)
> +#define TH1520_ADC_OP_CTRL_CH_EN_0 (12)
> +#define TH1520_ADC_OP_CTRL_MODE_SINGLE (0x1 << 0)
> +#define TH1520_ADC_OP_CTRL_MODE_CONTINOUS (0x0 << 0)
> +
> +/* ADC OP single start */
> +#define TH1520_ADC_OP_SINGLE_START_EN BIT(0)
> +
> +/* ADC fclk ctrl */
> +#define TH1520_ADC_FCLK_CTRL_FCLLK_DIV GENMASK(6, 0)
> +#define TH1520_ADC_FCLK_CTRL_TYP_1M (0x10004)
> +#define TH1520_ADC_START_TIME_TYP_1M (0x160)
> +#define TH1520_ADC_SAMPLE_TIME_TYP_1M (0x10)
> +#define TH1520_ADC_SAMPLE_TIME_TYP_6BIT (8)
> +#define TH1520_ADC_SAMPLE_TIME_TYP_8BIT (10)
> +#define TH1520_ADC_SAMPLE_TIME_TYP_10BIT (12)
> +#define TH1520_ADC_SAMPLE_TIME_TYP_12BIT (14)
> +
> +/* ADC sample data */
> +#define TH1520_ADC_SAMPLE_DATA_CH1 GENMASK(27, 16)
> +#define TH1520_ADC_SAMPLE_DATA_CH1_OFF (16)
> +#define TH1520_ADC_SAMPLE_DATA_CH1_VLD BIT(31)
> +#define TH1520_ADC_SAMPLE_DATA_CH1_NUMBER GENMASK(30, 28)
> +#define TH1520_ADC_SAMPLE_DATA_CH1_NUMBER_OFF (28)
> +#define TH1520_ADC_SAMPLE_DATA_CH0 GENMASK(11, 0)
> +#define TH1520_ADC_SAMPLE_DATA_CH0_VLD BIT(15)
> +#define TH1520_ADC_SAMPLE_DATA_CH0_OFF (0)
> +#define TH1520_ADC_SAMPLE_DATA_CH0_NUMBER GENMASK(14, 12)
> +#define TH1520_ADC_SAMPLE_DATA_CH0_NUMBER_OFF (12)
> +
> +/* ADC INT Ctrl */
> +#define TH1520_ADC_INT_CTRL1_CH1_INT_MODE BIT(1)
> +#define TH1520_ADC_INT_CTRL1_CH0_INT_MODE BIT(0)
> +#define TH1520_ADC_INT_CTRL2_CH1_INT_MASK BIT(1)
> +#define TH1520_ADC_INT_CTRL2_CH0_INT_MASK BIT(0)
> +#define TH1520_ADC_INT_STS_CH1_INT_STS BIT(1)
> +#define TH1520_ADC_INT_STS_CH0_INT_STS BIT(0)
> +
> +#define TH1520_ADC_ACTUAL_VALUE_CH0_HVAL GENMASK(27, 16)
> +#define TH1520_ADC_ACTUAL_VALUE_CH0_HVAL_OFF (16)
> +#define TH1520_ADC_ACTUAL_VALUE_CH0_LVAL GENMASK(11, 0)
> +#define TH1520_ADC_ACTUAL_VALUE_CH0_LVAL_OFF (0)
> +#define TH1520_ADC_ACTUAL_VALUE_CH1_HVAL GENMASK(27, 16)
> +#define TH1520_ADC_ACTUAL_VALUE_CH1_HVAL_OFF (16)
> +#define TH1520_ADC_ACTUAL_VALUE_CH1_LVAL GENMASK(11, 0)
> +#define TH1520_ADC_ACTUAL_VALUE_CH1_LVAL_OFF (0)
> +
> +#define TH1520_ADC_DLT_VALUE_CH0_HVAL GENMASK(27, 16)
> +#define TH1520_ADC_DLT_VALUE_CH0_HVAL_OFF (16)
> +#define TH1520_ADC_DLT_VALUE_CH0_LVAL GENMASK(11, 0)
> +#define TH1520_ADC_DLT_VALUE_CH0_LVAL_OFF (0)
> +#define TH1520_ADC_DLT_VALUE_CH1_HVAL GENMASK(27, 16)
> +#define TH1520_ADC_DLT_VALUE_CH1_HVAL_OFF (16)
> +#define TH1520_ADC_DLT_VALUE_CH1_LVAL GENMASK(11, 0)
> +#define TH1520_ADC_DLT_VALUE_CH1_LVAL_OFF (0)
Don't specify offsets. Use FIELD_GET() and FIELD_PREP() for all accesses
as those work with the mask alone. Also, make it clear what is a mask via
naming. Typically _MSK or similar.
> +
> +#define TH1520_ADC_FIFO_DATA_SIZE 32
> +#define TH1520_ADC_PHY_ENCTR 0x8e0
> +#define TH1520_ADC_TIMEOUT 500000
> +
> +#define TH1520_ADC_CHAN(_idx, _chan_type) { \
> + .type = (_chan_type), \
> + .indexed = 1, \
> + .channel = (_idx), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +}
> +
> +enum vol_ref {
> + TH1520_ADC_VOL_VREF,
> + TH1520_ADC_VOL_INTE,
> +};
> +
> +enum input_mode_sel {
> + TH1520_ADC_SINGLE_ENDED_INPUTS,
> + TH1520_ADC_DIFFERENTIAL_INPUTS,
> +};
> +
> +enum selres_sel {
> + TH1520_ADC_SELRES_6BIT = 6,
These seem unnecessary given the numeric values make sense without the
enum.
> + TH1520_ADC_SELRES_8BIT = 8,
> + TH1520_ADC_SELRES_10BIT = 10,
> + TH1520_ADC_SELRES_12BIT = 12,
> +};
> +
> +enum offset_mode_sel {
> + TH1520_ADC_OFFSET_DIS = 0,
> + TH1520_ADC_OFFSET_EN,
> +};
> +
> +enum conversion_mode_sel {
> + TH1520_ADC_MODE_SINGLE,
> + TH1520_ADC_MODE_CONTINOUS,
> +};
> +
> +enum clk_sel {
> + TH1520_ADC_FCLK_TYP_1M,
> +};
> +
> +enum int_actual_mask {
> + TH1520_ADC_ACTUAL_CH0,
> + TH1520_ADC_ACTUAL_CH1,
> + TH1520_ADC_ACTUAL_ALL,
> +
Excess blank line.
> +};
> +
> +enum int_delta_mask {
> + TH1520_ADC_DETAL_CH0,
> + TH1520_ADC_DETAL_CH1,
> + TH1520_ADC_DETAL_ALL,
> +};
> +
> +struct th1520_adc_feature {
> + enum selres_sel selres_sel;
> + enum input_mode_sel input_mode;
> + enum vol_ref vol_ref;
> + enum offset_mode_sel offset_mode;
> + enum conversion_mode_sel conv_mode;
> + enum clk_sel clk_sel;
> + enum int_actual_mask int_actual;
> + enum int_delta_mask int_detal;
> +};
> +
> +struct th1520_adc {
> + struct device *dev;
> + void __iomem *regs;
> + struct clk *clk;
> +
> + u32 vref_uv;
> + u32 value;
> + struct regulator *vref;
> + struct th1520_adc_feature adc_feature;
> + u32 current_clk;
> + u32 ch0_offmeas;
> + u32 ch1_offmeas;
As above - don't introduce this stuff until it is used by features
in the driver. Just hard code the defaults when you write the registers.
Then when control is enabled in a future patch, it's much more obvious
what has actually changed.
> +
> + struct completion completion;
> + /* lock to protect against multiple access to the device */
More info please on why this is a problem. Do we have read modify write
cycles, or sequences where we need to not change the config whilst
an ADC read is happening?
> + struct mutex mlock;
> +};
> +
^ permalink raw reply
* Re: [PATCH v2 1/3] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop
From: Krzysztof Kozlowski @ 2024-03-30 18:20 UTC (permalink / raw)
To: Marc Gonzalez, Kalle Valo, Jeff Johnson, ath10k
Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Konrad Dybcio,
Jami Kettunen, Jeffrey Hugo
In-Reply-To: <84f20fb5-5d48-419c-8eff-d7044afb81c0@freebox.fr>
On 28/03/2024 18:36, Marc Gonzalez wrote:
> The ath10k driver waits for an "MSA_READY" indicator
> to complete initialization. If the indicator is not
> received, then the device remains unusable.
>
> cf. ath10k_qmi_driver_event_work()
>
> Several msm8998-based devices are affected by this issue.
> Oddly, it seems safe to NOT wait for the indicator, and
> proceed immediately when QMI_EVENT_SERVER_ARRIVE.
>
This is v2, so where is the changelog?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/3] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop
From: Krzysztof Kozlowski @ 2024-03-30 18:23 UTC (permalink / raw)
To: Marc Gonzalez, Kalle Valo, Jeff Johnson, ath10k
Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Konrad Dybcio,
Jami Kettunen, Jeffrey Hugo
In-Reply-To: <72c162cc-45e0-48b6-8d90-d59fac299375@linaro.org>
On 30/03/2024 19:20, Krzysztof Kozlowski wrote:
> On 28/03/2024 18:36, Marc Gonzalez wrote:
>> The ath10k driver waits for an "MSA_READY" indicator
>> to complete initialization. If the indicator is not
>> received, then the device remains unusable.
>>
>> cf. ath10k_qmi_driver_event_work()
>>
>> Several msm8998-based devices are affected by this issue.
>> Oddly, it seems safe to NOT wait for the indicator, and
>> proceed immediately when QMI_EVENT_SERVER_ARRIVE.
>>
>
> This is v2, so where is the changelog?
Expecting reviewer to dig previous discussions will not help your case.
It helps reviewers if you provide necessary information, like resolution
of previous discussion in the changelog.
I dig the previous discussion, since you did not mention it here, and it
seems you entirely ignored its outcome. That's not a DT property.
NAK, sorry. Please go back to v1 and read the comments you got there.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi
From: Krzysztof Kozlowski @ 2024-03-30 18:25 UTC (permalink / raw)
To: Marc Gonzalez, Kalle Valo, Jeff Johnson, ath10k
Cc: wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson, Konrad Dybcio,
Jami Kettunen, Jeffrey Hugo
In-Reply-To: <5cdad89c-282a-4df5-a286-b8404bc4dd81@freebox.fr>
On 28/03/2024 18:39, Marc Gonzalez wrote:
> The ath10k driver waits for an "MSA_READY" indicator
> to complete initialization. If the indicator is not
> received, then the device remains unusable.
>
> cf. ath10k_qmi_driver_event_work()
>
> Several msm8998-based devices are affected by this issue.
> Oddly, it seems safe to NOT wait for the indicator, and
> proceed immediately when QMI_EVENT_SERVER_ARRIVE.
>
> Jeff Johnson wrote:
>
> The feedback I received was "it might be ok to change all ath10k qmi
> to skip waiting for msa_ready", and it was pointed out that ath11k
> (and ath12k) do not wait for it.
>
> However with so many deployed devices, "might be ok" isn't a strong
> argument for changing the default behavior.
>
I think you got pretty clear comments:
"This sounds more like a firmware feature, not a hardware feature."
"This is why having this property in DT does not look right
place for this."
Best regards,
Krzysztof
^ 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