Message-ID: <20200405120803.7ca63241@archlinux>
Date:   Sun, 5 Apr 2020 12:08:03 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     "Leslie Hsia夏邦進_Pegatron)" 
        <Leslie_Hsia@...atroncorp.com>
Cc:     "knaack.h@....de" <knaack.h@....de>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "pmeerw@...erw.net" <pmeerw@...erw.net>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Hermes Hsieh謝旻劭_Pegatron)" 
        <Hermes_Hsieh@...atroncorp.com>,
        "jesse.sung@...onical.com" <jesse.sung@...onical.com>,
        Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH] subsystem: Amplifier driver for TAS5805M,Texas
 instruments

On Tue, 31 Mar 2020 06:49:12 +0000
Leslie Hsia(夏邦進_Pegatron) <Leslie_Hsia@...atroncorp.com> wrote:

>   *   Author: Leslie Hsia
>   *   Amplifier driver for TAS5805M, initial the amplifier and set the sound's parameter.
>   *   Signed-off-by: Leslie Hsia <Leslie_Hsia@...atroncorp.com<mailto:Leslie_Hsia@...atroncorp.com>>

Hi. Please read the SubmittingPatches.rst file for how to submit a diver.

https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html

I've pasted the patch below though to allow some quick initial comments.

One high level comment to start off.  This is very much an audio focused amplifier.
I wouldn't normally expect to see a driver for such a device in IIO.  Why here
rather than somewhere in the audio subsystems?

sound/soc/codecs/
already has a large number of similar looking amplifiers.

The amplifiers in IIO tend to be general purpose (and normally extremely high
frequency supporting) amplifiers. Cc'd Mark Brown as one of the ASOC maintainers
who can probably give you a clear answer to if this belongs in ASOC.

Anyhow, some general comments inline.

Thanks

Jonathan

> 
>       -------------------------------------------------------------------------------------------------
> 
>   *   This is a new driver for TAS5805M, please help me to Cc to the related people. Thanks.
> 
> 
> 
> This e-mail and its attachment may contain information that is confidential or privileged, and are solely for the use of the individual to whom this e-mail is addressed. If you are not the intended recipient or have received it accidentally, please immediately notify the sender by reply e-mail and destroy all copies of this email and its attachment. Please be advised that any unauthorized use, disclosure, distribution or copying of this email or its attachment is strictly prohibited.
> 
> 本電子郵件及其附件可能含有機密或依法受特殊管制之資訊，僅供本電子郵件之受文者使用。台端如非本電子郵件之受文者或誤收本電子郵件，請立即回覆郵件通知寄件人，並銷毀本電子郵件之所有複本及附件。任何未經授權而使用、揭露、散佈或複製本電子郵件或其附件之行為，皆嚴格禁止 。
> 

diff -uprN -X linux-4.15-vanilla/Documentation/dontdiff linux-4.15-vanilla/drivers/iio/amplifiers/tas5805m.c linux-4.15/drivers/iio/amplifiers/tas5805m.c
--- linux-4.15-vanilla/drivers/iio/amplifiers/tas5805m.c	1970-01-01 08:00:00.000000000 +0800
+++ linux-4.15/drivers/iio/amplifiers/tas5805m.c	2020-03-13 13:58:36.201225000 +0800
@@ -0,0 +1,151 @@

Use an SPDX license identifier and then don't put the license
boilerplate in each file.

+/*
+ * Driver for the TAS5805M Audio Amplifier (I2C part only)
+ *
+ * Author: Leslie Hsia <Leslie_Hsia@...atroncorp.com>
+ * Author: Andy Liu <andy-liu@...com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+ 
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/init.h>
+#include <linux/acpi.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include "tas5805m.h"
+
+struct tas5805m_priv {
+	struct regmap *regmap;
+	struct mutex lock;
+};
+
+const struct regmap_config tas5805m_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.cache_type = REGCACHE_RBTREE,
+};
+
+static const struct reg_sequence tas5805m_init_dsp[] = {
+	{ 0x03, 0x00 },
+	{ 0x03, 0x02 },
+	{ 0x33, 0x00 },
+	{ 0x4c, 0x3d }, // set volume to -6.5dB

These magic numbers should ideally be replaced with defines that
decode the fields and identify the register names setc.

+	{ 0x03, 0x03 },
+};
+
+static int tas5805m_set_device_state(struct tas5805m_priv *tas5805m, int state)
+{
+	int ret = 0;
+
+	ret = regmap_write(tas5805m->regmap, 0x00, 0x00);
+	if (ret != 0)

if (ret)

+		return ret;
+
+	ret = regmap_write(tas5805m->regmap, 0x7F, 0x00);
+	if (ret != 0)
+		return ret;
+
+	ret = regmap_update_bits(tas5805m->regmap, TAS5805M_REG_DEV_CTL2,
+				 TAS5805M_DEV_STAT_CTL_MSK, state);
+	if (ret != 0)
+		return ret;
+
+	return ret;

return regmap_update_bits...

+}
+
+static int tas5805m_probe(struct device *dev, struct regmap *regmap)
+{

Superficially it seems like there is little benefit in separating this
from the i2c_probe below - I would just have it inline.

+	struct tas5805m_priv *tas5805m;
+	int ret;
+
+	tas5805m = devm_kzalloc(dev, sizeof(struct tas5805m_priv), GFP_KERNEL);

sizeof(*tas5805m)

+	if (!tas5805m)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, tas5805m);

Not used.

+	tas5805m->regmap = regmap;
+    
+	mutex_init(&tas5805m->lock);
+
+	ret = regmap_register_patch(regmap, tas5805m_init_dsp, ARRAY_SIZE(tas5805m_init_dsp));
+	if (ret != 0) {

if (ret)


+		dev_err(dev, "Failed to initialize TAS5805M: %d\n",ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tas5805m_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
+{	

Use probe_new as you aren't using the i2c_device_id anyway

+	struct regmap *regmap;
+	struct regmap_config config = tas5805m_regmap;
+
+	regmap = devm_regmap_init_i2c(i2c, &config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	return tas5805m_probe(&i2c->dev, regmap);
+}
+
+static int tas5805m_i2c_remove(struct i2c_client *i2c)
+{	
+	return 0;
+}
+
+#ifdef CONFIG_OF

No need for this protection and it will probably result in build errors
if you build with device tree support.

+static const struct of_device_id tas5805m_of_match[] = {
+	{ 
+		.compatible = "ti,TAS5805M", 
+		.data = TAS5805M_AMP_DEV_NAME,
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tas5805m_of_match);
+#else
+#define tas5805m_of_match NULL
+#endif
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id tas5805m_acpi_match[] = {
+	{"TXNM5805", TAS5805M},
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, tas5805m_acpi_match);
+#else
+#define st_accel_acpi_match NULL
+#endif
+
+static const struct i2c_device_id tas5805m_i2c_id[] = {
+	{ TAS5805M_AMP_DEV_NAME, TAS5805M },
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, tas5805m_i2c_id);
+
+static struct i2c_driver tas5805m_i2c_driver = {
+	.driver	= {
+		.name = TAS5805M_DRV_NAME,
+		.of_match_table = tas5805m_of_match,
+		.acpi_match_table = ACPI_PTR(tas5805m_acpi_match),
+	},
+	.probe = tas5805m_i2c_probe,
+	.remove = tas5805m_i2c_remove,
+	.id_table = tas5805m_i2c_id,
+};
+
+module_i2c_driver(tas5805m_i2c_driver);
+
+MODULE_AUTHOR("Leslie Hsia <Leslie_Hsia@...atroncorp.com>");
+MODULE_AUTHOR("Andy Liu <andy-liu@...com>");
+MODULE_DESCRIPTION("TAS5805M Audio Amplifier I2C Driver");
+MODULE_LICENSE("GPL v2");
diff -uprN -X linux-4.15-vanilla/Documentation/dontdiff linux-4.15-vanilla/drivers/iio/amplifiers/tas5805m.h linux-4.15/drivers/iio/amplifiers/tas5805m.h
--- linux-4.15-vanilla/drivers/iio/amplifiers/tas5805m.h	1970-01-01 08:00:00.000000000 +0800
+++ linux-4.15/drivers/iio/amplifiers/tas5805m.h	2020-03-13 14:00:11.350955000 +0800
@@ -0,0 +1,15 @@
+#include <linux/types.h>
+enum ti_ampfilier_type {
+	TAS5805M,
+};
+
+#define TAS5805M_DRV_NAME    "TAS5805M"
+
+#define TAS5805M_REG_DEV_CTL2		0x03
+#define TAS5805M_DEV_STAT_CTL_MSK	(BIT(1) | BIT(0))
+#define TAS5805M_DEV_STAT_DSLEEP 	0x00
+#define TAS5805M_DEV_STAT_SLEEP 	0x01
+#define TAS5805M_DEV_STAT_HIZ	 	0x02
+#define TAS5805M_DEV_STAT_PLAY		0x03
+#define TAS5805M_AMP_DEV_NAME		"TAS5805M"
+

Powered by blists - more mailing lists