[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(-)

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

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?