| Message ID | db10e96fbda121e7456d70e97a013cbfc9755f4d.1737533954.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: soc-core: Stop using of_property_read_bool() for non-boolean properties | expand |
Hi Geert, Mark Thank you for the patch > On R-Car: > > OF: /sound: Read of boolean property 'simple-audio-card,bitclock-master' with a value. > OF: /sound: Read of boolean property 'simple-audio-card,frame-master' with a value. > > or: > > OF: /soc/sound@ec500000/ports/port@0/endpoint: Read of boolean property 'bitclock-master' with a value. > OF: /soc/sound@ec500000/ports/port@0/endpoint: Read of boolean property 'frame-master' with a value. > > The use of of_property_read_bool() for non-boolean properties is > deprecated in favor of of_property_present() when testing for property > presence. > > Replace testing for presence before calling of_property_read_u32() by > testing for an -EINVAL return value from the latter, to simplify the > code. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- (snip) > - if (of_property_read_bool(np, "dai-tdm-slot-num")) { > - ret = of_property_read_u32(np, "dai-tdm-slot-num", &val); > - if (ret) > - return ret; > - > - if (slots) > - *slots = val; > - } (snip) > + ret = of_property_read_u32(np, "dai-tdm-slot-num", &val); > + if (ret && ret != -EINVAL) > + return ret; > + if (!ret && slots) > + *slots = val; Looks good to me Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> If my understanding was correct, old/new code should have same behavior. But because of the original code, new code looks complex for me. The case which this function return error are (A) if property does not have a value (B) if the property data isn't large enough I think "DT checker" will indicates error for both case ? If so, we can simply ignore these 2 cases. Then, the code will be more simple ret = of_property_read_u32(np, "dai-tdm-slot-num", &val); - if (ret && ret != -EINVAL) - return ret; if (!ret && slots) *slots = val; I think this should be extra new patch (if people can agree about it). Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Morimoto-san, On Thu, Jan 23, 2025 at 12:43 AM Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > > On R-Car: > > > > OF: /sound: Read of boolean property 'simple-audio-card,bitclock-master' with a value. > > OF: /sound: Read of boolean property 'simple-audio-card,frame-master' with a value. > > > > or: > > > > OF: /soc/sound@ec500000/ports/port@0/endpoint: Read of boolean property 'bitclock-master' with a value. > > OF: /soc/sound@ec500000/ports/port@0/endpoint: Read of boolean property 'frame-master' with a value. > > > > The use of of_property_read_bool() for non-boolean properties is > > deprecated in favor of of_property_present() when testing for property > > presence. > > > > Replace testing for presence before calling of_property_read_u32() by > > testing for an -EINVAL return value from the latter, to simplify the > > code. > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > (snip) > > - if (of_property_read_bool(np, "dai-tdm-slot-num")) { > > - ret = of_property_read_u32(np, "dai-tdm-slot-num", &val); > > - if (ret) > > - return ret; > > - > > - if (slots) > > - *slots = val; > > - } > (snip) > > + ret = of_property_read_u32(np, "dai-tdm-slot-num", &val); > > + if (ret && ret != -EINVAL) > > + return ret; > > + if (!ret && slots) > > + *slots = val; > > Looks good to me > > Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Thank you! > If my understanding was correct, old/new code should have same behavior. Indeed, that was my objective... > But because of the original code, new code looks complex for me. > The case which this function return error are > > (A) if property does not have a value > (B) if the property data isn't large enough > > I think "DT checker" will indicates error for both case ? Correct, of_property_read_u32_array() would return -ENODATA resp. -EOVERFLOW. > If so, we can simply ignore these 2 cases. Then, the code will be more > simple > > ret = of_property_read_u32(np, "dai-tdm-slot-num", &val); > - if (ret && ret != -EINVAL) > - return ret; > if (!ret && slots) > *slots = val; > > I think this should be extra new patch (if people can agree about it). That would be a change in behavior. Probably it would be fine for existing users, though, as no existing DTS should cause these errors, else sound wouldn't work. For a new DTS, it would silently ignore errors. You are in a better position to make that decision, though. BTW, is there any specific reason the code always checks for the presence of "dai-tdm-slot-num", even if slots is NULL, and the result sn't used? I.e. would if (slots) { ret = of_property_read_u32(np, "dai-tdm-slot-num", &val); if (!ret) *slots = val; else if (ret != -EINVAL) return ret; } (perhaps dropping the else, as per above) be acceptable? Thanks! Gr{oetje,eeting}s, Geert
Hi Geert, Mark > > ret = of_property_read_u32(np, "dai-tdm-slot-num", &val); > > - if (ret && ret != -EINVAL) > > - return ret; > > if (!ret && slots) > > *slots = val; (snip) > That would be a change in behavior. Probably it would be fine for > existing users, though, as no existing DTS should cause these errors, > else sound wouldn't work. For a new DTS, it would silently ignore errors. > You are in a better position to make that decision, though. Thanks I will post that patch if Gerrt's patch was applied. > BTW, is there any specific reason the code always checks for the > presence of "dai-tdm-slot-num", even if slots is NULL, and the result > sn't used? I.e. would > > if (slots) { > ret = of_property_read_u32(np, "dai-tdm-slot-num", &val); > if (!ret) > *slots = val; > else if (ret != -EINVAL) > return ret; > } I'm not 100% sure, but -num is used by the driver which can select the number of TDM. Some driver uses default / fixed number. And -width too. So I think these can be independent. Thank you for your help !! Best regards --- Kuninori Morimoto
On Wed, Jan 22, 2025 at 2:21 AM Geert Uytterhoeven <geert+renesas@glider.be> wrote: > > On R-Car: > > OF: /sound: Read of boolean property 'simple-audio-card,bitclock-master' with a value. > OF: /sound: Read of boolean property 'simple-audio-card,frame-master' with a value. > > or: > > OF: /soc/sound@ec500000/ports/port@0/endpoint: Read of boolean property 'bitclock-master' with a value. > OF: /soc/sound@ec500000/ports/port@0/endpoint: Read of boolean property 'frame-master' with a value. > > The use of of_property_read_bool() for non-boolean properties is > deprecated in favor of of_property_present() when testing for property > presence. > > Replace testing for presence before calling of_property_read_u32() by > testing for an -EINVAL return value from the latter, to simplify the > code. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > Seen since commit c141ecc3cecd7647 ("of: Warn when > of_property_read_bool() is used on non-boolean properties") in > dt-rh/for-next. > > I could not exercise all code paths, so review/testing would be > appreciated. Thanks! > --- > sound/soc/soc-core.c | 32 +++++++++++++------------------- > 1 file changed, 13 insertions(+), 19 deletions(-) Acked-by: Rob Herring (Arm) <robh@kernel.org>
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 3c6d8aef4130901c..26b34b6885083908 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -3046,7 +3046,7 @@ int snd_soc_of_parse_pin_switches(struct snd_soc_card *card, const char *prop) unsigned int i, nb_controls; int ret; - if (!of_property_read_bool(dev->of_node, prop)) + if (!of_property_present(dev->of_node, prop)) return 0; strings = devm_kcalloc(dev, nb_controls_max, @@ -3120,23 +3120,17 @@ int snd_soc_of_parse_tdm_slot(struct device_node *np, if (rx_mask) snd_soc_of_get_slot_mask(np, "dai-tdm-slot-rx-mask", rx_mask); - if (of_property_read_bool(np, "dai-tdm-slot-num")) { - ret = of_property_read_u32(np, "dai-tdm-slot-num", &val); - if (ret) - return ret; - - if (slots) - *slots = val; - } - - if (of_property_read_bool(np, "dai-tdm-slot-width")) { - ret = of_property_read_u32(np, "dai-tdm-slot-width", &val); - if (ret) - return ret; + ret = of_property_read_u32(np, "dai-tdm-slot-num", &val); + if (ret && ret != -EINVAL) + return ret; + if (!ret && slots) + *slots = val; - if (slot_width) - *slot_width = val; - } + ret = of_property_read_u32(np, "dai-tdm-slot-width", &val); + if (ret && ret != -EINVAL) + return ret; + if (!ret && slot_width) + *slot_width = val; return 0; } @@ -3403,12 +3397,12 @@ unsigned int snd_soc_daifmt_parse_clock_provider_raw(struct device_node *np, * check "[prefix]frame-master" */ snprintf(prop, sizeof(prop), "%sbitclock-master", prefix); - bit = of_property_read_bool(np, prop); + bit = of_property_present(np, prop); if (bit && bitclkmaster) *bitclkmaster = of_parse_phandle(np, prop, 0); snprintf(prop, sizeof(prop), "%sframe-master", prefix); - frame = of_property_read_bool(np, prop); + frame = of_property_present(np, prop); if (frame && framemaster) *framemaster = of_parse_phandle(np, prop, 0);
On R-Car: OF: /sound: Read of boolean property 'simple-audio-card,bitclock-master' with a value. OF: /sound: Read of boolean property 'simple-audio-card,frame-master' with a value. or: OF: /soc/sound@ec500000/ports/port@0/endpoint: Read of boolean property 'bitclock-master' with a value. OF: /soc/sound@ec500000/ports/port@0/endpoint: Read of boolean property 'frame-master' with a value. The use of of_property_read_bool() for non-boolean properties is deprecated in favor of of_property_present() when testing for property presence. Replace testing for presence before calling of_property_read_u32() by testing for an -EINVAL return value from the latter, to simplify the code. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- Seen since commit c141ecc3cecd7647 ("of: Warn when of_property_read_bool() is used on non-boolean properties") in dt-rh/for-next. I could not exercise all code paths, so review/testing would be appreciated. Thanks! --- sound/soc/soc-core.c | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-)