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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250709-elated-cat-of-variation-2d0bd1@krzk-bin>
Date: Wed, 9 Jul 2025 12:55:11 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Nick Li <nick.li@...rsemi.com>
Cc: lgirdwood@...il.com, broonie@...nel.org, robh@...nel.org, 
	krzk+dt@...nel.org, conor+dt@...nel.org, perex@...ex.cz, tiwai@...e.com, 
	xiaoming.yang@...rsemi.com, danyang.zheng@...rsemi.com, like.xy@...mail.com, 
	linux-sound@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/4] ASoC: codecs: Add FourSemi FS2104/5S audio
 amplifier driver

On Tue, Jul 08, 2025 at 07:29:01PM +0800, Nick Li wrote:
> @@ -564,6 +565,7 @@ obj-$(CONFIG_SND_SOC_ES8375)    += snd-soc-es8375.o
>  obj-$(CONFIG_SND_SOC_ES8389)    += snd-soc-es8389.o
>  obj-$(CONFIG_SND_SOC_FRAMER)	+= snd-soc-framer.o
>  obj-$(CONFIG_SND_SOC_FS_AMP_LIB)+= snd-soc-fs-amp-lib.o
> +obj-$(CONFIG_SND_SOC_FS210X)	+= snd-soc-fs210x.o
>  obj-$(CONFIG_SND_SOC_GTM601)    += snd-soc-gtm601.o
>  obj-$(CONFIG_SND_SOC_HDAC_HDMI) += snd-soc-hdac-hdmi.o
>  obj-$(CONFIG_SND_SOC_HDAC_HDA) += snd-soc-hdac-hda.o
> diff --git a/sound/soc/codecs/fs210x.c b/sound/soc/codecs/fs210x.c
> new file mode 100644
> index 000000000..5176b3399
> --- /dev/null
> +++ b/sound/soc/codecs/fs210x.c
> @@ -0,0 +1,1610 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * fs210x.c -- Driver for the FS2104/5S Audio Amplifier
> + *
> + * Copyright (C) 2016-2025 Shanghai FourSemi Semiconductor Co.,Ltd.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>

Where do you use it?

> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/workqueue.h>
> +#include <sound/soc.h>
> +#include <sound/tlv.h>
> +
> +#include "fs210x.h"
> +#include "fs-amp-lib.h"
> +
> +#define FS210X_DRV_VERSION		"v1.0.6"
> +#define FS210X_DEFAULT_FWM_NAME		"fs210x_fwm.bin"
> +#define FS210X_DEFAULT_DAI_NAME		"fs210x-aif"
> +#define FS2105S_DEVICE_ID		0x20 /* FS2105S */
> +#define FS210X_DEVICE_ID		0x45 /* FS2104 */
> +#define FS210X_REG_MAX			0xF8
> +#define FS210X_VOLUME_MIN		0
> +#define FS210X_VOLUME_MAX		511
> +#define FS210X_INIT_SCENE		0
> +#define FS210X_DEFAULT_SCENE		1
> +#define FS210X_START_DELAY_MS		5
> +#define FS210X_FAULT_CHECK_INTERVAL_MS	2000
> +#define FS2105S_RATES			(SNDRV_PCM_RATE_32000 | \
> +					 SNDRV_PCM_RATE_44100 | \
> +					 SNDRV_PCM_RATE_48000 | \
> +					 SNDRV_PCM_RATE_88200 | \
> +					 SNDRV_PCM_RATE_96000)
> +#define FS210X_RATES			(SNDRV_PCM_RATE_16000 | FS2105S_RATES)
> +#define FS210X_FORMATS			(SNDRV_PCM_FMTBIT_S16_LE | \
> +					 SNDRV_PCM_FMTBIT_S24_LE | \
> +					 SNDRV_PCM_FMTBIT_S24_3LE | \
> +					 SNDRV_PCM_FMTBIT_S32_LE)
> +#define FS210X_NUM_SUPPLIES		ARRAY_SIZE(fs210x_supply_names)
> +
> +static const char *const fs210x_supply_names[] = {
> +	"pvdd",
> +	"dvdd",
> +};
> +
> +struct fs210x_platform_data {
> +	const char *fwm_name;
> +};
> +
> +struct fs210x_priv {
> +	struct i2c_client *i2c;
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct fs210x_platform_data pdata;
> +	struct regulator_bulk_data supplies[FS210X_NUM_SUPPLIES];
> +	struct gpio_desc *gpio_sdz;
> +	struct delayed_work start_work;
> +	struct delayed_work fault_check_work;
> +	struct fs_amp_lib amp_lib;
> +	const struct fs_amp_scene *cur_scene;
> +	struct clk *clk_bclk;
> +	unsigned int bclk;
> +	unsigned int srate;
> +	int scene_id;
> +	u16 devid;
> +	u16 vol[2]; /* L/R channels volume */
> +	bool is_inited;
> +	bool is_suspended;
> +	bool is_bclk_on;
> +	bool is_playing;
> +};
> +
> +static DEFINE_MUTEX(fs210x_mutex);

Why is this file-scope? Why two independent codecs cannot work in parallel?

> +
> +static const struct fs_pll_div fs210x_pll_div[] = {
> +	/*    bclk,   pll1,   pll2,   pll3 */
> +	{   512000, 0x006C, 0x0120, 0x0001 },
> +	{   768000, 0x016C, 0x00C0, 0x0001 },
> +	{  1024000, 0x016C, 0x0090, 0x0001 },
> +	{  1536000, 0x016C, 0x0060, 0x0001 },
> +	{  2048000, 0x016C, 0x0090, 0x0002 },
> +	{  2304000, 0x016C, 0x0080, 0x0002 },
> +	{  3072000, 0x016C, 0x0090, 0x0003 },
> +	{  4096000, 0x016C, 0x0090, 0x0004 },
> +	{  4608000, 0x016C, 0x0080, 0x0004 },
> +	{  6144000, 0x016C, 0x0090, 0x0006 },
> +	{  8192000, 0x016C, 0x0090, 0x0008 },
> +	{  9216000, 0x016C, 0x0090, 0x0009 },
> +	{ 12288000, 0x016C, 0x0090, 0x000C },
> +	{ 16384000, 0x016C, 0x0090, 0x0010 },
> +	{ 18432000, 0x016C, 0x0090, 0x0012 },
> +	{ 24576000, 0x016C, 0x0090, 0x0018 },
> +	{  1411200, 0x016C, 0x0060, 0x0001 },
> +	{  2116800, 0x016C, 0x0080, 0x0002 },
> +	{  2822400, 0x016C, 0x0090, 0x0003 },
> +	{  4233600, 0x016C, 0x0080, 0x0004 },
> +	{  5644800, 0x016C, 0x0090, 0x0006 },
> +	{  8467200, 0x016C, 0x0090, 0x0009 },
> +	{ 11289600, 0x016C, 0x0090, 0x000C },
> +	{ 16934400, 0x016C, 0x0090, 0x0012 },
> +	{ 22579200, 0x016C, 0x0090, 0x0018 },
> +	{  2000000, 0x017C, 0x0093, 0x0002 },
> +};
> +
> +static int fs210x_bclk_set(struct fs210x_priv *fs210x, bool on)
> +{
> +	int ret = 0;
> +
> +	if (!fs210x || !fs210x->dev)
> +		return -EINVAL;
> +
> +	if (!fs210x->clk_bclk)
> +		return 0;
> +
> +	if ((fs210x->is_bclk_on ^ on) == 0)
> +		return 0;
> +
> +	dev_dbg(fs210x->dev, "bclk switch %s\n", on ? "on" : "off");
> +
> +	if (on) {
> +		clk_set_rate(fs210x->clk_bclk, fs210x->bclk);
> +		ret = clk_prepare_enable(fs210x->clk_bclk);
> +		fs210x->is_bclk_on = true;
> +		usleep_range(2000, 2050); /* >= 2ms */
> +	} else {
> +		clk_disable_unprepare(fs210x->clk_bclk);
> +		fs210x->is_bclk_on = false;
> +	}
> +
> +	return ret;
> +}
> +
> +static int fs210x_reg_write(struct fs210x_priv *fs210x,
> +			    u8 reg, u16 val)
> +{
> +	int ret;
> +
> +	ret = regmap_write(fs210x->regmap, reg, val);
> +	if (ret) {
> +		dev_err(fs210x->dev, "Failed to write %02Xh: %d\n", reg, ret);
> +		return ret;
> +	}
> +
> +	dev_dbg(fs210x->dev, "RW: %02x %04x\n", reg, val);
> +
> +	return 0;
> +}

...

> +
> +static int fs210x_add_optional_controls(struct fs210x_priv *fs210x,
> +					struct snd_soc_component *cmpnt)
> +{
> +	int count;
> +
> +	if (!fs210x || !cmpnt)
> +		return -EINVAL;
> +
> +	/*
> +	 * If the firmware has no scene or only init scene,
> +	 * we skip adding this mixer control.
> +	 */
> +	if (fs210x->amp_lib.scene_count < 2)
> +		return 0;
> +
> +	count = ARRAY_SIZE(fs210x_scene_control);
> +
> +	return snd_soc_add_component_controls(cmpnt,
> +					      fs210x_scene_control,
> +					      count);
> +}
> +
> +static int fs210x_get_bclk(struct fs210x_priv *fs210x,
> +			   struct snd_soc_component *cmpnt)
> +{
> +	struct clk *bclk;
> +	int ret;
> +
> +	bclk = devm_clk_get(fs210x->dev, "bclk");
> +	if (IS_ERR_OR_NULL(bclk)) {
> +		ret = bclk ? PTR_ERR(bclk) : -ENODATA;

Same pattern as regulators, eh...

> +		if (ret == -EPROBE_DEFER)

No. Stop handling own probe deferrals. Look how other drivers do it.

> +			return ret;
> +		/*
> +		 * If the SOC doesn't have the bclk clock source,
> +		 * we skip setting the bclk clock.
> +		 */
> +		return 0;

What is the point of this entire code? You got NULL, so assign NULL. Can
clk API handle NULLs? Answer this to yourself and write obvious, simple
code.

> +	}
> +
> +	fs210x->clk_bclk = bclk;
> +	dev_dbg(fs210x->dev, "Got I2S bclk clock\n");

Drop. Really, your debugs here and further are meaningless. You debug
static, well know, things - DTB. No, debug unexpected pieces, not
something which is 100% predictable because DT schema told you this
already.

> +
> +	return 0;
> +}
> +
> +static int fs210x_probe(struct snd_soc_component *cmpnt)
> +{
> +	struct fs210x_priv *fs210x;
> +	int ret;
> +
> +	fs210x = snd_soc_component_get_drvdata(cmpnt);
> +	if (!fs210x || !fs210x->dev)
> +		return -EINVAL;
> +
> +	fs210x->amp_lib.dev   = fs210x->dev;
> +	fs210x->amp_lib.devid = fs210x->devid;
> +
> +	ret = fs_amp_load_firmware(&fs210x->amp_lib, fs210x->pdata.fwm_name);
> +	if (ret) {
> +		dev_err(fs210x->dev, "Failed to load firmware: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = fs210x_add_optional_controls(fs210x, cmpnt);
> +	if (ret) {
> +		dev_err(fs210x->dev, "Failed to add opt-ctrl: %d\n", ret);
> +		return ret;
> +	}
> +
> +	INIT_DELAYED_WORK(&fs210x->fault_check_work, fs210x_fault_check_work);
> +	INIT_DELAYED_WORK(&fs210x->start_work, fs210x_start_work);
> +
> +	fs210x_get_bclk(fs210x, cmpnt);
> +
> +	mutex_lock(&fs210x_mutex);
> +	ret = fs210x_init_chip(fs210x);
> +	mutex_unlock(&fs210x_mutex);
> +	if (ret)
> +		dev_err(fs210x->dev, "Failed to probe: %d\n", ret);

Oh wait, it is the FOURTH time you print same error message. Sorry, this
error handling is terrible. Obfuscated and overcomplicated. Error should
be printed only once. Look at other recent codecs.

> +
> +	return ret;
> +}
> +
> +static void fs210x_remove(struct snd_soc_component *cmpnt)
> +{
> +	struct fs210x_priv *fs210x;
> +
> +	fs210x = snd_soc_component_get_drvdata(cmpnt);
> +	if (!fs210x || !fs210x->dev)
> +		return;
> +
> +	cancel_delayed_work_sync(&fs210x->start_work);
> +	cancel_delayed_work_sync(&fs210x->fault_check_work);
> +
> +	dev_dbg(fs210x->dev, "Codec removed\n");

No, drop all such simple probe enter/exit debugs. This is really useless
debug.

> +}
> +
> +#ifdef CONFIG_PM
> +static int fs210x_suspend(struct snd_soc_component *cmpnt)
> +{
> +	struct fs210x_priv *fs210x;
> +	int ret;
> +
> +	fs210x = snd_soc_component_get_drvdata(cmpnt);
> +	if (!fs210x || !fs210x->dev)
> +		return -EINVAL;
> +
> +	cancel_delayed_work_sync(&fs210x->start_work);
> +	cancel_delayed_work_sync(&fs210x->fault_check_work);
> +
> +	mutex_lock(&fs210x_mutex);
> +	fs210x->cur_scene = NULL;
> +	fs210x->is_inited = false;
> +	fs210x->is_playing = false;
> +	fs210x->is_suspended = true;
> +
> +	fs210x_sdz_pin_set(fs210x, true);
> +	mutex_unlock(&fs210x_mutex);
> +
> +	ret = regulator_bulk_disable(FS210X_NUM_SUPPLIES, fs210x->supplies);
> +	if (ret) {
> +		dev_err(fs210x->dev, "Failed to suspend: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dev_info(fs210x->dev, "pm suspended\n");

No, drop all such simple probe enter/exit debugs.

> +
> +	return 0;
> +}
> +
> +static int fs210x_resume(struct snd_soc_component *cmpnt)
> +{
> +	struct fs210x_priv *fs210x;
> +	int ret;
> +
> +	fs210x = snd_soc_component_get_drvdata(cmpnt);
> +	if (!fs210x || !fs210x->dev)
> +		return -EINVAL;
> +
> +	ret = regulator_bulk_enable(FS210X_NUM_SUPPLIES, fs210x->supplies);
> +	if (ret) {
> +		dev_err(fs210x->dev, "Failed to enable supplies: %d\n", ret);
> +		return ret;
> +	}
> +
> +	mutex_lock(&fs210x_mutex);
> +	fs210x_sdz_pin_set(fs210x, false);
> +
> +	fs210x->is_suspended = false;
> +	ret = fs210x_init_chip(fs210x);
> +	mutex_unlock(&fs210x_mutex);
> +
> +	dev_info(fs210x->dev, "pm resumed\n");

No, drop.

> +
> +	return ret;
> +}
> +#else
> +#define fs210x_suspend NULL
> +#define fs210x_resume NULL
> +#endif // CONFIG_PM
> +
> +static const struct snd_soc_component_driver fs210x_soc_component_dev = {
> +	.probe			= fs210x_probe,
> +	.remove			= fs210x_remove,
> +	.suspend		= fs210x_suspend,
> +	.resume			= fs210x_resume,
> +	.controls		= fs210x_controls,
> +	.num_controls		= ARRAY_SIZE(fs210x_controls),
> +	.dapm_widgets		= fs210x_dapm_widgets,
> +	.num_dapm_widgets	= ARRAY_SIZE(fs210x_dapm_widgets),
> +	.dapm_routes		= fs210x_dapm_routes,
> +	.num_dapm_routes	= ARRAY_SIZE(fs210x_dapm_routes),
> +	.use_pmdown_time	= 1,
> +};
> +
> +static const struct regmap_config fs210x_regmap = {
> +	.reg_bits		= 8,
> +	.val_bits		= 16,
> +	.max_register		= FS210X_REG_MAX,
> +	.val_format_endian	= REGMAP_ENDIAN_BIG,
> +	.cache_type		= REGCACHE_NONE,
> +};
> +
> +static int fs210x_detect_device(struct fs210x_priv *fs210x)
> +{
> +	u16 devid;
> +	int ret;
> +
> +	ret = fs210x_reg_read(fs210x, FS210X_03H_DEVID, &devid);
> +	if (ret)
> +		return ret;
> +
> +	fs210x->devid = HI_U16(devid);
> +
> +	switch (fs210x->devid) {
> +	case FS210X_DEVICE_ID:
> +		dev_info(fs210x->dev, "FS2104 detected\n");
> +		break;
> +	case FS2105S_DEVICE_ID:
> +		dev_info(fs210x->dev, "FS2105S detected\n");
> +		break;
> +	default:
> +		dev_err(fs210x->dev, "DEVID: 0x%04X dismatch\n", devid);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fs210x_parse_dts(struct fs210x_priv *fs210x,
> +			    struct fs210x_platform_data *pdata)
> +{
> +	struct device_node *node = fs210x->dev->of_node;
> +	int i, ret;
> +
> +	if (!node)
> +		return 0;
> +
> +	ret = of_property_read_string(node, "firmware-name", &pdata->fwm_name);
> +	if (ret)
> +		pdata->fwm_name = FS210X_DEFAULT_FWM_NAME;
> +
> +	fs210x->gpio_sdz = devm_gpiod_get_optional(fs210x->dev,
> +						   "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR_OR_NULL(fs210x->gpio_sdz)) {
> +		ret = fs210x->gpio_sdz ? PTR_ERR(fs210x->gpio_sdz) : -ENODATA;


Weird dance. Why assigning to ret enodata?

> +		fs210x->gpio_sdz = NULL;
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +		dev_dbg(fs210x->dev, "Assuming reset-gpios is unused\n");
> +	} else {
> +		dev_dbg(fs210x->dev, "reset-gpios: %d\n",
> +			desc_to_gpio(fs210x->gpio_sdz));
> +	}

This is over-complicated way of getting simple optional gpio.

> +
> +	for (i = 0; i < FS210X_NUM_SUPPLIES; i++)
> +		fs210x->supplies[i].supply = fs210x_supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(fs210x->dev,
> +				      ARRAY_SIZE(fs210x->supplies),
> +				      fs210x->supplies);
> +	if (ret) {
> +		dev_err(fs210x->dev, "Failed to get supplies: %d\n", ret);

Syntax is return dev_err_probe.

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fs210x_parse_platdata(struct fs210x_priv *fs210x)

I do not understand why you have so many functions doing simple OF
parsing. fs210x_init, fs210x_parse_platdata, fs210x_parse_dts... and
this one here does nothing.

> +{
> +	struct fs210x_platform_data *pdata;
> +	int ret;
> +
> +	pdata = &fs210x->pdata;
> +	ret = fs210x_parse_dts(fs210x, pdata);
> +	if (ret) {
> +		dev_err(fs210x->dev, "Failed to parse dts: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void fs210x_deinit(struct fs210x_priv *fs210x)
> +{
> +	fs210x_sdz_pin_set(fs210x, true);
> +	regulator_bulk_disable(FS210X_NUM_SUPPLIES, fs210x->supplies);
> +}
> +
> +static int fs210x_init(struct fs210x_priv *fs210x)
> +{
> +	int ret;
> +
> +	ret = fs210x_parse_platdata(fs210x);
> +	if (ret) {
> +		dev_err(fs210x->dev, "Failed to parse platdata: %d\n", ret);

So you print SAME ERROR three times?

> +		return ret;
> +	}
> +
> +	ret = regulator_bulk_enable(FS210X_NUM_SUPPLIES, fs210x->supplies);
> +	if (ret) {
> +		dev_err(fs210x->dev, "Failed to enable supplies: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Make sure the SDZ pin is pulled down enough time. */
> +	usleep_range(10000, 10050); /* >= 10ms */
> +	fs210x_sdz_pin_set(fs210x, false);
> +
> +	ret = fs210x_detect_device(fs210x);
> +	if (ret) {
> +		fs210x_deinit(fs210x);
> +		return ret;
> +	}
> +
> +	fs210x->scene_id     = -1; /* Invalid scene */
> +	fs210x->cur_scene    = NULL;
> +	fs210x->is_playing   = false;
> +	fs210x->is_inited    = false;
> +	fs210x->is_suspended = false;
> +	fs210x->vol[0]       = FS210X_VOLUME_MAX;
> +	fs210x->vol[1]       = FS210X_VOLUME_MAX;
> +
> +	return 0;
> +}
> +
> +static int fs210x_register_snd_component(struct fs210x_priv *fs210x)
> +{
> +	struct snd_soc_dai_driver *dai_drv;
> +	int ret;
> +
> +	dai_drv = devm_kmemdup(fs210x->dev, &fs210x_dai,
> +			       sizeof(fs210x_dai), GFP_KERNEL);
> +	if (!dai_drv)
> +		return -ENOMEM;
> +
> +	if (fs210x->devid == FS2105S_DEVICE_ID) {
> +		dai_drv->playback.rates = FS2105S_RATES;
> +		dai_drv->capture.rates  = FS2105S_RATES;
> +	}
> +
> +	ret = snd_soc_register_component(fs210x->dev,
> +					 &fs210x_soc_component_dev,
> +					 dai_drv, 1);
> +	return ret;
> +}
> +
> +static int fs210x_i2c_probe(struct i2c_client *client)
> +{
> +	struct fs210x_priv *fs210x;
> +	int ret;
> +
> +	dev_info(&client->dev, "version: %s\n", FS210X_DRV_VERSION);
> +
> +	fs210x = devm_kzalloc(&client->dev, sizeof(*fs210x), GFP_KERNEL);
> +	if (!fs210x)
> +		return -ENOMEM;
> +
> +	fs210x->i2c = client;
> +	fs210x->dev = &client->dev;
> +	i2c_set_clientdata(client, fs210x);
> +
> +	fs210x->regmap = devm_regmap_init_i2c(client, &fs210x_regmap);
> +	if (IS_ERR_OR_NULL(fs210x->regmap)) {

Can devm_regmap_init_i2c() return NULL? No, it cannot.

> +		dev_err(fs210x->dev, "Failed to get regmap\n");
> +		ret = fs210x->regmap ? PTR_ERR(fs210x->regmap) : -ENODATA;

Syntax is return dev_err_probe and drop NULL check.

> +		return ret;
> +	}
> +
> +	mutex_lock(&fs210x_mutex);
> +	ret = fs210x_init(fs210x);
> +	mutex_unlock(&fs210x_mutex);

Why do you need to lock it? Who and how can access device at this point?

> +	if (ret)
> +		return ret;
> +
> +	ret = fs210x_register_snd_component(fs210x);
> +	if (ret) {
> +		fs210x_deinit(fs210x);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void fs210x_i2c_remove(struct i2c_client *client)
> +{
> +	struct fs210x_priv *fs210x = i2c_get_clientdata(client);
> +
> +	snd_soc_unregister_component(fs210x->dev);
> +	fs210x_deinit(fs210x);
> +}
> +
> +static const struct i2c_device_id fs210x_i2c_id[] = {
> +	{ "fs2104" },
> +	{ "fs2105s" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, fs210x_i2c_id);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id fs210x_of_match[] = {
> +	{ .compatible = "foursemi,fs2104", },

I asked to express the fallback. Drop this, it is complete redundant.

> +	{ .compatible = "foursemi,fs2105s", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, fs210x_of_match);
> +#endif // CONFIG_OF

All these ifdefs and of_match_ptr should be dropped, not really helpful.

Best regards,
Krzysztof


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ