[PATCH v2 1/3] ASoC: cs4265: Fix part number ID error message
From: Fabio Estevam festevam@denx.de
The Chip ID - Register 01h contains the following description as per the CS4265 datasheet:
"Bits 7 through 4 are the part number ID, which is 1101b (0Dh)"
The current error message is incorrect as it prints CS4265_CHIP_ID, which is the register number, instead of printing the expected part number ID value.
To make it clearer, also do a shift by 4, so that the error message would become:
[ 4.218083] cs4265 1-004f: CS4265 Part Number ID: 0x0 Expected: 0xd
Signed-off-by: Fabio Estevam festevam@denx.de Acked-by: Charles Keepax ckeepax@opensource.cirrus.com --- Changes since v1: - None
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 cffd6111afac..b89002189a2b 100644 --- a/sound/soc/codecs/cs4265.c +++ b/sound/soc/codecs/cs4265.c @@ -611,8 +611,8 @@ static int cs4265_i2c_probe(struct i2c_client *i2c_client, if (devid != CS4265_CHIP_ID_VAL) { ret = -ENODEV; dev_err(&i2c_client->dev, - "CS4265 Device ID (%X). Expected %X\n", - devid, CS4265_CHIP_ID); + "CS4265 Part Number ID: 0x%x Expected: 0x%x\n", + devid >> 4, CS4265_CHIP_ID_VAL >> 4); return ret; } dev_info(&i2c_client->dev,
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); }
i2c_set_clientdata(i2c_client, cs4265);
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?
From: Fabio Estevam festevam@denx.de
When the reset_gpio GPIO is used, it is better to put the codec back into reset state when the driver unbinds.
Add a remove() function to accomplish that.
Suggested-by: Charles Keepax ckeepax@opensource.cirrus.com Signed-off-by: Fabio Estevam festevam@denx.de --- Changes since v1: - Newly introduced patch.
sound/soc/codecs/cs4265.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/sound/soc/codecs/cs4265.c b/sound/soc/codecs/cs4265.c index 294fa7ac16cb..8fa166e4b2a9 100644 --- a/sound/soc/codecs/cs4265.c +++ b/sound/soc/codecs/cs4265.c @@ -626,6 +626,16 @@ static int cs4265_i2c_probe(struct i2c_client *i2c_client, ARRAY_SIZE(cs4265_dai)); }
+static int cs4265_i2c_remove(struct i2c_client *i2c) +{ + struct cs4265_private *cs4265 = i2c_get_clientdata(i2c); + + if (cs4265->reset_gpio) + gpiod_set_value_cansleep(cs4265->reset_gpio, 1); + + return 0; +} + static const struct of_device_id cs4265_of_match[] = { { .compatible = "cirrus,cs4265", }, { } @@ -645,6 +655,7 @@ static struct i2c_driver cs4265_i2c_driver = { }, .id_table = cs4265_id, .probe = cs4265_i2c_probe, + .remove = cs4265_i2c_remove, };
module_i2c_driver(cs4265_i2c_driver);
participants (3)
-
Charles Keepax -
Fabio Estevam -
Mark Brown