* [PATCH 1/2] staging: iio: ad2s1210: Destroy mutex at remove @ 2019-05-18 22:15 Tallys Martins 2019-05-18 22:15 ` [PATCH 2/2] staging: iio: ad2s1210: Add devicetree yaml doc Tallys Martins 2019-05-19 11:12 ` [PATCH 1/2] staging: iio: ad2s1210: Destroy mutex at remove Jonathan Cameron 0 siblings, 2 replies; 5+ messages in thread From: Tallys Martins @ 2019-05-18 22:15 UTC (permalink / raw) To: Lars-Peter Clausen, Michael Hennerich, Stefan Popa, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman Cc: linux-iio, devel, linux-kernel, kernel-usp, Tallys Martins, Souza Guilherme Ensure the mutex will be destroyed on drive removal. Also adds mutex comment description. Signed-off-by: Tallys Martins <tallysmartins@gmail.com> Signed-off-by: Souza Guilherme <gdsdsilva@inf.ufpel.edu.br> Co-developed-by: Souza Guilherme <gdsdsilva@inf.ufpel.edu.br> --- drivers/staging/iio/resolver/ad2s1210.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c index b6be0bc202f5..b91cf57c5e57 100644 --- a/drivers/staging/iio/resolver/ad2s1210.c +++ b/drivers/staging/iio/resolver/ad2s1210.c @@ -86,7 +86,7 @@ static const struct ad2s1210_gpio gpios[] = { static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 }; struct ad2s1210_state { - struct mutex lock; + struct mutex lock; /* lock to protect the state on r/w operations */ struct spi_device *sdev; struct gpio_desc *gpios[5]; unsigned int fclkin; @@ -689,8 +689,10 @@ static int ad2s1210_probe(struct spi_device *spi) static int ad2s1210_remove(struct spi_device *spi) { struct iio_dev *indio_dev = spi_get_drvdata(spi); + struct ad2s1210_state *ad2s1210_ad = iio_priv(indio_dev); iio_device_unregister(indio_dev); + mutex_destroy(&ad2s1210_ad->lock); return 0; } -- 2.21.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] staging: iio: ad2s1210: Add devicetree yaml doc 2019-05-18 22:15 [PATCH 1/2] staging: iio: ad2s1210: Destroy mutex at remove Tallys Martins @ 2019-05-18 22:15 ` Tallys Martins 2019-05-19 11:17 ` Jonathan Cameron 2019-05-19 11:12 ` [PATCH 1/2] staging: iio: ad2s1210: Destroy mutex at remove Jonathan Cameron 1 sibling, 1 reply; 5+ messages in thread From: Tallys Martins @ 2019-05-18 22:15 UTC (permalink / raw) To: Lars-Peter Clausen, Michael Hennerich, Stefan Popa, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman Cc: linux-iio, devel, linux-kernel, kernel-usp, Tallys Martins, Souza Guilherme The driver currently has no devicetree documentation. This commit adds a devicetree folder and documentation for it. Documentation must be moved as well when the driver gets out of staging. Signed-off-by: Tallys Martins <tallysmartins@gmail.com> Signed-off-by: Souza Guilherme <gdsdsilva@inf.ufpel.edu.br> Co-developed-by: Souza Guilherme <gdsdsilva@inf.ufpel.edu.br> --- .../Documentation/devicetree/ad2s1210.yaml | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml diff --git a/drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml b/drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml new file mode 100644 index 000000000000..733aa07b4626 --- /dev/null +++ b/drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml @@ -0,0 +1,41 @@ +# SPDX-License-Identifier: GPL-2.0 +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/iio/ad2s1210.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: | + Analog Devices Inc. AD2S1210 10-Bit to 16-Bit R/D Converters + +maintainers: + - Graff Yang <graff.yang@gmail.com> + +description: | + Analog Devices AD2S1210 Resolver to Digital SPI driver + + https://www.analog.com/en/products/ad2s1210.html + +properties: + compatible: + enum: + - adi,ad2s1210 + + reg: + maxItems: 1 + + clock-frequency: + minimum: 2000 + maximum: 20000 + default: 8192 + +required: + - compatible + - reg + +examples: + - | + resolver@0 { + compatible = "adi,ad2s1210"; + reg = <0>; + }; +... -- 2.21.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] staging: iio: ad2s1210: Add devicetree yaml doc 2019-05-18 22:15 ` [PATCH 2/2] staging: iio: ad2s1210: Add devicetree yaml doc Tallys Martins @ 2019-05-19 11:17 ` Jonathan Cameron 2019-05-20 10:17 ` Alexandru Ardelean 0 siblings, 1 reply; 5+ messages in thread From: Jonathan Cameron @ 2019-05-19 11:17 UTC (permalink / raw) To: Tallys Martins Cc: Lars-Peter Clausen, Michael Hennerich, Stefan Popa, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, linux-iio, devel, linux-kernel, kernel-usp, Souza Guilherme On Sat, 18 May 2019 19:15:58 -0300 Tallys Martins <tallysmartins@gmail.com> wrote: > The driver currently has no devicetree documentation. This commit adds a > devicetree folder and documentation for it. Documentation must be moved > as well when the driver gets out of staging. > > Signed-off-by: Tallys Martins <tallysmartins@gmail.com> > Signed-off-by: Souza Guilherme <gdsdsilva@inf.ufpel.edu.br> > Co-developed-by: Souza Guilherme <gdsdsilva@inf.ufpel.edu.br> Hi, There is no need for a direct relationship between a binding and a driver at all. As such, we normally don't take binding documents in staging. Just put it directly in it's final destination. The driver can catch up with it later! I'm still not that comfortable with yaml (haven't gotten around to doing any conversions myself yet) so I'll be looking for additional review on these from others. A few comments inline. > --- > .../Documentation/devicetree/ad2s1210.yaml | 41 +++++++++++++++++++ > 1 file changed, 41 insertions(+) > create mode 100644 drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml > > diff --git a/drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml b/drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml > new file mode 100644 > index 000000000000..733aa07b4626 > --- /dev/null > +++ b/drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml > @@ -0,0 +1,41 @@ > +# SPDX-License-Identifier: GPL-2.0 > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/iio/ad2s1210.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: | > + Analog Devices Inc. AD2S1210 10-Bit to 16-Bit R/D Converters > + > +maintainers: > + - Graff Yang <graff.yang@gmail.com> I would check that one with the Analog devices team. > + > +description: | > + Analog Devices AD2S1210 Resolver to Digital SPI driver > + > + https://www.analog.com/en/products/ad2s1210.html > + > +properties: > + compatible: > + enum: > + - adi,ad2s1210 > + > + reg: > + maxItems: 1 > + > + clock-frequency: > + minimum: 2000 > + maximum: 20000 > + default: 8192 This doesn't look like a modern clock binding. If we are going to end up changing this, then we should probably delay having a binding doc until after that change is made. We often only do binding docs for drivers in staging just before moving them out so as to avoid this sort of issue. > + > +required: > + - compatible > + - reg > + > +examples: > + - | > + resolver@0 { > + compatible = "adi,ad2s1210"; > + reg = <0>; An example is probably more useful if it includes all the optional properties as well. > + }; > +... Thanks, Jonathan ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] staging: iio: ad2s1210: Add devicetree yaml doc 2019-05-19 11:17 ` Jonathan Cameron @ 2019-05-20 10:17 ` Alexandru Ardelean 0 siblings, 0 replies; 5+ messages in thread From: Alexandru Ardelean @ 2019-05-20 10:17 UTC (permalink / raw) To: Jonathan Cameron Cc: Tallys Martins, Lars-Peter Clausen, Michael Hennerich, Stefan Popa, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, linux-iio, devel, LKML, kernel-usp, Souza Guilherme, Alexandru Ardelean On Sun, May 19, 2019 at 8:18 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Sat, 18 May 2019 19:15:58 -0300 > Tallys Martins <tallysmartins@gmail.com> wrote: > CC-ing my work-email There are some issues with it and mailing lists; I'll hopefully sort them out in the next weeks. > > The driver currently has no devicetree documentation. This commit adds a > > devicetree folder and documentation for it. Documentation must be moved > > as well when the driver gets out of staging. > > > > Signed-off-by: Tallys Martins <tallysmartins@gmail.com> > > Signed-off-by: Souza Guilherme <gdsdsilva@inf.ufpel.edu.br> > > Co-developed-by: Souza Guilherme <gdsdsilva@inf.ufpel.edu.br> > > Hi, > > There is no need for a direct relationship between a binding and a driver > at all. As such, we normally don't take binding documents in staging. > > Just put it directly in it's final destination. The driver can catch > up with it later! > > I'm still not that comfortable with yaml (haven't gotten around > to doing any conversions myself yet) so I'll be looking for additional > review on these from others. Personal note: I also don't like YAML, but that may be me feeling old. I'm not that old to prefer XML, but old enough to prefer JSON. I am still trying to accommodate my taste/feeling for the format, even though I've been using it sporadically for ~5 years now in various other elements. Travis-CI may be the biggest user of it. Looks like Ruby users were the biggest pushers/initiators of YAML, and Python people seem to have jumped in shortly after. Oh well: ¯\_(ツ)_/¯ > > A few comments inline. > > > --- > > .../Documentation/devicetree/ad2s1210.yaml | 41 +++++++++++++++++++ > > 1 file changed, 41 insertions(+) > > create mode 100644 drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml Maybe also remove the old txt binding if adding this new one. No need to keep them both. > > > > diff --git a/drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml b/drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml > > new file mode 100644 > > index 000000000000..733aa07b4626 > > --- /dev/null > > +++ b/drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml > > @@ -0,0 +1,41 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/iio/iio/ad2s1210.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: | > > + Analog Devices Inc. AD2S1210 10-Bit to 16-Bit R/D Converters > > + > > +maintainers: > > + - Graff Yang <graff.yang@gmail.com> > I would check that one with the Analog devices team. I guess, add (here): Michael Hennerich <michael.hennerich@analog.com> > > > + > > +description: | > > + Analog Devices AD2S1210 Resolver to Digital SPI driver > > + > > + https://www.analog.com/en/products/ad2s1210.html > > + > > +properties: > > + compatible: > > + enum: > > + - adi,ad2s1210 These 2 are required here, since this chip operates in SPI mode 3. spi-cpha: true spi-cpol: true Also, from the driver, some GPIOs should also be documented: static const struct ad2s1210_gpio gpios[] = { [AD2S1210_SAMPLE] = { .name = "adi,sample", .flags = GPIOD_OUT_LOW }, [AD2S1210_A0] = { .name = "adi,a0", .flags = GPIOD_OUT_LOW }, [AD2S1210_A1] = { .name = "adi,a1", .flags = GPIOD_OUT_LOW }, [AD2S1210_RES0] = { .name = "adi,res0", .flags = GPIOD_OUT_LOW }, [AD2S1210_RES1] = { .name = "adi,res1", .flags = GPIOD_OUT_LOW }, }; So, adi,sample-gpios: adi,a0-gpios: adi,a1-gpios: adi,res0-gpios: adi,res10-gpios: These also look like they are required. > > + > > + reg: > > + maxItems: 1 > > + > > + clock-frequency: > > + minimum: 2000 > > + maximum: 20000 > > + default: 8192 > This doesn't look like a modern clock binding. > If we are going to end up changing this, then we should probably delay > having a binding doc until after that change is made. > > We often only do binding docs for drivers in staging just before moving > them out so as to avoid this sort of issue. These clock properties are not needed here. > > > + > > +required: > > + - compatible > > + - reg > > + > > +examples: > > + - | > > + resolver@0 { > > + compatible = "adi,ad2s1210"; > > + reg = <0>; > An example is probably more useful if it includes all the optional properties > as well. This should also be included in an spi block: So: spi0 { resolver@0 { compatible = "adi,ad2s1210"; reg = <0>; spi-cpha; spi-cpol; // and then the adi,***-gpios I think here }; }; I don't think much more-else is needed for this driver. > > + }; > > +... > > Thanks, > > Jonathan > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] staging: iio: ad2s1210: Destroy mutex at remove 2019-05-18 22:15 [PATCH 1/2] staging: iio: ad2s1210: Destroy mutex at remove Tallys Martins 2019-05-18 22:15 ` [PATCH 2/2] staging: iio: ad2s1210: Add devicetree yaml doc Tallys Martins @ 2019-05-19 11:12 ` Jonathan Cameron 1 sibling, 0 replies; 5+ messages in thread From: Jonathan Cameron @ 2019-05-19 11:12 UTC (permalink / raw) To: Tallys Martins Cc: Lars-Peter Clausen, Michael Hennerich, Stefan Popa, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, linux-iio, devel, linux-kernel, kernel-usp, Souza Guilherme On Sat, 18 May 2019 19:15:57 -0300 Tallys Martins <tallysmartins@gmail.com> wrote: > Ensure the mutex will be destroyed on drive removal. > Also adds mutex comment description. > > Signed-off-by: Tallys Martins <tallysmartins@gmail.com> > Signed-off-by: Souza Guilherme <gdsdsilva@inf.ufpel.edu.br> > Co-developed-by: Souza Guilherme <gdsdsilva@inf.ufpel.edu.br> Hi. This particular 'issue' is never a simple one. The destroy_mutex call is intended for lock debugging only, which is why there is devm_init_mutex or similar to allow for automatic unwinding. It simple cleanup paths like this, it provides very little value and leads to a more complex unwind. It is for this reason that the vast majority of drivers simply don't bother. Hence, unless there is a good reason I'm missing I won't be introducing more of them. Thanks, Jonathan > --- > drivers/staging/iio/resolver/ad2s1210.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index b6be0bc202f5..b91cf57c5e57 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -86,7 +86,7 @@ static const struct ad2s1210_gpio gpios[] = { > static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 }; > > struct ad2s1210_state { > - struct mutex lock; > + struct mutex lock; /* lock to protect the state on r/w operations */ > struct spi_device *sdev; > struct gpio_desc *gpios[5]; > unsigned int fclkin; > @@ -689,8 +689,10 @@ static int ad2s1210_probe(struct spi_device *spi) > static int ad2s1210_remove(struct spi_device *spi) > { > struct iio_dev *indio_dev = spi_get_drvdata(spi); > + struct ad2s1210_state *ad2s1210_ad = iio_priv(indio_dev); > > iio_device_unregister(indio_dev); > + mutex_destroy(&ad2s1210_ad->lock); > > return 0; > } ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-05-20 10:18 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-18 22:15 [PATCH 1/2] staging: iio: ad2s1210: Destroy mutex at remove Tallys Martins 2019-05-18 22:15 ` [PATCH 2/2] staging: iio: ad2s1210: Add devicetree yaml doc Tallys Martins 2019-05-19 11:17 ` Jonathan Cameron 2019-05-20 10:17 ` Alexandru Ardelean 2019-05-19 11:12 ` [PATCH 1/2] staging: iio: ad2s1210: Destroy mutex at remove Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).