mbox series

[v2,0/3] regmap: add SoundWire 1.2 MBQ support

Message ID 20200901162225.33343-1-pierre-louis.bossart@linux.intel.com
Headers show
Series regmap: add SoundWire 1.2 MBQ support | expand

Message

Pierre-Louis Bossart Sept. 1, 2020, 4:22 p.m. UTC 
In preparation of the upstream contribution of SDCA (SoundWire Device
Class for Audio) ASoC codec drivers [1] [2], add regmap support
SoundWire 1.2 MBQ support. The MBQ (Multi-Byte Quantity) registers
need to be handled in a different way from regular 8-bit SoundWire
registers, their main application is going to be for volume/gain
controls.

The second patch was initially suggested for inclusion in the
SoundWire tree, and was modified to add more background information on
SDCA in the commit message as requested by Vinod Koul.

Changes since v1:
Rebased on regmap tree (conflict with SPI stuff).
Removed mod_devicetable.h header
Removed -EOPNOTSUPP error codes, use -ENOTSUPP
Added long description of SDCA
Used FIELD_PREP/GET as suggested by Vinod Koul
Added Bard Liao's Acked-by tag.

Pierre-Louis Bossart (3):
  regmap: sdw: add required header files
  soundwire: SDCA: add helper macro to access controls
  regmap: sdw: add support for SoundWire 1.2 MBQ

 drivers/base/regmap/Kconfig             |   6 +-
 drivers/base/regmap/Makefile            |   1 +
 drivers/base/regmap/regmap-sdw-mbq.c    | 101 ++++++++++++++++++++++++
 drivers/base/regmap/regmap-sdw.c        |   2 +
 include/linux/regmap.h                  |  21 +++++
 include/linux/soundwire/sdw_registers.h |  33 ++++++++
 6 files changed, 163 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/regmap/regmap-sdw-mbq.c


base-commit: d17343b87da422a59d99a3ed130573dbeb96c582

Comments

Pierre-Louis Bossart Sept. 9, 2020, 1:48 p.m. UTC | #1 
>>>> + *	25		0 (Reserved)
>>>> + *	24:22		Function Number [2:0]
>>>> + *	21		Entity[6]
>>>> + *	20:19		Control Selector[5:4]
>>>> + *	18		0 (Reserved)
>>>> + *	17:15		Control Number[5:3]
>>>> + *	14		Next
>>>> + *	13		MBQ
>>>> + *	12:7		Entity[5:0]
>>>> + *	6:3		Control Selector[3:0]
>>>> + *	2:0		Control Number[2:0]
>>>> + */
>>>> +
>>>> +#define SDW_SDCA_CTL(fun, ent, ctl, ch)						\
>>>> +	(BIT(30)							|	\
>>>
>>> Programmatically this is fine, but then since we are defining for the
>>> description above, IMO it would actually make sense for this to be defined
>>> as FIELD_PREP:
>>>
>>>           FIELD_PREP(GENMASK(30, 26), 1)
>>>
>>> or better
>>>
>>>           u32_encode_bits(GENMASK(30, 26), 1)
>>>
>>>> +	FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun)))	|	\
>>>
>>> Why not use u32_encode_bits(GENMASK(24, 22), (fun)) instead for this and
>>> below?
>>
>> Because your comment for the v1 review was to use FIELD_PREP/FIELD_GET, and
>> your other patches for bitfield access only use FIELD_PREP/FIELD_GET.
> 
> yes and looking at this, I feel u32_encode_bits(GENMASK(24, 22), (fun))
> would look better than FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun)))
> 
> Do you agree?

The Function (fun) case is the easy one: the value is not split in two.

But look at the entity case, it's split in two:

FIELD_PREP(BIT(21), FIELD_GET(BIT(6), (ent)))			FIELD_PREP(GENMASK(12, 
7), FIELD_GET(GENMASK(5, 0), (ent)))

same for control

FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl)))	|	
FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl)))	|	

and same for channel number

FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), (ch)))	|	
FIELD_PREP(GENMASK(2, 0), FIELD_GET(GENMASK(2, 0), (ch))))

I don't see how we can avoid using the FIELD_GET to extract the relevant 
bits from entity, control, channel number values.

Or I am missing your point completely.


>>> And while at it, consider defining masks for various fields rather than
>>> using numbers in GENMASK() above, that would look better, be more
>>> readable and people can reuse it.
>>
>> Actually on this one I disagree. These fields are not intended to be used by
>> anyone, the goal is precisely to hide them behind regmap, and the use of raw
>> numbers makes it easier to cross-check the documentation and the code.
>> Adding a separate set of definitions would not increase readability.
> 
> Which one would you prefer:
> 
>          #define SDCA_FUN_MASK           GENMASK(24, 22)
> 
>          foo |= u32_encode_bits(SDCA_FUN_MASK, fun)
> 
> Or the one proposed...?

Same as above, let's see what this does with the control case where we'd 
need to have four definitions:

#define SDCA_CONTROL_DEST_MASK1 GENMASK(20, 19)
#define SDCA_CONTROL_ORIG_MASK1 GENMASK(5, 4)
#define SDCA_CONTROL_DEST_MASK2 GENMASK(6, 3)
#define SDCA_CONTROL_ORIG_MASK2 GENMASK(3, 0)

And the code would look like

foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK1, 
FIELD_GET(SDCA_CONTROL_ORIG_MASK1, fun));
foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK2, 
FIELD_GET(SDCA_CONTROL_ORIG_MASK2, fun));

The original suggestion was:

FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl)))	|	
FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl)))	|	

I prefer the original... Adding these defines doesn't really add value 
because
a) the values will not be reused anywhere else.
b) we need 12 of those defines
b) we need a prefix for those defines which makes the code heavier
Vinod Koul Sept. 10, 2020, 6:22 a.m. UTC | #2 
On 09-09-20, 08:48, Pierre-Louis Bossart wrote:
> 
> > > > > + *	25		0 (Reserved)
> > > > > + *	24:22		Function Number [2:0]
> > > > > + *	21		Entity[6]
> > > > > + *	20:19		Control Selector[5:4]
> > > > > + *	18		0 (Reserved)
> > > > > + *	17:15		Control Number[5:3]
> > > > > + *	14		Next
> > > > > + *	13		MBQ
> > > > > + *	12:7		Entity[5:0]
> > > > > + *	6:3		Control Selector[3:0]
> > > > > + *	2:0		Control Number[2:0]
> > > > > + */
> > > > > +
> > > > > +#define SDW_SDCA_CTL(fun, ent, ctl, ch)						\
> > > > > +	(BIT(30)							|	\
> > > > 
> > > > Programmatically this is fine, but then since we are defining for the
> > > > description above, IMO it would actually make sense for this to be defined
> > > > as FIELD_PREP:
> > > > 
> > > >           FIELD_PREP(GENMASK(30, 26), 1)
> > > > 
> > > > or better
> > > > 
> > > >           u32_encode_bits(GENMASK(30, 26), 1)
> > > > 
> > > > > +	FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun)))	|	\
> > > > 
> > > > Why not use u32_encode_bits(GENMASK(24, 22), (fun)) instead for this and
> > > > below?
> > > 
> > > Because your comment for the v1 review was to use FIELD_PREP/FIELD_GET, and
> > > your other patches for bitfield access only use FIELD_PREP/FIELD_GET.
> > 
> > yes and looking at this, I feel u32_encode_bits(GENMASK(24, 22), (fun))
> > would look better than FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun)))
> > 
> > Do you agree?
> 
> The Function (fun) case is the easy one: the value is not split in two.
> 
> But look at the entity case, it's split in two:
> 
> FIELD_PREP(BIT(21), FIELD_GET(BIT(6), (ent)))			FIELD_PREP(GENMASK(12, 7),
> FIELD_GET(GENMASK(5, 0), (ent)))
> 
> same for control
> 
> FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl)))	|	
> FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl)))	|	
> 
> and same for channel number
> 
> FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), (ch)))	|	
> FIELD_PREP(GENMASK(2, 0), FIELD_GET(GENMASK(2, 0), (ch))))
> 
> I don't see how we can avoid using the FIELD_GET to extract the relevant
> bits from entity, control, channel number values.

No, you dont need FIELD_GET, that would be pointless for this helper if
that was the case

> 
> Or I am missing your point completely.

Correct

It should be:

        foo |= u32_encode_bits(val, FOO_MASK_A);

which would write val into bits represented by FOO_MASK_A by
appropriately shifting val and masking it with FOO_MASK_A

So net result is bits in FOO_MASK_A are modified with val, rest of the
bits are not touched

> 
> 
> > > > And while at it, consider defining masks for various fields rather than
> > > > using numbers in GENMASK() above, that would look better, be more
> > > > readable and people can reuse it.
> > > 
> > > Actually on this one I disagree. These fields are not intended to be used by
> > > anyone, the goal is precisely to hide them behind regmap, and the use of raw
> > > numbers makes it easier to cross-check the documentation and the code.
> > > Adding a separate set of definitions would not increase readability.
> > 
> > Which one would you prefer:
> > 
> >          #define SDCA_FUN_MASK           GENMASK(24, 22)
> > 
> >          foo |= u32_encode_bits(SDCA_FUN_MASK, fun)
> > 
> > Or the one proposed...?
> 
> Same as above, let's see what this does with the control case where we'd
> need to have four definitions:
> 
> #define SDCA_CONTROL_DEST_MASK1 GENMASK(20, 19)
> #define SDCA_CONTROL_ORIG_MASK1 GENMASK(5, 4)
> #define SDCA_CONTROL_DEST_MASK2 GENMASK(6, 3)
> #define SDCA_CONTROL_ORIG_MASK2 GENMASK(3, 0)
> 
> And the code would look like
> 
> foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK1,
> FIELD_GET(SDCA_CONTROL_ORIG_MASK1, fun));
> foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK2,
> FIELD_GET(SDCA_CONTROL_ORIG_MASK2, fun));
> 
> The original suggestion was:
> 
> FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl)))	|	
> FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl)))	|	
> 
> I prefer the original... Adding these defines doesn't really add value
> because
> a) the values will not be reused anywhere else.
> b) we need 12 of those defines
> b) we need a prefix for those defines which makes the code heavier
Pierre-Louis Bossart Sept. 10, 2020, 1:53 p.m. UTC | #3 
On 9/10/20 1:22 AM, Vinod Koul wrote:
> On 09-09-20, 08:48, Pierre-Louis Bossart wrote:
>>
>>>>>> + *	25		0 (Reserved)
>>>>>> + *	24:22		Function Number [2:0]
>>>>>> + *	21		Entity[6]
>>>>>> + *	20:19		Control Selector[5:4]
>>>>>> + *	18		0 (Reserved)
>>>>>> + *	17:15		Control Number[5:3]
>>>>>> + *	14		Next
>>>>>> + *	13		MBQ
>>>>>> + *	12:7		Entity[5:0]
>>>>>> + *	6:3		Control Selector[3:0]
>>>>>> + *	2:0		Control Number[2:0]
>>>>>> + */
>>>>>> +
>>>>>> +#define SDW_SDCA_CTL(fun, ent, ctl, ch)						\
>>>>>> +	(BIT(30)							|	\
>>>>>
>>>>> Programmatically this is fine, but then since we are defining for the
>>>>> description above, IMO it would actually make sense for this to be defined
>>>>> as FIELD_PREP:
>>>>>
>>>>>            FIELD_PREP(GENMASK(30, 26), 1)
>>>>>
>>>>> or better
>>>>>
>>>>>            u32_encode_bits(GENMASK(30, 26), 1)
>>>>>
>>>>>> +	FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun)))	|	\
>>>>>
>>>>> Why not use u32_encode_bits(GENMASK(24, 22), (fun)) instead for this and
>>>>> below?
>>>>
>>>> Because your comment for the v1 review was to use FIELD_PREP/FIELD_GET, and
>>>> your other patches for bitfield access only use FIELD_PREP/FIELD_GET.
>>>
>>> yes and looking at this, I feel u32_encode_bits(GENMASK(24, 22), (fun))
>>> would look better than FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun)))
>>>
>>> Do you agree?
>>
>> The Function (fun) case is the easy one: the value is not split in two.
>>
>> But look at the entity case, it's split in two:
>>
>> FIELD_PREP(BIT(21), FIELD_GET(BIT(6), (ent)))			FIELD_PREP(GENMASK(12, 7),
>> FIELD_GET(GENMASK(5, 0), (ent)))
>>
>> same for control
>>
>> FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl)))	|	
>> FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl)))	|	
>>
>> and same for channel number
>>
>> FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), (ch)))	|	
>> FIELD_PREP(GENMASK(2, 0), FIELD_GET(GENMASK(2, 0), (ch))))
>>
>> I don't see how we can avoid using the FIELD_GET to extract the relevant
>> bits from entity, control, channel number values.
> 
> No, you dont need FIELD_GET, that would be pointless for this helper if
> that was the case

I don't get how one would specify which parts of the original value are 
extracted?

> 
>>
>> Or I am missing your point completely.
> 
> Correct
> 
> It should be:
> 
>          foo |= u32_encode_bits(val, FOO_MASK_A);
> 
> which would write val into bits represented by FOO_MASK_A by
> appropriately shifting val and masking it with FOO_MASK_A
> 
> So net result is bits in FOO_MASK_A are modified with val, rest of the
> bits are not touched

Vinod, please see the explanation below [1], we need to split the 
original value in two and insert the bits in two separate locations.

You only considered the simple case for the functions, your proposal 
will not work for entities, controls and channel numbers.

>>
>>
>>>>> And while at it, consider defining masks for various fields rather than
>>>>> using numbers in GENMASK() above, that would look better, be more
>>>>> readable and people can reuse it.
>>>>
>>>> Actually on this one I disagree. These fields are not intended to be used by
>>>> anyone, the goal is precisely to hide them behind regmap, and the use of raw
>>>> numbers makes it easier to cross-check the documentation and the code.
>>>> Adding a separate set of definitions would not increase readability.
>>>
>>> Which one would you prefer:
>>>
>>>           #define SDCA_FUN_MASK           GENMASK(24, 22)
>>>
>>>           foo |= u32_encode_bits(SDCA_FUN_MASK, fun)
>>>
>>> Or the one proposed...?
>>
>> Same as above, let's see what this does with the control case where we'd
>> need to have four definitions:

[1]

>>
>> #define SDCA_CONTROL_DEST_MASK1 GENMASK(20, 19)
>> #define SDCA_CONTROL_ORIG_MASK1 GENMASK(5, 4)
>> #define SDCA_CONTROL_DEST_MASK2 GENMASK(6, 3)
>> #define SDCA_CONTROL_ORIG_MASK2 GENMASK(3, 0)
>>
>> And the code would look like
>>
>> foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK1,
>> FIELD_GET(SDCA_CONTROL_ORIG_MASK1, fun));
>> foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK2,
>> FIELD_GET(SDCA_CONTROL_ORIG_MASK2, fun));
>>
>> The original suggestion was:
>>
>> FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl)))	|	
>> FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl)))	|	
>>
>> I prefer the original... Adding these defines doesn't really add value
>> because
>> a) the values will not be reused anywhere else.
>> b) we need 12 of those defines
>> b) we need a prefix for those defines which makes the code heavier
>
Vinod Koul Sept. 11, 2020, 7:06 a.m. UTC | #4 
On 10-09-20, 08:53, Pierre-Louis Bossart wrote:
> 
> 
> On 9/10/20 1:22 AM, Vinod Koul wrote:
> > On 09-09-20, 08:48, Pierre-Louis Bossart wrote:
> > > 
> > > > > > > + *	25		0 (Reserved)
> > > > > > > + *	24:22		Function Number [2:0]
> > > > > > > + *	21		Entity[6]
> > > > > > > + *	20:19		Control Selector[5:4]
> > > > > > > + *	18		0 (Reserved)
> > > > > > > + *	17:15		Control Number[5:3]
> > > > > > > + *	14		Next
> > > > > > > + *	13		MBQ
> > > > > > > + *	12:7		Entity[5:0]
> > > > > > > + *	6:3		Control Selector[3:0]
> > > > > > > + *	2:0		Control Number[2:0]
> > > > > > > + */
> > > > > > > +
> > > > > > > +#define SDW_SDCA_CTL(fun, ent, ctl, ch)						\
> > > > > > > +	(BIT(30)							|	\
> > > > > > 
> > > > > > Programmatically this is fine, but then since we are defining for the
> > > > > > description above, IMO it would actually make sense for this to be defined
> > > > > > as FIELD_PREP:
> > > > > > 
> > > > > >            FIELD_PREP(GENMASK(30, 26), 1)
> > > > > > 
> > > > > > or better
> > > > > > 
> > > > > >            u32_encode_bits(GENMASK(30, 26), 1)
> > > > > > 
> > > > > > > +	FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun)))	|	\
> > > > > > 
> > > > > > Why not use u32_encode_bits(GENMASK(24, 22), (fun)) instead for this and
> > > > > > below?
> > > > > 
> > > > > Because your comment for the v1 review was to use FIELD_PREP/FIELD_GET, and
> > > > > your other patches for bitfield access only use FIELD_PREP/FIELD_GET.
> > > > 
> > > > yes and looking at this, I feel u32_encode_bits(GENMASK(24, 22), (fun))
> > > > would look better than FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun)))
> > > > 
> > > > Do you agree?
> > > 
> > > The Function (fun) case is the easy one: the value is not split in two.
> > > 
> > > But look at the entity case, it's split in two:
> > > 
> > > FIELD_PREP(BIT(21), FIELD_GET(BIT(6), (ent)))			FIELD_PREP(GENMASK(12, 7),
> > > FIELD_GET(GENMASK(5, 0), (ent)))
> > > 
> > > same for control
> > > 
> > > FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl)))	|	
> > > FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl)))	|	
> > > 
> > > and same for channel number
> > > 
> > > FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), (ch)))	|	
> > > FIELD_PREP(GENMASK(2, 0), FIELD_GET(GENMASK(2, 0), (ch))))
> > > 
> > > I don't see how we can avoid using the FIELD_GET to extract the relevant
> > > bits from entity, control, channel number values.
> > 
> > No, you dont need FIELD_GET, that would be pointless for this helper if
> > that was the case
> 
> I don't get how one would specify which parts of the original value are
> extracted?
> 
> > 
> > > 
> > > Or I am missing your point completely.
> > 
> > Correct
> > 
> > It should be:
> > 
> >          foo |= u32_encode_bits(val, FOO_MASK_A);
> > 
> > which would write val into bits represented by FOO_MASK_A by
> > appropriately shifting val and masking it with FOO_MASK_A
> > 
> > So net result is bits in FOO_MASK_A are modified with val, rest of the
> > bits are not touched
> 
> Vinod, please see the explanation below [1], we need to split the original
> value in two and insert the bits in two separate locations.
> 
> You only considered the simple case for the functions, your proposal will
> not work for entities, controls and channel numbers.
> 
> > > 
> > > 
> > > > > > And while at it, consider defining masks for various fields rather than
> > > > > > using numbers in GENMASK() above, that would look better, be more
> > > > > > readable and people can reuse it.
> > > > > 
> > > > > Actually on this one I disagree. These fields are not intended to be used by
> > > > > anyone, the goal is precisely to hide them behind regmap, and the use of raw
> > > > > numbers makes it easier to cross-check the documentation and the code.
> > > > > Adding a separate set of definitions would not increase readability.
> > > > 
> > > > Which one would you prefer:
> > > > 
> > > >           #define SDCA_FUN_MASK           GENMASK(24, 22)
> > > > 
> > > >           foo |= u32_encode_bits(SDCA_FUN_MASK, fun)
> > > > 
> > > > Or the one proposed...?
> > > 
> > > Same as above, let's see what this does with the control case where we'd
> > > need to have four definitions:
> 
> [1]
> 
> > > 
> > > #define SDCA_CONTROL_DEST_MASK1 GENMASK(20, 19)
> > > #define SDCA_CONTROL_ORIG_MASK1 GENMASK(5, 4)
> > > #define SDCA_CONTROL_DEST_MASK2 GENMASK(6, 3)
> > > #define SDCA_CONTROL_ORIG_MASK2 GENMASK(3, 0)

I think I missed ORIG and DEST stuff, what does this mean here?

Relooking at the bit definition, for example 'Control Number' is defined
in both 17:15 as well as 2:0, why is that. Is it split?

How does one program a control number into this?

> > > 
> > > And the code would look like
> > > 
> > > foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK1,
> > > FIELD_GET(SDCA_CONTROL_ORIG_MASK1, fun));
> > > foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK2,
> > > FIELD_GET(SDCA_CONTROL_ORIG_MASK2, fun));
> > > 
> > > The original suggestion was:
> > > 
> > > FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl)))	|	
> > > FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl)))	|	
> > > 
> > > I prefer the original... Adding these defines doesn't really add value
> > > because
> > > a) the values will not be reused anywhere else.
> > > b) we need 12 of those defines
> > > b) we need a prefix for those defines which makes the code heavier
> >