Re: [PATCH v2 1/3] ASoC: sma1307: Add driver for Iron Device SMA1307

[Ki-Seok Jo <kiseok.jo@xxxxxxxxxxxxxx>]

> On Tue, Sep 03, 2024 at 02:44:33PM +0900, Kiseok Jo wrote:
> > Signed-off-by: Kiseok Jo <kiseok.jo@xxxxxxxxxxxxxx>
> > ---
> >  sound/soc/codecs/Kconfig   |    8 +
> >  sound/soc/codecs/Makefile  |    2 +
> >  sound/soc/codecs/sma1307.c | 2191
> > ++++++++++++++++++++++++++++++++++++
> >  sound/soc/codecs/sma1307.h |  454 ++++++++
> >  4 files changed, 2655 insertions(+)
> >  create mode 100644 sound/soc/codecs/sma1307.c  create mode 100644
> > sound/soc/codecs/sma1307.h
> >
> 
> What are the differences against sm1303? Why it cannot be merged there?
> You have entire commit msg to explain this instead of just sending empty
> commit.

I will add a commit message in the next version of the patch.


> > +	unsigned int tsdw_cnt;
> > +};
> > +
> > +static struct sma1307_pll_match sma1307_pll_matches[] = {
> 
> const

Okay! 

> > +
> > +/* DB scale conversion of speaker volume */ static const
> > +DECLARE_TLV_DB_SCALE(sma1307_spk_tlv, -6000, 50, 0);
> > +
> > +static int sma1307_regmap_write(struct sma1307_priv *sma1307,
> > +				unsigned int reg, unsigned int val) {
> > +	int ret = 0;
> > +	int cnt = sma1307->retry_cnt;
> > +
> > +	while (cnt--) {
> 
> Sorry, but why? What is so broken in this hardware that it requires such
> retries? Maybe just youro board is broken, not this codec?
> 

Generally, if there are no issues, retries shold not occur and it should execute only once.
Could this be a problem?

I included the retries because there might be issues with the connection.


> > +
> > +static void sma1307_remove(struct snd_soc_component *component) {
> > +	struct sma1307_priv *sma1307 =
> > +snd_soc_component_get_drvdata(component);
> > +
> > +	cancel_delayed_work_sync(&sma1307->check_fault_work);
> 
> Why do you cancel work in two different places?



> > +
> > +	sma1307 = devm_kzalloc(&client->dev,
> > +			       sizeof(struct sma1307_priv), GFP_KERNEL);
> 
> sizeof(*)
> 

Sorry. What does it mean?


> > +
> > +	ret = devm_snd_soc_register_component(&client->dev,
> > +					      &sma1307_component, sma1307_dai,
> > +					      1);
> > +
> 
> Drop blank line

Okay, When using 'Lindent', line breaks like this.
I'll review and fix this.
Thanks!


> > +
> > +static const struct i2c_device_id sma1307_i2c_id[] = {
> > +	{ "sma1307", 0 },
> > +	{ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, sma1307_i2c_id);
> > +
> > +static const struct of_device_id sma1307_of_match[] = {
> > +	{.compatible = "irondevice,sma1307a", },
> > +	{.compatible = "irondevice,sma1307aq", },	//AEC-Q100 Qualificated
> 
> Drop, useless. Also does not match your I2C ID table.

The SMA1307A and SMA1307AQ are different chips with different device settings.
Therefore, when registering in the devicetree, I intended to register the specific product and
Cofigure the device settings accordingly.

I set the I2C ID table to just 'sma1307' as shown.
So, should I change it to 'sma1307a' and 'sma1307aq instead?

> Anyway, bindings are before their users.
> 
> 
> > +	{ }
> > +/* SMA1307 Registers Bit Fields */
> > +/* Power On/Off */
> > +#define SMA1307_POWER_MASK			(1<<0)
> > +#define SMA1307_POWER_OFF			(0<<0)
> > +#define SMA1307_POWER_ON			(1<<0)
> 
> Use BIT() everywhere.
> 

Okay, thanks!

> Best regards,
> Krzysztof