mbox series

[v5,0/5] Add support for ToF sensor on Yoshino platform

Message ID 20220523175344.5845-1-markuss.broks@gmail.com
Headers show
Series Add support for ToF sensor on Yoshino platform | expand

Message

Markuss Broks May 23, 2022, 5:53 p.m. UTC
This series adds support for the ToF proximity sensor installed on
Yoshino devices. As part of this series, support handling the reset
GPIO and VDD supply by the VL53L0X driver. Also stop hardcoding the
interrupt type, since on Yoshino devices it seems that edge triggering
doesn't work properly.

Tested on Sony Xperia XZ1 (poplar).

Cc: Konrad Dybcio <konrad.dybcio@somainline.org>
Cc: Marijn Suijten <marijn.suijten@somainline.org>
Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

v2:
- Fix a nasty issue: turns out grouping the pinctrl makes it not apply,
which was the main cause of edge interrupts not working correctly and
having to use level interrupts, which caused a large amount of false
detections.
- handle the irq type more gracefully: if it's not provided, default
to falling edge, but if it's provided, then use the provided one.
v3:
- add irq.h header (forgot to commit)
- reword commit message (already initialized -> pre-initialized)
v4:
- reorder powering on and power off action (Jonathan)
- sort pinctrls by GPIO number (Konrad)
v5:
- "This patch adds..." -> "Add ..." (Krzysztof)

Markuss Broks (5):
  dt-bindings: proximity: vl53l0x: Document optional supply and GPIO
    properties
  proximity: vl53l0x: Get interrupt type from DT
  proximity: vl53l0x: Handle the VDD regulator
  proximity: vl53l0x: Handle the reset GPIO
  arm64: dts: qcom: msm8998-xperia: Introduce ToF sensor support

 .../bindings/iio/proximity/st,vl53l0x.yaml    |  5 ++
 .../dts/qcom/msm8998-sony-xperia-yoshino.dtsi | 34 +++++++++++++
 drivers/iio/proximity/vl53l0x-i2c.c           | 50 ++++++++++++++++++-
 3 files changed, 88 insertions(+), 1 deletion(-)

Comments

Jonathan Cameron June 14, 2022, 10:48 a.m. UTC | #1
On Sun, 12 Jun 2022 11:28:22 +0200
Luca Weiss <luca@z3ntu.xyz> wrote:

> Hi Jonathan,
> 
> On Sonntag, 12. Juni 2022 10:53:33 CEST Jonathan Cameron wrote:
> > On Wed, 08 Jun 2022 12:18:52 +0200
> > 
> > "Luca Weiss" <luca.weiss@fairphone.com> wrote:  
> > > Hi Markuss,
> > > 
> > > On Mon May 23, 2022 at 7:53 PM CEST, Markuss Broks wrote:  
> > > > Handle the regulator supplying the VDD pin of VL53L0X.
> > > > 
> > > > Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> > > > ---
> > > > 
> > > >  drivers/iio/proximity/vl53l0x-i2c.c | 37 +++++++++++++++++++++++++++++
> > > >  1 file changed, 37 insertions(+)
> > > > 
> > > > diff --git a/drivers/iio/proximity/vl53l0x-i2c.c
> > > > b/drivers/iio/proximity/vl53l0x-i2c.c index 12a3e2eff464..8581a873919f
> > > > 100644
> > > > --- a/drivers/iio/proximity/vl53l0x-i2c.c
> > > > +++ b/drivers/iio/proximity/vl53l0x-i2c.c
> > > > @@ -43,6 +43,7 @@
> > > > 
> > > >  struct vl53l0x_data {
> > > >  
> > > >  	struct i2c_client *client;
> > > >  	struct completion completion;
> > > > 
> > > > +	struct regulator *vdd_supply;
> > > > 
> > > >  };
> > > >  
> > > >  static irqreturn_t vl53l0x_handle_irq(int irq, void *priv)
> > > > 
> > > > @@ -192,10 +193,31 @@ static const struct iio_info vl53l0x_info = {
> > > > 
> > > >  	.read_raw = vl53l0x_read_raw,
> > > >  
> > > >  };
> > > > 
> > > > +static void vl53l0x_power_off(void *_data)
> > > > +{
> > > > +	struct vl53l0x_data *data = _data;
> > > > +
> > > > +	regulator_disable(data->vdd_supply);
> > > > +}
> > > > +
> > > > +static int vl53l0x_power_on(struct vl53l0x_data *data)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = regulator_enable(data->vdd_supply);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	usleep_range(3200, 5000);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > 
> > > >  static int vl53l0x_probe(struct i2c_client *client)
> > > >  {
> > > >  
> > > >  	struct vl53l0x_data *data;
> > > >  	struct iio_dev *indio_dev;
> > > > 
> > > > +	int error;
> > > > 
> > > >  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > > >  	if (!indio_dev)
> > > > 
> > > > @@ -210,6 +232,21 @@ static int vl53l0x_probe(struct i2c_client *client)
> > > > 
> > > >  				     I2C_FUNC_SMBUS_BYTE_DATA))
> > > >  		
> > > >  		return -EOPNOTSUPP;
> > > > 
> > > > +	data->vdd_supply = devm_regulator_get_optional(&client->dev,   
> "vdd");
> > > > +	if (IS_ERR(data->vdd_supply))
> > > > +		return dev_err_probe(&client->dev, PTR_ERR(data-  
> >vdd_supply),  
> > > > +				     "Unable to get VDD   
> regulator\n");
> > > 
> > > It looks like this optional regulator is not actually optional.
> > > 
> > > [    1.919995] vl53l0x-i2c 1-0029: error -ENODEV: Unable to get VDD
> > > regulator
> > > 
> > > When using devm_regulator_get instead, a dummy regulator gets returned
> > > which I think is what we want here:
> > > 
> > > [    1.905518] vl53l0x-i2c 1-0029: supply vdd not found, using dummy
> > > regulator
> > > 
> > > Can you fix this up or should I send a patch?  
> > 
> > Hi Luca,
> > 
> > Please send a patch.  
> 
> Which commit sha can I use for Fixes: here?
> Based your togreg[0] branch currently shows "Age: 20 hours" I guess it was 
> rebased recently?

It was rebased onto rc1 as you noticed.

In theory it is now stable, assuming nothing nasty shows up.
Fixes tag doesn't matter strongly given both will go into mainline via
the same pull request, so maybe just skip adding one to make my life
easier :)

Thanks,

Jonathan

> 
> Regards
> Luca
> 
> [0]https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/log/?h=togreg
> 
> > 
> > Jonathan
> >   
> > > Regards
> > > Luca
> > >   
> > > > +
> > > > +	error = vl53l0x_power_on(data);
> > > > +	if (error)
> > > > +		return dev_err_probe(&client->dev, error,
> > > > +				     "Failed to power on the   
> chip\n");
> > > > +
> > > > +	error = devm_add_action_or_reset(&client->dev, vl53l0x_power_off,
> > > > data);
> > > > +	if (error)
> > > > +		return dev_err_probe(&client->dev, error,
> > > > +				     "Failed to install poweroff   
> action\n");
> > > > +
> > > > 
> > > >  	indio_dev->name = "vl53l0x";
> > > >  	indio_dev->info = &vl53l0x_info;
> > > >  	indio_dev->channels = vl53l0x_channels;  
> 
> 
> 
>