| 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
| ||
|
Message-ID: <aAEjZGjNi_m9mfxP@smile.fi.intel.com>
Date: Thu, 17 Apr 2025 18:51:00 +0300
From: Andy Shevchenko <andy@...nel.org>
To: Ivan Vecera <ivecera@...hat.com>
Cc: netdev@...r.kernel.org, Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
Jiri Pirko <jiri@...nulli.us>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Prathosh Satish <Prathosh.Satish@...rochip.com>,
Lee Jones <lee@...nel.org>, Kees Cook <kees@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Michal Schmidt <mschmidt@...hat.com>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v3 net-next 3/8] mfd: Add Microchip ZL3073x support
On Wed, Apr 16, 2025 at 06:21:39PM +0200, Ivan Vecera wrote:
> Add base MFD driver for Microchip Azurite ZL3073x chip family.
> These chips provide DPLL and PHC (PTP) functionality and they can
> be connected over I2C or SPI bus.
>
> The MFD driver provide basic communication and synchronization
> over the bus and common functionality that are used by the DPLL
> driver (in the part 2) and by the PTP driver (will be added later).
>
> The chip family is characterized by following properties:
> * up to 5 separate DPLL units (channels)
> * 5 synthesizers
> * 10 input pins (references)
> * 10 outputs
> * 20 output pins (output pin pair shares one output)
> * Each reference and output can act in differential or single-ended
> mode (reference or output in differential mode consumes 2 pins)
> * Each output is connected to one of the synthesizers
> * Each synthesizer is driven by one of the DPLL unit
>
> The device uses 7-bit addresses and 8-bits for values. It exposes 8-, 16-,
> 32- and 48-bits registers in address range <0x000,0x77F>. Due to 7bit
> addressing the range is organized into pages of size 128 and each page
> contains page selector register (0x7F). To read/write multi-byte registers
> the device supports bulk transfers.
>
> There are 2 kinds of registers, simple ones that are present at register
> pages 0..9 and mailbox ones that are present at register pages 10..14.
>
> To access mailbox type registers a caller has to take mailbox_mutex that
> ensures the reading and committing mailbox content is done atomically.
> More information about it in later patch from the series.
...
> +#define ZL_NUM_PAGES 15
> +#define ZL_NUM_SIMPLE_PAGES 10
> +#define ZL_PAGE_SEL 0x7F
> +#define ZL_PAGE_SEL_MASK GENMASK(3, 0)
> +#define ZL_NUM_REGS (ZL_NUM_PAGES * ZL_PAGE_SIZE)
> +
> +/* Regmap range configuration */
> +static const struct regmap_range_cfg zl3073x_regmap_range = {
> + .range_min = ZL_RANGE_OFF,
> + .range_max = ZL_RANGE_OFF + ZL_NUM_REGS - 1,
> + .selector_reg = ZL_PAGE_SEL,
> + .selector_mask = ZL_PAGE_SEL_MASK,
> + .selector_shift = 0,
> + .window_start = 0,
> + .window_len = ZL_PAGE_SIZE,
> +};
On the first glance this looks good now, thanks for addressing that.
...
> +static bool
> +zl3073x_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> + /* Only page selector is non-volatile */
> + return (reg != ZL_PAGE_SEL);
Unneeded parentheses.
> +}
...
> +/**
> + * zl3073x_devm_alloc - allocates zl3073x device structure
> + * @dev: pointer to device structure
> + *
> + * Allocates zl3073x device structure as device resource and initializes
> + * regmap_mutex.
> + *
> + * Return: pointer to zl3073x device on success, error pointer on error
> + */
> +struct zl3073x_dev *zl3073x_devm_alloc(struct device *dev)
> +{
> + struct zl3073x_dev *zldev;
> + int rc;
> +
> + zldev = devm_kzalloc(dev, sizeof(*zldev), GFP_KERNEL);
> + if (!zldev)
> + return ERR_PTR(-ENOMEM);
> +
> + zldev->dev = dev;
> +
> + /* We have to initialize regmap mutex here because during
> + * zl3073x_dev_probe() is too late as the regmaps are already
> + * initialized.
> + */
> + rc = devm_mutex_init(zldev->dev, &zldev->mailbox_lock);
> + if (rc) {
> + dev_err_probe(zldev->dev, rc, "Failed to initialize mutex\n");
> + return ERR_PTR(rc);
return dev_err_probe(...);
> + }
> +
> + return zldev;
> +}
...
> +int zl3073x_dev_probe(struct zl3073x_dev *zldev,
> + const struct zl3073x_chip_info *chip_info)
> +{
> + u16 id, revision, fw_ver;
> + u32 cfg_ver;
> + int i, rc;
> +
> + /* Read chip ID */
> + rc = zl3073x_read_id(zldev, &id);
> + if (rc)
> + return rc;
> +
> + /* Check it matches */
> + for (i = 0; i < chip_info->num_ids; i++) {
> + if (id == chip_info->ids[i])
> + break;
> + }
> +
> + if (i == chip_info->num_ids) {
> + dev_err(zldev->dev, "Unknown or non-match chip ID: 0x%0x\n", id);
> + return -ENODEV;
return dev_err_probe(...);
> + }
> +
> + /* Read revision, firmware version and custom config version */
> + rc = zl3073x_read_revision(zldev, &revision);
> + if (rc)
> + return rc;
> + rc = zl3073x_read_fw_ver(zldev, &fw_ver);
> + if (rc)
> + return rc;
> + rc = zl3073x_read_custom_config_ver(zldev, &cfg_ver);
> + if (rc)
> + return rc;
> +
> + dev_dbg(zldev->dev, "ChipID(%X), ChipRev(%X), FwVer(%u)\n", id,
> + revision, fw_ver);
> + dev_dbg(zldev->dev, "Custom config version: %lu.%lu.%lu.%lu\n",
> + FIELD_GET(GENMASK(31, 24), cfg_ver),
> + FIELD_GET(GENMASK(23, 16), cfg_ver),
> + FIELD_GET(GENMASK(15, 8), cfg_ver),
> + FIELD_GET(GENMASK(7, 0), cfg_ver));
> +
> + return 0;
> +}
...
> +#include <linux/device.h>
Is it used?
> +#include <linux/dev_printk.h>
+ err.h
> +#include <linux/i2c.h>
> +#include <linux/mfd/zl3073x.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
...
> +static int zl3073x_i2c_probe(struct i2c_client *client)
> +{
> + struct regmap_config regmap_cfg;
> + struct device *dev = &client->dev;
> + struct zl3073x_dev *zldev;
> +
> + zldev = zl3073x_devm_alloc(dev);
> + if (IS_ERR(zldev))
> + return PTR_ERR(zldev);
> + i2c_set_clientdata(client, zldev);
Is it used anywhere?
> + zl3073x_dev_init_regmap_config(®map_cfg);
> +
> + zldev->regmap = devm_regmap_init_i2c(client, ®map_cfg);
> + if (IS_ERR(zldev->regmap)) {
> + dev_err_probe(dev, PTR_ERR(zldev->regmap),
> + "Failed to initialize regmap\n");
return dev_err_probe(...);
> + return PTR_ERR(zldev->regmap);
> + }
> +
> + return zl3073x_dev_probe(zldev, i2c_get_match_data(client));
> +}
...
> +++ b/drivers/mfd/zl3073x-spi.c
Same comments as per i2c part.
...
> +static inline void zl3073x_mailbox_lock(struct zl3073x_dev *zldev)
> +{
> + mutex_lock(&zldev->mailbox_lock);
Do you need sparse annotations? (build with `make C=1 ...` to check)
> +}
> +static inline void zl3073x_mailbox_unlock(struct zl3073x_dev *zldev)
> +{
> + mutex_unlock(&zldev->mailbox_lock);
> +}
> +
> +DEFINE_GUARD(zl3073x_mailbox, struct zl3073x_dev *, zl3073x_mailbox_lock(_T),
> + zl3073x_mailbox_unlock(_T));
Seems to be they are useless as you share the lock. One may use
guard(nutex)(...) directly.
> +#endif /* __LINUX_MFD_ZL3073X_H */
...
> +#include <asm/byteorder.h>
asm/* usually goes after linux/* as they are more specific and linux/* are more
generic.
> +#include <linux/lockdep.h>
> +#include <linux/mfd/zl3073x.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
...
> +static inline __maybe_unused int
Why __maybe_unused? Please, get rid of those.
> +zl3073x_read_id(struct zl3073x_dev *zldev, u16 *value)
> +{
> + __be16 temp;
> + int rc;
> +
> + rc = regmap_bulk_read(zldev->regmap, ZL_REG_ID, &temp, sizeof(temp));
> + if (rc)
> + return rc;
> +
> + *value = be16_to_cpu(temp);
> + return rc;
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists