Re: [PATCH] subsystem: Amplifier driver for TAS5805M,Texas instruments

Re: [PATCH] subsystem: Amplifier driver for TAS5805M,Texas instruments

[Jonathan Cameron]

On Tue, 31 Mar 2020 06:49:12 +0000
Leslie Hsia(夏邦進_Pegatron) <Leslie_Hsia@pegatroncorp.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@pegatroncorp.com<mailto:Leslie_Hsia@pegatroncorp.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@pegatroncorp.com>
+ * Author: Andy Liu <andy-liu@ti.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@pegatroncorp.com>");
+MODULE_AUTHOR("Andy Liu <andy-liu@ti.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"
+

Re: [PATCH] subsystem: Amplifier driver for TAS5805M,Texas instruments

[Jonathan Cameron]

On Mon, 20 Apr 2020 06:58:17 +0000
Leslie Hsia(夏邦進_Pegatron) <Leslie_Hsia@pegatroncorp.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@pegatroncorp.com<mailto:Leslie_Hsia@pegatroncorp.com>>
> 
>       -------------------------------------------------------------------------------------------------
> 
>   *   Hi Jonathan,
> tas5805m only needs to be initialized via i2c port, this driver doesn't use any ALSA API,
> so we put this driver into this path

I'm still unconvinced. This is very much an audio part so people
aren't going to expect to find it in IIO.  Mark? 

Also please ensure next version of code is sent inline.  See
Documentation/process/submitting-patches

Jonathan

> 
> 
> 
> 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.
> 
> 本電子郵件及其附件可能含有機密或依法受特殊管制之資訊，僅供本電子郵件之受文者使用。台端如非本電子郵件之受文者或誤收本電子郵件，請立即回覆郵件通知寄件人，並銷毀本電子郵件之所有複本及附件。任何未經授權而使用、揭露、散佈或複製本電子郵件或其附件之行為，皆嚴格禁止 。
> 

Re: [PATCH] subsystem: Amplifier driver for TAS5805M,Texas instruments

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]

On Tue, Apr 28, 2020 at 10:43:18AM +0000, Leslie Hsia(夏邦進_Pegatron) wrote:
>   *   Author: Leslie Hsia
>   *   Amplifier driver for TAS5805M, initial the amplifier and set the sound parameter.
>   *   Signed-off-by: Leslie Hsia <Leslie_Hsia@pegatroncorp.com<mailto:Leslie_Hsia@pegatroncorp.com>>

Please follow the patch submission process that is described in
Documentation/process/submitting-patches.rst in the kernel source.  Take
a look at other submissions on the list and follow a similar process.
There also appear to be both IIO and ASoC drivers in there which is at
best a bit weird.

Having done a quick scan through your code it doesn't actually seem to
integrate with the subsystem at all (there's no
snd_soc_register_codec()) 

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] subsystem: Amplifier driver for TAS5805M,Texas instruments

[Leslie Hsia(夏邦進_Pegatron)]

Hi Mark,

Yes, I agree that the code didn’t integrate with ALSA API,
and we only need to initialize i2c registers due to make the amplifier works properly,
and I do find there is already an amplifier driver in /drivers/iio/amplifier/,
that’s why I sent the patch to Jonathan,
I want to put this driver into /drivers/iio/amplifier/,
but he thought that this is an amplifier driver,
and we should put it into /sound/doc/codecs/,
so would you please give me an advice about the driver belongs to which subsystem?
Jonathan, what do you think?

Best Regards,
Leslie

> Mark Brown <broonie@kernel.org> 於 2020年4月28日 下午7:30 寫道：
>
> On Tue, Apr 28, 2020 at 10:43:18AM +0000, Leslie Hsia(夏邦進_Pegatron) wrote:
>> *   Author: Leslie Hsia
>> *   Amplifier driver for TAS5805M, initial the amplifier and set the sound parameter.
>> *   Signed-off-by: Leslie Hsia <Leslie_Hsia@pegatroncorp.com<mailto:Leslie_Hsia@pegatroncorp.com>>
>
> Please follow the patch submission process that is described in
> Documentation/process/submitting-patches.rst in the kernel source.  Take
> a look at other submissions on the list and follow a similar process.
> There also appear to be both IIO and ASoC drivers in there which is at
> best a bit weird.
>
> Having done a quick scan through your code it doesn't actually seem to
> integrate with the subsystem at all (there's no
> snd_soc_register_codec())
>
> Please submit patches using subject lines reflecting the style for the
> subsystem, this makes it easier for people to identify relevant patches.
> Look at what existing commits in the area you're changing are doing and
> make sure your subject lines visually resemble what they're doing.
> There's no need to resubmit to fix this alone.


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.

本電子郵件及其附件可能含有機密或依法受特殊管制之資訊，僅供本電子郵件之受文者使用。台端如非本電子郵件之受文者或誤收本電子郵件，請立即回覆郵件通知寄件人，並銷毀本電子郵件之所有複本及附件。任何未經授權而使用、揭露、散佈或複製本電子郵件或其附件之行為，皆嚴格禁止 。

Re: [PATCH] subsystem: Amplifier driver for TAS5805M,Texas instruments

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 731 bytes --]

On Tue, Apr 28, 2020 at 06:27:45PM +0000, Leslie Hsia(夏邦進_Pegatron) wrote:

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

> but he thought that this is an amplifier driver,
> and we should put it into /sound/doc/codecs/,
> so would you please give me an advice about the driver belongs to which subsystem?
> Jonathan, what do you think?

A quick google suggests that this is an audio amplifier so I'd expect it
to be in the audio subsystem.  If it were in IIO it wouldn't work with
most userspace audio software...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]