Hi Cheng-Yi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on asoc/for-next] [also build test ERROR on v4.19-rc4 next-20180919] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Cheng-Yi-Chiang/ASoC-max9892x-Add-d... base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next config: x86_64-randconfig-s3-09201857 (attached as .config) compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=x86_64

All errors (new ones prefixed by >>):

sound/soc/codecs/max98927.c: In function 'max98927_i2c_toggle_reset':

...

sound/soc/codecs/max98927.c:884:2: error: implicit declaration of function 'gpiod_set_value_cansleep'; did you mean 'gpio_set_value_cansleep'? [-Werror=implicit-function-declaration]

gpiod_set_value_cansleep(max98927->reset_gpio, 1); ^~~~~~~~~~~~~~~~~~~~~~~~ gpio_set_value_cansleep

...

sound/soc/codecs/max98927.c:902:2: error: implicit declaration of function 'devm_gpiod_put'; did you mean 'devm_gpio_free'? [-Werror=implicit-function-declaration]

devm_gpiod_put(dev, max98927->reset_gpio); ^~~~~~~~~~~~~~ devm_gpio_free sound/soc/codecs/max98927.c: In function 'max98927_i2c_probe':

...

sound/soc/codecs/max98927.c:950:25: error: implicit declaration of function 'devm_gpiod_get_optional'; did you mean 'devm_gpio_request_one'? [-Werror=implicit-function-declaration]

max98927->reset_gpio = devm_gpiod_get_optional( ^~~~~~~~~~~~~~~~~~~~~~~ devm_gpio_request_one

...

sound/soc/codecs/max98927.c:951:24: error: 'GPIOD_OUT_LOW' undeclared (first use in this function); did you mean 'GPIOF_INIT_LOW'?

&i2c->dev, "reset", GPIOD_OUT_LOW); ^~~~~~~~~~~~~ GPIOF_INIT_LOW sound/soc/codecs/max98927.c:951:24: note: each undeclared identifier is reported only once for each function it appears in cc1: some warnings being treated as errors

vim +884 sound/soc/codecs/max98927.c

873 874 static int max98927_i2c_toggle_reset(struct device *dev, 875 struct max98927_priv *max98927) 876 { 877 /* 878 * If we do not have reset gpio, assume platform firmware 879 * controls the regulator and toggles it for us. 880 */ 881 if (!max98927->reset_gpio) 882 return 0; 883

884 gpiod_set_value_cansleep(max98927->reset_gpio, 1);

885 886 /* 887 * We need to wait a bit before we are allowed to release reset GPIO. 888 */ 889 usleep_range(MAX98927_MIN_RESET_US, MAX98927_MIN_RESET_US + 5); 890 891 gpiod_set_value_cansleep(max98927->reset_gpio, 0); 892 893 /* 894 * We need to wait a bit before I2C communication is available. 895 */ 896 usleep_range(MAX98927_RELEASE_RESET_DELAY_US, 897 MAX98927_RELEASE_RESET_DELAY_US + 5); 898 899 /* 900 * Release reset GPIO because we are not going to use it. 901 */

902 devm_gpiod_put(dev, max98927->reset_gpio);

903 904 return 0; 905 } 906 907 static bool max98927_is_first_to_reset(struct max98927_priv *max98927) 908 { 909 struct max98927_priv *p; 910 911 if (!max98927->reset_gpio) 912 return false; 913 914 list_for_each_entry(p, &reset_list, list) { 915 if (max98927->reset_gpio == p->reset_gpio) 916 return false; 917 } 918 919 return true; 920 } 921 922 static int max98927_i2c_probe(struct i2c_client *i2c, 923 const struct i2c_device_id *id) 924 { 925 926 int ret = 0, value; 927 int reg = 0; 928 struct max98927_priv *max98927 = NULL; 929 930 max98927 = devm_kzalloc(&i2c->dev, 931 sizeof(*max98927), GFP_KERNEL); 932 933 if (!max98927) { 934 ret = -ENOMEM; 935 return ret; 936 } 937 i2c_set_clientdata(i2c, max98927); 938 939 /* update interleave mode info */ 940 if (!of_property_read_u32(i2c->dev.of_node, 941 "interleave_mode", &value)) { 942 if (value > 0) 943 max98927->interleave_mode = 1; 944 else 945 max98927->interleave_mode = 0; 946 } else 947 max98927->interleave_mode = 0; 948 949 /* Gets optional GPIO for reset line. */

950 max98927->reset_gpio = devm_gpiod_get_optional( 951 &i2c->dev, "reset", GPIOD_OUT_LOW);

952 if (IS_ERR(max98927->reset_gpio)) { 953 ret = PTR_ERR(max98927->reset_gpio); 954 dev_err(&i2c->dev, "error getting reset gpio: %d\n", ret); 955 return ret; 956 } 957 958 /* 959 * Only toggle reset line for the first instance when the 960 * reset line is shared among instances. For example, 961 * left and right amplifier share the same reset line, and 962 * we should only toggle the reset line once. 963 */ 964 if (max98927_is_first_to_reset(max98927)) { 965 dev_info(&i2c->dev, "%s: toggle reset line\n", __func__); 966 ret = max98927_i2c_toggle_reset(&i2c->dev, max98927); 967 if (ret) 968 return ret; 969 } 970 971 /* regmap initialization */ 972 max98927->regmap 973 = devm_regmap_init_i2c(i2c, &max98927_regmap); 974 if (IS_ERR(max98927->regmap)) { 975 ret = PTR_ERR(max98927->regmap); 976 dev_err(&i2c->dev, 977 "Failed to allocate regmap: %d\n", ret); 978 return ret; 979 } 980 981 /* Check Revision ID */ 982 ret = regmap_read(max98927->regmap, 983 MAX98927_R01FF_REV_ID, &reg); 984 if (ret < 0) { 985 dev_err(&i2c->dev, 986 "Failed to read: 0x%02X\n", MAX98927_R01FF_REV_ID); 987 return ret; 988 } 989 dev_info(&i2c->dev, "MAX98927 revisionID: 0x%02X\n", reg); 990 991 /* voltage/current slot configuration */ 992 max98927_slot_config(i2c, max98927); 993 994 /* codec registeration */ 995 ret = devm_snd_soc_register_component(&i2c->dev, 996 &soc_component_dev_max98927, 997 max98927_dai, ARRAY_SIZE(max98927_dai)); 998 if (ret < 0) 999 dev_err(&i2c->dev, "Failed to register component: %d\n", ret); 1000 1001 list_add(&max98927->list, &reset_list); 1002 1003 return ret; 1004 } 1005

--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation

On Fri, Oct 12, 2018 at 12:05:16PM +0200, Philipp Zabel wrote:

Hi Cheng-yi,

[adding Maxime, devicetree to Cc:, the old discussion about GPIO resets in [4] has never been resolved]

On Tue, 2018-10-09 at 21:46 +0800, Cheng-yi Chiang wrote:

...

+reset controller maintainer Philipp

Hi Mark, Sorry for the late reply. It took me a while to investigate reset controller and its possible usage. I would like to figure out the proper way of reset handling because it is common to have a shared reset line for two max98927 codecs for left and right channels. Without supporting this usage, a simple reset-gpios change for single codec would not be useful, and perhaps will be duplicated if reset controller is a better way.

Hi Philipp, I would like to seek your advice about whether we can use reset controller to support the use case where multiple devices share the same reset line.

Let me summarize the use case: There are two max98927 codecs sharing the same reset line. The reset line is controlled by a GPIO. The goal is to toggle reset line once and only once.

There was a similar scenario in tlv320aic3x codec driver [1]. A static list is used in the codec driver so probe function can know whether it is needed to toggle reset line. Mark pointed out that it is not suitable to handle the shared reset line inside codec driver. A point is that this only works for multiple devices using the same device driver. He suggested to look into reset controller so I searched through the usage for common reset line.

Here I found a shared reset line use case [2]. With the patch [2], reset_control_reset can support this “reset once and for all” use case.

This patch was applied as 7da33a37b48f. So the reset framework has support for shared reset controls where only the first reset request actually causes a reset pulse, and all others are no-ops.

...

However, I found that there are some missing pieces:

Let’s assume there are two codecs c1 and c2 sharing a reset line controlled by GPIO.

c1’s spec: Hold time: The minimum time to assert the reset line is t_a1. Release time: The minimum time to access the chip after deassert of the reset line is t_d1.

c2’s spec: Hold time: The minimum time to assert the reset line is t_a2. Release time: The minimum time to access the chip after deassert of the reset line is t_d2.

For both c1 and c2 to work properly, we need a reset controller that can assert the reset line for T = max(t_a1, t_a2).

1. We need reset controller to support GPIO.

I still don't like the idea of describing a separate gpio reset controller "device" in DT very much, as this is really just a software abstraction, not actual hardware. For example:

gpio_reset: reset-controller { compatible = "gpio-reset"; reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>, <&gpio1 16 GPIO_ACTIVE_HIGH>; reset-delays-us = <10000>, <500>; };

c1 { resets = <&gpio_reset 0>; /* maps to <&gpio0 15 ...> */ };

c2 { resets = <&gpio_reset 0>; };

What I would like better would be to let the consumers keep their reset- gpios bindings, but add an optional hold time override in the DT:

c1 { reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>; reset-delays-us = <10000>; };

c2 { reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>; reset-delays-us = <10000>; };

The reset framework could create a reset_control backed by a gpio instead of a rcdev. I have an unfinished patch for this, but without the sharing requirement adding the reset framework abstraction between gpiod and drivers never seemed really worth it.

I don't remember the exact details of our past discussion, but I (still) don't really think that this would work well. I see two main shortcomings with that approach:

- I guess that the main reason you want to do that would be to have easy DT backward compatibility. Yet, the addition of the reset-delay-us would break that compatibility, since drivers that would have been converted now need to have it somehow, but older DTs wouldn't have it

- There's so many variations of the reset-gpios property name that you wouldn't be able to cover all the cases anyhow, at least without breaking the compatibility (again).

But I also see your point, and you're right that converting everyone to a gpio-reset node will not happen (even though I'd still really like to have that binding).

What about having a function that would be called by the consumer to instantiate that reset controller from a GPIO for us, instead of trying to do it automatically? That function could take the property name that holds the GPIO, which would cover the second drawback, and the delay, which would cover the first.

And in order to cover shared GPIO reset lines, we could just look at the one already created by other drivers, and if one has been created, we just give the reference to that one instead of creating it.

Does that make sense?

Maxime

Hi Philipp,

On Thu, Oct 18, 2018 at 1:02 AM Philipp Zabel p.zabel@pengutronix.de wrote:

Hi Maxime,

On Fri, 2018-10-12 at 15:46 +0200, Maxime Ripard wrote:

...

On Fri, Oct 12, 2018 at 12:05:16PM +0200, Philipp Zabel wrote:

[...]

...

...

What I would like better would be to let the consumers keep their reset- gpios bindings, but add an optional hold time override in the DT:

c1 {
        reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>;
        reset-delays-us = <10000>;
};

c2 {
        reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>;
        reset-delays-us = <10000>;
};

The reset framework could create a reset_control backed by a gpio instead of a rcdev. I have an unfinished patch for this, but without the sharing requirement adding the reset framework abstraction between gpiod and drivers never seemed really worth it.

I don't remember the exact details of our past discussion, but I (still) don't really think that this would work well.

It has been a while :) Thanks for jumping back in.

...

I see two main shortcomings with that approach:

• I guess that the main reason you want to do that would be to have easy DT backward compatibility.

Yes, that is true. The other reason is that I'd like devices to have a single binding, regardless of whether somebody decided to put them onto a board with shared reset lines.

I'd find it hard to advocate for changing the thankfully common case of device-exclusive reset gpios from:

    some-device {
            reset-gpios = <&gpiox y>;
    };

to:

    rstc: reset-controller {
            compatible = "gpio-reset";
            reset-gpios <&gpiox y>;
    };

    some-device {
            resets = <&rstc 0>;
    };

even for new bindings.

If the reset framework only supports the latter, and not the former, drivers for devices which already have reset-gpios would have to handle both reset_control and gpiod functionality, which I think should not be necessary.

Note that this is not really an argument against a "gpio-reset" controller driver, but an argument for reset-gpios support integrated into the reset framework. My argument against a "gpio-reset" device node in the DT is only that it is basically a virtual device that would only be used to work around missing reset-gpios support in the reset framework. Physically, this "device" consists of no more than a few PCB traces.

...

Yet, the addition of the
reset-delay-us would break that compatibility, since drivers that
would have been converted now need to have it somehow, but older
DTs wouldn't have it

That is why such a property would have to be optional, and the drivers would have to keep providing the reset delay themselves, as they currently do when handling GPIOs directly. It would only serve to override the driver default in case of additional delay requirements due to board specifics (such as delay elements, or shared resets).

...

• There's so many variations of the reset-gpios property name that you wouldn't be able to cover all the cases anyhow, at least without breaking the compatibility (again).

This is true, and I do not have a solution for this. Especially for those cases that don't use gpiod yet.

    of_get_named_gpio(node, "reset-gpio")

is still quite common, but I'd really like on gpiolib for polarity support.

...

But I also see your point, and you're right that converting everyone to a gpio-reset node will not happen (even though I'd still really like to have that binding).

See above. I'd be a lot less reluctant to support this binding if somebody could demonstrate a real gpio controlled reset controller device of some kind. And even then I would not want to have to use this device just to connect a single GPIO with a single reset input.

...

What about having a function that would be called by the consumer to instantiate that reset controller from a GPIO for us, instead of trying to do it automatically? That function could take the property name that holds the GPIO, which would cover the second drawback, and the delay, which would cover the first.

I like this idea.

I'd like to avoid having to fall back from gpiod to of_get_named_gpio if possible, so maybe we'd have to extend gpiolib-of.c with an of_find_reset_gpio function, as is done for SPI and regulators already.

We probably would have to support delay ranges. I see a lot of usleep_range calls between reset GPIO toggles in the drivers.

This looks great.

I am not sure what is the plan proceeding from here. Are you going to implement this feature with the unfinished patch you mentioned to create reset_control backed by a GPIO? I am willing to help. If you have patch I can help testing it. It will be easier to test the shared reset line on my side since the board I am working now has this use case.

If you don't have time for this, I can work on it based on your WIP patch. In that case could you please post the patch ? I just want to make sure I am on the right direction solving this without making duplicate efforts..

...

And in order to cover shared GPIO reset lines, we could just look at the one already created by other drivers, and if one has been created, we just give the reference to that one instead of creating it.

Does that make sense?

I'm not quite sure how to match an already requested reset control from the list given of_node and gpio property name at the moment - this might involve exporting gpiod_find from gpiolib, but API wise I think this is a sane proposal.

I would not want to add reset controller devices for each of these GPIO reset controls, but rather store the gpio_desc reference in the reset_control instead of the rcdev reference.

This plan to support shared reset line makes sense.

Thanks a lot!

regards Philipp

On Wed, Sep 12, 2018 at 8:28 PM Mark Brown broonie@kernel.org wrote:

On Wed, Sep 12, 2018 at 08:19:54PM +0800, Cheng-Yi Chiang wrote:

...

max98927 codec driver will support reset-gpio binding so it can toggle reset line in its probe function.

All GPIO properties are supposed to end -gpios even if there can only ever be a single GPIO.

Hi Mark. I see. I will update this along with the comments of the change in max98927.c if there is any. Thanks!