Re: [PATCH 2/2] Input: add Himax HX852x(ES) touchscreen driver

[Stephan Gerhold]

Hi Jeff,

On Tue, Sep 26, 2023 at 09:59:53PM -0500, Jeff LaBundy wrote:
> On Sun, Sep 17, 2023 at 07:05:50PM +0200, Stephan Gerhold wrote:
> > On Sat, Sep 16, 2023 at 03:47:55PM -0500, Jeff LaBundy wrote:
> > > On Wed, Sep 13, 2023 at 03:25:30PM +0200, Stephan Gerhold wrote:
> > > > From: Jonathan Albrieux <jonathan.albrieux@xxxxxxxxx>
> [...]
> > > > +static int hx852x_probe(struct i2c_client *client)
> > > > +{
> > > > +	struct device *dev = &client->dev;
> > > > +	struct hx852x *hx;
> > > > +	int error, i;
> > > > +
> > > > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> > > > +				     I2C_FUNC_SMBUS_WRITE_BYTE |
> > > > +				     I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> > > > +				     I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
> > > > +		dev_err(dev, "not all i2c functionality supported\n");
> > > > +		return -ENXIO;
> > > > +	}
> > > > +
> > > > +	hx = devm_kzalloc(dev, sizeof(*hx), GFP_KERNEL);
> > > > +	if (!hx)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	hx->client = client;
> > > > +	hx->input_dev = devm_input_allocate_device(dev);
> > > > +	if (!hx->input_dev)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	hx->input_dev->name = "Himax HX852x";
> > > > +	hx->input_dev->id.bustype = BUS_I2C;
> > > > +	hx->input_dev->open = hx852x_input_open;
> > > > +	hx->input_dev->close = hx852x_input_close;
> > > > +
> > > > +	i2c_set_clientdata(client, hx);
> > > > +	input_set_drvdata(hx->input_dev, hx);
> > > > +
> > > > +	hx->supplies[0].supply = "vcca";
> > > > +	hx->supplies[1].supply = "vccd";
> > > > +	error = devm_regulator_bulk_get(dev, ARRAY_SIZE(hx->supplies), hx->supplies);
> > > > +	if (error < 0)
> > > > +		return dev_err_probe(dev, error, "failed to get regulators");
> > > > +
> > > > +	hx->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > > > +	if (IS_ERR(hx->reset_gpiod))
> > > > +		return dev_err_probe(dev, error, "failed to get reset gpio");
> > > 
> > > Can the reset GPIO be optional?
> > > 
> > 
> > I'm afraid I have no idea if the controller needs this or not. Would it
> > be better to keep it required until someone confirms otherwise or have
> > it optional for the other way around?
> 
> If you have a datasheet handy, or your hardware provides a means for you to
> test and confirm whether reset can be left out, I would make the reset GPIO
> optional. Often times, these controllers are part of a module and reset may
> be tied high locally as opposed to adding another signal to a flex cable.
> 
> If you have no way to confirm, I would keep it as required for now; it is not
> too cumbersome to be changed later if the need arises on different hardware.
> 

I don't have a datasheet unfortunately. :(

However, I tried to simulate this case on my board by keeping the reset
GPIO permanently de-asserted (i.e. high because of active-low). The
results are not entirely conclusive: The controller seems to respond to
commands and the initial configuration is read correctly. However, it
does not report any touch events. As soon as I add the temporary
assertion of the reset signal back it works fine again.

I suspect toggling the reset signal might be required to make the
controller come properly out of reset. I'll keep it required to be sure.

Thanks,
Stephan