| Message ID | 20231202125948.10345-6-karelb@gimli.ms.mff.cuni.cz (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | input/touchscreen: imagis: add support for IST3032C | expand |
Hi Karel, On 12/2/23 14:48, Karel Balej wrote: > From: Karel Balej <balejk@matfyz.cz> > > IST3032C is a touchscreen chip used for instance in the > samsung,coreprimevelte smartphone, with which this was tested. Add the > chip specific information to the driver. > > Signed-off-by: Karel Balej <balejk@matfyz.cz> > --- > drivers/input/touchscreen/imagis.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c > index 84a02672ac47..41f28e6e9cb1 100644 > --- a/drivers/input/touchscreen/imagis.c > +++ b/drivers/input/touchscreen/imagis.c > @@ -35,6 +35,8 @@ > #define IST3038B_REG_CHIPID 0x30 > #define IST3038B_WHOAMI 0x30380b > > +#define IST3032C_WHOAMI 0x32c > + Perhaps it should be ordered in alphabetic/alphanumeric order, alternatively, the chip ID values could be grouped. > struct imagis_properties { > unsigned int interrupt_msg_cmd; > unsigned int touch_coord_cmd; > @@ -363,6 +365,13 @@ static int imagis_resume(struct device *dev) > > static DEFINE_SIMPLE_DEV_PM_OPS(imagis_pm_ops, imagis_suspend, imagis_resume); > > +static const struct imagis_properties imagis_3032c_data = { > + .interrupt_msg_cmd = IST3038C_REG_INTR_MESSAGE, > + .touch_coord_cmd = IST3038C_REG_TOUCH_COORD, > + .whoami_cmd = IST3038C_REG_CHIPID, > + .whoami_val = IST3032C_WHOAMI, > +}; > + > static const struct imagis_properties imagis_3038b_data = { > .interrupt_msg_cmd = IST3038B_REG_STATUS, > .touch_coord_cmd = IST3038B_REG_STATUS, > @@ -380,6 +389,7 @@ static const struct imagis_properties imagis_3038c_data = { > > #ifdef CONFIG_OF > static const struct of_device_id imagis_of_match[] = { > + { .compatible = "imagis,ist3032c", .data = &imagis_3032c_data }, > { .compatible = "imagis,ist3038b", .data = &imagis_3038b_data }, > { .compatible = "imagis,ist3038c", .data = &imagis_3038c_data }, > { }, Other than that, Reviewed-by: Markuss Broks <markuss.broks@gmail.com> - Markuss
Hi Karel,
kernel test robot noticed the following build warnings:
[auto build test WARNING on dtor-input/next]
[also build test WARNING on dtor-input/for-linus robh/for-next linus/master v6.7-rc4 next-20231205]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Karel-Balej/dt-bindings-input-touchscreen-Add-compatible-for-IST3038B/20231202-215030
base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
patch link: https://lore.kernel.org/r/20231202125948.10345-6-karelb%40gimli.ms.mff.cuni.cz
patch subject: [PATCH v3 5/5] input/touchscreen: imagis: add support for IST3032C
config: hexagon-randconfig-r111-20231204 (https://download.01.org/0day-ci/archive/20231205/202312052257.8Qd4OcID-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce: (https://download.01.org/0day-ci/archive/20231205/202312052257.8Qd4OcID-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312052257.8Qd4OcID-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/input/touchscreen/imagis.c:5:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/input/touchscreen/imagis.c:5:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/input/touchscreen/imagis.c:5:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
>> drivers/input/touchscreen/imagis.c:368:39: warning: unused variable 'imagis_3032c_data' [-Wunused-const-variable]
368 | static const struct imagis_properties imagis_3032c_data = {
| ^
drivers/input/touchscreen/imagis.c:375:39: warning: unused variable 'imagis_3038b_data' [-Wunused-const-variable]
375 | static const struct imagis_properties imagis_3038b_data = {
| ^
drivers/input/touchscreen/imagis.c:383:39: warning: unused variable 'imagis_3038c_data' [-Wunused-const-variable]
383 | static const struct imagis_properties imagis_3038c_data = {
| ^
9 warnings generated.
vim +/imagis_3032c_data +368 drivers/input/touchscreen/imagis.c
367
> 368 static const struct imagis_properties imagis_3032c_data = {
369 .interrupt_msg_cmd = IST3038C_REG_INTR_MESSAGE,
370 .touch_coord_cmd = IST3038C_REG_TOUCH_COORD,
371 .whoami_cmd = IST3038C_REG_CHIPID,
372 .whoami_val = IST3032C_WHOAMI,
373 };
374
Markuss, thank you for the review. > > diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c > > index 84a02672ac47..41f28e6e9cb1 100644 > > --- a/drivers/input/touchscreen/imagis.c > > +++ b/drivers/input/touchscreen/imagis.c > > @@ -35,6 +35,8 @@ > > #define IST3038B_REG_CHIPID 0x30 > > #define IST3038B_WHOAMI 0x30380b > > > > +#define IST3032C_WHOAMI 0x32c > > + > Perhaps it should be ordered in alphabetic/alphanumeric order, > alternatively, the chip ID values could be grouped. Here I followed suit and just started a new section for the new chip, except there is only one entry. I do agree that it would be better to sort the chips alphanumerically and I am actually surprised that I didn't do that - but now I see that the chips that you added are not sorted either, so it might be because of that. I propose to definitely swap the order of the sections, putting 32C first, then 38B and 38C at the end (from top to bottom). The chip ID values could then still be grouped in a new section, but I think I would actually prefer to keep them as parts of the respective sections as it is now, although it is in no way a strong preference. Please let me know whether you agree with this or have a different preference. And if the former, please confirm that I can add your Reviewed-by trailer to the patch modified in such a way. Best regards, K. B.
Hi Karel, On 12/8/23 23:59, Karel Balej wrote: > Markuss, > > thank you for the review. > >>> diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c >>> index 84a02672ac47..41f28e6e9cb1 100644 >>> --- a/drivers/input/touchscreen/imagis.c >>> +++ b/drivers/input/touchscreen/imagis.c >>> @@ -35,6 +35,8 @@ >>> #define IST3038B_REG_CHIPID 0x30 >>> #define IST3038B_WHOAMI 0x30380b >>> >>> +#define IST3032C_WHOAMI 0x32c >>> + >> Perhaps it should be ordered in alphabetic/alphanumeric order, >> alternatively, the chip ID values could be grouped. > Here I followed suit and just started a new section for the new chip, > except there is only one entry. I do agree that it would be better to > sort the chips alphanumerically and I am actually surprised that I > didn't do that - but now I see that the chips that you added are not > sorted either, so it might be because of that. > > I propose to definitely swap the order of the sections, putting 32C > first, then 38B and 38C at the end (from top to bottom). The chip ID > values could then still be grouped in a new section, but I think I would > actually prefer to keep them as parts of the respective sections as it > is now, although it is in no way a strong preference. We could do that, yeah. It is not a problem right now since there's only 3 models supported, but it would maker sense and set some order for when we'd have more supported devices. > > Please let me know whether you agree with this or have a different > preference. And if the former, please confirm that I can add your > Reviewed-by trailer to the patch modified in such a way. Yeah, it's fine. > > Best regards, > K. B. - Markuss
diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c index 84a02672ac47..41f28e6e9cb1 100644 --- a/drivers/input/touchscreen/imagis.c +++ b/drivers/input/touchscreen/imagis.c @@ -35,6 +35,8 @@ #define IST3038B_REG_CHIPID 0x30 #define IST3038B_WHOAMI 0x30380b +#define IST3032C_WHOAMI 0x32c + struct imagis_properties { unsigned int interrupt_msg_cmd; unsigned int touch_coord_cmd; @@ -363,6 +365,13 @@ static int imagis_resume(struct device *dev) static DEFINE_SIMPLE_DEV_PM_OPS(imagis_pm_ops, imagis_suspend, imagis_resume); +static const struct imagis_properties imagis_3032c_data = { + .interrupt_msg_cmd = IST3038C_REG_INTR_MESSAGE, + .touch_coord_cmd = IST3038C_REG_TOUCH_COORD, + .whoami_cmd = IST3038C_REG_CHIPID, + .whoami_val = IST3032C_WHOAMI, +}; + static const struct imagis_properties imagis_3038b_data = { .interrupt_msg_cmd = IST3038B_REG_STATUS, .touch_coord_cmd = IST3038B_REG_STATUS, @@ -380,6 +389,7 @@ static const struct imagis_properties imagis_3038c_data = { #ifdef CONFIG_OF static const struct of_device_id imagis_of_match[] = { + { .compatible = "imagis,ist3032c", .data = &imagis_3032c_data }, { .compatible = "imagis,ist3038b", .data = &imagis_3038b_data }, { .compatible = "imagis,ist3038c", .data = &imagis_3038c_data }, { },