| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <SJ0PR03MB6681F237649A1FF0891ACAFB8A55A@SJ0PR03MB6681.namprd03.prod.outlook.com>
Date: Tue, 13 Jun 2023 06:08:14 +0000
From: "Lee, RyanS" <RyanS.Lee@...log.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
“Ryan <ryan.lee.analog@...il.com>,
"lgirdwood@...il.com" <lgirdwood@...il.com>,
"broonie@...nel.org" <broonie@...nel.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"krzysztof.kozlowski+dt@...aro.org"
<krzysztof.kozlowski+dt@...aro.org>,
"perex@...ex.cz" <perex@...ex.cz>,
"tiwai@...e.com" <tiwai@...e.com>,
"rf@...nsource.cirrus.com" <rf@...nsource.cirrus.com>,
"wangweidong.a@...nic.com" <wangweidong.a@...nic.com>,
"shumingf@...ltek.com" <shumingf@...ltek.com>,
"herve.codina@...tlin.com" <herve.codina@...tlin.com>,
"ckeepax@...nsource.cirrus.com" <ckeepax@...nsource.cirrus.com>,
"doug@...morgal.com" <doug@...morgal.com>,
"ajye_huang@...pal.corp-partner.google.com"
<ajye_huang@...pal.corp-partner.google.com>,
"kiseok.jo@...ndevice.com" <kiseok.jo@...ndevice.com>,
"alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: "venkataprasad.potturu@....com" <venkataprasad.potturu@....com>,
kernel test robot <lkp@...el.com>
Subject: RE: [PATCH V2 2/2] ASoC: max98388: add amplifier driver
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> Sent: Saturday, June 10, 2023 2:24 AM
> To: “Ryan <ryan.lee.analog@...il.com>; lgirdwood@...il.com;
> broonie@...nel.org; robh+dt@...nel.org; krzysztof.kozlowski+dt@...aro.org;
> perex@...ex.cz; tiwai@...e.com; rf@...nsource.cirrus.com; Lee, RyanS
> <RyanS.Lee@...log.com>; wangweidong.a@...nic.com;
> shumingf@...ltek.com; herve.codina@...tlin.com;
> ckeepax@...nsource.cirrus.com; doug@...morgal.com;
> ajye_huang@...pal.corp-partner.google.com; kiseok.jo@...ndevice.com;
> alsa-devel@...a-project.org; devicetree@...r.kernel.org; linux-
> kernel@...r.kernel.org
> Cc: venkataprasad.potturu@....com; kernel test robot <lkp@...el.com>
> Subject: Re: [PATCH V2 2/2] ASoC: max98388: add amplifier driver
>
> [External]
>
> On 10/06/2023 01:44, “Ryan wrote:
> > From: Ryan Lee <ryans.lee@...log.com>
> >
> > Added Analog Devices MAX98388 amplifier driver.
> > MAX98388 provides a PCM interface for audio data and a standard I2C
> > interface for control data communication.
> >
> > Signed-off-by: Ryan Lee <ryans.lee@...log.com>
> > Reported-by: kernel test robot <lkp@...el.com>
>
> There is nothing to report here.
Probably I misunderstood the mail from the kernel test robot.
Removing the line.
>
> > Closes:
> > https://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-all/2023
> > 06082054.jIU9oENf-
> lkp@...el.com/__;!!A3Ni8CS0y2Y!46sHiAsmIiXxZ_QXIobho
> > mY8F1f7F2yMYd_65NNFwRlcgut33--RdFjVAbg6jKf7Vs8GaYZ7oA$
>
> Nothing to close and also broken link. Fix your mailer.
>
> > ---
> > Changes from v1:
> > Fixed build warnings.
> >
> > sound/soc/codecs/Kconfig | 10 +
> > sound/soc/codecs/Makefile | 2 +
> > sound/soc/codecs/max98388.c | 1042
> > +++++++++++++++++++++++++++++++++++
> > sound/soc/codecs/max98388.h | 234 ++++++++
> > 4 files changed, 1288 insertions(+)
> > create mode 100644 sound/soc/codecs/max98388.c create mode 100644
> > sound/soc/codecs/max98388.h
>
> ...
>
> > +
> > +static void max98388_read_deveice_property(struct device *dev,
> > + struct max98388_priv *max98388) {
> > + int value;
> > +
> > + if (!device_property_read_u32(dev, "adi,vmon-slot-no", &value))
> > + max98388->v_slot = value & 0xF;
> > + else
> > + max98388->v_slot = 0;
> > +
> > + if (!device_property_read_u32(dev, "adi,imon-slot-no", &value))
> > + max98388->i_slot = value & 0xF;
> > + else
> > + max98388->i_slot = 1;
> > +
> > + if (device_property_read_bool(dev, "adi,interleave-mode"))
> > + max98388->interleave_mode = true;
> > + else
> > + max98388->interleave_mode = false;
> > +
> > + if (dev->of_node) {
> > + max98388->reset_gpio = of_get_named_gpio(dev->of_node,
> > + "reset-gpio", 0);
>
> Nope, use devm
OK.
>
> > + if (!gpio_is_valid(max98388->reset_gpio)) {
> > + dev_err(dev, "Looking up %s property in node %s
> failed %d\n",
> > + "reset-gpio", dev->of_node->full_name,
> > + max98388->reset_gpio);
> > + } else {
> > + dev_dbg(dev, "reset-gpio=%d",
> > + max98388->reset_gpio);
> > + }
> > + } else {
> > + /* this makes reset_gpio as invalid */
> > + max98388->reset_gpio = -1;
>
> Why? To request it again? It does not make sense.
This was to make gpio_is_valid() = false in order to skip the reset GPIO control in i2c_probe().
I shall remove these codes and keep the minimum configuration in i2c_probe() function.
>
> > + }
> > +}
> > +
> > +static int max98388_i2c_probe(struct i2c_client *i2c) {
> > + int ret = 0;
> > + int reg = 0;
> > +
> > + struct max98388_priv *max98388 = NULL;
> > +
> > + max98388 = devm_kzalloc(&i2c->dev, sizeof(*max98388),
> GFP_KERNEL);
> > +
>
> Drop blank line.
OK.
>
> > + if (!max98388) {
> > + ret = -ENOMEM;
>
> return -ENOMEM;
OK.
>
> > + return ret;
> > + }
> > + i2c_set_clientdata(i2c, max98388);
> > +
> > + /* regmap initialization */
> > + max98388->regmap = devm_regmap_init_i2c(i2c,
> &max98388_regmap);
> > + if (IS_ERR(max98388->regmap)) {
> > + ret = PTR_ERR(max98388->regmap);
> > + dev_err(&i2c->dev,
> > + "Failed to allocate regmap: %d\n", ret);
> > + return ret;
>
> return dev_err_probe
OK. Shall fix it.
>
> > + }
> > +
> > + /* voltage/current slot & gpio configuration */
> > + max98388_read_deveice_property(&i2c->dev, max98388);
> > +
> > + /* Power on device */
> > + if (gpio_is_valid(max98388->reset_gpio)) {
>
> What's this? You request it twice? No.
Will modify the code.
>
>
> > + ret = devm_gpio_request(&i2c->dev, max98388->reset_gpio,
> > + "MAX98388_RESET");
> > + if (ret) {
> > + dev_err(&i2c->dev, "%s: Failed to request gpio %d\n",
> > + __func__, max98388->reset_gpio);
>
> return dev_err_probe
OK
>
> > + return -EINVAL;
> > + }
> > + gpio_direction_output(max98388->reset_gpio, 0);
> > + msleep(50);
> > + gpio_direction_output(max98388->reset_gpio, 1);
>
> 1 means keep in reset, so why do you keep deviec reset afterwards? Was it
> tested? You probably messed up values used for GPIOs as you stated in
> example that it is active low.
The hardware reset function is active low, so the state needs to be high to restart the amp.
The code was functional, but I see room for improvement. I shall modify the code.
>
> > + msleep(20);
> > + }
> > +
> > + /* Read Revision ID */
> > + ret = regmap_read(max98388->regmap,
> > + MAX98388_R22FF_REV_ID, ®);
> > + if (ret < 0) {
> > + dev_err(&i2c->dev,
> > + "Failed to read: 0x%02X\n",
> MAX98388_R22FF_REV_ID);
> > + return ret;
>
> return dev_err_probe
OK.
>
> > + }
> > + dev_info(&i2c->dev, "MAX98388 revisionID: 0x%02X\n", reg);
> > +
> > + /* codec registration */
> > + ret = devm_snd_soc_register_component(&i2c->dev,
> > + &soc_codec_dev_max98388,
> > + max98388_dai,
> > + ARRAY_SIZE(max98388_dai));
> > + if (ret < 0)
> > + dev_err(&i2c->dev, "Failed to register codec: %d\n", ret);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct i2c_device_id max98388_i2c_id[] = {
> > + { "max98388", 0},
> > + { },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, max98388_i2c_id);
> > +
> > +#if defined(CONFIG_OF)
>
> Drop
OK Thanks.
>
> > +static const struct of_device_id max98388_of_match[] = {
> > + { .compatible = "adi,max98388", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, max98388_of_match); #endif
> > +
> > +#ifdef CONFIG_ACPI
>
> Drop
OK.
>
> > +static const struct acpi_device_id max98388_acpi_match[] = {
> > + { "ADS8388", 0 },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(acpi, max98388_acpi_match); #endif
> > +
> > +static struct i2c_driver max98388_i2c_driver = {
> > + .driver = {
> > + .name = "max98388",
> > + .of_match_table = of_match_ptr(max98388_of_match),
> > + .acpi_match_table = ACPI_PTR(max98388_acpi_match),
>
> Just drop all wrappers. They are useless and only limit your driver (OF can be
> used on some ACPI platforms).
I shall remove all wrappers.
Thanks for the review.
>
>
> Best regards,
> Krzysztof
Powered by blists - more mailing lists