Re: [PATCH v3 3/3] Input: Add TouchNetix axiom i2c touchscreen driver

[Marco Felsch]

Hi Dimitry,

On 23-10-30, Dmitry Torokhov wrote:
> On Sun, Oct 22, 2023 at 09:35:29PM +0200, Kamel Bouhara wrote:
> > On Fri, Oct 20, 2023 at 02:03:10PM +0200, Marco Felsch wrote:
> > > > +
> > > > +static int
> > > > +axiom_i2c_write(struct i2c_client *client, u8 usage, u8 page, u8 *buf, u16 len)
> > > > +{
> > > > +	struct axiom_data *ts = i2c_get_clientdata(client);
> > > > +	struct axiom_cmd_header cmd_header;
> > > > +	struct i2c_msg msg[2];
> > > > +	int ret;
> > > > +
> > > > +	cmd_header.target_address = cpu_to_le16(usage_to_target_address(ts, usage, page, 0));
> > > > +	cmd_header.length = cpu_to_le16(len);
> > > > +	cmd_header.read = 0;
> > > > +
> > > > +	msg[0].addr = client->addr;
> > > > +	msg[0].flags = 0;
> > > > +	msg[0].len = sizeof(cmd_header);
> > > > +	msg[0].buf = (u8 *)&cmd_header;
> > > > +
> > > > +	msg[1].addr = client->addr;
> > > > +	msg[1].flags = 0;
> > > > +	msg[1].len = len;
> > > > +	msg[1].buf = (char *)buf;
> > >
> > > Please check the "comms protocol app note", for write it is not allowed
> > > to send a stop, so the whole data must be send in one i2c_msg.
> > >
> > 
> > Well I only have the "Programmer's Guide", I'll have to check that as it
> > really seems to works as it yet.
> 
> As far as I know we only send "stop" on the last message in a sequence
> of messages in i2c_transfer() unless it is explicitly requested with
> I2C_M_STOP flag.

You're right, I re-checked this again but this approach is still wrong
since the protocol does not allow sending the payload as separate
message. The payload must be send as one message starting with the
i2c-address of the touchconroller followed by the target register
address and how many bytes should be written followed by the payload.

> ...
> > > > +
> > > > +static void axiom_i2c_remove(struct i2c_client *client)
> > > > +{
> > > > +	struct axiom_data *ts = i2c_get_clientdata(client);
> > > > +
> > > > +	input_unregister_device(ts->input_dev);
> > >
> > > This can be part of devm_add_action_or_reset() and we could remove the
> > > .remove() callback for this driver.
> > >
> > 
> > Sure, thanks for the tips :)!
> 
> Actually input devices allocated with devm do not need to be explicitly
> inregistered, so you do not need to bother with
> devm_add_action_or_reset() and simply delete axiom_i2c_remove().

Ah I see.. so all devm_alloced...() device don't need that explicite
unregister. Thanks.

Regards,
  Marco

> 
> Thanks.
> 
> -- 
> Dmitry
>