> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> > Sent: Saturday, June 10, 2023 2:24 AM > To: “Ryan <ryan.lee.analog@xxxxxxxxx>; lgirdwood@xxxxxxxxx; > broonie@xxxxxxxxxx; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; > perex@xxxxxxxx; tiwai@xxxxxxxx; rf@xxxxxxxxxxxxxxxxxxxxx; Lee, RyanS > <RyanS.Lee@xxxxxxxxxx>; wangweidong.a@xxxxxxxxxx; > shumingf@xxxxxxxxxxx; herve.codina@xxxxxxxxxxx; > ckeepax@xxxxxxxxxxxxxxxxxxxxx; doug@xxxxxxxxxxxxx; > ajye_huang@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx; kiseok.jo@xxxxxxxxxxxxxx; > alsa-devel@xxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx > Cc: venkataprasad.potturu@xxxxxxx; kernel test robot <lkp@xxxxxxxxx> > 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@xxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > Reported-by: kernel test robot <lkp@xxxxxxxxx> > > 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@xxxxxxxxx/__;!!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
