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

[Markuss Broks <markuss.broks@gmail.com>]

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