| Message ID | 20200518133645.19127-4-jonathan.albrieux@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: magnetometer: ak8975: Add gpio reset support | expand |
On Mon, May 18, 2020 at 4:38 PM Jonathan Albrieux <jonathan.albrieux@gmail.com> wrote: > + gpiod_set_value_cansleep(data->reset_gpiod, 1); (1) ... > + /* > + * If reset pin is provided then will be set to high on power on > + * and to low on power off according to AK09911 datasheet > + */ Wording is confusing, perhaps you have to use 'asserted / deasserted'. Btw, in (1) it's also "high" (asserted). I barely understand how it's supposed to work in all cases? > + reset_gpiod = devm_gpiod_get_optional(&client->dev, > + "reset", GPIOD_OUT_HIGH); > + if (IS_ERR(reset_gpiod)) > + return PTR_ERR(reset_gpiod);
On Mon, May 18, 2020 at 05:55:51PM +0300, Andy Shevchenko wrote: > On Mon, May 18, 2020 at 4:38 PM Jonathan Albrieux > <jonathan.albrieux@gmail.com> wrote: > > > + gpiod_set_value_cansleep(data->reset_gpiod, 1); > > (1) > > ... > > > + /* > > + * If reset pin is provided then will be set to high on power on > > + * and to low on power off according to AK09911 datasheet > > + */ > > Wording is confusing, perhaps you have to use 'asserted / deasserted'. Thank you for the suggestion, I'll be working on rewording as soon as possible. > Btw, in (1) it's also "high" (asserted). I barely understand how it's > supposed to work in all cases? > > > + reset_gpiod = devm_gpiod_get_optional(&client->dev, > > + "reset", GPIOD_OUT_HIGH); > > + if (IS_ERR(reset_gpiod)) > > + return PTR_ERR(reset_gpiod); > I'm sorry but I'm not sure about what you mean by saying all cases. Currently I'm testing this driver on a msm8916 device having AK09911 magnetometer. At the current stage the driver is failing on probe because reset pin is not connected to VID (as datasheet requires in case of pin not being used). In case of reset pin not asserted, register's reset is triggered resulting in empty registers, leading to probe fail. For this reason pin is asserted during power on in order to have informations in registers and deasserted before power off triggering a reset. A workaround that gets AK09911 working on device is by setting the reset pin always high on device tree. This way registers gets reset by a Power On Reset circuit autonomously and reset pin never triggers the reset. > -- > With Best Regards, > Andy Shevchenko Hoping to having answered to your question, Best regards, Jonathan Albrieux
On Mon, May 18, 2020 at 06:01:20PM +0200, Jonathan Albrieux wrote: > On Mon, May 18, 2020 at 05:55:51PM +0300, Andy Shevchenko wrote: > > On Mon, May 18, 2020 at 4:38 PM Jonathan Albrieux > > <jonathan.albrieux@gmail.com> wrote: > > > > > + gpiod_set_value_cansleep(data->reset_gpiod, 1); > > > > (1) > > > > ... > > > > > + /* > > > + * If reset pin is provided then will be set to high on power on > > > + * and to low on power off according to AK09911 datasheet > > > + */ > > > > Wording is confusing, perhaps you have to use 'asserted / deasserted'. > > Thank you for the suggestion, I'll be working on rewording as soon as > possible. > > > Btw, in (1) it's also "high" (asserted). I barely understand how it's > > supposed to work in all cases? > > > > > + reset_gpiod = devm_gpiod_get_optional(&client->dev, > > > + "reset", GPIOD_OUT_HIGH); > > > + if (IS_ERR(reset_gpiod)) > > > + return PTR_ERR(reset_gpiod); > > > > I'm sorry but I'm not sure about what you mean by saying all cases. > Currently I'm testing this driver on a msm8916 device having AK09911 > magnetometer. At the current stage the driver is failing on probe > because reset pin is not connected to VID (as datasheet requires in case > of pin not being used). In case of reset pin not asserted, register's > reset is triggered resulting in empty registers, leading to probe fail. > For this reason pin is asserted during power on in order to have > informations in registers and deasserted before power off triggering > a reset. > > A workaround that gets AK09911 working on device is by setting the > reset pin always high on device tree. This way registers gets reset by > a Power On Reset circuit autonomously and reset pin never triggers the > reset. You need to distinguish electrical level from logical (GPIO flag defines logical). So, I'm talking about active-high vs. active-low case. Now I re-read above, and see that here you assert the reset signal. But where is desertion?
On Mon, May 18, 2020 at 07:43:17PM +0300, Andy Shevchenko wrote: > On Mon, May 18, 2020 at 06:01:20PM +0200, Jonathan Albrieux wrote: > > On Mon, May 18, 2020 at 05:55:51PM +0300, Andy Shevchenko wrote: > > > On Mon, May 18, 2020 at 4:38 PM Jonathan Albrieux > > > <jonathan.albrieux@gmail.com> wrote: > > > > > > > + gpiod_set_value_cansleep(data->reset_gpiod, 1); > > > > > > (1) > > > > > > ... > > > > > > > + /* > > > > + * If reset pin is provided then will be set to high on power on > > > > + * and to low on power off according to AK09911 datasheet > > > > + */ > > > > > > Wording is confusing, perhaps you have to use 'asserted / deasserted'. > > > > Thank you for the suggestion, I'll be working on rewording as soon as > > possible. > > > > > Btw, in (1) it's also "high" (asserted). I barely understand how it's > > > supposed to work in all cases? > > > > > > > + reset_gpiod = devm_gpiod_get_optional(&client->dev, > > > > + "reset", GPIOD_OUT_HIGH); > > > > + if (IS_ERR(reset_gpiod)) > > > > + return PTR_ERR(reset_gpiod); > > > > > > > I'm sorry but I'm not sure about what you mean by saying all cases. > > Currently I'm testing this driver on a msm8916 device having AK09911 > > magnetometer. At the current stage the driver is failing on probe > > because reset pin is not connected to VID (as datasheet requires in case > > of pin not being used). In case of reset pin not asserted, register's > > reset is triggered resulting in empty registers, leading to probe fail. > > For this reason pin is asserted during power on in order to have > > informations in registers and deasserted before power off triggering > > a reset. > > > > A workaround that gets AK09911 working on device is by setting the > > reset pin always high on device tree. This way registers gets reset by > > a Power On Reset circuit autonomously and reset pin never triggers the > > reset. > > You need to distinguish electrical level from logical (GPIO flag defines > logical). So, I'm talking about active-high vs. active-low case. > > Now I re-read above, and see that here you assert the reset signal. But where > is desertion? Oh I see, I'll try explaining by points the proposed approach: - reset pin is active low - during power on gpio is set to 0 so the reset pin is high, thus no reset - during power off gpio is set to 1 so the reset pin becomes low, thus resetting this is a possible solution but maybe there are other ways to achieve that, do you have suggestions on how to get a better approach for solving this issue? > -- > With Best Regards, > Andy Shevchenko > > Best regards, Jonathan Albrieux
On Mon, May 18, 2020 at 8:43 PM Jonathan Albrieux <jonathan.albrieux@gmail.com> wrote: > > On Mon, May 18, 2020 at 07:43:17PM +0300, Andy Shevchenko wrote: > > On Mon, May 18, 2020 at 06:01:20PM +0200, Jonathan Albrieux wrote: > > > On Mon, May 18, 2020 at 05:55:51PM +0300, Andy Shevchenko wrote: > > > > On Mon, May 18, 2020 at 4:38 PM Jonathan Albrieux > > > > <jonathan.albrieux@gmail.com> wrote: > > > > > > > > > + gpiod_set_value_cansleep(data->reset_gpiod, 1); > > > > > > > > (1) > > > > > > > > ... > > > > > > > > > + /* > > > > > + * If reset pin is provided then will be set to high on power on > > > > > + * and to low on power off according to AK09911 datasheet > > > > > + */ > > > > > > > > Wording is confusing, perhaps you have to use 'asserted / deasserted'. > > > > > > Thank you for the suggestion, I'll be working on rewording as soon as > > > possible. > > > > > > > Btw, in (1) it's also "high" (asserted). I barely understand how it's > > > > supposed to work in all cases? > > > > > > > > > + reset_gpiod = devm_gpiod_get_optional(&client->dev, > > > > > + "reset", GPIOD_OUT_HIGH); > > > > > + if (IS_ERR(reset_gpiod)) > > > > > + return PTR_ERR(reset_gpiod); > > > > > > > > > > I'm sorry but I'm not sure about what you mean by saying all cases. > > > Currently I'm testing this driver on a msm8916 device having AK09911 > > > magnetometer. At the current stage the driver is failing on probe > > > because reset pin is not connected to VID (as datasheet requires in case > > > of pin not being used). In case of reset pin not asserted, register's > > > reset is triggered resulting in empty registers, leading to probe fail. > > > For this reason pin is asserted during power on in order to have > > > informations in registers and deasserted before power off triggering > > > a reset. > > > > > > A workaround that gets AK09911 working on device is by setting the > > > reset pin always high on device tree. This way registers gets reset by > > > a Power On Reset circuit autonomously and reset pin never triggers the > > > reset. > > > > You need to distinguish electrical level from logical (GPIO flag defines > > logical). So, I'm talking about active-high vs. active-low case. > > > > Now I re-read above, and see that here you assert the reset signal. But where > > is desertion? > > Oh I see, I'll try explaining by points the proposed approach: > - reset pin is active low > - during power on gpio is set to 0 so the reset pin is high, thus no reset deasserted > - during power off gpio is set to 1 so the reset pin becomes low, thus resetting asserted > this is a possible solution but maybe there are other ways to achieve that, > do you have suggestions on how to get a better approach for solving this issue? I see now, that at requesting reset you wanted to chip be in reset state (asserted) till driver calls power_on(). Seems everything you done is correct. Just correct terminology, please.
On Mon, May 18, 2020 at 08:51:29PM +0300, Andy Shevchenko wrote: > On Mon, May 18, 2020 at 8:43 PM Jonathan Albrieux > <jonathan.albrieux@gmail.com> wrote: > > > > On Mon, May 18, 2020 at 07:43:17PM +0300, Andy Shevchenko wrote: > > > On Mon, May 18, 2020 at 06:01:20PM +0200, Jonathan Albrieux wrote: > > > > On Mon, May 18, 2020 at 05:55:51PM +0300, Andy Shevchenko wrote: > > > > > On Mon, May 18, 2020 at 4:38 PM Jonathan Albrieux > > > > > <jonathan.albrieux@gmail.com> wrote: > > > > > > > > > > > + gpiod_set_value_cansleep(data->reset_gpiod, 1); > > > > > > > > > > (1) > > > > > > > > > > ... > > > > > > > > > > > + /* > > > > > > + * If reset pin is provided then will be set to high on power on > > > > > > + * and to low on power off according to AK09911 datasheet > > > > > > + */ > > > > > > > > > > Wording is confusing, perhaps you have to use 'asserted / deasserted'. > > > > > > > > Thank you for the suggestion, I'll be working on rewording as soon as > > > > possible. > > > > > > > > > Btw, in (1) it's also "high" (asserted). I barely understand how it's > > > > > supposed to work in all cases? > > > > > > > > > > > + reset_gpiod = devm_gpiod_get_optional(&client->dev, > > > > > > + "reset", GPIOD_OUT_HIGH); > > > > > > + if (IS_ERR(reset_gpiod)) > > > > > > + return PTR_ERR(reset_gpiod); > > > > > > > > > > > > > I'm sorry but I'm not sure about what you mean by saying all cases. > > > > Currently I'm testing this driver on a msm8916 device having AK09911 > > > > magnetometer. At the current stage the driver is failing on probe > > > > because reset pin is not connected to VID (as datasheet requires in case > > > > of pin not being used). In case of reset pin not asserted, register's > > > > reset is triggered resulting in empty registers, leading to probe fail. > > > > For this reason pin is asserted during power on in order to have > > > > informations in registers and deasserted before power off triggering > > > > a reset. > > > > > > > > A workaround that gets AK09911 working on device is by setting the > > > > reset pin always high on device tree. This way registers gets reset by > > > > a Power On Reset circuit autonomously and reset pin never triggers the > > > > reset. > > > > > > You need to distinguish electrical level from logical (GPIO flag defines > > > logical). So, I'm talking about active-high vs. active-low case. > > > > > > Now I re-read above, and see that here you assert the reset signal. But where > > > is desertion? > > > > Oh I see, I'll try explaining by points the proposed approach: > > - reset pin is active low > > - during power on gpio is set to 0 so the reset pin is high, thus no reset > > deasserted > > > - during power off gpio is set to 1 so the reset pin becomes low, thus resetting > > asserted > > > this is a possible solution but maybe there are other ways to achieve that, > > do you have suggestions on how to get a better approach for solving this issue? > > I see now, that at requesting reset you wanted to chip be in reset > state (asserted) till driver calls power_on(). > > Seems everything you done is correct. Just correct terminology, please. Will surely do, thank you! > -- > With Best Regards, > Andy Shevchenko Best regards, Jonathan Albrieux
Hi! > AK09911 has a reset gpio to handle register's reset. If reset gpio is > set to low it will trigger the reset. AK09911 datasheed says that if not > used reset pin should be connected to VID and this patch emulates this > situation > > Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com> > --- > drivers/iio/magnetometer/ak8975.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > /* > - * According to the datasheet the power supply rise time i 200us > + * According to the datasheet the power supply rise time is 200us > * and the minimum wait time before mode setting is 100us, in > - * total 300 us. Add some margin and say minimum 500us here. > + * total 300us. Add some margin and say minimum 500us here. > */ > usleep_range(500, 1000); I'd assume that datasheet added some safety margin too, and there's another one in usleep... So I believe usleep..(300) should be okay.. Best regards, Pavel
diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c index 3c881541ae72..c1ba5cd2cb6c 100644 --- a/drivers/iio/magnetometer/ak8975.c +++ b/drivers/iio/magnetometer/ak8975.c @@ -358,6 +358,7 @@ struct ak8975_data { u8 asa[3]; long raw_to_gauss[3]; struct gpio_desc *eoc_gpiod; + struct gpio_desc *reset_gpiod; int eoc_irq; wait_queue_head_t data_ready_queue; unsigned long flags; @@ -384,10 +385,13 @@ static int ak8975_power_on(const struct ak8975_data *data) "Failed to enable specified Vid supply\n"); return ret; } + + gpiod_set_value_cansleep(data->reset_gpiod, 0); + /* - * According to the datasheet the power supply rise time i 200us + * According to the datasheet the power supply rise time is 200us * and the minimum wait time before mode setting is 100us, in - * total 300 us. Add some margin and say minimum 500us here. + * total 300us. Add some margin and say minimum 500us here. */ usleep_range(500, 1000); return 0; @@ -396,6 +400,8 @@ static int ak8975_power_on(const struct ak8975_data *data) /* Disable attached power regulator if any. */ static void ak8975_power_off(const struct ak8975_data *data) { + gpiod_set_value_cansleep(data->reset_gpiod, 1); + regulator_disable(data->vid); regulator_disable(data->vdd); } @@ -839,6 +845,7 @@ static int ak8975_probe(struct i2c_client *client, struct ak8975_data *data; struct iio_dev *indio_dev; struct gpio_desc *eoc_gpiod; + struct gpio_desc *reset_gpiod; const void *match; unsigned int i; int err; @@ -856,6 +863,15 @@ static int ak8975_probe(struct i2c_client *client, if (eoc_gpiod) gpiod_set_consumer_name(eoc_gpiod, "ak_8975"); + /* + * If reset pin is provided then will be set to high on power on + * and to low on power off according to AK09911 datasheet + */ + reset_gpiod = devm_gpiod_get_optional(&client->dev, + "reset", GPIOD_OUT_HIGH); + if (IS_ERR(reset_gpiod)) + return PTR_ERR(reset_gpiod); + /* Register with IIO */ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); if (indio_dev == NULL) @@ -866,6 +882,7 @@ static int ak8975_probe(struct i2c_client *client, data->client = client; data->eoc_gpiod = eoc_gpiod; + data->reset_gpiod = reset_gpiod; data->eoc_irq = 0; err = iio_read_mount_matrix(&client->dev, "mount-matrix", &data->orientation);
Set reset gpio to high on ak8975_power_on, set reset gpio to low on ak8975_power_off. Without setting it to high on ak8975_power_on driver's probe fails on ak8975_who_i_am while checking for device identity for AK09911 chip AK09911 has a reset gpio to handle register's reset. If reset gpio is set to low it will trigger the reset. AK09911 datasheed says that if not used reset pin should be connected to VID and this patch emulates this situation Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com> --- drivers/iio/magnetometer/ak8975.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)