linux-kernel - Re: [PATCH v3] soundwire: SDCA: add helper macro to access controls

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] [day] [month] [year] [list]

Message-ID: <s5hr1pfr7qk.wl-tiwai@suse.de>
Date:   Fri, 30 Oct 2020 17:36:35 +0100
From:   Takashi Iwai <tiwai@...e.de>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
Cc:     "Liao, Bard" <bard.liao@...el.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "vinod.koul@...aro.org" <vinod.koul@...aro.org>,
        "Lin, Mengdong" <mengdong.lin@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "hui.wang@...onical.com" <hui.wang@...onical.com>,
        "vkoul@...nel.org" <vkoul@...nel.org>,
        "srinivas.kandagatla@...aro.org" <srinivas.kandagatla@...aro.org>,
        "ranjani.sridharan@...ux.intel.com" 
        <ranjani.sridharan@...ux.intel.com>,
        "jank@...ence.com" <jank@...ence.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        "Kale, Sanyog R" <sanyog.r.kale@...el.com>,
        Bard Liao <yung-chuan.liao@...ux.intel.com>,
        "rander.wang@...ux.intel.com" <rander.wang@...ux.intel.com>
Subject: Re: [PATCH v3] soundwire: SDCA: add helper macro to access controls

On Fri, 30 Oct 2020 16:52:24 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> 
> >>>> +#define SDW_SDCA_CTL(fun, ent, ctl, ch)		(BIT(30) |
> >>> 		\
> >>>> +						 (((fun) & 0x7) << 22) |	\
> >>>> +						 (((ent) & 0x40) << 15) |	\
> >>>> +						 (((ent) & 0x3f) << 7) |	\
> >>>> +						 (((ctl) & 0x30) << 15) |	\
> >>>> +						 (((ctl) & 0x0f) << 3) |	\
> >>>> +						 (((ch) & 0x38) << 12) |	\
> >>>> +						 ((ch) & 0x07))
> >>>> +
> >>>> +#define SDW_SDCA_MBQ_CTL(reg)			((reg) | BIT(13))
> >>>> +#define SDW_SDCA_NEXT_CTL(reg)			((reg) | BIT(14))
> >>>> +
> >>>>   #endif /* __SDW_REGISTERS_H */
> >>>
> >>>
> >>> No users of these macros?
> >>
> >> SDW_SDCA_CTL is used in sdca codec drivers which are not upstream yet.
> >> SDW_SDCA_MBQ_CTL will be used in a new regmap method.
> >> SDW_SDCA_NEXT_CTL can be used in sdca codec drivers, too.
> >
> > Well, the point is that it's hard to review without seeing how the
> > code of actual users are.
> 
> Agree, but our job is not made easy by the three-way dependency on
> regmap, SoundWire before we can submit ASoC codec drivers (developed
> by Realtek and tested by Intel).
> 
> If you prefer us to send all patches for SDCA codec support in one
> shot, that would be fine with us.

It's not necessarily mandatory to send the whole series, but if a
relevant code is already available, mentioning a repo URL in the patch
description (or in the comment below the delimiter) would be helpful,
for example.

> > BTW, the bit definitions can be simplified with GENMASK().
> > I personally don't think GENMASK() necessarily good, but it may fit
> > better in a case like this.
> 
> we use this macro in switch cases, e.g. for regmap properties to
> define read/volatile registers:
> 
> case SDW_SDCA_CTL(FUN_JACK_CODEC, RT711_SDCA_ENT_GE49,
> RT711_SDCA_CTL_SELECTED_MODE, 0):
> 	case SDW_SDCA_CTL(FUN_JACK_CODEC, RT711_SDCA_ENT_GE49,
> RT711_SDCA_CTL_DETECTED_MODE, 0):
> 	case SDW_SDCA_CTL(FUN_HID, RT711_SDCA_ENT_HID01,
> RT711_SDCA_CTL_HIDTX_CURRENT_OWNER, 0) ...
> 		SDW_SDCA_CTL(FUN_HID, RT711_SDCA_ENT_HID01,
> RT711_SDCA_CTL_HIDTX_MESSAGE_LENGTH, 0):
> 	case RT711_BUF_ADDR_HID1 ... RT711_BUF_ADDR_HID2:
> 		return true;
> 
> https://github.com/thesofproject/linux/blob/70fe32e776dafb4b03581d62a4569f65c2f13ada/sound/soc/codecs/rt711-sdca-sdw.c#L35
> 
> and unfortunately all our attempts to use FIELD_PREP, FIELD_GET,
> u32_encode, as suggested by Vinod, failed for this case due to
> compilation issues (can't use these macros outside of a function
> scope). The errors were shared with Vinod.
> 
> That's why we went back to the initial suggestion to deal with the
> shifts/masks by hand. For now we don't have a better solution that
> works in all cases were the macro is used.

Hrm, OK, in this case the value is masked then shifted, so it's not
trivial to deal with a macro.


thanks,

Takashi