Linaro-mm-sig June 2024

linaro-mm-sig@lists.linaro.org

26 participants
62 discussions

Re: [PATCH v6 03/16] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC

by Krzysztof Kozlowski

On 25/06/2024 11:23, Alexandre Mergnat wrote: > > > On 21/06/2024 17:00, Krzysztof Kozlowski wrote: >> On 19/06/2024 16:46, Alexandre Mergnat wrote: >>> Add the audio codec sub-device. This sub-device is used to set the >>> optional voltage values according to the hardware. >>> The properties are: >>> - Setup of microphone bias voltage. >>> - Setup of the speaker pin pull-down. >>> >>> Also, add the audio power supply property which is dedicated for >>> the audio codec sub-device. >>> >>> Signed-off-by: Alexandre Mergnat<amergnat(a)baylibre.com> >>> --- >>> .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33 ++++++++++++++++++++++ >>> 1 file changed, 33 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>> index 37423c2e0fdf..d95307393e75 100644 >>> --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>> +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>> @@ -37,6 +37,32 @@ properties: >>> "#interrupt-cells": >>> const: 2 >>> >>> + vaud28-supply: >>> + description: 2.8 volt supply phandle for the audio codec >>> + >>> + audio-codec: >>> + type: object >> Still not much improved. You do not have any resources there, so these >> should go to the parent node. > > Hi Krzysztof, > > vaud28-supply seems to be a mistake that I forward port. > In the V4, AFAII, your feedback [1] suggested me to move the vaud28-supply from the "audio-codec" > sub-node to the parent node, which for me is the "pmic" (mfd), because the property is considered as > power-supply. > > pwrap { > pmic { > ... > audio-codec { > ... > > Hardware side, vaud28-supply is the output of PMIC-regulator subsystem, and AVDD28 is the input of > PMIC-audio-codec subsystem. Then: > - The property name is wrong and must be change to AVDD28, which is a consumer (power input), not a > power-supply. => description: 2.8 volt power input for microphones (AU_VIN0, AU_VIN1, AU_VIN2) > - IMHO, move this property to the next parent (pwrap) isn't consistent. It should be moved back to > Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml (Done in the V4) into audio-codec > substystem, beside mediatek,micbias0-microvolt I don't understand why do we talk again about supply. My comment was not under the supply. Best regards, Krzysztof

6 months, 1 week

Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory

by Christian König

Am 25.06.24 um 13:02 schrieb Jason-JH Lin (林睿祥): > > Hi Christian, > > On Tue, 2024-05-21 at 20:36 +0200, Christian König wrote: > > Am 20.05.24 um 09:58 schrieb Yong Wu (吴勇): > > > On Thu, 2024-05-16 at 10:17 +0200, Christian König wrote: > > > > > > > > External email : Please do not click links or open attachments > > > > until > > > > you have verified the sender or the content. > > > > Am 15.05.24 um 13:23 schrieb Yong Wu: > > > > > Introduce a FLAG for the restricted memory which means the > > > > > memory > > > > > > > > is > > > > > protected by TEE or hypervisor, then it's inaccessiable for > > > > > kernel. > > > > > > > > > > Currently we don't use sg_dma_unmark_restricted, thus this > > > > > > > > interface > > > > > has not been added. > > > > > > > > Why should that be part of the scatterlist? It doesn't seem to > > > > affect > > > > any of it's functionality. > > > > > > > > As far as I can see the scatterlist shouldn't be the transport of > > > > this > > > > kind of information. > > > > > > Thanks for the review. I will remove this. > > > > > > In our user scenario, DRM will import these buffers and check if > > > this > > > is a restricted buffer. If yes, it will use secure GCE takes over. > > > > > > If this judgment is not suitable to be placed in scatterlist. I > > > don't > > > know if it is ok to limit this inside dma-buf. Adding such an > > > interface: > > > > > > static bool dma_buf_is_restricted(struct dma_buf *dmabuf) > > > { > > > return !strncmp(dmabuf->exp_name, "restricted", 10); > > > } > > > > No, usually stuff like that doesn't belong into DMA buf either. > > > > Question here really is who controls the security status of the > > memory > > backing the buffer? > > > > In other words who tells the exporter that it should allocate and > > fill a > > buffer with encrypted data? > > > > If that is userspace then that is part of the format information and > > it > > is also userspace who should tell the importer that it needs to work > > with encrypted data. > > > > The kernel is intentionally not involved in stuff like that. > > > > Here is the expected protected content buffer flow in DRM: > 1) userspace allocates a dma-buf FD from the "restricted_mtk_cma" by > DMA_HEAP_IOCTL_ALLOC. > 2) userspace imports that dma-buf into the device using prime for the > drm_file. > 3) userspace uses the already implemented driver import code for the > special cases of protected content buffer. What is so special on that case? > > In the step 3), we need to verify the dma-buf is allocated from > "restricted_mtk_cma", but there is no way to pass the secure flag or > private data from userspace to the import interface in DRM driver. Why do you need to verify that? > > So I can only verify it like this now: > struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device > *dev, struct dma_buf_attachment *attach, struct sg_table *sg) > { > struct mtk_gem_obj *mtk_gem; > > /* check if the entries in the sg_table are contiguous */ > if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) { > DRM_ERROR("sg_table is not contiguous"); > return ERR_PTR(-EINVAL); > } > mtk_gem = mtk_gem_init(dev, attach->dmabuf->size); > if (IS_ERR(mtk_gem)) > return ERR_CAST(mtk_gem); > > + mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted", > 10)); > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > mtk_gem->size = attach->dmabuf->size; > mtk_gem->sg = sg; > > return &mtk_gem->base; > } Complete NAK from my side to that approach. Importing of a DMA-buf should be independent of the exporter. What you could do is to provide the secure buffer from a device and not a device heap. > I think I have the same problem as the ECC_FLAG mention in: > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd04… > > I think it would be better to have the user configurable private > information in dma-buf, so all the drivers who have the same > requirement can get their private information from dma-buf directly and > no need to change or add the interface. > > What's your opinion in this point? Well of hand I don't see the need for that. What happens if you get a non-secure buffer imported in your secure device? Regards, Christian. > > Regards, > Jason-JH.Lin > > > Regards, > > Christian. > > ************* MEDIATEK Confidentiality Notice > ******************** > The information contained in this e-mail message (including any > attachments) may be confidential, proprietary, privileged, or otherwise > exempt from disclosure under applicable laws. It is intended to be > conveyed only to the designated recipient(s). Any use, dissemination, > distribution, printing, retaining or copying of this e-mail (including its > attachments) by unintended recipient(s) is strictly prohibited and may > be unlawful. If you are not an intended recipient of this e-mail, or believe > > that you have received this e-mail in error, please notify the sender > immediately (by replying to this e-mail), delete any and all copies of > this e-mail (including any attachments) from your system, and do not > disclose the content of this e-mail to any other person. Thank you!

6 months, 1 week

Re: [PATCH v6 12/16] ASoC: codecs: add MT6357 support

by Mark Brown

On Wed, Jun 19, 2024 at 04:46:48PM +0200, amergnat(a)baylibre.com wrote: > From: Nicolas Belin <nbelin(a)baylibre.com> > > Add the support of MT6357 PMIC audio codec. This breaks an x86 allmodconfig build: /build/stage/linux/sound/soc/codecs/mt6357.c: In function ‘mt_delay_250_event’: /build/stage/linux/sound/soc/codecs/mt6357.c:993:29: error: unused variable ‘pri v’ [-Werror=unused-variable] 993 | struct mt6357_priv *priv = snd_soc_component_get_drvdata(cmpnt); | ^~~~ /build/stage/linux/sound/soc/codecs/mt6357.c: In function ‘mt6357_platform_drive r_probe’: /build/stage/linux/sound/soc/codecs/mt6357.c:1867:55: error: too many arguments for format [-Werror=format-extra-args] 1867 | return dev_err_probe(&pdev->dev, ret, "Failed to parse d ts\n", __func__); | ^~~~~~~~~~~~~~~~~~ ~~~~~ cc1: all warnings being treated as errors

6 months, 1 week

Re: [PATCH v6 12/16] ASoC: codecs: add MT6357 support

by Mark Brown

On Wed, Jun 19, 2024 at 04:46:48PM +0200, amergnat(a)baylibre.com wrote: > + /* gain default values*/ > + regmap_update_bits(priv->regmap, MT6357_AUDENC_ANA_CON0, MT6357_AUDPREAMPLGAIN_MASK, > + UL_GAIN_0DB << MT6357_AUDPREAMPLGAIN_SFT); > + regmap_update_bits(priv->regmap, MT6357_AUDENC_ANA_CON1, MT6357_AUDPREAMPRGAIN_MASK, > + UL_GAIN_0DB << MT6357_AUDPREAMPRGAIN_SFT); > + > + regmap_update_bits(priv->regmap, MT6357_ZCD_CON1, > + MT6357_AUD_LOL_GAIN_MASK | > + MT6357_AUD_LOR_GAIN_MASK, > + DL_GAIN_0DB << MT6357_AUD_LOL_GAIN_SFT | > + DL_GAIN_0DB << MT6357_AUD_LOR_GAIN_SFT); > + > + regmap_update_bits(priv->regmap, MT6357_ZCD_CON2, > + MT6357_AUD_HPL_GAIN_MASK | > + MT6357_AUD_HPR_GAIN_MASK, > + DL_GAIN_0DB << MT6357_AUD_HPL_GAIN_SFT | > + DL_GAIN_0DB << MT6357_AUD_HPR_GAIN_SFT); > + > + regmap_update_bits(priv->regmap, MT6357_ZCD_CON3, > + MT6357_AUD_HS_GAIN_MASK, DL_GAIN_0DB); We generally leave everything at chip defaults, why is this different? > +static int mt6357_codec_probe(struct snd_soc_component *codec) > +{ > + struct mt6357_priv *priv = snd_soc_component_get_drvdata(codec); > + > + mt6357_codec_init(priv); > + return 0; > +} Why not just inline mt6357_codec_init() into the one user? > +static unsigned int mt6357_read(struct snd_soc_component *codec, unsigned int reg) > +{ > + struct mt6357_priv *priv = snd_soc_component_get_drvdata(codec); > + unsigned int val; > + > + regmap_read(priv->regmap, reg, &val); > + return val; > +} > + > +static int mt6357_write(struct snd_soc_component *codec, unsigned int reg, unsigned int value) > +{ > + struct mt6357_priv *priv = snd_soc_component_get_drvdata(codec); > + > + return regmap_update_bits(priv->regmap, reg, 0xffff, value); > +} Why open code these, the core has standard adaptors for regmap? > +static const u32 micbias_values[MT6357_MICBIAS_ARRAY_SIZE] = { > + 1700000, 1800000, 1900000, 2000000, > + 2100000, 2500000, 2600000, 2700000 > +}; Just use ARRAY_SIZE() for the size then the number can't be out of sync.

6 months, 1 week

Re: [PATCH v6 0/7] Add mediate-drm secure flow for SVP

by mripard＠kernel.org

Hi, On Tue, Jun 11, 2024 at 09:13:03AM GMT, Jason-JH Lin (林睿祥) wrote: > Hi Maxime, > > [snip] > > > > > > > --- > > > > > > TODO: > > > > > > 1) Drop MTK_DRM_IOCTL_GEM_CREATE and use DMA_HEAP_IOCTL_ALLOC > > > > > > in > > > > > > userspace > > > > > > 2) DRM driver use secure mailbox channel to handle normal and > > > > > > secure flow > > > > > > 3) Implement setting mmsys routing table in the secure world > > > > > > series > > > > > > > > > > I'm not sure what you mean here. Why are you trying to upstream > > > > > something that still needs to be removed from your patch > > > > > series? > > > > > > > > > > > > > Because their is too much patches need to be fixed in this > > > > series, > > > > so I > > > > list down the remaining TODO items and send to review for the > > > > other > > > > patches. > > > > > > > > Sorry for the bothering, I'll drop this at the next version. > > > > > > If you don't intend to use it, we just shouldn't add it. Removing > > > the > > > TODO item doesn't make sense, even more so if heaps should be the > > > way > > > you handle this. > > > > > > > Sorry for this misunderstanding. > > > > I mean I'll remove the DRM_IOCTL_GEM_CREATE patch and then change > > user > > space calling DMA_HEAP_IOCTL_ALLOC to allocate buffer from secure > > heap. > > > > I have changed user space to use DMA_HEAP_IOCTL_ALLOC to allocate > secure buffer, but I still encounter the problem of determining whether > the buffer is secure in mediatek-drm driver to add some secure > configure for hardware. > > > As the comment in [1], dma driver won't provide API for use. > [1]: > https://patchwork.kernel.org/project/linux-mediatek/patch/20240515112308.10… > > > So I use name checking at [PATCH v6 3/7] like this currently: > > struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device > *dev, > struct dma_buf_attachment *attach, struct sg_table *sg) > { > struct mtk_gem_obj *mtk_gem; > > /* check if the entries in the sg_table are contiguous */ > if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) { > DRM_ERROR("sg_table is not contiguous"); > return ERR_PTR(-EINVAL); > } > > mtk_gem = mtk_gem_init(dev, attach->dmabuf->size); > if (IS_ERR(mtk_gem)) > return ERR_CAST(mtk_gem); > > + mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted", > 10)); > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > + mtk_gem->size = attach->dmabuf->size; > mtk_gem->sg = sg; > > return &mtk_gem->base; > } > > But I want to change this name checking to the information brought from > user space. > I tried to use arg->flags to append the secure flag in user space and > call drmPrimeHandleToFD() to pass it to DRM driver, but it will be > blocked by at the beginning of the drm_prime_handle_to_fd_ioctl(). I agree with you, it's something to discuss mostly with the dma-buf maintainers but it would be better to just set a flag on the dma-buf, and use that flag whenever necessary. It might be related to the recent work I did to introduce allocation flags too: https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd04… Maxime

6 months, 1 week

Re: [RFC] Patch review challenges

by Lee Jones

On Fri, 21 Jun 2024, Markus Elfring wrote: > > The issue is one of communication and the way reviews are conducted. > > > > Reviewing other people's work is challenging and requires a certain > > skill-set, of which _excellent_ communication skills are non-negotiable. > > Patch feedback and change tolerance can vary also according to involved communities. Agreed. For this community, I suggest you build your skills for a while longer. > > Why not concentrate on more complex submissions for a while and grow > > your repertoire of common review points, > > Further collateral evolution can be considered there depending on > corresponding development resources. > > > rather than repeating the same few over and over? > > Some factors are probably known also according to corresponding statistics. > Several contributors are stumbling on recurring improvement possibilities > in published information. Right, this will always be true, however the few you've picked up on are not important enough to keep reiterating. By doing so, you're receiving undesirable attention. > > Reading other, more experienced maintainer's reviews would also be a good use > > of your time. > > I am trying to influence adjustments in desirable directions for a while. Never stop trying to improve. These are only my opinions of course. Take the advice or leave it. There's no need to reply to this. -- Lee Jones [李琼斯]

6 months, 1 week

Re: [v11 3/7] iio: core: Add new DMABUF interface infrastructure

by Lee Jones

On Fri, 21 Jun 2024, Markus Elfring wrote: > > Sadly, I am yet to see a constructive approach or even better a helpful > > patch which improve something, rather than vague suggestions on the list > > Can you get any more constructive impressions from another data representation? > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=… > > Are you aware how many change suggestions (also from my selection) are still > in various waiting queues? No one is doubting your overall contributions Markus. The issue is one of communication and the way reviews are conducted. Reviewing other people's work is challenging and requires a certain skill-set, of which _excellent_ communication skills are non-negotiable. Why not concentrate on more complex submissions for a while and grow your repertoire of common review points, rather than repeating the same few over and over? Reading other, more experienced maintainer's reviews would also be a good use of your time. -- Lee Jones [李琼斯]

6 months, 1 week

Re: [v11 3/7] iio: core: Add new DMABUF interface infrastructure

by Vinod Koul

On 20-06-24, 12:45, Markus Elfring wrote: > … > > All errors (new ones prefixed by >>): > > > >>> drivers/iio/industrialio-buffer.c:1715:3: error: cannot jump from this goto statement to its label > > 1715 | goto err_dmabuf_unmap_attachment; > … > > Which software design options would you like to try out next > so that such a questionable compilation error message will be avoided finally? The one where all emails from Markus go to dev/null -- ~Vinod

6 months, 1 week

Re: [Patch V3] i2c: imx-lpi2c: add eDMA mode support for LPI2C

by kernel test robot

Hi, kernel test robot noticed the following build warnings: [auto build test WARNING on andi-shyti/i2c/i2c-host] [also build test WARNING on linus/master v6.10-rc4 next-20240620] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/carlos-song-nxp-com/i2c-imx-… base: git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host patch link: https://lore.kernel.org/r/20240614045616.1751784-1-carlos.song%40nxp.com patch subject: [Patch V3] i2c: imx-lpi2c: add eDMA mode support for LPI2C config: loongarch-randconfig-r071-20240620 (https://download.01.org/0day-ci/archive/20240621/202406210604.bil4fzu2-lkp@…) compiler: loongarch64-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240621/202406210604.bil4fzu2-lkp@…) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp(a)intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202406210604.bil4fzu2-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/i2c/busses/i2c-imx-lpi2c.o: warning: objtool: unexpected relocation symbol type in .rela.discard.reachable -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki

6 months, 1 week

Re: [PATCH v12 3/7] iio: core: Add new DMABUF interface infrastructure

by Lee Jones

On Thu, 20 Jun 2024, Markus Elfring wrote: > … > > v11: > … > > - Use guard(mutex) > > > > v12: > > - Revert to mutex_lock/mutex_unlock in iio_buffer_attach_dmabuf(), > > as it uses cleanup GOTOs > … > > I would find it nice if better design options could gain acceptance. > Will the chances grow to adjust scopes another bit for involved variables > in such function implementations? > > A) Enclosing a source code part with extra curly brackets? > > B) scoped_guard()? > https://elixir.bootlin.com/linux/v6.10-rc4/source/include/linux/cleanup.h#L… > > C) Moving a locked source code part into a separate function implementation? I think it would help your cause if you quoted the exact piece of code you're referring to. Then tone down the language a bit - keep it as simple and natural as you can. Ex 1: Please place curly brackets around this section to aid with <reason> Ex 2: To save N lines of clean-up, please use scoped_guard() Ex 3: Moving out this chunk to another function would help with <reason> Etc. -- Lee Jones [李琼斯]

6 months, 1 week

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig June 2024