Re: [PATCH 1/3] clk: fix redefinition of clk_prepare on MIPS with HAVE_LEGACY_CLK
On Tue, Nov 17, 2020 at 11:41:57PM -0800, Stephen Boyd wrote: > Quoting Krzysztof Kozlowski (2020-11-15 09:09:48) > > COMMON_CLK even though is a user-selectable symbol, is still selected by > > multiple other config options. COMMON_CLK should not be used when > > legacy clocks are provided by architecture, so it correctly depends on > > !HAVE_LEGACY_CLK. > > > > However it is possible to create a config which selects both COMMON_CLK > > (by SND_SUN8I_CODEC) and HAVE_LEGACY_CLK (by SOC_RT305X) which leads to > > Why is SND_SUN8I_CODEC selecting COMMON_CLK? Or really, why is > SOC_RT305X selecting HAVE_LEGACY_CLK? The SND_SUN8I_CODEC I fixed in following patch (I sent separately v2 of it). The SOC_RT305X select HAVE_LEGACY_CLK? because it is an old, Ralink platform, not converted to Common clock frm. Few clock operations are defined in: arch/mips/ralink/clk.c Best regards, Krzysztof ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/3] clk: fix redefinition of clk_prepare on MIPS with HAVE_LEGACY_CLK
Quoting Krzysztof Kozlowski (2020-11-15 09:09:48) > COMMON_CLK even though is a user-selectable symbol, is still selected by > multiple other config options. COMMON_CLK should not be used when > legacy clocks are provided by architecture, so it correctly depends on > !HAVE_LEGACY_CLK. > > However it is possible to create a config which selects both COMMON_CLK > (by SND_SUN8I_CODEC) and HAVE_LEGACY_CLK (by SOC_RT305X) which leads to Why is SND_SUN8I_CODEC selecting COMMON_CLK? Or really, why is SOC_RT305X selecting HAVE_LEGACY_CLK? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[staging:staging-linus] BUILD SUCCESS 2dde2821b57f12fa8601d35d438b5e300fcbbe1d
defconfig mips allyesconfig mips allmodconfig powerpc allyesconfig powerpc allmodconfig powerpc allnoconfig i386 randconfig-a006-20201117 i386 randconfig-a005-20201117 i386 randconfig-a001-20201117 i386 randconfig-a002-20201117 i386 randconfig-a004-20201117 i386 randconfig-a003-20201117 x86_64 randconfig-a015-20201115 x86_64 randconfig-a011-20201115 x86_64 randconfig-a014-20201115 x86_64 randconfig-a013-20201115 x86_64 randconfig-a016-20201115 x86_64 randconfig-a012-20201115 i386 randconfig-a012-20201117 i386 randconfig-a014-20201117 i386 randconfig-a016-20201117 i386 randconfig-a011-20201117 i386 randconfig-a015-20201117 i386 randconfig-a013-20201117 riscvnommu_k210_defconfig riscvallyesconfig riscvnommu_virt_defconfig riscv allnoconfig riscv rv32_defconfig riscvallmodconfig x86_64 rhel x86_64 allyesconfig x86_64rhel-7.6-kselftests x86_64 defconfig x86_64 rhel-8.3 x86_64 kexec clang tested configs: x86_64 randconfig-a003-20201117 x86_64 randconfig-a005-20201117 x86_64 randconfig-a004-20201117 x86_64 randconfig-a002-20201117 x86_64 randconfig-a001-20201117 x86_64 randconfig-a006-20201117 x86_64 randconfig-a015-20201116 x86_64 randconfig-a011-20201116 x86_64 randconfig-a014-20201116 x86_64 randconfig-a013-20201116 x86_64 randconfig-a016-20201116 x86_64 randconfig-a012-20201116 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/8] regulator: hi6421v600-regulator: move it from staging
On Tue, Nov 17, 2020 at 09:38:30AM +0100, Mauro Carvalho Chehab wrote: > Mark Brown escreveu: > > This also appears to be missing a DT binding document, binding > > documentation is required for anything with DT support. > The DT binding is documented on patch 3/8, together with MFD. > As there's just one compatible for MFD and regulators altogether, > I opted to have just one DT binding document for both. I should've been copied on that patch then so the bindings can be reviewed. > > > +static DEFINE_MUTEX(enable_mutex); > > Drivers shouldn't be declaring global variables, if this is required it > > should be allocated in driver data. > I'll try to see a better place for this, but in this case, the > mutex should be global anyway, as the access to the SPMI bus > should be serialized. Surely the bus should be dealing with basic I/O serialisation? > > It looks like it would be less code overall to just implement a regmap > > for the MFD, even if it were only used in this driver - almost > > everything in here is just a carbon copy of standard helpers that the > > core provides for regmap devices. Doing it in the MFD would be more > > idiomatic for everything though. > I tried to use regmap for this driver while porting it from Linaro's > OOT to upstream, bkut it turns that adding support for it is not trivial. Could you expand on "not trivial" please? What did you try and what difficulties did you encounter? > I suspect that, just like I2C has their own set of regmap functions > (regmap_init_i2c, devm_regmap_init_i2c), if we want to properly support > regmap, we need first to add a SPMI variant to it, as the locking > should be bus-aware. drivers/base/regmap/regmap-spmi.c has been there since 2013, or if for some reason this device is doing something non-standard for the bus then the reg_read() and reg_write() operations can be used. > > > + ret = of_property_read_u32(np, "vsel-reg", >vsel_reg); > > There is no way this stuff should be being parsed out of DT, we should > > know the register map for the device and what regulators it has based > > purely off knowing what device we have. Baking the register map into > > the ABI is bad practice, it prevents us from improving our support for > > the hardware without going and updating people's DTs. > Well, I can move the existing registers out of DT, but, as I don't > have any datasheet for this device, it means that I can implement > there only a subset of the available LDOs. So, from the 36 LDOs that > this chip support, we only know the registers for 8 of them: > ldo3, ldo4, ldo9, ldo15, ldo16, ldo17, ldo33 and ldo34. People will be free to submit patches adding additional functionality to the driver if they need it. > > > + /* > > > + * Not all regulators work on idle mode > > > + */ > > > + ret = of_property_read_u32(np, "idle-mode-mask", >eco_mode_mask); > > There's no conditional code to not register the mode operations if the > > mode information is not available (and again this should be done based > > on knowing the properties of the device, this is unlikely to be system > > dependent). > This is the same case as before: as we don't have any datasheets, > I only know what's there at the DT for just one device (Hikey 970). > Again, we could hardcode those assuming Hikey 970 settings, but, > if some other device using the same chip will ever be added, and > they use some different configuration then we may face some > backward-compatibility issues. I can't follow the logic here, sorry. If we have no idea how to configure something how would offering operations to configure that thing be useful? > > > + constraint->valid_modes_mask = REGULATOR_MODE_NORMAL; > > This is absolutely not OK, a regulator driver should *not* be modifying > > the constraints that the machine has set. If it is safe to change modes > > on a platform and the system integrator wants to do that then they will > > set the constraints appropriately, there is no way the regulator driver > > can tell what is appropriate on a given system. The fact that the > > driver is including machine.h at all ought to have been an indicator > > that there's an abstraction problem here. > Ok. Where such constraints should be instead? at the Hikey 970 DT > file? They should be configured whereever all the other constraints are configured by the platform, if that is DT then they should also be configured in DT. signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/8] spmi: hi6421-spmi-pmic: move driver from staging
Hi Mauro,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.10-rc4 next-20201117]
[cannot apply to staging/staging-testing robh/for-next lee-mfd/for-mfd-next]
[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]
url:
https://github.com/0day-ci/linux/commits/Mauro-Carvalho-Chehab/Move-Hikey-970-USB-support-out-of-staging-and-add-DT/20201116-210334
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
09162bc32c880a791c6c0668ce0745cf7958f576
config: x86_64-randconfig-s022-20201115 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.3-107-gaf3512a6-dirty
#
https://github.com/0day-ci/linux/commit/b972250f20fc571defa4b23c9cc959df61eb0803
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Mauro-Carvalho-Chehab/Move-Hikey-970-USB-support-out-of-staging-and-add-DT/20201116-210334
git checkout b972250f20fc571defa4b23c9cc959df61eb0803
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
"sparse warnings: (new ones prefixed by >>)"
>> drivers/spmi/hisi-spmi-controller.c:164:24: sparse: sparse: cast to
>> restricted __be32
>> drivers/spmi/hisi-spmi-controller.c:164:24: sparse: sparse: cast to
>> restricted __be32
>> drivers/spmi/hisi-spmi-controller.c:164:24: sparse: sparse: cast to
>> restricted __be32
>> drivers/spmi/hisi-spmi-controller.c:164:24: sparse: sparse: cast to
>> restricted __be32
>> drivers/spmi/hisi-spmi-controller.c:164:24: sparse: sparse: cast to
>> restricted __be32
>> drivers/spmi/hisi-spmi-controller.c:164:24: sparse: sparse: cast to
>> restricted __be32
>> drivers/spmi/hisi-spmi-controller.c:239:25: sparse: sparse: cast from
>> restricted __be32
vim +164 drivers/spmi/hisi-spmi-controller.c
70f59c90c8199e8 drivers/staging/hikey9xx/hisi-spmi-controller.c Mayulong
2020-08-17 110
70f59c90c8199e8 drivers/staging/hikey9xx/hisi-spmi-controller.c Mayulong
2020-08-17 111 static int spmi_read_cmd(struct spmi_controller *ctrl,
7f3ac6c502fd7ff drivers/staging/hikey9xx/hisi-spmi-controller.c Mauro Carvalho
Chehab 2020-08-17 112u8 opc, u8 slave_id, u16 slave_addr,
u8 *__buf, size_t bc)
70f59c90c8199e8 drivers/staging/hikey9xx/hisi-spmi-controller.c Mayulong
2020-08-17 113 {
70f59c90c8199e8 drivers/staging/hikey9xx/hisi-spmi-controller.c Mayulong
2020-08-17 114 struct spmi_controller_dev *spmi_controller =
dev_get_drvdata(>dev);
7f3ac6c502fd7ff drivers/staging/hikey9xx/hisi-spmi-controller.c Mauro Carvalho
Chehab 2020-08-17 115 u32 chnl_ofst = SPMI_CHANNEL_OFFSET *
spmi_controller->channel;
70f59c90c8199e8 drivers/staging/hikey9xx/hisi-spmi-controller.c Mayulong
2020-08-17 116 unsigned long flags;
6af364501949d99 drivers/staging/hikey9xx/hisi-spmi-controller.c Mauro Carvalho
Chehab 2020-08-17 117 u8 *buf = __buf;
70f59c90c8199e8 drivers/staging/hikey9xx/hisi-spmi-controller.c Mayulong
2020-08-17 118 u32 cmd, data;
70f59c90c8199e8 drivers/staging/hikey9xx/hisi-spmi-controller.c Mayulong
2020-08-17 119 int rc;
70f59c90c8199e8 drivers/staging/hikey9xx/hisi-spmi-controller.c Mayulong
2020-08-17 120 u8 op_code, i;
70f59c90c8199e8 drivers/staging/hikey9xx/hisi-spmi-controller.c Mayulong
2020-08-17 121
70f59c90c8199e8 drivers/staging/hikey9xx/hisi-spmi-controller.c Mayulong
2020-08-17 122 if (bc > SPMI_CONTROLLER_MAX_TRANS_BYTES) {
4d914a8c480c312 drivers/staging/hikey9xx/hisi-spmi-controller.c Mauro Carvalho
Chehab 2020-08-17 123 dev_err(>dev,
4c6491a343e91a5 drivers/staging/hikey9xx/hisi-spmi-controller.c YueHaibing
2020-09-01 124 "spmi_controller supports 1..%d bytes
per trans, but:%zu requested\n",
4d914a8c480c312 drivers/staging/hikey9xx/hisi-spmi-controller.c Mauro Carvalho
Chehab 2020-08-17 125 SPMI_CONTROLLER_MAX_TRANS_BYTES, bc);
70f59c90c8199e8 drivers/staging/hikey9xx/hisi-spmi-controller.c Mayulong
2020-08-17 126 return -EINVAL;
70f59c90c8199e8 drivers/staging/hikey9xx/hisi-spmi-controller.c Mayulong
2020-08-17 127 }
70f59c90c8199e8 drivers/staging/hikey9xx/hisi-spmi-controller.c Mayulong
2020-08-17 128
7f3ac6c502fd7ff drivers/staging/hikey9xx/hisi-spmi-controller.c Mauro Carvalho
Chehab 2020-
Re: [PATCH 4/8] regulator: hi6421v600-regulator: move it from staging
On Tue, Nov 17, 2020 at 09:08:22AM +0100, Mauro Carvalho Chehab wrote: > Mark Brown escreveu: > > This probe code looks very different to other regulator drivers, this > > alone should have been a warning that the driver needs some substantial > > refactoring here. As indicated information about what regulators are > > present on devices and their properties should be in the driver not the > > DT, the driver should just be able to register them unconditionally and > > use of_match and regulators_node to allow the core to find any > > constraints that are provided by the platform. > The setup for MFD/regulator is different than almost all other > regulator drivers currently upstreamed[1]. It really shouldn't be doing anything unusual. > It means that, for the regulator driver to work, two drivers > should be probed first: the SPMI bus controller driver > (hisi-spmi-controller.c) and the SPMI bus client driver, which is > at the MFD driver(hi6421-spmi-pmic.c). > Only after having both probed, the regulator driver can be > probed. This is totally fine and very common for drivers in general, a combination of probe deferral and fw_devlink exists to sort this stuff out. > Also, as all the communication between the PM chip > and the SoC happens via a single serial bus, there's no > sense on probing the regulators in parallel. > That's mainly the reason why I opted to serialize the probe > inside hi6421v600-regulator.c. I can't think of a regulator driver that doesn't have an entirly serialized probe routine, that's not the issue. The issue is that almost nothing that the probe routine is doing is done by other regulator drivers. signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] drivers: most: add ALSA sound driver
On Tue, 2020-11-17 at 11:41 +0300, Dan Carpenter wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Tue, Nov 17, 2020 at 08:08:50AM +, christian.gr...@microchip.com wrote:
> > On Tue, 2020-11-10 at 11:48 +0300, Dan Carpenter wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > > the content is safe
> > >
> > > On Fri, Nov 06, 2020 at 05:30:54PM +0100, Christian Gromm wrote:
> > > > +static struct list_head adpt_list;
> > > > +
> > > > +#define MOST_PCM_INFO (SNDRV_PCM_INFO_MMAP | \
> > > > +SNDRV_PCM_INFO_MMAP_VALID | \
> > > > +SNDRV_PCM_INFO_BATCH | \
> > > > +SNDRV_PCM_INFO_INTERLEAVED | \
> > > > +SNDRV_PCM_INFO_BLOCK_TRANSFER)
> > > > +
> > > > +static void swap_copy16(u16 *dest, const u16 *source, unsigned int
> > > > bytes)
> > > > +{
> > > > + unsigned int i = 0;
> > > > +
> > > > + while (i < (bytes / 2)) {
> > > > + dest[i] = swab16(source[i]);
> > > > + i++;
> > > > + }
> > > > +}
> > > > +
> > > > +static void swap_copy24(u8 *dest, const u8 *source, unsigned int bytes)
> > > > +{
> > > > + unsigned int i = 0;
> > > > +
> > > > + while (i < bytes - 2) {
> > >
> > > Can bytes ever be zero? If so then this will corrupt memory and crash.
> > >
> > > Generally "int i;" is less risky than "unsigned int i;". Of course, I
> > > recently almost introduced a situation where we were copying up to
> > > ULONG_MAX bytes so there are times when iterators should be size_t so
> > > that does happen. It could be buggy either way is what I'm saying but
> > > generally "unsigned int i;" is more often buggy.
> > >
> > > > + dest[i] = source[i + 2];
> > > > + dest[i + 1] = source[i + 1];
> > > > + dest[i + 2] = source[i];
> > > > + i += 3;
> > > > + }
> > > > +}
> > > > +
> > > > +static void swap_copy32(u32 *dest, const u32 *source, unsigned int
> > > > bytes)
> > > > +{
> > > > + unsigned int i = 0;
> > > > +
> > > > + while (i < bytes / 4) {
> > > > + dest[i] = swab32(source[i]);
> > > > + i++;
> > > > + }
> > > > +}
> > > > +
> > > > +static void alsa_to_most_memcpy(void *alsa, void *most, unsigned int
> > > > bytes)
> > > > +{
> > > > + memcpy(most, alsa, bytes);
> > > > +}
> > > > +
> > > > +static void alsa_to_most_copy16(void *alsa, void *most, unsigned int
> > > > bytes)
> > > > +{
> > > > + swap_copy16(most, alsa, bytes);
> > > > +}
> > > > +
> > > > +static void alsa_to_most_copy24(void *alsa, void *most, unsigned int
> > > > bytes)
> > > > +{
> > > > + swap_copy24(most, alsa, bytes);
> > > > +}
> > > > +
> > > > +static void alsa_to_most_copy32(void *alsa, void *most, unsigned int
> > > > bytes)
> > > > +{
> > > > + swap_copy32(most, alsa, bytes);
> > > > +}
> > > > +
> > > > +static void most_to_alsa_memcpy(void *alsa, void *most, unsigned int
> > > > bytes)
> > > > +{
> > > > + memcpy(alsa, most, bytes);
> > > > +}
> > > > +
> > > > +static void most_to_alsa_copy16(void *alsa, void *most, unsigned int
> > > > bytes)
> > > > +{
> > > > + swap_copy16(alsa, most, bytes);
> > > > +}
> > > > +
> > > > +static void most_to_alsa_copy24(void *alsa, void *most, unsigned int
> > > > bytes)
> > > > +{
> > > > + swap_copy24(alsa, most, bytes);
> > > > +}
> > > > +
> > > > +static void most_to_alsa_copy32(void *alsa, void *most, unsigned int
> > > > bytes)
> > > > +{
> > > > + swap_copy32(alsa, most, bytes);
> > > > +}
> > > > +
> > > > +/**
> > > > + * get_channel - get pointer to channel
> > > > + * @iface: interface structure
> > > > + * @channel_id: channel ID
> > > > + *
> > > > + * This traverses the channel list and returns the channel matching the
> > > > + * ID and interface.
> > > > + *
> > > > + * Returns pointer to channel on success or NULL otherwise.
> > > > + */
> > > > +static struct channel *get_channel(struct most_interface *iface,
> > > > +int channel_id)
> > > > +{
> > > > + struct sound_adapter *adpt = iface->priv;
> > > > + struct channel *channel, *tmp;
> > > > +
> > > > + list_for_each_entry_safe(channel, tmp, >dev_list, list) {
> > > > + if ((channel->iface == iface) && (channel->id ==
> > > > channel_id))
> > > > + return channel;
> > >
> > > No need to use the _safe() version of this loop macro. You're not
> > > freeing anything. My concern is that sometimes people think the _safe()
> > > has something to do with locking and it does not.
> > >
> > > > + }
> > > > + return NULL;
> > > > +}
> > > > +
> > >
> > > [ Snip ]
> > >
> > > > +/**
> > > > + * audio_probe_channel - probe function of the driver module
> > > > + * @iface: pointer to interface instance
> > > > + * @channel_id: channel index/ID
> > > > + * @cfg: pointer to actual channel
Re: [PATCH v2] drivers: most: add ALSA sound driver
On Tue, 2020-11-10 at 11:48 +0300, Dan Carpenter wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Fri, Nov 06, 2020 at 05:30:54PM +0100, Christian Gromm wrote:
> > +static struct list_head adpt_list;
> > +
> > +#define MOST_PCM_INFO (SNDRV_PCM_INFO_MMAP | \
> > +SNDRV_PCM_INFO_MMAP_VALID | \
> > +SNDRV_PCM_INFO_BATCH | \
> > +SNDRV_PCM_INFO_INTERLEAVED | \
> > +SNDRV_PCM_INFO_BLOCK_TRANSFER)
> > +
> > +static void swap_copy16(u16 *dest, const u16 *source, unsigned int bytes)
> > +{
> > + unsigned int i = 0;
> > +
> > + while (i < (bytes / 2)) {
> > + dest[i] = swab16(source[i]);
> > + i++;
> > + }
> > +}
> > +
> > +static void swap_copy24(u8 *dest, const u8 *source, unsigned int bytes)
> > +{
> > + unsigned int i = 0;
> > +
> > + while (i < bytes - 2) {
>
> Can bytes ever be zero? If so then this will corrupt memory and crash.
>
> Generally "int i;" is less risky than "unsigned int i;". Of course, I
> recently almost introduced a situation where we were copying up to
> ULONG_MAX bytes so there are times when iterators should be size_t so
> that does happen. It could be buggy either way is what I'm saying but
> generally "unsigned int i;" is more often buggy.
>
> > + dest[i] = source[i + 2];
> > + dest[i + 1] = source[i + 1];
> > + dest[i + 2] = source[i];
> > + i += 3;
> > + }
> > +}
> > +
> > +static void swap_copy32(u32 *dest, const u32 *source, unsigned int bytes)
> > +{
> > + unsigned int i = 0;
> > +
> > + while (i < bytes / 4) {
> > + dest[i] = swab32(source[i]);
> > + i++;
> > + }
> > +}
> > +
> > +static void alsa_to_most_memcpy(void *alsa, void *most, unsigned int bytes)
> > +{
> > + memcpy(most, alsa, bytes);
> > +}
> > +
> > +static void alsa_to_most_copy16(void *alsa, void *most, unsigned int bytes)
> > +{
> > + swap_copy16(most, alsa, bytes);
> > +}
> > +
> > +static void alsa_to_most_copy24(void *alsa, void *most, unsigned int bytes)
> > +{
> > + swap_copy24(most, alsa, bytes);
> > +}
> > +
> > +static void alsa_to_most_copy32(void *alsa, void *most, unsigned int bytes)
> > +{
> > + swap_copy32(most, alsa, bytes);
> > +}
> > +
> > +static void most_to_alsa_memcpy(void *alsa, void *most, unsigned int bytes)
> > +{
> > + memcpy(alsa, most, bytes);
> > +}
> > +
> > +static void most_to_alsa_copy16(void *alsa, void *most, unsigned int bytes)
> > +{
> > + swap_copy16(alsa, most, bytes);
> > +}
> > +
> > +static void most_to_alsa_copy24(void *alsa, void *most, unsigned int bytes)
> > +{
> > + swap_copy24(alsa, most, bytes);
> > +}
> > +
> > +static void most_to_alsa_copy32(void *alsa, void *most, unsigned int bytes)
> > +{
> > + swap_copy32(alsa, most, bytes);
> > +}
> > +
> > +/**
> > + * get_channel - get pointer to channel
> > + * @iface: interface structure
> > + * @channel_id: channel ID
> > + *
> > + * This traverses the channel list and returns the channel matching the
> > + * ID and interface.
> > + *
> > + * Returns pointer to channel on success or NULL otherwise.
> > + */
> > +static struct channel *get_channel(struct most_interface *iface,
> > +int channel_id)
> > +{
> > + struct sound_adapter *adpt = iface->priv;
> > + struct channel *channel, *tmp;
> > +
> > + list_for_each_entry_safe(channel, tmp, >dev_list, list) {
> > + if ((channel->iface == iface) && (channel->id == channel_id))
> > + return channel;
>
> No need to use the _safe() version of this loop macro. You're not
> freeing anything. My concern is that sometimes people think the _safe()
> has something to do with locking and it does not.
>
> > + }
> > + return NULL;
> > +}
> > +
>
> [ Snip ]
>
> > +/**
> > + * audio_probe_channel - probe function of the driver module
> > + * @iface: pointer to interface instance
> > + * @channel_id: channel index/ID
> > + * @cfg: pointer to actual channel configuration
> > + * @arg_list: string that provides the name of the device to be created in
> > /dev
> > + * plus the desired audio resolution
> > + *
> > + * Creates sound card, pcm device, sets pcm ops and registers sound card.
> > + *
> > + * Returns 0 on success or error code otherwise.
> > + */
> > +static int audio_probe_channel(struct most_interface *iface, int
> > channel_id,
> > +struct most_channel_config *cfg,
> > +char *device_name, char *arg_list)
> > +{
> > + struct channel *channel;
> > + struct sound_adapter *adpt;
> > + struct snd_pcm *pcm;
> > + int playback_count = 0;
> > + int capture_count = 0;
> > + int ret;
> > + int direction;
> > + u16 ch_num;
> > + char *sample_res;
> > + char arg_list_cpy[STRING_SIZE];
> > +
> > +
Re: [PATCH 1/8] phy: phy-hi3670-usb3: move driver from staging into phy
On 17-11-20, 07:55, Mauro Carvalho Chehab wrote: > Em Mon, 16 Nov 2020 09:31:06 -0600 > Rob Herring escreveu: > > > On Mon, Nov 16, 2020 at 01:59:27PM +0100, Mauro Carvalho Chehab wrote: > > > The phy USB3 driver for Hisilicon 970 (hi3670) is ready > > > for mainstream. Mode it from staging into the main driver's > > > > s/Mode/Move/ > > > > > phy/ directory. > > > > > > Signed-off-by: Mauro Carvalho Chehab > > > --- > > > .../bindings/phy/phy-hi3670-usb3.yaml | 72 ++ > > > MAINTAINERS | 9 +- > > > drivers/phy/hisilicon/Kconfig | 10 + > > > drivers/phy/hisilicon/Makefile| 1 + > > > drivers/phy/hisilicon/phy-hi3670-usb3.c | 671 ++ > > > drivers/staging/hikey9xx/Kconfig | 11 - > > > drivers/staging/hikey9xx/Makefile | 2 - > > > drivers/staging/hikey9xx/phy-hi3670-usb3.c| 671 -- > > > drivers/staging/hikey9xx/phy-hi3670-usb3.yaml | 72 -- > > > > I assume this is only a move? Use '-M' option. > > This is a move, although I explicitly disabled -M on this series, as both > the driver code and DT may still require some review, as those patches > are for subsystems that I haven't made any relevant contributions > so far. I for one am appreciating the intent here, it helps to review the patch coming in from staging Thanks -- ~Vinod ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] drivers: most: add ALSA sound driver
On Tue, Nov 17, 2020 at 08:08:50AM +, christian.gr...@microchip.com wrote:
> On Tue, 2020-11-10 at 11:48 +0300, Dan Carpenter wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > On Fri, Nov 06, 2020 at 05:30:54PM +0100, Christian Gromm wrote:
> > > +static struct list_head adpt_list;
> > > +
> > > +#define MOST_PCM_INFO (SNDRV_PCM_INFO_MMAP | \
> > > +SNDRV_PCM_INFO_MMAP_VALID | \
> > > +SNDRV_PCM_INFO_BATCH | \
> > > +SNDRV_PCM_INFO_INTERLEAVED | \
> > > +SNDRV_PCM_INFO_BLOCK_TRANSFER)
> > > +
> > > +static void swap_copy16(u16 *dest, const u16 *source, unsigned int bytes)
> > > +{
> > > + unsigned int i = 0;
> > > +
> > > + while (i < (bytes / 2)) {
> > > + dest[i] = swab16(source[i]);
> > > + i++;
> > > + }
> > > +}
> > > +
> > > +static void swap_copy24(u8 *dest, const u8 *source, unsigned int bytes)
> > > +{
> > > + unsigned int i = 0;
> > > +
> > > + while (i < bytes - 2) {
> >
> > Can bytes ever be zero? If so then this will corrupt memory and crash.
> >
> > Generally "int i;" is less risky than "unsigned int i;". Of course, I
> > recently almost introduced a situation where we were copying up to
> > ULONG_MAX bytes so there are times when iterators should be size_t so
> > that does happen. It could be buggy either way is what I'm saying but
> > generally "unsigned int i;" is more often buggy.
> >
> > > + dest[i] = source[i + 2];
> > > + dest[i + 1] = source[i + 1];
> > > + dest[i + 2] = source[i];
> > > + i += 3;
> > > + }
> > > +}
> > > +
> > > +static void swap_copy32(u32 *dest, const u32 *source, unsigned int bytes)
> > > +{
> > > + unsigned int i = 0;
> > > +
> > > + while (i < bytes / 4) {
> > > + dest[i] = swab32(source[i]);
> > > + i++;
> > > + }
> > > +}
> > > +
> > > +static void alsa_to_most_memcpy(void *alsa, void *most, unsigned int
> > > bytes)
> > > +{
> > > + memcpy(most, alsa, bytes);
> > > +}
> > > +
> > > +static void alsa_to_most_copy16(void *alsa, void *most, unsigned int
> > > bytes)
> > > +{
> > > + swap_copy16(most, alsa, bytes);
> > > +}
> > > +
> > > +static void alsa_to_most_copy24(void *alsa, void *most, unsigned int
> > > bytes)
> > > +{
> > > + swap_copy24(most, alsa, bytes);
> > > +}
> > > +
> > > +static void alsa_to_most_copy32(void *alsa, void *most, unsigned int
> > > bytes)
> > > +{
> > > + swap_copy32(most, alsa, bytes);
> > > +}
> > > +
> > > +static void most_to_alsa_memcpy(void *alsa, void *most, unsigned int
> > > bytes)
> > > +{
> > > + memcpy(alsa, most, bytes);
> > > +}
> > > +
> > > +static void most_to_alsa_copy16(void *alsa, void *most, unsigned int
> > > bytes)
> > > +{
> > > + swap_copy16(alsa, most, bytes);
> > > +}
> > > +
> > > +static void most_to_alsa_copy24(void *alsa, void *most, unsigned int
> > > bytes)
> > > +{
> > > + swap_copy24(alsa, most, bytes);
> > > +}
> > > +
> > > +static void most_to_alsa_copy32(void *alsa, void *most, unsigned int
> > > bytes)
> > > +{
> > > + swap_copy32(alsa, most, bytes);
> > > +}
> > > +
> > > +/**
> > > + * get_channel - get pointer to channel
> > > + * @iface: interface structure
> > > + * @channel_id: channel ID
> > > + *
> > > + * This traverses the channel list and returns the channel matching the
> > > + * ID and interface.
> > > + *
> > > + * Returns pointer to channel on success or NULL otherwise.
> > > + */
> > > +static struct channel *get_channel(struct most_interface *iface,
> > > +int channel_id)
> > > +{
> > > + struct sound_adapter *adpt = iface->priv;
> > > + struct channel *channel, *tmp;
> > > +
> > > + list_for_each_entry_safe(channel, tmp, >dev_list, list) {
> > > + if ((channel->iface == iface) && (channel->id ==
> > > channel_id))
> > > + return channel;
> >
> > No need to use the _safe() version of this loop macro. You're not
> > freeing anything. My concern is that sometimes people think the _safe()
> > has something to do with locking and it does not.
> >
> > > + }
> > > + return NULL;
> > > +}
> > > +
> >
> > [ Snip ]
> >
> > > +/**
> > > + * audio_probe_channel - probe function of the driver module
> > > + * @iface: pointer to interface instance
> > > + * @channel_id: channel index/ID
> > > + * @cfg: pointer to actual channel configuration
> > > + * @arg_list: string that provides the name of the device to be created
> > > in /dev
> > > + * plus the desired audio resolution
> > > + *
> > > + * Creates sound card, pcm device, sets pcm ops and registers sound card.
> > > + *
> > > + * Returns 0 on success or error code otherwise.
> > > + */
> > > +static int audio_probe_channel(struct most_interface *iface, int
> > > channel_id,
> > > +
Re: [PATCH 4/8] regulator: hi6421v600-regulator: move it from staging
Em Mon, 16 Nov 2020 18:38:33 +
Mark Brown escreveu:
> On Mon, Nov 16, 2020 at 01:59:30PM +0100, Mauro Carvalho Chehab wrote:
>
> > This driver is ready for mainstream. Move it out of staging.
>
> There's quite a few issues here, to be honest I'm disappointed some of
> them weren't caught during staging review, this needs fairly substantial
> work and there's signs that this also applies to at least the MFD
> portion.
>
> This also appears to be missing a DT binding document, binding
> documentation is required for anything with DT support.
The DT binding is documented on patch 3/8, together with MFD.
As there's just one compatible for MFD and regulators altogether,
I opted to have just one DT binding document for both.
>
> > +config REGULATOR_HI6421V600
> > + tristate "HiSilicon Hi6421v600 PMIC voltage regulator support"
> > + depends on MFD_HI6421_SPMI && OF
> > + depends on REGULATOR
>
> This is inside an if REGULATOR block, as with all the other regulator
> drivers it doesn't need a dependency on REGULATOR.
I'll drop it. This was needed while this was inside staging.
>
> > --- /dev/null
> > +++ b/drivers/regulator/hi6421v600-regulator.c
> > @@ -0,0 +1,478 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Device driver for regulators in Hisi IC
>
> Please make the entire comment a C++ one so things look more
> intentional.
Ok.
>
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> Are we sure about *all* these includes?
I'll double check and do some cleanups.
>
> > +#define rdev_dbg(rdev, fmt, arg...)\
> > +pr_debug("%s: %s: " fmt, (rdev)->desc->name, __func__, ##arg)
>
> If it is useful to copy this then put it in a header rather than
> cut'n'pasting it. I'm not sure I see a pressing need for most of the
> trace here, it looks to be duplicating a lot of things the core does for
> you.
I'll cleanup the debug messages, and place this on a header.
Due to SPMI bus, the standard debug macros don't work fine on
this file.
>
> > +static DEFINE_MUTEX(enable_mutex);
>
> Drivers shouldn't be declaring global variables, if this is required it
> should be allocated in driver data.
I'll try to see a better place for this, but in this case, the
mutex should be global anyway, as the access to the SPMI bus
should be serialized.
I suspect that a change like that will likely require touching
the SPMI bus core, but I can't foresee the side effects to the
Qualcomm SPMI drivers that would likely have their own ways
to serialize access to the bus.
>
> > +/*
> > + * helper function to ensure when it returns it is at least 'delay_us'
> > + * microseconds after 'since'.
> > + */
> > +
> > +static int hi6421_spmi_regulator_is_enabled(struct regulator_dev *rdev)
>
> The helper function appears to have been removed?
I'll drop the comment.
>
> > +{
> > + struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev);
> > + struct hi6421_spmi_pmic *pmic = sreg->pmic;
> > + u32 reg_val;
> > +
> > + reg_val = hi6421_spmi_pmic_read(pmic, rdev->desc->enable_reg);
> > +
> > + rdev_dbg(rdev,
> > +"enable_reg=0x%x, val= 0x%x, enable_state=%d\n",
> > +rdev->desc->enable_reg,
> > +reg_val, (reg_val & rdev->desc->enable_mask));
> > +
> > + return ((reg_val & rdev->desc->enable_mask) != 0);
> > +}
>
> It looks like it would be less code overall to just implement a regmap
> for the MFD, even if it were only used in this driver - almost
> everything in here is just a carbon copy of standard helpers that the
> core provides for regmap devices. Doing it in the MFD would be more
> idiomatic for everything though.
I tried to use regmap for this driver while porting it from Linaro's
OOT to upstream, but it turns that adding support for it is not trivial.
I suspect that, just like I2C has their own set of regmap functions
(regmap_init_i2c, devm_regmap_init_i2c), if we want to properly support
regmap, we need first to add a SPMI variant to it, as the locking
should be bus-aware.
>
> > +static int hi6421_spmi_regulator_enable(struct regulator_dev *rdev)
> > +{
> > + struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev);
> > + struct hi6421_spmi_pmic *pmic = sreg->pmic;
> > +
> > + /* cannot enable more than one regulator at one time */
> > + mutex_lock(_mutex);
> > + usleep_range(HISI_REGS_ENA_PROTECT_TIME,
> > +HISI_REGS_ENA_PROTECT_TIME + 1000);
> > +
> > + /* set enable register */
> > + rdev_dbg(rdev,
> > +"off_on_delay=%d us, enable_reg=0x%x, enable_mask=0x%x\n",
> > +rdev->desc->off_on_delay, rdev->desc->enable_reg,
> > +rdev->desc->enable_mask);
> > +
> > + hi6421_spmi_pmic_rmw(pmic,
[PATCH] media: atomisp: remove redundant NULL check of "params"
The check result of (!A || (A && B)) is equivalent to (!A || B),
so remove redundant NULL check of "params"
Signed-off-by: Ding Xiang
---
drivers/staging/media/atomisp/pci/sh_css_params.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/sh_css_params.c
b/drivers/staging/media/atomisp/pci/sh_css_params.c
index 24fc497bd491..9eb02f463eba 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_params.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_params.c
@@ -4649,10 +4649,8 @@ ia_css_dvs2_6axis_config_allocate(const struct
ia_css_stream *stream)
params = stream->isp_params_configs;
/* Backward compatibility by default consider pipe as Video*/
- if (!params || (params &&
- !params->pipe_dvs_6axis_config[IA_CSS_PIPE_ID_VIDEO])) {
+ if (!params || !params->pipe_dvs_6axis_config[IA_CSS_PIPE_ID_VIDEO])
goto err;
- }
dvs_config = kvcalloc(1, sizeof(struct ia_css_dvs_6axis_config),
GFP_KERNEL);
--
2.28.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/8] regulator: hi6421v600-regulator: move it from staging
Hi Mark,
Em Mon, 16 Nov 2020 18:38:33 +
Mark Brown escreveu:
> > This driver is ready for mainstream. Move it out of staging.
>
> There's quite a few issues here, to be honest I'm disappointed some of
> them weren't caught during staging review, this needs fairly substantial
> work and there's signs that this also applies to at least the MFD
> portion.
>
...
> > +static int hi6421_spmi_regulator_probe_ldo(struct platform_device *pdev,
> > + struct device_node *np,
> > + struct hi6421_spmi_pmic *pmic)
> > +{
>
> This probe code looks very different to other regulator drivers, this
> alone should have been a warning that the driver needs some substantial
> refactoring here. As indicated information about what regulators are
> present on devices and their properties should be in the driver not the
> DT, the driver should just be able to register them unconditionally and
> use of_match and regulators_node to allow the core to find any
> constraints that are provided by the platform.
Let me reply to this before handling the other issues you pointed, as
this one is related to some design decisions I had to make for this driver
to properly work upstream.
FYI, all documentation I have about this board is at:
https://www.96boards.org/documentation/consumer/hikey/hikey970/
-
The setup for MFD/regulator is different than almost all other
regulator drivers currently upstreamed[1].
The PM part of Hikey970 is based on a master/slave SPMI bus. Each
bus can have up to 16 PM chips connected into it.
It means that, for the regulator driver to work, two drivers
should be probed first: the SPMI bus controller driver
(hisi-spmi-controller.c) and the SPMI bus client driver, which is
at the MFD driver(hi6421-spmi-pmic.c).
Only after having both probed, the regulator driver can be
probed.
Also, as all the communication between the PM chip
and the SoC happens via a single serial bus, there's no
sense on probing the regulators in parallel.
That's mainly the reason why I opted to serialize the probe
inside hi6421v600-regulator.c.
The relevant changeset that ensures that everything is
probed the right way is this one:
75937f8f961e ("staging: regulator: hi6421v600-regulator: change the
binding logic")
Without such change, the driver doesn't work upstream, as the
regulator driver ends by being probed before the bus client
driver (MFD).
There's a second reason, though: when letting regulator probe to
happen in parallel, the LDOs got probed on a random order, which
makes more difficult to debug the driver, as the LDO numbering
may not be following the LDO name, making harder to debug the
drivers that depend on regulator support.
Thanks,
Mauro
[1] The only other drivers for SPMI bus are from some Qualcomm
based boards - those seem to be using a different setup.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
