Re: [alsa-devel] [PATCH] ASoC: rt1308: Add RT1308 amplifier driver
On Wed, Apr 17, 2019 at 11:40:28AM +0000, Derek [方德義] wrote:
Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to.
The ones here with comments look an awful lot like they're system specific config which should be left up to either platform data (or DT/ACPI configuration) or done at runtime (like gains). Some of the others like the pads configuration might also fit into that.
We don't open the DA gain setting for user at runtime, so have to set a default level in the init list.
That's not the way Linux drivers generally work... is there some technical reason for that or is it just some kind of policy decision?
+static void rt1308_reset(struct regmap *regmap) {
- regmap_write(regmap, RT1308_RESET, 0); }
If we do need the init list shouldn't we also do a reninit every time we do a reset?
The init list is only set at driver probe, not every time after resetting.
This is what I'm pointing out; if you don't restore these values then why set them at all?
On Wed, Apr 17, 2019 at 11:40:28AM +0000, Derek wrote:
Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to.
The ones here with comments look an awful lot like they're system specific config which should be left up to either platform data (or DT/ACPI configuration) or done at runtime (like gains). Some of the others like the pads configuration might also fit into that.
We don't open the DA gain setting for user at runtime, so have to set a default level in the init list.
That's not the way Linux drivers generally work... is there some technical reason for that or is it just some kind of policy decision?
Maybe I could add a gain control for the user and set the gain to safety max level in init list for speaker protection.
+static void rt1308_reset(struct regmap *regmap) {
- regmap_write(regmap, RT1308_RESET, 0); }
If we do need the init list shouldn't we also do a reninit every time we do a
reset?
The init list is only set at driver probe, not every time after resetting.
This is what I'm pointing out; if you don't restore these values then why set them at all?
OK, I could reinit after every time resetting.
------Please consider the environment before printing this e-mail.
On Tue, Apr 23, 2019 at 01:39:59PM +0000, Derek [方德義] wrote:
On Wed, Apr 17, 2019 at 11:40:28AM +0000, Derek wrote:
Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to.
Please don't ignore review comments, people are generally making them for a reason and are likely to have the same concerns if issues remain unaddressed. Having to repeat the same comments can get repetitive and make people question the value of time spent reviewing. If you disagree with the review comments that's fine but you need to reply and discuss your concerns so that the reviewer can understand your decisions.
The ones here with comments look an awful lot like they're system specific config which should be left up to either platform data (or DT/ACPI configuration) or done at runtime (like gains). Some of the others like the pads configuration might also fit into that.
We don't open the DA gain setting for user at runtime, so have to set a default level in the init list.
That's not the way Linux drivers generally work... is there some technical reason for that or is it just some kind of policy decision?
Maybe I could add a gain control for the user and set the gain to safety max level in init list for speaker protection.
The maximum volume can be limited with snd_soc_limit_volume() but if the limit is for the speaker then that's machine dependent anyway.
Subject: Re: [PATCH] ASoC: rt1308: Add RT1308 amplifier driver
On Tue, Apr 23, 2019 at 01:39:59PM +0000, Derek wrote:
On Wed, Apr 17, 2019 at 11:40:28AM +0000, Derek wrote:
Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to.
Sorry for the neglect of mail format. I will pay attention.
Please don't ignore review comments, people are generally making them for a reason and are likely to have the same concerns if issues remain unaddressed. Having to repeat the same comments can get repetitive and make people question the value of time spent reviewing. If you disagree with the review comments that's fine but you need to reply and discuss your concerns so that the reviewer can understand your decisions.
The ones here with comments look an awful lot like they're system specific config which should be left up to either platform data (or DT/ACPI configuration) or done at runtime (like gains). Some of the others like the pads configuration might also fit into that.
We don't open the DA gain setting for user at runtime, so have to set a default level in the init list.
That's not the way Linux drivers generally work... is there some technical reason for that or is it just some kind of policy decision?
Maybe I could add a gain control for the user and set the gain to safety max level in init list for speaker protection.
The maximum volume can be limited with snd_soc_limit_volume() but if the limit is for the speaker then that's machine dependent anyway.
The maximum volume setting is temporarily not machine dependent. It avoids harming if any machine doesn't set the limit volume.
------Please consider the environment before printing this e-mail.
On Wed, May 08, 2019 at 01:17:54PM +0000, Derek [方德義] wrote:
The maximum volume can be limited with snd_soc_limit_volume() but if the limit is for the speaker then that's machine dependent anyway.
The maximum volume setting is temporarily not machine dependent. It avoids harming if any machine doesn't set the limit volume.
I'm confused about what this limit is - is it just some arbatrary limit imposed in case some board has problems or is it an actual limit that comes from the chip? If it's a "just in case" limit then the board should be the one doing the limiting, yes people can break things but it's difficult to get decision like that done sensibly in the CODEC driver in a way that works for everything and the general expectation is that this is all for use by system integrators rather than end users.
Subject: Re: [PATCH] ASoC: rt1308: Add RT1308 amplifier driver
On Wed, May 08, 2019 at 01:17:54PM +0000, Derek wrote:
The maximum volume can be limited with snd_soc_limit_volume() but if the limit is for the speaker then that's machine dependent anyway.
The maximum volume setting is temporarily not machine dependent. It avoids harming if any machine doesn't set the limit volume.
I'm confused about what this limit is - is it just some arbatrary limit imposed in case some board has problems or is it an actual limit that comes from the chip? If it's a "just in case" limit then the board should be the one doing the limiting, yes people can break things but it's difficult to get decision like that done sensibly in the CODEC driver in a way that works for everything and the general expectation is that this is all for use by system integrators rather than end users.
It is an actual limit that comes from the chip.
------Please consider the environment before printing this e-mail.
On Mon, May 13, 2019 at 01:54:08PM +0000, Derek [方德義] wrote:
I'm confused about what this limit is - is it just some arbatrary limit imposed in case some board has problems or is it an actual limit that comes from the chip? If it's a "just in case" limit then the board should be the one doing the limiting, yes people can break things but it's difficult to get decision like that done sensibly in the CODEC driver in a way that works for everything and the general expectation is that this is all for use by system integrators rather than end users.
It is an actual limit that comes from the chip.
If there is an actual limit in the chip the driver just shouldn't expose anything beyond what that limit is - the original thing I was calling out was that you're just hard coding a value for this setting into the driver, that just means that the expectation is that the values should be user settable within whatever the chip can support. If there's settings that can't be supported at all the driver doesn't need to pretend to offer them.
Subject: Re: [PATCH] ASoC: rt1308: Add RT1308 amplifier driver
On Mon, May 13, 2019 at 01:54:08PM +0000, Derek wrote:
I'm confused about what this limit is - is it just some arbatrary limit imposed in case some board has problems or is it an actual limit that comes from the chip? If it's a "just in case" limit then the board should be the one doing the limiting, yes people can break things but it's difficult to get decision like that done sensibly in the CODEC driver in a way that works for everything and the general expectation is that this is all for use by system integrators rather than end users.
It is an actual limit that comes from the chip.
If there is an actual limit in the chip the driver just shouldn't expose anything beyond what that limit is - the original thing I was calling out was that you're just hard coding a value for this setting into the driver, that just means that the expectation is that the values should be user settable within whatever the chip can support. If there's settings that can't be supported at all the driver doesn't need to pretend to offer them.
I could remove comment for the limit setting in register patch/init list to avoid users or integrators have any expectation. And also there will not be any support at all the driver according to the limit setting.
------Please consider the environment before printing this e-mail.
On Tue, May 14, 2019 at 11:10:58AM +0000, Derek [方德義] wrote:
I could remove comment for the limit setting in register patch/init list to avoid users or integrators have any expectation. And also there will not be any support at all the driver according to the limit setting.
It's better to have a comment than not have one, I think I'd be happier if I understood what was actually being limited here - that might make it clear why it's not system specific which is what's concerning me.
Subject: Re: [PATCH] ASoC: rt1308: Add RT1308 amplifier driver
On Tue, May 14, 2019 at 11:10:58AM +0000, Derek [方德義] wrote:
I could remove comment for the limit setting in register patch/init list to avoid users or integrators have any expectation. And also there will not be any support at all the driver according to the limit setting.
It's better to have a comment than not have one, I think I'd be happier if I understood what was actually being limited here - that might make it clear why it's not system specific which is what's concerning me.
I will give a comment that describes the limit setting reason clearly.
------Please consider the environment before printing this e-mail.
On Thu, May 16, 2019 at 01:46:53PM +0000, Derek [方德義] wrote:
It's better to have a comment than not have one, I think I'd be happier if I understood what was actually being limited here - that might make it clear why it's not system specific which is what's concerning me.
I will give a comment that describes the limit setting reason clearly.
OK.
participants (2)
-
Derek [方德義] -
Mark Brown