| Message ID | 20250408-asoc-gpio-v1-0-c0db9d3fd6e9@nxp.com |
|---|---|
| Headers | show |
| Series | ASoC: codec: Convert to GPIO descriptors | expand |
On Tue, Apr 08, 2025 at 09:40:00AM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > of_gpio.h is deprecated, update the driver to use GPIO descriptors. > - Use devm_gpiod_get_optional to get GPIO descriptor with default > polarity GPIOD_OUT_LOW, set consumer name. > - Use gpiod_set_value_cansleep to configure output value. > While at here > - reorder the included headers. > - Move cs42l56_platform_data from sound/cs42l56.h to driver code > - Drop sound/cs42l56.h because no user is creating the device using > platform data This is a good sign that things should be split into multiple patches. The series would probably be a little more manageable in general if it were done per driver. > Per datasheet, the DTS polarity should be GPIOD_ACTIVE_LOW. The binding > example use value 0(GPIOD_ACTIVE_HIGH) which seems wrong. There is > no in-tree DTS has the device, so all should be fine. This is the whole thing where gpiolib introducing inversion causing confusion.
On Tue, Apr 08, 2025 at 01:53:15PM +0100, Charles Keepax wrote: >On Tue, Apr 08, 2025 at 09:40:00AM +0800, Peng Fan (OSS) wrote: >> From: Peng Fan <peng.fan@nxp.com> >> Checking the current driver using legacy GPIO API, the >> nreset value is first output HIGH, then LOW, then HIGH. >> >> Checking the datasheet, nreset is should be held low after power >> on, when nreset is high, it starts to work. >> > >Does feel like it would have made more sense to request it in >reset at the start certainly, but as you say reasonable to leave >well enough alone. yeah. request it in reset state and set HIGH later is better. I could update to use this new flow. > >> Per datasheet, the DTS polarity should be GPIOD_ACTIVE_LOW. The binding >> example use value 0(GPIOD_ACTIVE_HIGH) which seems wrong. There is >> no in-tree DTS has the device, so all should be fine. > >Yeah it is technically wrong, discussed more below. > >> - pdata->gpio_nreset = of_get_named_gpio(np, "cirrus,gpio-nreset", 0); >> + pdata->gpio_nreset = devm_gpiod_get_optional(&i2c_client->dev, "cirrus,gpio-nreset", >> + GPIOD_OUT_LOW); > >Would be nice to call out that this part is already included in >the quirks array in of_find_gpio_rename: > >944004eb56dc ("gpiolib: of: add a quirk for reset line for Cirrus CS42L56") I will update commit log to include this. > >Took me a while to realise this would request the right property. My bad. > >> - gpio_set_value_cansleep(cs42l56->pdata.gpio_nreset, 0); >> - gpio_set_value_cansleep(cs42l56->pdata.gpio_nreset, 1); >> + gpiod_set_value_cansleep(cs42l56->pdata.gpio_nreset, 1); >> + gpiod_set_value_cansleep(cs42l56->pdata.gpio_nreset, 0); > >I can't say I super love this change as it will mean any users >with a DT that worked with the driver before this change will see >things break. As far as I know the parts you are updating in >this series do not have a lot of users, (and none in tree as you >note) so I guess if everyone else is happy, I don't really object. A polarity quirk could be added to gpiolib-of, if this is preferred. Before adding quirk, I would like to see whether Linus and Bartosz agree on this. BTW, [1] shows the chip is discontinued. Since there is no in-tree user for quite some time, and new users would not use end-of-life chips, should we totally delete this driver? [1] https://www.cirrus.com/products/cs42l56/ Thanks, Peng > >Thanks, >Charles >
of_gpio.h is deprecated, update the driver to use GPIO descriptors. Mostly about dropping legacy platform data usage, and switching to use devm_gpiod_get_optional to get GPIO descriptors and use gpiod_set_value to configure output. Because of lacking of DTS users, I checked datasheet especially for CS42Lxx and update code accordingly. I not have devices to test, so just my best effort to do this work. For cs42lxx codecs, there is no in-tree users for quite long time, I was thinking to remove the drivers. But in case people have concern, so I still do the convertion. For those that have in-tree uers, I have added Cc in each patch and appreciate if there is T-b from users. With this patchset post out for ASoC, the left one under ASoC is sound/arm/pxa2xx-ac97-lib.c which I have not looked into. For others, below patches are alreay in maillist for reviewing. ASoC: codec: sma1307: Remove including of_gpio.h ASoC: codec: wcd9335: Convert to GPIO descriptors ASoC: codec: wcd938x: Convert to GPIO descriptors ASoC: codec: wcd939x: Convert to GPIO descriptors ASoC: codec: ak5386: Convert to GPIO descriptors Signed-off-by: Peng Fan <peng.fan@nxp.com> --- Peng Fan (7): ASoC: codec: tlv320aic32x4: Drop aic32x4_pdata usage ASoC: codec: tlv320aic32x4: Convert to GPIO descriptors ASoC: codec: twl4030: Convert to GPIO descriptors ASoC: codec: cs42l56: Convert to GPIO descriptors ASoC: codec: cs42l73: Convert to GPIO descriptors ASoC: codec: cs42l52: Convert to GPIO descriptors ASoC: codec: tpa6130a2: Convert to GPIO descriptors MAINTAINERS | 1 - include/sound/cs42l52.h | 29 ----------- include/sound/cs42l56.h | 45 ---------------- include/sound/cs42l73.h | 19 ------- include/sound/tlv320aic32x4.h | 9 ---- include/sound/tpa6130a2-plat.h | 17 ------ sound/soc/codecs/cs42l52.c | 108 ++++++++++++++++++++------------------- sound/soc/codecs/cs42l56.c | 91 +++++++++++++++++++++------------ sound/soc/codecs/cs42l73.c | 81 ++++++++++++++--------------- sound/soc/codecs/tlv320aic32x4.c | 53 +++++++++---------- sound/soc/codecs/tpa6130a2.c | 54 ++++++-------------- sound/soc/codecs/twl4030.c | 76 +++++++++++---------------- 12 files changed, 220 insertions(+), 363 deletions(-) --- base-commit: 2bdde620f7f2bff2ff1cb7dc166859eaa0c78a7c change-id: 20250408-asoc-gpio-8862a7ae9090 Best regards,