| Message ID | 20250408-asoc-gpio-v1-4-c0db9d3fd6e9@nxp.com (mailing list archive) |
|---|---|
| State | New |
| 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> > 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. > 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") Took me a while to realise this would request the right property. > - 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. Thanks, Charles
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 >
On Tue, Apr 08, 2025 at 03:24:35PM +0100, Mark Brown wrote: >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. Sure. Just drop this patchset, I will do the patches per driver and split the changes to a single driver into multiple small patches. > >> 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. Let's see whether Linus and Bartosz prefer a polarity quirk for this. But honestly, I am thinking if might be better to remove the drivers that for end-of-life chips and no users for quite long time(years?). Thanks, Peng
On Wed, Apr 09, 2025 at 12:04:08AM +0800, Peng Fan wrote: > On Tue, Apr 08, 2025 at 03:24:35PM +0100, Mark Brown wrote: > >On Tue, Apr 08, 2025 at 09:40:00AM +0800, Peng Fan (OSS) wrote: > >> From: Peng Fan <peng.fan@nxp.com> > >> 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. Indeed it comes up pretty often, even worse with ACPI, at least in DT we can generally make new things use the flag. > Let's see whether Linus and Bartosz prefer a polarity quirk for this. Yeah happy to wait see what Linus and Bartosz think, as noted I am ok either way really, just mostly wanted to make sure we were making a concious choice. > But honestly, I am thinking if might be better to remove the drivers > that for end-of-life chips and no users for quite long time(years?). As for removing the drivers, I don't have a great objection to that they are all end of life products with no in tree users. But on the flip side the cost of keeping them seems fairly low here. Personally, I would be inclined to keep the driver and just leave the polarity stuff as is. That should mean any out of tree users remain in what ever state they are currently in and if a user turns up wanting it one way or the other then an in tree user probably wins. Thanks, Charles
diff --git a/include/sound/cs42l56.h b/include/sound/cs42l56.h deleted file mode 100644 index 62e9f7a3b414f6d1bcb651b22f7f8bd1f29b0eb3..0000000000000000000000000000000000000000 --- a/include/sound/cs42l56.h +++ /dev/null @@ -1,45 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -/* - * linux/sound/cs42l56.h -- Platform data for CS42L56 - * - * Copyright (c) 2014 Cirrus Logic Inc. - */ - -#ifndef __CS42L56_H -#define __CS42L56_H - -struct cs42l56_platform_data { - - /* GPIO for Reset */ - unsigned int gpio_nreset; - - /* MICBIAS Level. Check datasheet Pg48 */ - unsigned int micbias_lvl; - - /* Analog Input 1A Reference 0=Single 1=Pseudo-Differential */ - unsigned int ain1a_ref_cfg; - - /* Analog Input 2A Reference 0=Single 1=Pseudo-Differential */ - unsigned int ain2a_ref_cfg; - - /* Analog Input 1B Reference 0=Single 1=Pseudo-Differential */ - unsigned int ain1b_ref_cfg; - - /* Analog Input 2B Reference 0=Single 1=Pseudo-Differential */ - unsigned int ain2b_ref_cfg; - - /* Charge Pump Freq. Check datasheet Pg62 */ - unsigned int chgfreq; - - /* HighPass Filter Right Channel Corner Frequency */ - unsigned int hpfb_freq; - - /* HighPass Filter Left Channel Corner Frequency */ - unsigned int hpfa_freq; - - /* Adaptive Power Control for LO/HP */ - unsigned int adaptive_pwr; - -}; - -#endif /* __CS42L56_H */ diff --git a/sound/soc/codecs/cs42l56.c b/sound/soc/codecs/cs42l56.c index aaf90c8b7339dc7d9fa469048a56f38dca1797cd..8223e22dd1fea68e746151e637b611100f4e1a6e 100644 --- a/sound/soc/codecs/cs42l56.c +++ b/sound/soc/codecs/cs42l56.c @@ -7,32 +7,66 @@ * Author: Brian Austin <brian.austin@cirrus.com> */ +#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/init.h> +#include <linux/input.h> +#include <linux/i2c.h> +#include <linux/kernel.h> #include <linux/module.h> #include <linux/moduleparam.h> -#include <linux/kernel.h> -#include <linux/init.h> -#include <linux/delay.h> +#include <linux/of.h> +#include <linux/platform_device.h> #include <linux/pm.h> -#include <linux/i2c.h> -#include <linux/input.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h> #include <linux/slab.h> #include <linux/workqueue.h> -#include <linux/platform_device.h> -#include <linux/regulator/consumer.h> -#include <linux/of.h> -#include <linux/of_gpio.h> #include <sound/core.h> +#include <sound/initval.h> #include <sound/pcm.h> #include <sound/pcm_params.h> #include <sound/soc.h> #include <sound/soc-dapm.h> -#include <sound/initval.h> #include <sound/tlv.h> -#include <sound/cs42l56.h> #include "cs42l56.h" #define CS42L56_NUM_SUPPLIES 3 + +struct cs42l56_platform_data { + + /* GPIO for Reset */ + struct gpio_desc *gpio_nreset; + + /* MICBIAS Level. Check datasheet Pg48 */ + unsigned int micbias_lvl; + + /* Analog Input 1A Reference 0=Single 1=Pseudo-Differential */ + unsigned int ain1a_ref_cfg; + + /* Analog Input 2A Reference 0=Single 1=Pseudo-Differential */ + unsigned int ain2a_ref_cfg; + + /* Analog Input 1B Reference 0=Single 1=Pseudo-Differential */ + unsigned int ain1b_ref_cfg; + + /* Analog Input 2B Reference 0=Single 1=Pseudo-Differential */ + unsigned int ain2b_ref_cfg; + + /* Charge Pump Freq. Check datasheet Pg62 */ + unsigned int chgfreq; + + /* HighPass Filter Right Channel Corner Frequency */ + unsigned int hpfb_freq; + + /* HighPass Filter Left Channel Corner Frequency */ + unsigned int hpfa_freq; + + /* Adaptive Power Control for LO/HP */ + unsigned int adaptive_pwr; + +}; + static const char *const cs42l56_supply_names[CS42L56_NUM_SUPPLIES] = { "VA", "VCP", @@ -1161,7 +1195,13 @@ static int cs42l56_handle_of_data(struct i2c_client *i2c_client, if (of_property_read_u32(np, "cirrus,hpf-left-freq", &val32) >= 0) pdata->hpfb_freq = val32; - 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); + + if (IS_ERR(pdata->gpio_nreset)) + return PTR_ERR(pdata->gpio_nreset); + + gpiod_set_consumer_name(pdata->gpio_nreset, "CS42L56 /RST"); return 0; } @@ -1169,8 +1209,6 @@ static int cs42l56_handle_of_data(struct i2c_client *i2c_client, static int cs42l56_i2c_probe(struct i2c_client *i2c_client) { struct cs42l56_private *cs42l56; - struct cs42l56_platform_data *pdata = - dev_get_platdata(&i2c_client->dev); int ret, i; unsigned int devid; unsigned int alpha_rev, metal_rev; @@ -1188,28 +1226,15 @@ static int cs42l56_i2c_probe(struct i2c_client *i2c_client) return ret; } - if (pdata) { - cs42l56->pdata = *pdata; - } else { - if (i2c_client->dev.of_node) { - ret = cs42l56_handle_of_data(i2c_client, - &cs42l56->pdata); - if (ret != 0) - return ret; - } + if (i2c_client->dev.of_node) { + ret = cs42l56_handle_of_data(i2c_client, &cs42l56->pdata); + if (ret != 0) + return ret; } if (cs42l56->pdata.gpio_nreset) { - ret = gpio_request_one(cs42l56->pdata.gpio_nreset, - GPIOF_OUT_INIT_HIGH, "CS42L56 /RST"); - if (ret < 0) { - dev_err(&i2c_client->dev, - "Failed to request /RST %d: %d\n", - cs42l56->pdata.gpio_nreset, ret); - return ret; - } - 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); }