| Message ID | 20211229114457.4101989-2-festevam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] ASoC: cs4265: Fix part number ID error message | expand |
On Wed, Dec 29, 2021 at 08:44:56AM -0300, Fabio Estevam wrote: > From: Fabio Estevam <festevam@denx.de> > > Currently, the reset_gpio polarity handling is done backwards. > > The gpiod API takes the logic value of the GPIO, not the physical one. > > As per the CS4265 datasheet: > > "When RESET is low, the CS4265 enters a low-power mode and all > internal states are reset" > > If a devicetree describes reset_gpio as active-low, the correct behaviour > would be to retrieve the GPIO and put in its active state (GPIOD_OUT_HIGH) > and then move it to its inactive state so that it can be operational > (logic level 0). > > Fix it accordingly. > > Fixes: fb6f806967f6 ("ASoC: Add support for the CS4265 CODEC") > Signed-off-by: Fabio Estevam <festevam@denx.de> > --- > Changes since v1: > - Newly introduced patch. > > sound/soc/codecs/cs4265.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/codecs/cs4265.c b/sound/soc/codecs/cs4265.c > index b89002189a2b..294fa7ac16cb 100644 > --- a/sound/soc/codecs/cs4265.c > +++ b/sound/soc/codecs/cs4265.c > @@ -590,13 +590,13 @@ static int cs4265_i2c_probe(struct i2c_client *i2c_client, > } > > cs4265->reset_gpio = devm_gpiod_get_optional(&i2c_client->dev, > - "reset", GPIOD_OUT_LOW); > + "reset", GPIOD_OUT_HIGH); > if (IS_ERR(cs4265->reset_gpio)) > return PTR_ERR(cs4265->reset_gpio); > > if (cs4265->reset_gpio) { > mdelay(1); > - gpiod_set_value_cansleep(cs4265->reset_gpio, 1); > + gpiod_set_value_cansleep(cs4265->reset_gpio, 0); Hmm... I might defer to Mark on this one. I totally agree your new code is more correct, however, I would have a slight worry about existing device trees not correctly specifying the GPIO. As in if existing systems had been specifying the GPIO correctly they are presumably currently broken. But I am not sure what the obvious solution would be, and I don't really have a good feel for how widely used this part is. Thanks, Charles
Hi Charles, On Wed, Dec 29, 2021 at 8:54 AM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > Hmm... I might defer to Mark on this one. I totally agree your > new code is more correct, however, I would have a slight worry > about existing device trees not correctly specifying the GPIO. As > in if existing systems had been specifying the GPIO correctly > they are presumably currently broken. But I am not sure what the > obvious solution would be, and I don't really have a good feel > for how widely used this part is. I could not find a single user of the cs4265 in linux-next. The board I have does not connect a GPIO to the CS4264 reset line, so I am not affected by it. Cheers
On Wed, Dec 29, 2021 at 09:04:19AM -0300, Fabio Estevam wrote: > I could not find a single user of the cs4265 in linux-next. > The board I have does not connect a GPIO to the CS4264 reset line, so > I am not affected by it. There might be more out of tree users of course - there's no requirement for people to upstream their DTs. Probably better to play it safe.
Hi Mark, On Wed, Dec 29, 2021 at 9:43 AM Mark Brown <broonie@kernel.org> wrote: > There might be more out of tree users of course - there's no requirement > for people to upstream their DTs. Probably better to play it safe. If someone correctly describes the gpio_reset as active-low, then the driver will not work. If there is any out-of-tree user of the gpio_reset property, they are passing the incorrect polarity in the device tree and things are working by pure luck. (wrong polarity in dts + wrong polarity handling in the driver = working state) Ok, if this can't be fixed, then let's drop patches 2 and 3, which is unfortunate.
On Wed, Dec 29, 2021 at 10:11:57AM -0300, Fabio Estevam wrote: > On Wed, Dec 29, 2021 at 9:43 AM Mark Brown <broonie@kernel.org> wrote: > > > There might be more out of tree users of course - there's no requirement > > for people to upstream their DTs. Probably better to play it safe. > > If someone correctly describes the gpio_reset as active-low, then the > driver will not work. > > If there is any out-of-tree user of the gpio_reset property, they are > passing the incorrect polarity in the device tree > and things are working by pure luck. (wrong polarity in dts + wrong > polarity handling in the driver = working state) Yeah a fair bit of this sort of thing kinda happened in the early gpiod conversion, and it is indeed a bit sad. > > Ok, if this can't be fixed, then let's drop patches 2 and 3, which is > unfortunate. I think we can still keep patch 3 here, still valuable to put the device back into reset, even if we are using the backwards reset. Thanks, Charles
On Wed, Dec 29, 2021 at 10:11:57AM -0300, Fabio Estevam wrote: > On Wed, Dec 29, 2021 at 9:43 AM Mark Brown <broonie@kernel.org> wrote: > > There might be more out of tree users of course - there's no requirement > > for people to upstream their DTs. Probably better to play it safe. > If someone correctly describes the gpio_reset as active-low, then the > driver will not work. > If there is any out-of-tree user of the gpio_reset property, they are > passing the incorrect polarity in the device tree > and things are working by pure luck. (wrong polarity in dts + wrong > polarity handling in the driver = working state) To be honest I think the transparent polarity handling in the GPIO bindings was a mistake, it's caused no end of problems especially with signals like this which are active low - it's a nasty gotcha for conversion to descriptors. > Ok, if this can't be fixed, then let's drop patches 2 and 3, which is > unfortunate. Why drop patch 3?
diff --git a/sound/soc/codecs/cs4265.c b/sound/soc/codecs/cs4265.c index b89002189a2b..294fa7ac16cb 100644 --- a/sound/soc/codecs/cs4265.c +++ b/sound/soc/codecs/cs4265.c @@ -590,13 +590,13 @@ static int cs4265_i2c_probe(struct i2c_client *i2c_client, } cs4265->reset_gpio = devm_gpiod_get_optional(&i2c_client->dev, - "reset", GPIOD_OUT_LOW); + "reset", GPIOD_OUT_HIGH); if (IS_ERR(cs4265->reset_gpio)) return PTR_ERR(cs4265->reset_gpio); if (cs4265->reset_gpio) { mdelay(1); - gpiod_set_value_cansleep(cs4265->reset_gpio, 1); + gpiod_set_value_cansleep(cs4265->reset_gpio, 0); } i2c_set_clientdata(i2c_client, cs4265);