| Message ID | 1547729177-14317-1-git-send-email-viorel.suman@nxp.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add NXP AUDMIX device and machine drivers | expand |
On Thu, Jan 17, 2019 at 12:46:25PM +0000, Viorel Suman wrote: > The patchset adds NXP Audio Mixer (AUDMIX) device and machine > drivers and related DT bindings documentation. > > Changes since V2: > 1. Moved "dais" node from machine driver DTS node to device driver DTS node > as suggested by Rob. That was not what I suggested. You still have a virtual node which looks to me to be unnecessary. > > Changes since V1: > 1. Original patch split into distinct patches for the device driver and > DT binding documentation. > 2. Replaced AMIX with AUDMIX in both code and file names as it looks more > RM-compliant. > 3. Removed polarity control from CPU DAI driver as suggested by Nicolin. > 4. Added machine driver and related DT binding documentation. > > Viorel Suman (4): > ASoC: fsl: Add Audio Mixer CPU DAI driver > ASoC: add fsl_audmix DT binding documentation > ASoC: fsl: Add Audio Mixer machine driver > ASoC: add imx-audmix DT binding documentation > > .../devicetree/bindings/sound/fsl,audmix.txt | 50 ++ > .../devicetree/bindings/sound/imx-audmix.txt | 18 + > sound/soc/fsl/Kconfig | 16 + > sound/soc/fsl/Makefile | 5 + > sound/soc/fsl/fsl_audmix.c | 551 +++++++++++++++++++++ > sound/soc/fsl/fsl_audmix.h | 102 ++++ > sound/soc/fsl/imx-audmix.c | 334 +++++++++++++ > 7 files changed, 1076 insertions(+) > create mode 100644 Documentation/devicetree/bindings/sound/fsl,audmix.txt > create mode 100644 Documentation/devicetree/bindings/sound/imx-audmix.txt > create mode 100644 sound/soc/fsl/fsl_audmix.c > create mode 100644 sound/soc/fsl/fsl_audmix.h > create mode 100644 sound/soc/fsl/imx-audmix.c > > -- > 2.7.4 >
On Thu, Jan 17, 2019 at 12:46:25PM +0000, Viorel Suman wrote: > The patchset adds NXP Audio Mixer (AUDMIX) device and machine > drivers and related DT bindings documentation. > > Changes since V2: > 1. Moved "dais" node from machine driver DTS node to device driver DTS node > as suggested by Rob. Yea, it makes a lot of sense. Otherwise, since the connection between IP blocks is fixed inside the SoC, the virtual sound node would need to be put in the soc-level dtsi, which sounds odd to me. > Changes since V1: > 1. Original patch split into distinct patches for the device driver and > DT binding documentation. > 2. Replaced AMIX with AUDMIX in both code and file names as it looks more > RM-compliant. > 3. Removed polarity control from CPU DAI driver as suggested by Nicolin. > 4. Added machine driver and related DT binding documentation. > > Viorel Suman (4): > ASoC: fsl: Add Audio Mixer CPU DAI driver > ASoC: add fsl_audmix DT binding documentation > ASoC: fsl: Add Audio Mixer machine driver > ASoC: add imx-audmix DT binding documentation Reviewed-by: Nicolin Chen <nicoleotsuka@gmail.com>
On Thu, Jan 17, 2019 at 02:22:18PM -0800, Nicolin Chen wrote: > On Thu, Jan 17, 2019 at 12:46:25PM +0000, Viorel Suman wrote: > > The patchset adds NXP Audio Mixer (AUDMIX) device and machine > > drivers and related DT bindings documentation. > > > > Changes since V2: > > 1. Moved "dais" node from machine driver DTS node to device driver DTS node > > as suggested by Rob. > > Yea, it makes a lot of sense. Otherwise, since the connection > between IP blocks is fixed inside the SoC, the virtual sound > node would need to be put in the soc-level dtsi, which sounds > odd to me. > > > Changes since V1: > > 1. Original patch split into distinct patches for the device driver and > > DT binding documentation. > > 2. Replaced AMIX with AUDMIX in both code and file names as it looks more > > RM-compliant. > > 3. Removed polarity control from CPU DAI driver as suggested by Nicolin. > > 4. Added machine driver and related DT binding documentation. > > > > Viorel Suman (4): > > ASoC: fsl: Add Audio Mixer CPU DAI driver > > ASoC: add fsl_audmix DT binding documentation > > ASoC: fsl: Add Audio Mixer machine driver > > ASoC: add imx-audmix DT binding documentation > > Reviewed-by: Nicolin Chen <nicoleotsuka@gmail.com> Oops. Just saw Rob's new reply. The drivers look good to me though.
Hi Rob, Nicolin, All, On Jo, 2019-01-17 at 10:18 -0600, Rob Herring wrote: > On Thu, Jan 17, 2019 at 12:46:25PM +0000, Viorel Suman wrote: > > > > The patchset adds NXP Audio Mixer (AUDMIX) device and machine > > drivers and related DT bindings documentation. > > > > Changes since V2: > > 1. Moved "dais" node from machine driver DTS node to device driver > > DTS node > > as suggested by Rob. > That was not what I suggested. You still have a virtual node which > looks to me to be unnecessary. To me removing virtual node implies that AUDMIX machine driver (imx- audmix.c + virtual node) shall be removed and machine driver code merged into device driver (fsl_audmix.c + device node) - please let me know if my understanding is wrong. The implication is that this makes AUDMIX device driver bounded to a particular DAI type of interface - SAI. The original intention is to keep AUDMIX device driver DAI-agnostic. Indeed, currently the connection between AUDMIX and SAI IP blocks in i.MX8QM and i.MX8QXP is fixed inside the SoC, but on other platforms we may expect AUDMIX to be connected inside the SoC to other IP blocks - to ESAI interface for instance. At this moment it's a bit difficult for me to evaluate how critical is to keep the device driver DAI-agnostic, so if you think it's better to go now with a SAI bounded AUDMIX device driver - please confirm, I'll merge machine driver code into device driver. Thank you, Viorel > > > > > > > Changes since V1: > > 1. Original patch split into distinct patches for the device driver > > and > > DT binding documentation. > > 2. Replaced AMIX with AUDMIX in both code and file names as it > > looks more > > RM-compliant. > > 3. Removed polarity control from CPU DAI driver as suggested by > > Nicolin. > > 4. Added machine driver and related DT binding documentation. > > > > Viorel Suman (4): > > ASoC: fsl: Add Audio Mixer CPU DAI driver > > ASoC: add fsl_audmix DT binding documentation > > ASoC: fsl: Add Audio Mixer machine driver > > ASoC: add imx-audmix DT binding documentation > > > > .../devicetree/bindings/sound/fsl,audmix.txt | 50 ++ > > .../devicetree/bindings/sound/imx-audmix.txt | 18 + > > sound/soc/fsl/Kconfig | 16 + > > sound/soc/fsl/Makefile | 5 + > > sound/soc/fsl/fsl_audmix.c | 551 > > +++++++++++++++++++++ > > sound/soc/fsl/fsl_audmix.h | 102 ++++ > > sound/soc/fsl/imx-audmix.c | 334 > > +++++++++++++ > > 7 files changed, 1076 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/sound/fsl,audmix.txt > > create mode 100644 Documentation/devicetree/bindings/sound/imx- > > audmix.txt > > create mode 100644 sound/soc/fsl/fsl_audmix.c > > create mode 100644 sound/soc/fsl/fsl_audmix.h > > create mode 100644 sound/soc/fsl/imx-audmix.c > >
On Fri, Jan 18, 2019 at 01:16:24PM +0000, Viorel Suman wrote: > > > 1. Moved "dais" node from machine driver DTS node to device driver > > > DTS node > > > as suggested by Rob. > > That was not what I suggested. You still have a virtual node which > > looks to me to be unnecessary. > > To me removing virtual node implies that AUDMIX machine driver (imx- > audmix.c + virtual node) shall be removed and machine driver code > merged into device driver (fsl_audmix.c + device node) - please let me > know if my understanding is wrong. We could use a non-DT configuration right? From the driver logic, DT just registers a device corresponding to the machine driver so that it can probe(). We could register one in fsl_audmix instead. Please refer to how fsl_ssi registers the sound card device. The machine driver can get audmix_np from the parent device pointer, and I think that's all you need. Or maybe someone else would provide a better way. But it'd work.
On Fri, Jan 18, 2019 at 11:46:42AM -0800, Nicolin Chen wrote: > On Fri, Jan 18, 2019 at 01:16:24PM +0000, Viorel Suman wrote: > > > > 1. Moved "dais" node from machine driver DTS node to device driver > > > > DTS node > > > > as suggested by Rob. > > > That was not what I suggested. You still have a virtual node which > > > looks to me to be unnecessary. > > > > To me removing virtual node implies that AUDMIX machine driver (imx- > > audmix.c + virtual node) shall be removed and machine driver code > > merged into device driver (fsl_audmix.c + device node) - please let me > > know if my understanding is wrong. > > We could use a non-DT configuration right? From the driver logic, > DT just registers a device corresponding to the machine driver so > that it can probe(). We could register one in fsl_audmix instead. > Please refer to how fsl_ssi registers the sound card device. The > machine driver can get audmix_np from the parent device pointer, > and I think that's all you need. Yes. > Or maybe someone else would provide a better way. But it'd work. Or the machine driver could create the audmix device. That probably makes less sense, but either way there doesn't have to be a 1-1 correspondence of DT nodes and (platform) devices. I'm not an ASoC expert, but why can't the machine driver just control the audmix directly (with DAIs as separate drivers)? Is the audmix ever going to a be a component for a different machine driver? Rob
Hi Nicolin, On Vi, 2019-01-18 at 11:46 -0800, Nicolin Chen wrote: > On Fri, Jan 18, 2019 at 01:16:24PM +0000, Viorel Suman wrote: > > > > > > > > > > > > > 1. Moved "dais" node from machine driver DTS node to device > > > > driver > > > > DTS node > > > > as suggested by Rob. > > > That was not what I suggested. You still have a virtual node > > > which > > > looks to me to be unnecessary. > > To me removing virtual node implies that AUDMIX machine driver > > (imx- > > audmix.c + virtual node) shall be removed and machine driver code > > merged into device driver (fsl_audmix.c + device node) - please let > > me > > know if my understanding is wrong. > We could use a non-DT configuration right? From the driver logic, > DT just registers a device corresponding to the machine driver so > that it can probe(). We could register one in fsl_audmix instead. > Please refer to how fsl_ssi registers the sound card device. The > machine driver can get audmix_np from the parent device pointer, > and I think that's all you need. > > Or maybe someone else would provide a better way. But it'd work. Sent V4 - it implements the approach you suggested. Thank you for the hint, works well indeed. Regards, Viorel
Hi Rob, On Lu, 2019-01-21 at 09:23 -0600, Rob Herring wrote: > On Fri, Jan 18, 2019 at 11:46:42AM -0800, Nicolin Chen wrote: > > > > On Fri, Jan 18, 2019 at 01:16:24PM +0000, Viorel Suman wrote: > > > > > > > > > > > > > > > > > 1. Moved "dais" node from machine driver DTS node to device > > > > > driver > > > > > DTS node > > > > > as suggested by Rob. > > > > That was not what I suggested. You still have a virtual node > > > > which > > > > looks to me to be unnecessary. > > > To me removing virtual node implies that AUDMIX machine driver > > > (imx- > > > audmix.c + virtual node) shall be removed and machine driver code > > > merged into device driver (fsl_audmix.c + device node) - please > > > let me > > > know if my understanding is wrong. > > We could use a non-DT configuration right? From the driver logic, > > DT just registers a device corresponding to the machine driver so > > that it can probe(). We could register one in fsl_audmix instead. > > Please refer to how fsl_ssi registers the sound card device. The > > machine driver can get audmix_np from the parent device pointer, > > and I think that's all you need. > Yes. Thank you, sent V4 which implements the approach suggested by Nicolin. > > > > > Or maybe someone else would provide a better way. But it'd work. > Or the machine driver could create the audmix device. That probably > makes less sense, but either way there doesn't have to be a 1-1 > correspondence of DT nodes and (platform) devices. > > I'm not an ASoC expert, but why can't the machine driver just control > the audmix directly (with DAIs as separate drivers)? Is the audmix > ever going to a be a component for a different machine driver? > > Rob Currently I'm not aware of any information with regard to if audmix is ever going to work with other DAIs than SAI. Howerver from audmix IP block integration perspective the only requirement is that the input DAI must be connected to audmix over I2S bus, possible DAI options are SAI, ESAI, etc - I think the approach to mix both device and machine drivers into a single entity is not the best way to go. /Viorel