Re: [PATCH v2 1/2] Input: mms114 - add support for mms345l

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Stephan Gerhold <stephan@gerhold.net>
Cc: Andi Shyti <andi@etezian.org>,
	linux-input@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v2 1/2] Input: mms114 - add support for mms345l
Date: Wed, 22 Apr 2020 14:05:52 -0700	[thread overview]
Message-ID: <20200422210552.GF125362@dtor-ws> (raw)
In-Reply-To: <20200410102631.GA85423@gerhold.net>

On Fri, Apr 10, 2020 at 12:27:02PM +0200, Stephan Gerhold wrote:
> Hi Dmitry,
> 
> On Thu, Apr 09, 2020 at 03:15:03PM -0700, Dmitry Torokhov wrote:
> > Hi Stephan,
> > 
> > On Sun, Apr 05, 2020 at 07:09:03PM +0200, Stephan Gerhold wrote:
> > > MMS345L is another first generation touch screen from Melfas,
> > > which uses the same registers as MMS152.
> > > 
> > > However, using I2C_M_NOSTART for it causes errors when reading:
> > > 
> > > 	i2c i2c-0: sendbytes: NAK bailout.
> > > 	mms114 0-0048: __mms114_read_reg: i2c transfer failed (-5)
> > > 
> > > The driver works fine as soon as I2C_M_NOSTART is removed.
> > > 
> > > Add a separate melfas,mms345l binding, and make use of I2C_M_NOSTART
> > > only for MMS114 and MMS152.
> > > 
> > > Reviewed-by: Andi Shyti <andi@etezian.org>
> > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > 
> > Sorry for sitting on this for so long. I looked around, and I think that
> > instead of adding separate handling for 345L we shoudl simply drop the
> > "no start" bit for everyone. I am not sure what the original version
> > tried to do here, as far as I can see in various public Android trees the
> > driver for these devices does not use I2C_M_NOSTART.
> > 
> > Actually, I wonder if the difference is not in the touch controller that
> > is being used, but rather in the I2C controller the device in connected
> > to.
> > 
> > I would like to apply the version of the patch below.
> 
> Thanks a lot for your reply!
> 
> Your suggested patch below works fine on my MMS345L device
> (with the melfas,mms152 compatible). Applying it would allow
> the touchscreen to work on my device in mainline :)
> 
> I wonder if we should still add a separate melfas,mms345l compatible:
> 
> The only remaining difference to MMS152 at the moment is the
> MMS152_COMPAT_GROUP register. It seems to be used for something
> different on MMS345L, so when it is printed in mms114_get_version()
> it just displays some garbage:
> 
> TSP FW Rev: bootloader 0x6 / core 0x26 / config 0x26, Compat group: \x06
> 
> (It used to actually print it as some ASCII control character but
>  apparently that was fixed...)
> 
> It's not really critical, but in case we find more differences I think
> it's better to directly add a separate compatible string to the device
> tree.
> 
> If you agree with this I can send a separate patch with the new
> compatible string once you have applied the patch below.

Yes, if there is legitimate differences between the chips that would be
a good reason to add a new compatible string.

Please send a separate patch with updated justification and I will apply
it.

Thank you.

-- 
Dmitry