Re: [PATCH v12 12/13] media: imagination: Round to closest multiple for cropping region
On Thu, Jun 06, 2024 at 01:44:59PM +0200, Sebastian Fricke wrote: > Hey, > > On 04.06.2024 16:23, Devarsh Thakkar wrote: > > If neither of the flags to round down (V4L2_SEL_FLAG_LE) or round up > > (V4L2_SEL_FLAG_GE) are specified by the user, then round to nearest > > multiple of requested value while updating the crop rectangle coordinates. > > > > Use the rounding macro which gives preference to rounding down in case two > > nearest values (high and low) are possible to raise the probability of > > cropping rectangle falling inside the bound region. > > > > This complies with the VIDIOC_G_SELECTION, VIDIOC_S_SELECTION ioctl > > description as documented in v4l uapi [1] which specifies that driver > > should choose crop rectangle as close as possible if no flags are passed by > > user-space, as quoted below : > > > > "``0`` - The driver can adjust the rectangle size freely and shall choose a > > crop/compose rectangle as close as possible to the requested > > one." > > > > Link: > > https://www.kernel.org/doc/Documentation/userspace-api/media/v4l/vidioc-g-selection.rst > > [1] > > Signed-off-by: Devarsh Thakkar > > Acked-by: Sebastian Fricke > > Can, whoever picks up the math changes, pick up this change as well? > I will send 1-6 via the media subsystem. math.h is orphaned, meaning any Tier-1 maintainer may push this through. So, there is nobody behind it. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] drm/mipi-dbi: Add missing MODULE_DESCRIPTION()
On Thu, Apr 25, 2024 at 03:56:26PM +0300, Andy Shevchenko wrote: > The modpost script is not happy > > WARNING: modpost: missing MODULE_DESCRIPTION() in > drivers/gpu/drm/drm_mipi_dbi.o > > because there is a missing module description. > > Add it to the module. Any comments on this? -- With Best Regards, Andy Shevchenko
[PATCH v1 1/1] treewide: Align match_string() with sysfs_match_string()
Make two APIs look similar. Hence convert match_string() to be
a 2-argument macro. In order to avoid unneeded churn, convert
all users as well. There is no functional change intended.
Signed-off-by: Andy Shevchenko
---
Compile tested with `make allyesconfig` and `make allmodconfig`
on x86_64, arm, aarch64, powerpc64 (8 builds total).
I guess the best is to apply it to Linus' tree directly.
And now it seems a good timing as there are no new users
of this API either in v6.10-rcX, or in Linux Next.
But if you think differently, tell me.
arch/powerpc/xmon/xmon.c | 5 ++--
arch/x86/kernel/cpu/mtrr/if.c | 4 +--
crypto/asymmetric_keys/pkcs7_verify.c | 4 +--
drivers/acpi/scan.c | 4 +--
drivers/ata/pata_hpt366.c | 2 +-
drivers/ata/pata_hpt37x.c | 2 +-
drivers/base/property.c | 6 ++--
drivers/char/ipmi/ipmi_msghandler.c | 2 +-
drivers/char/ipmi/ipmi_si_hardcode.c | 2 +-
drivers/clk/bcm/clk-bcm2835.c | 4 +--
drivers/clk/rockchip/clk.c| 4 +--
drivers/clk/tegra/clk-tegra124-emc.c | 7 ++---
drivers/cpufreq/amd-pstate.c | 4 +--
drivers/cpufreq/intel_pstate.c| 2 +-
.../intel/qat/qat_common/adf_cfg_services.c | 5 ++--
.../gpu/drm/drm_panel_orientation_quirks.c| 2 +-
drivers/gpu/drm/i915/display/intel_pipe_crc.c | 2 +-
drivers/gpu/drm/nouveau/dispnv04/tvnv17.c | 4 +--
drivers/gpu/drm/nouveau/dispnv50/crc.c| 2 +-
drivers/hwmon/ltc4282.c | 14 -
drivers/hwmon/nct6775-platform.c | 6 ++--
drivers/hwtracing/intel_th/msu.c | 2 +-
drivers/i2c/busses/i2c-i801.c | 4 +--
drivers/leds/leds-sun50i-a100.c | 2 +-
drivers/mfd/omap-usb-host.c | 2 +-
drivers/mmc/host/sdhci-xenon-phy.c| 13 -
drivers/mtd/nand/raw/nand_macronix.c | 10 ++-
.../net/ethernet/chelsio/cxgb4/cudbg_lib.c| 6 ++--
.../net/wireless/intel/iwlwifi/mvm/debugfs.c | 2 +-
drivers/pci/pcie/aer.c| 2 +-
drivers/phy/mediatek/phy-mtk-tphy.c | 8 ++---
drivers/phy/tegra/xusb.c | 4 +--
drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 6 ++--
drivers/pinctrl/pinmux.c | 6 ++--
drivers/platform/x86/hp/hp-wmi.c | 29 +++
drivers/platform/x86/msi-ec.c | 4 +--
drivers/power/supply/ab8500_btemp.c | 2 +-
drivers/power/supply/ab8500_chargalg.c| 2 +-
drivers/power/supply/ab8500_charger.c | 2 +-
drivers/power/supply/ab8500_fg.c | 2 +-
drivers/staging/gdm724x/gdm_tty.c | 5 ++--
.../int340x_thermal/processor_thermal_rfim.c | 4 +--
.../processor_thermal_wt_req.c| 2 +-
drivers/usb/common/common.c | 8 ++---
drivers/usb/dwc3/dwc3-rtk.c | 2 +-
drivers/usb/typec/class.c | 14 -
drivers/usb/typec/tipd/core.c | 3 +-
drivers/video/fbdev/pxafb.c | 4 +--
fs/bcachefs/compress.c| 2 +-
fs/bcachefs/opts.c| 4 +--
fs/bcachefs/util.c| 4 +--
fs/ubifs/auth.c | 8 ++---
include/linux/string.h| 12 +++-
kernel/cgroup/rdma.c | 2 +-
kernel/sched/debug.c | 2 +-
kernel/trace/trace.c | 4 +--
kernel/trace/trace_osnoise.c | 4 +--
lib/dynamic_debug.c | 5 ++--
lib/string_helpers.c | 6 ++--
mm/mempolicy.c| 4 +--
mm/vmpressure.c | 4 +--
security/apparmor/lsm.c | 9 +++---
security/integrity/ima/ima_main.c | 2 +-
security/integrity/ima/ima_policy.c | 2 +-
sound/firewire/oxfw/oxfw.c| 2 +-
sound/pci/oxygen/oxygen_mixer.c | 2 +-
sound/soc/codecs/max98088.c | 2 +-
sound/soc/codecs/max98095.c | 2 +-
sound/soc/soc-dapm.c | 5 ++--
69 files changed, 150 insertions(+), 174 deletions(-)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index bd4813bad317..f479cc62674a 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -3478,8 +3478,7 @@ skipbl(void)
return c;
}
-#define N_PTREGS 44
-static const char *regnames[N_PTREGS] = {
+static const char *regnames[] = {
"r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7",
"r8", "
Re: [PATCH v1 1/1] drm/amd/display: Fix too big frame size
On Sun, Jun 02, 2024 at 05:21:03PM +0300, Andy Shevchenko wrote:
> Compilation fails on arm with:
>
> link_factory.c:743:1: error: the frame size of 1032 bytes is larger than
> 1024 bytes [-Werror=frame-larger-than=]
>
> Fix the frame size by allocation one of the big structures.
Fixed even in better way in 0b6dc64b4e22 ("drm/amd/display: Refactor
construct_phy function in dc/link/link_factory.c").
--
With Best Regards,
Andy Shevchenko
[PATCH v1 1/1] drm/amd/display: Fix too big frame size
Compilation fails on arm with:
link_factory.c:743:1: error: the frame size of 1032 bytes is larger than 1024
bytes [-Werror=frame-larger-than=]
Fix the frame size by allocation one of the big structures.
Signed-off-by: Andy Shevchenko
---
.../gpu/drm/amd/display/dc/link/link_factory.c| 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/link/link_factory.c
b/drivers/gpu/drm/amd/display/dc/link/link_factory.c
index cf22b8f28ba6..78f1b2102839 100644
--- a/drivers/gpu/drm/amd/display/dc/link/link_factory.c
+++ b/drivers/gpu/drm/amd/display/dc/link/link_factory.c
@@ -456,10 +456,10 @@ static bool construct_phy(struct dc_link *link,
struct dc_context *dc_ctx = init_params->ctx;
struct encoder_init_data enc_init_data = { 0 };
struct panel_cntl_init_data panel_cntl_init_data = { 0 };
- struct integrated_info info = { 0 };
struct dc_bios *bios = init_params->dc->ctx->dc_bios;
const struct dc_vbios_funcs *bp_funcs = bios->funcs;
struct bp_disp_connector_caps_info disp_connect_caps_info = { 0 };
+ struct integrated_info *info;
DC_LOGGER_INIT(dc_ctx->logger);
@@ -672,12 +672,16 @@ static bool construct_phy(struct dc_link *link,
}
if (bios->integrated_info)
- info = *bios->integrated_info;
+ info = kmemdup(bios->integrated_info, sizeof(*info),
GFP_KERNEL);
+ else
+ info = kzalloc(sizeof(*info), GFP_KERNEL);
+ if (!info)
+ goto device_tag_fail;
/* Look for channel mapping corresponding to connector and device tag */
for (i = 0; i < MAX_NUMBER_OF_EXT_DISPLAY_PATH; i++) {
struct external_display_path *path =
- _disp_conn_info.path[i];
+ >ext_disp_conn_info.path[i];
if (path->device_connector_id.enum_id == link->link_id.enum_id
&&
path->device_connector_id.id == link->link_id.id &&
@@ -698,14 +702,15 @@ static bool construct_phy(struct dc_link *link,
if (link->chip_caps &
EXT_DISPLAY_PATH_CAPS__DP_FIXED_VS_EN) {
link->bios_forced_drive_settings.VOLTAGE_SWING =
-
(info.ext_disp_conn_info.fixdpvoltageswing & 0x3);
+
info->ext_disp_conn_info.fixdpvoltageswing & 0x3;
link->bios_forced_drive_settings.PRE_EMPHASIS =
-
((info.ext_disp_conn_info.fixdpvoltageswing >> 2) & 0x3);
+
(info->ext_disp_conn_info.fixdpvoltageswing >> 2) & 0x3;
}
break;
}
}
+ kfree(info);
if (bios->funcs->get_atom_dc_golden_table)
bios->funcs->get_atom_dc_golden_table(bios);
--
2.43.0.rc1.1336.g36b5255a03ac
Re: [PATCH v1 2/4] iio: light: lm3533-als: Remove the driver
On Sat, Jun 01, 2024 at 02:05:08PM +0100, Jonathan Cameron wrote: > On Fri, 31 May 2024 19:56:14 +0300 > Andy Shevchenko wrote: > > > The driver has no in kernel users and requires a board file > > to be instantiated. Remove basically a dead code. > > > > If ever needed, it can be reinstantiated and converted to one > > that uses firmware node interfaces. > > > > Signed-off-by: Andy Shevchenko > Given the header removal in patch 4, I assume these all need to go together > via mfd. > > Acked-by: Jonathan Cameron Thank you! We are waiting for Johan to tell what his plans about the driver. If it comes to removal, I add your tag to the new version of this mini-series. -- With Best Regards, Andy Shevchenko
Re: [PATCH] kernel/resource: optimize find_next_iomem_res
On Fri, May 31, 2024 at 02:31:45PM -0700, Chia-I Wu wrote: > On Fri, May 31, 2024 at 1:57 AM Andy Shevchenko < > andriy.shevche...@linux.intel.com> wrote: > > On Thu, May 30, 2024 at 10:36:57PM -0700, Chia-I Wu wrote: ... > > P.S> I'm not so sure about this change. It needs a thoroughly testing, esp. > > in PCI case. Cc'ing to Ilpo. > What's special about PCI? PCI, due to its nature, may rebuild resources either by shrinking or expanding of the entire subtree after the PCI bridge in question. And this may happen at run-time due to hotplug support. But I'm not a deep expert in this area, Ilpo knows much more than me. -- With Best Regards, Andy Shevchenko
Re: [PATCH v11 09/11] lib: math_kunit: Add tests for new macros related to rounding to nearest value
On Fri, May 31, 2024 at 10:46:28PM +0530, Devarsh Thakkar wrote: > Add tests for round_closest_up/down and roundclosest macros which round > to nearest multiple of specified argument. These are tested with kunit > tool as shared here [1]. > > [1]: https://gist.github.com/devarsht/3f9042825be3da4e133b8f4eda067876 Make it a tag... > ...here Link: ... [1] > Signed-off-by: Devarsh Thakkar With the same caveat as per patch 1, Acked-by: Andy Shevchenko -- With Best Regards, Andy Shevchenko
Re: [PATCH v11 06/11] math.h: Add macros for rounding to closest value
On Fri, May 31, 2024 at 10:41:36PM +0530, Devarsh Thakkar wrote: > Add below rounding related macros: > > round_closest_up(x, y) : Rounds x to closest multiple of y where y is a > power of 2, with a preference to round up in case two nearest values are > possible. > > round_closest_down(x, y) : Rounds x to closest multiple of y where y is a > power of 2, with a preference to round down in case two nearest values are > possible. > > roundclosest(x, y) : Rounds x to closest multiple of y, this macro should > generally be used only when y is not multiple of 2 as otherwise > round_closest* macros should be used which are much faster. > > Examples: > * round_closest_up(17, 4) = 16 > * round_closest_up(15, 4) = 16 > * round_closest_up(14, 4) = 16 > * round_closest_down(17, 4) = 16 > * round_closest_down(15, 4) = 16 > * round_closest_down(14, 4) = 12 > * roundclosest(21, 5) = 20 > * roundclosest(19, 5) = 20 > * roundclosest(17, 5) = 15 I don't know the estimation on how these will really useful or not, but I'm not objecting if people think it's needed API. Acked-by: Andy Shevchenko -- With Best Regards, Andy Shevchenko
Re: [PATCH v11 07/11] Documentation: core-api: Add math.h macros and functions
On Fri, May 31, 2024 at 10:42:20PM +0530, Devarsh Thakkar wrote: > Add documentation for rounding, scaling, absolute value and difference, > 32-bit division related macros and functions exported by math.h header > file. As long as it renders correctly, fine to me Reviewed-by: Andy Shevchenko -- With Best Regards, Andy Shevchenko
Re: [PATCH v11 08/11] lib: add basic KUnit test for lib/math
On Fri, May 31, 2024 at 10:43:05PM +0530, Devarsh Thakkar wrote: > From: Daniel Latypov > > Add basic test coverage for files that don't require any config options: > * part of math.h (what seem to be the most commonly used macros) > * gcd.c > * lcm.c > * int_sqrt.c > * reciprocal_div.c > (Ignored int_pow.c since it's a simple textbook algorithm.) > > These tests aren't particularly interesting, but they > * provide short and simple examples of parameterized tests > * provide a place to add tests for any new files in this dir > * are written so adding new test cases to cover edge cases should be > easy > * looking at code coverage, we hit all the branches in the .c files Reviewed-by: Andy Shevchenko -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 0/4] lm3533: Remove the outdated drivers
On Fri, May 31, 2024 at 06:15:46PM +0100, Lee Jones wrote: > On Fri, 31 May 2024, Andy Shevchenko wrote: > > On Fri, May 31, 2024 at 07:56:12PM +0300, Andy Shevchenko wrote: > > > Driver is quite outdated from the Linux kernel internal APIs > > > perspective. In particular GPIO code is using legacy calls, > > > that started being replaced by a new API ca. 2014, i.e. ten > > > years ago. > > > > > > Suggested-by: Linus Walleij > > > > > drivers/mfd/lm3533-core.c | 645 --- > > > > Oops, still leftovers: one file and Kconfig/Makefile updates... > > If needed I'll send a v2, but now I leave it to Lee and Johan to decide > > the destiny of the drivers. > > Let's not rush into it. Take your time. Exactly, excellente fin de semaine! -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 0/4] lm3533: Remove the outdated drivers
+Cc: Johan (via kernel.org) On Fri, May 31, 2024 at 08:14:43PM +0300, Andy Shevchenko wrote: > On Fri, May 31, 2024 at 07:56:12PM +0300, Andy Shevchenko wrote: > > Driver is quite outdated from the Linux kernel internal APIs > > perspective. In particular GPIO code is using legacy calls, > > that started being replaced by a new API ca. 2014, i.e. ten > > years ago. > > > > Suggested-by: Linus Walleij > > > drivers/mfd/lm3533-core.c | 645 --- > > Oops, still leftovers: one file and Kconfig/Makefile updates... > If needed I'll send a v2, but now I leave it to Lee and Johan to decide > the destiny of the drivers. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 0/4] lm3533: Remove the outdated drivers
On Fri, May 31, 2024 at 06:14:25PM +0100, Lee Jones wrote: > Making sure Johan is aware of this with his new address. Right, in any case this is not the final version (a couple of leftovers). I have mentioned this series in the original thread. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 0/4] lm3533: Remove the outdated drivers
On Fri, May 31, 2024 at 07:56:12PM +0300, Andy Shevchenko wrote: > Driver is quite outdated from the Linux kernel internal APIs > perspective. In particular GPIO code is using legacy calls, > that started being replaced by a new API ca. 2014, i.e. ten > years ago. > > Suggested-by: Linus Walleij > drivers/mfd/lm3533-core.c | 645 --- Oops, still leftovers: one file and Kconfig/Makefile updates... If needed I'll send a v2, but now I leave it to Lee and Johan to decide the destiny of the drivers. -- With Best Regards, Andy Shevchenko
[PATCH v1 3/4] leds: lm3533: Remove the driver
The driver has no in kernel users and requires a board file
to be instantiated. Remove basically a dead code.
If ever needed, it can be reinstantiated and converted to one
that uses firmware node interfaces.
Signed-off-by: Andy Shevchenko
---
drivers/leds/Kconfig | 13 -
drivers/leds/Makefile | 1 -
drivers/leds/leds-lm3533.c | 755 -
3 files changed, 769 deletions(-)
delete mode 100644 drivers/leds/leds-lm3533.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 05e6af88b88c..4cdc3a687421 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -229,19 +229,6 @@ config LEDS_LM3532
controlled manually or using PWM input or using ambient
light automatically.
-config LEDS_LM3533
- tristate "LED support for LM3533"
- depends on LEDS_CLASS
- depends on MFD_LM3533
- help
- This option enables support for the LEDs on National Semiconductor /
- TI LM3533 Lighting Power chips.
-
- The LEDs can be controlled directly, through PWM input, or by the
- ambient-light-sensor interface. The chip supports
- hardware-accelerated blinking with maximum on and off periods of 9.8
- and 77 seconds respectively.
-
config LEDS_LM3642
tristate "LED support for LM3642 Chip"
depends on LEDS_CLASS && I2C
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index effdfc6f1e95..6bc8c412d3ac 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -38,7 +38,6 @@ obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o
obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o
obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o
obj-$(CONFIG_LEDS_LM3532) += leds-lm3532.o
-obj-$(CONFIG_LEDS_LM3533) += leds-lm3533.o
obj-$(CONFIG_LEDS_LM355x) += leds-lm355x.o
obj-$(CONFIG_LEDS_LM36274) += leds-lm36274.o
obj-$(CONFIG_LEDS_LM3642) += leds-lm3642.o
diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
deleted file mode 100644
index a3d33165d262..
--- a/drivers/leds/leds-lm3533.c
+++ /dev/null
@@ -1,755 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * leds-lm3533.c -- LM3533 LED driver
- *
- * Copyright (C) 2011-2012 Texas Instruments
- *
- * Author: Johan Hovold
- */
-
-#include
-#include
-#include
-#include
-#include
-#include
-
-#include
-
-
-#define LM3533_LVCTRLBANK_MIN 2
-#define LM3533_LVCTRLBANK_MAX 5
-#define LM3533_LVCTRLBANK_COUNT4
-#define LM3533_RISEFALLTIME_MAX7
-#define LM3533_ALS_CHANNEL_LV_MIN 1
-#define LM3533_ALS_CHANNEL_LV_MAX 2
-
-#define LM3533_REG_CTRLBANK_BCONF_BASE 0x1b
-#define LM3533_REG_PATTERN_ENABLE 0x28
-#define LM3533_REG_PATTERN_LOW_TIME_BASE 0x71
-#define LM3533_REG_PATTERN_HIGH_TIME_BASE 0x72
-#define LM3533_REG_PATTERN_RISETIME_BASE 0x74
-#define LM3533_REG_PATTERN_FALLTIME_BASE 0x75
-
-#define LM3533_REG_PATTERN_STEP0x10
-
-#define LM3533_REG_CTRLBANK_BCONF_MAPPING_MASK 0x04
-#define LM3533_REG_CTRLBANK_BCONF_ALS_EN_MASK 0x02
-#define LM3533_REG_CTRLBANK_BCONF_ALS_CHANNEL_MASK 0x01
-
-#define LM3533_LED_FLAG_PATTERN_ENABLE 1
-
-
-struct lm3533_led {
- struct lm3533 *lm3533;
- struct lm3533_ctrlbank cb;
- struct led_classdev cdev;
- int id;
-
- struct mutex mutex;
- unsigned long flags;
-};
-
-
-static inline struct lm3533_led *to_lm3533_led(struct led_classdev *cdev)
-{
- return container_of(cdev, struct lm3533_led, cdev);
-}
-
-static inline int lm3533_led_get_ctrlbank_id(struct lm3533_led *led)
-{
- return led->id + 2;
-}
-
-static inline u8 lm3533_led_get_lv_reg(struct lm3533_led *led, u8 base)
-{
- return base + led->id;
-}
-
-static inline u8 lm3533_led_get_pattern(struct lm3533_led *led)
-{
- return led->id;
-}
-
-static inline u8 lm3533_led_get_pattern_reg(struct lm3533_led *led,
- u8 base)
-{
- return base + lm3533_led_get_pattern(led) * LM3533_REG_PATTERN_STEP;
-}
-
-static int lm3533_led_pattern_enable(struct lm3533_led *led, int enable)
-{
- u8 mask;
- u8 val;
- int pattern;
- int state;
- int ret = 0;
-
- dev_dbg(led->cdev.dev, "%s - %d\n", __func__, enable);
-
- mutex_lock(>mutex);
-
- state = test_bit(LM3533_LED_FLAG_PATTERN_ENABLE, >flags);
- if ((enable && state) || (!enable && !state))
- goto out;
-
- pattern = lm3533_led_get_pattern(led);
- mask = 1 << (2 * pattern);
-
- if (enable)
- val = mask;
- else
- val = 0;
-
- ret = lm3533_update(led->lm3533, LM3533_REG_PATTERN_ENABLE, val, m
[PATCH v1 0/4] lm3533: Remove the outdated drivers
Driver is quite outdated from the Linux kernel internal APIs perspective. In particular GPIO code is using legacy calls, that started being replaced by a new API ca. 2014, i.e. ten years ago. Suggested-by: Linus Walleij Andy Shevchenko (4): backlight: lm3533_bl: Remove the driver iio: light: lm3533-als: Remove the driver leds: lm3533: Remove the driver mfd: lm3533: Remove the driver drivers/iio/light/Kconfig | 17 - drivers/iio/light/Makefile | 1 - drivers/iio/light/lm3533-als.c | 922 drivers/leds/Kconfig| 13 - drivers/leds/Makefile | 1 - drivers/leds/leds-lm3533.c | 755 --- drivers/mfd/lm3533-core.c | 645 --- drivers/video/backlight/Kconfig | 11 - drivers/video/backlight/Makefile| 1 - drivers/video/backlight/lm3533_bl.c | 399 include/linux/mfd/lm3533.h | 100 --- 11 files changed, 2865 deletions(-) delete mode 100644 drivers/iio/light/lm3533-als.c delete mode 100644 drivers/leds/leds-lm3533.c delete mode 100644 drivers/mfd/lm3533-core.c delete mode 100644 drivers/video/backlight/lm3533_bl.c delete mode 100644 include/linux/mfd/lm3533.h -- 2.43.0.rc1.1336.g36b5255a03ac
[PATCH v1 1/4] backlight: lm3533_bl: Remove the driver
The driver has no in kernel users and requires a board file
to be instantiated. Remove basically a dead code.
If ever needed, it can be reinstantiated and converted to one
that uses firmware node interfaces.
Signed-off-by: Andy Shevchenko
---
drivers/video/backlight/Kconfig | 11 -
drivers/video/backlight/Makefile| 1 -
drivers/video/backlight/lm3533_bl.c | 399
3 files changed, 411 deletions(-)
delete mode 100644 drivers/video/backlight/lm3533_bl.c
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 230bca07b09d..91d6618d69a0 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -198,17 +198,6 @@ config BACKLIGHT_KTZ8866
Say Y to enable the backlight driver for the Kinetic KTZ8866
found in Xiaomi Mi Pad 5 series.
-config BACKLIGHT_LM3533
- tristate "Backlight Driver for LM3533"
- depends on MFD_LM3533
- help
- Say Y to enable the backlight driver for National Semiconductor / TI
- LM3533 Lighting Power chips.
-
- The backlights can be controlled directly, through PWM input, or by
- the ambient-light-sensor interface. The chip supports 256 brightness
- levels.
-
config BACKLIGHT_LOCOMO
tristate "Sharp LOCOMO LCD/Backlight Driver"
depends on SHARP_LOCOMO
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index 8d2cb252042d..fc75b9f059f9 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -36,7 +36,6 @@ 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_LM3533) += lm3533_bl.o
obj-$(CONFIG_BACKLIGHT_LM3630A)+= lm3630a_bl.o
obj-$(CONFIG_BACKLIGHT_LM3639) += lm3639_bl.o
obj-$(CONFIG_BACKLIGHT_LOCOMO) += locomolcd.o
diff --git a/drivers/video/backlight/lm3533_bl.c
b/drivers/video/backlight/lm3533_bl.c
deleted file mode 100644
index 3e10d480cb7f..
--- a/drivers/video/backlight/lm3533_bl.c
+++ /dev/null
@@ -1,399 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * lm3533-bl.c -- LM3533 Backlight driver
- *
- * Copyright (C) 2011-2012 Texas Instruments
- *
- * Author: Johan Hovold
- */
-
-#include
-#include
-#include
-#include
-#include
-#include
-
-#include
-
-
-#define LM3533_HVCTRLBANK_COUNT2
-#define LM3533_BL_MAX_BRIGHTNESS 255
-
-#define LM3533_REG_CTRLBANK_AB_BCONF 0x1a
-
-
-struct lm3533_bl {
- struct lm3533 *lm3533;
- struct lm3533_ctrlbank cb;
- struct backlight_device *bd;
- int id;
-};
-
-
-static inline int lm3533_bl_get_ctrlbank_id(struct lm3533_bl *bl)
-{
- return bl->id;
-}
-
-static int lm3533_bl_update_status(struct backlight_device *bd)
-{
- struct lm3533_bl *bl = bl_get_data(bd);
-
- return lm3533_ctrlbank_set_brightness(>cb,
backlight_get_brightness(bd));
-}
-
-static int lm3533_bl_get_brightness(struct backlight_device *bd)
-{
- struct lm3533_bl *bl = bl_get_data(bd);
- u8 val;
- int ret;
-
- ret = lm3533_ctrlbank_get_brightness(>cb, );
- if (ret)
- return ret;
-
- return val;
-}
-
-static const struct backlight_ops lm3533_bl_ops = {
- .get_brightness = lm3533_bl_get_brightness,
- .update_status = lm3533_bl_update_status,
-};
-
-static ssize_t show_id(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct lm3533_bl *bl = dev_get_drvdata(dev);
-
- return scnprintf(buf, PAGE_SIZE, "%d\n", bl->id);
-}
-
-static ssize_t show_als_channel(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct lm3533_bl *bl = dev_get_drvdata(dev);
- unsigned channel = lm3533_bl_get_ctrlbank_id(bl);
-
- return scnprintf(buf, PAGE_SIZE, "%u\n", channel);
-}
-
-static ssize_t show_als_en(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct lm3533_bl *bl = dev_get_drvdata(dev);
- int ctrlbank = lm3533_bl_get_ctrlbank_id(bl);
- u8 val;
- u8 mask;
- bool enable;
- int ret;
-
- ret = lm3533_read(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF, );
- if (ret)
- return ret;
-
- mask = 1 << (2 * ctrlbank);
- enable = val & mask;
-
- return scnprintf(buf, PAGE_SIZE, "%d\n", enable);
-}
-
-static ssize_t store_als_en(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t len)
-{
- struct lm3533_bl *
[PATCH v1 2/4] iio: light: lm3533-als: Remove the driver
The driver has no in kernel users and requires a board file
to be instantiated. Remove basically a dead code.
If ever needed, it can be reinstantiated and converted to one
that uses firmware node interfaces.
Signed-off-by: Andy Shevchenko
---
drivers/iio/light/Kconfig | 17 -
drivers/iio/light/Makefile | 1 -
drivers/iio/light/lm3533-als.c | 922 -
3 files changed, 940 deletions(-)
delete mode 100644 drivers/iio/light/lm3533-als.c
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 9a587d403118..827eee527a62 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -358,23 +358,6 @@ config RPR0521
To compile this driver as a module, choose M here:
the module will be called rpr0521.
-config SENSORS_LM3533
- tristate "LM3533 ambient light sensor"
- depends on MFD_LM3533
- help
- If you say yes here you get support for the ambient light sensor
- interface on National Semiconductor / TI LM3533 Lighting Power
- chips.
-
- The sensor interface can be used to control the LEDs and backlights
- of the chip through defining five light zones and three sets of
- corresponding output-current values.
-
- The driver provides raw and mean adc readings along with the current
- light zone through sysfs. A threshold event can be generated on zone
- changes. The ALS-control output values can be set per zone for the
- three current output channels.
-
config LTR390
tristate "LTR-390UV-01 ambient light and UV sensor"
depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index a30f906e91ba..6fd7b6f95d1d 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -31,7 +31,6 @@ obj-$(CONFIG_SENSORS_ISL29028)+= isl29028.o
obj-$(CONFIG_ISL29125) += isl29125.o
obj-$(CONFIG_ISL76682) += isl76682.o
obj-$(CONFIG_JSA1212) += jsa1212.o
-obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o
obj-$(CONFIG_LTR390) += ltr390.o
obj-$(CONFIG_LTR501) += ltr501.o
obj-$(CONFIG_LTRF216A) += ltrf216a.o
diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
deleted file mode 100644
index 7800f7fa51b7..
--- a/drivers/iio/light/lm3533-als.c
+++ /dev/null
@@ -1,922 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * lm3533-als.c -- LM3533 Ambient Light Sensor driver
- *
- * Copyright (C) 2011-2012 Texas Instruments
- *
- * Author: Johan Hovold
- */
-
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-
-#include
-
-
-#define LM3533_ALS_RESISTOR_MIN1
-#define LM3533_ALS_RESISTOR_MAX127
-#define LM3533_ALS_CHANNEL_CURRENT_MAX 2
-#define LM3533_ALS_THRESH_MAX 3
-#define LM3533_ALS_ZONE_MAX4
-
-#define LM3533_REG_ALS_RESISTOR_SELECT 0x30
-#define LM3533_REG_ALS_CONF0x31
-#define LM3533_REG_ALS_ZONE_INFO 0x34
-#define LM3533_REG_ALS_READ_ADC_RAW0x37
-#define LM3533_REG_ALS_READ_ADC_AVERAGE0x38
-#define LM3533_REG_ALS_BOUNDARY_BASE 0x50
-#define LM3533_REG_ALS_TARGET_BASE 0x60
-
-#define LM3533_ALS_ENABLE_MASK 0x01
-#define LM3533_ALS_INPUT_MODE_MASK 0x02
-#define LM3533_ALS_INT_ENABLE_MASK 0x01
-
-#define LM3533_ALS_ZONE_SHIFT 2
-#define LM3533_ALS_ZONE_MASK 0x1c
-
-#define LM3533_ALS_FLAG_INT_ENABLED1
-
-
-struct lm3533_als {
- struct lm3533 *lm3533;
- struct platform_device *pdev;
-
- unsigned long flags;
- int irq;
-
- atomic_t zone;
- struct mutex thresh_mutex;
-};
-
-
-static int lm3533_als_get_adc(struct iio_dev *indio_dev, bool average,
- int *adc)
-{
- struct lm3533_als *als = iio_priv(indio_dev);
- u8 reg;
- u8 val;
- int ret;
-
- if (average)
- reg = LM3533_REG_ALS_READ_ADC_AVERAGE;
- else
- reg = LM3533_REG_ALS_READ_ADC_RAW;
-
- ret = lm3533_read(als->lm3533, reg, );
- if (ret) {
- dev_err(_dev->dev, "failed to read adc\n");
- return ret;
- }
-
- *adc = val;
-
- return 0;
-}
-
-static int _lm3533_als_get_zone(struct iio_dev *indio_dev, u8 *zone)
-{
- struct lm3533_als *als = iio_priv(indio_dev);
- u8 val;
- int ret;
-
- ret = lm3533_read(als->lm3533, LM3533_REG_ALS_ZONE_INFO, );
- if (ret) {
- dev_err(_dev->dev, "failed to read zone\n");
- return ret;
- }
-
- val = (val & LM3533_ALS_ZONE_MASK)
[PATCH v1 4/4] mfd: lm3533: Remove the driver
The driver has no in kernel users and requires a board file
to be instantiated. Remove basically a dead code.
If ever needed, it can be reinstantiated and converted to one
that uses firmware node interfaces.
Signed-off-by: Andy Shevchenko
---
drivers/mfd/lm3533-core.c | 645 -
include/linux/mfd/lm3533.h | 100 --
2 files changed, 745 deletions(-)
delete mode 100644 drivers/mfd/lm3533-core.c
delete mode 100644 include/linux/mfd/lm3533.h
diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
deleted file mode 100644
index c211183cecb2..
--- a/drivers/mfd/lm3533-core.c
+++ /dev/null
@@ -1,645 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * lm3533-core.c -- LM3533 Core
- *
- * Copyright (C) 2011-2012 Texas Instruments
- *
- * Author: Johan Hovold
- */
-
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-
-#include
-
-
-#define LM3533_BOOST_OVP_MASK 0x06
-#define LM3533_BOOST_OVP_SHIFT 1
-
-#define LM3533_BOOST_FREQ_MASK 0x01
-#define LM3533_BOOST_FREQ_SHIFT0
-
-#define LM3533_BL_ID_MASK 1
-#define LM3533_LED_ID_MASK 3
-#define LM3533_BL_ID_MAX 1
-#define LM3533_LED_ID_MAX 3
-
-#define LM3533_HVLED_ID_MAX2
-#define LM3533_LVLED_ID_MAX5
-
-#define LM3533_REG_OUTPUT_CONF10x10
-#define LM3533_REG_OUTPUT_CONF20x11
-#define LM3533_REG_BOOST_PWM 0x2c
-
-#define LM3533_REG_MAX 0xb2
-
-
-static struct mfd_cell lm3533_als_devs[] = {
- {
- .name = "lm3533-als",
- .id = -1,
- },
-};
-
-static struct mfd_cell lm3533_bl_devs[] = {
- {
- .name = "lm3533-backlight",
- .id = 0,
- },
- {
- .name = "lm3533-backlight",
- .id = 1,
- },
-};
-
-static struct mfd_cell lm3533_led_devs[] = {
- {
- .name = "lm3533-leds",
- .id = 0,
- },
- {
- .name = "lm3533-leds",
- .id = 1,
- },
- {
- .name = "lm3533-leds",
- .id = 2,
- },
- {
- .name = "lm3533-leds",
- .id = 3,
- },
-};
-
-int lm3533_read(struct lm3533 *lm3533, u8 reg, u8 *val)
-{
- int tmp;
- int ret;
-
- ret = regmap_read(lm3533->regmap, reg, );
- if (ret < 0) {
- dev_err(lm3533->dev, "failed to read register %02x: %d\n",
- reg, ret);
- return ret;
- }
-
- *val = tmp;
-
- dev_dbg(lm3533->dev, "read [%02x]: %02x\n", reg, *val);
-
- return ret;
-}
-EXPORT_SYMBOL_GPL(lm3533_read);
-
-int lm3533_write(struct lm3533 *lm3533, u8 reg, u8 val)
-{
- int ret;
-
- dev_dbg(lm3533->dev, "write [%02x]: %02x\n", reg, val);
-
- ret = regmap_write(lm3533->regmap, reg, val);
- if (ret < 0) {
- dev_err(lm3533->dev, "failed to write register %02x: %d\n",
- reg, ret);
- }
-
- return ret;
-}
-EXPORT_SYMBOL_GPL(lm3533_write);
-
-int lm3533_update(struct lm3533 *lm3533, u8 reg, u8 val, u8 mask)
-{
- int ret;
-
- dev_dbg(lm3533->dev, "update [%02x]: %02x/%02x\n", reg, val, mask);
-
- ret = regmap_update_bits(lm3533->regmap, reg, mask, val);
- if (ret < 0) {
- dev_err(lm3533->dev, "failed to update register %02x: %d\n",
- reg, ret);
- }
-
- return ret;
-}
-EXPORT_SYMBOL_GPL(lm3533_update);
-
-static int lm3533_set_boost_freq(struct lm3533 *lm3533,
- enum lm3533_boost_freq freq)
-{
- int ret;
-
- ret = lm3533_update(lm3533, LM3533_REG_BOOST_PWM,
- freq << LM3533_BOOST_FREQ_SHIFT,
- LM3533_BOOST_FREQ_MASK);
- if (ret)
- dev_err(lm3533->dev, "failed to set boost frequency\n");
-
- return ret;
-}
-
-
-static int lm3533_set_boost_ovp(struct lm3533 *lm3533,
- enum lm3533_boost_ovp ovp)
-{
- int ret;
-
- ret = lm3533_update(lm3533, LM3533_REG_BOOST_PWM,
- ovp << LM3533_BOOST_OVP_SHIFT,
- LM3533_BOOST_OVP_MASK);
- if (ret)
- dev_err(lm3533->dev, "failed to set boost ovp\n");
-
- return ret;
-}
-
-
Re: [PATCH] kernel/resource: optimize find_next_iomem_res
On Thu, May 30, 2024 at 10:36:57PM -0700, Chia-I Wu wrote: > We can skip children resources when the parent resource does not cover > the range. > This should help vmf_insert_* users on x86, such as several DRM drivers. vmf_insert_*() > On my AMD Ryzen 5 7520C, when streaming data from cpu memory into amdgpu > bo, the throughput goes from 5.1GB/s to 6.6GB/s. perf report says Also in the $Subj (and pay attention to the prefix) "resource: ... find_next_iomem_res()" -- With Best Regards, Andy Shevchenko
Re: [PATCH] kernel/resource: optimize find_next_iomem_res
On Thu, May 30, 2024 at 10:36:57PM -0700, Chia-I Wu wrote: > We can skip children resources when the parent resource does not cover > the range. > > This should help vmf_insert_* users on x86, such as several DRM drivers. > On my AMD Ryzen 5 7520C, when streaming data from cpu memory into amdgpu > bo, the throughput goes from 5.1GB/s to 6.6GB/s. perf report says > > 34.69%--__do_fault > 34.60%--amdgpu_gem_fault > 34.00%--ttm_bo_vm_fault_reserved > 32.95%--vmf_insert_pfn_prot > 25.89%--track_pfn_insert > 24.35%--lookup_memtype > 21.77%--pat_pagerange_is_ram > 20.80%--walk_system_ram_range > 17.42%--find_next_iomem_res > > before this change, and > > 26.67%--__do_fault > 26.57%--amdgpu_gem_fault > 25.83%--ttm_bo_vm_fault_reserved > 24.40%--vmf_insert_pfn_prot > 14.30%--track_pfn_insert > 12.20%--lookup_memtype > 9.34%--pat_pagerange_is_ram > 8.22%--walk_system_ram_range > 5.09%--find_next_iomem_res > > after. Is there any documentation that explicitly says that the children resources must not overlap parent's one? Do we have some test cases? (Either way they needs to be added / expanded). P.S> I'm not so sure about this change. It needs a thoroughly testing, esp. in PCI case. Cc'ing to Ilpo. -- With Best Regards, Andy Shevchenko
Re: [PATCH v10 08/11] lib: add basic KUnit test for lib/math
On Thu, May 30, 2024 at 10:48:10PM +0530, Devarsh Thakkar wrote: > From: Daniel Latypov > > Add basic test coverage for files that don't require any config options: > * part of math.h (what seem to be the most commonly used macros) > * gcd.c > * lcm.c > * int_sqrt.c > * reciprocal_div.c > (Ignored int_pow.c since it's a simple textbook algorithm.) > > These tests aren't particularly interesting, but they > * provide short and simple examples of parameterized tests > * provide a place to add tests for any new files in this dir > * are written so adding new test cases to cover edge cases should be > easy > * looking at code coverage, we hit all the branches in the .c files ... > +#include > +#include > +#include > +#include Really, you ignored my comment a second (?) time? This is road to nowhere. You need to update the inclusion bloc in accordance with IWYU principle. I see a few headers are missing. -- With Best Regards, Andy Shevchenko
Re: [PATCH v10 07/11] Documentation: core-api: Add math.h macros and functions
On Thu, May 30, 2024 at 10:47:40PM +0530, Devarsh Thakkar wrote: > Add documentation for rounding, scaling, absolute value and difference, > 32-bit division related macros and functions exported by math.h header > file. ... > +Rounding, absolute value, scaling and 32bit division functions > +-- > + > +.. kernel-doc:: include/linux/math.h > + :internal: Please, double check that this is correct keyword in this case. Otherwise LGTM. -- With Best Regards, Andy Shevchenko
Re: [PATCH v10 06/11] math.h: Add macros for rounding to closest value
On Thu, May 30, 2024 at 10:42:25PM +0530, Devarsh Thakkar wrote: > Add below rounding related macros: > > round_closest_up(x, y) : Rounds x to closest multiple of y where y is a > power of 2, with a preference to round up in case two nearest values are > possible. > > round_closest_down(x, y) : Rounds x to closest multiple of y where y is a > power of 2, with a preference to round down in case two nearest values are > possible. > > roundclosest(x, y) : Rounds x to closest multiple of y, this macro should > generally be used only when y is not multiple of 2 as otherwise > round_closest* macros should be used which are much faster. > > Examples: > * round_closest_up(17, 4) = 16 > * round_closest_up(15, 4) = 16 > * round_closest_up(14, 4) = 16 > * round_closest_down(17, 4) = 16 > * round_closest_down(15, 4) = 16 > * round_closest_down(14, 4) = 12 > * roundclosest(21, 5) = 20 > * roundclosest(19, 5) = 20 > * roundclosest(17, 5) = 15 ... > + * Examples : It's inconsistent with the other one below. > + * round_closest_up(17, 4) = 16 > + * > + * round_closest_up(15, 4) = 16 > + * > + * round_closest_up(14, 4) = 16 The three have TABs/spaces mixture. I believe you wanted: * Examples:: * * round_closest_up(17, 4) = 16 * * round_closest_up(15, 4) = 16 * * round_closest_up(14, 4) = 16 ... > + * Examples: > + * > + * round_closest_down(17, 4) = 16 > + * > + * round_closest_down(15, 4) = 16 > + * > + * round_closest_down(14, 4) = 12 As per above ... > + * Examples : > + * > + * roundclosest(21, 5) = 20 > + * > + * roundclosest(19, 5) = 20 > + * > + * roundclosest(17, 5) = 15 As per above. -- With Best Regards, Andy Shevchenko
Re: [PATCH v9 07/10] lib: add basic KUnit test for lib/math
On Tue, May 28, 2024 at 05:01:31PM +0530, Devarsh Thakkar wrote:
> On 28/05/24 02:08, Andy Shevchenko wrote:
> > On Mon, May 27, 2024 at 11:37:20PM +0300, Andy Shevchenko wrote:
> >> On Sun, May 26, 2024 at 11:39:33PM +0530, Devarsh Thakkar wrote:
...
> >>> +MODULE_LICENSE("GPL");
> >>
> >> modpost validator won't be happy about this, i.e. missing
> >> MODULE_DESCRIPTION().
> >
> > And obviously + module.h in the inclusion block.
>
> The module.h is already included under include/kunit/test.h and that's the
> reason compiler did not give any error. But I can still include it under
> math.h for better readability as you suggested as anyway compiler will not
> re-include if already included by another header file.
Please do as it will be in line with IWYU principle.
> Also I see we were missing a dependency between math_kunit and kunit modules,
> so adding a dependency there too.
Thank you.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v9 07/10] lib: add basic KUnit test for lib/math
On Tue, May 28, 2024 at 04:51:46PM +0530, Devarsh Thakkar wrote: > On 28/05/24 02:07, Andy Shevchenko wrote: [..] > >> +#include > >> +#include > >> +#include > > > > + math.h (where abs()/DIV_ROUND_*()/etc come from?) > > I believe I mentioned that. > > > > I did compile and test this, so math.h was indirectly getting included via > some other header file already included but I would not rely on that and > include math.h separately as you suggested. Please, follow the IWYU principle, what you are suggesting is a fragile approach. > >> +#include -- With Best Regards, Andy Shevchenko
Re: [PATCH v9 06/10] math.h: Add macros for rounding to closest value
On Tue, May 28, 2024 at 04:02:30PM +0530, Devarsh Thakkar wrote: > On 28/05/24 02:02, Andy Shevchenko wrote: > > On Sun, May 26, 2024 at 11:38:56PM +0530, Devarsh Thakkar wrote: ... > >> +/** > >> + * round_closest_up - round closest to be multiple of specified value > >> (which is > >> + *power of 2) with preference to rounding up > >> + > > > > Not that big deal, but missing '*' here. Personally I would not even put > > a blank line between Summary and Field Descriptions. > > My bad. Yes I would remove the blank line here. This is picked up as warning > from kernel-doc too. > > >> + * @x: the value to round > >> + * @y: multiple to round closest to (must be a power of 2) > >> + * > >> + * Rounds @x to closest multiple of @y (which must be a power of 2). > >> + * The value can be either rounded up or rounded down depending upon > >> rounded > >> + * value's closeness to the specified value. If there are two closest > >> possible > >> + * values, i.e. the difference between the specified value and it's > >> rounded up > >> + * and rounded down values is same then preference is given to rounded up > >> + * value. > >> + * > >> + * To perform arbitrary rounding to closest value (not multiple of 2), use > >> + * roundclosest(). > >> + * > >> + * Examples : > > > > What is this suppose to be rendered to? > > The file math.h is not rendered as part of kernel-doc right now. I can put > this under Documentation/core-api/kernel-api.rst perhaps I can create a new > section as below: > > Rounding, absolute diff and 32bit division macros > - > > under the section: > CRC and Math Functions in Linux > > === > > is that okay ? This is up to you, but what I meant is that you always can render manually yourself. And I was asking about the result you got when you tried (and you did, right?) to render to man, html, and pdf. > >> + * round_closest_up(17, 4) = 16 > >> + * round_closest_up(15, 4) = 16 > >> + * round_closest_up(14, 4) = 16 > > > > Btw, is kernel-doc validator happy about all kernel docs you added? > > Yes, except the aforementioned blank line. -- With Best Regards, Andy Shevchenko
Re: [DO NOT MERGE v8 08/36] clocksource: sh_tmu: CLOCKSOURCE support.
On Wed, May 29, 2024 at 05:00:54PM +0900, Yoshinori Sato wrote:
> Allows initialization as CLOCKSOURCE.
...
> - dev_info(>tmu->pdev->dev, "ch%u: used for %s clock events\n",
> - ch->index, periodic ? "periodic" : "oneshot");
> + pr_info("%s ch%u: used for %s clock events\n",
> + ch->tmu->name, ch->index, periodic ? "periodic" : "oneshot");
This is a step back change. We should use dev_*() if we have a device
available. And I believe this is the case (at least for the previous boards),
no?
...
> - ch->irq = platform_get_irq(tmu->pdev, index);
> + if (tmu->np)
> + ch->irq = of_irq_get(tmu->np, index);
> + else if (tmu->pdev)
> + ch->irq = platform_get_irq(tmu->pdev, index);
I found these changes counterproductive. Instead better to have up to three
files to cover:
- the common code (library)
- the platform device support
- the pure OF support.
...
> - res = platform_get_resource(tmu->pdev, IORESOURCE_MEM, 0);
> - if (!res) {
> - dev_err(>pdev->dev, "failed to get I/O memory\n");
> - return -ENXIO;
> + if (tmu->pdev) {
> + res = platform_get_resource(tmu->pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + pr_err("sh_tmu failed to get I/O memory\n");
> + return -ENXIO;
> + }
> +
> + tmu->mapbase = ioremap(res->start, resource_size(res));
devm_platform_ioremap_resource() should be good to have.
Again, consider proper splitting.
> }
> + if (tmu->np)
> + tmu->mapbase = of_iomap(tmu->np, 0);
So, how many boards are non-OF compatible? Maybe makes sense to move them to OF
and drop these platform code entirely from everywhere?
...
> + tmu->name = dev_name(>dev);
> + tmu->clk = clk_get(>pdev->dev, "fck");
devm_ approach can help a lot in case of platform device code.
> + if (IS_ERR(tmu->clk)) {
> + dev_err(>pdev->dev, "cannot get clock\n");
> + return PTR_ERR(tmu->clk);
return dev_err_probe() ?
> + }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v9 07/10] lib: add basic KUnit test for lib/math
On Mon, May 27, 2024 at 11:37:20PM +0300, Andy Shevchenko wrote:
> On Sun, May 26, 2024 at 11:39:33PM +0530, Devarsh Thakkar wrote:
...
> > +MODULE_LICENSE("GPL");
>
> modpost validator won't be happy about this, i.e. missing
> MODULE_DESCRIPTION().
And obviously + module.h in the inclusion block.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v9 07/10] lib: add basic KUnit test for lib/math
On Sun, May 26, 2024 at 11:39:33PM +0530, Devarsh Thakkar wrote:
> From: Daniel Latypov
>
> Add basic test coverage for files that don't require any config options:
> * part of math.h (what seem to be the most commonly used macros)
> * gcd.c
> * lcm.c
> * int_sqrt.c
> * reciprocal_div.c
> (Ignored int_pow.c since it's a simple textbook algorithm.)
>
> These tests aren't particularly interesting, but they
> * provide short and simple examples of parameterized tests
> * provide a place to add tests for any new files in this dir
> * are written so adding new test cases to cover edge cases should be
> easy
> * looking at code coverage, we hit all the branches in the .c files
...
> +#include
> +#include
> +#include
+ math.h (where abs()/DIV_ROUND_*()/etc come from?)
I believe I mentioned that.
> +#include
...
> +MODULE_LICENSE("GPL");
modpost validator won't be happy about this, i.e. missing MODULE_DESCRIPTION().
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v9 06/10] math.h: Add macros for rounding to closest value
On Sun, May 26, 2024 at 11:38:56PM +0530, Devarsh Thakkar wrote: > Add below rounding related macros: > > round_closest_up(x, y) : Rounds x to closest multiple of y where y is a > power of 2, with a preference to round up in case two nearest values are > possible. > > round_closest_down(x, y) : Rounds x to closest multiple of y where y is a > power of 2, with a preference to round down in case two nearest values are > possible. > > roundclosest(x, y) : Rounds x to closest multiple of y, this macro should > generally be used only when y is not multiple of 2 as otherwise > round_closest* macros should be used which are much faster. > > Examples: > * round_closest_up(17, 4) = 16 > * round_closest_up(15, 4) = 16 > * round_closest_up(14, 4) = 16 > * round_closest_down(17, 4) = 16 > * round_closest_down(15, 4) = 16 > * round_closest_down(14, 4) = 12 > * roundclosest(21, 5) = 20 > * roundclosest(19, 5) = 20 > * roundclosest(17, 5) = 15 ... > +/** > + * round_closest_up - round closest to be multiple of specified value (which > is > + *power of 2) with preference to rounding up > + Not that big deal, but missing '*' here. Personally I would not even put a blank line between Summary and Field Descriptions. > + * @x: the value to round > + * @y: multiple to round closest to (must be a power of 2) > + * > + * Rounds @x to closest multiple of @y (which must be a power of 2). > + * The value can be either rounded up or rounded down depending upon rounded > + * value's closeness to the specified value. If there are two closest > possible > + * values, i.e. the difference between the specified value and it's rounded > up > + * and rounded down values is same then preference is given to rounded up > + * value. > + * > + * To perform arbitrary rounding to closest value (not multiple of 2), use > + * roundclosest(). > + * > + * Examples : What is this suppose to be rendered to? > + * round_closest_up(17, 4) = 16 > + * round_closest_up(15, 4) = 16 > + * round_closest_up(14, 4) = 16 Btw, is kernel-doc validator happy about all kernel docs you added? > + */ -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/2] string: add mem_is_zero() helper to check if memory area is all zeros
On Mon, May 27, 2024 at 12:43 PM Jani Nikula wrote:
>
> Almost two thirds of the memchr_inv() usages check if the memory area is
> all zeros, with no interest in where in the buffer the first non-zero
> byte is located. Checking for !memchr_inv(s, 0, n) is also not very
> intuitive or discoverable. Add an explicit mem_is_zero() helper for this
> use case.
...
> +static inline bool mem_is_zero(const void *s, size_t n)
> +{
> + return !memchr_inv(s, 0, n);
> +}
There are potential users for the 0xff check as well. Hence the
following question:
Are we going to have a new function per byte in question, or do we
come up with a common denominator, like mem_is_all_of(mem, byte)?
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 2/2] drm: use mem_is_zero() instead of !memchr_inv(s, 0, n)
On Mon, May 27, 2024 at 12:43 PM Jani Nikula wrote:
>
> Use the mem_is_zero() helper where possible.
...
> - if (memchr_inv(guid, 0, 16) == NULL) {
> + if (mem_is_zero(guid, 16)) {
> tmp64 = get_jiffies_64();
> memcpy([0], , sizeof(u64));
> memcpy([8], , sizeof(u64));
What is the type of guid? Shouldn't it be guid_t with the respective
guid_is_null()
...
> - if (memchr_inv(guid, 0, 16))
> + if (!mem_is_zero(guid, 16))
> return true;
Ditto.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v8 07/10] lib: add basic KUnit test for lib/math
On Mon, May 20, 2024 at 07:51:24PM +0530, Devarsh Thakkar wrote: > On 20/05/24 17:52, Andy Shevchenko wrote: > > On Mon, May 20, 2024 at 05:11:18PM +0530, Devarsh Thakkar wrote: > >> On 18/05/24 01:44, Andy Shevchenko wrote: > >>> On Fri, May 17, 2024 at 11:06:07PM +0530, Devarsh Thakkar wrote: [..] > > Yes, and one should follow IWYU principle and not cargo cult or whatever > > arbitrary lists. > > Agreed. > >>>> +#include > >>> > >>> + math.h // obviously > >>> + module.h > >>> > >>>> +#include > >>> > >>> + types.h > >> > >> All the above headers are already included as part of kernel.h > > > > Yes, that's why you should not use "proxy" headers. > > Have you read the top comment in the kernel.h? > > Yes, it says it is not recommended to include this inside another header file. > Although here we are adding it inside c file, but I can still try avoid it and > include only the required headers instead of kernel.h as you recommended. Right, but the first sentence there is "This header has combined a lot of unrelated to each other stuff." Can you explain how you use in your code all that unrelated stuff? For example, how do you use *trace_*() calls? Or maybe might_*() calls? or anything else that is directly provided by kernel.h? Besides IWYU principle above, it's good to have a justification for each inclusion the C file has. I believe there is no a such in _this_ case. -- With Best Regards, Andy Shevchenko
Re: [PATCH v8 07/10] lib: add basic KUnit test for lib/math
On Mon, May 20, 2024 at 05:11:18PM +0530, Devarsh Thakkar wrote: > On 18/05/24 01:44, Andy Shevchenko wrote: > > On Fri, May 17, 2024 at 11:06:07PM +0530, Devarsh Thakkar wrote: [..] > >> +#include > >> +#include > > > >> +#include > > > > Do you know why this header is included? > > It includes all the other required headers (including those you mentioned > below), and header list is probably copied from other files in same directory. > But it does suffice the requirements as I have verified the compilation. Yes, and one should follow IWYU principle and not cargo cult or whatever arbitrary lists. > >> +#include > > > > + math.h // obviously > > + module.h > > > >> +#include > > > > + types.h > > All the above headers are already included as part of kernel.h Yes, that's why you should not use "proxy" headers. Have you read the top comment in the kernel.h? -- With Best Regards, Andy Shevchenko
Re: [PATCH v8 07/10] lib: add basic KUnit test for lib/math
On Fri, May 17, 2024 at 01:53:47PM -0700, Daniel Latypov wrote:
> On Fri, May 17, 2024 at 1:14 PM Andy Shevchenko
> wrote:
...
> > > [devarsht: Rebase to 6.9 and change license to GPL]
> >
> > I'm not sure that you may change license. It needs the author's
> > confirmation.
>
> Checking, this is referring to the MODULE_LICENSE, which used to be
> MODULE_LICENSE("GPL v2");
>
> and is now
> MODULE_LICENSE("GPL");
>
> If checkpatch is suggesting that now, then changing it sounds good to me.
In this case I agree on the change as it's purely syntax and not semantic.
> > > ---
> > > Changes since v6:
> > > * Rebase to linux-next, change license to GPL as suggested by checkpatch.
> >
> > Note, checkpatch.pl is not false positives free. Be careful
> > with what it suggests.
> >
> > > +#include
> > > +#include
> >
> > > +#include
> >
> > Do you know why this header is included?
>
> I think I had put it in the original before a lot of the work you did
> to split things out of kernel.h.
> I haven't had time to look apply this patch series locally yet, but
> I'd be pretty sure we can remove it without anything breaking.
Briefly looking at the code I even not sure it was needed before, but maybe
I missed something, in any case, please remove / replace it.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v8 07/10] lib: add basic KUnit test for lib/math
On Fri, May 17, 2024 at 11:06:07PM +0530, Devarsh Thakkar wrote: > From: Daniel Latypov > > Add basic test coverage for files that don't require any config options: > * part of math.h (what seem to be the most commonly used macros) > * gcd.c > * lcm.c > * int_sqrt.c > * reciprocal_div.c > (Ignored int_pow.c since it's a simple textbook algorithm.) > > These tests aren't particularly interesting, but they > * provide short and simple examples of parameterized tests > * provide a place to add tests for any new files in this dir > * are written so adding new test cases to cover edge cases should be > easy > * looking at code coverage, we hit all the branches in the .c files ... > [devarsht: Rebase to 6.9 and change license to GPL] I'm not sure that you may change license. It needs the author's confirmation. > --- > Changes since v6: > * Rebase to linux-next, change license to GPL as suggested by checkpatch. Note, checkpatch.pl is not false positives free. Be careful with what it suggests. > +#include > +#include > +#include Do you know why this header is included? > +#include + math.h // obviously + module.h > +#include + types.h ... Other than above, LGTM. -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 6/8] math.h Add macros to round to closest specified power of 2
On Mon, May 13, 2024 at 06:34:19PM +0530, Devarsh Thakkar wrote: > On 13/05/24 17:55, Andy Shevchenko wrote: > > On Mon, May 13, 2024 at 04:55:58PM +0530, Devarsh Thakkar wrote: > >> On 13/05/24 14:29, Andy Shevchenko wrote: > >>> On Sat, May 11, 2024 at 11:11:14PM +0530, Devarsh Thakkar wrote: > >>>> On 10/05/24 20:45, Jani Nikula wrote: [...] > > - align naming (with the existing round*() macros) > > I think round_closest_up/round_closest_down align already and inspired by the > existing naming convention used for round*() and DIV_ROUND_CLOSEST() macros in > math.h as explained below (copied from my previous reply [1]) > > "Coming back to naming, this is as per existing convention used for naming > round_up, round_down (notice the `_` being used for macros working with pow of > 2) and DIV_ROUND_CLOSEST (notice the work `closest` used to specify the answer > to be nearest to specified value)" > > But do let me know if you have any other suggestions for naming? Just make sure that semantically the naming is aligned, that's it. If you think it's already done that way, fine! -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 6/8] math.h Add macros to round to closest specified power of 2
On Mon, May 13, 2024 at 04:55:58PM +0530, Devarsh Thakkar wrote: > On 13/05/24 14:29, Andy Shevchenko wrote: > > On Sat, May 11, 2024 at 11:11:14PM +0530, Devarsh Thakkar wrote: > >> On 10/05/24 20:45, Jani Nikula wrote: [...] > >>> Moreover, I think the naming of round_up() and round_down() should have > >>> reflected the fact that they operate on powers of 2. It's unfortunate > >>> that the difference to roundup() and rounddown() is just the underscore! > >>> That's just a trap. > >>> > >>> So let's perhaps not repeat the same with round_closest_up() and > >>> round_closest_down()? > >> > >> Yes the naming is inspired by existing macros i.e. round_up, round_down > >> (which round up/down to next pow of 2) and DIV_ROUND_CLOSEST (which > >> round the divided value to closest value) and there are already a lot of > >> users for these API's : > >> > >> linux-next git:(heads/next-20240509) ✗ grep -nr round_up drivers | wc > >> 7304261 74775 > >> > >> linux-next git:(heads/next-20240509) ✗ grep -nr round_down drivers | wc > >> 2261293 22194 > >> > >> linux-next git:(heads/next-20240509) ✗ grep -nr DIV_ROUND_CLOSEST > >> drivers | wc > >>12077461 111822 > > > > Side note, discover `git grep ...`: it's much much faster on Git index, > > than classic one on a working copy. > > > >> so I thought to align with existing naming convention assuming > >> developers are already familiar with this. > >> > >> But if a wider consensus is to go with a newer naming convention then I > >> am open to it, although a challenge there would be to keep it short. For > >> e.g. this one is already 3 words, if we go with more explicit > >> "round_closest_up_pow_2" it looks quite long in my opinion :) . > > > > You need properly name the macros. Again, round_up() / roundup() and > > roundup_pow_of_two() are three _different_ macros, and it's not clear > > why you can't use one of them in your case. > > I can't use any of these because these macros either round up or round down, > whereas I want to round to closest value for the argument specified by the > user, be it achieved either by rounding up or rounding down depending upon > whichever makes the answer closer to the user-specified argument. > > To make it clear, I have already included the examples in the macro > description [2], copying it here, maybe I can put the same examples in the > commit message too to avoid confusions : > > round_closest_up(17, 4) = 16 > round_closest_up(15, 4) = 16 > round_closest_up(14, 4) = 16 > > round_closest_down(17, 4) = 16 > round_closest_down(15, 4) = 16 > round_closest_down(14, 4) = 12 > > Coming back to naming, this is as per existing convention used for naming > round_up, round_down (notice the `_` being used for macros working with pow of > 2) and DIV_ROUND_CLOSEST (notice the work `closest` used to specify the answer > to be nearest to specified value). Naming is a bit subjective, but I > personally don't think it is a good idea to go away with the existing naming > convention or go with longer names. > > > The patch that changes those to a new one are doubtful to begin with. > > I.e. need a careful review on the arithmetics side of the change > > including HW capabilities of handling "closest" results. > > This is already tested from my side, in-fact I have posted some of the results > in cover-letter with these macros [1] : > > Regarding hardware capabilities, it uses existing round_up, round_down macros > underneath which are optimized to handle pow of 2 after modifying the user > provided argument using addition/subtraction and division, so I don't think it > should generally a problem with the hardware. > And I see other macros DIV_ROUND_CLOSEST [3] already using similar operations > i.e. addition/subtraction and division so don't think it should be a problem > to keep similar other macros in the same file. Okay, thank you for elaborating. So, what I would expect in the new version of the series is that: - align naming (with the existing round*() macros) - add examples into commit message of the math.h patch - add test cases (you need to create lib/math_kunit.c for that) - elaborate in the commit message of the IPU3 change what you posted above (some kind of a summary) > [1]: > https://gist.github.com/devarsht/de6f5142f678bb1a5338abfd9f814abd#file-v7_jpeg_encoder_crop_validation-L204 > [2]: https://lore.kernel.org/all/20240509183952.4064331-1-devar...@ti.com/ > [3]: https://elixir.bootlin.com/linux/latest/source/include/linux/math.h#L86 -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 6/8] math.h Add macros to round to closest specified power of 2
On Sat, May 11, 2024 at 11:11:14PM +0530, Devarsh Thakkar wrote: > On 10/05/24 20:45, Jani Nikula wrote: [...] > > Moreover, I think the naming of round_up() and round_down() should have > > reflected the fact that they operate on powers of 2. It's unfortunate > > that the difference to roundup() and rounddown() is just the underscore! > > That's just a trap. > > > > So let's perhaps not repeat the same with round_closest_up() and > > round_closest_down()? > > Yes the naming is inspired by existing macros i.e. round_up, round_down > (which round up/down to next pow of 2) and DIV_ROUND_CLOSEST (which > round the divided value to closest value) and there are already a lot of > users for these API's : > > linux-next git:(heads/next-20240509) ✗ grep -nr round_up drivers | wc > 7304261 74775 > > linux-next git:(heads/next-20240509) ✗ grep -nr round_down drivers | wc > 2261293 22194 > > linux-next git:(heads/next-20240509) ✗ grep -nr DIV_ROUND_CLOSEST > drivers | wc >12077461 111822 Side note, discover `git grep ...`: it's much much faster on Git index, than classic one on a working copy. > so I thought to align with existing naming convention assuming > developers are already familiar with this. > > But if a wider consensus is to go with a newer naming convention then I > am open to it, although a challenge there would be to keep it short. For > e.g. this one is already 3 words, if we go with more explicit > "round_closest_up_pow_2" it looks quite long in my opinion :) . You need properly name the macros. Again, round_up() / roundup() and roundup_pow_of_two() are three _different_ macros, and it's not clear why you can't use one of them in your case. The patch that changes those to a new one are doubtful to begin with. I.e. need a careful review on the arithmetics side of the change including HW capabilities of handling "closest" results. -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 6/8] math.h Add macros to round to closest specified power of 2
On Sun, May 12, 2024 at 07:46:58AM +0300, Alexey Dobriyan wrote: > I think > > roundup(x, 1 << n) Since it's about power-of-two, round_up() is better. > is more readable. -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 8/8] gpu: ipu-v3: Use generic macro for rounding to nearest multiple
On Fri, May 10, 2024 at 06:16:42PM +0300, Laurent Pinchart wrote: > On Fri, May 10, 2024 at 06:03:52PM +0300, Andy Shevchenko wrote: > > On Fri, May 10, 2024 at 12:10:10AM +0530, Devarsh Thakkar wrote: > > > Use generic macro round_closest_up for rounding to nearest multiple > > > instead > > > > round_closest_up() > > > > We refer to the functions as func(). > > > > > of using local function. ... > > > @@ -565,7 +563,7 @@ static void find_best_seam(struct > > > ipu_image_convert_ctx *ctx, > > >* The closest input sample position that we could actually > > >* start the input tile at, 19.13 fixed point. > > >*/ > > > - in_pos_aligned = round_closest(in_pos, 8192U * in_align); > > > + in_pos_aligned = round_closest_up(in_pos, 8192U * in_align); > > > /* Convert 19.13 fixed point to integer */ > > > in_pos_rounded = in_pos_aligned / 8192U; > > > > Oh, these seems to be better to use either ALIGN*(), or PFN_*() / PAGE_*() > > families of macros. What the semantic of 8192 is? > > The comment mentions 19.13 fixed point, so I assume that's the > fractional part of the integer. It doesn't seem related to pages. Okay, and align word in all those variable names? -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 6/8] math.h Add macros to round to closest specified power of 2
On Fri, May 10, 2024 at 06:15:34PM +0300, Jani Nikula wrote: > On Fri, 10 May 2024, Andy Shevchenko > wrote: > > On Fri, May 10, 2024 at 12:09:52AM +0530, Devarsh Thakkar wrote: > >> Add macros to round to nearest specified power of 2. > > > > This is not what they are doing. For the above we already have macros > > defined. > > > >> Two macros are added : > > > > (Yes, after I wrapped to comment this line looks better on its own, > > so whatever will be the first sentence, this line should be separated > > from.) > > > >> round_closest_up and round_closest_down which round up to nearest multiple > > > > round_closest_up() and round_closest_down() > > > > > >> of 2 with a preference to round up or round down respectively if there are > >> two possible nearest values to the given number. > > > > You should reformulate, because AFAICS there is the crucial difference > > from these and existing round_*_pow_of_two(). > > Moreover, I think the naming of round_up() and round_down() should have > reflected the fact that they operate on powers of 2. It's unfortunate > that the difference to roundup() and rounddown() is just the underscore! > That's just a trap. FYI: https://stackoverflow.com/questions/58139219/difference-of-align-and-round-up-macro-in-the-linux-kernel > So let's perhaps not repeat the same with round_closest_up() and > round_closest_down()? -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 7/8] media: imagination: Round to closest multiple for cropping region
On Fri, May 10, 2024 at 12:10:01AM +0530, Devarsh Thakkar wrote: > If neither of the flags to round down (V4L2_SEL_FLAG_LE) or round up > (V4L2_SEL_FLAG_GE) are specified by the user, then round to nearest > multiple of requested value while updating the crop rectangle coordinates. > > Use the rounding macro which gives preference to rounding down in case two > nearest values (high and low) are possible to raise the probability of > cropping rectangle falling inside the bound region. This is arguable. How do we know that the bigger range is supported? The safest side is to go smaller than bigger. -- With Best Regards, Andy Shevchenko
Re: [PATCH RESEND v7 0/8] Add V4L2 M2M Driver for E5010 JPEG Encoder
On Fri, May 10, 2024 at 01:56:03PM +0530, Devarsh Thakkar wrote: > Resending this V7 series to have proper linking of patches in the series > with cover-letter while doing git send-email. > > Original cover letter: > This adds support for V4L2 M2M based driver for E5010 JPEG Encoder > which is a stateful JPEG encoder from Imagination technologies > and is present in TI AM62A SoC. > > While adding support for it, following additional framework changes were > made: > - Moved reference quantization and huffman tables provided in >ITU-T-REC-T.81 to v4l2-jpeg.c as suggested in mailing list [1]. > - Add macros to round to closest integer (either higher or lower) while >rounding in order of 2. Nice, now it's chained! But all my comments to the previous mails are still applicable here. -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 8/8] gpu: ipu-v3: Use generic macro for rounding to nearest multiple
On Fri, May 10, 2024 at 12:10:10AM +0530, Devarsh Thakkar wrote: > Use generic macro round_closest_up for rounding to nearest multiple instead round_closest_up() We refer to the functions as func(). > of using local function. ... > @@ -565,7 +563,7 @@ static void find_best_seam(struct ipu_image_convert_ctx > *ctx, >* The closest input sample position that we could actually >* start the input tile at, 19.13 fixed point. >*/ > - in_pos_aligned = round_closest(in_pos, 8192U * in_align); > + in_pos_aligned = round_closest_up(in_pos, 8192U * in_align); > /* Convert 19.13 fixed point to integer */ > in_pos_rounded = in_pos_aligned / 8192U; Oh, these seems to be better to use either ALIGN*(), or PFN_*() / PAGE_*() families of macros. What the semantic of 8192 is? -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 6/8] math.h Add macros to round to closest specified power of 2
On Fri, May 10, 2024 at 12:09:52AM +0530, Devarsh Thakkar wrote: > Add macros to round to nearest specified power of 2. This is not what they are doing. For the above we already have macros defined. > Two macros are added : (Yes, after I wrapped to comment this line looks better on its own, so whatever will be the first sentence, this line should be separated from.) > round_closest_up and round_closest_down which round up to nearest multiple round_closest_up() and round_closest_down() > of 2 with a preference to round up or round down respectively if there are > two possible nearest values to the given number. You should reformulate, because AFAICS there is the crucial difference from these and existing round_*_pow_of_two(). > This patch is inspired from the Mentor Graphics IPU driver [1] which uses > similar macro locally and which can be updated to use this generic macro > instead along with other drivers having similar requirements. > > [1]: > https://elixir.bootlin.com/linux/v6.8.9/source/drivers/gpu/ipu-v3/ipu-image-convert.c#L480 Instead of this, just add a patch to convert that driver to use this new macro. Besides, this paragraph should go to the comment/changelog area below. > Signed-off-by: Devarsh Thakkar > --- > V1->V6 (No change, patch introduced in V7) > --- -- With Best Regards, Andy Shevchenko
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Fri, May 03, 2024 at 12:57:33PM +0800, Sui Jingfeng wrote:
> On 2024/5/3 01:28, Andy Shevchenko wrote:
> > On Fri, May 03, 2024 at 12:25:17AM +0800, Sui Jingfeng wrote:
> > > On 2024/5/2 16:32, Andy Shevchenko wrote:
> > > > On Wed, May 01, 2024 at 12:27:14AM +0800, Sui Jingfeng wrote:
> > > > > On 2024/4/30 22:13, Andy Shevchenko wrote:
> > > > > > On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote:
...
> > > > > > the former might be subdivided to "is it swnode backed or real
> > > > > > fwnode one?"
> > > > > >
> > > > > Yeah,
> > > > > On non-DT cases, it can be subdivided to swnode backed case and ACPI
> > > > > fwnode backed case.
> > > > >
> > > > >- For swnode backed case: the device_get_match_data() don't has a
> > > > > implemented backend.
> > > > >- For ACPI fwnode backed case: the device_get_match_data() has a
> > > > > implemented backend.
> > > > >
> > > > > But the driver has *neither* software node support
> > > > True.
> > > >
> > > > > nor ACPI support,
> > > > Not true.
> > > Why this is not true? Are you means that the panel-ilitek-ili9341 driver
> > > has ACPI support?
> > That's the idea (as far as I see the copy of the code from tinyDRM),
> > but broken over the copy'n'paste. This patch rectifies that to be
> > in align with the original code, which *does* support ACPI.
> >
> > > I'm asking because I don't see struct acpi_device_id related stuff in
> > > that source file,
> > > am I miss something?
> > Yes, you are. I leave it for you to research.
>
> After researching a few hours I still don't understand how does
> the panel-ilitek-ili9341 driver has the ACPI support and be able
> to ACPI probed when compiled as module.
>
> As far as I know, drivers that has the ACPI support *must* has the
> .acpi_match_table hooked, so that be able to be probed when the
> driver is compiled as a module.
No, and this is the thing. Hint: there is a glue code to reuse compatible
strings from OF, that's why dependency to OF prevents *some* systems from
being able to use that. But it's easy to fix by enabling OF in the
configuration, however the ID tables are orthogonal to the environment.
That's why all those ACPI_PTR() and of_match_ptr() are design mistakes
that are going to be removed eventually (the work is ongoing, btw,
as well as killing specific *_device_get_match_data() calls).
> For example, see commit 75a1b44a54bd9 ("spi: tegra210-quad: add acpi support")
> to get a feel what a SPI device with *real* ACPI support looks like.
If under *real* you assume the allocated _HID in use, yes, that's how it's
done. But there is the other tricky way of achieving similar effect w/o
allocating a new / custom ACPI _HID.
Hint: it's all documented in kernel under firmware-guide/acpi/.
> I have double checked the panel-ilitek-ili9341 driver, it doesn't
> has acpi_match_table hooked, which means that this driver won't
> even be able probed. And probed as pure SPI device still out of
> the scope of "correct use of device property APIs". Because SPI
> device specific method don't belong to the device property API.
> I don't really know what's we are missing, but we already intend
> to let it go, thanks.
>
> > > > So, slow down and take your time to get into the code and understand
> > > > how it works.
> > > >
> > > > > so that the rotation property can not get and ili9341_dpi_probe()
> > > > > will fails.
> > > > > So in total, this is not a 100% correct use of device property APIs.
> > > > >
> > > > > But I'm fine that if you want to leave(ignore) those less frequent
> > > > > use cases temporarily,
> > > > > there may have programmers want to post patches, to complete the
> > > > > missing in the future.
> > > > >
> > > > > So, there do have some gains on non-DT cases.
> > > > >
> > > > >- As you make it be able to compiled on X86 with the
> > > > > drm-misc-defconfig.
> > > > >- You cleanup the code up (at least patch 2 in this series is no
> > > > > obvious problem).
> > > > >- You allow people to modprobe it, and maybe half right and half
> > > > > undefined.
> > > > >
> > > > > But you do helps moving something forward, so congratulations for the
> > > > > wake up.
--
With Best Regards,
Andy Shevchenko
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Fri, May 03, 2024 at 01:28:26AM +0800, Sui Jingfeng wrote: > On 2024/5/2 15:34, Neil Armstrong wrote: > > On 30/04/2024 11:34, Maxime Ripard wrote: > > > On Tue, Apr 30, 2024 at 12:54:39AM +0800, Sui Jingfeng wrote: > > > > On 2024/4/29 19:55, Maxime Ripard wrote: > > > > > On Sat, Apr 27, 2024 at 01:57:46PM +0800, Sui Jingfeng wrote: > > > > > > On 2024/4/26 14:23, Maxime Ripard wrote: > > > > > > > On Fri, Apr 26, 2024 at 04:43:18AM +0800, Sui Jingfeng wrote: > > > > > > > > On 2024/4/26 03:10, Andy Shevchenko wrote: > > > > > > > > > On Fri, Apr 26, 2024 at 02:08:16AM +0800, Sui Jingfeng wrote: > > > > > > > > > > On 2024/4/25 22:26, Andy Shevchenko wrote: ... > > > > > > > > > > > It seems driver missed the point of proper use of device > > > > > > > > > > > property APIs. Correct this by updating headers and > > > > > > > > > > > calls respectively. > > > > > > > > > > You are using the 'seems' here exactly saying that you are > > > > > > > > > > not 100% sure. > > > > > > > > > > Please allow me to tell you the truth: This patch again has > > > > > > > > > > ZERO effect. It fix nothing. And this patch is has the > > > > > > > > > > risks to be wrong. > > > > > > > > > Huh?! Really, stop commenting the stuff you do not understand. > > > > > > > > I'm actually a professional display drivers developer at the > > > > > > > > downstream in the past, despite my contribution to upstream is > > > > > > > > less. But I believe that all panel driver developers know what > > > > > > > > I'm talking about. So please have take a look at my replies. > > > > > > > Most of the interactions you had in this series has been uncalled > > > > > > > for. You might be against a patch, but there's no need to go to > > > > > > > such length. > > > > > > > > > > > > > > As far as I'm concerned, this patch is fine to me in itself, and > > > > > > > I don't see anything that would prevent us from merging it. > > > > > > No one is preventing you, as long as don't misunderstanding what > > > > > > other people's technical replies intentionally. I'm just a usual > > > > > > and normal contributor, I hope the world will better than > > > > > > yesterday. > > > > > You should seriously consider your tone when replying then. > > > > > > > > > > > Saying such thing to me may not proper, I guess you may want to talk > > > > > > to peoples who has the push rights > > > > > I think you misunderstood me. My point was that your several rants > > > > > were uncalled for and aren't the kind of things we're doing here. > > > > > > > > > > I know very well how to get a patch merged, thanks. > > > > > > > > > > > just make sure it isn't a insult to the professionalism of drm > > > > > > bridge > > > > > > community itself though. > > > > > I'm not sure why you're bringing the bridge community or its > > > > > professionalism. It's a panel, not a bridge, and I never doubted the > > > > > professionalism of anyone. > > > > > > > > I means that the code itself could be adopted, as newer and younger > > > > programmer (like Andy) need to be encouraged to contribute. > > > > > > Andy has thousands of commits in Linux. He's *very* far from being a new > > > contributor. > > > > > > > I express no obvious objections, just hints him that something else > > > > probably should also be taken into consideration as well. > > > > > > That might be what you wanted to express, but you definitely didn't > > > express it that way. > > > > > > > On the other hand, we probably should allow other people participate in > > > > discussion so that it is sufficient discussed and ensure that it won't > > > > be reverted by someone in the future for some reasons. Backing to out > > > > case happens here, we may need to move things forward. Therefore, it > > > > definitely deserve to have a t
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Fri, May 03, 2024 at 12:25:17AM +0800, Sui Jingfeng wrote: > On 2024/5/2 16:32, Andy Shevchenko wrote: > > On Wed, May 01, 2024 at 12:27:14AM +0800, Sui Jingfeng wrote: > > > On 2024/4/30 22:13, Andy Shevchenko wrote: > > > > On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote: ... > > > > the former might be subdivided to "is it swnode backed or real fwnode > > > > one?" > > > > > > > Yeah, > > > On non-DT cases, it can be subdivided to swnode backed case and ACPI > > > fwnode backed case. > > > > > > - For swnode backed case: the device_get_match_data() don't has a > > > implemented backend. > > > - For ACPI fwnode backed case: the device_get_match_data() has a > > > implemented backend. > > > > > > But the driver has *neither* software node support > > True. > > > > > nor ACPI support, > > Not true. > > Why this is not true? Are you means that the panel-ilitek-ili9341 driver has > ACPI support? That's the idea (as far as I see the copy of the code from tinyDRM), but broken over the copy'n'paste. This patch rectifies that to be in align with the original code, which *does* support ACPI. > I'm asking because I don't see struct acpi_device_id related stuff in that > source file, > am I miss something? Yes, you are. I leave it for you to research. > > So, slow down and take your time to get into the code and understand how it > > works. > > > > > so that the rotation property can not get and ili9341_dpi_probe() will > > > fails. > > > So in total, this is not a 100% correct use of device property APIs. > > > > > > But I'm fine that if you want to leave(ignore) those less frequent use > > > cases temporarily, > > > there may have programmers want to post patches, to complete the missing > > > in the future. > > > > > > So, there do have some gains on non-DT cases. > > > > > > - As you make it be able to compiled on X86 with the drm-misc-defconfig. > > > - You cleanup the code up (at least patch 2 in this series is no > > > obvious problem). > > > - You allow people to modprobe it, and maybe half right and half > > > undefined. > > > > > > But you do helps moving something forward, so congratulations for the > > > wake up. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 0/3] drm/panel: ili9341: Obvious fixes
On Thu, May 02, 2024 at 09:43:42AM +0200, Neil Armstrong wrote: > Hi, > > On Thu, 25 Apr 2024 17:26:16 +0300, Andy Shevchenko wrote: > > A few obvious fixes to the driver. > > > > Andy Shevchenko (3): > > drm/panel: ili9341: Correct use of device property APIs > > drm/panel: ili9341: Respect deferred probe > > drm/panel: ili9341: Use predefined error codes > > > > [...] > > Thanks, Applied to https://gitlab.freedesktop.org/drm/misc/kernel.git > (drm-misc-fixes) > > [1/3] drm/panel: ili9341: Correct use of device property APIs > > https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/d43cd48ef1791801c61a54fade4a88d294dedf77 > [2/3] drm/panel: ili9341: Respect deferred probe > > https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/740fc1e0509be3f7e2207e89125b06119ed62943 > [3/3] drm/panel: ili9341: Use predefined error codes > > https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/da85f0aaa9f21999753b01d45c0343f885a8f905 Thank you, Neil, appreciated! -- With Best Regards, Andy Shevchenko
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Thu, May 02, 2024 at 09:34:17AM +0200, Neil Armstrong wrote: > On 30/04/2024 11:34, Maxime Ripard wrote: ... > Anyway since the rant is finished I'll land the patches. Thank you all for the review and discussion! -- With Best Regards, Andy Shevchenko
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Wed, May 01, 2024 at 12:27:14AM +0800, Sui Jingfeng wrote: > On 2024/4/30 22:13, Andy Shevchenko wrote: > > On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote: ... > > the former might be subdivided to "is it swnode backed or real fwnode one?" > > > Yeah, > On non-DT cases, it can be subdivided to swnode backed case and ACPI fwnode > backed case. > > - For swnode backed case: the device_get_match_data() don't has a > implemented backend. > - For ACPI fwnode backed case: the device_get_match_data() has a implemented > backend. > > But the driver has *neither* software node support True. > nor ACPI support, Not true. So, slow down and take your time to get into the code and understand how it works. > so that the rotation property can not get and ili9341_dpi_probe() will fails. > So in total, this is not a 100% correct use of device property APIs. > > But I'm fine that if you want to leave(ignore) those less frequent use cases > temporarily, > there may have programmers want to post patches, to complete the missing in > the future. > > So, there do have some gains on non-DT cases. > > - As you make it be able to compiled on X86 with the drm-misc-defconfig. > - You cleanup the code up (at least patch 2 in this series is no obvious > problem). > - You allow people to modprobe it, and maybe half right and half undefined. > > But you do helps moving something forward, so congratulations for the wake up. -- With Best Regards, Andy Shevchenko
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Fri, Apr 26, 2024 at 04:43:18AM +0800, Sui Jingfeng wrote:
> On 2024/4/26 03:10, Andy Shevchenko wrote:
> > On Fri, Apr 26, 2024 at 02:08:16AM +0800, Sui Jingfeng wrote:
> > > On 2024/4/25 22:26, Andy Shevchenko wrote:
> > > > It seems driver missed the point of proper use of device property APIs.
> > > > Correct this by updating headers and calls respectively.
> > > You are using the 'seems' here exactly saying that you are not 100% sure.
To add here, "seems" is used to show that I have no knowledge on what was
the idea behind this implementation by the original author. Plus my knowledge
in the firmware node / device property APIs and use cases in Linux kernel.
> > > Please allow me to tell you the truth: This patch again has ZERO effect.
> > > It fix nothing. And this patch is has the risks to be wrong.
> > Huh?! Really, stop commenting the stuff you do not understand.
>
> I'm actually a professional display drivers developer at the downstream
> in the past, despite my contribution to upstream is less. But I believe
> that all panel driver developers know what I'm talking about. So please
> have take a look at my replies.
Okay.
> > > Simple because the "ili9341_probe() ---> ili9341_dbi_prob()" code path
> > > is DT dependent.
> > >
> > > First of all, the devm_of_find_backlight() is called in
> > > ili9341_dbi_probe()
> > > under *non-DT* environment, devm_of_find_backlight() is just a just a
> > > no-op and will return NULL. NULL is not an error code, so
> > > ili9341_dbi_probe()
> > > won't rage quit. But the several side effect is that the backlight will
> > > NOT works at all.
> > Is it a problem?
>
> Yes, it is.
>
> The core problem is that the driver you are modifying has *implicit*
> *dependency* on DT.
> The implicit dependency is due to the calling of devm_of_find_backlight().
> This function
> is a no-op under non-DT systems.
Okay.
> Therefore, before the devm_of_find_backlight() and
> the device_get_match_data() function can truly DT independent.
True for the first part, not true for the second.
> Removing the "OF" dependency just let the tigers run out from the jail.
>
> It is not really meant to targeting at you, but I thinks, all of drm_panel
> drivers
> that has the devm_of_find_backlight() invoked will suffer such concerns.
> In short, the reason is that the *implicit* *dependency* populates and
> the undefined behavior gets triggered.
Still no problem statement. My hardware works nicely on non-DT environment.
(And since it's Arduino-based one, I assume it will work on DT environments
the very same way.)
> I'm sure you know that device_get_match_data() is same with
> of_device_get_match_data()
> for DT based systems. For non DT based systems, device_get_match_data() is
> just *undefined*
> Note that ACPI is not in the scope of the discussion here, as all of the drm
> bridges and
> panels driver under drivers/gpu/drm/ hasn't the ACPI support yet.
This patch shows exactly how to bring back the ACPI support to one of them
(as it's done for tinyDRM cases).
> Therefore, at present,
> it safe to say that device_get_match_data() is *undefined* under no-DT
> environment.
This is not true.
> Removing the "OF" dependency hints to us that it allows the driver to be
> probed as a
> pure SPI device under non DT systems. When device_get_match_data() is called,
> it returns
> NULL to us now. As a result, the drm driver being modified will tears down.
>
> See bellow code snippet extracted frompanel-ilitek-ili9341.c:
>
>
> ```
> ili->conf = of_device_get_match_data(dev);
> if (!ili->conf) {
> dev_err(dev, "missing device configuration\n");
> return -ENODEV;
> }
> ```
>
> > > It is actually considered as fatal bug for *panels* if the backlight of
> > > it is not light up, at least the brightness of *won't* be able to adjust.
> > > What's worse, if there is no sane platform setup code at the firmware
> > > or boot loader stage to set a proper initial state. The screen is complete
> > > dark. Even though the itself panel is refreshing framebuffers, it can not
> > > be seen by human's eye. Simple because of no backlight.
> > Can you imagine that I may have different hardware that considered
> > this is non-fatal error?
> >
> Yes, I can imagine.
>
> I believe you have the hardware which make you patch correct to run
> in 99.9% of all cases. But as long as there one bug happened, you patch
> are going to be blamed.
>
> Because its your patch that open the door, b
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote:
> On 2024/4/26 03:12, Andy Shevchenko wrote:
> > On Fri, Apr 26, 2024 at 02:53:22AM +0800, Sui Jingfeng wrote:
> > > On 2024/4/26 02:08, Sui Jingfeng wrote:
...
> > Are you speaking to yourself? I'm totally lost.
> >
> > Please, if you want to give a constructive feedback, try to understand
> > the topic from different aspects and then clearly express it.
>
> OK,
>
> The previous email analysis the non-DT cases exhaustively, this email intend
> to
> demonstrate the more frequently use case.
>
> That is, in the *DT('OF')* based systems,
> device_get_match_data() is completely equivalent to
> of_device_get_match_data().
> So the net results of applying this patch are "no gains and no lost".
This is not true. It's only part of the cases, i.e. DT. So, I assume you meant
"So the net results of applying this patch are "no gains and no lost" in DT
case".
> Things will become clear if we divide the whole problem into two cases(DT and
> non-DT)
> to discuss, that's it. That's all I can tell.
Not really. non-DT cases can also be divided to "fwnode backed or not", and
the former might be subdivided to "is it swnode backed or real fwnode one?"
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 1/1] drm/ili9341: Remove the duplicative driver
On Mon, Apr 29, 2024 at 01:39:06PM +0200, Maxime Ripard wrote: > On Thu, Apr 25, 2024 at 06:04:50PM +0300, Andy Shevchenko wrote: > > On Thu, Apr 25, 2024 at 04:58:06PM +0200, Maxime Ripard wrote: > > > On Thu, Apr 25, 2024 at 03:42:07PM +0300, Andy Shevchenko wrote: > > > > First of all, the driver was introduced when it was already > > > > two drivers available for Ilitek 9341 panels. > > > > > > > > Second, the most recent (fourth!) driver has incorporated this one > > > > and hence, when enabled, it covers the provided functionality. > > > > > > > > Taking into account the above, remove duplicative driver and make > > > > maintenance and support eaiser for everybody. > > > > > > > > Also see discussion [1] for details about Ilitek 9341 duplication > > > > code. > > > > > > > > Link: https://lore.kernel.org/r/zxm9pg-53v4s8...@smile.fi.intel.com [1] > > > > Signed-off-by: Andy Shevchenko > > > > > > I think it should be the other way around and we should remove the > > > mipi-dbi handling from panel/panel-ilitek-ili9341.c > > > > Then please do it! I whining already for a few years about this. > > I have neither the hardware nor the interest to do so. Seems it looks > like you have plenty of the latter at least, I'm sure you'll find some > time to tackle this. Hmm... Since the use of Arduino part in panel IliTek 9341 is clarified in this thread, I won't use that, but I have no time to clean up the mess in DRM in the nearest future, sorry. And TBH it seems you, guys, know much better what you want. FYI: The drivers/gpu/drm/tiny/mi0283qt.c works for me (the plenty of the HW you referred to). TL;DR: consider this as a (bug/feature/cleanup) report. -- With Best Regards, Andy Shevchenko
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Fri, Apr 26, 2024 at 02:53:22AM +0800, Sui Jingfeng wrote: > On 2024/4/26 02:08, Sui Jingfeng wrote: Are you speaking to yourself? I'm totally lost. Please, if you want to give a constructive feedback, try to understand the topic from different aspects and then clearly express it. -- With Best Regards, Andy Shevchenko
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Fri, Apr 26, 2024 at 02:08:16AM +0800, Sui Jingfeng wrote: > On 2024/4/25 22:26, Andy Shevchenko wrote: > > It seems driver missed the point of proper use of device property APIs. > > Correct this by updating headers and calls respectively. > > You are using the 'seems' here exactly saying that you are not 100% sure. > > Please allow me to tell you the truth: This patch again has ZERO effect. > It fix nothing. And this patch is has the risks to be wrong. Huh?! Really, stop commenting the stuff you do not understand. > Simple because the "ili9341_probe() ---> ili9341_dbi_prob()" code path > is DT dependent. > > First of all, the devm_of_find_backlight() is called in ili9341_dbi_probe() > under *non-DT* environment, devm_of_find_backlight() is just a just a > no-op and will return NULL. NULL is not an error code, so ili9341_dbi_probe() > won't rage quit. But the several side effect is that the backlight will > NOT works at all. Is it a problem? > It is actually considered as fatal bug for *panels* if the backlight of > it is not light up, at least the brightness of *won't* be able to adjust. > What's worse, if there is no sane platform setup code at the firmware > or boot loader stage to set a proper initial state. The screen is complete > dark. Even though the itself panel is refreshing framebuffers, it can not > be seen by human's eye. Simple because of no backlight. Can you imagine that I may have different hardware that considered this is non-fatal error? > Second, the ili9341_dbi_probe() requires additional device properties to > be able to works very well on the rotation screen case. See the calling > of "device_property_read_u32(dev, "rotation", )" in > ili9341_dbi_probe() function. Yes, exactly, and how does it object the purpose of this patch? > Combine with those two factors, it is actually can conclude that the > panel-ilitek-ili9394 driver has the *implicit* dependency on 'OF'. > Removing the 'OF' dependency from its Kconfig just trigger the > leakage of such risks. What?! > My software node related patches can help to reduce part of the potential > risks, but it still need some extra work. And it is not landed yet. Your patch has nothing to do with this series. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] drm: fixed: Don't use "proxy" headers
On Mon, Apr 22, 2024 at 09:49:04PM +0300, Jani Nikula wrote: > On Mon, 22 Apr 2024, Andy Shevchenko > wrote: > > Update header inclusions to follow IWYU (Include What You Use) > > principle. > > > > Signed-off-by: Andy Shevchenko > > Assuming it builds, and nothing depends on other stuff from kernel.h via > drm_fixed.h, For the record, I have built-tested this via `make allyesconfig` on x86_64. > Reviewed-by: Jani Nikula Thank you! -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 0/3] drm/panel: ili9341: Obvious fixes
On Thu, Apr 25, 2024 at 05:26:16PM +0300, Andy Shevchenko wrote: > A few obvious fixes to the driver. Note, despite the desire of removal Adafruit support from this driver, the older (read: stable) kernels will need this. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] drm/ili9341: Remove the duplicative driver
On Thu, Apr 25, 2024 at 04:58:06PM +0200, Maxime Ripard wrote: > Hi, > > On Thu, Apr 25, 2024 at 03:42:07PM +0300, Andy Shevchenko wrote: > > First of all, the driver was introduced when it was already > > two drivers available for Ilitek 9341 panels. > > > > Second, the most recent (fourth!) driver has incorporated this one > > and hence, when enabled, it covers the provided functionality. > > > > Taking into account the above, remove duplicative driver and make > > maintenance and support eaiser for everybody. > > > > Also see discussion [1] for details about Ilitek 9341 duplication > > code. > > > > Link: https://lore.kernel.org/r/zxm9pg-53v4s8...@smile.fi.intel.com [1] > > Signed-off-by: Andy Shevchenko > > I think it should be the other way around and we should remove the > mipi-dbi handling from panel/panel-ilitek-ili9341.c Then please do it! I whining already for a few years about this. > It's basically two drivers glued together for no particular reason and > handling two very different use cases which just adds more complexity > than it needs to. > > And it's the only driver doing so afaik, so it's definitely not "least > surprise" compliant. -- With Best Regards, Andy Shevchenko
[PATCH v1 2/3] drm/panel: ili9341: Respect deferred probe
GPIO controller might not be available when driver is being probed.
There are plenty of reasons why, one of which is deferred probe.
Since GPIOs are optional, return any error code we got to the upper
layer, including deferred probe. With that in mind, use dev_err_probe()
in order to avoid spamming the logs.
Fixes: 5a04227326b0 ("drm/panel: Add ilitek ili9341 panel driver")
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
index 7584ddb0e441..24c74c56e564 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -715,11 +715,11 @@ static int ili9341_probe(struct spi_device *spi)
reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(reset))
- dev_err(dev, "Failed to get gpio 'reset'\n");
+ return dev_err_probe(dev, PTR_ERR(reset), "Failed to get gpio
'reset'\n");
dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
if (IS_ERR(dc))
- dev_err(dev, "Failed to get gpio 'dc'\n");
+ return dev_err_probe(dev, PTR_ERR(dc), "Failed to get gpio
'dc'\n");
if (!strcmp(id->name, "sf-tc240t-9370-t"))
return ili9341_dpi_probe(spi, dc, reset);
--
2.43.0.rc1.1336.g36b5255a03ac
[PATCH v1 3/3] drm/panel: ili9341: Use predefined error codes
In one case the -1 is returned which is quite confusing code for
the wrong device ID, in another the ret is returning instead of
plain 0 that also confusing as readed may ask the possible meaning
of positive codes, which are never the case there. Convert both
to use explicit predefined error codes to make it clear what's going
on there.
Fixes: 5a04227326b0 ("drm/panel: Add ilitek ili9341 panel driver")
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
index 24c74c56e564..b933380b7eb7 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -422,7 +422,7 @@ static int ili9341_dpi_prepare(struct drm_panel *panel)
ili9341_dpi_init(ili);
- return ret;
+ return 0;
}
static int ili9341_dpi_enable(struct drm_panel *panel)
@@ -726,7 +726,7 @@ static int ili9341_probe(struct spi_device *spi)
else if (!strcmp(id->name, "yx240qv29"))
return ili9341_dbi_probe(spi, dc, reset);
- return -1;
+ return -ENODEV;
}
static void ili9341_remove(struct spi_device *spi)
--
2.43.0.rc1.1336.g36b5255a03ac
[PATCH v1 0/3] drm/panel: ili9341: Obvious fixes
A few obvious fixes to the driver. Andy Shevchenko (3): drm/panel: ili9341: Correct use of device property APIs drm/panel: ili9341: Respect deferred probe drm/panel: ili9341: Use predefined error codes drivers/gpu/drm/panel/Kconfig| 2 +- drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 13 +++-- 2 files changed, 8 insertions(+), 7 deletions(-) -- 2.43.0.rc1.1336.g36b5255a03ac
[PATCH v1 1/3] drm/panel: ili9341: Correct use of device property APIs
It seems driver missed the point of proper use of device property APIs.
Correct this by updating headers and calls respectively.
Fixes: 5a04227326b0 ("drm/panel: Add ilitek ili9341 panel driver")
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/panel/Kconfig| 2 +-
drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 5 +++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index e54f6f5604ed..2d451820 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -177,7 +177,7 @@ config DRM_PANEL_ILITEK_IL9322
config DRM_PANEL_ILITEK_ILI9341
tristate "Ilitek ILI9341 240x320 QVGA panels"
- depends on OF && SPI
+ depends on SPI
select DRM_KMS_HELPER
select DRM_GEM_DMA_HELPER
depends on BACKLIGHT_CLASS_DEVICE
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
index 3574681891e8..7584ddb0e441 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -22,8 +22,9 @@
#include
#include
#include
+#include
#include
-#include
+#include
#include
#include
@@ -691,7 +692,7 @@ static int ili9341_dpi_probe(struct spi_device *spi, struct
gpio_desc *dc,
* Every new incarnation of this display must have a unique
* data entry for the system in this driver.
*/
- ili->conf = of_device_get_match_data(dev);
+ ili->conf = device_get_match_data(dev);
if (!ili->conf) {
dev_err(dev, "missing device configuration\n");
return -ENODEV;
--
2.43.0.rc1.1336.g36b5255a03ac
Re: [PATCH v2] software node: Implement device_get_match_data fwnode callback
On Thu, Apr 25, 2024 at 09:42:53PM +0800, Sui Jingfeng wrote: > On 2024/4/25 00:44, Andy Shevchenko wrote: > > On Wed, Apr 24, 2024 at 07:34:54PM +0300, Dmitry Baryshkov wrote: > > > On Wed, Apr 24, 2024 at 05:52:03PM +0300, Andy Shevchenko wrote: > > > > On Wed, Apr 24, 2024 at 04:34:39PM +0300, Dmitry Baryshkov wrote: > > > > > On Wed, 24 Apr 2024 at 16:11, Andy Shevchenko > > > > > wrote: > > > > > > On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote: > > > > > > > On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote: > > > > > > > > On 2024/4/23 21:28, Andy Shevchenko wrote: > > > > > > > > > On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote: ... > > > > > > > But let me throw an argument why this patch (or something > > > > > > > similar) looks > > > > > > > to be necessary. > > > > > > > > > > > > > > Both on DT and non-DT systems the kernel allows using the non-OF > > > > > > > based > > > > > > > matching. For the platform devices there is > > > > > > > platform_device_id-based > > > > > > > matching. > > > > > > > > > > > > > > Currently handling the data coming from such device_ids requires > > > > > > > using > > > > > > > special bits of code, e.g. > > > > > > > platform_get_device_id(pdev)->driver_data to > > > > > > > get the data from the platform_device_id. Having such codepaths > > > > > > > goes > > > > > > > against the goal of unifying DT and non-DT paths via generic > > > > > > > property / > > > > > > > fwnode code. > > > > > > > > > > > > > > As such, I support Sui's idea of being able to use > > > > > > > device_get_match_data > > > > > > > for non-DT, non-ACPI platform devices. > > > > > > I'm not sure I buy this. We have a special helpers based on the bus > > > > > > type to > > > > > > combine device_get_match_data() with the respective ID table > > > > > > crawling, see > > > > > > the SPI and I²C cases as the examples. > > > > > I was thinking that we might be able to deprecate these helpers and > > > > > always use device_get_match_data(). > > > > True, but that is orthogonal to swnode match_data support, right? > > > > There even was (still is?) a patch series to do something like a new > > > > member to struct device_driver (? don't remember) to achieve that. > > > Maybe the scenario was not properly described in the commit message, or > > > maybe I missed something. The usecase that I understood from the commit > > > message was to use instatiated i2c / spi devices, which means > > > i2c_device_id / spi_device_id. The commit message should describe why > > > the usecase requires using 'compatible' property and swnode. Ideally it > > > should describe how these devices are instantiated at the first place. > > Yep. I also do not clearly understand the use case and why we need to have > > a board file, because the swnodes all are about board files that we must not > > use for the new platforms. > > Would you like to tell us what's the 'board file'? > > I am asking because I can not understand those two words at all. > I'm really don't know what's the meanings of 'board file'. Hmm... This is very well established term meaning the hard coded platform description (you may consider that as "device tree" written in C inside the Linux kernel). There are plenty of legacy platforms still exist in the Linux kernel source tree, you may find examples, like (first comes to mind) arch/arm/mach-pxa/spitz.c. > Do you means that board file is something like the dts, or > somethings describe the stuff on the motherboard but outside > the CPU? > > Does the hardware IP core belong to the "board file"? > > Can we using more concrete vocabulary instead of the vague > vocabulary to communicate? Most of (I though 100% before this message) the Linux kernel developers _know_ this term, sorry that you maybe young enough :-) -- With Best Regards, Andy Shevchenko
[PATCH v1 1/1] drm/mipi-dbi: Add missing MODULE_DESCRIPTION()
The modpost script is not happy
WARNING: modpost: missing MODULE_DESCRIPTION() in
drivers/gpu/drm/drm_mipi_dbi.o
because there is a missing module description.
Add it to the module.
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/drm_mipi_dbi.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index daac649aabdb..ee6fa8185b13 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -1475,4 +1475,5 @@ EXPORT_SYMBOL(mipi_dbi_debugfs_init);
#endif
+MODULE_DESCRIPTION("MIPI Display Bus Interface (DBI) LCD controller support");
MODULE_LICENSE("GPL");
--
2.43.0.rc1.1336.g36b5255a03ac
[PATCH v1 1/1] drm/ili9341: Remove the duplicative driver
First of all, the driver was introduced when it was already
two drivers available for Ilitek 9341 panels.
Second, the most recent (fourth!) driver has incorporated this one
and hence, when enabled, it covers the provided functionality.
Taking into account the above, remove duplicative driver and make
maintenance and support eaiser for everybody.
Also see discussion [1] for details about Ilitek 9341 duplication
code.
Link: https://lore.kernel.org/r/zxm9pg-53v4s8...@smile.fi.intel.com [1]
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/tiny/Kconfig | 13 --
drivers/gpu/drm/tiny/Makefile | 1 -
drivers/gpu/drm/tiny/ili9341.c | 253 -
3 files changed, 267 deletions(-)
delete mode 100644 drivers/gpu/drm/tiny/ili9341.c
diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index f6889f649bc1..2ab07bd0bb44 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -134,19 +134,6 @@ config TINYDRM_ILI9225
If M is selected the module will be called ili9225.
-config TINYDRM_ILI9341
- tristate "DRM support for ILI9341 display panels"
- depends on DRM && SPI
- select DRM_KMS_HELPER
- select DRM_GEM_DMA_HELPER
- select DRM_MIPI_DBI
- select BACKLIGHT_CLASS_DEVICE
- help
- DRM driver for the following Ilitek ILI9341 panels:
- * YX240QV29-T 2.4" 240x320 TFT (Adafruit 2.4")
-
- If M is selected the module will be called ili9341.
-
config TINYDRM_ILI9486
tristate "DRM support for ILI9486 display panels"
depends on DRM && SPI
diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
index 76dde89a044b..37cc9b27e79d 100644
--- a/drivers/gpu/drm/tiny/Makefile
+++ b/drivers/gpu/drm/tiny/Makefile
@@ -10,7 +10,6 @@ obj-$(CONFIG_DRM_SIMPLEDRM) += simpledrm.o
obj-$(CONFIG_TINYDRM_HX8357D) += hx8357d.o
obj-$(CONFIG_TINYDRM_ILI9163) += ili9163.o
obj-$(CONFIG_TINYDRM_ILI9225) += ili9225.o
-obj-$(CONFIG_TINYDRM_ILI9341) += ili9341.o
obj-$(CONFIG_TINYDRM_ILI9486) += ili9486.o
obj-$(CONFIG_TINYDRM_MI0283QT) += mi0283qt.o
obj-$(CONFIG_TINYDRM_REPAPER) += repaper.o
diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c
deleted file mode 100644
index 47b61c3bf145..
--- a/drivers/gpu/drm/tiny/ili9341.c
+++ /dev/null
@@ -1,253 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * DRM driver for Ilitek ILI9341 panels
- *
- * Copyright 2018 David Lechner
- *
- * Based on mi0283qt.c:
- * Copyright 2016 Noralf Trønnes
- */
-
-#include
-#include
-#include
-#include
-#include
-#include
-
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-
-#define ILI9341_FRMCTR10xb1
-#define ILI9341_DISCTRL0xb6
-#define ILI9341_ETMOD 0xb7
-
-#define ILI9341_PWCTRL10xc0
-#define ILI9341_PWCTRL20xc1
-#define ILI9341_VMCTRL10xc5
-#define ILI9341_VMCTRL20xc7
-#define ILI9341_PWCTRLA0xcb
-#define ILI9341_PWCTRLB0xcf
-
-#define ILI9341_PGAMCTRL 0xe0
-#define ILI9341_NGAMCTRL 0xe1
-#define ILI9341_DTCTRLA0xe8
-#define ILI9341_DTCTRLB0xea
-#define ILI9341_PWRSEQ 0xed
-
-#define ILI9341_EN3GAM 0xf2
-#define ILI9341_PUMPCTRL 0xf7
-
-#define ILI9341_MADCTL_BGR BIT(3)
-#define ILI9341_MADCTL_MV BIT(5)
-#define ILI9341_MADCTL_MX BIT(6)
-#define ILI9341_MADCTL_MY BIT(7)
-
-static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
-struct drm_crtc_state *crtc_state,
-struct drm_plane_state *plane_state)
-{
- struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
- struct mipi_dbi *dbi = >dbi;
- u8 addr_mode;
- int ret, idx;
-
- if (!drm_dev_enter(pipe->crtc.dev, ))
- return;
-
- DRM_DEBUG_KMS("\n");
-
- ret = mipi_dbi_poweron_conditional_reset(dbidev);
- if (ret < 0)
- goto out_exit;
- if (ret == 1)
- goto out_enable;
-
- mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_OFF);
-
- mipi_dbi_command(dbi, ILI9341_PWCTRLB, 0x00, 0xc1, 0x30);
- mipi_dbi_command(dbi, ILI9341_PWRSEQ, 0x64, 0x03, 0x12, 0x81);
- mipi_dbi_command(dbi, ILI9341_DTCTRLA, 0x85, 0x00, 0x78);
- mipi_dbi_command(dbi, ILI9341_PWCTRLA, 0x39, 0x2c, 0x00, 0x34, 0x02);
- mipi_dbi_command(dbi, ILI9341_PUMPCTRL, 0x20);
- mipi_dbi_command(dbi, ILI9341_DTCTRLB, 0x00, 0x00);
-
- /* Power Control */
- mipi_dbi_command(dbi, ILI9341_PWCTRL1, 0x23);
- mipi_dbi_command(dbi, ILI9341_PWCTRL2, 0x10);
- /* VCOM */
- mipi_dbi_command(dbi, ILI9341
Re: [PATCH v2] software node: Implement device_get_match_data fwnode callback
On Wed, Apr 24, 2024 at 07:34:54PM +0300, Dmitry Baryshkov wrote: > On Wed, Apr 24, 2024 at 05:52:03PM +0300, Andy Shevchenko wrote: > > On Wed, Apr 24, 2024 at 04:34:39PM +0300, Dmitry Baryshkov wrote: > > > On Wed, 24 Apr 2024 at 16:11, Andy Shevchenko > > > wrote: > > > > On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote: > > > > > On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote: > > > > > > On 2024/4/23 21:28, Andy Shevchenko wrote: > > > > > > > On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote: ... > > > > > But let me throw an argument why this patch (or something similar) > > > > > looks > > > > > to be necessary. > > > > > > > > > > Both on DT and non-DT systems the kernel allows using the non-OF based > > > > > matching. For the platform devices there is platform_device_id-based > > > > > matching. > > > > > > > > > > Currently handling the data coming from such device_ids requires using > > > > > special bits of code, e.g. platform_get_device_id(pdev)->driver_data > > > > > to > > > > > get the data from the platform_device_id. Having such codepaths goes > > > > > against the goal of unifying DT and non-DT paths via generic property > > > > > / > > > > > fwnode code. > > > > > > > > > > As such, I support Sui's idea of being able to use > > > > > device_get_match_data > > > > > for non-DT, non-ACPI platform devices. > > > > > > > > I'm not sure I buy this. We have a special helpers based on the bus > > > > type to > > > > combine device_get_match_data() with the respective ID table crawling, > > > > see > > > > the SPI and I²C cases as the examples. > > > > > > I was thinking that we might be able to deprecate these helpers and > > > always use device_get_match_data(). > > > > True, but that is orthogonal to swnode match_data support, right? > > There even was (still is?) a patch series to do something like a new > > member to struct device_driver (? don't remember) to achieve that. > > Maybe the scenario was not properly described in the commit message, or > maybe I missed something. The usecase that I understood from the commit > message was to use instatiated i2c / spi devices, which means > i2c_device_id / spi_device_id. The commit message should describe why > the usecase requires using 'compatible' property and swnode. Ideally it > should describe how these devices are instantiated at the first place. Yep. I also do not clearly understand the use case and why we need to have a board file, because the swnodes all are about board files that we must not use for the new platforms. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2] software node: Implement device_get_match_data fwnode callback
On Wed, Apr 24, 2024 at 04:34:39PM +0300, Dmitry Baryshkov wrote: > On Wed, 24 Apr 2024 at 16:11, Andy Shevchenko > wrote: > > > > On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote: > > > On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote: > > > > On 2024/4/23 21:28, Andy Shevchenko wrote: > > > > > On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote: ... > > > But let me throw an argument why this patch (or something similar) looks > > > to be necessary. > > > > > > Both on DT and non-DT systems the kernel allows using the non-OF based > > > matching. For the platform devices there is platform_device_id-based > > > matching. > > > > > > Currently handling the data coming from such device_ids requires using > > > special bits of code, e.g. platform_get_device_id(pdev)->driver_data to > > > get the data from the platform_device_id. Having such codepaths goes > > > against the goal of unifying DT and non-DT paths via generic property / > > > fwnode code. > > > > > > As such, I support Sui's idea of being able to use device_get_match_data > > > for non-DT, non-ACPI platform devices. > > > > I'm not sure I buy this. We have a special helpers based on the bus type to > > combine device_get_match_data() with the respective ID table crawling, see > > the SPI and I²C cases as the examples. > > I was thinking that we might be able to deprecate these helpers and > always use device_get_match_data(). True, but that is orthogonal to swnode match_data support, right? There even was (still is?) a patch series to do something like a new member to struct device_driver (? don't remember) to achieve that. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2] software node: Implement device_get_match_data fwnode callback
On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote: > On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote: > > On 2024/4/23 21:28, Andy Shevchenko wrote: > > > On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote: ... > But let me throw an argument why this patch (or something similar) looks > to be necessary. > > Both on DT and non-DT systems the kernel allows using the non-OF based > matching. For the platform devices there is platform_device_id-based > matching. > > Currently handling the data coming from such device_ids requires using > special bits of code, e.g. platform_get_device_id(pdev)->driver_data to > get the data from the platform_device_id. Having such codepaths goes > against the goal of unifying DT and non-DT paths via generic property / > fwnode code. > > As such, I support Sui's idea of being able to use device_get_match_data > for non-DT, non-ACPI platform devices. I'm not sure I buy this. We have a special helpers based on the bus type to combine device_get_match_data() with the respective ID table crawling, see the SPI and I²C cases as the examples. -- With Best Regards, Andy Shevchenko
[PATCH v1 1/1] fbtft: seps525: Don't use "proxy" headers
Update header inclusions to follow IWYU (Include What You Use) principle. Signed-off-by: Andy Shevchenko --- drivers/staging/fbtft/fb_seps525.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/staging/fbtft/fb_seps525.c b/drivers/staging/fbtft/fb_seps525.c index 05882e2cde7f..46c257308b49 100644 --- a/drivers/staging/fbtft/fb_seps525.c +++ b/drivers/staging/fbtft/fb_seps525.c @@ -16,11 +16,10 @@ * GNU General Public License for more details. */ -#include -#include -#include -#include +#include #include +#include +#include #include "fbtft.h" -- 2.43.0.rc1.1336.g36b5255a03ac
Re: [PATCH v2] software node: Implement device_get_match_data fwnode callback
On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote:
> Because the software node backend of the fwnode API framework lacks an
> implementation for the .device_get_match_data function callback. This
> makes it difficult to use(and/or test) a few drivers that originates
Missing space before opening parenthesis.
> from DT world on the non-DT platform.
>
> Implement the .device_get_match_data fwnode callback, device drivers or
> platform setup codes are expected to provide a string property, named as
> "compatible", the value of this software node string property is used to
> match against the compatible entries in the of_device_id table.
Yep and again, how is this related? If you want to test a driver originating
from DT, you would probably want to have a DT (overlay) to be provided.
> This also helps to keep the three backends of the fwnode API aligned as
> much as possible, which is a fundamential step to make device driver
> OF-independent truely possible.
>
> Fixes: ffb42e64561e ("drm/tiny/repaper: Make driver OF-independent")
> Fixes: 5703d6ae9573 ("drm/tiny/st7735r: Make driver OF-independent")
How is it a fix?
> Closes: https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/
Yes, and then Reported-by, which is missing here.
> Cc: Andy Shevchenko
> Cc: Daniel Scally
> Cc: Heikki Krogerus
> Cc: Sakari Ailus
> Cc: Greg Kroah-Hartman
> Cc: "Rafael J. Wysocki"
Please, move these after the cutter '---' line (note you may have that line in
your local repo).
...
> +static const void *
> +software_node_get_match_data(const struct fwnode_handle *fwnode,
> + const struct device *dev)
> +{
> + struct swnode *swnode = to_swnode(fwnode);
> + const struct of_device_id *matches = dev->driver->of_match_table;
> + const char *val = NULL;
> + int ret;
> + ret = property_entry_read_string_array(swnode->node->properties,
> +"compatible", , 1);
And if there are more than one compatible provided?
> + if (ret < 0 || !val)
> + return NULL;
> + while (matches && matches->compatible[0]) {
First part of the conditional is invariant to the loop. Can be simply
matches = dev->driver->of_match_table;
if (!matches)
return NULL;
while (...)
> + if (!strcmp(matches->compatible, val))
> + return matches->data;
> +
> + matches++;
> + }
> +
> + return NULL;
> +}
--
With Best Regards,
Andy Shevchenko
[PATCH v1 1/1] drm: fixed: Don't use "proxy" headers
Update header inclusions to follow IWYU (Include What You Use)
principle.
Signed-off-by: Andy Shevchenko
---
include/drm/drm_fixed.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
index 81572d32db0c..387fb81d5b81 100644
--- a/include/drm/drm_fixed.h
+++ b/include/drm/drm_fixed.h
@@ -25,8 +25,9 @@
#ifndef DRM_FIXED_H
#define DRM_FIXED_H
-#include
#include
+#include
+#include
typedef union dfixed {
u32 full;
--
2.43.0.rc1.1336.g36b5255a03ac
Re: In kernel virtual HID devices (was Future handling of complex RGB devices on Linux v3)
On Mon, Mar 25, 2024 at 07:38:46PM +0100, Miguel Ojeda wrote: > On Mon, Mar 25, 2024 at 3:25 PM Hans de Goede wrote: > > > > +Cc: Bentiss, Jiri > > Cc'ing Andy and Geert as well who recently became the > maintainers/reviewers of auxdisplay, in case they are interested in > these threads (one of the initial solutions discussed in a past thread > a while ago was to extend auxdisplay). Without diving into this, just sharing my view on auxdisplay subsystem: I consider it _mostly_ (like lim->100% mathematically speaking) as for 7-segment and alike displays, not any comples RGB or so devices. If those devices are capable of representing characters/digits in similar way, we may export linedisp library for them to utilise. -- With Best Regards, Andy Shevchenko
Re: [PATCH] staging: fbtft: core: fix potential memory leak in fbtft_probe_common()
On Wed, Sep 28, 2022 at 02:23:01PM +0800, Jianglei Nie wrote:
> fbtft_probe_common() allocates a memory chunk for "info" with
> fbtft_framebuffer_alloc(). When "display->buswidth == 0" is true, the
> function returns without releasing the "info", which will lead to a
> memory leak.
>
> Fix it by calling fbtft_framebuffer_release() when "display->buswidth
> == 0" is true.
Fixes tag?
...
> if (display->buswidth == 0) {
> dev_err(dev, "buswidth is not set\n");
> + fbtft_framebuffer_release(info);
> return -EINVAL;
ret = dev_err_probe(dev, -EINVAL, "buswidth is not set\n");
goto out_release;
> }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] software node: Implement device_get_match_data fwnode callback
On Sat, Mar 23, 2024 at 02:30:08AM +0800, Sui Jingfeng wrote: > On 2024/3/23 02:16, Andy Shevchenko wrote: > > On Sat, Mar 23, 2024 at 02:12:14AM +0800, Sui Jingfeng wrote: > > > On 2024/3/23 02:05, Andy Shevchenko wrote: > > > >Besides that, the kernel project rule is "we do not add > > > > the dead (unused) code". > > > This rule is good an correct and I admit. > > > > > > But the problem is that it is chicken-and-egg problem, > > > it probably have at least two user now. > > Then show them! Convert in the same series and show that. > > I believe that Vladimir has show enough to you. I have read that thread, > I think Vladimit have explained very well. Let's continue there. I replied there just now. -- With Best Regards, Andy Shevchenko
Re: [PATCH] software node: Implement device_get_match_data fwnode callback
On Sat, Mar 23, 2024 at 02:12:14AM +0800, Sui Jingfeng wrote: > On 2024/3/23 02:05, Andy Shevchenko wrote: > > Besides that, the kernel project rule is "we do not add > > the dead (unused) code". > > This rule is good an correct and I admit. > > But the problem is that it is chicken-and-egg problem, > it probably have at least two user now. Then show them! Convert in the same series and show that. > it's possible that it will gain more users in the future. > > But if you reject everybody from now, then it is zero. As a no-user patch, yes, I reject this. -- With Best Regards, Andy Shevchenko
Re: [PATCH] software node: Implement device_get_match_data fwnode callback
On Sat, Mar 23, 2024 at 01:43:56AM +0800, Sui Jingfeng wrote:
> On 2024/3/23 00:14, Andy Shevchenko wrote:
> > On Fri, Mar 22, 2024 at 05:00:05PM +0800, Sui Jingfeng wrote:
> > > On 2024/3/21 04:28, Andy Shevchenko wrote:
...
> > > > > > > By replacing it with device_get_match_data() and creating a
> > > > > > > software
> > > > > > > graph that mimics the OF graph, everything else works fine,
> > > > > > > except that
> > > > > > > there isn't an out-of-box replacement for the
> > > > > > > of_device_get_match_data()
> > > > > > > function. Because the software node backend of the fwnode
> > > > > > > framework lacks
> > > > > > > an implementation for the device_get_match_data callback.
> > > > > > .device_get_match_data
> > > > > >
> > > > > > > Implement device_get_match_data fwnode callback fwnode callback
> > > > > > > to fill
> > > > > > .device_get_match_data
> > > > > OK, thanks a lot.
> > > > >
> > > > > > > this gap. Device drivers or platform setup codes are expected to
> > > > > > > provide
> > > > > > > a "compatible" string property. The value of this string property
> > > > > > > is used
> > > > > > > to match against the compatible entries in the of_device_id
> > > > > > > table. Which
> > > > > > > is consistent with the original usage style.
> > > > > > Why do you need to implement the graph in the board file?
> > > > > It can be inside the chip, there is no clear cut.\
> > > > Which chip? Flash memory / ROM or you meant something like FPGA here?
> > > > For the latter there is another discussion on how to use DT overlays
> > > > in ACPI-enabled environments for the FPGA configurations.
> > > There are some hardware resource or software entity is created on the
> > > driver runtime. But DT or DT overlays are compiled before device driver
> > > get loaded. GPIO-emulated-I2C is just an example, this is kind of driver
> > > level knowledge on the runtime. With the GPIO or programmable some
> > > hardware IP unit, device driver authors can change the connection
> > > relationship
> > > at their will at the runtime. While with DT, every thing has to be sure
> > > before the compile time.
> > >
> > > DT overlays can be a alternative solution, but this doesn't conflict with
> > > this patch. This patch won't assume how device drives go to use it, and
> > > allow device driver creating device instead enumerating by DT. In one
> > > word: "flexibility".
> > Software nodes in general for the device driver / platform quirks.
>
> The real problem is that we probably shouldn't make an assumption
> how does the user is going to use the infrastructure, right?
>
> You could say it is *mostly* for quirks or whatever, Like the
> ./drivers/i2c/busses/i2c-cht-wc.c. But software nodes *can* also
> be something else.
>
> Can we stop restricting its usage by limited understanding or someone
> personal judgement?
Please, try to research the topic before calling it 'personal judgement'.
59abd83672f7 ("drivers: base: Introducing software nodes to the firmware node
framework")
(Read the first paragraph carefully.)
Let's say it's not personal, it's by design. Extending this to cover more needs
a good justification. I do not see a such.
> A workaround or quirk may be enough for some corner usage. Vladimir is also
> encounter similar problem, right?
> > They are not designed for what you are talking about here.
>
> I have never hint anything about any real applications, the materials
> and/or talk given here is just for example purpose.
>
> What we are doing here is to keep the three back-ends aligned.
>
>
> > Consider using SSDT / DT overlays instead.
> >
> NAK,
>
> When developers are doing task 'A' , reviewers ask them to do task 'B'.
> And when developers doing task 'B', reviewers then recommend that the tool
> 'C' is a better alternative.
> ...
> ...
>
> This is not good.
>
>
> As I have read the lengthy thread in link [1] as you pointed to me.
>
> The boring coding review is just as the following scheme:
>
> 1) Asking details about what they do with software nodes impolitely.
> 2) Wasting time to talk about irreverent things by brute force.
> 3) Tell everybody that software nodes are not designed for what you
> application.
> 4) Recommending DT overlays or something else.
>
> Again, this is non-technical discussion, the time being wasting is not
> worthwhile.
> And the judgements being given is irrelevant to the *patch itself*.
The patch tries to tight the driver data to the device description provided by
a software node, which is 100% equivalent to the legacy board files which we
do NOT want to have. Besides that, the kernel project rule is "we do not add
the dead (unused) code".
I believe these two is quite enough to NAK patch.
You may come with a better explanation AND a user of this in the same series.
People at least can see your use case.
> [1] https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] software node: Implement device_get_match_data fwnode callback
On Fri, Mar 22, 2024 at 05:00:05PM +0800, Sui Jingfeng wrote: > On 2024/3/21 04:28, Andy Shevchenko wrote: ... > > > > > By replacing it with device_get_match_data() and creating a software > > > > > graph that mimics the OF graph, everything else works fine, except > > > > > that > > > > > there isn't an out-of-box replacement for the > > > > > of_device_get_match_data() > > > > > function. Because the software node backend of the fwnode framework > > > > > lacks > > > > > an implementation for the device_get_match_data callback. > > > > .device_get_match_data > > > > > > > > > Implement device_get_match_data fwnode callback fwnode callback to > > > > > fill > > > > .device_get_match_data > > > OK, thanks a lot. > > > > > > > > this gap. Device drivers or platform setup codes are expected to > > > > > provide > > > > > a "compatible" string property. The value of this string property is > > > > > used > > > > > to match against the compatible entries in the of_device_id table. > > > > > Which > > > > > is consistent with the original usage style. > > > > Why do you need to implement the graph in the board file? > > > It can be inside the chip, there is no clear cut.\ > > Which chip? Flash memory / ROM or you meant something like FPGA here? > > For the latter there is another discussion on how to use DT overlays > > in ACPI-enabled environments for the FPGA configurations. > > There are some hardware resource or software entity is created on the > driver runtime. But DT or DT overlays are compiled before device driver > get loaded. GPIO-emulated-I2C is just an example, this is kind of driver > level knowledge on the runtime. With the GPIO or programmable some > hardware IP unit, device driver authors can change the connection relationship > at their will at the runtime. While with DT, every thing has to be sure > before the compile time. > > DT overlays can be a alternative solution, but this doesn't conflict with > this patch. This patch won't assume how device drives go to use it, and > allow device driver creating device instead enumerating by DT. In one > word: "flexibility". Software nodes in general for the device driver / platform quirks. They are not designed for what you are talking about here. Consider using SSDT / DT overlays instead. -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/5] drm/bridge: simple-bridge: Extend match support for non-DT based systems
On Tue, Jan 23, 2024 at 12:32:17AM +0800, Sui Jingfeng wrote: > Which is intended to be used on non-DT environment, where the simple-bridge > platform device is created by either the display controller driver side or > platform firmware subsystem. To avoid duplication and to keep consistent, > we choose to reuse the OF match tables. Because the potentional user may > not has a of_node attached, nor a ACPI match id. If this is the case, > a software node string property can be provide to fill the niche. ... > - sbridge->info = of_device_get_match_data(>dev); > + if (pdev->dev.of_node) > + sbridge->info = of_device_get_match_data(>dev); > + else > + sbridge->info = simple_bridge_get_match_data(>dev); This is wrong. Just use device_get_match_data() instead of of_ counter part. The rest, if required, has to be addressed elsewhere. So, formal NAK for the changes like above. -- With Best Regards, Andy Shevchenko
Re: [PATCH 5/5] drm-bridge: display-connector: Switch to use fwnode API
On Tue, Jan 23, 2024 at 03:20:26AM +0200, Laurent Pinchart wrote: > On Tue, Jan 23, 2024 at 12:32:20AM +0800, Sui Jingfeng wrote: ... > > conn->bridge.of_node = pdev->dev.of_node; > > + conn->bridge.fwnode = pdev->dev.fwnode; > > This goes in the right direction. Let's address the other drivers and > drop the OF-based calls in the same series :-) +1. BUT, please use device_set_node() instead of both lines. -- With Best Regards, Andy Shevchenko
Re: [PATCH] software node: Implement device_get_match_data fwnode callback
On Thu, Mar 21, 2024 at 03:22:05AM +0800, Sui Jingfeng wrote: > On 2024/3/20 18:39, Andy Shevchenko wrote: > > On Tue, Mar 19, 2024 at 07:42:22AM +0800, Sui Jingfeng wrote: > > > This makes it possible to support (and/or test) a few drivers that > > > originates from DT World on the x86-64 platform. Originally, those > > > drivers using the of_device_get_match_data() function to get match > > > data. For example, drivers/gpu/drm/bridge/simple-bridge.c and > > > drivers/gpu/drm/bridge/display-connector.c. Those drivers works very > > > well in the DT world, however, there is no counterpart to > > > of_device_get_match_data() when porting them to the x86 platform, > > > because x86 CPUs lack DT support. > > This is not true. > > > > First of all, there is counter part that called device_get_match_data(). > > Are you means that the acpi_fwnode_device_get_match_data() implementation? > As the fwnode API framework has three backend: OF, ACPI, and software node. > If you are hinting me that the acpi backend has the .device_get_match_data > implemented. Then you are right. Yes, for all firmware property providers there is a callback. > > Second, there *is* DT support for the _selected_ x86 based platforms. > > Yeah, you maybe right again here. I guess you means that some special > hardware or platform may have a *limited* support? > > Can you pointed it out for study of learning purpose? Point to what? This arch/x86/kernel/devicetree.c ? > To speak precisely, there are some drm display bridges drivers are > lack of the DT support on X86. Those display bridges belong to the > device drivers catalogs. Do they support Device Tree? Do you want to enable them in ACPI environment? > OK, I will update my commit message at the next version if possible, > and try my best to describe the problem precisely. Please do. > > > By replacing it with device_get_match_data() and creating a software > > > graph that mimics the OF graph, everything else works fine, except that > > > there isn't an out-of-box replacement for the of_device_get_match_data() > > > function. Because the software node backend of the fwnode framework lacks > > > an implementation for the device_get_match_data callback. > > .device_get_match_data > > > > > Implement device_get_match_data fwnode callback fwnode callback to fill > > .device_get_match_data > > OK, thanks a lot. > > > > this gap. Device drivers or platform setup codes are expected to provide > > > a "compatible" string property. The value of this string property is used > > > to match against the compatible entries in the of_device_id table. Which > > > is consistent with the original usage style. > > Why do you need to implement the graph in the board file? > > It can be inside the chip, there is no clear cut.\ Which chip? Flash memory / ROM or you meant something like FPGA here? For the latter there is another discussion on how to use DT overlays in ACPI-enabled environments for the FPGA configurations. > I means that > the graph(including fwnode graph, OF graph or swnode graph) can > be used at anywhere. The examples given here may lead you to > think it is board specific, but it is not limited to board specific. > > fwnode graph, OF graph and swnode graph, all of them are implements > of the graph. Its common that different hardware vendors bought the > some IP and has been integrated it into their SoC. So it can be inside > of the chip if you want *code sharing*. > > > Back to the patch itself, we want to keep the three backends aligned as much > as possible. Is this reasonable enough? Yes, but it misses details about board files approach. See also above. ... > > Have you seen this discussion? > > https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/ > > I really didn't have seen that thread before this patch is sent, > I'm a graphic developer, I'm mainly focus on graphics domain. > > Previously, I have implemented similar functionality at the drivers > layer [1][2]. But as the instances grows, I realized there is a > risk to introducing *boilerplate*. So I send this patch. [1][2] can > be drop if this patch could be merged. > > [1] https://patchwork.freedesktop.org/patch/575414/?series=129040=1 > > [2] https://patchwork.freedesktop.org/patch/575411/?series=129040=1 > > > After a brief skim, I guess we encounter similar problems. Oops! > In a nutshell, there is a need to *emulation* on X86 platform, > to suit the need of device-driver coding style of DT world. What does "emulation" mean? Can you elaborate a bit? > Besides, at the swnode backend layer, we should not call > fwnode_property_read_string(), instead, we should usethe > property_entry_read_string_array() function. Because the > fwnode_property_read_string() is belong to upper layer. > While backend implementations should call functions from > bottom layer only. -- With Best Regards, Andy Shevchenko
Re: [PATCH] software node: Implement device_get_match_data fwnode callback
+Cc: Vladimir On Tue, Mar 19, 2024 at 07:42:22AM +0800, Sui Jingfeng wrote: > This makes it possible to support (and/or test) a few drivers that > originates from DT World on the x86-64 platform. Originally, those > drivers using the of_device_get_match_data() function to get match > data. For example, drivers/gpu/drm/bridge/simple-bridge.c and > drivers/gpu/drm/bridge/display-connector.c. Those drivers works very > well in the DT world, however, there is no counterpart to > of_device_get_match_data() when porting them to the x86 platform, > because x86 CPUs lack DT support. This is not true. First of all, there is counter part that called device_get_match_data(). Second, there *is* DT support for the _selected_ x86 based platforms. > By replacing it with device_get_match_data() and creating a software > graph that mimics the OF graph, everything else works fine, except that > there isn't an out-of-box replacement for the of_device_get_match_data() > function. Because the software node backend of the fwnode framework lacks > an implementation for the device_get_match_data callback. .device_get_match_data > Implement device_get_match_data fwnode callback fwnode callback to fill .device_get_match_data > this gap. Device drivers or platform setup codes are expected to provide > a "compatible" string property. The value of this string property is used > to match against the compatible entries in the of_device_id table. Which > is consistent with the original usage style. Why do you need to implement the graph in the board file? ... Have you seen this discussion? https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/ -- With Best Regards, Andy Shevchenko
Re: [PATCH] backlight: mp3309c: fix signedness bug in mp3309c_parse_fwnode()
On Sat, Mar 16, 2024 at 12:45:27PM +0300, Dan Carpenter wrote: > The "num_levels" variable is used to store error codes from > device_property_count_u32() so it needs to be signed. This doesn't > cause an issue at runtime because devm_kcalloc() won't allocate negative > sizes. However, it's still worth fixing. Agree. Reviewed-by: Andy Shevchenko -- With Best Regards, Andy Shevchenko
[PATCH v1 1/1] drm/gma500: Remove unused intel-mid.h
intel-mid.h is providing some core parts of the South Complex PM, which are usually are not used by individual drivers. In particular, this driver doesn't use it, so simply remove the unused header. Signed-off-by: Andy Shevchenko --- drivers/gpu/drm/gma500/oaktrail_lvds.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/gma500/oaktrail_lvds.c b/drivers/gpu/drm/gma500/oaktrail_lvds.c index d974d0c60d2a..72191d6f0d06 100644 --- a/drivers/gpu/drm/gma500/oaktrail_lvds.c +++ b/drivers/gpu/drm/gma500/oaktrail_lvds.c @@ -11,8 +11,6 @@ #include #include -#include - #include #include #include -- 2.43.0.rc1.1.gbec44491f096
[PATCH v1 1/1] drm/msm/hdmi: Replace of_gpio.h by proper one
of_gpio.h is deprecated and subject to remove. The driver doesn't use it directly, replace it with what is really being used. Signed-off-by: Andy Shevchenko --- drivers/gpu/drm/msm/hdmi/hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c index c8ebd75176bb..24abcb7254cc 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c @@ -5,8 +5,8 @@ * Author: Rob Clark */ +#include #include -#include #include #include -- 2.43.0.rc1.1.gbec44491f096
Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
On Thu, Feb 29, 2024 at 12:21:34PM -0600, Lucas De Marchi wrote: > On Thu, Feb 29, 2024 at 12:49:57PM +0200, Andy Shevchenko wrote: > > On Wed, Feb 28, 2024 at 05:39:21PM -0600, Lucas De Marchi wrote: > > > On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote: ... > > > I build-tested this in x86-64, x86-32 and arm64. I didn't like much the > > > need to fork the __GENMASK() implementation on the 2 sides of the ifdef > > > since I think the GENMASK_INPUT_CHECK() should be the one covering the > > > input checks. However to make it common we'd need to solve 2 problems: > > > the casts and the sizeof. The sizeof can be passed as arg to > > > __GENMASK(), however the casts I think would need a __CAST_U8(x) > > > or the like and sprinkle it everywhere, which would hurt readability. > > > Not pretty. Or go back to the original submission and make it less > > > horrible :-/ > > > > I'm wondering if we can use _Generic() approach here. > > in assembly? Yes. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
On Wed, Feb 28, 2024 at 05:39:21PM -0600, Lucas De Marchi wrote: > On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote: > > On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote: > > > On Wed, Feb 21, 2024 at 11:04:22PM +0200, Andy Shevchenko wrote: > > > > On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote: > > > > > On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi > > > > > wrote: ... > I build-tested this in x86-64, x86-32 and arm64. I didn't like much the > need to fork the __GENMASK() implementation on the 2 sides of the ifdef > since I think the GENMASK_INPUT_CHECK() should be the one covering the > input checks. However to make it common we'd need to solve 2 problems: > the casts and the sizeof. The sizeof can be passed as arg to > __GENMASK(), however the casts I think would need a __CAST_U8(x) > or the like and sprinkle it everywhere, which would hurt readability. > Not pretty. Or go back to the original submission and make it less > horrible :-/ I'm wondering if we can use _Generic() approach here. ... > > #define GENMASK_INPUT_CHECK(h, l) 0 > > +#define __GENMASK(t, h, l) \ > > + ((~0 - (1 << (l)) + 1) & (~0 >> (BITS_PER_LONG - 1 - (h > > humn... this builds, but does it work if GENMASK_ULL() is used in > assembly? That BITS_PER_LONG does not match the type width. UL()/ULL() macros are not just for fun. -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init
Thu, Feb 22, 2024 at 03:58:37PM +0100, Marek Behún kirjoitti:
> A few drivers are doing resource-managed mutex initialization by
> implementing ad-hoc one-liner mutex dropping functions and using them
> with devm_add_action_or_reset(). Help drivers avoid these repeated
> one-liners by adding managed version of mutex initialization.
>
> Use the new function devm_mutex_init() in the following drivers:
> drivers/gpio/gpio-pisosr.c
> drivers/gpio/gpio-sim.c
> drivers/gpu/drm/xe/xe_hwmon.c
> drivers/hwmon/nzxt-smart2.c
> drivers/leds/leds-is31fl319x.c
> drivers/power/supply/mt6370-charger.c
> drivers/power/supply/rt9467-charger.c
Pardon me, but why?
https://lore.kernel.org/linux-leds/20231214173614.2820929-1-gnst...@salutedevices.com/
Can you cooperate, folks, instead of doing something independently?
> --- a/include/linux/devm-helpers.h
> +++ b/include/linux/devm-helpers.h
> @@ -24,6 +24,8 @@
> */
>
> #include
> +#include
> +#include
> #include
>
> static inline void devm_delayed_work_drop(void *res)
> @@ -76,4 +78,34 @@ static inline int devm_work_autocancel(struct device *dev,
> return devm_add_action(dev, devm_work_drop, w);
> }
>
> +static inline void devm_mutex_drop(void *res)
> +{
> + mutex_destroy(res);
> +}
> +
> +/**
> + * devm_mutex_init - Resource managed mutex initialization
> + * @dev: Device which lifetime mutex is bound to
> + * @lock:Mutex to be initialized (and automatically destroyed)
> + *
> + * Initialize mutex which is automatically destroyed when driver is detached.
> + * A few drivers initialize mutexes which they want destroyed before driver
> is
> + * detached, for debugging purposes.
> + * devm_mutex_init() can be used to omit the explicit mutex_destroy() call
> when
> + * driver is detached.
> + */
> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> + mutex_init(lock);
> +
> + /*
> + * mutex_destroy() is an empty function if CONFIG_DEBUG_MUTEXES is
> + * disabled. No need to allocate an action in that case.
> + */
> + if (IS_ENABLED(CONFIG_DEBUG_MUTEXES))
> + return devm_add_action_or_reset(dev, devm_mutex_drop, lock);
> + else
> + return 0;
> +}
Cc: George Stark
--
With Best Regards,
Andy Shevchenko
Re: Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote: > On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote: ... > +#define __GENMASK(t, h, l) \ > + ((~0 - (1 << (l)) + 1) & (~0 >> (BITS_PER_LONG - 1 - (h What's wrong on using the UL/ULL() macros? Also it would be really good to avoid bifurcation of the implementations of __GENMASK() for both cases. ... > -#define __GENMASK(h, l) \ > - (((~UL(0)) - (UL(1) << (l)) + 1) & \ > - (~UL(0) >> (BITS_PER_LONG - 1 - (h This at bare minimum can be left untouched for asm case, no? -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
On Wed, Feb 21, 2024 at 11:06:10PM +0200, Andy Shevchenko wrote: > On Wed, Feb 21, 2024 at 10:37:30PM +0200, Dmitry Baryshkov wrote: > > On Wed, 21 Feb 2024 at 22:30, Dmitry Baryshkov > > wrote: ... > > Excuse me, it seems a c from gitlab didn't work as expected. > > No problem, it's clear that the patch has not been properly tested and > obviously wrong. Has to be dropped. If somebody wants that, it will > be material for v6.10 cycle. ...which makes me think that bitmap(bitops) lack of assembly test case(s). -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
On Wed, Feb 21, 2024 at 10:37:30PM +0200, Dmitry Baryshkov wrote: > On Wed, 21 Feb 2024 at 22:30, Dmitry Baryshkov > wrote: ... > Excuse me, it seems a c from gitlab didn't work as expected. No problem, it's clear that the patch has not been properly tested and obviously wrong. Has to be dropped. If somebody wants that, it will be material for v6.10 cycle. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote: > On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi wrote: ... > > +#define BITS_PER_TYPE(type)(sizeof(type) * BITS_PER_BYTE) Can sizeof() be used in assembly? ... > > -#define __GENMASK(h, l) \ > > - (((~UL(0)) - (UL(1) << (l)) + 1) & \ > > -(~UL(0) >> (BITS_PER_LONG - 1 - (h > > -#define GENMASK(h, l) \ > > - (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l)) > > +#define __GENMASK(t, h, l) \ > > + (GENMASK_INPUT_CHECK(h, l) + \ > > +(((t)~0ULL - ((t)(1) << (l)) + 1) & \ > > +((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h) Nevertheless, the use ~0ULL is not proper assembly, this broke initial implementation using UL() / ULL(). > > -#define __GENMASK_ULL(h, l) \ > > - (((~ULL(0)) - (ULL(1) << (l)) + 1) & \ > > -(~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h > > -#define GENMASK_ULL(h, l) \ > > - (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l)) Ditto. > > +#define GENMASK(h, l) __GENMASK(unsigned long, h, l) > > +#define GENMASK_ULL(h, l) __GENMASK(unsigned long long, h, l) > > +#define GENMASK_U8(h, l) __GENMASK(u8, h, l) > > +#define GENMASK_U16(h, l) __GENMASK(u16, h, l) > > +#define GENMASK_U32(h, l) __GENMASK(u32, h, l) > > +#define GENMASK_U64(h, l) __GENMASK(u64, h, l) > > This breaks drm-tip on arm64 architecture: > > arch/arm64/kernel/entry-fpsimd.S: Assembler messages: > 465arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')' > 466arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here > 467arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')' > 468arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here > 469arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')' > 470arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here > 471arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')' > 472arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here > 473arch/arm64/kernel/entry-fpsimd.S:271: Error: unexpected characters > following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned > long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned > long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)' > 474arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here > 475arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')' > 476arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here > 477arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')' > 478arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here > 479arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')' > 480arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here > 481arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')' > 482arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here > 483arch/arm64/kernel/entry-fpsimd.S:282: Error: unexpected characters > following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned > long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned > long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)' > 484arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 01/10] backlight: Match backlight device against struct fb_info.bl_dev
On Wed, Feb 21, 2024 at 5:45 PM Thomas Zimmermann wrote: > Am 21.02.24 um 15:34 schrieb Andy Shevchenko: > > On Wed, Feb 21, 2024 at 10:41:28AM +0100, Thomas Zimmermann wrote: > >> Framebuffer drivers for devices with dedicated backlight are supposed > >> to set struct fb_info.bl_dev to the backlight's respective device. Use > >> the value to match backlight and framebuffer in the backlight core code. ... > >> if (!bd->ops) > >> goto out; > >> -if (bd->ops->check_fb && !bd->ops->check_fb(bd, evdata->info)) > >> +else if (bd->ops->check_fb && !bd->ops->check_fb(bd, info)) > > What's the point of adding redundant 'else'? > > > >> goto out; > >> +#if IS_ENABLED(CONFIG_FB_BACKLIGHT) > >> +else if (info->bl_dev && info->bl_dev != bd) > > Ditto. > > They group these tests into one single block of code; signaling that > these tests serve the same purpose. Commit message has nothing about this. Also if needed, it should be a separate change. > >> +goto out; > >> +#endif -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 02/10] auxdisplay/ht16k33: Remove struct backlight_ops.check_fb
On Wed, Feb 21, 2024 at 10:41:29AM +0100, Thomas Zimmermann wrote: > The driver sets struct fb_info.bl_dev to the correct backlight > device. Thus rely on the backlight core code to match backlight > and framebuffer devices, and remove the extra check_fb function > from struct backlight_ops. check_fb() Acked-by: Andy Shevchenko -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 01/10] backlight: Match backlight device against struct fb_info.bl_dev
On Wed, Feb 21, 2024 at 10:41:28AM +0100, Thomas Zimmermann wrote: > Framebuffer drivers for devices with dedicated backlight are supposed > to set struct fb_info.bl_dev to the backlight's respective device. Use > the value to match backlight and framebuffer in the backlight core code. ... > if (!bd->ops) > goto out; > - if (bd->ops->check_fb && !bd->ops->check_fb(bd, evdata->info)) > + else if (bd->ops->check_fb && !bd->ops->check_fb(bd, info)) What's the point of adding redundant 'else'? > goto out; > +#if IS_ENABLED(CONFIG_FB_BACKLIGHT) > + else if (info->bl_dev && info->bl_dev != bd) Ditto. > + goto out; > +#endif -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 0/3] backlight: mp3309c: Allow to use on non-OF platforms
On Fri, Feb 09, 2024 at 07:50:52AM +, Lee Jones wrote: > On Thu, 08 Feb 2024, Andy Shevchenko wrote: > > On Thu, Feb 08, 2024 at 06:14:55PM +, Lee Jones wrote: > > > On Thu, 08 Feb 2024, Andy Shevchenko wrote: > > > > On Thu, Feb 08, 2024 at 05:39:46PM +, Lee Jones wrote: > > > > > On Thu, 08 Feb 2024, Andy Shevchenko wrote: > > > > > > On Thu, Feb 08, 2024 at 11:34:25AM +, Lee Jones wrote: > > > > > > > On Thu, 01 Feb 2024, Andy Shevchenko wrote: ... > > > > > > > > backlight: mp3309c: Utilise temporary variable for struct > > > > > > > > device > > > > > > > > > > > > (1) > > > > > > > > > > > > > Set no longer applies. Please rebase, thanks. > > > > > > > > > > > > I got a contradictory messages: > > > > > > 1) email that says that all had been applied; > > > > > > 2) this email (that tells the complete opposite); > > > > > > 3) the repository where the first two were applied. > > > > > > > > > > > > While you can amend your scripts, I think I need to rebase only the > > > > > > last patch > > > > > > > > > > This is what I assume happened: > > > > > > > > > > 1. Attempted to apply the set (as a set) > > > > > 2. 2 commits applied cleanly > > > > > 3. The final commit conflicted > > > > > > > > Which is really strange. I have just applied (with b4) on top of your > > > > changes > > > > and no complains so far. > > > > > > > > $ git am > > > > ./v2_20240201_andriy_shevchenko_backlight_mp3309c_allow_to_use_on_non_of_platforms.mbx > > > > Applying: backlight: mp3309c: Make use of device properties > > > > Applying: backlight: mp3309c: use dev_err_probe() instead of dev_err() > > > > Applying: backlight: mp3309c: Utilise temporary variable for struct > > > > device > > > > > > > > Can you show what b4 tells you about this? > > > > > > Fetching patch(es) > > > Analyzing 14 messages in the thread > > > Checking attestation on all messages, may take a moment... > > > --- > > > ✓ [PATCH v2 1/3] backlight: mp3309c: Make use of device properties > > > + Reviewed-by: Daniel Thompson (✓ > > > DKIM/linaro.org) > > > + Link: > > > https://lore.kernel.org/r/20240201151537.367218-2-andriy.shevche...@linux.intel.com > > > + Signed-off-by: Lee Jones > > > ✓ [PATCH v2 2/3] backlight: mp3309c: use dev_err_probe() instead of > > > dev_err() > > > + Tested-by: Flavio Suligoi (✗ DKIM/asem.it) > > > + Reviewed-by: Daniel Thompson (✓ > > > DKIM/linaro.org) > > > + Link: > > > https://lore.kernel.org/r/20240201151537.367218-3-andriy.shevche...@linux.intel.com > > > + Signed-off-by: Lee Jones > > > ✓ [PATCH v2 3/3] backlight: mp3309c: Utilise temporary variable for > > > struct device > > > + Link: > > > https://lore.kernel.org/r/20240201151537.367218-4-andriy.shevche...@linux.intel.com > > > + Signed-off-by: Lee Jones > > > --- > > > ✓ Signed: DKIM/intel.com (From: andriy.shevche...@linux.intel.com) > > > --- > > > Total patches: 3 > > > Prepared a fake commit range for 3-way merge (672ecc5199b5..d507b9f4c5b9) > > > --- > > > Link: > > > https://lore.kernel.org/r/20240201151537.367218-1-andriy.shevche...@linux.intel.com > > > Base: not specified > > > > > > Running through checkpatch.pl > > > total: 0 errors, 0 warnings, 103 lines checked > > > > > > "[PATCH v2 1/3] backlight: mp3309c: Make use of device properties" has no > > > obvious style problems and is ready for submission. > > > total: 0 errors, 0 warnings, 41 lines checked > > > > > > "[PATCH v2 2/3] backlight: mp3309c: use dev_err_probe() instead of" has > > > no obvious style problems and is ready for submission. > > > total: 0 errors, 0 warnings, 81 lines checked > > > > > > "[PATCH v2 3/3] backlight: mp3309c: Utilise temporary variable for" has > > > no obvious style problems and is ready for submission. > > > > > > Check the results (hit return to continue or Ctrl+c to exit) > > > > > > >
Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
On Thu, Feb 08, 2024 at 08:48:59PM +0100, Andi Shyti wrote: > On Wed, Feb 07, 2024 at 11:45:19PM -0800, Lucas De Marchi wrote: ... > > Signed-off-by: Yury Norov > > Signed-off-by: Lucas De Marchi > > Acked-by: Jani Nikula > > Lucas' SoB should be at the bottom here. In any case, nice patch: And it's at the bottom (among SoB lines), there is no violation with Submitting Patches. -- With Best Regards, Andy Shevchenko
