Re: [PATCH 1/2] input: generalize the Imagis touchscreen driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Karel,

On 9/28/23 21:07, Karel Balej wrote:
Hello, Markuss,

thank you for your response and please forgive that my original emails
didn't reach you - I am trying to see if I can get the SMTP server I use
for my primary address officially authorized to send emails in its name
so that Google does not reject them.
Yeah, I have not received your first series, only knew of it when the replies came. It's fine though :)

To be fair, this driver should work with all Imagis3 chips, which could
be a better name for it. However, I agree with Jeff here: the driver
doesn't necessarily need renaming.
I will be sure to drop the renaming for v2.

Additionally, there used to be my series [1] that generalized the
driver, but it never got applied. I will re-send it. It introduces
`struct imagis_properties`, which contains register addresses for the
registers that we use to read the touch input. In your specific case, I
believe it should be:

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, /* change it to your whoami */
};
I have come across your patch series and in fact my original experiments
were based on it. However I thought it was abandoned and decided not to
try to make any further generalizations to the driver for now, except
making it work with IST3032C. Should I then perhaps wait before your
series gets applied before sending v2 of my changes? Or should I send
you a patch on top of your series, where I would add the `struct
imagis_properties` for the IST3032C (which you surely could do yourself,
but I can at least test it with my device). Please let me know if and
how we should coordinate.

If you don't mind the extra hassle, I'm all in for my generalization thing going together with your series.

Alternatively, I can resend it myself, but I believe it would be better if they go in bulk since they need to be applied together.


As for the voltage set, I believe this does not belong in a kernel
driver. You should set it in device-tree with `regulator-max-microvolt`
and `regulator-min-microvolt`.
Please see my response to Jeff regarding this. I will be happy to hear
your thoughts on what I propose.

Actually, the regulator values belong to the device-tree, because the device-tree for the board is what describes the board's regulators, and since you know what components are installed on that specific board you can know which regulator values are supposed to be set for it. This manual voltage setting can cause conflicts with other drivers for example. Also some device can use a variable wide voltage range, and some only specific narrow one, and e.g. the driver with wide range would set it to voltage that isn't suitable for the narrow range device, so it's much better to just specify it manually than have it resolved.

The actual min/max values for regulators or its voltage table is provided by the regulator driver itself, so there's not much point in specifying those again in the device-tree.


Kind regards,
K. B.
- Markuss



[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux