* Re: [PATCH v3 26/42] ata: pata_ep93xx: add device tree support
From: Sergey Shtylyov @ 2023-07-20 9:45 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Guenter Roeck, Sebastian Reichel,
Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Dmitry Torokhov,
Arnd Bergmann, Olof Johansson, soc, Liam Girdwood,
Jaroslav Kysela, Takashi Iwai, Andy Shevchenko, Michael Peters,
Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-26-3d63a5f1103e@maquefel.me>
On 7/20/23 2:29 PM, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> - Add OF ID match table
> - Drop ep93xx_chip_revision and use soc_device_match instead
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
> drivers/ata/pata_ep93xx.c | 26 ++++++++++++++++++++------
> 1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
> index c6e043e05d43..a88824dfc5fa 100644
> --- a/drivers/ata/pata_ep93xx.c
> +++ b/drivers/ata/pata_ep93xx.c
[...]
> @@ -910,6 +912,12 @@ static struct ata_port_operations ep93xx_pata_port_ops = {
> .port_start = ep93xx_pata_port_start,
> };
>
> +static const struct soc_device_attribute ep93xx_soc_table[] = {
> + { .revision = "E1", .data = (void *)ATA_UDMA3 },
> + { .revision = "E2", .data = (void *)ATA_UDMA4 },
> + { /* sentinel */ }
> +};
> +
> static int ep93xx_pata_probe(struct platform_device *pdev)
> {
> struct ep93xx_pata_data *drv_data;
> @@ -939,7 +947,7 @@ static int ep93xx_pata_probe(struct platform_device *pdev)
>
> drv_data = devm_kzalloc(&pdev->dev, sizeof(*drv_data), GFP_KERNEL);
> if (!drv_data) {
> - err = -ENXIO;
> + err = -ENOMEM;
> goto err_rel_gpio;
> }
>
Hm, deserves its own patch. And even for this one, you should've documented it
in the patch secription...
> @@ -976,12 +984,11 @@ static int ep93xx_pata_probe(struct platform_device *pdev)
> * so this driver supports only UDMA modes.
> */
> if (drv_data->dma_rx_channel && drv_data->dma_tx_channel) {
> - int chip_rev = ep93xx_chip_revision();
> + const struct soc_device_attribute *match;
>
> - if (chip_rev == EP93XX_CHIP_REV_E1)
> - ap->udma_mask = ATA_UDMA3;
> - else if (chip_rev == EP93XX_CHIP_REV_E2)
> - ap->udma_mask = ATA_UDMA4;
> + match = soc_device_match(ep93xx_soc_table);
> + if (match)
> + ap->udma_mask = (unsigned int) match->data;
> else
> ap->udma_mask = ATA_UDMA2;
> }
This one also looks as it could have been done separately -- before the DT
conversion?
[...]
MBR, Sergey
^ permalink raw reply
* Re: [PATCH v3 38/42] ata: pata_ep93xx: remove legacy pinctrl use
From: Sergey Shtylyov @ 2023-07-20 9:40 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Guenter Roeck, Sebastian Reichel,
Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Dmitry Torokhov,
Arnd Bergmann, Olof Johansson, soc, Liam Girdwood,
Jaroslav Kysela, Takashi Iwai, Andy Shevchenko, Michael Peters,
Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-38-3d63a5f1103e@maquefel.me>
Hello!
On 7/20/23 2:29 PM, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Drop legacy acquire/release since we are using pinctrl for this now.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
I think I've already given you my:
Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
[...]
MBR, Sergey
^ permalink raw reply
* Re: [PATCH v3 00/42] ep93xx device tree conversion
From: Krzysztof Kozlowski @ 2023-07-20 8:54 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Guenter Roeck, Sebastian Reichel,
Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Andy Shevchenko,
Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel,
Andy Shevchenko, Andrew Lunn
In-Reply-To: <20230605-ep93xx-v3-0-3d63a5f1103e@maquefel.me>
On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> This series aims to convert ep93xx from platform to full device tree support.
>
> The main goal is to receive ACK's to take it via Arnd's arm-soc branch.
>
> I've moved to b4, tricking it to consider v0 as v1, so it consider's
> this version to be v3, this exactly the third version.
Fix your clock/timezone, so your patches are not sent in the future.
Unfortunately this will stay at top of my queue, which is unfair, so I
will just ignore for now.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 13/42] watchdog: ep93xx: add DT support for Cirrus EP93xx
From: Alexander Sverdlin @ 2023-07-20 8:54 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek, Russell King,
Lukasz Majewski, Linus Walleij, Bartosz Golaszewski, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Michael Turquette,
Stephen Boyd, Daniel Lezcano, Thomas Gleixner, Alessandro Zummo,
Alexandre Belloni, Wim Van Sebroeck, Guenter Roeck,
Sebastian Reichel, Thierry Reding, Uwe Kleine-König,
Mark Brown, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Andy Shevchenko,
Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-13-3d63a5f1103e@maquefel.me>
Hi Nikita,
On Thu, 2023-07-20 at 14:29 +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Add OF ID match table.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
Tested-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
--
Alexander Sverdlin.
^ permalink raw reply
* [dtor-input:next] BUILD SUCCESS bf93349b11ab6f4a5861fbabe01236bf3d58075a
From: kernel test robot @ 2023-07-19 18:36 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
branch HEAD: bf93349b11ab6f4a5861fbabe01236bf3d58075a Input: mms114 - add support for touch keys
elapsed time: 720m
configs tested: 142
configs skipped: 10
The following configs have been built successfully.
More configs may be tested in the coming days.
tested configs:
alpha allyesconfig gcc
alpha defconfig gcc
alpha randconfig-r015-20230718 gcc
alpha randconfig-r016-20230718 gcc
alpha randconfig-r036-20230718 gcc
arc allyesconfig gcc
arc defconfig gcc
arc randconfig-r033-20230718 gcc
arc randconfig-r043-20230718 gcc
arc vdk_hs38_smp_defconfig gcc
arm allmodconfig gcc
arm allyesconfig gcc
arm am200epdkit_defconfig clang
arm defconfig gcc
arm ixp4xx_defconfig clang
arm omap1_defconfig clang
arm randconfig-r001-20230718 clang
arm randconfig-r003-20230718 clang
arm randconfig-r033-20230718 clang
arm randconfig-r046-20230718 gcc
arm64 allyesconfig gcc
arm64 defconfig gcc
arm64 randconfig-r011-20230718 clang
arm64 randconfig-r012-20230718 clang
arm64 randconfig-r023-20230718 clang
csky defconfig gcc
csky randconfig-r023-20230718 gcc
csky randconfig-r032-20230718 gcc
csky randconfig-r034-20230718 gcc
csky randconfig-r036-20230718 gcc
hexagon randconfig-r035-20230718 clang
hexagon randconfig-r041-20230718 clang
hexagon randconfig-r045-20230718 clang
i386 allyesconfig gcc
i386 buildonly-randconfig-r004-20230717 clang
i386 buildonly-randconfig-r004-20230718 gcc
i386 buildonly-randconfig-r005-20230717 clang
i386 buildonly-randconfig-r005-20230718 gcc
i386 buildonly-randconfig-r006-20230717 clang
i386 buildonly-randconfig-r006-20230718 gcc
i386 debian-10.3 gcc
i386 defconfig gcc
i386 randconfig-i001-20230718 gcc
i386 randconfig-i002-20230718 gcc
i386 randconfig-i003-20230718 gcc
i386 randconfig-i004-20230718 gcc
i386 randconfig-i005-20230718 gcc
i386 randconfig-i006-20230718 gcc
i386 randconfig-i011-20230718 clang
i386 randconfig-i012-20230718 clang
i386 randconfig-i013-20230718 clang
i386 randconfig-i014-20230718 clang
i386 randconfig-i015-20230718 clang
i386 randconfig-i016-20230718 clang
i386 randconfig-r001-20230718 gcc
i386 randconfig-r004-20230718 gcc
i386 randconfig-r034-20230718 gcc
loongarch allmodconfig gcc
loongarch allnoconfig gcc
loongarch defconfig gcc
loongarch randconfig-r031-20230718 gcc
loongarch randconfig-r034-20230718 gcc
m68k allmodconfig gcc
m68k allyesconfig gcc
m68k defconfig gcc
m68k randconfig-r004-20230718 gcc
m68k randconfig-r006-20230718 gcc
m68k randconfig-r022-20230718 gcc
microblaze randconfig-r021-20230718 gcc
microblaze randconfig-r032-20230718 gcc
microblaze randconfig-r033-20230718 gcc
mips allmodconfig gcc
mips allyesconfig gcc
mips randconfig-r005-20230718 clang
mips randconfig-r013-20230718 gcc
nios2 defconfig gcc
nios2 randconfig-r002-20230718 gcc
parisc allyesconfig gcc
parisc defconfig gcc
parisc randconfig-r024-20230718 gcc
parisc64 defconfig gcc
powerpc allmodconfig gcc
powerpc allnoconfig gcc
powerpc mgcoge_defconfig gcc
powerpc randconfig-r014-20230718 clang
powerpc randconfig-r015-20230718 clang
powerpc randconfig-r025-20230718 clang
riscv allmodconfig gcc
riscv allnoconfig gcc
riscv allyesconfig gcc
riscv defconfig gcc
riscv randconfig-r003-20230718 gcc
riscv randconfig-r006-20230718 gcc
riscv randconfig-r031-20230718 gcc
riscv randconfig-r042-20230718 clang
riscv rv32_defconfig gcc
s390 allmodconfig gcc
s390 allyesconfig gcc
s390 defconfig gcc
s390 randconfig-r005-20230718 gcc
s390 randconfig-r044-20230718 clang
sh allmodconfig gcc
sh randconfig-r012-20230718 gcc
sh se7724_defconfig gcc
sparc allyesconfig gcc
sparc defconfig gcc
sparc randconfig-r016-20230718 gcc
sparc randconfig-r025-20230718 gcc
sparc randconfig-r026-20230718 gcc
sparc64 randconfig-r024-20230718 gcc
sparc64 randconfig-r035-20230718 gcc
sparc64 randconfig-r036-20230718 gcc
um allmodconfig clang
um allnoconfig clang
um allyesconfig clang
um defconfig gcc
um i386_defconfig gcc
um x86_64_defconfig gcc
x86_64 allyesconfig gcc
x86_64 buildonly-randconfig-r001-20230717 clang
x86_64 buildonly-randconfig-r001-20230718 gcc
x86_64 buildonly-randconfig-r002-20230717 clang
x86_64 buildonly-randconfig-r002-20230718 gcc
x86_64 buildonly-randconfig-r003-20230717 clang
x86_64 buildonly-randconfig-r003-20230718 gcc
x86_64 defconfig gcc
x86_64 kexec gcc
x86_64 randconfig-r021-20230718 clang
x86_64 randconfig-x001-20230718 clang
x86_64 randconfig-x002-20230718 clang
x86_64 randconfig-x003-20230718 clang
x86_64 randconfig-x004-20230718 clang
x86_64 randconfig-x005-20230718 clang
x86_64 randconfig-x006-20230718 clang
x86_64 randconfig-x011-20230718 gcc
x86_64 randconfig-x012-20230718 gcc
x86_64 randconfig-x013-20230718 gcc
x86_64 randconfig-x014-20230718 gcc
x86_64 randconfig-x015-20230718 gcc
x86_64 randconfig-x016-20230718 gcc
x86_64 rhel-8.3-rust clang
x86_64 rhel-8.3 gcc
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: pm8xxx-vib - Add support for more PMICs
From: Fenglin Wu @ 2023-07-19 8:24 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: linux-arm-msm, linux-kernel, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Dmitry Torokhov, linux-input, quic_collinsd,
quic_subbaram, quic_kamalw, jestar, quic_huliu
In-Reply-To: <CAA8EJpoeFwu7P7jEqhmwttAeF4VAhaCSaKP4XrUQz1YVsg9zvw@mail.gmail.com>
On 7/19/2023 4:02 PM, Dmitry Baryshkov wrote:
> On Wed, 19 Jul 2023 at 07:09, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>
>>
>>
>> On 7/18/2023 7:04 PM, Dmitry Baryshkov wrote:
>>> On Tue, 18 Jul 2023 at 13:55, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 7/18/2023 5:41 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
>>>>>>> On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>>>>>>>
>>>>>>>> Add support for vibrator module inside PMI632, PM7250B, PM7325B.
>>>>>>>> It is very similar to vibrator inside PM8xxx but just the drive
>>>>>>>> amplitude is controlled through 2 bytes registers.
>>>>>>>>
>>>>>>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>>>>>>> ---
>>>>>>>> drivers/input/misc/pm8xxx-vibrator.c | 48 ++++++++++++++++++++++++++++
>>>>>>>> 1 file changed, 48 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
>>>>>>>> index 04cb87efd799..213fdfd47c7f 100644
>>>>>>>> --- a/drivers/input/misc/pm8xxx-vibrator.c
>>>>>>>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
>>>>>>>> @@ -25,6 +25,9 @@ struct pm8xxx_regs {
>>>>>>>> unsigned int drv_addr;
>>>>>>>> unsigned int drv_mask;
>>>>>>>> unsigned int drv_shift;
>>>>>>>> + unsigned int drv_addr2;
>
> Unused
>
>>>>>>>> + unsigned int drv_mask2;
>>>>>>>> + unsigned int drv_shift2;
>>>>>>>> unsigned int drv_en_manual_mask;
>>>>>>>> };
>>>>>>>>
>>>>>>>> @@ -44,6 +47,42 @@ static struct pm8xxx_regs pm8916_regs = {
>>>>>>>> .drv_en_manual_mask = 0,
>>>>>>>> };
>>>>>>>>
>>>>>>>> +static struct pm8xxx_regs pmi632_regs = {
>>>>>>>> + .enable_addr = 0x5746,
>>>>>>>> + .enable_mask = BIT(7),
>>>>>>>> + .drv_addr = 0x5740,
>>>>>>>> + .drv_mask = 0xff,
>>>>>>>> + .drv_shift = 0,
>>>>>>>> + .drv_addr2 = 0x5741,
>>>>>>>> + .drv_mask2 = 0x0f,
>>>>>>>> + .drv_shift2 = 8,
>>>>>>>
>>>>>>> I see that you are just expanding what was done for SSBI PMICs and
>>>>>>> later expanded to support pm8916. However it might be better to drop
>>>>>>> the hardcoded .drv_addr (and drv_addr2) and read address from DT
>>>>>>> instead.
>>>>>>>
>>>>>>
>>>>>> Right, this is the simplest change without updating the code logic too
>>>>>> much. If we decided to read .drv_addr and .drv_add2 from DT, we will
>>>>>> have to read .enable_addr along with all other mask/shift for each
>>>>>> register address from DT as well because they are not consistent from
>>>>>> target to target. I don't know how would you suggest to add the DT
>>>>>> properties for all of them, but if we end up to add a property for each
>>>>>> of them, it won't be cleaner than hard-coding them.
>>>>>
>>>>> No, we (correctly) have device compatibles for that. The issue with
>>>>> hardcoding register addresses is that it adds extra issues here.
>>>>>
>>>>> If I understand correctly, we have several 'generation':
>>>>> - SSBI PMIC, shifted 5-bit mask, en_manual_mask, no enable_register.
>>>>> - older SPMI PMIC, 5 bit drv_mask, 0 en_manual_mask, enable register at +6
>>>>> - new SPMI PMIC, 12 bit drv_mask, 0 en_manual_mask, enable register at +6
>>>>>
>>>>> For the last generation you are adding three independent entries,
>>>>> while the block looks the same. If you remove drv_addr (and get it
>>>>> from reg property), it would allow us to keep only the functional data
>>>>> in struct pm8xxxx_regs (masks / shifts).
>>>>>
>>>>
>>>> Okay, let me know if I understood it correctly, this is what you are
>>>> suggesting:
>>>>
>>>> - hard code the mask/shifts and still keep them in struct pm8xxx_regs,
>>>> combine the drv_mask2 to the upper byte of the drv_mask, so we will
>>>> have following data structure for the 3rd generation vibrator
>>>>
>>>> static struct pm8xxx_regs pm7250b_regs = {
>>>> .enable_addr = 0x5346,
>>>> .enable_mask = BIT(7),
>>>> .drv_mask = 0xfff,
>>>> .drv_shift = 0,
>>>> .drv_en_manual_mask = 0,
>>>> };
>>>>
>>>>
>>>> - move the drv_addr/drv_addr2 into DT, read them from 'reg' property.
>>>> Because of 'mfd/qcom,spmi-pmic.yaml' has defined the 'address-cells'
>>>> as 1 and the 'size-cells' as 0 for qcom spmi devices, we couldn't
>>>> specify the address size to 2 even the drv_addr for the 3rd
>>>> generation vibrator is 2 adjacent bytes. So we will end of having
>>>> following DT scheme:
>>>>
>>>> For the 2nd generation which only has drv_addr
>>>> vibrator@c041 {
>>>> compatible = "qcom,pm8916-vib";
>>>> reg = <0xc041>; /* drv_addr */
>>>
>>> No. This is <0xc000>.
>>>
>>>> ...
>>>> };
>>>>
>>>> For the 3rd generation which has both drv_addr and drv_addr2
>>>> vibrator@5340 {
>>>> compatible = "qcom,pm7250b-vib";
>>>> reg = <0x5340>, /* drv_addr */
>>>> <0x5341>; /* drv_addr2 */
>>>> ...
>>>> };
>>>>
>>>> Not sure how do you feel, I actually don't see too much benefit than
>>>> hard-coding them in the driver.
>>>> We will end up having code to check how many u32 value in the 'reg' and
>>>> only assign it to drv_addr2 when the 2nd is available, also when
>>>> programming drv_addr2 register, the driver will always assume the mask
>>>> is in the upper byte of the drv_mask and the shift to the drive level is
>>>> 8 (this seems hacky to me and it was my biggest concern while I made
>>>> this change, and it led me to defining drv_shift2/drv_mask2 along with
>>>> drv_addr2).
>>>
>>> We only need drv_addr2 if drv_mask has more than 8 bits. So you don't
>>> have to specify it in the DT. It is always equal to base_reg + 0x41.
>>> The same way drv_addr is always equal to base_reg + 0x40 for all
>>> SPMI-based PMIC vibrator devices.
>>>
>>
>> Thanks. I got it now, I agree this will be beneficial for the case that
>> different PMICs have the same vibrator module but with different
>> register base address. I am going to change it to this way, let me know
>> if this is what you thought:
>>
>> @@ -25,6 +29,9 @@ struct pm8xxx_regs {
>> unsigned int drv_addr;
>> unsigned int drv_mask;
>> unsigned int drv_shift;
>> + unsigned int drv_addr2;
>> + unsigned int drv_mask2;
>> + unsigned int drv_shift2;
>> unsigned int drv_en_manual_mask;
>> };
>>
>> +static struct pm8xxx_regs spmi_vib_regs = {
>> + .enable_mask = BIT(7),
>> + .drv_mask = 0xff,
>> + .drv_shift = 0,
>> + .drv_mask2 = 0xf,
>> + .drv_shift2 = 8,
>> + .drv_en_manual_mask = 0,
>> +};
>
> Ideally the static data should be const. I'd suggest moving
> drv_addr/drv_addr2 to struct pm8xxx_vib.
>
>> +
>>
>> +#define SPMI_VIB_VSET_LB_REG 0x40
>> +#define SPMI_VIB_VSET_UP_REG 0x41
>> +#define SPMI_VIB_EN_CTL_REG 0x46
>> +
>>
>> regs = of_device_get_match_data(&pdev->dev);
>>
>> + if (regs->drv_addr == 0) {
>> + rc = fwnode_property_read_u32(pdev->dev.fwnode,
>> + "reg", ®_base);
>> + if (rc < 0)
>> + return rc;
>> +
>> + regs->enable_addr = reg_base + SPMI_VIB_EN_CTL_REG;
>> + regs->drv_addr = reg_base + SPMI_VIB_VSET_LB_REG;
>> + regs->drv_addr2 = reg_base + SPMI_VIB_VSET_UP_REG;
>
> Yes, this looks good (except s/regs->/vib->/). Moreover this also
> applies to pm8916. I'd suggest splitting this into two patches: first,
> refactor pm8916 support to use reg, then add support for new devices.
Thanks. I will refactor this, test it, and send it out. The only problem
is I don't have a pm8916 device with me, but I guess the change should
be straightforward and I will rely on the test result on my PM7550BA
device which has the vibrator with the latest generation.
>
>> + }
>> +
>>
>>
>> @@ -242,6 +277,7 @@ static const struct of_device_id
>> pm8xxx_vib_id_table[] = {
>> { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
>> { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
>> { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
>> + ( .compabitle = "qcom,spmi-vib", .data = &spmi_vib_regs },
>> { }
>>
>>
>>>>
>>>>
>>>>
>>>>>>
>>>>>>
>>>>>>>> + .drv_en_manual_mask = 0,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +static struct pm8xxx_regs pm7250b_regs = {
>>>>>>>> + .enable_addr = 0x5346,
>>>>>>>> + .enable_mask = BIT(7),
>>>>>>>> + .drv_addr = 0x5340,
>>>>>>>> + .drv_mask = 0xff,
>>>>>>>> + .drv_shift = 0,
>>>>>>>> + .drv_addr2 = 0x5341,
>>>>>>>> + .drv_mask2 = 0x0f,
>>>>>>>> + .drv_shift2 = 8,
>>>>>>>> + .drv_en_manual_mask = 0,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +static struct pm8xxx_regs pm7325b_regs = {
>>>>>>>> + .enable_addr = 0xdf46,
>>>>>>>> + .enable_mask = BIT(7),
>>>>>>>> + .drv_addr = 0xdf40,
>>>>>>>> + .drv_mask = 0xff,
>>>>>>>> + .drv_shift = 0,
>>>>>>>> + .drv_addr2 = 0xdf41,
>>>>>>>> + .drv_mask2 = 0x0f,
>>>>>>>> + .drv_shift2 = 8,
>>>>>>>> + .drv_en_manual_mask = 0,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> /**
>>>>>>>> * struct pm8xxx_vib - structure to hold vibrator data
>>>>>>>> * @vib_input_dev: input device supporting force feedback
>>>>>>>> @@ -87,6 +126,12 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
>>>>>>>> return rc;
>>>>>>>>
>>>>>>>> vib->reg_vib_drv = val;
>>>>>>>> + if (regs->drv_addr2 != 0 && on) {
>>>>>>>> + val = (vib->level << regs->drv_shift2) & regs->drv_mask2;
>>>>>>>> + rc = regmap_write(vib->regmap, regs->drv_addr2, val);
>>>>>>>> + if (rc < 0)
>>>>>>>> + return rc;
>>>>>>>> + }
>>>>>>>>
>>>>>>>> if (regs->enable_mask)
>>>>>>>> rc = regmap_update_bits(vib->regmap, regs->enable_addr,
>>>>>>>> @@ -242,6 +287,9 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
>>>>>>>> { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
>>>>>>>> { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
>>>>>>>> { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
>>>>>>>> + { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
>>>>>>>> + { .compatible = "qcom,pm7250b-vib", .data = &pm7250b_regs },
>>>>>>>> + { .compatible = "qcom,pm7325b-vib", .data = &pm7325b_regs },
>>>>>>>> { }
>>>>>>>> };
>>>>>>>> MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
>>>>>>>> --
>>>>>>>> 2.25.1
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
>
>
>
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: pm8xxx-vib - Add support for more PMICs
From: Dmitry Baryshkov @ 2023-07-19 8:02 UTC (permalink / raw)
To: Fenglin Wu
Cc: linux-arm-msm, linux-kernel, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Dmitry Torokhov, linux-input, quic_collinsd,
quic_subbaram, quic_kamalw, jestar, quic_huliu
In-Reply-To: <138fac39-8c28-2c14-0b16-ec08189179cf@quicinc.com>
On Wed, 19 Jul 2023 at 07:09, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>
>
>
> On 7/18/2023 7:04 PM, Dmitry Baryshkov wrote:
> > On Tue, 18 Jul 2023 at 13:55, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 7/18/2023 5:41 PM, Dmitry Baryshkov wrote:
> >>> On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
> >>>>> On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
> >>>>>>
> >>>>>> Add support for vibrator module inside PMI632, PM7250B, PM7325B.
> >>>>>> It is very similar to vibrator inside PM8xxx but just the drive
> >>>>>> amplitude is controlled through 2 bytes registers.
> >>>>>>
> >>>>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> >>>>>> ---
> >>>>>> drivers/input/misc/pm8xxx-vibrator.c | 48 ++++++++++++++++++++++++++++
> >>>>>> 1 file changed, 48 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> >>>>>> index 04cb87efd799..213fdfd47c7f 100644
> >>>>>> --- a/drivers/input/misc/pm8xxx-vibrator.c
> >>>>>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> >>>>>> @@ -25,6 +25,9 @@ struct pm8xxx_regs {
> >>>>>> unsigned int drv_addr;
> >>>>>> unsigned int drv_mask;
> >>>>>> unsigned int drv_shift;
> >>>>>> + unsigned int drv_addr2;
Unused
> >>>>>> + unsigned int drv_mask2;
> >>>>>> + unsigned int drv_shift2;
> >>>>>> unsigned int drv_en_manual_mask;
> >>>>>> };
> >>>>>>
> >>>>>> @@ -44,6 +47,42 @@ static struct pm8xxx_regs pm8916_regs = {
> >>>>>> .drv_en_manual_mask = 0,
> >>>>>> };
> >>>>>>
> >>>>>> +static struct pm8xxx_regs pmi632_regs = {
> >>>>>> + .enable_addr = 0x5746,
> >>>>>> + .enable_mask = BIT(7),
> >>>>>> + .drv_addr = 0x5740,
> >>>>>> + .drv_mask = 0xff,
> >>>>>> + .drv_shift = 0,
> >>>>>> + .drv_addr2 = 0x5741,
> >>>>>> + .drv_mask2 = 0x0f,
> >>>>>> + .drv_shift2 = 8,
> >>>>>
> >>>>> I see that you are just expanding what was done for SSBI PMICs and
> >>>>> later expanded to support pm8916. However it might be better to drop
> >>>>> the hardcoded .drv_addr (and drv_addr2) and read address from DT
> >>>>> instead.
> >>>>>
> >>>>
> >>>> Right, this is the simplest change without updating the code logic too
> >>>> much. If we decided to read .drv_addr and .drv_add2 from DT, we will
> >>>> have to read .enable_addr along with all other mask/shift for each
> >>>> register address from DT as well because they are not consistent from
> >>>> target to target. I don't know how would you suggest to add the DT
> >>>> properties for all of them, but if we end up to add a property for each
> >>>> of them, it won't be cleaner than hard-coding them.
> >>>
> >>> No, we (correctly) have device compatibles for that. The issue with
> >>> hardcoding register addresses is that it adds extra issues here.
> >>>
> >>> If I understand correctly, we have several 'generation':
> >>> - SSBI PMIC, shifted 5-bit mask, en_manual_mask, no enable_register.
> >>> - older SPMI PMIC, 5 bit drv_mask, 0 en_manual_mask, enable register at +6
> >>> - new SPMI PMIC, 12 bit drv_mask, 0 en_manual_mask, enable register at +6
> >>>
> >>> For the last generation you are adding three independent entries,
> >>> while the block looks the same. If you remove drv_addr (and get it
> >>> from reg property), it would allow us to keep only the functional data
> >>> in struct pm8xxxx_regs (masks / shifts).
> >>>
> >>
> >> Okay, let me know if I understood it correctly, this is what you are
> >> suggesting:
> >>
> >> - hard code the mask/shifts and still keep them in struct pm8xxx_regs,
> >> combine the drv_mask2 to the upper byte of the drv_mask, so we will
> >> have following data structure for the 3rd generation vibrator
> >>
> >> static struct pm8xxx_regs pm7250b_regs = {
> >> .enable_addr = 0x5346,
> >> .enable_mask = BIT(7),
> >> .drv_mask = 0xfff,
> >> .drv_shift = 0,
> >> .drv_en_manual_mask = 0,
> >> };
> >>
> >>
> >> - move the drv_addr/drv_addr2 into DT, read them from 'reg' property.
> >> Because of 'mfd/qcom,spmi-pmic.yaml' has defined the 'address-cells'
> >> as 1 and the 'size-cells' as 0 for qcom spmi devices, we couldn't
> >> specify the address size to 2 even the drv_addr for the 3rd
> >> generation vibrator is 2 adjacent bytes. So we will end of having
> >> following DT scheme:
> >>
> >> For the 2nd generation which only has drv_addr
> >> vibrator@c041 {
> >> compatible = "qcom,pm8916-vib";
> >> reg = <0xc041>; /* drv_addr */
> >
> > No. This is <0xc000>.
> >
> >> ...
> >> };
> >>
> >> For the 3rd generation which has both drv_addr and drv_addr2
> >> vibrator@5340 {
> >> compatible = "qcom,pm7250b-vib";
> >> reg = <0x5340>, /* drv_addr */
> >> <0x5341>; /* drv_addr2 */
> >> ...
> >> };
> >>
> >> Not sure how do you feel, I actually don't see too much benefit than
> >> hard-coding them in the driver.
> >> We will end up having code to check how many u32 value in the 'reg' and
> >> only assign it to drv_addr2 when the 2nd is available, also when
> >> programming drv_addr2 register, the driver will always assume the mask
> >> is in the upper byte of the drv_mask and the shift to the drive level is
> >> 8 (this seems hacky to me and it was my biggest concern while I made
> >> this change, and it led me to defining drv_shift2/drv_mask2 along with
> >> drv_addr2).
> >
> > We only need drv_addr2 if drv_mask has more than 8 bits. So you don't
> > have to specify it in the DT. It is always equal to base_reg + 0x41.
> > The same way drv_addr is always equal to base_reg + 0x40 for all
> > SPMI-based PMIC vibrator devices.
> >
>
> Thanks. I got it now, I agree this will be beneficial for the case that
> different PMICs have the same vibrator module but with different
> register base address. I am going to change it to this way, let me know
> if this is what you thought:
>
> @@ -25,6 +29,9 @@ struct pm8xxx_regs {
> unsigned int drv_addr;
> unsigned int drv_mask;
> unsigned int drv_shift;
> + unsigned int drv_addr2;
> + unsigned int drv_mask2;
> + unsigned int drv_shift2;
> unsigned int drv_en_manual_mask;
> };
>
> +static struct pm8xxx_regs spmi_vib_regs = {
> + .enable_mask = BIT(7),
> + .drv_mask = 0xff,
> + .drv_shift = 0,
> + .drv_mask2 = 0xf,
> + .drv_shift2 = 8,
> + .drv_en_manual_mask = 0,
> +};
Ideally the static data should be const. I'd suggest moving
drv_addr/drv_addr2 to struct pm8xxx_vib.
> +
>
> +#define SPMI_VIB_VSET_LB_REG 0x40
> +#define SPMI_VIB_VSET_UP_REG 0x41
> +#define SPMI_VIB_EN_CTL_REG 0x46
> +
>
> regs = of_device_get_match_data(&pdev->dev);
>
> + if (regs->drv_addr == 0) {
> + rc = fwnode_property_read_u32(pdev->dev.fwnode,
> + "reg", ®_base);
> + if (rc < 0)
> + return rc;
> +
> + regs->enable_addr = reg_base + SPMI_VIB_EN_CTL_REG;
> + regs->drv_addr = reg_base + SPMI_VIB_VSET_LB_REG;
> + regs->drv_addr2 = reg_base + SPMI_VIB_VSET_UP_REG;
Yes, this looks good (except s/regs->/vib->/). Moreover this also
applies to pm8916. I'd suggest splitting this into two patches: first,
refactor pm8916 support to use reg, then add support for new devices.
> + }
> +
>
>
> @@ -242,6 +277,7 @@ static const struct of_device_id
> pm8xxx_vib_id_table[] = {
> { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
> { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
> { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
> + ( .compabitle = "qcom,spmi-vib", .data = &spmi_vib_regs },
> { }
>
>
> >>
> >>
> >>
> >>>>
> >>>>
> >>>>>> + .drv_en_manual_mask = 0,
> >>>>>> +};
> >>>>>> +
> >>>>>> +static struct pm8xxx_regs pm7250b_regs = {
> >>>>>> + .enable_addr = 0x5346,
> >>>>>> + .enable_mask = BIT(7),
> >>>>>> + .drv_addr = 0x5340,
> >>>>>> + .drv_mask = 0xff,
> >>>>>> + .drv_shift = 0,
> >>>>>> + .drv_addr2 = 0x5341,
> >>>>>> + .drv_mask2 = 0x0f,
> >>>>>> + .drv_shift2 = 8,
> >>>>>> + .drv_en_manual_mask = 0,
> >>>>>> +};
> >>>>>> +
> >>>>>> +static struct pm8xxx_regs pm7325b_regs = {
> >>>>>> + .enable_addr = 0xdf46,
> >>>>>> + .enable_mask = BIT(7),
> >>>>>> + .drv_addr = 0xdf40,
> >>>>>> + .drv_mask = 0xff,
> >>>>>> + .drv_shift = 0,
> >>>>>> + .drv_addr2 = 0xdf41,
> >>>>>> + .drv_mask2 = 0x0f,
> >>>>>> + .drv_shift2 = 8,
> >>>>>> + .drv_en_manual_mask = 0,
> >>>>>> +};
> >>>>>> +
> >>>>>> /**
> >>>>>> * struct pm8xxx_vib - structure to hold vibrator data
> >>>>>> * @vib_input_dev: input device supporting force feedback
> >>>>>> @@ -87,6 +126,12 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
> >>>>>> return rc;
> >>>>>>
> >>>>>> vib->reg_vib_drv = val;
> >>>>>> + if (regs->drv_addr2 != 0 && on) {
> >>>>>> + val = (vib->level << regs->drv_shift2) & regs->drv_mask2;
> >>>>>> + rc = regmap_write(vib->regmap, regs->drv_addr2, val);
> >>>>>> + if (rc < 0)
> >>>>>> + return rc;
> >>>>>> + }
> >>>>>>
> >>>>>> if (regs->enable_mask)
> >>>>>> rc = regmap_update_bits(vib->regmap, regs->enable_addr,
> >>>>>> @@ -242,6 +287,9 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
> >>>>>> { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
> >>>>>> { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
> >>>>>> { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
> >>>>>> + { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
> >>>>>> + { .compatible = "qcom,pm7250b-vib", .data = &pm7250b_regs },
> >>>>>> + { .compatible = "qcom,pm7325b-vib", .data = &pm7325b_regs },
> >>>>>> { }
> >>>>>> };
> >>>>>> MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
> >>>>>> --
> >>>>>> 2.25.1
> >>>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >>>
> >
> >
> >
--
With best wishes
Dmitry
^ permalink raw reply
* RE: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
From: Biju Das @ 2023-07-19 6:43 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mark Brown, Mike Looijmans, Andreas Helbech Kleist,
Geert Uytterhoeven, Uwe Kleine-König,
linux-input@vger.kernel.org, Prabhakar Mahadev Lad,
linux-renesas-soc@vger.kernel.org, Wolfram Sang, Andy Shevchenko,
linux-kernel@vger.kernel.org
In-Reply-To: <ZLbbslBiIQXFWpmN@google.com>
Hi Dmitry Torokhov,
Thanks for the feedback.
> Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
>
> On Mon, Jul 17, 2023 at 06:45:27PM +0000, Biju Das wrote:
> > Hi Dmitry,
> >
> > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
> > >
> > > On Mon, Jul 17, 2023 at 07:15:50PM +0100, Mark Brown wrote:
> > > > On Mon, Jul 17, 2023 at 04:35:02PM +0000, Biju Das wrote:
> > > >
> > > > > The .device_get_match_data callbacks are missing for I2C and SPI
> > > > > bus
> > > subsystems.
> > > > > Can you please throw some lights on this?
> > > >
> > > > It's the first time I've ever heard of that callback, I don't know
> > > > why whoever added it wouldn't have done those buses in particular
> > > > or if it just didn't happen. Try adding it and if it works send
> the patches?
> > >
> > > I think there is a disconnect. Right now device_get_match_data
> > > callbacks are part of fwnode_operations. I was proposing to add
> > > another optional device_get_match_data callback to 'struct bus_type'
> > > to allow individual buses control how match data is handled, before
> > > (or after) jumping into the fwnode-backed device_get_match_data
> callbacks.
> >
> > That is what implemented here [1] and [2] right?
[1] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/spi/spi.c#L364
[2] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/i2c/i2c-core-base.c#L117
> >
> >
> > First it check for fwnode-backed device_get_match_data callbacks and
> > Fallback is bus-type based match.
> >
> > Looks like you are proposing to unify [1] and [2] and you want the
> > logic to be other way around. ie, first bus-type match, then
> > fwnode-backed callbacks?
> >
>
> I do not have a strong preference for the ordering, i.e. I think it is
> perfectly fine to do the generic fwnode-based lookup and if there is no
> match have bus method called as a fallback,
That involves a bit of work.
const void *device_get_match_data(const struct device *dev);
const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
const struct i2c_client *client);
const struct spi_device_id *spi_get_device_id(const struct spi_device *sdev);
Basically, the bus-client driver(such as exc3000) needs to pass struct device
and device_get_match_data after generic fwnode-based lookup,
needs to find the bus type based on struct device and call a new generic
void* bus_get_match_data(void*) callback, so that each bus interface
can do a match.
I am not sure, is this proposal acceptable to wider people??
> but I do not want driver
> writers to learn about multiple <bus-prefix>_get_match_data()
> implementations, I would prefer if they could call
> device_get_match_data() and the right thing happened in all cases.
The driver is bus specific. So I don't know, why you want to
be it generic. If it is i2c client, like other I2C api call the bus-subsystem api for match_data. Similarly, if it is spi client, do the same.
Cheers,
Biju
^ permalink raw reply
* Re: [RESEND PATCH 2/2] Input: mms114 - add support for touch keys
From: Dmitry Torokhov @ 2023-07-19 6:22 UTC (permalink / raw)
To: Artur Weber
Cc: Rob Herring, Krzysztof Kozlowski, Linus Walleij, linux-input,
devicetree, linux-kernel, ~postmarketos/upstreaming
In-Reply-To: <20230714100424.29798-3-aweber.kernel@gmail.com>
On Fri, Jul 14, 2023 at 12:04:24PM +0200, Artur Weber wrote:
> + } else if (data->num_keycodes > MMS114_MAX_TOUCHKEYS) {
> + dev_warn(&client->dev,
> + "Found %d linux,keycodes but max is %zd, ignoring the rest\n",
> + data->num_keycodes, MMS114_MAX_TOUCHKEYS);
This gives me the following warning:
drivers/input/touchscreen/mms114.c: In function ‘mms114_probe’:
drivers/input/touchscreen/mms114.c:520:25: error: format ‘%zd’ expects argument of type ‘signed size_t’, but argument 4 has type ‘int’ [-Werror=format=]
520 | "Found %d linux,keycodes but max is %zd, ignoring the rest\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I changed the format specifier to "%d" and applied.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: pm8xxx-vib - Add support for more PMICs
From: Fenglin Wu @ 2023-07-19 4:09 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: linux-arm-msm, linux-kernel, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Dmitry Torokhov, linux-input, quic_collinsd,
quic_subbaram, quic_kamalw, jestar, quic_huliu
In-Reply-To: <CAA8EJppjshTCqeYQL1QUbLd03bopZjGHuOhoFqy7P8XuzYXc9w@mail.gmail.com>
On 7/18/2023 7:04 PM, Dmitry Baryshkov wrote:
> On Tue, 18 Jul 2023 at 13:55, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>
>>
>>
>> On 7/18/2023 5:41 PM, Dmitry Baryshkov wrote:
>>> On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>>>>>
>>>>>> Add support for vibrator module inside PMI632, PM7250B, PM7325B.
>>>>>> It is very similar to vibrator inside PM8xxx but just the drive
>>>>>> amplitude is controlled through 2 bytes registers.
>>>>>>
>>>>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>>>>> ---
>>>>>> drivers/input/misc/pm8xxx-vibrator.c | 48 ++++++++++++++++++++++++++++
>>>>>> 1 file changed, 48 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
>>>>>> index 04cb87efd799..213fdfd47c7f 100644
>>>>>> --- a/drivers/input/misc/pm8xxx-vibrator.c
>>>>>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
>>>>>> @@ -25,6 +25,9 @@ struct pm8xxx_regs {
>>>>>> unsigned int drv_addr;
>>>>>> unsigned int drv_mask;
>>>>>> unsigned int drv_shift;
>>>>>> + unsigned int drv_addr2;
>>>>>> + unsigned int drv_mask2;
>>>>>> + unsigned int drv_shift2;
>>>>>> unsigned int drv_en_manual_mask;
>>>>>> };
>>>>>>
>>>>>> @@ -44,6 +47,42 @@ static struct pm8xxx_regs pm8916_regs = {
>>>>>> .drv_en_manual_mask = 0,
>>>>>> };
>>>>>>
>>>>>> +static struct pm8xxx_regs pmi632_regs = {
>>>>>> + .enable_addr = 0x5746,
>>>>>> + .enable_mask = BIT(7),
>>>>>> + .drv_addr = 0x5740,
>>>>>> + .drv_mask = 0xff,
>>>>>> + .drv_shift = 0,
>>>>>> + .drv_addr2 = 0x5741,
>>>>>> + .drv_mask2 = 0x0f,
>>>>>> + .drv_shift2 = 8,
>>>>>
>>>>> I see that you are just expanding what was done for SSBI PMICs and
>>>>> later expanded to support pm8916. However it might be better to drop
>>>>> the hardcoded .drv_addr (and drv_addr2) and read address from DT
>>>>> instead.
>>>>>
>>>>
>>>> Right, this is the simplest change without updating the code logic too
>>>> much. If we decided to read .drv_addr and .drv_add2 from DT, we will
>>>> have to read .enable_addr along with all other mask/shift for each
>>>> register address from DT as well because they are not consistent from
>>>> target to target. I don't know how would you suggest to add the DT
>>>> properties for all of them, but if we end up to add a property for each
>>>> of them, it won't be cleaner than hard-coding them.
>>>
>>> No, we (correctly) have device compatibles for that. The issue with
>>> hardcoding register addresses is that it adds extra issues here.
>>>
>>> If I understand correctly, we have several 'generation':
>>> - SSBI PMIC, shifted 5-bit mask, en_manual_mask, no enable_register.
>>> - older SPMI PMIC, 5 bit drv_mask, 0 en_manual_mask, enable register at +6
>>> - new SPMI PMIC, 12 bit drv_mask, 0 en_manual_mask, enable register at +6
>>>
>>> For the last generation you are adding three independent entries,
>>> while the block looks the same. If you remove drv_addr (and get it
>>> from reg property), it would allow us to keep only the functional data
>>> in struct pm8xxxx_regs (masks / shifts).
>>>
>>
>> Okay, let me know if I understood it correctly, this is what you are
>> suggesting:
>>
>> - hard code the mask/shifts and still keep them in struct pm8xxx_regs,
>> combine the drv_mask2 to the upper byte of the drv_mask, so we will
>> have following data structure for the 3rd generation vibrator
>>
>> static struct pm8xxx_regs pm7250b_regs = {
>> .enable_addr = 0x5346,
>> .enable_mask = BIT(7),
>> .drv_mask = 0xfff,
>> .drv_shift = 0,
>> .drv_en_manual_mask = 0,
>> };
>>
>>
>> - move the drv_addr/drv_addr2 into DT, read them from 'reg' property.
>> Because of 'mfd/qcom,spmi-pmic.yaml' has defined the 'address-cells'
>> as 1 and the 'size-cells' as 0 for qcom spmi devices, we couldn't
>> specify the address size to 2 even the drv_addr for the 3rd
>> generation vibrator is 2 adjacent bytes. So we will end of having
>> following DT scheme:
>>
>> For the 2nd generation which only has drv_addr
>> vibrator@c041 {
>> compatible = "qcom,pm8916-vib";
>> reg = <0xc041>; /* drv_addr */
>
> No. This is <0xc000>.
>
>> ...
>> };
>>
>> For the 3rd generation which has both drv_addr and drv_addr2
>> vibrator@5340 {
>> compatible = "qcom,pm7250b-vib";
>> reg = <0x5340>, /* drv_addr */
>> <0x5341>; /* drv_addr2 */
>> ...
>> };
>>
>> Not sure how do you feel, I actually don't see too much benefit than
>> hard-coding them in the driver.
>> We will end up having code to check how many u32 value in the 'reg' and
>> only assign it to drv_addr2 when the 2nd is available, also when
>> programming drv_addr2 register, the driver will always assume the mask
>> is in the upper byte of the drv_mask and the shift to the drive level is
>> 8 (this seems hacky to me and it was my biggest concern while I made
>> this change, and it led me to defining drv_shift2/drv_mask2 along with
>> drv_addr2).
>
> We only need drv_addr2 if drv_mask has more than 8 bits. So you don't
> have to specify it in the DT. It is always equal to base_reg + 0x41.
> The same way drv_addr is always equal to base_reg + 0x40 for all
> SPMI-based PMIC vibrator devices.
>
Thanks. I got it now, I agree this will be beneficial for the case that
different PMICs have the same vibrator module but with different
register base address. I am going to change it to this way, let me know
if this is what you thought:
@@ -25,6 +29,9 @@ struct pm8xxx_regs {
unsigned int drv_addr;
unsigned int drv_mask;
unsigned int drv_shift;
+ unsigned int drv_addr2;
+ unsigned int drv_mask2;
+ unsigned int drv_shift2;
unsigned int drv_en_manual_mask;
};
+static struct pm8xxx_regs spmi_vib_regs = {
+ .enable_mask = BIT(7),
+ .drv_mask = 0xff,
+ .drv_shift = 0,
+ .drv_mask2 = 0xf,
+ .drv_shift2 = 8,
+ .drv_en_manual_mask = 0,
+};
+
+#define SPMI_VIB_VSET_LB_REG 0x40
+#define SPMI_VIB_VSET_UP_REG 0x41
+#define SPMI_VIB_EN_CTL_REG 0x46
+
regs = of_device_get_match_data(&pdev->dev);
+ if (regs->drv_addr == 0) {
+ rc = fwnode_property_read_u32(pdev->dev.fwnode,
+ "reg", ®_base);
+ if (rc < 0)
+ return rc;
+
+ regs->enable_addr = reg_base + SPMI_VIB_EN_CTL_REG;
+ regs->drv_addr = reg_base + SPMI_VIB_VSET_LB_REG;
+ regs->drv_addr2 = reg_base + SPMI_VIB_VSET_UP_REG;
+ }
+
@@ -242,6 +277,7 @@ static const struct of_device_id
pm8xxx_vib_id_table[] = {
{ .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
{ .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
{ .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
+ ( .compabitle = "qcom,spmi-vib", .data = &spmi_vib_regs },
{ }
>>
>>
>>
>>>>
>>>>
>>>>>> + .drv_en_manual_mask = 0,
>>>>>> +};
>>>>>> +
>>>>>> +static struct pm8xxx_regs pm7250b_regs = {
>>>>>> + .enable_addr = 0x5346,
>>>>>> + .enable_mask = BIT(7),
>>>>>> + .drv_addr = 0x5340,
>>>>>> + .drv_mask = 0xff,
>>>>>> + .drv_shift = 0,
>>>>>> + .drv_addr2 = 0x5341,
>>>>>> + .drv_mask2 = 0x0f,
>>>>>> + .drv_shift2 = 8,
>>>>>> + .drv_en_manual_mask = 0,
>>>>>> +};
>>>>>> +
>>>>>> +static struct pm8xxx_regs pm7325b_regs = {
>>>>>> + .enable_addr = 0xdf46,
>>>>>> + .enable_mask = BIT(7),
>>>>>> + .drv_addr = 0xdf40,
>>>>>> + .drv_mask = 0xff,
>>>>>> + .drv_shift = 0,
>>>>>> + .drv_addr2 = 0xdf41,
>>>>>> + .drv_mask2 = 0x0f,
>>>>>> + .drv_shift2 = 8,
>>>>>> + .drv_en_manual_mask = 0,
>>>>>> +};
>>>>>> +
>>>>>> /**
>>>>>> * struct pm8xxx_vib - structure to hold vibrator data
>>>>>> * @vib_input_dev: input device supporting force feedback
>>>>>> @@ -87,6 +126,12 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
>>>>>> return rc;
>>>>>>
>>>>>> vib->reg_vib_drv = val;
>>>>>> + if (regs->drv_addr2 != 0 && on) {
>>>>>> + val = (vib->level << regs->drv_shift2) & regs->drv_mask2;
>>>>>> + rc = regmap_write(vib->regmap, regs->drv_addr2, val);
>>>>>> + if (rc < 0)
>>>>>> + return rc;
>>>>>> + }
>>>>>>
>>>>>> if (regs->enable_mask)
>>>>>> rc = regmap_update_bits(vib->regmap, regs->enable_addr,
>>>>>> @@ -242,6 +287,9 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
>>>>>> { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
>>>>>> { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
>>>>>> { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
>>>>>> + { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
>>>>>> + { .compatible = "qcom,pm7250b-vib", .data = &pm7250b_regs },
>>>>>> + { .compatible = "qcom,pm7325b-vib", .data = &pm7325b_regs },
>>>>>> { }
>>>>>> };
>>>>>> MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
>>>>>> --
>>>>>> 2.25.1
>>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
>
>
>
^ permalink raw reply
* Re: [PATCH] Input: da9063 - add wakeup support
From: Dmitry Torokhov @ 2023-07-18 18:46 UTC (permalink / raw)
To: Samuel Holland; +Cc: Support Opensource, linux-input, linux-kernel
In-Reply-To: <20230717192004.1304287-1-samuel.holland@sifive.com>
On Mon, Jul 17, 2023 at 12:20:03PM -0700, Samuel Holland wrote:
> Mark the IRQ as a wake IRQ so it will be enabled during system suspend.
>
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
From: Dmitry Torokhov @ 2023-07-18 18:36 UTC (permalink / raw)
To: Biju Das
Cc: Mark Brown, Mike Looijmans, Andreas Helbech Kleist,
Geert Uytterhoeven, Uwe Kleine-König,
linux-input@vger.kernel.org, Prabhakar Mahadev Lad,
linux-renesas-soc@vger.kernel.org, Wolfram Sang, Andy Shevchenko
In-Reply-To: <OS0PR01MB59225D8CF3E96808DD776A8E863BA@OS0PR01MB5922.jpnprd01.prod.outlook.com>
On Mon, Jul 17, 2023 at 06:45:27PM +0000, Biju Das wrote:
> Hi Dmitry,
>
> > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
> >
> > On Mon, Jul 17, 2023 at 07:15:50PM +0100, Mark Brown wrote:
> > > On Mon, Jul 17, 2023 at 04:35:02PM +0000, Biju Das wrote:
> > >
> > > > The .device_get_match_data callbacks are missing for I2C and SPI bus
> > subsystems.
> > > > Can you please throw some lights on this?
> > >
> > > It's the first time I've ever heard of that callback, I don't know why
> > > whoever added it wouldn't have done those buses in particular or if it
> > > just didn't happen. Try adding it and if it works send the patches?
> >
> > I think there is a disconnect. Right now device_get_match_data callbacks
> > are part of fwnode_operations. I was proposing to add another optional
> > device_get_match_data callback to 'struct bus_type' to allow individual
> > buses control how match data is handled, before (or after) jumping into
> > the fwnode-backed device_get_match_data callbacks.
>
> That is what implemented here [1] and [2] right?
>
> [1] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/i2c/i2c-core-base.c#L117
> [2] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/spi/spi.c#L364
>
> First it check for fwnode-backed device_get_match_data callbacks and
> Fallback is bus-type based match.
>
> Looks like you are proposing to unify [1] and [2] and you want the
> logic to be other way around. ie, first bus-type match, then
> fwnode-backed callbacks?
>
I do not have a strong preference for the ordering, i.e. I think it is
perfectly fine to do the generic fwnode-based lookup and if there is no
match have bus method called as a fallback, but I do not want driver
writers to learn about multiple <bus-prefix>_get_match_data()
implementations, I would prefer if they could call
device_get_match_data() and the right thing happened in all cases.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: pm8xxx-vib - Add support for more PMICs
From: Dmitry Baryshkov @ 2023-07-18 11:04 UTC (permalink / raw)
To: Fenglin Wu
Cc: linux-arm-msm, linux-kernel, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Dmitry Torokhov, linux-input, quic_collinsd,
quic_subbaram, quic_kamalw, jestar, quic_huliu
In-Reply-To: <501bc7d0-1f97-1c53-a7f8-701ab9dfdf8e@quicinc.com>
On Tue, 18 Jul 2023 at 13:55, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>
>
>
> On 7/18/2023 5:41 PM, Dmitry Baryshkov wrote:
> > On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
> >>> On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
> >>>>
> >>>> Add support for vibrator module inside PMI632, PM7250B, PM7325B.
> >>>> It is very similar to vibrator inside PM8xxx but just the drive
> >>>> amplitude is controlled through 2 bytes registers.
> >>>>
> >>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> >>>> ---
> >>>> drivers/input/misc/pm8xxx-vibrator.c | 48 ++++++++++++++++++++++++++++
> >>>> 1 file changed, 48 insertions(+)
> >>>>
> >>>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> >>>> index 04cb87efd799..213fdfd47c7f 100644
> >>>> --- a/drivers/input/misc/pm8xxx-vibrator.c
> >>>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> >>>> @@ -25,6 +25,9 @@ struct pm8xxx_regs {
> >>>> unsigned int drv_addr;
> >>>> unsigned int drv_mask;
> >>>> unsigned int drv_shift;
> >>>> + unsigned int drv_addr2;
> >>>> + unsigned int drv_mask2;
> >>>> + unsigned int drv_shift2;
> >>>> unsigned int drv_en_manual_mask;
> >>>> };
> >>>>
> >>>> @@ -44,6 +47,42 @@ static struct pm8xxx_regs pm8916_regs = {
> >>>> .drv_en_manual_mask = 0,
> >>>> };
> >>>>
> >>>> +static struct pm8xxx_regs pmi632_regs = {
> >>>> + .enable_addr = 0x5746,
> >>>> + .enable_mask = BIT(7),
> >>>> + .drv_addr = 0x5740,
> >>>> + .drv_mask = 0xff,
> >>>> + .drv_shift = 0,
> >>>> + .drv_addr2 = 0x5741,
> >>>> + .drv_mask2 = 0x0f,
> >>>> + .drv_shift2 = 8,
> >>>
> >>> I see that you are just expanding what was done for SSBI PMICs and
> >>> later expanded to support pm8916. However it might be better to drop
> >>> the hardcoded .drv_addr (and drv_addr2) and read address from DT
> >>> instead.
> >>>
> >>
> >> Right, this is the simplest change without updating the code logic too
> >> much. If we decided to read .drv_addr and .drv_add2 from DT, we will
> >> have to read .enable_addr along with all other mask/shift for each
> >> register address from DT as well because they are not consistent from
> >> target to target. I don't know how would you suggest to add the DT
> >> properties for all of them, but if we end up to add a property for each
> >> of them, it won't be cleaner than hard-coding them.
> >
> > No, we (correctly) have device compatibles for that. The issue with
> > hardcoding register addresses is that it adds extra issues here.
> >
> > If I understand correctly, we have several 'generation':
> > - SSBI PMIC, shifted 5-bit mask, en_manual_mask, no enable_register.
> > - older SPMI PMIC, 5 bit drv_mask, 0 en_manual_mask, enable register at +6
> > - new SPMI PMIC, 12 bit drv_mask, 0 en_manual_mask, enable register at +6
> >
> > For the last generation you are adding three independent entries,
> > while the block looks the same. If you remove drv_addr (and get it
> > from reg property), it would allow us to keep only the functional data
> > in struct pm8xxxx_regs (masks / shifts).
> >
>
> Okay, let me know if I understood it correctly, this is what you are
> suggesting:
>
> - hard code the mask/shifts and still keep them in struct pm8xxx_regs,
> combine the drv_mask2 to the upper byte of the drv_mask, so we will
> have following data structure for the 3rd generation vibrator
>
> static struct pm8xxx_regs pm7250b_regs = {
> .enable_addr = 0x5346,
> .enable_mask = BIT(7),
> .drv_mask = 0xfff,
> .drv_shift = 0,
> .drv_en_manual_mask = 0,
> };
>
>
> - move the drv_addr/drv_addr2 into DT, read them from 'reg' property.
> Because of 'mfd/qcom,spmi-pmic.yaml' has defined the 'address-cells'
> as 1 and the 'size-cells' as 0 for qcom spmi devices, we couldn't
> specify the address size to 2 even the drv_addr for the 3rd
> generation vibrator is 2 adjacent bytes. So we will end of having
> following DT scheme:
>
> For the 2nd generation which only has drv_addr
> vibrator@c041 {
> compatible = "qcom,pm8916-vib";
> reg = <0xc041>; /* drv_addr */
No. This is <0xc000>.
> ...
> };
>
> For the 3rd generation which has both drv_addr and drv_addr2
> vibrator@5340 {
> compatible = "qcom,pm7250b-vib";
> reg = <0x5340>, /* drv_addr */
> <0x5341>; /* drv_addr2 */
> ...
> };
>
> Not sure how do you feel, I actually don't see too much benefit than
> hard-coding them in the driver.
> We will end up having code to check how many u32 value in the 'reg' and
> only assign it to drv_addr2 when the 2nd is available, also when
> programming drv_addr2 register, the driver will always assume the mask
> is in the upper byte of the drv_mask and the shift to the drive level is
> 8 (this seems hacky to me and it was my biggest concern while I made
> this change, and it led me to defining drv_shift2/drv_mask2 along with
> drv_addr2).
We only need drv_addr2 if drv_mask has more than 8 bits. So you don't
have to specify it in the DT. It is always equal to base_reg + 0x41.
The same way drv_addr is always equal to base_reg + 0x40 for all
SPMI-based PMIC vibrator devices.
>
>
>
> >>
> >>
> >>>> + .drv_en_manual_mask = 0,
> >>>> +};
> >>>> +
> >>>> +static struct pm8xxx_regs pm7250b_regs = {
> >>>> + .enable_addr = 0x5346,
> >>>> + .enable_mask = BIT(7),
> >>>> + .drv_addr = 0x5340,
> >>>> + .drv_mask = 0xff,
> >>>> + .drv_shift = 0,
> >>>> + .drv_addr2 = 0x5341,
> >>>> + .drv_mask2 = 0x0f,
> >>>> + .drv_shift2 = 8,
> >>>> + .drv_en_manual_mask = 0,
> >>>> +};
> >>>> +
> >>>> +static struct pm8xxx_regs pm7325b_regs = {
> >>>> + .enable_addr = 0xdf46,
> >>>> + .enable_mask = BIT(7),
> >>>> + .drv_addr = 0xdf40,
> >>>> + .drv_mask = 0xff,
> >>>> + .drv_shift = 0,
> >>>> + .drv_addr2 = 0xdf41,
> >>>> + .drv_mask2 = 0x0f,
> >>>> + .drv_shift2 = 8,
> >>>> + .drv_en_manual_mask = 0,
> >>>> +};
> >>>> +
> >>>> /**
> >>>> * struct pm8xxx_vib - structure to hold vibrator data
> >>>> * @vib_input_dev: input device supporting force feedback
> >>>> @@ -87,6 +126,12 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
> >>>> return rc;
> >>>>
> >>>> vib->reg_vib_drv = val;
> >>>> + if (regs->drv_addr2 != 0 && on) {
> >>>> + val = (vib->level << regs->drv_shift2) & regs->drv_mask2;
> >>>> + rc = regmap_write(vib->regmap, regs->drv_addr2, val);
> >>>> + if (rc < 0)
> >>>> + return rc;
> >>>> + }
> >>>>
> >>>> if (regs->enable_mask)
> >>>> rc = regmap_update_bits(vib->regmap, regs->enable_addr,
> >>>> @@ -242,6 +287,9 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
> >>>> { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
> >>>> { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
> >>>> { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
> >>>> + { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
> >>>> + { .compatible = "qcom,pm7250b-vib", .data = &pm7250b_regs },
> >>>> + { .compatible = "qcom,pm7325b-vib", .data = &pm7325b_regs },
> >>>> { }
> >>>> };
> >>>> MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
> >>>> --
> >>>> 2.25.1
> >>>>
> >>>
> >>>
> >
> >
> >
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v4 11/18] media: Remove flag FBINFO_FLAG_DEFAULT from fbdev drivers
From: Hans Verkuil @ 2023-07-18 10:58 UTC (permalink / raw)
To: Thomas Zimmermann, deller, javierm, geert, dan.carpenter
Cc: linux-sh, dri-devel, linux-kernel, amd-gfx, linux-input,
linux-media, linux-fbdev, linux-staging, linux-arm-kernel,
linux-geode, linux-hyperv, linux-omap, linuxppc-dev, kvm,
Sam Ravnborg, Andy Walls, Mauro Carvalho Chehab
In-Reply-To: <20230715185343.7193-12-tzimmermann@suse.de>
Hi Thomas,
On 15/07/2023 20:51, Thomas Zimmermann wrote:
> The flag FBINFO_FLAG_DEFAULT is 0 and has no effect, as struct
> fbinfo.flags has been allocated to zero by kzalloc(). So do not
> set it.
>
> Flags should signal differences from the default values. After cleaning
> up all occurrences of FBINFO_DEFAULT, the token will be removed.
>
> v2:
> * fix commit message (Miguel)
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Andy Walls <awalls@md.metrocast.net>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> ---
> drivers/media/pci/ivtv/ivtvfb.c | 1 -
> drivers/media/test-drivers/vivid/vivid-osd.c | 1 -
> 2 files changed, 2 deletions(-)
I can take this patches for 6.6, unless you prefer to have this whole series
merged in one go?
In that case you can use my:
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Regards,
Hans
>
> diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
> index 0aeb9daaee4c..23c8c094e791 100644
> --- a/drivers/media/pci/ivtv/ivtvfb.c
> +++ b/drivers/media/pci/ivtv/ivtvfb.c
> @@ -1048,7 +1048,6 @@ static int ivtvfb_init_vidmode(struct ivtv *itv)
> /* Generate valid fb_info */
>
> oi->ivtvfb_info.node = -1;
> - oi->ivtvfb_info.flags = FBINFO_FLAG_DEFAULT;
> oi->ivtvfb_info.par = itv;
> oi->ivtvfb_info.var = oi->ivtvfb_defined;
> oi->ivtvfb_info.fix = oi->ivtvfb_fix;
> diff --git a/drivers/media/test-drivers/vivid/vivid-osd.c b/drivers/media/test-drivers/vivid/vivid-osd.c
> index ec25edc679b3..051f1805a16d 100644
> --- a/drivers/media/test-drivers/vivid/vivid-osd.c
> +++ b/drivers/media/test-drivers/vivid/vivid-osd.c
> @@ -310,7 +310,6 @@ static int vivid_fb_init_vidmode(struct vivid_dev *dev)
> /* Generate valid fb_info */
>
> dev->fb_info.node = -1;
> - dev->fb_info.flags = FBINFO_FLAG_DEFAULT;
> dev->fb_info.par = dev;
> dev->fb_info.var = dev->fb_defined;
> dev->fb_info.fix = dev->fb_fix;
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: qcom,pm8xxx-vib: add more PMIC support
From: Fenglin Wu @ 2023-07-18 10:56 UTC (permalink / raw)
To: Konrad Dybcio, Krzysztof Kozlowski, linux-arm-msm, linux-kernel,
Andy Gross, Bjorn Andersson, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, linux-input, devicetree
Cc: quic_collinsd, quic_subbaram, quic_kamalw, jestar
In-Reply-To: <1bf484c6-b648-a14a-8bac-4b95b3776294@linaro.org>
On 7/18/2023 6:51 PM, Konrad Dybcio wrote:
> On 18.07.2023 08:38, Fenglin Wu wrote:
>>
>>
>> On 7/18/2023 2:33 PM, Krzysztof Kozlowski wrote:
>>> On 18/07/2023 08:26, Fenglin Wu wrote:
>>>> Add support for vibrator module inside Qualcomm PMI632, PM7250B, PM7325B
>>>> PMICs.
>>>>
>>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>>> ---
>>>
>>> I don't see changelog. No changes then?
>>>
>> Sorry, I updated the change log in the cover letter which didn't seems to be sent to a wider audience, I will resend it by adding more receivers in the to list
> Please consider using the b4 tool which takes care of all that
>
> https://b4.docs.kernel.org/en/latest/index.html
>
Thanks Konrad, I will check and update at my side.
> Konrad
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: pm8xxx-vib - Add support for more PMICs
From: Fenglin Wu @ 2023-07-18 10:55 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: linux-arm-msm, linux-kernel, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Dmitry Torokhov, linux-input, quic_collinsd,
quic_subbaram, quic_kamalw, jestar, quic_huliu
In-Reply-To: <CAA8EJpr--XZnFJX96etagAa0uT0yNBcgZTfFDuu8gH2C5sgoCg@mail.gmail.com>
On 7/18/2023 5:41 PM, Dmitry Baryshkov wrote:
> On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>
>>
>>
>> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
>>> On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>>>
>>>> Add support for vibrator module inside PMI632, PM7250B, PM7325B.
>>>> It is very similar to vibrator inside PM8xxx but just the drive
>>>> amplitude is controlled through 2 bytes registers.
>>>>
>>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>>> ---
>>>> drivers/input/misc/pm8xxx-vibrator.c | 48 ++++++++++++++++++++++++++++
>>>> 1 file changed, 48 insertions(+)
>>>>
>>>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
>>>> index 04cb87efd799..213fdfd47c7f 100644
>>>> --- a/drivers/input/misc/pm8xxx-vibrator.c
>>>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
>>>> @@ -25,6 +25,9 @@ struct pm8xxx_regs {
>>>> unsigned int drv_addr;
>>>> unsigned int drv_mask;
>>>> unsigned int drv_shift;
>>>> + unsigned int drv_addr2;
>>>> + unsigned int drv_mask2;
>>>> + unsigned int drv_shift2;
>>>> unsigned int drv_en_manual_mask;
>>>> };
>>>>
>>>> @@ -44,6 +47,42 @@ static struct pm8xxx_regs pm8916_regs = {
>>>> .drv_en_manual_mask = 0,
>>>> };
>>>>
>>>> +static struct pm8xxx_regs pmi632_regs = {
>>>> + .enable_addr = 0x5746,
>>>> + .enable_mask = BIT(7),
>>>> + .drv_addr = 0x5740,
>>>> + .drv_mask = 0xff,
>>>> + .drv_shift = 0,
>>>> + .drv_addr2 = 0x5741,
>>>> + .drv_mask2 = 0x0f,
>>>> + .drv_shift2 = 8,
>>>
>>> I see that you are just expanding what was done for SSBI PMICs and
>>> later expanded to support pm8916. However it might be better to drop
>>> the hardcoded .drv_addr (and drv_addr2) and read address from DT
>>> instead.
>>>
>>
>> Right, this is the simplest change without updating the code logic too
>> much. If we decided to read .drv_addr and .drv_add2 from DT, we will
>> have to read .enable_addr along with all other mask/shift for each
>> register address from DT as well because they are not consistent from
>> target to target. I don't know how would you suggest to add the DT
>> properties for all of them, but if we end up to add a property for each
>> of them, it won't be cleaner than hard-coding them.
>
> No, we (correctly) have device compatibles for that. The issue with
> hardcoding register addresses is that it adds extra issues here.
>
> If I understand correctly, we have several 'generation':
> - SSBI PMIC, shifted 5-bit mask, en_manual_mask, no enable_register.
> - older SPMI PMIC, 5 bit drv_mask, 0 en_manual_mask, enable register at +6
> - new SPMI PMIC, 12 bit drv_mask, 0 en_manual_mask, enable register at +6
>
> For the last generation you are adding three independent entries,
> while the block looks the same. If you remove drv_addr (and get it
> from reg property), it would allow us to keep only the functional data
> in struct pm8xxxx_regs (masks / shifts).
>
Okay, let me know if I understood it correctly, this is what you are
suggesting:
- hard code the mask/shifts and still keep them in struct pm8xxx_regs,
combine the drv_mask2 to the upper byte of the drv_mask, so we will
have following data structure for the 3rd generation vibrator
static struct pm8xxx_regs pm7250b_regs = {
.enable_addr = 0x5346,
.enable_mask = BIT(7),
.drv_mask = 0xfff,
.drv_shift = 0,
.drv_en_manual_mask = 0,
};
- move the drv_addr/drv_addr2 into DT, read them from 'reg' property.
Because of 'mfd/qcom,spmi-pmic.yaml' has defined the 'address-cells'
as 1 and the 'size-cells' as 0 for qcom spmi devices, we couldn't
specify the address size to 2 even the drv_addr for the 3rd
generation vibrator is 2 adjacent bytes. So we will end of having
following DT scheme:
For the 2nd generation which only has drv_addr
vibrator@c041 {
compatible = "qcom,pm8916-vib";
reg = <0xc041>; /* drv_addr */
...
};
For the 3rd generation which has both drv_addr and drv_addr2
vibrator@5340 {
compatible = "qcom,pm7250b-vib";
reg = <0x5340>, /* drv_addr */
<0x5341>; /* drv_addr2 */
...
};
Not sure how do you feel, I actually don't see too much benefit than
hard-coding them in the driver.
We will end up having code to check how many u32 value in the 'reg' and
only assign it to drv_addr2 when the 2nd is available, also when
programming drv_addr2 register, the driver will always assume the mask
is in the upper byte of the drv_mask and the shift to the drive level is
8 (this seems hacky to me and it was my biggest concern while I made
this change, and it led me to defining drv_shift2/drv_mask2 along with
drv_addr2).
>>
>>
>>>> + .drv_en_manual_mask = 0,
>>>> +};
>>>> +
>>>> +static struct pm8xxx_regs pm7250b_regs = {
>>>> + .enable_addr = 0x5346,
>>>> + .enable_mask = BIT(7),
>>>> + .drv_addr = 0x5340,
>>>> + .drv_mask = 0xff,
>>>> + .drv_shift = 0,
>>>> + .drv_addr2 = 0x5341,
>>>> + .drv_mask2 = 0x0f,
>>>> + .drv_shift2 = 8,
>>>> + .drv_en_manual_mask = 0,
>>>> +};
>>>> +
>>>> +static struct pm8xxx_regs pm7325b_regs = {
>>>> + .enable_addr = 0xdf46,
>>>> + .enable_mask = BIT(7),
>>>> + .drv_addr = 0xdf40,
>>>> + .drv_mask = 0xff,
>>>> + .drv_shift = 0,
>>>> + .drv_addr2 = 0xdf41,
>>>> + .drv_mask2 = 0x0f,
>>>> + .drv_shift2 = 8,
>>>> + .drv_en_manual_mask = 0,
>>>> +};
>>>> +
>>>> /**
>>>> * struct pm8xxx_vib - structure to hold vibrator data
>>>> * @vib_input_dev: input device supporting force feedback
>>>> @@ -87,6 +126,12 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
>>>> return rc;
>>>>
>>>> vib->reg_vib_drv = val;
>>>> + if (regs->drv_addr2 != 0 && on) {
>>>> + val = (vib->level << regs->drv_shift2) & regs->drv_mask2;
>>>> + rc = regmap_write(vib->regmap, regs->drv_addr2, val);
>>>> + if (rc < 0)
>>>> + return rc;
>>>> + }
>>>>
>>>> if (regs->enable_mask)
>>>> rc = regmap_update_bits(vib->regmap, regs->enable_addr,
>>>> @@ -242,6 +287,9 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
>>>> { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
>>>> { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
>>>> { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
>>>> + { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
>>>> + { .compatible = "qcom,pm7250b-vib", .data = &pm7250b_regs },
>>>> + { .compatible = "qcom,pm7325b-vib", .data = &pm7325b_regs },
>>>> { }
>>>> };
>>>> MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
>>>> --
>>>> 2.25.1
>>>>
>>>
>>>
>
>
>
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: qcom,pm8xxx-vib: add more PMIC support
From: Konrad Dybcio @ 2023-07-18 10:51 UTC (permalink / raw)
To: Fenglin Wu, Krzysztof Kozlowski, linux-arm-msm, linux-kernel,
Andy Gross, Bjorn Andersson, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, linux-input, devicetree
Cc: quic_collinsd, quic_subbaram, quic_kamalw, jestar
In-Reply-To: <4cb9f443-bdea-695a-f1b7-3963747e9a17@quicinc.com>
On 18.07.2023 08:38, Fenglin Wu wrote:
>
>
> On 7/18/2023 2:33 PM, Krzysztof Kozlowski wrote:
>> On 18/07/2023 08:26, Fenglin Wu wrote:
>>> Add support for vibrator module inside Qualcomm PMI632, PM7250B, PM7325B
>>> PMICs.
>>>
>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>> ---
>>
>> I don't see changelog. No changes then?
>>
> Sorry, I updated the change log in the cover letter which didn't seems to be sent to a wider audience, I will resend it by adding more receivers in the to list
Please consider using the b4 tool which takes care of all that
https://b4.docs.kernel.org/en/latest/index.html
Konrad
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: pm8xxx-vib - Add support for more PMICs
From: Dmitry Baryshkov @ 2023-07-18 9:41 UTC (permalink / raw)
To: Fenglin Wu
Cc: linux-arm-msm, linux-kernel, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Dmitry Torokhov, linux-input, quic_collinsd,
quic_subbaram, quic_kamalw, jestar
In-Reply-To: <86631cb4-6b99-c3a7-428e-4534891da4c7@quicinc.com>
On Tue, 18 Jul 2023 at 09:58, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>
>
>
> On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
> > On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
> >>
> >> Add support for vibrator module inside PMI632, PM7250B, PM7325B.
> >> It is very similar to vibrator inside PM8xxx but just the drive
> >> amplitude is controlled through 2 bytes registers.
> >>
> >> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> >> ---
> >> drivers/input/misc/pm8xxx-vibrator.c | 48 ++++++++++++++++++++++++++++
> >> 1 file changed, 48 insertions(+)
> >>
> >> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> >> index 04cb87efd799..213fdfd47c7f 100644
> >> --- a/drivers/input/misc/pm8xxx-vibrator.c
> >> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> >> @@ -25,6 +25,9 @@ struct pm8xxx_regs {
> >> unsigned int drv_addr;
> >> unsigned int drv_mask;
> >> unsigned int drv_shift;
> >> + unsigned int drv_addr2;
> >> + unsigned int drv_mask2;
> >> + unsigned int drv_shift2;
> >> unsigned int drv_en_manual_mask;
> >> };
> >>
> >> @@ -44,6 +47,42 @@ static struct pm8xxx_regs pm8916_regs = {
> >> .drv_en_manual_mask = 0,
> >> };
> >>
> >> +static struct pm8xxx_regs pmi632_regs = {
> >> + .enable_addr = 0x5746,
> >> + .enable_mask = BIT(7),
> >> + .drv_addr = 0x5740,
> >> + .drv_mask = 0xff,
> >> + .drv_shift = 0,
> >> + .drv_addr2 = 0x5741,
> >> + .drv_mask2 = 0x0f,
> >> + .drv_shift2 = 8,
> >
> > I see that you are just expanding what was done for SSBI PMICs and
> > later expanded to support pm8916. However it might be better to drop
> > the hardcoded .drv_addr (and drv_addr2) and read address from DT
> > instead.
> >
>
> Right, this is the simplest change without updating the code logic too
> much. If we decided to read .drv_addr and .drv_add2 from DT, we will
> have to read .enable_addr along with all other mask/shift for each
> register address from DT as well because they are not consistent from
> target to target. I don't know how would you suggest to add the DT
> properties for all of them, but if we end up to add a property for each
> of them, it won't be cleaner than hard-coding them.
No, we (correctly) have device compatibles for that. The issue with
hardcoding register addresses is that it adds extra issues here.
If I understand correctly, we have several 'generation':
- SSBI PMIC, shifted 5-bit mask, en_manual_mask, no enable_register.
- older SPMI PMIC, 5 bit drv_mask, 0 en_manual_mask, enable register at +6
- new SPMI PMIC, 12 bit drv_mask, 0 en_manual_mask, enable register at +6
For the last generation you are adding three independent entries,
while the block looks the same. If you remove drv_addr (and get it
from reg property), it would allow us to keep only the functional data
in struct pm8xxxx_regs (masks / shifts).
>
>
> >> + .drv_en_manual_mask = 0,
> >> +};
> >> +
> >> +static struct pm8xxx_regs pm7250b_regs = {
> >> + .enable_addr = 0x5346,
> >> + .enable_mask = BIT(7),
> >> + .drv_addr = 0x5340,
> >> + .drv_mask = 0xff,
> >> + .drv_shift = 0,
> >> + .drv_addr2 = 0x5341,
> >> + .drv_mask2 = 0x0f,
> >> + .drv_shift2 = 8,
> >> + .drv_en_manual_mask = 0,
> >> +};
> >> +
> >> +static struct pm8xxx_regs pm7325b_regs = {
> >> + .enable_addr = 0xdf46,
> >> + .enable_mask = BIT(7),
> >> + .drv_addr = 0xdf40,
> >> + .drv_mask = 0xff,
> >> + .drv_shift = 0,
> >> + .drv_addr2 = 0xdf41,
> >> + .drv_mask2 = 0x0f,
> >> + .drv_shift2 = 8,
> >> + .drv_en_manual_mask = 0,
> >> +};
> >> +
> >> /**
> >> * struct pm8xxx_vib - structure to hold vibrator data
> >> * @vib_input_dev: input device supporting force feedback
> >> @@ -87,6 +126,12 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
> >> return rc;
> >>
> >> vib->reg_vib_drv = val;
> >> + if (regs->drv_addr2 != 0 && on) {
> >> + val = (vib->level << regs->drv_shift2) & regs->drv_mask2;
> >> + rc = regmap_write(vib->regmap, regs->drv_addr2, val);
> >> + if (rc < 0)
> >> + return rc;
> >> + }
> >>
> >> if (regs->enable_mask)
> >> rc = regmap_update_bits(vib->regmap, regs->enable_addr,
> >> @@ -242,6 +287,9 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
> >> { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
> >> { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
> >> { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
> >> + { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
> >> + { .compatible = "qcom,pm7250b-vib", .data = &pm7250b_regs },
> >> + { .compatible = "qcom,pm7325b-vib", .data = &pm7325b_regs },
> >> { }
> >> };
> >> MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
> >> --
> >> 2.25.1
> >>
> >
> >
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: qcom,pm8xxx-vib: add more PMIC support
From: Krzysztof Kozlowski @ 2023-07-18 8:02 UTC (permalink / raw)
To: Fenglin Wu, linux-arm-msm, linux-kernel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, linux-input, devicetree
Cc: quic_collinsd, quic_subbaram, quic_kamalw, jestar, quic_huliu
In-Reply-To: <4210b137-2d5d-a467-ea8c-d047701fdcc2@quicinc.com>
On 18/07/2023 09:59, Fenglin Wu wrote:
>>> Just FYI,the change log was updated in the cover letter here:
>>> https://lore.kernel.org/linux-arm-msm/20230718062639.2339589-1-quic_fenglinw@quicinc.com/T/#m3819b50503ef19e0933a10bf797351a4af35537f
>>>
>>> Also the commit text and the driver change were also updated accordingly
>>> to address your review comment by removing 'pm7550ba-vib' compatible string.
>>
>> Removing compatible was never my feedback. Did you read:
>> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
>> ?
>>
> Okay, so do you want me to add 'pm7550ba-vib' as a fallback compatible
> like this?
>
> properties:
> compatible:
> - enum:
> - - qcom,pm8058-vib
> - - qcom,pm8916-vib
> - - qcom,pm8921-vib
> - - qcom,pmi632-vib
> - - qcom,pm7250b-vib
> - - qcom,pm7325b-vib
> + oneOf:
> + - enum:
> + - qcom,pm8058-vib
> + - qcom,pm8916-vib
> + - qcom,pm8921-vib
> + - qcom,pmi632-vib
> + - qcom,pm7250b-vib
> + - qcom,pm7325b-vib
> + - items:
> + - enum:
> + - qcom,pm7550ba-vib
> + - const: qcom,pm7325b-vib
>
Yes
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: qcom,pm8xxx-vib: add more PMIC support
From: Fenglin Wu @ 2023-07-18 7:59 UTC (permalink / raw)
To: Krzysztof Kozlowski, linux-arm-msm, linux-kernel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, linux-input, devicetree
Cc: quic_collinsd, quic_subbaram, quic_kamalw, jestar, quic_huliu
In-Reply-To: <fec38f3a-f103-ff0f-138c-cffa3a808001@linaro.org>
On 7/18/2023 3:20 PM, Krzysztof Kozlowski wrote:
> On 18/07/2023 09:06, Fenglin Wu wrote:
>>
>>
>> On 7/18/2023 2:38 PM, Fenglin Wu wrote:
>>>
>>>
>>> On 7/18/2023 2:33 PM, Krzysztof Kozlowski wrote:
>>>> On 18/07/2023 08:26, Fenglin Wu wrote:
>>>>> Add support for vibrator module inside Qualcomm PMI632, PM7250B, PM7325B
>>>>> PMICs.
>>>>>
>>>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>>>> ---
>>>>
>>>> I don't see changelog. No changes then?
>>>>
>>> Sorry, I updated the change log in the cover letter which didn't seems
>>> to be sent to a wider audience, I will resend it by adding more
>>> receivers in the to list
>>>
>>> Fenglin
>>
>> Just FYI,the change log was updated in the cover letter here:
>> https://lore.kernel.org/linux-arm-msm/20230718062639.2339589-1-quic_fenglinw@quicinc.com/T/#m3819b50503ef19e0933a10bf797351a4af35537f
>>
>> Also the commit text and the driver change were also updated accordingly
>> to address your review comment by removing 'pm7550ba-vib' compatible string.
>
> Removing compatible was never my feedback. Did you read:
> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
> ?
>
Okay, so do you want me to add 'pm7550ba-vib' as a fallback compatible
like this?
properties:
compatible:
- enum:
- - qcom,pm8058-vib
- - qcom,pm8916-vib
- - qcom,pm8921-vib
- - qcom,pmi632-vib
- - qcom,pm7250b-vib
- - qcom,pm7325b-vib
+ oneOf:
+ - enum:
+ - qcom,pm8058-vib
+ - qcom,pm8916-vib
+ - qcom,pm8921-vib
+ - qcom,pmi632-vib
+ - qcom,pm7250b-vib
+ - qcom,pm7325b-vib
+ - items:
+ - enum:
+ - qcom,pm7550ba-vib
+ - const: qcom,pm7325b-vib
> Best regards,
> Krzysztof
>
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: qcom,pm8xxx-vib: add more PMIC support
From: Krzysztof Kozlowski @ 2023-07-18 7:20 UTC (permalink / raw)
To: Fenglin Wu, linux-arm-msm, linux-kernel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, linux-input, devicetree
Cc: quic_collinsd, quic_subbaram, quic_kamalw, jestar, quic_huliu
In-Reply-To: <5b7e624b-5d06-826d-92d1-2a721b7c83b7@quicinc.com>
On 18/07/2023 09:06, Fenglin Wu wrote:
>
>
> On 7/18/2023 2:38 PM, Fenglin Wu wrote:
>>
>>
>> On 7/18/2023 2:33 PM, Krzysztof Kozlowski wrote:
>>> On 18/07/2023 08:26, Fenglin Wu wrote:
>>>> Add support for vibrator module inside Qualcomm PMI632, PM7250B, PM7325B
>>>> PMICs.
>>>>
>>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>>> ---
>>>
>>> I don't see changelog. No changes then?
>>>
>> Sorry, I updated the change log in the cover letter which didn't seems
>> to be sent to a wider audience, I will resend it by adding more
>> receivers in the to list
>>
>> Fenglin
>
> Just FYI,the change log was updated in the cover letter here:
> https://lore.kernel.org/linux-arm-msm/20230718062639.2339589-1-quic_fenglinw@quicinc.com/T/#m3819b50503ef19e0933a10bf797351a4af35537f
>
> Also the commit text and the driver change were also updated accordingly
> to address your review comment by removing 'pm7550ba-vib' compatible string.
Removing compatible was never my feedback. Did you read:
https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: qcom,pm8xxx-vib: add more PMIC support
From: Fenglin Wu @ 2023-07-18 7:06 UTC (permalink / raw)
To: Krzysztof Kozlowski, linux-arm-msm, linux-kernel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, linux-input, devicetree
Cc: quic_collinsd, quic_subbaram, quic_kamalw, jestar, quic_huliu
In-Reply-To: <4cb9f443-bdea-695a-f1b7-3963747e9a17@quicinc.com>
On 7/18/2023 2:38 PM, Fenglin Wu wrote:
>
>
> On 7/18/2023 2:33 PM, Krzysztof Kozlowski wrote:
>> On 18/07/2023 08:26, Fenglin Wu wrote:
>>> Add support for vibrator module inside Qualcomm PMI632, PM7250B, PM7325B
>>> PMICs.
>>>
>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>> ---
>>
>> I don't see changelog. No changes then?
>>
> Sorry, I updated the change log in the cover letter which didn't seems
> to be sent to a wider audience, I will resend it by adding more
> receivers in the to list
>
> Fenglin
Just FYI,the change log was updated in the cover letter here:
https://lore.kernel.org/linux-arm-msm/20230718062639.2339589-1-quic_fenglinw@quicinc.com/T/#m3819b50503ef19e0933a10bf797351a4af35537f
Also the commit text and the driver change were also updated accordingly
to address your review comment by removing 'pm7550ba-vib' compatible string.
Since the changes are receiving review comments, I will not resend it. I
will add a larger to-list when pushing the next patchset.
>>> Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
>>> b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
>>> index c8832cd0d7da..481163105d24 100644
>>> --- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
>>> +++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
>>> @@ -15,6 +15,9 @@ properties:
>>> - qcom,pm8058-vib
>>> - qcom,pm8916-vib
>>> - qcom,pm8921-vib
>>> + - qcom,pmi632-vib
>>> + - qcom,pm7250b-vib
>>> + - qcom,pm7325b-vib
>>
>> Not much improved. With missing changelog, it seems you ignored the
>> feedback.
>>
>>
>> Best regards,
>> Krzysztof
>>
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: pm8xxx-vib - Add support for more PMICs
From: Fenglin Wu @ 2023-07-18 6:58 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: linux-arm-msm, linux-kernel, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Dmitry Torokhov, linux-input, quic_collinsd,
quic_subbaram, quic_kamalw, jestar
In-Reply-To: <CAA8EJpq_DGu3+kc_ex_2MUyiQFJvZhbWOc7NO6x1pO1Wx4RbNw@mail.gmail.com>
On 7/18/2023 2:44 PM, Dmitry Baryshkov wrote:
> On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>
>> Add support for vibrator module inside PMI632, PM7250B, PM7325B.
>> It is very similar to vibrator inside PM8xxx but just the drive
>> amplitude is controlled through 2 bytes registers.
>>
>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>> ---
>> drivers/input/misc/pm8xxx-vibrator.c | 48 ++++++++++++++++++++++++++++
>> 1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
>> index 04cb87efd799..213fdfd47c7f 100644
>> --- a/drivers/input/misc/pm8xxx-vibrator.c
>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
>> @@ -25,6 +25,9 @@ struct pm8xxx_regs {
>> unsigned int drv_addr;
>> unsigned int drv_mask;
>> unsigned int drv_shift;
>> + unsigned int drv_addr2;
>> + unsigned int drv_mask2;
>> + unsigned int drv_shift2;
>> unsigned int drv_en_manual_mask;
>> };
>>
>> @@ -44,6 +47,42 @@ static struct pm8xxx_regs pm8916_regs = {
>> .drv_en_manual_mask = 0,
>> };
>>
>> +static struct pm8xxx_regs pmi632_regs = {
>> + .enable_addr = 0x5746,
>> + .enable_mask = BIT(7),
>> + .drv_addr = 0x5740,
>> + .drv_mask = 0xff,
>> + .drv_shift = 0,
>> + .drv_addr2 = 0x5741,
>> + .drv_mask2 = 0x0f,
>> + .drv_shift2 = 8,
>
> I see that you are just expanding what was done for SSBI PMICs and
> later expanded to support pm8916. However it might be better to drop
> the hardcoded .drv_addr (and drv_addr2) and read address from DT
> instead.
>
Right, this is the simplest change without updating the code logic too
much. If we decided to read .drv_addr and .drv_add2 from DT, we will
have to read .enable_addr along with all other mask/shift for each
register address from DT as well because they are not consistent from
target to target. I don't know how would you suggest to add the DT
properties for all of them, but if we end up to add a property for each
of them, it won't be cleaner than hard-coding them.
>> + .drv_en_manual_mask = 0,
>> +};
>> +
>> +static struct pm8xxx_regs pm7250b_regs = {
>> + .enable_addr = 0x5346,
>> + .enable_mask = BIT(7),
>> + .drv_addr = 0x5340,
>> + .drv_mask = 0xff,
>> + .drv_shift = 0,
>> + .drv_addr2 = 0x5341,
>> + .drv_mask2 = 0x0f,
>> + .drv_shift2 = 8,
>> + .drv_en_manual_mask = 0,
>> +};
>> +
>> +static struct pm8xxx_regs pm7325b_regs = {
>> + .enable_addr = 0xdf46,
>> + .enable_mask = BIT(7),
>> + .drv_addr = 0xdf40,
>> + .drv_mask = 0xff,
>> + .drv_shift = 0,
>> + .drv_addr2 = 0xdf41,
>> + .drv_mask2 = 0x0f,
>> + .drv_shift2 = 8,
>> + .drv_en_manual_mask = 0,
>> +};
>> +
>> /**
>> * struct pm8xxx_vib - structure to hold vibrator data
>> * @vib_input_dev: input device supporting force feedback
>> @@ -87,6 +126,12 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
>> return rc;
>>
>> vib->reg_vib_drv = val;
>> + if (regs->drv_addr2 != 0 && on) {
>> + val = (vib->level << regs->drv_shift2) & regs->drv_mask2;
>> + rc = regmap_write(vib->regmap, regs->drv_addr2, val);
>> + if (rc < 0)
>> + return rc;
>> + }
>>
>> if (regs->enable_mask)
>> rc = regmap_update_bits(vib->regmap, regs->enable_addr,
>> @@ -242,6 +287,9 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
>> { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
>> { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
>> { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
>> + { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
>> + { .compatible = "qcom,pm7250b-vib", .data = &pm7250b_regs },
>> + { .compatible = "qcom,pm7325b-vib", .data = &pm7325b_regs },
>> { }
>> };
>> MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
>> --
>> 2.25.1
>>
>
>
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: pm8xxx-vib - Add support for more PMICs
From: Dmitry Baryshkov @ 2023-07-18 6:44 UTC (permalink / raw)
To: Fenglin Wu
Cc: linux-arm-msm, linux-kernel, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Dmitry Torokhov, linux-input, quic_collinsd,
quic_subbaram, quic_kamalw, jestar
In-Reply-To: <20230718062639.2339589-3-quic_fenglinw@quicinc.com>
On Tue, 18 Jul 2023 at 09:27, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>
> Add support for vibrator module inside PMI632, PM7250B, PM7325B.
> It is very similar to vibrator inside PM8xxx but just the drive
> amplitude is controlled through 2 bytes registers.
>
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> ---
> drivers/input/misc/pm8xxx-vibrator.c | 48 ++++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> index 04cb87efd799..213fdfd47c7f 100644
> --- a/drivers/input/misc/pm8xxx-vibrator.c
> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> @@ -25,6 +25,9 @@ struct pm8xxx_regs {
> unsigned int drv_addr;
> unsigned int drv_mask;
> unsigned int drv_shift;
> + unsigned int drv_addr2;
> + unsigned int drv_mask2;
> + unsigned int drv_shift2;
> unsigned int drv_en_manual_mask;
> };
>
> @@ -44,6 +47,42 @@ static struct pm8xxx_regs pm8916_regs = {
> .drv_en_manual_mask = 0,
> };
>
> +static struct pm8xxx_regs pmi632_regs = {
> + .enable_addr = 0x5746,
> + .enable_mask = BIT(7),
> + .drv_addr = 0x5740,
> + .drv_mask = 0xff,
> + .drv_shift = 0,
> + .drv_addr2 = 0x5741,
> + .drv_mask2 = 0x0f,
> + .drv_shift2 = 8,
I see that you are just expanding what was done for SSBI PMICs and
later expanded to support pm8916. However it might be better to drop
the hardcoded .drv_addr (and drv_addr2) and read address from DT
instead.
> + .drv_en_manual_mask = 0,
> +};
> +
> +static struct pm8xxx_regs pm7250b_regs = {
> + .enable_addr = 0x5346,
> + .enable_mask = BIT(7),
> + .drv_addr = 0x5340,
> + .drv_mask = 0xff,
> + .drv_shift = 0,
> + .drv_addr2 = 0x5341,
> + .drv_mask2 = 0x0f,
> + .drv_shift2 = 8,
> + .drv_en_manual_mask = 0,
> +};
> +
> +static struct pm8xxx_regs pm7325b_regs = {
> + .enable_addr = 0xdf46,
> + .enable_mask = BIT(7),
> + .drv_addr = 0xdf40,
> + .drv_mask = 0xff,
> + .drv_shift = 0,
> + .drv_addr2 = 0xdf41,
> + .drv_mask2 = 0x0f,
> + .drv_shift2 = 8,
> + .drv_en_manual_mask = 0,
> +};
> +
> /**
> * struct pm8xxx_vib - structure to hold vibrator data
> * @vib_input_dev: input device supporting force feedback
> @@ -87,6 +126,12 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
> return rc;
>
> vib->reg_vib_drv = val;
> + if (regs->drv_addr2 != 0 && on) {
> + val = (vib->level << regs->drv_shift2) & regs->drv_mask2;
> + rc = regmap_write(vib->regmap, regs->drv_addr2, val);
> + if (rc < 0)
> + return rc;
> + }
>
> if (regs->enable_mask)
> rc = regmap_update_bits(vib->regmap, regs->enable_addr,
> @@ -242,6 +287,9 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
> { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
> { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
> { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
> + { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
> + { .compatible = "qcom,pm7250b-vib", .data = &pm7250b_regs },
> + { .compatible = "qcom,pm7325b-vib", .data = &pm7325b_regs },
> { }
> };
> MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
> --
> 2.25.1
>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: qcom,pm8xxx-vib: add more PMIC support
From: Fenglin Wu @ 2023-07-18 6:38 UTC (permalink / raw)
To: Krzysztof Kozlowski, linux-arm-msm, linux-kernel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, linux-input, devicetree
Cc: quic_collinsd, quic_subbaram, quic_kamalw, jestar
In-Reply-To: <cb534cdb-508e-b03e-4e39-50cd6654377a@linaro.org>
On 7/18/2023 2:33 PM, Krzysztof Kozlowski wrote:
> On 18/07/2023 08:26, Fenglin Wu wrote:
>> Add support for vibrator module inside Qualcomm PMI632, PM7250B, PM7325B
>> PMICs.
>>
>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>> ---
>
> I don't see changelog. No changes then?
>
Sorry, I updated the change log in the cover letter which didn't seems
to be sent to a wider audience, I will resend it by adding more
receivers in the to list
Fenglin
>> Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
>> index c8832cd0d7da..481163105d24 100644
>> --- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
>> +++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
>> @@ -15,6 +15,9 @@ properties:
>> - qcom,pm8058-vib
>> - qcom,pm8916-vib
>> - qcom,pm8921-vib
>> + - qcom,pmi632-vib
>> + - qcom,pm7250b-vib
>> + - qcom,pm7325b-vib
>
> Not much improved. With missing changelog, it seems you ignored the
> feedback.
>
>
> Best regards,
> Krzysztof
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox