Re: Re: [PATCH v1] pinctrl: ralink: rt2880: Check for return value of devm_kcalloc()
On Fri, Apr 1, 2022 at 6:06 AM unSimple wrote: > > The main consideration of the 'continue' in the patch is that those > statements are inner a loop. > > The next allocation may be successful so I think it is better to use > 'continue' here. Please, do not top-post! What you explained is logical from APIs point of view, what I was asking is about functional point of view. Why do you think the skipping iteration is fine? You need to explain this in the code/commit message. > At 2022-03-29 19:45:50, "Andy Shevchenko" wrote: > >On Tue, Mar 29, 2022 at 11:36 AM QintaoShen wrote: > >> > >> The memory allocation function devm_kcalloc() may return NULL pointer, > > > >may --> might > > > >> so it is better to add a check for 'p->func[i]->pins' to avoid possible > >> NULL pointer dereference. ... > >> @@ -266,6 +266,10 @@ static int rt2880_pinmux_pins(struct rt2880_priv *p) > >> p->func[i]->pin_count, > >> sizeof(int), > >> GFP_KERNEL); > > > >> + > > > >Stray change. Also it seems it has trailing space character(s). > > > >> +if (!p->func[i]->pins) > > > >> +continue; > > > >Why is 'continue' the proper choice here? No clarification is given in > >the commit message. > > > >> for (j = 0; j < p->func[i]->pin_count; j++) > >> p->func[i]->pins[j] = p->func[i]->pin_first + j; -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1] pinctrl: ralink: rt2880: Check for return value of devm_kcalloc()
On Tue, Mar 29, 2022 at 11:36 AM QintaoShen wrote: > > The memory allocation function devm_kcalloc() may return NULL pointer, may --> might > so it is better to add a check for 'p->func[i]->pins' to avoid possible > NULL pointer dereference. ... > @@ -266,6 +266,10 @@ static int rt2880_pinmux_pins(struct rt2880_priv *p) > p->func[i]->pin_count, > sizeof(int), > GFP_KERNEL); > + Stray change. Also it seems it has trailing space character(s). > +if (!p->func[i]->pins) > +continue; Why is 'continue' the proper choice here? No clarification is given in the commit message. > for (j = 0; j < p->func[i]->pin_count; j++) > p->func[i]->pins[j] = p->func[i]->pin_first + j; -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [driver-core:driver-core-testing 28/31] WARNING: modpost: vmlinux.o(.text.unlikely+0x156c): Section mismatch in reference from the function bitmap_equal() to the variable .init.data:initcall_level
On Sat, Aug 14, 2021 at 4:36 PM Greg Kroah-Hartman wrote: > On Sat, Aug 14, 2021 at 07:03:00PM +0800, kernel test robot wrote: > > tree: > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git > > driver-core-testing > > head: 3b35f2a6a625126c57475aa56b5357d8e80b404c > > commit: 291f93ca339f5b5e6e90ad037bb8271f0f618165 [28/31] lib: test_bitmap: > > add bitmap_print_bitmask/list_to_buf test cases > > config: xtensa-randconfig-r004-20210814 (attached as .config) > > compiler: xtensa-linux-gcc (GCC) 11.2.0 > > reproduce (this is a W=1 build): > > wget > > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O > > ~/bin/make.cross > > chmod +x ~/bin/make.cross > > # > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commit/?id=291f93ca339f5b5e6e90ad037bb8271f0f618165 > > git remote add driver-core > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git > > git fetch --no-tags driver-core driver-core-testing > > git checkout 291f93ca339f5b5e6e90ad037bb8271f0f618165 > > # save the attached .config to linux build tree > > mkdir build_dir > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross > > O=build_dir ARCH=xtensa SHELL=/bin/bash > > > > If you fix the issue, kindly add following tag as appropriate > > Reported-by: kernel test robot > > > > All warnings (new ones prefixed by >>, old ones prefixed by <<): > > > > >> WARNING: modpost: vmlinux.o(.text.unlikely+0x156c): Section mismatch in > > >> reference from the function bitmap_equal() to the variable > > >> .init.data:initcall_level_names > > The function bitmap_equal() references > > the variable __initdata initcall_level_names. > > This is often because bitmap_equal lacks a __initdata > > annotation or the annotation of initcall_level_names is wrong. > > > > The below error/warnings are from parent commit: > > << WARNING: modpost: vmlinux.o(.data+0x1a86d8): Section mismatch in > > reference from the variable qed_mfw_legacy_maps to the variable > > .init.rodata:__setup_str_initcall_blacklist > > << WARNING: modpost: vmlinux.o(.data+0x1a87c8): Section mismatch in > > reference from the variable qed_mfw_ext_maps to the variable > > .init.rodata:__setup_str_initcall_blacklist > > << WARNING: modpost: vmlinux.o(.data+0x1a8948): Section mismatch in > > reference from the variable qede_forced_speed_maps to the variable > > .init.rodata:__setup_str_initcall_blacklist > Barry, can I get a fix for this? Max already pointed out, but I guess you were not in Cc list, that it's a GCC bug in his opinion, but GCC people don't ack it. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92938 -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: atomisp: reduce kernel stack usage
On Fri, Feb 26, 2021 at 03:05:02PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> The atomisp_set_fmt() function has multiple copies of the large
> v4l2_format structure on its stack, resulting in a warning about
> excessive stack usage in some rare randconfig builds.
>
> drivers/staging/media/atomisp/pci/atomisp_cmd.c:5600:5: error: stack frame
> size of 1084 bytes in function 'atomisp_set_fmt'
> [-Werror,-Wframe-larger-than=]
>
> Of this structure, only three members in the 'fmt.pix' member are
> used, so simplify it by using the smaller v4l2_pix_format structure
> directly. This reduces the stack usage to 612 bytes, and it could
> be reduced further by only storing the three members that are used.
Good to me!
Reviewed-by: Andy Shevchenko
> Signed-off-by: Arnd Bergmann
> ---
> .../staging/media/atomisp/pci/atomisp_cmd.c | 65 +--
> .../staging/media/atomisp/pci/atomisp_cmd.h | 2 +-
> .../staging/media/atomisp/pci/atomisp_ioctl.c | 2 +-
> 3 files changed, 33 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> index 592ea990d4ca..3192c0716eb9 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> @@ -4837,7 +4837,7 @@ static void __atomisp_init_stream_info(u16 stream_index,
> }
>
> /* This function looks up the closest available resolution. */
> -int atomisp_try_fmt(struct video_device *vdev, struct v4l2_format *f,
> +int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f,
> bool *res_overflow)
> {
> struct atomisp_device *isp = video_get_drvdata(vdev);
> @@ -4859,18 +4859,18 @@ int atomisp_try_fmt(struct video_device *vdev, struct
> v4l2_format *f,
> return -EINVAL;
>
> stream_index = atomisp_source_pad_to_stream_id(asd, source_pad);
> - fmt = atomisp_get_format_bridge(f->fmt.pix.pixelformat);
> + fmt = atomisp_get_format_bridge(f->pixelformat);
> if (!fmt) {
> dev_err(isp->dev, "unsupported pixelformat!\n");
> fmt = atomisp_output_fmts;
> }
>
> - if (f->fmt.pix.width <= 0 || f->fmt.pix.height <= 0)
> + if (f->width <= 0 || f->height <= 0)
> return -EINVAL;
>
> snr_mbus_fmt->code = fmt->mbus_code;
> - snr_mbus_fmt->width = f->fmt.pix.width;
> - snr_mbus_fmt->height = f->fmt.pix.height;
> + snr_mbus_fmt->width = f->width;
> + snr_mbus_fmt->height = f->height;
>
> __atomisp_init_stream_info(stream_index, stream_info);
>
> @@ -4892,7 +4892,7 @@ int atomisp_try_fmt(struct video_device *vdev, struct
> v4l2_format *f,
> return -EINVAL;
> }
>
> - f->fmt.pix.pixelformat = fmt->pixelformat;
> + f->pixelformat = fmt->pixelformat;
>
> /*
>* If the format is jpeg or custom RAW, then the width and height will
> @@ -4900,17 +4900,17 @@ int atomisp_try_fmt(struct video_device *vdev, struct
> v4l2_format *f,
>* the below conditions. So just assign to what is being returned from
>* the sensor driver.
>*/
> - if (f->fmt.pix.pixelformat == V4L2_PIX_FMT_JPEG ||
> - f->fmt.pix.pixelformat == V4L2_PIX_FMT_CUSTOM_M10MO_RAW) {
> - f->fmt.pix.width = snr_mbus_fmt->width;
> - f->fmt.pix.height = snr_mbus_fmt->height;
> + if (f->pixelformat == V4L2_PIX_FMT_JPEG ||
> + f->pixelformat == V4L2_PIX_FMT_CUSTOM_M10MO_RAW) {
> + f->width = snr_mbus_fmt->width;
> + f->height = snr_mbus_fmt->height;
> return 0;
> }
>
> - if (snr_mbus_fmt->width < f->fmt.pix.width
> - && snr_mbus_fmt->height < f->fmt.pix.height) {
> - f->fmt.pix.width = snr_mbus_fmt->width;
> - f->fmt.pix.height = snr_mbus_fmt->height;
> + if (snr_mbus_fmt->width < f->width
> + && snr_mbus_fmt->height < f->height) {
> + f->width = snr_mbus_fmt->width;
> + f->height = snr_mbus_fmt->height;
> /* Set the flag when resolution requested is
>* beyond the max value supported by sensor
>*/
> @@ -4919,12 +4919,10 @@ int atomisp_try_fmt(struct video_device *vdev, struct
> v4l2_format *f,
> }
>
> /* app vs isp */
> - f->fmt.pix.width = rounddown(
> -clamp_t(u32, f->fmt.pix.width,
Re: [PATCH v12] staging: fbtft: add tearing signal detect
On Tue, Feb 2, 2021 at 3:52 AM Carlis wrote: > On Mon, 1 Feb 2021 19:40:21 +0200 > Andy Shevchenko wrote: > > > On Sat, Jan 30, 2021 at 8:39 AM carlis wrote: > > > On Fri, 29 Jan 2021 16:26:12 +0200 > > > Andy Shevchenko wrote: > > > > On Fri, Jan 29, 2021 at 3:56 PM carlis > > > > wrote: > > > > > On Fri, 29 Jan 2021 12:23:08 +0200 > > > > > Andy Shevchenko wrote: > > > > ... > > > > > > > Hi, I apologize for what I said in the previous two emails. I > > > > > missed one email you sent before, and now I probably understand > > > > > what you meant. Here is a version I modified according to your > > > > > suggestion: > > > > I have realized that you are mocking stuff in the generic fbtft > > structure for all drivers while only a single one is going to use > > that. Consider moving everything to the driver in question. >Do you mean that i define the TE completion and irq_te in the >fb_st7789v.c as i did before? Not in global variables. Perhaps it will require to add/update the custom (to the specific driver) data structure. But the idea is that all changes should be isolated to that driver. -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 3/9] sfi: Remove framework for deprecated firmware
SFI-based platforms are gone. So does this framework. This removes mention of SFI through the drivers and other code as well. Signed-off-by: Andy Shevchenko Reviewed-by: Hans de Goede Acked-by: Linus Walleij --- Documentation/ABI/testing/sysfs-firmware-sfi | 15 - Documentation/ABI/testing/sysfs-platform-kim | 2 +- MAINTAINERS | 7 - arch/x86/Kconfig | 7 +- arch/x86/include/asm/intel-mid.h | 37 -- arch/x86/include/asm/intel_scu_ipc_legacy.h | 58 +- arch/x86/include/asm/platform_sst_audio.h | 2 - arch/x86/kernel/apic/io_apic.c| 4 +- arch/x86/kernel/setup.c | 2 - arch/x86/pci/mmconfig-shared.c| 6 +- arch/x86/platform/Makefile| 1 - arch/x86/platform/intel-mid/Makefile | 5 - .../platform/intel-mid/device_libs/Makefile | 23 - .../intel-mid/device_libs/platform_bcm43xx.c | 101 .../intel-mid/device_libs/platform_bma023.c | 16 - .../intel-mid/device_libs/platform_bt.c | 101 .../intel-mid/device_libs/platform_emc1403.c | 39 -- .../device_libs/platform_gpio_keys.c | 81 --- .../intel-mid/device_libs/platform_lis331.c | 37 -- .../intel-mid/device_libs/platform_max7315.c | 77 --- .../intel-mid/device_libs/platform_mpu3050.c | 32 -- .../device_libs/platform_mrfld_pinctrl.c | 39 -- .../device_libs/platform_mrfld_rtc.c | 44 -- .../intel-mid/device_libs/platform_mrfld_sd.c | 43 -- .../device_libs/platform_mrfld_spidev.c | 50 -- .../device_libs/platform_pcal9555a.c | 95 .../intel-mid/device_libs/platform_tc35876x.c | 42 -- .../intel-mid/device_libs/platform_tca6416.c | 53 -- arch/x86/platform/intel-mid/intel-mid.c | 1 - arch/x86/platform/intel-mid/sfi.c | 419 -- arch/x86/platform/sfi/Makefile| 2 - arch/x86/platform/sfi/sfi.c | 100 drivers/Makefile | 2 +- drivers/platform/x86/intel_scu_pcidrv.c | 22 +- drivers/sfi/Kconfig | 18 - drivers/sfi/Makefile | 4 - drivers/sfi/sfi_acpi.c| 214 --- drivers/sfi/sfi_core.c| 522 -- drivers/sfi/sfi_core.h| 81 --- include/linux/sfi.h | 210 --- include/linux/sfi_acpi.h | 93 init/main.c | 2 - 42 files changed, 14 insertions(+), 2695 deletions(-) delete mode 100644 Documentation/ABI/testing/sysfs-firmware-sfi delete mode 100644 arch/x86/platform/intel-mid/device_libs/Makefile delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_bcm43xx.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_bma023.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_bt.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_emc1403.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_gpio_keys.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_lis331.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_max7315.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_mrfld_pinctrl.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_mrfld_rtc.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_mrfld_sd.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_mrfld_spidev.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_tc35876x.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_tca6416.c delete mode 100644 arch/x86/platform/intel-mid/sfi.c delete mode 100644 arch/x86/platform/sfi/Makefile delete mode 100644 arch/x86/platform/sfi/sfi.c delete mode 100644 drivers/sfi/Kconfig delete mode 100644 drivers/sfi/Makefile delete mode 100644 drivers/sfi/sfi_acpi.c delete mode 100644 drivers/sfi/sfi_core.c delete mode 100644 drivers/sfi/sfi_core.h delete mode 100644 include/linux/sfi.h delete mode 100644 include/linux/sfi_acpi.h diff --git a/Documentation/ABI/testing/sysfs-firmware-sfi b/Documentation/ABI/testing/sysfs-firmware-sfi deleted file mode 100644 index 5210e0f06ddb.. --- a/Documentation/ABI/testing/sysfs-firmware-sfi +++ /dev/null @@ -1,15 +0,0 @@ -What: /sys/firmware/sfi/tables/ -Date: May 2010 -Contact: Len Brown -Description: - SFI defines a number of small static memory tables - so the kernel can get platform information from firmware. - - The tables are defined in the latest SFI
[PATCH v1 8/9] x86/platform/intel-mid: Remove unused header inclusion in intel-mid.h
After the commit f1be6cdaf57c ("x86/platform/intel-mid: Make
intel_scu_device_register() static") the platform_device.h is not being
used anymore by intel-mid.h. Remove it.
Signed-off-by: Andy Shevchenko
---
arch/x86/include/asm/intel-mid.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/x86/include/asm/intel-mid.h b/arch/x86/include/asm/intel-mid.h
index c2c7da4c60cf..c043e91f45ad 100644
--- a/arch/x86/include/asm/intel-mid.h
+++ b/arch/x86/include/asm/intel-mid.h
@@ -8,7 +8,6 @@
#define _ASM_X86_INTEL_MID_H
#include
-#include
extern int intel_mid_pci_init(void);
extern int intel_mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t
state);
--
2.30.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 6/9] x86/platform/intel-mid: Get rid of intel_scu_ipc_legacy.h
The header is used by a single user. Move header content to that user.
Signed-off-by: Andy Shevchenko
Reviewed-by: Mika Westerberg
---
arch/x86/include/asm/intel_scu_ipc.h| 2 --
arch/x86/include/asm/intel_scu_ipc_legacy.h | 18 --
arch/x86/platform/intel-mid/intel-mid.c | 7 +--
3 files changed, 5 insertions(+), 22 deletions(-)
delete mode 100644 arch/x86/include/asm/intel_scu_ipc_legacy.h
diff --git a/arch/x86/include/asm/intel_scu_ipc.h
b/arch/x86/include/asm/intel_scu_ipc.h
index 11d457af68c5..8537f597d20a 100644
--- a/arch/x86/include/asm/intel_scu_ipc.h
+++ b/arch/x86/include/asm/intel_scu_ipc.h
@@ -65,6 +65,4 @@ static inline int intel_scu_ipc_dev_command(struct
intel_scu_ipc_dev *scu, int c
inlen, out, outlen);
}
-#include
-
#endif
diff --git a/arch/x86/include/asm/intel_scu_ipc_legacy.h
b/arch/x86/include/asm/intel_scu_ipc_legacy.h
deleted file mode 100644
index fa529e5ec142..
--- a/arch/x86/include/asm/intel_scu_ipc_legacy.h
+++ /dev/null
@@ -1,18 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _ASM_X86_INTEL_SCU_IPC_LEGACY_H_
-#define _ASM_X86_INTEL_SCU_IPC_LEGACY_H_
-
-#include
-
-#define IPCMSG_COLD_OFF0x80/* Only for Tangier */
-#define IPCMSG_COLD_RESET 0xF1
-
-/* Don't call these in new code - they will be removed eventually */
-
-/* Issue commands to the SCU with or without data */
-static inline int intel_scu_ipc_simple_command(int cmd, int sub)
-{
- return intel_scu_ipc_dev_simple_command(NULL, cmd, sub);
-}
-
-#endif
diff --git a/arch/x86/platform/intel-mid/intel-mid.c
b/arch/x86/platform/intel-mid/intel-mid.c
index 2c044e260b4b..846b2ded39d9 100644
--- a/arch/x86/platform/intel-mid/intel-mid.c
+++ b/arch/x86/platform/intel-mid/intel-mid.c
@@ -29,6 +29,9 @@
#include
#include
+#define IPCMSG_COLD_OFF0x80/* Only for Tangier */
+#define IPCMSG_COLD_RESET 0xF1
+
enum intel_mid_cpu_type __intel_mid_cpu_chip;
EXPORT_SYMBOL_GPL(__intel_mid_cpu_chip);
@@ -38,12 +41,12 @@ static void intel_mid_power_off(void)
intel_mid_pwr_power_off();
/* Only for Tangier, the rest will ignore this command */
- intel_scu_ipc_simple_command(IPCMSG_COLD_OFF, 1);
+ intel_scu_ipc_dev_simple_command(NULL, IPCMSG_COLD_OFF, 1);
};
static void intel_mid_reboot(void)
{
- intel_scu_ipc_simple_command(IPCMSG_COLD_RESET, 0);
+ intel_scu_ipc_dev_simple_command(NULL, IPCMSG_COLD_RESET, 0);
}
static void __init intel_mid_time_init(void)
--
2.30.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 7/9] x86/platform/intel-mid: Drop unused __intel_mid_cpu_chip and Co.
Since there is no more user of this global variable and associated custom API,
we may safely drop this legacy reinvented a wheel from the kernel sources.
Signed-off-by: Andy Shevchenko
---
arch/x86/include/asm/intel-mid.h| 23 ---
arch/x86/platform/intel-mid/intel-mid.c | 17 -
2 files changed, 40 deletions(-)
diff --git a/arch/x86/include/asm/intel-mid.h b/arch/x86/include/asm/intel-mid.h
index 6306fe3e5c4a..c2c7da4c60cf 100644
--- a/arch/x86/include/asm/intel-mid.h
+++ b/arch/x86/include/asm/intel-mid.h
@@ -21,36 +21,13 @@ extern void intel_mid_pwr_power_off(void);
extern int intel_mid_pwr_get_lss_id(struct pci_dev *pdev);
-/*
- * Medfield is the follow-up of Moorestown, it combines two chip solution into
- * one. Other than that it also added always-on and constant tsc and lapic
- * timers. Medfield is the platform name, and the chip name is called Penwell
- * we treat Medfield/Penwell as a variant of Moorestown. Penwell can be
- * identified via MSRs.
- */
-enum intel_mid_cpu_type {
- /* 1 was Moorestown */
- INTEL_MID_CPU_CHIP_PENWELL = 2,
- INTEL_MID_CPU_CHIP_CLOVERVIEW,
- INTEL_MID_CPU_CHIP_TANGIER,
-};
-
-extern enum intel_mid_cpu_type __intel_mid_cpu_chip;
-
#ifdef CONFIG_X86_INTEL_MID
-static inline enum intel_mid_cpu_type intel_mid_identify_cpu(void)
-{
- return __intel_mid_cpu_chip;
-}
-
extern void intel_scu_devices_create(void);
extern void intel_scu_devices_destroy(void);
#else /* !CONFIG_X86_INTEL_MID */
-#define intel_mid_identify_cpu() 0
-
static inline void intel_scu_devices_create(void) { }
static inline void intel_scu_devices_destroy(void) { }
diff --git a/arch/x86/platform/intel-mid/intel-mid.c
b/arch/x86/platform/intel-mid/intel-mid.c
index 846b2ded39d9..2802b5e4637b 100644
--- a/arch/x86/platform/intel-mid/intel-mid.c
+++ b/arch/x86/platform/intel-mid/intel-mid.c
@@ -32,9 +32,6 @@
#define IPCMSG_COLD_OFF0x80/* Only for Tangier */
#define IPCMSG_COLD_RESET 0xF1
-enum intel_mid_cpu_type __intel_mid_cpu_chip;
-EXPORT_SYMBOL_GPL(__intel_mid_cpu_chip);
-
static void intel_mid_power_off(void)
{
/* Shut down South Complex via PWRMU */
@@ -58,29 +55,15 @@ static void __init intel_mid_time_init(void)
static void intel_mid_arch_setup(void)
{
- if (boot_cpu_data.x86 != 6) {
- pr_err("Unknown Intel MID CPU (%d:%d), default to Penwell\n",
- boot_cpu_data.x86, boot_cpu_data.x86_model);
- __intel_mid_cpu_chip = INTEL_MID_CPU_CHIP_PENWELL;
- goto out;
- }
-
switch (boot_cpu_data.x86_model) {
- case 0x35:
- __intel_mid_cpu_chip = INTEL_MID_CPU_CHIP_CLOVERVIEW;
- break;
case 0x3C:
case 0x4A:
- __intel_mid_cpu_chip = INTEL_MID_CPU_CHIP_TANGIER;
x86_platform.legacy.rtc = 1;
break;
- case 0x27:
default:
- __intel_mid_cpu_chip = INTEL_MID_CPU_CHIP_PENWELL;
break;
}
-out:
/*
* Intel MID platforms are using explicitly defined regulators.
*
--
2.30.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 5/9] x86/PCI: Describe @reg for type1_access_ok()
Describe missed parameter in documentation of type1_access_ok(). Otherwise "make W=1 arch/x86/pci/" produces the following warning: CHECK arch/x86/pci/intel_mid_pci.c CC arch/x86/pci/intel_mid_pci.o arch/x86/pci/intel_mid_pci.c:152: warning: Function parameter or member 'reg' not described in 'type1_access_ok' Signed-off-by: Andy Shevchenko --- arch/x86/pci/intel_mid_pci.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c index 938a8b7bfe7f..8edd62206604 100644 --- a/arch/x86/pci/intel_mid_pci.c +++ b/arch/x86/pci/intel_mid_pci.c @@ -142,6 +142,7 @@ static int pci_device_update_fixed(struct pci_bus *bus, unsigned int devfn, * type1_access_ok - check whether to use type 1 * @bus: bus number * @devfn: device & function in question + * @reg: configuration register offset * * If the bus is on a Lincroft chip and it exists, or is not on a Lincroft at * all, the we can go ahead with any reads & writes. If it's on a Lincroft, -- 2.30.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 9/9] x86/platform/intel-mid: Update Copyright year and drop file names
Update Copyright year and drop file names from files themselves. Signed-off-by: Andy Shevchenko --- arch/x86/include/asm/intel-mid.h| 4 ++-- arch/x86/platform/intel-mid/intel-mid.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/x86/include/asm/intel-mid.h b/arch/x86/include/asm/intel-mid.h index c043e91f45ad..c201083b34f6 100644 --- a/arch/x86/include/asm/intel-mid.h +++ b/arch/x86/include/asm/intel-mid.h @@ -1,8 +1,8 @@ /* SPDX-License-Identifier: GPL-2.0-only */ /* - * intel-mid.h: Intel MID specific setup code + * Intel MID specific setup code * - * (C) Copyright 2009 Intel Corporation + * (C) Copyright 2009, 2021 Intel Corporation */ #ifndef _ASM_X86_INTEL_MID_H #define _ASM_X86_INTEL_MID_H diff --git a/arch/x86/platform/intel-mid/intel-mid.c b/arch/x86/platform/intel-mid/intel-mid.c index 2802b5e4637b..f4592dc7a1c1 100644 --- a/arch/x86/platform/intel-mid/intel-mid.c +++ b/arch/x86/platform/intel-mid/intel-mid.c @@ -1,8 +1,8 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * intel-mid.c: Intel MID platform setup code + * Intel MID platform setup code * - * (C) Copyright 2008, 2012 Intel Corporation + * (C) Copyright 2008, 2012, 2021 Intel Corporation * Author: Jacob Pan (jacob.jun@intel.com) * Author: Sathyanarayanan Kuppuswamy */ -- 2.30.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 1/9] media: atomisp: Remove unused header
sfi.h is not anyhow used by the driver. Remove it. Signed-off-by: Andy Shevchenko Acked-by: Sakari Ailus Acked-by: Linus Walleij --- drivers/staging/media/atomisp/include/linux/atomisp_platform.h | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/staging/media/atomisp/include/linux/atomisp_platform.h b/drivers/staging/media/atomisp/include/linux/atomisp_platform.h index 5a5121d958ed..8c65733e0255 100644 --- a/drivers/staging/media/atomisp/include/linux/atomisp_platform.h +++ b/drivers/staging/media/atomisp/include/linux/atomisp_platform.h @@ -22,7 +22,6 @@ #include #include -#include #include #include "atomisp.h" -- 2.30.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 2/9] cpufreq: sfi-cpufreq: Remove driver for deprecated firmware
SFI-based platforms are gone. So does this driver.
Signed-off-by: Andy Shevchenko
Acked-by: Linus Walleij
---
drivers/cpufreq/Kconfig.x86 | 10 ---
drivers/cpufreq/Makefile | 1 -
drivers/cpufreq/sfi-cpufreq.c | 127 --
3 files changed, 138 deletions(-)
delete mode 100644 drivers/cpufreq/sfi-cpufreq.c
diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86
index 399526289320..92701a18bdd9 100644
--- a/drivers/cpufreq/Kconfig.x86
+++ b/drivers/cpufreq/Kconfig.x86
@@ -62,16 +62,6 @@ config X86_ACPI_CPUFREQ_CPB
By enabling this option the acpi_cpufreq driver provides the old
entry in addition to the new boost ones, for compatibility reasons.
-config X86_SFI_CPUFREQ
- tristate "SFI Performance-States driver"
- depends on X86_INTEL_MID && SFI
- help
- This adds a CPUFreq driver for some Silvermont based Intel Atom
- architectures like Z34xx and Z35xx which enumerate processor
- performance states through SFI.
-
- If in doubt, say N.
-
config ELAN_CPUFREQ
tristate "AMD Elan SC400 and SC410"
depends on MELAN
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index f1b7e3dd6e5d..18c9b0eafd09 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -43,7 +43,6 @@ obj-$(CONFIG_X86_P4_CLOCKMOD) += p4-clockmod.o
obj-$(CONFIG_X86_CPUFREQ_NFORCE2) += cpufreq-nforce2.o
obj-$(CONFIG_X86_INTEL_PSTATE) += intel_pstate.o
obj-$(CONFIG_X86_AMD_FREQ_SENSITIVITY) += amd_freq_sensitivity.o
-obj-$(CONFIG_X86_SFI_CPUFREQ) += sfi-cpufreq.o
##
# ARM SoC drivers
diff --git a/drivers/cpufreq/sfi-cpufreq.c b/drivers/cpufreq/sfi-cpufreq.c
deleted file mode 100644
index 45cfdf67cf03..
--- a/drivers/cpufreq/sfi-cpufreq.c
+++ /dev/null
@@ -1,127 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * SFI Performance States Driver
- *
- * Author: Vishwesh M Rudramuni
- * Author: Srinidhi Kasagar
- */
-
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-
-#include
-
-static struct cpufreq_frequency_table *freq_table;
-static struct sfi_freq_table_entry *sfi_cpufreq_array;
-static int num_freq_table_entries;
-
-static int sfi_parse_freq(struct sfi_table_header *table)
-{
- struct sfi_table_simple *sb;
- struct sfi_freq_table_entry *pentry;
- int totallen;
-
- sb = (struct sfi_table_simple *)table;
- num_freq_table_entries = SFI_GET_NUM_ENTRIES(sb,
- struct sfi_freq_table_entry);
- if (num_freq_table_entries <= 1) {
- pr_err("No p-states discovered\n");
- return -ENODEV;
- }
-
- pentry = (struct sfi_freq_table_entry *)sb->pentry;
- totallen = num_freq_table_entries * sizeof(*pentry);
-
- sfi_cpufreq_array = kmemdup(pentry, totallen, GFP_KERNEL);
- if (!sfi_cpufreq_array)
- return -ENOMEM;
-
- return 0;
-}
-
-static int sfi_cpufreq_target(struct cpufreq_policy *policy, unsigned int
index)
-{
- unsigned int next_perf_state = 0; /* Index into perf table */
- u32 lo, hi;
-
- next_perf_state = policy->freq_table[index].driver_data;
-
- rdmsr_on_cpu(policy->cpu, MSR_IA32_PERF_CTL, , );
- lo = (lo & ~INTEL_PERF_CTL_MASK) |
- ((u32) sfi_cpufreq_array[next_perf_state].ctrl_val &
- INTEL_PERF_CTL_MASK);
- wrmsr_on_cpu(policy->cpu, MSR_IA32_PERF_CTL, lo, hi);
-
- return 0;
-}
-
-static int sfi_cpufreq_cpu_init(struct cpufreq_policy *policy)
-{
- policy->shared_type = CPUFREQ_SHARED_TYPE_HW;
- policy->cpuinfo.transition_latency = 10;/* 100us */
- policy->freq_table = freq_table;
-
- return 0;
-}
-
-static struct cpufreq_driver sfi_cpufreq_driver = {
- .flags = CPUFREQ_CONST_LOOPS,
- .verify = cpufreq_generic_frequency_table_verify,
- .target_index = sfi_cpufreq_target,
- .init = sfi_cpufreq_cpu_init,
- .name = "sfi-cpufreq",
- .attr = cpufreq_generic_attr,
-};
-
-static int __init sfi_cpufreq_init(void)
-{
- int ret, i;
-
- /* parse the freq table from SFI */
- ret = sfi_table_parse(SFI_SIG_FREQ, NULL, NULL, sfi_parse_freq);
- if (ret)
- return ret;
-
- freq_table = kcalloc(num_freq_table_entries + 1, sizeof(*freq_table),
-GFP_KERNEL);
- if (!freq_table) {
- ret = -ENOMEM;
- goto err_free_array;
- }
-
- for (i = 0; i < num_freq_table_entries; i++) {
- freq_table[i].driver_data = i;
- freq_table[i].frequency = sfi_cpufreq_array[i].freq_mhz * 1000;
- }
- f
[PATCH v1 4/9] x86/PCI: Get rid of custom x86 model comparison
Switch the platform code to use x86_id_table and accompanying API
instead of custom comparison against x86 CPU model.
This is one of the last users of custom API for that and following
changes will remove it for the good.
Signed-off-by: Andy Shevchenko
---
arch/x86/pci/intel_mid_pci.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c
index 95e2e6bd8d8c..938a8b7bfe7f 100644
--- a/arch/x86/pci/intel_mid_pci.c
+++ b/arch/x86/pci/intel_mid_pci.c
@@ -28,10 +28,12 @@
#include
#include
+#include
#include
#include
#include
#include
+#include
#include
#include
@@ -212,10 +214,17 @@ static int pci_write(struct pci_bus *bus, unsigned int
devfn, int where,
where, size, value);
}
+static const struct x86_cpu_id intel_mid_cpu_ids[] = {
+ X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_MID, NULL),
+ {}
+};
+
static int intel_mid_pci_irq_enable(struct pci_dev *dev)
{
+ const struct x86_cpu_id *id;
struct irq_alloc_info info;
bool polarity_low;
+ u16 model = 0;
int ret;
u8 gsi;
@@ -228,8 +237,12 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev)
return ret;
}
- switch (intel_mid_identify_cpu()) {
- case INTEL_MID_CPU_CHIP_TANGIER:
+ id = x86_match_cpu(intel_mid_cpu_ids);
+ if (id)
+ model = id->model;
+
+ switch (model) {
+ case INTEL_FAM6_ATOM_SILVERMONT_MID:
polarity_low = false;
/* Special treatment for IRQ0 */
--
2.30.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 0/9] x86/platform: Remove SFI framework and users
This is last part of Intel MID (SFI based) removal. We have no more users of it in the kernel and since SFI has been marked Obsolete for a few years already, Remove all the stuff altogether. Note, the more recent platforms (Intel Merrifield and Moorefield) still work as long as they provide correct ACPI tables. The series requires two prerequisite branches to be pulled first, i.e. - one form Rafael's PM tree (currently bleeding-edge) - one form TIP tree (x86/platform), actually only one patch is needed from it Due to above it's convenient to proceed all of these via Rafael's PM tree, Note, atomisp change is tagged by Sakari on behalf of media tree maintainers. Andy Shevchenko (9): media: atomisp: Remove unused header cpufreq: sfi-cpufreq: Remove driver for deprecated firmware sfi: Remove framework for deprecated firmware x86/PCI: Get rid of custom x86 model comparison x86/PCI: Describe @reg for type1_access_ok() x86/platform/intel-mid: Get rid of intel_scu_ipc_legacy.h x86/platform/intel-mid: Drop unused __intel_mid_cpu_chip and Co. x86/platform/intel-mid: Remove unused header inclusion in intel-mid.h x86/platform/intel-mid: Update Copyright year and drop file names Documentation/ABI/testing/sysfs-firmware-sfi | 15 - Documentation/ABI/testing/sysfs-platform-kim | 2 +- MAINTAINERS | 7 - arch/x86/Kconfig | 7 +- arch/x86/include/asm/intel-mid.h | 65 +-- arch/x86/include/asm/intel_scu_ipc.h | 2 - arch/x86/include/asm/intel_scu_ipc_legacy.h | 74 --- arch/x86/include/asm/platform_sst_audio.h | 2 - arch/x86/kernel/apic/io_apic.c| 4 +- arch/x86/kernel/setup.c | 2 - arch/x86/pci/intel_mid_pci.c | 18 +- arch/x86/pci/mmconfig-shared.c| 6 +- arch/x86/platform/Makefile| 1 - arch/x86/platform/intel-mid/Makefile | 5 - .../platform/intel-mid/device_libs/Makefile | 23 - .../intel-mid/device_libs/platform_bcm43xx.c | 101 .../intel-mid/device_libs/platform_bma023.c | 16 - .../intel-mid/device_libs/platform_bt.c | 101 .../intel-mid/device_libs/platform_emc1403.c | 39 -- .../device_libs/platform_gpio_keys.c | 81 --- .../intel-mid/device_libs/platform_lis331.c | 37 -- .../intel-mid/device_libs/platform_max7315.c | 77 --- .../intel-mid/device_libs/platform_mpu3050.c | 32 -- .../device_libs/platform_mrfld_pinctrl.c | 39 -- .../device_libs/platform_mrfld_rtc.c | 44 -- .../intel-mid/device_libs/platform_mrfld_sd.c | 43 -- .../device_libs/platform_mrfld_spidev.c | 50 -- .../device_libs/platform_pcal9555a.c | 95 .../intel-mid/device_libs/platform_tc35876x.c | 42 -- .../intel-mid/device_libs/platform_tca6416.c | 53 -- arch/x86/platform/intel-mid/intel-mid.c | 27 +- arch/x86/platform/intel-mid/sfi.c | 419 -- arch/x86/platform/sfi/Makefile| 2 - arch/x86/platform/sfi/sfi.c | 100 drivers/Makefile | 2 +- drivers/cpufreq/Kconfig.x86 | 10 - drivers/cpufreq/Makefile | 1 - drivers/cpufreq/sfi-cpufreq.c | 127 - drivers/platform/x86/intel_scu_pcidrv.c | 22 +- drivers/sfi/Kconfig | 18 - drivers/sfi/Makefile | 4 - drivers/sfi/sfi_acpi.c| 214 --- drivers/sfi/sfi_core.c| 522 -- drivers/sfi/sfi_core.h| 81 --- .../atomisp/include/linux/atomisp_platform.h | 1 - include/linux/sfi.h | 210 --- include/linux/sfi_acpi.h | 93 init/main.c | 2 - 48 files changed, 37 insertions(+), 2901 deletions(-) delete mode 100644 Documentation/ABI/testing/sysfs-firmware-sfi delete mode 100644 arch/x86/include/asm/intel_scu_ipc_legacy.h delete mode 100644 arch/x86/platform/intel-mid/device_libs/Makefile delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_bcm43xx.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_bma023.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_bt.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_emc1403.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_gpio_keys.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_lis331.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_max7315.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_mrfld_pinctrl.c delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_mrfld_rtc.c delete mode
Re: [PATCH v12] staging: fbtft: add tearing signal detect
On Sat, Jan 30, 2021 at 8:39 AM carlis wrote: > On Fri, 29 Jan 2021 16:26:12 +0200 > Andy Shevchenko wrote: > > On Fri, Jan 29, 2021 at 3:56 PM carlis wrote: > > > On Fri, 29 Jan 2021 12:23:08 +0200 > > > Andy Shevchenko wrote: ... > > > Hi, I apologize for what I said in the previous two emails. I missed > > > one email you sent before, and now I probably understand what you > > > meant. Here is a version I modified according to your suggestion: I have realized that you are mocking stuff in the generic fbtft structure for all drivers while only a single one is going to use that. Consider moving everything to the driver in question. -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v12] staging: fbtft: add tearing signal detect
On Fri, Jan 29, 2021 at 3:56 PM carlis wrote:
> On Fri, 29 Jan 2021 12:23:08 +0200
> Andy Shevchenko wrote:
We are almost there, I have no idea what Noralf or others are going to
say though.
...
> Hi, I apologize for what I said in the previous two emails. I missed
> one email you sent before, and now I probably understand what you
> meant. Here is a version I modified according to your suggestion:
>
> From 399e7fb91d1dcba4924cd38cc8283393c80b97e4 Mon Sep 17 00:00:00 2001
> From: Carlis
> Date: Sun, 24 Jan 2021 22:43:21 +0800
> Subject: [PATCH v13] staging: fbtft: add tearing signal detect
>
> For st7789v IC,when we need continuous full screen refresh, it is best
Missed space after comma.
> to wait for the tearing effect line signal arrive to avoid screen
to arrive
> tearing.
...
> +#define PANEL_TE_TIMEOUT_MS 34 /* 60Hz for 16.6ms, configured as
> 2*16.6ms */ +
Move comment before the definition
/* comment */
#define DEFINITION
Also consider to use 33 ms as closest to what you mentioned in the comment.
Or leave it with mention that you are using ceil() value.
...
> +/**
> + * init_tearing_effect_line() - init tearing effect line
> + *
As per a few previous reviews.
Okay, I have noticed that the existing kernel-doc is written like
this, but it doesn't prevent you from avoiding this little mistake.
> + * @par: FBTFT parameter object
> + *
> + * Return: 0 on success, or a negative error code otherwise.
> + */
> +static int init_tearing_effect_line(struct fbtft_par *par)
> +{
> + struct device *dev = par->info->device;
> + struct gpio_desc *te;
> + int rc;
> +
> + te = gpiod_get_optional(dev, "te", GPIOD_IN);
> + if (IS_ERR(te))
> + return dev_err_probe(dev, PTR_ERR(te), "Failed to
> request te GPIO\n"); +
Below is okay, but needs a comment explaining why we return a success.
> + if (!te)
> + return 0;
> +
> + par->irq_te = gpiod_to_irq(te);
> +
> + /* GPIO is locked as an IRQ, we may drop the reference */
> + gpiod_put(te);
> +
> + if (par->irq_te < 0)
> + return par->irq_te;
I recommend using a temporary variable. In such a case you won't need
to specifically check for negative error code. So, something like
int irq;
irq = ...
if (irq < 0)
return irq;
->irq_te = irq;
> + init_completion(>panel_te);
> + rc = devm_request_irq(dev, par->irq_te, panel_te_handler,
> + IRQF_TRIGGER_RISING, "TE_GPIO", par);
Right. Now it needs a comment explaining the choice of rising edge type of IRQ.
> + if (rc)
> + return dev_err_probe(dev, rc, "TE IRQ request
> failed.\n"); +
> + disable_irq_nosync(par->irq_te);
> +
> + return 0;
> +}
...
> + rc = init_tearing_effect_line(par);
> + if (rc < 0)
Here is no need to specifically check against less than 0,
if (ret)
will work nicely.
> + return rc;
...
> + if (par->irq_te)
> + write_reg(par, MIPI_DCS_SET_TEAR_ON, 0x00);
Do you need to call MIPI_DCS_SET_TEAR_SCANLINE in this case?
Alos, when there is no IRQ, shouldn't we explicitly call
write_reg(par, MIPI_DCS_SET_TEAR_OFF);
?
...
> /**
> + * st7789v_write_vmem16_bus8() - write data to display
> + *
Redundant.
> + * @par: FBTFT parameter object
> + * @offset: offset from screen_buffer
> + * @len: the length of data to be written
> + *
> + * 16 bit pixel over 8-bit databus
Write 16-bit pixels over 8-bit data bus.
> + * Return: 0 on success, or a negative error code otherwise.
> + */
...
> + if (par->irq_te) {
> + enable_irq(par->irq_te);
> + reinit_completion(>panel_te);
> + ret = wait_for_completion_timeout(>panel_te,
> +
> msecs_to_jiffies(PANEL_TE_TIMEOUT_MS));
> + if (ret == 0)
> + dev_err(dev, "wait panel TE time out\n");
timeout
> +
> + disable_irq(par->irq_te);
> + }
...
> + * @panel_te: completion for panel te line
TE line
> + * @irq_te: LCD Chip tearing effect line
"Linux IRQ for LCD..."
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v12] staging: fbtft: add tearing signal detect
On Fri, Jan 29, 2021 at 2:54 PM carlis wrote: > On Fri, 29 Jan 2021 12:23:08 +0200 > Andy Shevchenko wrote: > > On Fri, Jan 29, 2021 at 7:01 AM carlis wrote: > > > On Thu, 28 Jan 2021 16:33:02 +0200 > > > Andy Shevchenko wrote: ... > > This one is not like I suggested. > I don't think I have a problem here, if te GPIO is not configured, it > should return NULL, if it is configured, it should be greater than 0 Pointers are always greater than 0 or a special NULL case. The rationale I explained in the previous mail. ... > > > rc = devm_request_irq(dev, > > > par->irq_te, > > > panel_te_handler, > > > IRQF_TRIGGER_RISING, > > > "TE_GPIO", par); > > > > Try to use less LOCs. > > LOCs i can not get you Lines Of Code. Above can occupy less LOCs. -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v12] staging: fbtft: add tearing signal detect
On Fri, Jan 29, 2021 at 2:47 PM carlis wrote:
> On Fri, 29 Jan 2021 12:23:08 +0200
> Andy Shevchenko wrote:
> > On Fri, Jan 29, 2021 at 7:01 AM carlis wrote:
> > > On Thu, 28 Jan 2021 16:33:02 +0200
> > > Andy Shevchenko wrote:
> > > > On Thu, Jan 28, 2021 at 2:58 PM Carlis
> > > > wrote:
...
> > Please, go again thru my comments and comments from others and
> > carefully address all of them everywhere in your contribution. If you
> > have questions, ask them in reply in the corresponding context.
...
> > > /**
> > > * init_tearing_effect_line() - init tearing effect line
> >
> > > *
> >
> > For example, above was commented on and hasn't been addressed here.
> >
> hi,here i can not get you.
The above is a blank line which is redundant. It also applied to the
other function in the code.
> > > * @par: FBTFT parameter object
> > > *
> > > * Return: 0 on success, < 0 if error occurred.
> > > */
> > > static int init_tearing_effect_line(struct fbtft_par *par)
> > > {
> > > struct device *dev = par->info->device;
> > > struct gpio_desc *te;
> > > int rc;
> > >
> > > te = gpiod_get_optional(dev, "te", GPIOD_IN);
> > > if (IS_ERR(te))
> > > return dev_err_probe(dev, PTR_ERR(te), "Failed to
> > > request te GPIO\n");
> > >
> >
> > > if (te) {
> >
> > This one is not like I suggested.
> Why? My thinking is that if the TE is not configured and NULL is
> returned, the initialization can still proceed.
I have suggested to bail out immediately. It will reduce an
indentation level on the below code.
> > > par->irq_te = gpiod_to_irq(te);
> > > gpiod_put(te);
> > >
> >
> > > if (par->irq_te) {
> >
> > This is wrong.
>
> Why? i have read gpiod_to_irq code, if an error occurs, a negative
> value is returned, and zero is not possible,so I need this value to
> determine if TE IRQ is configured
It returns two possible cases:
- error code (when negative)
- Linux IRQ number otherwise
You check makes a no-op since in either variant it will proceed to the
request of IRQ, which is wrong in an error case.
> > > rc = devm_request_irq(dev,
> > > par->irq_te,
> > > panel_te_handler,
> > > IRQF_TRIGGER_RISING,
> > > "TE_GPIO", par);
> >
> > Try to use less LOCs.
> >
> > > if (rc)
> > > return dev_err_probe(dev, rc, "TE
> > > IRQ request failed.\n");
> > >
> > > disable_irq_nosync(par->irq_te);
> > > init_completion(>panel_te);
> >
> > > } else {
> > > return dev_err_probe(dev, par->irq_te,
> > > "gpiod to TE IRQ failed.\n");
> > > }
> >
> > Again, it is not what had been suggested.
> >
> > > }
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v12] staging: fbtft: add tearing signal detect
On Fri, Jan 29, 2021 at 7:01 AM carlis wrote:
> On Thu, 28 Jan 2021 16:33:02 +0200
> Andy Shevchenko wrote:
> > On Thu, Jan 28, 2021 at 2:58 PM Carlis wrote:
> >
> > Thanks for your contribution, my comments below.
> >
> > > From: zhangxuezhi
> >
> > You probably have to configure your Git to use the same account for
> > author and committer.
>
> hi,you mean like below:
> Carlis
> ?
I meant that you shouldn't probably have a From: line in the commit message.
...
> hi, i have modified it according to your suggestion like below:
Please, go again thru my comments and comments from others and
carefully address all of them everywhere in your contribution. If you
have questions, ask them in reply in the corresponding context.
...
> /**
> * init_tearing_effect_line() - init tearing effect line
> *
For example, above was commented on and hasn't been addressed here.
> * @par: FBTFT parameter object
> *
> * Return: 0 on success, < 0 if error occurred.
> */
> static int init_tearing_effect_line(struct fbtft_par *par)
> {
> struct device *dev = par->info->device;
> struct gpio_desc *te;
> int rc;
>
> te = gpiod_get_optional(dev, "te", GPIOD_IN);
> if (IS_ERR(te))
> return dev_err_probe(dev, PTR_ERR(te), "Failed to
> request te GPIO\n");
>
> if (te) {
This one is not like I suggested.
> par->irq_te = gpiod_to_irq(te);
> gpiod_put(te);
>
> if (par->irq_te) {
This is wrong.
> rc = devm_request_irq(dev,
> par->irq_te,
> panel_te_handler,
> IRQF_TRIGGER_RISING,
> "TE_GPIO", par);
Try to use less LOCs.
> if (rc)
> return dev_err_probe(dev, rc, "TE IRQ
> request failed.\n");
>
> disable_irq_nosync(par->irq_te);
> init_completion(>panel_te);
> } else {
> return dev_err_probe(dev, par->irq_te, "gpiod
> to TE IRQ failed.\n");
> }
Again, it is not what had been suggested.
> }
>
> return 0;
> }
The rest is better, but we will see later on when you submit a new
version (And I feel it won't be last).
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v12] staging: fbtft: add tearing signal detect
On Thu, Jan 28, 2021 at 4:33 PM Andy Shevchenko
wrote:
> On Thu, Jan 28, 2021 at 2:58 PM Carlis wrote:
...
> Taking all together you probably need to create a helper and use it
> inside init_display(), like
>
> static int init_tearing_effect_line(struct fbtft_par *par)
> {
> struct device *dev = par->info->device;
> struct gpio_desc *te;
> int irq, rc;
>
> te = gpiod_get_optional(dev, "te", GPIOD_IN);
> if (IS_ERR(te))
>return dev_err_probe(dev, PTR_ERR(te), "Failed to request
> te GPIO\n");
Sorry, here I missed the following:
/* Absence of TE IRQ is not critical */
if (!te)
return 0;
> irq = gpiod_to_irq(te); // this value you have to save in the
> driver's (per device) data structure.
>
> /* GPIO is locked as an IRQ, we may drop the reference */
> gpiod_put(te);
...and here:
if (irq < 0)
return irq;
> init_completion(_panel_te); // should be in the (per device)
> data structure
> rc = devm_request_irq(dev, irq, spi_panel_te_handler,
> IRQF_TRIGGER_RISING, "TE_GPIO", par);
> if (rc)
> return dev_err_probe(dev, rc, "TE IRQ request failed.\n");
> disable_irq_nosync(irq);
> return irq;
> }
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v12] staging: fbtft: add tearing signal detect
x_array_size;
> + int i;
> + int ret = 0;
> + size_t startbyte_size = 0;
Reversed xmas tree order.
> + fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "st7789v ---%s(offset=%zu,
> len=%zu)\n",
> + __func__, offset, len);
> +
> + remain = len / 2;
> + vmem16 = (u16 *)(par->info->screen_buffer + offset);
> + if (par->gpio.dc)
Useless duplicate check.
> + gpiod_set_value(par->gpio.dc, 1);
> + if (par->gpio.te) {
> + enable_irq(gpiod_to_irq(par->gpio.te));
Here you should use the IRQ line rather than the GPIO descriptor. See above.
> + reinit_completion(_panel_te);
> + ret = wait_for_completion_timeout(_panel_te,
> +
> msecs_to_jiffies(SPI_PANEL_TE_TIMEOUT));
> + if (ret == 0)
> + dev_err(par->info->device, "wait panel TE time
> out\n");
> +
> + disable_irq(gpiod_to_irq(par->gpio.te));
> + }
> +
> + while (remain) {
> + to_copy = min(tx_array_size, remain);
> + dev_dbg(par->info->device, "to_copy=%zu, remain=%zu\n",
> + to_copy, remain - to_copy);
Like in previous functions create a temporary variable to keep a
pointer to struct device and use it here and everywhere else. It might
save you LOCs and make code easier to read and understand.
> + for (i = 0; i < to_copy; i++)
> + txbuf16[i] = cpu_to_be16(vmem16[i]);
If both of them are 16-bit wide, consider moving this to a helper
which somebody can move to byteorder/generic.h in the future.
> + vmem16 = vmem16 + to_copy;
> + ret = par->fbtftops.write(par, par->txbuf.buf,
> +startbyte_size + to_copy * 2);
> + if (ret < 0)
> + return ret;
> + remain -= to_copy;
> + }
> +
> + return ret;
> +}
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 01/11] firmware: raspberrypi: Keep count of all consumers
On Thu, Nov 12, 2020 at 6:40 PM Nicolas Saenz Julienne
wrote:
>
> When unbinding the firmware device we need to make sure it has no
> consumers left. Otherwise we'd leave them with a firmware handle
> pointing at freed memory.
>
> Keep a reference count of all consumers and introduce rpi_firmware_put()
> which will permit automatically decrease the reference count upon
> unbinding consumer drivers.
...
> /**
> - * rpi_firmware_get - Get pointer to rpi_firmware structure.
> * @firmware_node:Pointer to the firmware Device Tree node.
> *
> + * The reference to rpi_firmware has to be released with rpi_firmware_put().
> + *
> * Returns NULL is the firmware device is not ready.
> */
> struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node)
> {
> struct platform_device *pdev = of_find_device_by_node(firmware_node);
> + struct rpi_firmware *fw;
>
> if (!pdev)
> return NULL;
>
> - return platform_get_drvdata(pdev);
> + fw = platform_get_drvdata(pdev);
> + if (!fw)
> + return NULL;
> +
> + if (!kref_get_unless_zero(>consumers))
> + return NULL;
Don't we have a more traditional way of doing this, i.e.
try_module_get() coupled with get_device() ?
> + return fw;
> }
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: fieldbus: Use %pM format specifier for MAC addresses
On Wed, Oct 28, 2020 at 07:39:58PM -0400, Sven Van Asbroeck wrote: > On Tue, Oct 27, 2020 at 2:34 PM Andy Shevchenko > wrote: > > > > + return snprintf(buf, max_size, "%pM\n", response.addr); > > Judging from a few Outreachy patches that have hit my inbox, snprintf() is > considered unsafe in a sysfs_get callback. It should be replaced by > scnprintf() or even better, sysfs_emit(), which was recently introduced > to address sprintf-variant issues in sysfs callbacks. It's already applied, I think you can send a follow up for above, which is good point, but not the scope of the patch (although they might have been unified together). -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: fieldbus: Use %pM format specifier for MAC addresses
Convert to %pM instead of using custom code. Signed-off-by: Andy Shevchenko --- v2: dropped struct removal (Sven), rebased on top of v5.10-rc1 drivers/staging/fieldbus/anybuss/hms-profinet.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/staging/fieldbus/anybuss/hms-profinet.c b/drivers/staging/fieldbus/anybuss/hms-profinet.c index 31c43a0a5776..eca7d97b8e85 100644 --- a/drivers/staging/fieldbus/anybuss/hms-profinet.c +++ b/drivers/staging/fieldbus/anybuss/hms-profinet.c @@ -66,10 +66,7 @@ static int profi_id_get(struct fieldbus_dev *fbdev, char *buf, sizeof(response)); if (ret < 0) return ret; - return snprintf(buf, max_size, "%02X:%02X:%02X:%02X:%02X:%02X\n", - response.addr[0], response.addr[1], - response.addr[2], response.addr[3], - response.addr[4], response.addr[5]); + return snprintf(buf, max_size, "%pM\n", response.addr); } static bool profi_enable_get(struct fieldbus_dev *fbdev) -- 2.28.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 10/10] pwm: Add Raspberry Pi Firmware based PWM bus
On Thu, Oct 22, 2020 at 9:05 PM Nicolas Saenz Julienne
wrote:
>
> Adds support to control the PWM bus available in official Raspberry Pi
> PoE HAT. Only RPi's co-processor has access to it, so commands have to
> be sent through RPi's firmware mailbox interface.
> drivers/pwm/pwm-raspberrypi.c | 221 ++
Name is completely confusing.
Please, make it unique enough to understand that this is exactly the
device it serves for.
For example, pwm-rpi-poe is better.
...
> + * - Only normal polarity
Can't it be emulated? Isn't it 100% - duty cycle % ?
> +#include
> +#include
> +#include
> +#include
...
> + ret = rpi_firmware_property(firmware, RPI_FIRMWARE_SET_POE_HAT_VAL,
> + , sizeof(msg));
> + if (ret)
> + return ret;
> + else if (msg.ret)
Redundant 'else'
> + return -EIO;
...
> + ret = rpi_firmware_property(firmware, RPI_FIRMWARE_GET_POE_HAT_VAL,
> + , sizeof(msg));
> + if (ret)
> + return ret;
> + else if (msg.ret)
Ditto.
> + return -EIO;
...
> + firmware_node = of_get_parent(dev->of_node);
> + if (!firmware_node) {
> + dev_err(dev, "Missing firmware node\n");
> + return -ENOENT;
> + }
> +
> + firmware = rpi_firmware_get(firmware_node);
> + of_node_put(firmware_node);
> + if (!firmware)
> + return -EPROBE_DEFER;
Looks like a hack.
...
> + ret = pwmchip_remove(>chip);
> + if (!ret)
> + rpi_firmware_put(rpipwm->firmware);
> +
> + return ret;
Can't you use the usual pattern?
ret = ...
if (ret)
return ret;
...
return 0;
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 01/10] firmware: raspberrypi: Introduce rpi_firmware_put()
On Thu, Oct 22, 2020 at 9:06 PM Nicolas Saenz Julienne wrote: > > When unbinding the firmware device we need to make sure it has no > consumers left. Otherwise we'd leave them with a firmware handle > pointing at freed memory. > > Keep a reference count of all consumers and make sure they all finished > unbinding before we do. Wait, if it's a device, why do we need all these? get_device() / put_device() along with module_get() / module_put() should be sufficient, no? -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] media: atomisp: remove compat_ioctl32 code
On Wed, Oct 07, 2020 at 05:34:15PM +0200, Arnd Bergmann wrote:
> This is one of the last remaining users of compat_alloc_user_space()
> and copy_in_user(), which are in the process of getting removed.
>
> As of commit 57e6b6f2303e ("media: atomisp_fops.c: disable
> atomisp_compat_ioctl32"), nothing in this file is actually getting used
> as the only reference has been stubbed out.
>
> Remove the entire file -- anyone willing to restore the functionality
> can equally well just look up the contents in the git history if needed.
Good one!
Reviewed-by: Andy Shevchenko
> Cc: Sakari Ailus
> Cc: Hans Verkuil
> Suggested-by: Christoph Hellwig
> Signed-off-by: Arnd Bergmann
> ---
> This is the alternative approach for the patch, removing the already
> dead code instead of just not compiling it.
> ---
> drivers/staging/media/atomisp/Makefile|1 -
> drivers/staging/media/atomisp/TODO|5 +
> .../atomisp/pci/atomisp_compat_ioctl32.c | 1202 -
> .../staging/media/atomisp/pci/atomisp_fops.c |8 +-
> 4 files changed, 8 insertions(+), 1208 deletions(-)
> delete mode 100644 drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.c
>
> diff --git a/drivers/staging/media/atomisp/Makefile
> b/drivers/staging/media/atomisp/Makefile
> index 1dfad0dd02d0..3bbd4bf4408c 100644
> --- a/drivers/staging/media/atomisp/Makefile
> +++ b/drivers/staging/media/atomisp/Makefile
> @@ -16,7 +16,6 @@ atomisp-objs += \
> pci/atomisp_acc.o \
> pci/atomisp_cmd.o \
> pci/atomisp_compat_css20.o \
> - pci/atomisp_compat_ioctl32.o \
> pci/atomisp_csi2.o \
> pci/atomisp_drvfs.o \
> pci/atomisp_file.o \
> diff --git a/drivers/staging/media/atomisp/TODO
> b/drivers/staging/media/atomisp/TODO
> index 6987bb2d32cf..2d1ef9eb262a 100644
> --- a/drivers/staging/media/atomisp/TODO
> +++ b/drivers/staging/media/atomisp/TODO
> @@ -120,6 +120,11 @@ TODO
> for this driver until the other work is done, as there will be a lot
> of code churn until this driver becomes functional again.
>
> +16. Fix private ioctls to not need a compat_ioctl handler for running
> +32-bit tasks. The compat code has been removed because of bugs,
> +and should not be needed for modern drivers. Fixing this properly
> +unfortunately means an incompatible ABI change.
> +
> Limitations
> ===
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.c
> b/drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.c
> deleted file mode 100644
> index e5553df5bad4..
> --- a/drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.c
> +++ /dev/null
> @@ -1,1202 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * Support for Intel Camera Imaging ISP subsystem.
> - *
> - * Copyright (c) 2013 Intel Corporation. All Rights Reserved.
> - *
> - * This program is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU General Public License version
> - * 2 as published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> - * GNU General Public License for more details.
> - *
> - *
> - */
> -#ifdef CONFIG_COMPAT
> -#include
> -
> -#include
> -
> -#include "atomisp_internal.h"
> -#include "atomisp_compat.h"
> -#include "atomisp_ioctl.h"
> -#include "atomisp_compat_ioctl32.h"
> -
> -/* Macros borrowed from v4l2-compat-ioctl32.c */
> -
> -#define get_user_cast(__x, __ptr)\
> -({ \
> - get_user(__x, (typeof(*__ptr) __user *)(__ptr));\
> -})
> -
> -#define put_user_force(__x, __ptr) \
> -({ \
> - put_user((typeof(*__x) __force *)(__x), __ptr); \
> -})
> -
> -/* Use the same argument order as copy_in_user */
> -#define assign_in_user(to, from) \
> -({ \
> - typeof(*from) __assign_tmp; \
> - \
> - get_user_cast(__assign_tmp, from) || put_user(__assign_tmp, to);\
> -})
> -
> -static int get_atomisp_histogram32(struct atomisp_histogram __user *kp,
> -str
Re: [PATCH v3] media: atomisp: fixes build breakage for ISP2400 due to a cleanup
On Thu, Oct 1, 2020 at 6:55 PM Mauro Carvalho Chehab wrote: > Em Thu, 1 Oct 2020 18:50:12 +0300 > Andy Shevchenko escreveu: > > > On Thu, Oct 1, 2020 at 2:17 PM Mauro Carvalho Chehab > > wrote: > > > > > > A temporary var needed for building with ISP2400 was removed > > > by accident on a cleanup patch. > > > > > > Fix the breakage. > > > > Is this in any of your branches? > > I added v3 of the fixes today at the media tree master branch. > > If no merge issues happen, it should be at tomorrow's linux-next. Fixes the issue to me, thanks! Tested-by: Andy Shevchenko -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] media: atomisp: fixes build breakage for ISP2400 due to a cleanup
On Thu, Oct 1, 2020 at 2:17 PM Mauro Carvalho Chehab
wrote:
>
> A temporary var needed for building with ISP2400 was removed
> by accident on a cleanup patch.
>
> Fix the breakage.
Is this in any of your branches?
>
> Fixes: 852a53a02cf0 ("media: atomisp: get rid of unused vars")
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/staging/media/atomisp/pci/sh_css.c | 14 +++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/sh_css.c
> b/drivers/staging/media/atomisp/pci/sh_css.c
> index e8c5caf3dfe6..ddee04c8248d 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css.c
> @@ -1365,7 +1365,6 @@ start_binary(struct ia_css_pipe *pipe,
> {
> assert(pipe);
> /* Acceleration uses firmware, the binary thus can be NULL */
> - /* assert(binary != NULL); */
>
> if (binary)
> sh_css_metrics_start_binary(>metrics);
> @@ -1381,10 +1380,10 @@ start_binary(struct ia_css_pipe *pipe,
> #endif
>
> #if !defined(ISP2401)
> - if (stream->reconfigure_css_rx) {
> + if (pipe->stream->reconfigure_css_rx) {
> ia_css_isys_rx_configure(>stream->csi_rx_config,
> pipe->stream->config.mode);
> - stream->reconfigure_css_rx = false;
> + pipe->stream->reconfigure_css_rx = false;
> }
> #endif
> }
> @@ -2820,6 +2819,8 @@ load_preview_binaries(struct ia_css_pipe *pipe) {
> bool need_isp_copy_binary = false;
> #ifdef ISP2401
> bool sensor = false;
> +#else
> + bool continuous;
> #endif
> /* preview only have 1 output pin now */
> struct ia_css_frame_info *pipe_out_info = >output_info[0];
> @@ -2833,6 +2834,8 @@ load_preview_binaries(struct ia_css_pipe *pipe) {
> online = pipe->stream->config.online;
> #ifdef ISP2401
> sensor = pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR;
> +#else
> + continuous = pipe->stream->config.continuous;
> #endif
>
> if (mycs->preview_binary.info)
> @@ -5987,6 +5990,8 @@ static int load_primary_binaries(
> bool need_ldc = false;
> #ifdef ISP2401
> bool sensor = false;
> +#else
> + bool memory, continuous;
> #endif
> struct ia_css_frame_info prim_in_info,
>prim_out_info,
> @@ -6009,6 +6014,9 @@ static int load_primary_binaries(
> online = pipe->stream->config.online;
> #ifdef ISP2401
> sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR);
> +#else
> + memory = pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY;
> + continuous = pipe->stream->config.continuous;
> #endif
>
> mycs = >pipe_settings.capture;
> --
> 2.26.2
>
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms
On Mon, Sep 21, 2020 at 02:33:57PM +0100, Dan Scally wrote:
> On 18/09/2020 14:07, Andy Shevchenko wrote:
> > On Fri, Sep 18, 2020 at 10:51:57AM +0300, Sakari Ailus wrote:
> >> On Thu, Sep 17, 2020 at 03:45:14PM +0300, Andy Shevchenko wrote:
> >>> On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote:
> >>>> On 17/09/2020 11:33, Sakari Ailus wrote:
> >>>>> a module and not enlarge everyone's kernel, and the initialisation
> >>>>> would at
> >>>>> the same time take place before the rest of what the CIO2 driver does in
> >>>>> probe.
> >>>> I thought of that as well, but wasn't sure which was preferable. I can
> >>>> compress it into the CIO2 driver though sure.
> >>> Sakari, I tend to agree with Dan and have the board file separated from
> >>> the
> >>> driver and even framework.
> >> And it'll be linked to the kernel binary then I suppose?
> > Solely depends to your Kconfig dependencies and declaration.
> >
> > From code perspective you may do it before enumeration of the certain
> > device or
> > after with reprobe.
> >
> >> I don't have a strong opinion either way, just thought that this will
> >> affect anyone using x86 machines, whether or not they have IPU3. I guess it
> >> could be compiled in if the ipu3-cio2 driver is enabled?
> > Of course!
>
> Apologies both - my inexperience is showing here: I thought you couldn't
> make it compile into the kernel where it's dependent on another piece of
> code that's configured to be a module? In my case, ipu3-cio2 plus some
> other dependencies are configured as modules; VIDEO_DEV and VIDEO_V4L2
> notably. Is that not right?
It's not correct.
We specifically have export.h (usually implied by module.h) which provides an
API for symbols that may be used in modules independently on where they are
(in-kernel or in a module).
So, provider does
bar.h:
int foo();
bar.c:
int foo()
{
}
EXPORT_SYMBOL(foo); // normally is EXPORT_SYMBOL_GPL() in use
Consumer:
#include
ret = foo();
> It would probably make the probe() ordering issues easier if it could be
> compiled in, since I could just rely on late_initcall() in that case and
> I guess that should work.
So, you may have two cases, your module went first, the other module went
first. Some similar problems as your bridge is trying to address are mitigated
in intel_cht_int33fe_typec.c. Look at it. It's not an ideal example, but that's
due to miserable ACPI tables BIOS provided.
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms
On Fri, Sep 18, 2020 at 10:51:57AM +0300, Sakari Ailus wrote: > On Thu, Sep 17, 2020 at 03:45:14PM +0300, Andy Shevchenko wrote: > > On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote: > > > On 17/09/2020 11:33, Sakari Ailus wrote: > > > > a module and not enlarge everyone's kernel, and the initialisation > > > > would at > > > > the same time take place before the rest of what the CIO2 driver does in > > > > probe. > > > I thought of that as well, but wasn't sure which was preferable. I can > > > compress it into the CIO2 driver though sure. > > > > Sakari, I tend to agree with Dan and have the board file separated from the > > driver and even framework. > > And it'll be linked to the kernel binary then I suppose? Solely depends to your Kconfig dependencies and declaration. >From code perspective you may do it before enumeration of the certain device or after with reprobe. > I don't have a strong opinion either way, just thought that this will > affect anyone using x86 machines, whether or not they have IPU3. I guess it > could be compiled in if the ipu3-cio2 driver is enabled? Of course! -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms
On Thu, Sep 17, 2020 at 02:36:22PM +0100, Dan Scally wrote: > On 17/09/2020 13:45, Andy Shevchenko wrote: > > On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote: > >> On 17/09/2020 11:33, Sakari Ailus wrote: > > I will do better review for next version, assuming you will Cc reviewers and > > TWIMC people. Below is like small part of comments I may give to the code. > TWIMC? Missed this. To Whom It May Concern. -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms
On Thu, Sep 17, 2020 at 5:19 PM Kieran Bingham wrote: > On 17/09/2020 15:08, Andy Shevchenko wrote: ... > Ayee, ok so we have 'half' the driver for IPU3 out of staging. Correct. And your below analysis is correct. > From my understanding, the IPU3 consists of two components, the CIO2 > (CSI2 capture), and the IMGU (the ISP). > > - drivers/media/pci/intel/ipu3 > > This is indeed the CIO2 component (config VIDEO_IPU3_CIO2), and that is > the part that this bridge relates to, so in fact this cio2-bridge should > probably go there indeed. No need to go through staging. > > The files remaining at: > > - drivers/staging/media/ipu3 > > are in fact also for the IPU3 but the ISP component (VIDEO_IPU3_IMGU). > > I'm sorry for the confusion, I knew that the ISP was still in staging, I > hadn't realised the CSI2 receiver (CIO2) was not. > > >> Hopefully with more users of the IPU3 brought in by this cio2-bridge, > >> that will help gather momentum to get the IPU3 developments required > >> completed and moved into drivers/media. -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms
On Thu, Sep 17, 2020 at 4:53 PM Dan Scally wrote:
>
> Hi Andy, thanks for input (as always)
You're welcome! I'm really impressed by your activity in this area.
> On 17/09/2020 13:45, Andy Shevchenko wrote:
> > On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote:
> >> On 17/09/2020 11:33, Sakari Ailus wrote:
To the point of placement, I think this should go under
drivers/platform/x86 (Adding Hans and Mark, who can express their
opinions).
...
> > Ah, I think you misinterpreted the meaning of above. The above is a switch
> > how
> > camera device appears either as PCI or an ACPI. So, it effectively means you
> > should *not* have any relation for this HID until you find a platform where
> > the
> > device is for real enumerated via ACPI.
> >
> Ah, ok. So that was never going to work. Thanks. That does raise another
> question; we have had some testers report failure, which turns out to be
> because on their platforms the definition of their cameras in ACPI is
> never translated into an i2c_client so the cio2-bridge doesn't bind.
> Those have a similar conditional in the _STA method, see CAM1 in this
> DSDT for example:
> https://raw.githubusercontent.com/linux-surface/acpidumps/master/surface_go/dsdt.dsl.
> Is there anything we can do to enable those cameras to be discovered too?
It means that this
...
> >>>> +#define PROPERTY_ENTRY_NULL \
> >>>> +((const struct property_entry) { })
> >>> Alignment. Same appears to apply to other macros (please indent).
> >> Yep
> >>>> +#define SOFTWARE_NODE_NULL\
> >>>> +((const struct software_node) { })
> > Why?!
> >
> It felt ugly to have the other definitions be macros and not this one,
> but I can change it.
My point is that those macros are simply redundant. The point is to
have a terminator record (all 0:s in the last entry of an array) which
is usually being achieved by allocating memory with kcalloc() which
does implicitly this for you.
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms
On Thu, Sep 17, 2020 at 4:31 PM Kieran Bingham wrote: > On 17/09/2020 10:47, Dan Scally wrote: > > On 17/09/2020 08:53, Greg KH wrote: > >> On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote: > >>> drivers/staging/media/ipu3/Kconfig | 15 + > >>> drivers/staging/media/ipu3/Makefile | 1 + > >>> drivers/staging/media/ipu3/cio2-bridge.c | 448 +++ > >> Why does this have to be in drivers/staging/ at all? Why not spend the > >> time to fix it up properly and get it merged correctly? It's a very > >> small driver, and should be smaller, so it should not be a lot of work > >> to do. And it would be faster to do that than to take it through > >> staging... > > I was just under the impression that that was the process to be honest, > > if that's not right I'll just move it directly to drivers/media/ipu3 > > The IPU3 driver is still in staging (unless I've missed something), so I > think this cio2-bridge should stay with it. You missed something. https://elixir.bootlin.com/linux/v5.9-rc5/source/drivers/media/pci/intel IPU3 from Freescale (IIRC) is a different story. > Hopefully with more users of the IPU3 brought in by this cio2-bridge, > that will help gather momentum to get the IPU3 developments required > completed and moved into drivers/media. -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms
es?
...
> >> \ No newline at end of file
???
Be sure you are using good editor.
...
> >> +#include
Redundant. ACPI headers are designed the way that you are using a single header
in Linux kernel for all. It might be different in drivers/acpi stuff, but not
in general.
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +
> >> +#include
> >> +#include
Please, keep them sorted. And since it's for media, the media inclusion may be
placed last in a separate group.
...
> >> +#define PROPERTY_ENTRY_NULL \
> >> +((const struct property_entry) { })
> > Alignment. Same appears to apply to other macros (please indent).
> Yep
> >
> >> +#define SOFTWARE_NODE_NULL\
> >> +((const struct software_node) { })
Why?!
...
> >> +struct software_node cio2_hid_node = { CIO2_HID, };
static ?
Same for other global variables.
...
> >> +struct cio2_bridge bridge = { 0, };
When define as static the assignment will not be needed.
...
> >> +static int read_acpi_block(struct device *dev, char *id, void *data, u32
> >> size)
> >> +{
> >> + union acpi_object *obj;
> >> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> >> + struct acpi_handle *dev_handle = ACPI_HANDLE(dev);
Usually we use simple handle if there is no ambiguous reading.
> >> + int status;
Should be acpi_status
> >> + u32 buffer_length;
> >> +
> >> + status = acpi_evaluate_object(dev_handle, id, NULL, );
> >> + if (!ACPI_SUCCESS(status))
ACPI_FAILURE()
> >> + return -ENODEV;
> >> +
> >> + obj = (union acpi_object *)buffer.pointer;
Why explicit casting?
> >> + if (!obj || obj->type != ACPI_TYPE_BUFFER) {
> >> + dev_err(dev, "Could't read acpi buffer\n");
> >> + status = -ENODEV;
Should have different int type variable for that.
> >> + goto err;
If there is no obj, you may return directly without freeing.
> >> + }
> >> +
> >> + if (obj->buffer.length > size) {
> >> + dev_err(dev, "Given buffer is too small\n");
> >> + status = -ENODEV;
> >> + goto err;
> >> + }
> >> +
> >> + memcpy(data, obj->buffer.pointer, min(size, obj->buffer.length));
Does type of size and length the same? Otherwise you need min_t().
> >> + buffer_length = obj->buffer.length;
> >> + kfree(buffer.pointer);
> >> +
> >> + return buffer_length;
> >> +err:
Consider naming labels by what they are about to do. Like
err_free:
here.
> >> + kfree(buffer.pointer);
> >> + return status;
> >> +}
> >> +static int get_acpi_ssdb_sensor_data(struct device *dev,
> >> + struct sensor_bios_data *sensor)
> >> +{
> >> + struct sensor_bios_data_packed sensor_data;
> >> + int ret = read_acpi_block(dev, "SSDB", _data,
> >> +sizeof(sensor_data));
Please, split declaration and assignment especially in the cases where it
requires long lines.
> >> + if (ret < 0) {
> >> + dev_err(dev, "Failed to fetch SSDB data\n");
> >> + return ret;
> >> + }
> >> +
> >> + sensor->link = sensor_data.link;
> >> + sensor->lanes = sensor_data.lanes;
> >> + sensor->mclkspeed = sensor_data.mclkspeed;
> >> +
> >> + return 0;
> >> +}
...
> >> + if (!dev->driver_data) {
> >> + pr_info("ACPI match for %s, but it has no driver\n",
> >> + supported_devices[i]);
> >> + continue;
> >> + } else {
> >> + pr_info("Found supported device %s\n",
> >> + supported_devices[i]);
> >> + }
Positive conditions are easier to read, but on the other hand 'else' is
redundant in such conditionals (where if branch bails out from the flow).
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms
On Thu, Sep 17, 2020 at 01:49:41PM +0300, Dan Carpenter wrote: > On Thu, Sep 17, 2020 at 01:33:43PM +0300, Sakari Ailus wrote: > > > + int i, ret; > > > > unsigned int i > > > > Why? > > For list iterators then "int i;" is best... For sizes then unsigned is > sometimes best. Or if it's part of the hardware spec or network spec > unsigned is best. Otherwise unsigned variables cause a ton of bugs. > They're not as intuitive as signed variables. Imagine if there is an > error in this loop and you want to unwind. With a signed variable you > can do: > > while (--i >= 0) > cleanup([i]); Ha-ha. It's actually a counter argument to your stuff because above is the same as while (i--) cleanup([i]); with pretty much unsigned int i. > There are very few times where raising the type maximum from 2 billion > to 4 billion fixes anything. -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 5/5] media: atomisp: get rid of -Wsuggest-attribute=format warnings
On Thu, Sep 03, 2020 at 03:57:32PM +0200, Mauro Carvalho Chehab wrote:
> There are some warnings reported by gcc:
> drivers/staging/media/atomisp//pci/atomisp_compat_css20.c:164:2:
> warning: function ‘atomisp_css2_dbg_print’ might be a candidate for
> ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> drivers/staging/media/atomisp//pci/atomisp_compat_css20.c:170:2:
> warning: function ‘atomisp_css2_dbg_ftrace_print’ might be a candidate for
> ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> drivers/staging/media/atomisp//pci/atomisp_compat_css20.c:170:2:
> warning: function ‘atomisp_css2_dbg_ftrace_print’ might be a candidate for
> ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> drivers/staging/media/atomisp//pci/atomisp_compat_css20.c:176:2:
> warning: function ‘atomisp_css2_err_print’ might be a candidate for
> ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
>
> That are due to the usage of printf-like messages without
> enabling the error checking logic.
>
> Add the proper attributes in order to shut up such warnings.
> +static int __attribute__((format (printf, 1, 0)))
> +atomisp_css2_dbg_ftrace_print(const char *fmt, va_list args)
> {
> ftrace_vprintk(fmt, args);
> return 0;
> }
>
Why not to drop it completely as well?
> -static int atomisp_css2_err_print(const char *fmt, va_list args)
> -{
> - vprintk(fmt, args);
> - return 0;
> -}
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH][next] staging: media: atomisp: fix memory leak of object flash
On Wed, Sep 2, 2020 at 8:02 PM Colin King wrote:
>
> From: Colin Ian King
>
> In the case where the call to lm3554_platform_data_func returns an
> error there is a memory leak on the error return path of object
> flash. Fix this by adding an error return path that will free
> flash and rename labels fail2 to fail3 and fail1 to fail2.
>
Wouldn't be proper fix to move to devm_kmalloc() and return
dev_err_probe() where appropriate?
> Fixes: 9289cdf39992 ("staging: media: atomisp: Convert to GPIO descriptors")
> Signed-off-by: Colin Ian King
> ---
> .../media/atomisp/i2c/atomisp-lm3554.c| 19 +++
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
> b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
> index 7ca7378b1859..5516c98f63bc 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
> @@ -843,8 +843,10 @@ static int lm3554_probe(struct i2c_client *client)
> return -ENOMEM;
>
> flash->pdata = lm3554_platform_data_func(client);
> - if (IS_ERR(flash->pdata))
> - return PTR_ERR(flash->pdata);
> + if (IS_ERR(flash->pdata)) {
> + err = PTR_ERR(flash->pdata);
> + goto fail1;
> + }
>
> v4l2_i2c_subdev_init(>sd, client, _ops);
> flash->sd.internal_ops = _internal_ops;
> @@ -856,7 +858,7 @@ static int lm3554_probe(struct i2c_client *client)
>ARRAY_SIZE(lm3554_controls));
> if (ret) {
> dev_err(>dev, "error initialize a ctrl_handler.\n");
> - goto fail2;
> + goto fail3;
> }
>
> for (i = 0; i < ARRAY_SIZE(lm3554_controls); i++)
> @@ -865,14 +867,14 @@ static int lm3554_probe(struct i2c_client *client)
>
> if (flash->ctrl_handler.error) {
> dev_err(>dev, "ctrl_handler error.\n");
> - goto fail2;
> + goto fail3;
> }
>
> flash->sd.ctrl_handler = >ctrl_handler;
> err = media_entity_pads_init(>sd.entity, 0, NULL);
> if (err) {
> dev_err(>dev, "error initialize a media entity.\n");
> - goto fail1;
> + goto fail2;
> }
>
> flash->sd.entity.function = MEDIA_ENT_F_FLASH;
> @@ -884,14 +886,15 @@ static int lm3554_probe(struct i2c_client *client)
> err = lm3554_gpio_init(client);
> if (err) {
> dev_err(>dev, "gpio request/direction_output fail");
> - goto fail2;
> + goto fail3;
> }
> return atomisp_register_i2c_module(>sd, NULL, LED_FLASH);
> -fail2:
> +fail3:
> media_entity_cleanup(>sd.entity);
> v4l2_ctrl_handler_free(>ctrl_handler);
> -fail1:
> +fail2:
> v4l2_device_unregister_subdev(>sd);
> +fail1:
> kfree(flash);
>
> return err;
> --
> 2.27.0
>
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v11 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset
On Mon, Aug 24, 2020 at 03:30:20PM -0400, Jim Quinlan wrote:
> The new field 'dma_range_map' in struct device is used to facilitate the
> use of single or multiple offsets between mapping regions of cpu addrs and
> dma addrs. It subsumes the role of "dev->dma_pfn_offset" which was only
> capable of holding a single uniform offset and had no region bounds
> checking.
>
> The function of_dma_get_range() has been modified so that it takes a single
> argument -- the device node -- and returns a map, NULL, or an error code.
> The map is an array that holds the information regarding the DMA regions.
> Each range entry contains the address offset, the cpu_start address, the
> dma_start address, and the size of the region.
>
> of_dma_configure() is the typical manner to set range offsets but there are
> a number of ad hoc assignments to "dev->dma_pfn_offset" in the kernel
> driver code. These cases now invoke the function
> dma_attach_offset_range(dev, cpu_addr, dma_addr, size).
...
> + /*
> + * Record all info in the generic DMA ranges array for struct device.
> + */
> + *map = r;
> + for_each_of_range(, ) {
> + pr_debug("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n",
> + range.bus_addr, range.cpu_addr, range.size);
> + r->cpu_start = range.cpu_addr;
> + r->dma_start = range.bus_addr;
> + r->size = range.size;
> + r->offset = (u64)range.cpu_addr - (u64)range.bus_addr;
What's the point in explicit castings to the same type?
> + r++;
> + }
...
> + phys_addr_t paddr;
> + dma_addr_t dma_addr;
> + struct device dev_bogus;
> unittest(paddr == expect_paddr,
> - "of_dma_get_range wrong phys addr (%llx) on node
> %pOF", paddr, np);
> + "of_dma_get_range: wrong phys addr %llx (expecting
> %llx) on node %pOF\n",
> + (u64)paddr, expect_paddr, np);
%llx -> %pap
> unittest(dma_addr == expect_dma_addr,
> - "of_dma_get_range wrong DMA addr (%llx) on node %pOF",
> dma_addr, np);
> + "of_dma_get_range: wrong DMA addr %llx (expecting
> %llx) on node %pOF\n",
> + (u64)dma_addr, expect_dma_addr, np);
%llx -> %pad
...
> + if (mem->use_dev_dma_pfn_offset) {
> + u64 base_addr = PFN_PHYS((u64)mem->pfn_base);
Do we need explicit casting here?
> +
> + return base_addr - dma_offset_from_phys_addr(dev, base_addr);
> + }
...
> +int dma_set_offset_range(struct device *dev, phys_addr_t cpu_start,
> + dma_addr_t dma_start, u64 size)
> +{
> + struct bus_dma_region *map;
> + u64 offset = (u64)cpu_start - (u64)dma_start;
> +
> + if (dev->dma_range_map) {
> + dev_err(dev, "attempt to add DMA range to existing map\n");
> + return -EINVAL;
> + }
Wouldn't be better to do an assignment of offset here?
> + if (!offset)
> + return 0;
> +
> + map = kcalloc(2, sizeof(*map), GFP_KERNEL);
> + if (!map)
> + return -ENOMEM;
> + map[0].cpu_start = cpu_start;
> + map[0].dma_start = dma_start;
> + map[0].offset = offset;
> + map[0].size = size;
> + dev->dma_range_map = map;
> +
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RESEND v10 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset
On Thu, Aug 20, 2020 at 09:37:12AM -0400, Jim Quinlan wrote:
> On Tue, Aug 18, 2020 at 4:14 AM Andy Shevchenko
> wrote:
> > On Mon, Aug 17, 2020 at 05:53:09PM -0400, Jim Quinlan wrote:
...
> > > +static inline u64 dma_offset_from_dma_addr(struct device *dev,
> > > dma_addr_t dma_addr)
> > > +{
> > > + const struct bus_dma_region *m = dev->dma_range_map;
> > > +
> > > + if (!m)
> > > + return 0;
> > > + for (; m->size; m++)
> > > + if (dma_addr >= m->dma_start && dma_addr - m->dma_start <
> > > m->size)
> > > + return m->offset;
> > > + return 0;
> > > +}
> > > +
> > > +static inline u64 dma_offset_from_phys_addr(struct device *dev,
> > > phys_addr_t paddr)
> > > +{
> > > + const struct bus_dma_region *m = dev->dma_range_map;
> > > +
> > > + if (!m)
> > > + return 0;
> > > + for (; m->size; m++)
> > > + if (paddr >= m->cpu_start && paddr - m->cpu_start < m->size)
> > > + return m->offset;
> > > + return 0;
> > > +}
> >
> > Perhaps for these the form with one return 0 is easier to read
> >
> > if (m) {
> > for (; m->size; m++)
> > if (paddr >= m->cpu_start && paddr - m->cpu_start <
> > m->size)
> > return m->offset;
> > }
> > return 0;
> >
> > ?
> I see what you are saying but I don't think there is enough difference
> between the two to justify changing it.
The difference is that you have return 0 / non-0 cases one each. I think it's
slightly easier to read and understand, but it's up to you.
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RESEND v10 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset
On Mon, Aug 17, 2020 at 05:53:09PM -0400, Jim Quinlan wrote:
> The new field 'dma_range_map' in struct device is used to facilitate the
> use of single or multiple offsets between mapping regions of cpu addrs and
> dma addrs. It subsumes the role of "dev->dma_pfn_offset" which was only
> capable of holding a single uniform offset and had no region bounds
> checking.
>
> The function of_dma_get_range() has been modified so that it takes a single
> argument -- the device node -- and returns a map, NULL, or an error code.
> The map is an array that holds the information regarding the DMA regions.
> Each range entry contains the address offset, the cpu_start address, the
> dma_start address, and the size of the region.
>
> of_dma_configure() is the typical manner to set range offsets but there are
> a number of ad hoc assignments to "dev->dma_pfn_offset" in the kernel
> driver code. These cases now invoke the function
> dma_attach_offset_range(dev, cpu_addr, dma_addr, size).
...
> + if (dev) {
> + phys_addr_t paddr = PFN_PHYS(pfn);
> +
> + pfn -= (dma_offset_from_phys_addr(dev, paddr) >> PAGE_SHIFT);
PFN_DOWN() ?
> + }
...
> + pfn += (dma_offset_from_dma_addr(dev, addr) >> PAGE_SHIFT);
Ditto.
...
> +static inline u64 dma_offset_from_dma_addr(struct device *dev, dma_addr_t
> dma_addr)
> +{
> + const struct bus_dma_region *m = dev->dma_range_map;
> +
> + if (!m)
> + return 0;
> + for (; m->size; m++)
> + if (dma_addr >= m->dma_start && dma_addr - m->dma_start <
> m->size)
> + return m->offset;
> + return 0;
> +}
> +
> +static inline u64 dma_offset_from_phys_addr(struct device *dev, phys_addr_t
> paddr)
> +{
> + const struct bus_dma_region *m = dev->dma_range_map;
> +
> + if (!m)
> + return 0;
> + for (; m->size; m++)
> + if (paddr >= m->cpu_start && paddr - m->cpu_start < m->size)
> + return m->offset;
> + return 0;
> +}
Perhaps for these the form with one return 0 is easier to read
if (m) {
for (; m->size; m++)
if (paddr >= m->cpu_start && paddr - m->cpu_start <
m->size)
return m->offset;
}
return 0;
?
...
> + if (mem->use_dev_dma_pfn_offset) {
> + u64 base_addr = (u64)mem->pfn_base << PAGE_SHIFT;
PFN_PHYS() ?
> +
> + return base_addr - dma_offset_from_phys_addr(dev, base_addr);
> + }
...
> + * It returns -ENOMEM if out of memory, 0 otherwise.
This doesn't describe cases dev->dma_range_map != NULL and offset == 0.
> +int dma_set_offset_range(struct device *dev, phys_addr_t cpu_start,
> + dma_addr_t dma_start, u64 size)
> +{
> + struct bus_dma_region *map;
> + u64 offset = (u64)cpu_start - (u64)dma_start;
> +
> + if (!offset)
> + return 0;
> +
> + if (dev->dma_range_map) {
> + dev_err(dev, "attempt to add DMA range to existing map\n");
> + return -EINVAL;
> + }
> +
> + map = kcalloc(2, sizeof(*map), GFP_KERNEL);
> + if (!map)
> + return -ENOMEM;
> + map[0].cpu_start = cpu_start;
> + map[0].dma_start = dma_start;
> + map[0].offset = offset;
> + map[0].size = size;
> + dev->dma_range_map = map;
> +
> + return 0;
> +}
...
> +void *dma_copy_dma_range_map(const struct bus_dma_region *map)
> +{
> + int num_ranges;
> + struct bus_dma_region *new_map;
> + const struct bus_dma_region *r = map;
> +
> + for (num_ranges = 0; r->size; num_ranges++)
> + r++;
> + new_map = kcalloc(num_ranges + 1, sizeof(*map), GFP_KERNEL);
> + if (new_map)
> + memcpy(new_map, map, sizeof(*map) * num_ranges);
Looks like krealloc() on the first glance...
> +
> + return new_map;
> +}
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: atomisp: move null check to earlier point
On Fri, Jul 31, 2020 at 01:17:38AM +0300, Cengiz Can wrote:
> `find_gmin_subdev` function that returns a pointer to `struct
func() are referred with name followed by parentheses.
> gmin_subdev` can return NULL.
> In `gmin_v2p8_ctrl` there's a call to this function but the possibility
> of a NULL was not checked before its being dereferenced. ie:
'. ie:' -> ', i.e.:'
> ```
Instead just shift right by two spaces.
> /* Acquired here v */
> struct gmin_subdev *gs = find_gmin_subdev(subdev);
>
> /* v--Dereferenced here */
> if (gs->v2p8_gpio >= 0) {
> ...
> }
> ```
Drop this as per above comment.
> To avoid the issue, null check has been moved to an earlier point
> and return semantics has been changed to reflect this exception.
> Please do note that this change introduces a new return value to
> `gmin_v2p8_ctrl`.
This rather should explain better the semantics change, e.g.
"Now the function() refuses to take NULL argument and returns an error. We also
WARN() for sake of the debugging."
> [NEW] - raise a WARN and return -ENODEV if there are no subdevices.
> - return result of `gpio_request` or `gpio_direction_output`.
> - return 0 if GPIO is ON.
> - return results of `regulator_enable` or `regulator_disable`.
> - according to PMIC type, return result of `axp_regulator_set`
> or `gmin_i2c_write`.
> - return -EINVAL if unknown PMIC type.
This has to go after cutter '---' line below.
> Caught-by: Coverity Static Analyzer CID 1465536
Reported-by:
And as discussed previously,
Suggested-by: Mauro ...
> Signed-off-by: Cengiz Can
The code looks good enough (WARN() will probably go away when driver gains
better quality).
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1] staging: fieldbus: Use %pM format specifier for MAC addresses
Convert to %pM instead of using custom code.
While here, replace one time use structure by buffer on stack.
Signed-off-by: Andy Shevchenko
---
drivers/staging/fieldbus/anybuss/hms-profinet.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/fieldbus/anybuss/hms-profinet.c
b/drivers/staging/fieldbus/anybuss/hms-profinet.c
index 31c43a0a5776..505480fb281d 100644
--- a/drivers/staging/fieldbus/anybuss/hms-profinet.c
+++ b/drivers/staging/fieldbus/anybuss/hms-profinet.c
@@ -26,10 +26,6 @@
* exactly as advertised.
*/
-struct msg_mac_addr {
- u8 addr[6];
-};
-
struct profi_priv {
struct fieldbus_dev fbdev;
struct anybuss_client *client;
@@ -59,17 +55,13 @@ static int profi_id_get(struct fieldbus_dev *fbdev, char
*buf,
size_t max_size)
{
struct profi_priv *priv = container_of(fbdev, struct profi_priv, fbdev);
- struct msg_mac_addr response;
+ u8 mac[ETH_ALEN];
int ret;
- ret = anybuss_recv_msg(priv->client, 0x0010, ,
- sizeof(response));
+ ret = anybuss_recv_msg(priv->client, 0x0010, , sizeof(mac));
if (ret < 0)
return ret;
- return snprintf(buf, max_size, "%02X:%02X:%02X:%02X:%02X:%02X\n",
- response.addr[0], response.addr[1],
- response.addr[2], response.addr[3],
- response.addr[4], response.addr[5]);
+ return snprintf(buf, max_size, "%pM\n", mac);
}
static bool profi_enable_get(struct fieldbus_dev *fbdev)
--
2.27.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1] staging: ks7010: Use %pM format specifier for MAC addresses
Convert to %pM instead of using custom code.
Signed-off-by: Andy Shevchenko
---
drivers/staging/ks7010/ks_hostif.c | 11 +++
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/ks7010/ks_hostif.c
b/drivers/staging/ks7010/ks_hostif.c
index d70b671b06aa..eaaf6a5440a9 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -161,7 +161,7 @@ int get_current_ap(struct ks_wlan_private *priv, struct
link_ap_info *ap_info)
wireless_send_event(netdev, SIOCGIWAP, , NULL);
}
netdev_dbg(priv->net_dev, "Link AP\n"
- "- bssid=%02X:%02X:%02X:%02X:%02X:%02X\n"
+ "- bssid=%pM\n"
"- essid=%s\n"
"- rate_set=%02X,%02X,%02X,%02X,%02X,%02X,%02X,%02X\n"
"- channel=%d\n"
@@ -172,8 +172,7 @@ int get_current_ap(struct ks_wlan_private *priv, struct
link_ap_info *ap_info)
"- rsn.size=%d\n"
"- ext_rate_set_size=%d\n"
"- rate_set_size=%d\n",
- ap->bssid[0], ap->bssid[1], ap->bssid[2],
- ap->bssid[3], ap->bssid[4], ap->bssid[5],
+ ap->bssid,
>ssid.body[0],
ap->rate_set.body[0], ap->rate_set.body[1],
ap->rate_set.body[2], ap->rate_set.body[3],
@@ -439,11 +438,7 @@ void hostif_data_indication(struct ks_wlan_private *priv)
/* source address check */
if (ether_addr_equal(>eth_addr[0], eth_hdr->h_source)) {
netdev_err(priv->net_dev, "invalid : source is own mac address
!!\n");
- netdev_err(priv->net_dev,
-
"eth_hdrernet->h_dest=%02X:%02X:%02X:%02X:%02X:%02X\n",
- eth_hdr->h_source[0], eth_hdr->h_source[1],
- eth_hdr->h_source[2], eth_hdr->h_source[3],
- eth_hdr->h_source[4], eth_hdr->h_source[5]);
+ netdev_err(priv->net_dev, "eth_hdrernet->h_dest=%pM\n",
eth_hdr->h_source);
priv->nstats.rx_errors++;
return;
}
--
2.27.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1] staging: most: Use %pM format specifier for MAC addresses
Convert to %pM instead of using custom code.
Signed-off-by: Andy Shevchenko
---
drivers/staging/most/net/net.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/most/net/net.c b/drivers/staging/most/net/net.c
index 830f089f1a88..b6fecb06a0e6 100644
--- a/drivers/staging/most/net/net.c
+++ b/drivers/staging/most/net/net.c
@@ -564,13 +564,11 @@ static void on_netinfo(struct most_interface *iface,
if (m && is_valid_ether_addr(m)) {
if (!is_valid_ether_addr(dev->dev_addr)) {
- netdev_info(dev, "set mac
%02x-%02x-%02x-%02x-%02x-%02x\n",
- m[0], m[1], m[2], m[3], m[4], m[5]);
+ netdev_info(dev, "set mac %pM\n", m);
ether_addr_copy(dev->dev_addr, m);
netif_dormant_off(dev);
} else if (!ether_addr_equal(dev->dev_addr, m)) {
- netdev_warn(dev, "reject mac
%02x-%02x-%02x-%02x-%02x-%02x\n",
- m[0], m[1], m[2], m[3], m[4], m[5]);
+ netdev_warn(dev, "reject mac %pM\n", m);
}
}
--
2.27.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: atomisp: move null check to earlier point
On Wed, Jul 29, 2020 at 5:00 PM Cengiz Can wrote:
>
> `find_gmin_subdev` function that returns a pointer to `struct
> gmin_subdev` can return NULL.
>
> In `gmin_v2p8_ctrl` there's a call to this function but the possibility
> of a NULL was not checked before its being dereferenced. ie:
>
> ```
> /* Acquired here v */
> struct gmin_subdev *gs = find_gmin_subdev(subdev);
> int ret;
> int value;
>
> /* v--Dereferenced here */
> if (gs->v2p8_gpio >= 0) {
> pr_info("atomisp_gmin_platform: 2.8v power on GPIO %d\n",
> gs->v2p8_gpio);
> ret = gpio_request(gs->v2p8_gpio, "camera_v2p8");
> if (!ret)
> ret = gpio_direction_output(gs->v2p8_gpio, 0);
> if (ret)
> pr_err("V2P8 GPIO initialization failed\n");
> }
> ```
>
> I have moved the NULL check before deref point.
"Move the NULL check..."
See Submitting Patches documentation how to avoid "This patch", "I", "we", etc.
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> index 0df46a1af5f0..8e9c5016f299 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> @@ -871,6 +871,11 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev,
> int on)
> int ret;
> int value;
>
> + if (!gs) {
> + pr_err("Unable to find gmin subdevice\n");
> + return -EINVAL;
And here is a change of semantics...
> + }
...
> - if (!gs || gs->v2p8_on == on)
> + if (gs->v2p8_on == on)
> return 0;
...compared to above.
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] Staging: media: atomisp: fixed a brace coding sytle issue
On Wed, Jul 29, 2020 at 01:19:50PM +0530, Ankit Baluni wrote:
> Removed braces for a 'if' condition as it contain only single line &
> there is no need for braces for such case according to coding style
> rules.
Reviewed-by: Andy Shevchenko
> Signed-off-by: Ankit Baluni
>
> ---
> Changes in v2:
> -Added more description about the patch.
> -Added space before the symobol '<' in 'From'
>and 'Signed-off-by' line.
> Changes in v3:
> -Removed space before ':' in subject line.
>
> drivers/staging/media/atomisp/pci/atomisp_cmd.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> index 8ea65bef35d2..28b96b66f4f3 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> @@ -4981,9 +4981,8 @@ enum mipi_port_id __get_mipi_port(struct atomisp_device
> *isp,
> case ATOMISP_CAMERA_PORT_SECONDARY:
> return MIPI_PORT1_ID;
> case ATOMISP_CAMERA_PORT_TERTIARY:
> - if (MIPI_PORT1_ID + 1 != N_MIPI_PORT_ID) {
> + if (MIPI_PORT1_ID + 1 != N_MIPI_PORT_ID)
> return MIPI_PORT1_ID + 1;
> - }
> /* fall through */
> default:
> dev_err(isp->dev, "unsupported port: %d\n", port);
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH -v2] Staging: iio: Fixed a punctuation and a spelling mistake.
On Wed, Jul 29, 2020 at 11:12 AM Ankit Baluni wrote: > > Added a missing comma and changed 'it it useful' to 'it is useful'. Reviewed-by: Andy Shevchenko > Signed-off-by: Ankit Baluni > --- > Changes in -v2: > -Remove space before ':' in subject line. > > drivers/staging/iio/Documentation/overview.txt | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/iio/Documentation/overview.txt > b/drivers/staging/iio/Documentation/overview.txt > index ebdc64f451d7..00409d5dab4e 100644 > --- a/drivers/staging/iio/Documentation/overview.txt > +++ b/drivers/staging/iio/Documentation/overview.txt > @@ -9,7 +9,7 @@ The aim is to fill the gap between the somewhat similar hwmon > and > input subsystems. Hwmon is very much directed at low sample rate > sensors used in applications such as fan speed control and temperature > measurement. Input is, as its name suggests focused on input > -devices. In some cases there is considerable overlap between these and > +devices. In some cases, there is considerable overlap between these and > IIO. > > A typical device falling into this category would be connected via SPI > @@ -38,7 +38,7 @@ series and Analog Devices ADXL345 accelerometers. Each > buffer supports > polling to establish when data is available. > > * Trigger and software buffer support. In many data analysis > -applications it it useful to be able to capture data based on some > +applications it is useful to be able to capture data based on some > external signal (trigger). These triggers might be a data ready > signal, a gpio line connected to some external system or an on > processor periodic interrupt. A single trigger may initialize data > -- > 2.25.1 > -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging : iio : Fixed a punctuation and a spelling mistake.
On Wed, Jul 29, 2020 at 3:34 AM Ankit Baluni wrote: > > Added a missing comma and changed 'it it useful' to 'it is useful'. Please, drop spaces before : in the subject line. In all patches you submitted there is such an issue. -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging : media : atomisp : pci : fixed a brace coding sytle issue
On Mon, Jul 27, 2020 at 01:31:50PM +0530, Ankit wrote: > From: Ankit Baluni > > Fixed a coding style issue. One time is enough to be sent :-) The Subject nevertheless can be amended, like media: atomisp: fixed a brace coding sytle issue -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 08/12] device core: Introduce DMA range map, supplanting dma_pfn_offset
On Wed, Jul 01, 2020 at 05:21:38PM -0400, Jim Quinlan wrote: > The new field 'dma_range_map' in struct device is used to facilitate the > use of single or multiple offsets between mapping regions of cpu addrs and > dma addrs. It subsumes the role of "dev->dma_pfn_offset" which was only > capable of holding a single uniform offset and had no region bounds > checking. > > The function of_dma_get_range() has been modified so that it takes a single > argument -- the device node -- and returns a map, NULL, or an error code. > The map is an array that holds the information regarding the DMA regions. > Each range entry contains the address offset, the cpu_start address, the > dma_start address, and the size of the region. > > of_dma_configure() is the typical manner to set range offsets but there are > a number of ad hoc assignments to "dev->dma_pfn_offset" in the kernel > driver code. These cases now invoke the function > dma_attach_offset_range(dev, cpu_addr, dma_addr, size). ... > + if (dev && dev->dma_range_map) > + pfn -= (unsigned long)PFN_DOWN(dma_offset_from_phys_addr(dev, > PFN_PHYS(pfn))); Instead of casting use PHYS_PFN() and it will be consistent with latter in the same line. > + if (dev && dev->dma_range_map) > + pfn += (unsigned long)PFN_DOWN(dma_offset_from_dma_addr(dev, > addr)); Ditto. ... > + dev_err(dev, "set dma_offset%08llx%s\n", > KEYSTONE_HIGH_PHYS_START > + - KEYSTONE_LOW_PHYS_START, ret ? " failed" : ""); Please, avoid such indentation. Better split it to the three lines, argument per line (expect dev which will go on the first one). This applies to all similar places. ... > unsigned long pfn = (dma_handle >> PAGE_SHIFT); PHYS_PFN() / PFN_DOWN() ? > + if (!WARN_ON(!dev) && dev->dma_range_map) > + pfn += (unsigned long)PFN_DOWN(dma_offset_from_dma_addr(dev, > dma_handle)); PHYS_PFN() ? ... > + r = kcalloc(num_ranges + 1, sizeof(struct bus_dma_region), GFP_KERNEL); sizeof(*r) ? > + if (!r) > + return ERR_PTR(-ENOMEM); ... > + ret = IS_ERR(map) ? PTR_ERR(map) : 0; PTR_ERR_OR_ZERO() ... > + /* We want the offset map to be device-managed, so alloc & copy > */ > + dev->dma_range_map = devm_kcalloc(dev, num_ranges + 1, > sizeof(*r), > + GFP_KERNEL); The question is how many times per device lifetime this can be called? ... > + if (!dev->dma_range_map) > + return -ENOMEM; > + memcpy((void *)dev->dma_range_map, map, sizeof(*r) * num_ranges > + 1); If it's continuous, perhaps kmemdup() ? ... > + rc = IS_ERR(map) ? PTR_ERR(map) : 0; PTR_ERR_OR_ZERO() ... > + = dma_offset_from_phys_addr(dev, > PFN_PHYS(mem->pfn_base)); > + > + return (dma_addr_t)PFN_PHYS(mem->pfn_base) - dma_offset; Looking at this more, I think you need to introduce in the same header (pfn.h) something like: #define PFN_DMA_ADDR() #define DMA_ADDR_PFN() -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/7] Some atomisp fixes and improvements
On Fri, Jun 26, 2020 at 05:52:16PM +0200, Mauro Carvalho Chehab wrote: > Em Fri, 26 Jun 2020 18:00:21 +0300 > Andy Shevchenko escreveu: > > > On Fri, Jun 26, 2020 at 04:04:52PM +0200, Mauro Carvalho Chehab wrote: > > > Those patches are meant to improve device detection by the atomisp driver, > > > relying on ACPI bios when possible. > > > > > > It also adds a basis for using ACPI PM, but only if the DSDT tables have > > > a description about how to turn on the resources needed by the cameras. > > > > > > At least on the device I'm using for tests, this is not the case. > > > > Is this in your experimental tree? > > Yes. > > > I'll rebase mine on top and test. > > After I will send the rest from my series and give a tag to this. > > It would be helpful if you could test removing the DMI match table from > your board. If your device has a DSDT table close to the one I have, the > new code may be able to get everything from DSDT. I have checked the atomisp_v5 branch and it doesn't bring any regression to my case. So, feel free to add Tested-by: Andy Shevchenko I'll send rebased patches soon. -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/7] Some atomisp fixes and improvements
On Fri, Jun 26, 2020 at 04:04:52PM +0200, Mauro Carvalho Chehab wrote: > Those patches are meant to improve device detection by the atomisp driver, > relying on ACPI bios when possible. > > It also adds a basis for using ACPI PM, but only if the DSDT tables have > a description about how to turn on the resources needed by the cameras. > > At least on the device I'm using for tests, this is not the case. Is this in your experimental tree? I'll rebase mine on top and test. After I will send the rest from my series and give a tag to this. > > Mauro Carvalho Chehab (7): > media: atomisp: reorganize the code under gmin_subdev_add() > media: atomisp: Prepare sensor support for ACPI PM > media: atomisp: properly parse CLK PMIC on newer devices > media: atomisp: fix call to g_frame_interval > media: atomisp: print info if gpio0 and gpio2 were detected > media: atomisp: split add from find subdev > media: atomisp: place all gpio parsing together > > .../staging/media/atomisp/pci/atomisp_cmd.c | 2 +- > .../media/atomisp/pci/atomisp_gmin_platform.c | 393 -- > 2 files changed, 267 insertions(+), 128 deletions(-) > > -- > 2.26.2 > > -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 08/12] device core: Introduce multiple dma pfn offsets
On Mon, Jun 08, 2020 at 11:48:51AM -0400, Jim Quinlan wrote: > On Sun, Jun 7, 2020 at 12:500f9bfe0fb8840b268af1bbcc51f1cd440514e PM > Andy Shevchenko wrote: > > On Fri, Jun 05, 2020 at 05:26:48PM -0400, Jim Quinlan wrote: ... > > > + *map_size = (num_ranges + 1) * sizeof(**map); > > > + r = kzalloc(*map_size, GFP_KERNEL); > > > > kcalloc() > Since I have to calculate the size anyway I thought kzalloc was fine. > I'll switch. The point is to check multiplication overflow. See overflow.h for helpers. > > > + if (!r) > > > + return -ENOMEM; -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 08/12] device core: Introduce multiple dma pfn offsets
On Fri, Jun 05, 2020 at 05:26:48PM -0400, Jim Quinlan wrote: > The new field in struct device 'dma_pfn_offset_map' is used to facilitate > the use of single or multiple pfn offsets between cpu addrs and dma addrs. > It subsumes the role of dev->dma_pfn_offset -- a uniform offset. > > The function of_dma_get_range() has been modified to take two additional > arguments: the "map", which is an array that holds the information > regarding the pfn offset regions, and map_size, which is the size in bytes > of the map array. > > of_dma_configure() is the typical manner to set pfn offsets but there are a > number of ad hoc assignments to dev->dma_pfn_offset in the kernel driver > code. These cases now invoke the function > dma_attach_uniform_pfn_offset(dev, pfn_offset). ... > + int ret = dma_attach_uniform_pfn_offset > + (dev, keystone_dma_pfn_offset); It's strange indentation. Have you configured your editor correctly? Seems to me as fit on one line. > + dev_err(dev, "set dma_pfn_offset%08lx%s\n", > + dev->dma_pfn_offset, ret ? " failed" : ""); ... > + *map_size = (num_ranges + 1) * sizeof(**map); > + r = kzalloc(*map_size, GFP_KERNEL); kcalloc() > + if (!r) > + return -ENOMEM; ... > + r->pfn_offset = PFN_DOWN(range.cpu_addr) > + - PFN_DOWN(range.bus_addr); Ditto (indentation). ... > + unsigned long dma_pfn_offset > + = dma_pfn_offset_from_phys_addr(dev, paddr); Ditto. ... > + unsigned long dma_pfn_offset > + = dma_pfn_offset_from_dma_addr(dev, dev_addr); Ditto. Check entire your series for a such, please! -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 09/13] device core: Introduce multiple dma pfn offsets
On Thu, Jun 04, 2020 at 10:35:12AM -0400, Jim Quinlan wrote: > On Thu, Jun 4, 2020 at 9:53 AM Nicolas Saenz Julienne > wrote: > > On Wed, 2020-06-03 at 15:20 -0400, Jim Quinlan wrote: ... > > > + phys = virt_to_phys(ret); > > > + pfn = phys >> PAGE_SHIFT; > > > > nit: not sure it really pays off to have a pfn variable here. > Did it for readability; the compiler's optimization should take care > of any extra variables. But I can switch if you insist. One side note: please, try to get familiar with existing helpers in the kernel. For example, above line is like pfn = PFN_DOWN(phys); ... > > > + if (!WARN_ON(!dev) && dev->dma_pfn_offset_map) > > > + *dma_handle -= PFN_PHYS( > > > + dma_pfn_offset_from_phys_addr(dev, phys)); Don't do such indentation, esp. we have now 100! :-) -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/5] iio: core: pass parent device as parameter during allocation
On Fri, May 22, 2020 at 11:36 AM Alexandru Ardelean
wrote:
>
> The change passes the parent device to the iio_device_alloc() call. This
> also updates the devm_iio_device_alloc() call to consider the device object
> as the parent device by default.
>
> Having it passed like this, should ensure that any IIO device object
> already has a device object as parent, allowing for neater control, like
> passing the 'indio_dev' object for other stuff [like buffers/triggers/etc],
> and potentially creating iiom_xxx(indio_dev) functions.
>
> With this patch, only the 'drivers/platform/x86/toshiba_acpi.c' needs an
> update to pass the parent object as a parameter.
Acked-by: Andy Shevchenko
>
> In the next patch all devm_iio_device_alloc() calls will be handled.
>
> Signed-off-by: Alexandru Ardelean
> ---
> drivers/iio/dummy/iio_simple_dummy.c | 14 --
> drivers/iio/industrialio-core.c | 11 ++-
> drivers/platform/x86/toshiba_acpi.c | 3 +--
> drivers/staging/iio/Documentation/device.txt | 4 +---
> include/linux/iio/iio.h | 4 ++--
> 5 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/iio/dummy/iio_simple_dummy.c
> b/drivers/iio/dummy/iio_simple_dummy.c
> index 6cb02299a215..b35ae7c039f7 100644
> --- a/drivers/iio/dummy/iio_simple_dummy.c
> +++ b/drivers/iio/dummy/iio_simple_dummy.c
> @@ -566,6 +566,13 @@ static struct iio_sw_device *iio_dummy_probe(const char
> *name)
> struct iio_dev *indio_dev;
> struct iio_dummy_state *st;
> struct iio_sw_device *swd;
> + struct device *parent = NULL;
> +
> + /*
> +* With hardware: Set the parent device.
> +* parent = >dev;
> +* parent = >dev;
> +*/
>
> swd = kzalloc(sizeof(*swd), GFP_KERNEL);
> if (!swd) {
> @@ -580,7 +587,7 @@ static struct iio_sw_device *iio_dummy_probe(const char
> *name)
> * It also has a region (accessed by iio_priv()
> * for chip specific state information.
> */
> - indio_dev = iio_device_alloc(sizeof(*st));
> + indio_dev = iio_device_alloc(parent, sizeof(*st));
> if (!indio_dev) {
> ret = -ENOMEM;
> goto error_ret;
> @@ -590,11 +597,6 @@ static struct iio_sw_device *iio_dummy_probe(const char
> *name)
> mutex_init(>lock);
>
> iio_dummy_init_device(indio_dev);
> - /*
> -* With hardware: Set the parent device.
> -* indio_dev->dev.parent = >dev;
> -* indio_dev->dev.parent = >dev;
> -*/
>
> /*
> * Make the iio_dev struct available to remove function.
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 1527f01a44f1..75661661aaba 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1493,7 +1493,7 @@ struct device_type iio_device_type = {
> * iio_device_alloc() - allocate an iio_dev from a driver
> * @sizeof_priv: Space to allocate for private structure.
> **/
> -struct iio_dev *iio_device_alloc(int sizeof_priv)
> +struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
> {
> struct iio_dev *dev;
> size_t alloc_size;
> @@ -1510,6 +1510,7 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
> if (!dev)
> return NULL;
>
> + dev->dev.parent = parent;
> dev->dev.groups = dev->groups;
> dev->dev.type = _device_type;
> dev->dev.bus = _bus_type;
> @@ -1551,7 +1552,7 @@ static void devm_iio_device_release(struct device *dev,
> void *res)
>
> /**
> * devm_iio_device_alloc - Resource-managed iio_device_alloc()
> - * @dev: Device to allocate iio_dev for
> + * @parent:Device to allocate iio_dev for, and parent for this
> IIO device
> * @sizeof_priv: Space to allocate for private structure.
> *
> * Managed iio_device_alloc. iio_dev allocated with this function is
> @@ -1560,7 +1561,7 @@ static void devm_iio_device_release(struct device *dev,
> void *res)
> * RETURNS:
> * Pointer to allocated iio_dev on success, NULL on failure.
> */
> -struct iio_dev *devm_iio_device_alloc(struct device *dev, int sizeof_priv)
> +struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv)
> {
> struct iio_dev **ptr, *iio_dev;
>
> @@ -1569,10 +1570,10 @@ struct iio_dev *devm_iio_device_alloc(struct device
> *dev, int sizeof_priv)
> if (!ptr)
> return NULL;
>
> - iio_dev = iio_device_alloc(sizeof_priv);
Re: [PATCH 5/5] iio: remove left-over parent assignments
On Fri, May 22, 2020 at 11:37 AM Alexandru Ardelean wrote: > > These were found by doing some shell magic: > > for file in $(git grep -w devm_iio_device_alloc | cut -d: -f1 | sort | uniq) > ; do > if grep 'parent =' $file | grep -v trig | grep -vq devm_; then > echo "$file -> $(grep "parent =" $file)" > fi > done > --- Side note: time to learn coccinelle or shell better :-) -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: ipu3: add a module to probe sensors via ACPI
+Cc: Heikki (swnode expert) On Wed, May 20, 2020 at 2:19 PM Mauro Carvalho Chehab wrote: > Em Wed, 20 May 2020 11:26:08 +0300 > Sakari Ailus escreveu: ... > As I said, the problem is not probing the sensor via ACPI, but, instead, > to be able receive platform-specific data. There is no problem with swnodes, except missing parts (*). I have Skylake laptop with IPU3 and with half-baked ACPI tables, but since we have drivers in place with fwnode support, we only need to recreate fwnode graph in some board file to compensate the gap in ACPI. *) Missing part is graph support for swnodes. With that done it will be feasible to achieve the rest. I forgot if we have anything for this already done. Heikki? -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH] staging: fbtft: Replace udelay with preferred usleep_range
On Sun, Mar 29, 2020 at 2:23 PM Sam Muhammed wrote: > On Sun, 2020-03-29 at 12:37 +0200, Julia Lawall wrote: > > On Sun, 29 Mar 2020, Soumyajit Deb wrote: > > First of all, let's stop topposting. > > > I had the same doubt the other day about the replacement of udelay() with > > > usleep_range(). The corresponding range for the single argument value of > > > udelay() is quite confusing as I couldn't decide the range. But as much > > > as I > > > noticed checkpatch.pl gives warning for replacing udelay() with > > > usleep_range() by checking the argument value of udelay(). In the > > > documentation, it is written udelay() should be used for a sleep time of > > > at > > > most 10 microseconds but between 10 microseconds and 20 milliseconds, > > > usleep_range() should be used. > > > I think the range is code specific and will depend on what range is > > > acceptable and doesn't break the code. > > > Please correct me if I am wrong. > > > > The range depends on the associated hardware. Just because checkpatch > > suggests something doesn't mean that it is easy to address the problem. > Hi all, i think when it comes to a significant change in the code, we > should at least be familiar with the driver or be able to test the > change. > > In the very beginning of the Documentation/timers/timers-howto.rst > there is the question: > "Is my code in an atomic context?" > It's not just about the range, it's more of at which context this code > runs, for atomic-context -> udelay must be used. > for non-atomic context -> usleep-range is better for power-management. > > unless we are familiar with the driver we wouldn't really know in what > context this code is run at. > > This thread though had the same conversation about this change, for the > same driver. > https://patchwork.kernel.org/patch/11137125/ While it's a good discussion it reminds me that this entire function, i.e. reset(), repeats the on provided by fbtft core. Yes, the only question if it's atomic or not. IIRC ->reset() is being called only in non-atomic contexts and keeping reset signal longer is fine (but better to check with datasheet). So, I would rather to drop the function completely in order to use fbtft's core one. -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 02/17] media: v4l2-fwnode: Pass notifier to v4l2_async_register_fwnode_subdev()
On Tue, Feb 04, 2020 at 03:49:03PM -0800, Steve Longerbeam wrote: > Instead of allocating a notifier in v4l2_async_register_fwnode_subdev(), > have the caller provide one. This allows the caller to implement > notifier ops (bind, unbind). > > The caller is now responsible for first initializing its notifier with a > call to v4l2_async_notifier_init(). Hint: When creating a list of maintainers use this (or similar) command: scripts/get_maintainer.pl --git --git-min-percent=67 ... P.S. Please, do not add me in Cc list for v4l2 matters. -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1] fbtft: Fix the initialization from property algorithm
When converting to device property API the commit
8b2d3aeeb7ec ("fbtft: Make use of device property API")
mistakenly placed the reading of the first value inside the loop,
that jumps over value after initialization sequence or sleep commands.
Move the above mentioned reading outside of the loop to restore
correct behaviour.
Besides that, we are using pre-increment operation which may lead to
out of the boundary access at the end of sequence. Thus, allocate buffer
with an additional element at the end to prevent out of the boundary
access.
Fixes: 8b2d3aeeb7ec ("fbtft: Make use of device property API")
Signed-off-by: Andy Shevchenko
---
drivers/staging/fbtft/fbtft-core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/fbtft/fbtft-core.c
b/drivers/staging/fbtft/fbtft-core.c
index e87d839d86ac..4fcc82b7de1b 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -913,7 +913,7 @@ static int fbtft_init_display_from_property(struct
fbtft_par *par)
if (count == 0)
return -EINVAL;
- values = kmalloc_array(count, sizeof(*values), GFP_KERNEL);
+ values = kmalloc_array(count + 1, sizeof(*values), GFP_KERNEL);
if (!values)
return -ENOMEM;
@@ -926,9 +926,9 @@ static int fbtft_init_display_from_property(struct
fbtft_par *par)
gpiod_set_value(par->gpio.cs, 0); /* Activate chip */
index = -1;
- while (index < count) {
- val = values[++index];
+ val = values[++index];
+ while (index < count) {
if (val & FBTFT_OF_INIT_CMD) {
val &= 0x;
i = 0;
--
2.24.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 3/5] fbtft: Drop useless #ifdef CONFIG_OF and dead code
On Wed, Nov 20, 2019 at 04:04:17PM +0100, Noralf Trønnes wrote: > Den 20.11.2019 15.43, skrev Noralf Trønnes: > > Den 20.11.2019 10.57, skrev Andy Shevchenko: > >> First of all there is no need to guard GPIO request by CONFIG_OF. > >> It works for everybody independently on resource provider. While here, > >> rename the function to reflect the above. > >> > >> Moreover, since we have a global dependency to OF, the rest of > >> conditional compilation is no-op, i.e. it's always be true. > >> > >> Due to above drop useless #ifdef CONFIG_OF and therefore dead code. > >> > >> Signed-off-by: Andy Shevchenko > >> --- > >> drivers/staging/fbtft/fbtft-core.c | 19 ++- > >> 1 file changed, 2 insertions(+), 17 deletions(-) > >> > >> diff --git a/drivers/staging/fbtft/fbtft-core.c > >> b/drivers/staging/fbtft/fbtft-core.c > > > > > > > >> @@ -1184,17 +1176,10 @@ static struct fbtft_platform_data > >> *fbtft_probe_dt(struct device *dev) > >>pdata->display.backlight = 1; > >>if (of_find_property(node, "init", NULL)) > >>pdata->display.fbtftops.init_display = fbtft_init_display_dt; > >> - pdata->display.fbtftops.request_gpios = fbtft_request_gpios_dt; > >> + pdata->display.fbtftops.request_gpios = fbtft_request_gpios; > > > > You can ditch the .request_gpios callback and call fbtft_request_gpios() > > directly in fbtft_register_framebuffer(). That will make it safe to drop > > the OF dependency, otherwise .request_gpios will be NULL in the non-DT > > case. This is one of the bugs that follwed the gpio refactoring. > > Really difficult to read this fbtft code (that I wrote...). > The NULL deref can only happen when dev->platform_data is set. That > can't happen, in mainline at least, now that fbtft_device is gone. Hmm... If I read code correctly this patch doesn't change this logic. We have non-NULL ->request_gpios() in case of pdata != NULL if and only if supplier gives it to us. The above assignment happens only for DT case (fbtft_properties_read() is guarded against non-DT, okay non-fwnode, cases). > > You can also ditch the .request_gpios_match callback if you want, it > > isn't called anymore (it is set in fb_agm1264k-fl). I guess both improvements can be done later since they are not affecting the logic in this series. -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 1/5] fbtft: Make sure string is NULL terminated
New GCC warns about inappropriate use of strncpy(): drivers/staging/fbtft/fbtft-core.c: In function ‘fbtft_framebuffer_alloc’: drivers/staging/fbtft/fbtft-core.c:665:2: warning: ‘strncpy’ specified bound 16 equals destination size [-Wstringop-truncation] 665 | strncpy(info->fix.id, dev->driver->name, 16); | ^~~~ Later on the copy is being used with the assumption to be NULL terminated. Make sure string is NULL terminated by switching to snprintf(). Signed-off-by: Andy Shevchenko --- drivers/staging/fbtft/fbtft-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c index a0a67aa517f0..61f0286fb157 100644 --- a/drivers/staging/fbtft/fbtft-core.c +++ b/drivers/staging/fbtft/fbtft-core.c @@ -666,7 +666,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display, fbdefio->deferred_io = fbtft_deferred_io; fb_deferred_io_init(info); - strncpy(info->fix.id, dev->driver->name, 16); + snprintf(info->fix.id, sizeof(info->fix.id), "%s", dev->driver->name); info->fix.type = FB_TYPE_PACKED_PIXELS; info->fix.visual = FB_VISUAL_TRUECOLOR; info->fix.xpanstep = 0; -- 2.24.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 3/5] fbtft: Drop useless #ifdef CONFIG_OF and dead code
First of all there is no need to guard GPIO request by CONFIG_OF.
It works for everybody independently on resource provider. While here,
rename the function to reflect the above.
Moreover, since we have a global dependency to OF, the rest of
conditional compilation is no-op, i.e. it's always be true.
Due to above drop useless #ifdef CONFIG_OF and therefore dead code.
Signed-off-by: Andy Shevchenko
---
drivers/staging/fbtft/fbtft-core.c | 19 ++-
1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/drivers/staging/fbtft/fbtft-core.c
b/drivers/staging/fbtft/fbtft-core.c
index 2122c4407bdb..ff8cb6670ea1 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -70,7 +70,6 @@ void fbtft_dbg_hex(const struct device *dev, int groupsize,
}
EXPORT_SYMBOL(fbtft_dbg_hex);
-#ifdef CONFIG_OF
static int fbtft_request_one_gpio(struct fbtft_par *par,
const char *name, int index,
struct gpio_desc **gpiop)
@@ -92,14 +91,11 @@ static int fbtft_request_one_gpio(struct fbtft_par *par,
return ret;
}
-static int fbtft_request_gpios_dt(struct fbtft_par *par)
+static int fbtft_request_gpios(struct fbtft_par *par)
{
int i;
int ret;
- if (!par->info->device->of_node)
- return -EINVAL;
-
ret = fbtft_request_one_gpio(par, "reset", 0, >gpio.reset);
if (ret)
return ret;
@@ -135,7 +131,6 @@ static int fbtft_request_gpios_dt(struct fbtft_par *par)
return 0;
}
-#endif
#ifdef CONFIG_FB_BACKLIGHT
static int fbtft_backlight_update_status(struct backlight_device *bd)
@@ -898,7 +893,6 @@ int fbtft_unregister_framebuffer(struct fb_info *fb_info)
}
EXPORT_SYMBOL(fbtft_unregister_framebuffer);
-#ifdef CONFIG_OF
/**
* fbtft_init_display_dt() - Device Tree init_display() function
* @par: Driver data
@@ -977,7 +971,6 @@ static int fbtft_init_display_dt(struct fbtft_par *par)
return 0;
}
-#endif
/**
* fbtft_init_display() - Generic init_display() function
@@ -1138,7 +1131,6 @@ static int fbtft_verify_gpios(struct fbtft_par *par)
return 0;
}
-#ifdef CONFIG_OF
/* returns 0 if the property is not present */
static u32 fbtft_of_value(struct device_node *node, const char *propname)
{
@@ -1184,17 +1176,10 @@ static struct fbtft_platform_data
*fbtft_probe_dt(struct device *dev)
pdata->display.backlight = 1;
if (of_find_property(node, "init", NULL))
pdata->display.fbtftops.init_display = fbtft_init_display_dt;
- pdata->display.fbtftops.request_gpios = fbtft_request_gpios_dt;
+ pdata->display.fbtftops.request_gpios = fbtft_request_gpios;
return pdata;
}
-#else
-static struct fbtft_platform_data *fbtft_probe_dt(struct device *dev)
-{
- dev_err(dev, "Missing platform data\n");
- return ERR_PTR(-EINVAL);
-}
-#endif
/**
* fbtft_probe_common() - Generic device probe() helper function
--
2.24.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 5/5] fbtft: Drop OF dependency
Now, since driver became OF independent, no need to keep OF dependency.
Signed-off-by: Andy Shevchenko
---
drivers/staging/fbtft/Kconfig | 2 +-
drivers/staging/fbtft/fbtft.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
index cb61c2a772bd..54751d9fc0ff 100644
--- a/drivers/staging/fbtft/Kconfig
+++ b/drivers/staging/fbtft/Kconfig
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
menuconfig FB_TFT
tristate "Support for small TFT LCD display modules"
- depends on FB && SPI && OF
+ depends on FB && SPI
depends on GPIOLIB || COMPILE_TEST
select FB_SYS_FILLRECT
select FB_SYS_COPYAREA
diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 9b6bdb62093d..5f782da51959 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -309,7 +309,7 @@ MODULE_DEVICE_TABLE(of, dt_ids);
\
static struct spi_driver fbtft_driver_spi_driver = { \
.driver = {\
.name = _name, \
- .of_match_table = of_match_ptr(dt_ids),\
+ .of_match_table = dt_ids, \
}, \
.probe = fbtft_driver_probe_spi, \
.remove = fbtft_driver_remove_spi, \
@@ -319,7 +319,7 @@ static struct platform_driver fbtft_driver_platform_driver
= { \
.driver = {\
.name = _name, \
.owner = THIS_MODULE, \
- .of_match_table = of_match_ptr(dt_ids),\
+ .of_match_table = dt_ids, \
}, \
.probe = fbtft_driver_probe_pdev, \
.remove = fbtft_driver_remove_pdev,\
--
2.24.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 4/5] fbtft: Make use of device property API
Make use of device property API in this driver so that both OF based
system and ACPI based system can use this driver.
Signed-off-by: Andy Shevchenko
---
drivers/staging/fbtft/fbtft-core.c | 105 -
1 file changed, 58 insertions(+), 47 deletions(-)
diff --git a/drivers/staging/fbtft/fbtft-core.c
b/drivers/staging/fbtft/fbtft-core.c
index ff8cb6670ea1..e87d839d86ac 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -22,8 +22,9 @@
#include
#include
#include
+#include
#include
-#include
+
#include
#include "fbtft.h"
@@ -894,44 +895,53 @@ int fbtft_unregister_framebuffer(struct fb_info *fb_info)
EXPORT_SYMBOL(fbtft_unregister_framebuffer);
/**
- * fbtft_init_display_dt() - Device Tree init_display() function
+ * fbtft_init_display_from_property() - Device Tree init_display() function
* @par: Driver data
*
* Return: 0 if successful, negative if error
*/
-static int fbtft_init_display_dt(struct fbtft_par *par)
+static int fbtft_init_display_from_property(struct fbtft_par *par)
{
- struct device_node *node = par->info->device->of_node;
- struct property *prop;
- const __be32 *p;
+ struct device *dev = par->info->device;
+ int buf[64], count, index, i, j, ret;
+ u32 *values;
u32 val;
- int buf[64], i, j;
- if (!node)
+ count = device_property_count_u32(dev, "init");
+ if (count < 0)
+ return count;
+ if (count == 0)
return -EINVAL;
- prop = of_find_property(node, "init", NULL);
- p = of_prop_next_u32(prop, NULL, );
- if (!p)
- return -EINVAL;
+ values = kmalloc_array(count, sizeof(*values), GFP_KERNEL);
+ if (!values)
+ return -ENOMEM;
+
+ ret = device_property_read_u32_array(dev, "init", values, count);
+ if (ret)
+ goto out_free;
par->fbtftops.reset(par);
if (par->gpio.cs)
gpiod_set_value(par->gpio.cs, 0); /* Activate chip */
- while (p) {
+ index = -1;
+ while (index < count) {
+ val = values[++index];
+
if (val & FBTFT_OF_INIT_CMD) {
val &= 0x;
i = 0;
- while (p && !(val & 0x)) {
+ while ((index < count) && !(val & 0x)) {
if (i > 63) {
- dev_err(par->info->device,
+ dev_err(dev,
"%s: Maximum register values
exceeded\n",
__func__);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_free;
}
buf[i++] = val;
- p = of_prop_next_u32(prop, p, );
+ val = values[++index];
}
/* make debug message */
fbtft_par_dbg(DEBUG_INIT_DISPLAY, par,
@@ -961,15 +971,17 @@ static int fbtft_init_display_dt(struct fbtft_par *par)
fbtft_par_dbg(DEBUG_INIT_DISPLAY, par,
"init: msleep(%u)\n", val & 0x);
msleep(val & 0x);
- p = of_prop_next_u32(prop, p, );
+ val = values[++index];
} else {
- dev_err(par->info->device, "illegal init value 0x%X\n",
- val);
- return -EINVAL;
+ dev_err(dev, "illegal init value 0x%X\n", val);
+ ret = -EINVAL;
+ goto out_free;
}
}
- return 0;
+out_free:
+ kfree(values);
+ return ret;
}
/**
@@ -1132,25 +1144,24 @@ static int fbtft_verify_gpios(struct fbtft_par *par)
}
/* returns 0 if the property is not present */
-static u32 fbtft_of_value(struct device_node *node, const char *propname)
+static u32 fbtft_property_value(struct device *dev, const char *propname)
{
int ret;
u32 val = 0;
- ret = of_property_read_u32(node, propname, );
+ ret = device_property_read_u32(dev, propname, );
if (ret == 0)
- pr_info("%s: %s = %u\n", __func__, propname, val);
+ dev_info(dev, "%s: %s = %u\n", __func__, propname, val);
return val;
}
-static struct fbtft_platform_data *fbtft_probe_dt(struct device *dev)
+static struct fbtft_platform_data
[PATCH v1 2/5] fbtft: Describe function parameters in kernel-doc
Kernel documentation script complains that some of the function parameters are not described: drivers/staging/fbtft/fbtft-core.c:543: warning: Function parameter or member 'pdata' not described in 'fbtft_framebuffer_alloc' Describe function parameters where it's appropriate. Signed-off-by: Andy Shevchenko --- drivers/staging/fbtft/fbtft-core.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c index 61f0286fb157..2122c4407bdb 100644 --- a/drivers/staging/fbtft/fbtft-core.c +++ b/drivers/staging/fbtft/fbtft-core.c @@ -529,6 +529,7 @@ static void fbtft_merge_fbtftops(struct fbtft_ops *dst, struct fbtft_ops *src) * * @display: pointer to structure describing the display * @dev: pointer to the device for this fb, this can be NULL + * @pdata: platform data for the display in use * * Creates a new frame buffer info structure. * -- 2.24.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [staging:staging-testing 314/401] drivers/iio/common/hid-sensors/hid-sensor-attributes.c:312: undefined reference to `__udivdi3'
On Wed, Sep 04, 2019 at 11:33:50AM +0800, kbuild test robot wrote: > tree: > https://kernel.googlesource.com/pub/scm/linux/kernel/git/gregkh/staging.git > staging-testing > head: 74eb9c06b1d722468db397595ac6834b9e4ac235 > commit: 473d12f7638c93acbd9296a8cd455b203d5eb528 [314/401] iio: > hid-sensor-attributes: Convert to use int_pow() > config: i386-randconfig-e004-201935 (attached as .config) > compiler: gcc-7 (Debian 7.4.0-11) 7.4.0 > reproduce: > git checkout 473d12f7638c93acbd9296a8cd455b203d5eb528 > # save the attached .config to linux build tree > make ARCH=i386 > > If you fix the issue, kindly add following tag > Reported-by: kbuild test robot > > All errors (new ones prefixed by >>): So, as far as I understood it wasn't compiled on 32-bit before, so, it's not a new error and thus would (has to?) be fixed separately. >ld: drivers/iio/common/hid-sensors/hid-sensor-attributes.o: in function > `adjust_exponent_nano': > >> drivers/iio/common/hid-sensors/hid-sensor-attributes.c:312: undefined > >> reference to `__udivdi3' > >> ld: drivers/iio/common/hid-sensors/hid-sensor-attributes.c:314: undefined > >> reference to `__umoddi3' > >> ld: drivers/iio/common/hid-sensors/hid-sensor-attributes.c:324: undefined > >> reference to `__udivdi3' >ld: drivers/iio/common/hid-sensors/hid-sensor-attributes.c:325: undefined > reference to `__umoddi3' -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 04/22] media: Move v4l2_fwnode_parse_link from v4l2 to driver base
On Tue, Aug 6, 2019 at 2:37 AM Steve Longerbeam wrote:
>
> There is nothing v4l2-specific about v4l2_fwnode_{parse|put}_link().
> Make these functions more generally available by moving them to driver
> base, with the appropriate name changes to the functions and struct.
>
> In the process embed a 'struct fwnode_endpoint' in 'struct fwnode_link'
> for both sides of the link, and make use of fwnode_graph_parse_endpoint()
> to fully parse both endpoints. Rename members local_node and
> remote_node to more descriptive local_port_parent and
> remote_port_parent.
>
May I ask if it's going to be used outside of v4l2?
Any user in mind?
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 48/87] rtl8723bs: os_dep: replace rtw_malloc and memset with rtw_zmalloc
On Thu, Jun 27, 2019 at 8:41 PM Fuqian Huang wrote: > > rtw_malloc + memset(0) -> rtw_zmalloc I have a feeling that everything under os_dep folder should be replaced with native kernel APIs. > drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 8 ++-- > drivers/staging/rtl8723bs/os_dep/ioctl_linux.c| 12 +++- -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 03/16] lib,treewide: add new match_string() helper/macro
tatic int name_card(struct snd_oxfw *oxfw)
> diff --git a/sound/soc/codecs/max98088.c b/sound/soc/codecs/max98088.c
> index 3ef743075bda..911ffe84c37e 100644
> --- a/sound/soc/codecs/max98088.c
> +++ b/sound/soc/codecs/max98088.c
> @@ -1405,7 +1405,7 @@ static int max98088_get_channel(struct
> snd_soc_component *component, const char
> {
> int ret;
>
> - ret = __match_string(eq_mode_name, ARRAY_SIZE(eq_mode_name), name);
> + ret = match_string(eq_mode_name, name);
> if (ret < 0)
> dev_err(component->dev, "Bad EQ channel name '%s'\n", name);
> return ret;
> diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c
> index cd69916d5dcb..d182d45d0c83 100644
> --- a/sound/soc/codecs/max98095.c
> +++ b/sound/soc/codecs/max98095.c
> @@ -1636,7 +1636,7 @@ static int max98095_get_bq_channel(struct
> snd_soc_component *component,
> {
> int ret;
>
> - ret = __match_string(bq_mode_name, ARRAY_SIZE(bq_mode_name), name);
> + ret = match_string(bq_mode_name, name);
> if (ret < 0)
> dev_err(component->dev, "Bad biquad channel name '%s'\n", name);
> return ret;
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/26] compat_ioctl: cleanups
On Tue, Apr 16, 2019 at 11:23 PM Arnd Bergmann wrote: > > Hi Al, > > It took me way longer than I had hoped to revisit this series, see > https://lore.kernel.org/lkml/20180912150142.157913-1-a...@arndb.de/ > for the previously posted version. > > I've come to the point where all conversion handlers and most > COMPATIBLE_IOCTL() entries are gone from this file, but for > now, this series only has the parts that have either been reviewed > previously, or that are simple enough to include. > > The main missing piece is the SG_IO/SG_GET_REQUEST_TABLE conversion. > I'll post the patches I made for that later, as they need more > testing and review from the scsi maintainers. > > I hope you can still take these for the coming merge window, unless > new problems come up. > drivers/platform/x86/wmi.c | 2 +- Acked-by: Andy Shevchenko -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] vmbus: Switch to use new generic UUID API
There are new types and helpers that are supposed to be used in new code.
As a preparation to get rid of legacy types and API functions do
the conversion here.
Cc: "K. Y. Srinivasan"
Cc: Haiyang Zhang
Cc: Stephen Hemminger
Cc: de...@linuxdriverproject.org
Signed-off-by: Andy Shevchenko
---
v2:
- leave uapi untouched (Christoph, Haiyang)
- rebase on top of latest linux-next
drivers/hv/channel.c | 4 +-
drivers/hv/channel_mgmt.c | 18 +++
drivers/hv/hyperv_vmbus.h | 4 +-
drivers/hv/vmbus_drv.c| 48 +++
include/linux/hyperv.h| 98 +++
5 files changed, 79 insertions(+), 93 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index ce0ba2062723..7f3f9994e55e 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -282,8 +282,8 @@ int vmbus_open(struct vmbus_channel *newchannel,
EXPORT_SYMBOL_GPL(vmbus_open);
/* Used for Hyper-V Socket: a guest client's connect() to the host */
-int vmbus_send_tl_connect_request(const uuid_le *shv_guest_servie_id,
- const uuid_le *shv_host_servie_id)
+int vmbus_send_tl_connect_request(const guid_t *shv_guest_servie_id,
+ const guid_t *shv_host_servie_id)
{
struct vmbus_channel_tl_connect_request conn_msg;
int ret;
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index d01689079e9b..62703b354d6d 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -141,7 +141,7 @@ static const struct vmbus_device vmbus_devs[] = {
};
static const struct {
- uuid_le guid;
+ guid_t guid;
} vmbus_unsupported_devs[] = {
{ HV_AVMA1_GUID },
{ HV_AVMA2_GUID },
@@ -171,26 +171,26 @@ static void vmbus_rescind_cleanup(struct vmbus_channel
*channel)
spin_unlock_irqrestore(_connection.channelmsg_lock, flags);
}
-static bool is_unsupported_vmbus_devs(const uuid_le *guid)
+static bool is_unsupported_vmbus_devs(const guid_t *guid)
{
int i;
for (i = 0; i < ARRAY_SIZE(vmbus_unsupported_devs); i++)
- if (!uuid_le_cmp(*guid, vmbus_unsupported_devs[i].guid))
+ if (guid_equal(guid, _unsupported_devs[i].guid))
return true;
return false;
}
static u16 hv_get_dev_type(const struct vmbus_channel *channel)
{
- const uuid_le *guid = >offermsg.offer.if_type;
+ const guid_t *guid = >offermsg.offer.if_type;
u16 i;
if (is_hvsock_channel(channel) || is_unsupported_vmbus_devs(guid))
return HV_UNKNOWN;
for (i = HV_IDE; i < HV_UNKNOWN; i++) {
- if (!uuid_le_cmp(*guid, vmbus_devs[i].guid))
+ if (guid_equal(guid, _devs[i].guid))
return i;
}
pr_info("Unknown GUID: %pUl\n", guid);
@@ -561,10 +561,10 @@ static void vmbus_process_offer(struct vmbus_channel
*newchannel)
atomic_dec(_connection.offer_in_progress);
list_for_each_entry(channel, _connection.chn_list, listentry) {
- if (!uuid_le_cmp(channel->offermsg.offer.if_type,
-newchannel->offermsg.offer.if_type) &&
- !uuid_le_cmp(channel->offermsg.offer.if_instance,
-newchannel->offermsg.offer.if_instance)) {
+ if (guid_equal(>offermsg.offer.if_type,
+ >offermsg.offer.if_type) &&
+ guid_equal(>offermsg.offer.if_instance,
+ >offermsg.offer.if_instance)) {
fnew = false;
break;
}
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index a1f6ce6e5974..cb86b133eb4d 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -312,8 +312,8 @@ extern const struct vmbus_channel_message_table_entry
/* General vmbus interface */
-struct hv_device *vmbus_device_create(const uuid_le *type,
- const uuid_le *instance,
+struct hv_device *vmbus_device_create(const guid_t *type,
+ const guid_t *instance,
struct vmbus_channel *channel);
int vmbus_device_register(struct hv_device *child_device_obj);
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 403fee01572c..126c2de39e35 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -654,38 +654,28 @@ static int vmbus_uevent(struct device *device, struct
kobj_uevent_env *env)
return ret;
}
-static const uuid_le null_guid;
-
-static inline bool is_null_guid(const uuid_le *guid)
-{
- if (uuid_le_cmp(*guid, null_guid))
- return false;
- return true;
-}
-
static const struct hv_vmbus_device_id *
-hv_vmbus_dev_match(const struct hv_vmbu
Re: [PATCH v2 06/17] Platform: OLPC: Add XO-1.75 EC driver
On Mon, Dec 3, 2018 at 1:13 AM Darren Hart wrote: > On Fri, Nov 16, 2018 at 05:23:52PM +0100, Lubomir Rintel wrote: > > It's based off the driver from the OLPC kernel sources. Somewhat > > modernized and cleaned up, for better or worse. > > > > Modified to plug into the olpc-ec driver infrastructure (so that battery > > interface and debugfs could be reused) and the SPI slave framework. > > - Count the terminating NUL in LOG_BUF_SIZE > > - Make olpc_xo175_ec_is_valid_cmd() return -EINVAL instead of -1 > > on error > > - Spell keyboard/touchpad in full for CHAN_KEYBOARD/TOUCHPAD messages > > - Use a #define for PM wakeup processing time > > - Log a message on unknown event > > - Escape logging payload with %*pE > > - Replace an open-coded min() > > - Correct an error code on short read > > - replaced PM callback #ifdefs with __maybe_unusedm SET_RUNTIME_PM_OPS > > and SET_NOIRQ_SYSTEM_SLEEP_PM_OPS > > - dev_get_drvdata() instead of a round-trip through platform device > > - s/unsigned char x/u8 x/ in olpc_xo175_ec_resume() > > - Use GENMASK() instead of 0x for the event mask > > - Replace cmd tx/resp rx buffers with structures > > - Turned suspend hint arguments into a struct, and tidied up the comment > > Just from these comments, each of these could be a separate patch. You > can group related things together, or those that change the same line or > function for example. Order them with cleanups / non-functional-changes > first, followed by functional changes. > > > > > Basically all of the above is based on the review by Andy Shevchenko. > > Andy, what was your intent for Lubomir here? From the above, this looks > like it should be several patches to me. This is a new module, I don't see why it can't be one patch. For the existing code I agree with you. -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 06/15] Platform: OLPC: Add XO-1.75 EC driver
mdelay(1000);
> + }
> +}
> +
> +static void olpc_xo175_ec_power_off(void)
> +{
> + while (1) {
> + olpc_ec_cmd(CMD_POWER_OFF, NULL, 0, NULL, 0);
> + mdelay(1000);
> + }
> +}
> +
> +#ifdef CONFIG_PM
> +static int olpc_xo175_ec_suspend(struct device *dev)
__maybe_unused instead of ugly #ifdef?
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
dev_get_drvdata() or how is it called?
> + unsigned char hintargs[5];
struct olpc_ec_hint_cmd {
u8 ...
u32 ...
};
?
> + static unsigned int suspend_count;
u32 I suppose.
> +
> + suspend_count++;
> + dev_dbg(dev, "%s: suspend sync %08x\n", __func__, suspend_count);
__func__ can be issued if user asked for via Dynamic Debug interface.
> + /*
> +* First byte is 1 to indicate suspend, the rest is an integer
> +* counter.
> +*/
> + hintargs[0] = 1;
> + memcpy([1], _count, 4);
> + olpc_ec_cmd(CMD_SUSPEND_HINT, hintargs, 5, NULL, 0);
What do you need this counter for?
> + priv->suspended = true;
Hmm... Who is the user of it?
> + return 0;
> +}
> +
> +static int olpc_xo175_ec_resume_noirq(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> +
> + priv->suspended = false;
> +
> + return 0;
> +}
> +
> +static int olpc_xo175_ec_resume(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> + unsigned char x = 0;
u8
> + priv->suspended = false;
Isn't it redundant since noirq callback above?
> + /*
> +* The resume hint is only needed if no other commands are
> +* being sent during resume. all it does is tell the EC
> +* the SoC is definitely awake.
> +*/
> + olpc_ec_cmd(CMD_SUSPEND_HINT, , 1, NULL, 0);
> +
> + /* Enable all EC events while we're awake */
> + olpc_xo175_ec_set_event_mask(0x);
#define EC_ALL_EVENTS GENMASK(15, 0)
> +}
> +#endif
> +static struct platform_device *olpc_ec;
I would rather see this at the top of file.
> +static int olpc_xo175_ec_probe(struct spi_device *spi)
> +{
> + if (olpc_ec) {
> + dev_err(>dev, "OLPC EC already registered.\n");
> + return -EBUSY;
> + }
It's racy against parallel probe called. I don't think it would be a
real issue, just let you know.
> + /* Set up power button input device */
> + priv->pwrbtn = devm_input_allocate_device(>dev);
> + if (!priv->pwrbtn)
> + return -ENOMEM;
> + priv->pwrbtn->name = "Power Button";
> + priv->pwrbtn->dev.parent = >dev;
> + input_set_capability(priv->pwrbtn, EV_KEY, KEY_POWER);
> + ret = input_register_device(priv->pwrbtn);
> + if (ret) {
> + dev_err(>dev, "error registering input device: %d\n",
> ret);
> + return ret;
> + }
I would split out power button driver, but it's up to you.
> + /* Enable all EC events while we're awake */
> + olpc_xo175_ec_set_event_mask(0x);
See above about this magic.
> +}
> +#ifdef CONFIG_PM
> + .suspend= olpc_xo175_ec_suspend,
> + .resume_noirq = olpc_xo175_ec_resume_noirq,
> + .resume = olpc_xo175_ec_resume,
> +#endif
SET_SYSTEM_SLEEP_PM_OPS() ?
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() ?
> +static const struct of_device_id olpc_xo175_ec_of_match[] = {
> + { .compatible = "olpc,xo1.75-ec" },
> + { },
No comma for terminators.
> +};
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller
On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel wrote: > > Hi. > > This patchset adds support for the Embedded Controller on an OLPC XO > 1.75 machine. OLPC XO 1.75 is a MMP2 based ARM laptop. It plugs into > the existing OLPC platform infrastructure, currently used by the x86 > based models. > > The EC operates in SPI master mode, meaning the SOC is the SPI slave. It > uses extra handshake signal to signal readiness of SOC to accept data > from EC and to initiate a transaction if SOC wishes to submit data. > > The SPI slave support for MMP2 was submitted separately: > https://lore.kernel.org/lkml/20181010170936.316862-1-lkund...@v3.sk/T/#t > > THe "power: supply: olpc_battery: correct the temperature" patch was The > already sent out separately, but I'm including it because the last > commit of the set depends on it. > > Tested to work on an OLPC XO 1.75 and also tested not to break x86 > support with an OLPC XO 1 machine. I don't have a XO 1.5, but it's > unlikely this breaks it when XO 1 works. > > Thanks in advance for reviews and feedback of any kind. Thanks for the series. I'm about to review the patch 6, otherwise read my comments for the rest and consider addressing them. > > Lubo > -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 15/15] power: supply: olpc_battery: Add OLPC XO 1.75 support
On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel wrote:
>
> The battery and the protocol are essentially the same as OLPC XO 1.5,
> but the responses from the EC are LSB first.
>
> Signed-off-by: Lubomir Rintel
> ---
> drivers/power/supply/olpc_battery.c | 23 ++-
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/power/supply/olpc_battery.c
> b/drivers/power/supply/olpc_battery.c
> index dde9863e5c4a..2adf33b9f641 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -56,6 +56,7 @@ struct olpc_battery_data {
> struct power_supply *olpc_bat;
> char bat_serial[17];
> int new_proto;
> + int little_endian;
> };
>
> /*
> @@ -321,6 +322,14 @@ static int olpc_bat_get_voltage_max_design(union
> power_supply_propval *val)
> return ret;
> }
>
> +static s16 ecword_to_cpu(struct olpc_battery_data *data, u16 ec_byte)
ec_byte is misleading name. It's a 16-bit word. and since we called in
I/O accessor it as word, you may do the similar here.
And why result is s16 and not u16?
> +{
> + if (data->little_endian)
> + return le16_to_cpu(ec_byte);
> + else
> + return be16_to_cpu(ec_byte);
> +
> /*
> * Battery properties
> */
> @@ -393,7 +402,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
> if (ret)
> return ret;
>
> - val->intval = (s16)be16_to_cpu(ec_word) * 9760L / 32;
> + val->intval = ecword_to_cpu(data, ec_word) * 9760L / 32;
> break;
> case POWER_SUPPLY_PROP_CURRENT_AVG:
> case POWER_SUPPLY_PROP_CURRENT_NOW:
> @@ -401,7 +410,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
> if (ret)
> return ret;
>
> - val->intval = (s16)be16_to_cpu(ec_word) * 15625L / 120;
> + val->intval = ecword_to_cpu(data, ec_word) * 15625L / 120;
> break;
> case POWER_SUPPLY_PROP_CAPACITY:
> ret = olpc_ec_cmd(EC_BAT_SOC, NULL, 0, _byte, 1);
> @@ -432,21 +441,21 @@ static int olpc_bat_get_property(struct power_supply
> *psy,
> if (ret)
> return ret;
>
> - val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256;
> + val->intval = ecword_to_cpu(data, ec_word) * 10 / 256;
> break;
> case POWER_SUPPLY_PROP_TEMP_AMBIENT:
> ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)_word, 2);
> if (ret)
> return ret;
>
> - val->intval = (int)be16_to_cpu(ec_word) * 10 / 256;
> + val->intval = (int)ecword_to_cpu(data, ec_word) * 10 / 256;
> break;
> case POWER_SUPPLY_PROP_CHARGE_COUNTER:
> ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)_word, 2);
> if (ret)
> return ret;
>
> - val->intval = (s16)be16_to_cpu(ec_word) * 6250 / 15;
> + val->intval = ecword_to_cpu(data, ec_word) * 6250 / 15;
> break;
> case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> ret = olpc_ec_cmd(EC_BAT_SERIAL, NULL, 0, (void *)_buf,
> 8);
> @@ -626,6 +635,10 @@ static int olpc_battery_probe(struct platform_device
> *pdev)
> if (ecver[0] > 0x44) {
> /* XO 1 or 1.5 with a new EC firmware. */
> data->new_proto = 1;
> + } else if (of_find_compatible_node(NULL, NULL, "olpc,xo1.75-ec")) {
> + /* XO 1.75 */
> + data->new_proto = 1;
> + data->little_endian = 1;
> } else if (ecver[0] < 0x44) {
> /*
> * We've seen a number of EC protocol changes; this driver
> --
> 2.19.0
>
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 14/15] power: supply: olpc_battery: Avoid using platform_info
On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel wrote:
>
> This wouldn't work on the DT-based ARM platform. Let's read the EC version
> directly from the EC driver instead.
>
> This makes the driver no longer x86 specific.
>
> Signed-off-by: Lubomir Rintel
> ---
> drivers/power/supply/Kconfig| 2 +-
> drivers/power/supply/olpc_battery.c | 35 +
> 2 files changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index ff6dab0bf0dd..f0361e4dd457 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -151,7 +151,7 @@ config BATTERY_PMU
>
> config BATTERY_OLPC
> tristate "One Laptop Per Child battery"
> - depends on X86_32 && OLPC
> + depends on OLPC
> help
> Say Y to enable support for the battery on the OLPC laptop.
>
> diff --git a/drivers/power/supply/olpc_battery.c
> b/drivers/power/supply/olpc_battery.c
> index 2a2d7cc995f0..dde9863e5c4a 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -20,8 +20,6 @@
> #include
> #include
> #include
Btw, Kconfig might miss
depends on OF
part.
> -#include
> -
>
> #define EC_BAT_VOLTAGE 0x10/* uint16_t,*9.76/32,mV */
> #define EC_BAT_CURRENT 0x11/* int16_t, *15.625/120, mA */
> @@ -57,6 +55,7 @@ struct olpc_battery_data {
> struct power_supply *olpc_ac;
> struct power_supply *olpc_bat;
> char bat_serial[17];
> + int new_proto;
> };
>
> /*
> @@ -100,7 +99,7 @@ static const struct power_supply_desc olpc_ac_desc = {
> static int olpc_bat_get_status(struct olpc_battery_data *data,
> union power_supply_propval *val, uint8_t ec_byte)
> {
> - if (olpc_platform_info.ecver > 0x44) {
> + if (data->new_proto) {
> if (ec_byte & (BAT_STAT_CHARGING | BAT_STAT_TRICKLE))
> val->intval = POWER_SUPPLY_STATUS_CHARGING;
> else if (ec_byte & BAT_STAT_DISCHARGING)
> @@ -608,14 +607,32 @@ static int olpc_battery_probe(struct platform_device
> *pdev)
> struct power_supply_config psy_cfg = {};
> struct olpc_battery_data *data;
> uint8_t status;
> + unsigned char ecver[1];
isn't it simple
uint8_t ecver;
?
> + int ret;
> +
> + data = devm_kzalloc(>dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> + platform_set_drvdata(pdev, data);
> +
> + /* See if the EC is already there and get the EC revision */
> + ret = olpc_ec_cmd(EC_FIRMWARE_REV, NULL, 0, ecver, ARRAY_SIZE(ecver));
> + if (ret) {
> + if (ret == -ENODEV)
> + return -EPROBE_DEFER;
Yeah, exactly a question I asked somewhere in the first part of the series.
> + return ret;
> + }
>
> - /*
> -* We've seen a number of EC protocol changes; this driver requires
> -* the latest EC protocol, supported by 0x44 and above.
> -*/
> - if (olpc_platform_info.ecver < 0x44) {
> + if (ecver[0] > 0x44) {
> + /* XO 1 or 1.5 with a new EC firmware. */
> + data->new_proto = 1;
> + } else if (ecver[0] < 0x44) {
> + /*
> +* We've seen a number of EC protocol changes; this driver
> +* requires the latest EC protocol, supported by 0x44 and
> above.
> + */
> printk(KERN_NOTICE "OLPC EC version 0x%02x too old for "
> - "battery driver.\n", olpc_platform_info.ecver);
> + "battery driver.\n", ecver[0]);
> return -ENXIO;
> }
>
> --
> 2.19.0
>
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 13/15] power: supply: olpc_battery: Move priv data to a struct
On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel wrote:
>
> The global variables for private data are not too nice. I'd like some
> more, and that would clutter the global name space even further.
>
Good change!
Reviewed-by: Andy Shevchenko
> Signed-off-by: Lubomir Rintel
> ---
> drivers/power/supply/olpc_battery.c | 73 +++--
> 1 file changed, 38 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/power/supply/olpc_battery.c
> b/drivers/power/supply/olpc_battery.c
> index 540d44bf536f..2a2d7cc995f0 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -53,6 +53,12 @@
>
> #define BAT_ADDR_MFR_TYPE 0x5F
>
> +struct olpc_battery_data {
> + struct power_supply *olpc_ac;
> + struct power_supply *olpc_bat;
> + char bat_serial[17];
> +};
> +
> /*
> * Power
> */
> @@ -91,11 +97,8 @@ static const struct power_supply_desc olpc_ac_desc = {
> .get_property = olpc_ac_get_prop,
> };
>
> -static struct power_supply *olpc_ac;
> -
> -static char bat_serial[17]; /* Ick */
> -
> -static int olpc_bat_get_status(union power_supply_propval *val, uint8_t
> ec_byte)
> +static int olpc_bat_get_status(struct olpc_battery_data *data,
> + union power_supply_propval *val, uint8_t ec_byte)
> {
> if (olpc_platform_info.ecver > 0x44) {
> if (ec_byte & (BAT_STAT_CHARGING | BAT_STAT_TRICKLE))
> @@ -326,6 +329,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
> enum power_supply_property psp,
> union power_supply_propval *val)
> {
> + struct olpc_battery_data *data = power_supply_get_drvdata(psy);
> int ret = 0;
> __be16 ec_word;
> uint8_t ec_byte;
> @@ -347,7 +351,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
>
> switch (psp) {
> case POWER_SUPPLY_PROP_STATUS:
> - ret = olpc_bat_get_status(val, ec_byte);
> + ret = olpc_bat_get_status(data, val, ec_byte);
> if (ret)
> return ret;
> break;
> @@ -450,8 +454,8 @@ static int olpc_bat_get_property(struct power_supply *psy,
> if (ret)
> return ret;
>
> - sprintf(bat_serial, "%016llx", (long
> long)be64_to_cpu(ser_buf));
> - val->strval = bat_serial;
> + sprintf(data->bat_serial, "%016llx", (long
> long)be64_to_cpu(ser_buf));
> + val->strval = data->bat_serial;
> break;
> case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> ret = olpc_bat_get_voltage_max_design(val);
> @@ -579,17 +583,17 @@ static struct power_supply_desc olpc_bat_desc = {
> .use_for_apm = 1,
> };
>
> -static struct power_supply *olpc_bat;
> -
> static int olpc_battery_suspend(struct platform_device *pdev,
> pm_message_t state)
> {
> - if (device_may_wakeup(_ac->dev))
> + struct olpc_battery_data *data = platform_get_drvdata(pdev);
> +
> + if (device_may_wakeup(>olpc_ac->dev))
> olpc_ec_wakeup_set(EC_SCI_SRC_ACPWR);
> else
> olpc_ec_wakeup_clear(EC_SCI_SRC_ACPWR);
>
> - if (device_may_wakeup(_bat->dev))
> + if (device_may_wakeup(>olpc_bat->dev))
> olpc_ec_wakeup_set(EC_SCI_SRC_BATTERY | EC_SCI_SRC_BATSOC
>| EC_SCI_SRC_BATERR);
> else
> @@ -601,7 +605,8 @@ static int olpc_battery_suspend(struct platform_device
> *pdev,
>
> static int olpc_battery_probe(struct platform_device *pdev)
> {
> - int ret;
> + struct power_supply_config psy_cfg = {};
> + struct olpc_battery_data *data;
> uint8_t status;
>
> /*
> @@ -620,9 +625,13 @@ static int olpc_battery_probe(struct platform_device
> *pdev)
>
> /* Ignore the status. It doesn't actually matter */
>
> - olpc_ac = power_supply_register(>dev, _ac_desc, NULL);
> - if (IS_ERR(olpc_ac))
> - return PTR_ERR(olpc_ac);
> + psy_cfg.of_node = pdev->dev.of_node;
> + psy_cfg.drv_data = data;
> +
> + data->olpc_ac = devm_power_supply_register(>dev, _ac_desc,
> _cfg);
> + if (IS_ERR(data->olpc_ac))
> + return PTR_ERR(data->
Re: [PATCH 12/15] power: supply: olpc_battery: Use DT to get battery version
On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel wrote:
>
> Avoid using the x86 OLPC platform specific call to get the board
> version. It won't work on FDT-based ARM MMP2 platform.
>
> Signed-off-by: Lubomir Rintel
> ---
> drivers/power/supply/olpc_battery.c | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/power/supply/olpc_battery.c
> b/drivers/power/supply/olpc_battery.c
> index 5a97e42a3547..540d44bf536f 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -19,6 +19,7 @@
> #include
> #include
> #include
> +#include
> #include
Keep it sorted, otherwise the change is good!
Reviewed-by: Andy Shevchenko
>
>
> @@ -622,11 +623,13 @@ static int olpc_battery_probe(struct platform_device
> *pdev)
> olpc_ac = power_supply_register(>dev, _ac_desc, NULL);
> if (IS_ERR(olpc_ac))
> return PTR_ERR(olpc_ac);
> -
> - if (olpc_board_at_least(olpc_board_pre(0xd0))) { /* XO-1.5 */
> + if (of_property_match_string(pdev->dev.of_node, "compatible",
> + "olpc,xo1.5-battery") >= 0) {
> + /* XO-1.5 */
> olpc_bat_desc.properties = olpc_xo15_bat_props;
> olpc_bat_desc.num_properties =
> ARRAY_SIZE(olpc_xo15_bat_props);
> - } else { /* XO-1 */
> + } else {
> + /* XO-1 */
> olpc_bat_desc.properties = olpc_xo1_bat_props;
> olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
> }
> @@ -672,6 +675,7 @@ static int olpc_battery_remove(struct platform_device
> *pdev)
>
> static const struct of_device_id olpc_battery_ids[] = {
> { .compatible = "olpc,xo1-battery" },
> + { .compatible = "olpc,xo1.5-battery" },
> {}
> };
> MODULE_DEVICE_TABLE(of, olpc_battery_ids);
> --
> 2.19.0
>
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 11/15] x86, olpc: Use a correct version when making up a battery node
On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel wrote:
>
> The XO-1 and XO-1.5 batteries apparently differ in an ability to report
> ambient temperature. Add a different compatible string to the 1.5
> battery.
> +int olpc_dt_compatible_match(phandle node, const char *compat)
> {
> char buf[64];
> + int plen;
> + char *p;
> + int len;
> +
> + plen = olpc_dt_getproperty(node, "compatible", buf, sizeof(buf));
> + if (plen <= 0)
> + return 0;
> +
> + len = strlen(compat);
> + for (p = buf; p < buf + plen; p += strlen(p) + 1) {
> + if (strcmp(p, compat) == 0)
> + return 1;
> + }
> +
> + return 0;
> +}
DT doesn't still have a helper for that?!
I hardly believe in that.
> + olpc_dt_interpret("\" /battery@0\" find-device"
> + " \" olpc,xo1.5-battery\" +compatible"
> + " device-end");
Please, don't split string literals.
> olpc_dt_interpret("\" /pci/display@1\" find-device"
> " new-device"
> " \" dcon\" device-name \" olpc,xo1-dcon\"
> +compatible"
> " finish-device device-end");
Yeah, this and similar also needs to be fixed.
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 09/15] Platform: OLPC: add a regulator for the DCON
On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel wrote:
>
> All OLPC ECs are able to turn the power to the DCON on an off. Use the
> regulator framework to expose the functionality.
> +static int olpc_ec_set_dcon_power(struct olpc_ec_priv *ec, bool state)
> +{
> + unsigned char ec_byte = state;
> + int ret;
> +
> + if (ec->dcon_enabled == state)
> + return 0;
> +
> + ret = olpc_ec_cmd(EC_DCON_POWER_MODE, _byte, 1, NULL, 0);
> + if (ret == 0)
> + ec->dcon_enabled = state;
> +
> + return ret;
I would prefer to see usual pattern, i.e.
if (ret)
return ret;
...
return 0;
This comment applies to entire series. (Yes, you might address an old
code in a separate patch to be on consistent side)
> +}
> + return olpc_ec_set_dcon_power(ec, 1);
defined as boolean, supplied an int. Not good.
> + return olpc_ec_set_dcon_power(ec, 0);
Ditto.
> +static int dcon_regulator_is_enabled(struct regulator_dev *rdev)
> + return ec->dcon_enabled;
Ditto.
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 08/15] Platform: OLPC: Move EC-specific functionality out from x86
On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel wrote:
>
> It is actually plaform independent. Move it to the olpc-ec driver from
> the X86 OLPC platform, so that it could be used by the ARM based laptops
> too.
What is platform independent exactly?
> #define OLPC_F_PRESENT 0x01
> #define OLPC_F_DCON0x02
> -#define OLPC_F_EC_WIDE_SCI 0x04
I think these lines grouped on purpose. Thus, I don't like this.
Either move either all or none.
> +int olpc_ec_mask_write(u16 bits)
> +{
> #ifdef CONFIG_DEBUG_FS
>
> /*
> @@ -277,14 +369,17 @@ static int olpc_ec_probe(struct platform_device *pdev)
> ec_priv = ec;
> platform_set_drvdata(pdev, ec);
>
> + /* EC version 0x5f adds support for wide SCI mask */
> + if (ec->version >= 0x5f) {
> + __be16 ec_word = cpu_to_be16(bits);
> +
> + return olpc_ec_cmd(EC_WRITE_EXT_SCI_MASK, (void *) _word,
> 2,
> + NULL, 0);
I would leave it on one line.
> + } else {
> + unsigned char ec_byte = bits & 0xff;
You may noticed that the parameter is of type u8, which clearly makes
& 0xff part redundant.
> + return olpc_ec_cmd(EC_WRITE_SCI_MASK, _byte, 1, NULL, 0);
> + }
> +}
> +EXPORT_SYMBOL_GPL(olpc_ec_mask_write);
I see that the above is inherited from older code, so, no need to
address those comments in here, but consider a follow up fix.
> +int olpc_ec_sci_query(u16 *sci_value)
> +{
> +}
> +EXPORT_SYMBOL_GPL(olpc_ec_sci_query);
Similar comments are applied here.
> +
> - err = ec_driver->probe ? ec_driver->probe(pdev) : 0;
> + /* get the EC revision */
> + err = olpc_ec_cmd(EC_FIRMWARE_REV, NULL, 0,
> + (unsigned char *) >version, 1);
Perhaps version should be defined as u8.
> +/* SCI source values */
> +#define EC_SCI_SRC_EMPTY0x00
> +#define EC_SCI_SRC_GAME 0x01
> +#define EC_SCI_SRC_BATTERY 0x02
> +#define EC_SCI_SRC_BATSOC 0x04
> +#define EC_SCI_SRC_BATERR 0x08
> +#define EC_SCI_SRC_EBOOK0x10/* XO-1 only */
> +#define EC_SCI_SRC_WLAN 0x20/* XO-1 only */
> +#define EC_SCI_SRC_ACPWR0x40
> +#define EC_SCI_SRC_BATCRIT 0x80
> +#define EC_SCI_SRC_GPWAKE 0x100 /* XO-1.5 only */
BIT() ?
> +#define EC_SCI_SRC_ALL 0x1FF
GENMASK()?
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 07/15] Platform: OLPC: Avoid a warning if the EC didn't register yet
On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel wrote: > > Just return ENODEV, so that whoever attempted to use the EC call can > defer their work. > > Signed-off-by: Lubomir Rintel > --- > drivers/platform/olpc/olpc-ec.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c > index 35a21c66cd0d..342f5bb7f7a8 100644 > --- a/drivers/platform/olpc/olpc-ec.c > +++ b/drivers/platform/olpc/olpc-ec.c > @@ -116,8 +116,11 @@ int olpc_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8 > *outbuf, size_t outlen) > struct olpc_ec_priv *ec = ec_priv; > struct ec_cmd_desc desc; > > - /* Ensure a driver and ec hook have been registered */ > - if (WARN_ON(!ec_driver || !ec_driver->ec_cmd)) > + /* Driver not yet registered. */ > + if (!ec_driver) > + return -ENODEV; Why -ENODEV is preferred over -EPROBE_DEFER? > + > + if (WARN_ON(!ec_driver->ec_cmd)) > return -ENODEV; > > if (!ec) > -- > 2.19.0 > -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 05/15] Platform: OLPC: Move OLPC config symbol out of x86 tree
On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel wrote: > > There are ARM OLPC machines that use mostly the same drivers, including > EC infrastructure, DCON and Battery. > > While at that, fix Kconfig to allow building this as a module. > - depends on MOUSE_PS2 && OLPC > + depends on MOUSE_PS2 && X86 && OLPC > - depends on OLPC && FB > + depends on X86 && OLPC && FB Looking to this I would rather introduce an idiom depends on OLPC && X86 on the opposite you may do similar for ARM depends on OLPC && ARM // or ARM64 or whatever it's called thus, above would look like depends on MOUSE_PS2 depends on OLPC && X86 and depends on FB depends on OLPC && X86 respectively. -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 04/15] Platform: OLPC: Remove an unused include
On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel wrote:
>
> Also, the header is x86 specific, while there are non-x86 OLPC machines.
Same concern. as per patch 2.
Also, you might want to sort headers in alphabetical order.
>
> Signed-off-by: Lubomir Rintel
> ---
> drivers/platform/olpc/olpc-ec.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c
> index f99b183d5296..35a21c66cd0d 100644
> --- a/drivers/platform/olpc/olpc-ec.c
> +++ b/drivers/platform/olpc/olpc-ec.c
> @@ -15,7 +15,6 @@
> #include
> #include
> #include
> -#include
>
> struct ec_cmd_desc {
> u8 cmd;
> --
> 2.19.0
>
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 02/15] Revert "platform/olpc: Make ec explicitly non-modular"
On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel wrote:
>
> It doesn't make sense to always have this built-in, e.g. on ARM
> multiplatform kernels. A better way to address the problem the original
> commit aimed to solve is to fix Kconfig.
>
> This reverts commit f48d1496b8537d75776478c6942dd87f34d7f270.
>
This change doesn't make any sense when put in _this_ order in the series.
First, you need to show the CONFIG_OLPC as tristate, which doesn't (Am
I missing something?).
> Signed-off-by: Lubomir Rintel
> ---
> drivers/platform/olpc/olpc-ec.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c
> index 374a8028fec7..f99b183d5296 100644
> --- a/drivers/platform/olpc/olpc-ec.c
> +++ b/drivers/platform/olpc/olpc-ec.c
> @@ -1,8 +1,6 @@
> /*
> * Generic driver for the OLPC Embedded Controller.
> *
> - * Author: Andres Salomon
> - *
> * Copyright (C) 2011-2012 One Laptop per Child Foundation.
> *
> * Licensed under the GPL v2 or later.
> @@ -14,7 +12,7 @@
> #include
> #include
> #include
> -#include
> +#include
> #include
> #include
> #include
> @@ -328,4 +326,8 @@ static int __init olpc_ec_init_module(void)
> {
> return platform_driver_register(_ec_plat_driver);
> }
> +
> arch_initcall(olpc_ec_init_module);
> +
> +MODULE_AUTHOR("Andres Salomon ");
> +MODULE_LICENSE("GPL");
> --
> 2.19.0
>
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/15] power: supply: olpc_battery: correct the temperature units
On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel wrote: > > According to [1] and [2], the temperature values are in tenths of degree > Celsius. Exposing the Celsius value makes the battery appear on fire: > > $ upower -i /org/freedesktop/UPower/devices/battery_olpc_battery > ... > temperature: 236.9 degrees C > > Tested on OLPC XO-1 and OLPC XO-1.75 laptops. It's interesting that the very author of that code is not included in so-o long Cc list :) Cc: David. David, do you remember if and how you had tested temperature report of the battery on OLPC? I guess this kind of error would be appear immediately. OTOH it might be that power framework had changed requirements (which would be noticeable change). If the latter is true, this patch misses Fixes tag. Actually in any case it misses it. > > [1] include/linux/power_supply.h > [2] Documentation/power/power_supply_class.txt > > Cc: sta...@vger.kernel.org > Signed-off-by: Lubomir Rintel > --- > drivers/power/supply/olpc_battery.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/power/supply/olpc_battery.c > b/drivers/power/supply/olpc_battery.c > index 6da79ae14860..5a97e42a3547 100644 > --- a/drivers/power/supply/olpc_battery.c > +++ b/drivers/power/supply/olpc_battery.c > @@ -428,14 +428,14 @@ static int olpc_bat_get_property(struct power_supply > *psy, > if (ret) > return ret; > > - val->intval = (s16)be16_to_cpu(ec_word) * 100 / 256; > + val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256; > break; > case POWER_SUPPLY_PROP_TEMP_AMBIENT: > ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)_word, 2); > if (ret) > return ret; > > - val->intval = (int)be16_to_cpu(ec_word) * 100 / 256; > + val->intval = (int)be16_to_cpu(ec_word) * 10 / 256; > break; > case POWER_SUPPLY_PROP_CHARGE_COUNTER: > ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)_word, 2); > -- > 2.19.0 > -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5] staging: rts5208: add error handling into rtsx_probe
On Wed, Aug 1, 2018 at 5:08 PM, Anton Vasilyev wrote: > I found that staging-next already contains my patch v3, committed by Greg > Kroah-Hartman. > > Do I need to send a new patch Yes. Based on staging-next. > with a label renaming based on Dan Carpenter > comments? Dan is talking for himself :-) -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5] staging: rts5208: add error handling into rtsx_probe
On Wed, Aug 1, 2018 at 2:55 PM, Anton Vasilyev wrote: > If rtsx_probe() fails to allocate dev->chip, then release_everything() > will crash on uninitialized dev->cmnd_ready complete. > > Patch adds an error handling into rtsx_probe. > Found by Linux Driver Verification project (linuxtesting.org). Have you based your change on staging-next? Seems not. You need to rebase and resend. -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: fbtft: Replace mdelay() with msleep() and usleep_range()
On Fri, Jul 27, 2018 at 12:21 PM, Jia-Ju Bai wrote: > reset() and init_display() are never called in atomic context. > They call mdelay() to busily wait, which is not necessary. > mdelay() can be replaced with msleep(). > gpio_set_value(par->gpio.reset, 0); > udelay(20); > gpio_set_value(par->gpio.reset, 1); > - mdelay(120); > + msleep(120); I didn't look to the rest, but this one will be inconsistent after your patch. The question here is why udelay() is needed, while mdelay() changed? -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v7 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public
On Wed, Jul 18, 2018 at 7:36 PM, Andy Shevchenko wrote: > On Wed, Jul 18, 2018 at 5:49 AM, Baoquan He wrote: >> reparent_resources() is duplicated in arch/microblaze/pci/pci-common.c >> and arch/powerpc/kernel/pci-common.c, so move it to kernel/resource.c >> so that it's shared. >> + * Returns 0 on success, -ENOTSUPP if child resource is not completely >> + * contained by 'res', -ECANCELED if no any conflicting entry found. You also can refer to constants by prefixing them with %, e.g. %-ENOTSUPP. But this is up to you completely. -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v7 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public
On Wed, Jul 18, 2018 at 5:49 AM, Baoquan He wrote:
> reparent_resources() is duplicated in arch/microblaze/pci/pci-common.c
> and arch/powerpc/kernel/pci-common.c, so move it to kernel/resource.c
> so that it's shared.
Some minor stuff.
> +/**
> + * reparent_resources - reparent resource children of parent that res covers
> + * @parent: parent resource descriptor
> + * @res: resource descriptor desired by caller
> + *
> + * Returns 0 on success, -ENOTSUPP if child resource is not completely
> + * contained by 'res', -ECANCELED if no any conflicting entry found.
'res' -> @res
> + *
> + * Reparent resource children of 'parent' that conflict with 'res'
Ditto + 'parent' -> @parent
> + * under 'res', and make 'res' replace those children.
Ditto.
> + */
> +int reparent_resources(struct resource *parent, struct resource *res)
> +{
> + struct resource *p, **pp;
> + struct resource **firstpp = NULL;
> +
> + for (pp = >child; (p = *pp) != NULL; pp = >sibling) {
> + if (p->end < res->start)
> + continue;
> + if (res->end < p->start)
> + break;
> + if (p->start < res->start || p->end > res->end)
> + return -ENOTSUPP; /* not completely contained */
> + if (firstpp == NULL)
> + firstpp = pp;
> + }
> + if (firstpp == NULL)
> + return -ECANCELED; /* didn't find any conflicting entries? */
> + res->parent = parent;
> + res->child = *firstpp;
> + res->sibling = *pp;
> + *firstpp = res;
> + *pp = NULL;
> + for (p = res->child; p != NULL; p = p->sibling) {
> + p->parent = res;
> + pr_debug("PCI: Reparented %s %pR under %s\n",
> +p->name, p, res->name);
Now, PCI is a bit confusing here.
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL(reparent_resources);
> +
> static void __init __reserve_region_with_split(struct resource *root,
> resource_size_t start, resource_size_t end,
> const char *name)
> --
> 2.13.6
>
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 2/4] resource: Use list_head to link sibling resource
On Sun, Jul 8, 2018 at 5:59 AM, Baoquan He wrote:
> On 07/05/18 at 01:00am, kbuild test robot wrote:
> However, I didn't find below branch. And tried to open it in web
> broswer, also failed.
While this is kinda valid point...
> Could you help have a look at this?
...isn't obvious that you didn't change the file mentioned in a report?
Just take latest linux-next and you will see.
>> All error/warnings (new ones prefixed by >>):
>>
>> >> arch/mips/pci/pci-rc32434.c:57:11: error: initialization from
>> >> incompatible pointer type [-Werror=incompatible-pointer-types]
>> .child = _res_pci_mem2
>> ^
>>arch/mips/pci/pci-rc32434.c:57:11: note: (near initialization for
>> 'rc32434_res_pci_mem1.child.next')
>> >> arch/mips/pci/pci-rc32434.c:51:47: warning: missing braces around
>> >> initializer [-Wmissing-braces]
>> static struct resource rc32434_res_pci_mem1 = {
>> ^
>>arch/mips/pci/pci-rc32434.c:60:47: warning: missing braces around
>> initializer [-Wmissing-braces]
>> static struct resource rc32434_res_pci_mem2 = {
>> ^
>>cc1: some warnings being treated as errors
>>
>> vim +57 arch/mips/pci/pci-rc32434.c
>>
>> 73b4390f Ralf Baechle 2008-07-16 50
>> 73b4390f Ralf Baechle 2008-07-16 @51 static struct resource
>> rc32434_res_pci_mem1 = {
>> 73b4390f Ralf Baechle 2008-07-16 52 .name = "PCI MEM1",
>> 73b4390f Ralf Baechle 2008-07-16 53 .start = 0x5000,
>> 73b4390f Ralf Baechle 2008-07-16 54 .end = 0x5FFF,
>> 73b4390f Ralf Baechle 2008-07-16 55 .flags = IORESOURCE_MEM,
>> 73b4390f Ralf Baechle 2008-07-16 56 .sibling = NULL,
>> 73b4390f Ralf Baechle 2008-07-16 @57 .child = _res_pci_mem2
>> 73b4390f Ralf Baechle 2008-07-16 58 };
>> 73b4390f Ralf Baechle 2008-07-16 59
>>
>> :: The code at line 57 was first introduced by commit
>> :: 73b4390fb23456964201abda79f1210fe337d01a [MIPS] Routerboard 532:
>> Support for base system
>>
>> :: TO: Ralf Baechle
>> :: CC: Ralf Baechle
>>
>> ---
>> 0-DAY kernel test infrastructureOpen Source Technology Center
>> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
>
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public
On Wed, Jul 4, 2018 at 7:10 AM, Baoquan He wrote:
> reparent_resources() is duplicated in arch/microblaze/pci/pci-common.c
> and arch/powerpc/kernel/pci-common.c, so move it to kernel/resource.c
> so that it's shared.
With couple of comments below,
Reviewed-by: Andy Shevchenko
P.S. In some commit message in this series you used 'likt' instead of 'like'.
>
> Signed-off-by: Baoquan He
> ---
> arch/microblaze/pci/pci-common.c | 37 -
> arch/powerpc/kernel/pci-common.c | 35 ---
> include/linux/ioport.h | 1 +
> kernel/resource.c| 39 +++
> 4 files changed, 40 insertions(+), 72 deletions(-)
>
> diff --git a/arch/microblaze/pci/pci-common.c
> b/arch/microblaze/pci/pci-common.c
> index f34346d56095..7899bafab064 100644
> --- a/arch/microblaze/pci/pci-common.c
> +++ b/arch/microblaze/pci/pci-common.c
> @@ -619,43 +619,6 @@ int pcibios_add_device(struct pci_dev *dev)
> EXPORT_SYMBOL(pcibios_add_device);
>
> /*
> - * Reparent resource children of pr that conflict with res
> - * under res, and make res replace those children.
> - */
> -static int __init reparent_resources(struct resource *parent,
> -struct resource *res)
> -{
> - struct resource *p, **pp;
> - struct resource **firstpp = NULL;
> -
> - for (pp = >child; (p = *pp) != NULL; pp = >sibling) {
> - if (p->end < res->start)
> - continue;
> - if (res->end < p->start)
> - break;
> - if (p->start < res->start || p->end > res->end)
> - return -1; /* not completely contained */
> - if (firstpp == NULL)
> - firstpp = pp;
> - }
> - if (firstpp == NULL)
> - return -1; /* didn't find any conflicting entries? */
> - res->parent = parent;
> - res->child = *firstpp;
> - res->sibling = *pp;
> - *firstpp = res;
> - *pp = NULL;
> - for (p = res->child; p != NULL; p = p->sibling) {
> - p->parent = res;
> - pr_debug("PCI: Reparented %s [%llx..%llx] under %s\n",
> -p->name,
> -(unsigned long long)p->start,
> -(unsigned long long)p->end, res->name);
> - }
> - return 0;
> -}
> -
> -/*
> * Handle resources of PCI devices. If the world were perfect, we could
> * just allocate all the resource regions and do nothing more. It isn't.
> * On the other hand, we cannot just re-allocate all devices, as it would
> diff --git a/arch/powerpc/kernel/pci-common.c
> b/arch/powerpc/kernel/pci-common.c
> index fe9733aa..926035bb378d 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -1088,41 +1088,6 @@ resource_size_t pcibios_align_resource(void *data,
> const struct resource *res,
> EXPORT_SYMBOL(pcibios_align_resource);
>
> /*
> - * Reparent resource children of pr that conflict with res
> - * under res, and make res replace those children.
> - */
> -static int reparent_resources(struct resource *parent,
> -struct resource *res)
> -{
> - struct resource *p, **pp;
> - struct resource **firstpp = NULL;
> -
> - for (pp = >child; (p = *pp) != NULL; pp = >sibling) {
> - if (p->end < res->start)
> - continue;
> - if (res->end < p->start)
> - break;
> - if (p->start < res->start || p->end > res->end)
> - return -1; /* not completely contained */
> - if (firstpp == NULL)
> - firstpp = pp;
> - }
> - if (firstpp == NULL)
> - return -1; /* didn't find any conflicting entries? */
> - res->parent = parent;
> - res->child = *firstpp;
> - res->sibling = *pp;
> - *firstpp = res;
> - *pp = NULL;
> - for (p = res->child; p != NULL; p = p->sibling) {
> - p->parent = res;
> - pr_debug("PCI: Reparented %s %pR under %s\n",
> -p->name, p, res->name);
> - }
> - return 0;
> -}
> -
> -/*
> * Handle resources of PCI devices. If the world were perfect, we could
> * just allocate all the resource regions and do nothing more. I
Re: [PATCH v5 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public
On Tue, Jul 3, 2018 at 5:55 PM, Baoquan He wrote: > On 06/12/18 at 05:24pm, Andy Shevchenko wrote: >> On Tue, Jun 12, 2018 at 5:20 PM, Andy Shevchenko >> wrote: >> > I briefly looked at the code and error codes we have, so, my proposal >> > is one of the following >> >> > - use -ECANCELED (not the best choice for first occurrence here, >> > though I can't find better) >> >> Actually -ENOTSUPP might suit the first case (although the actual >> would be something like -EOVERLAP, which we don't have) > > Sorry for late reply, and many thanks for your great suggestion. > > I am fine to use -ENOTSUPP as the first returned value, and -ECANCELED > for the 2nd one. I have no strong opinion, but I like (slightly better) this approach ^^^ > Or define an enum as you suggested inside the function > or in header file. > > Or use -EBUSY for the first case because existing resource is > overlapping but not fully contained by 'res'; and -EINVAL for > the 2nd case since didn't find any one resources which is contained by > 'res', means we passed in a invalid resource. > > All is fine to me, I can repost with each of them. >> > - use positive integers (or enum), like >> > #define RES_REPARENTED 0 >> > #define RES_OVERLAPPED 1 >> > #define RES_NOCONFLICT 2 -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8188eu: remove rtw_ioctl_rtl.h
On Sat, Jun 30, 2018 at 5:59 PM, Michael Straube wrote: > The header rtw_ioctl_rtl.h is not used anywhere. > Running 'grep -r rtw_ioctl_rtl *' from kernel root > directory returns nothing, remove the file. Just a side note, using `git grep` is much more efficient against kernel source tree. Something like `git grep -n rtw_ioctl_rtl.h` in this case. -- With Best Regards, Andy Shevchenko ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
