Re: [PATCH v2 2/3] nvmem: syscon: Add syscon backed nvmem driver

[Marek Vasut <marex@denx.de>]

On 5/18/23 16:22, Krzysztof Kozlowski wrote:

[...]

>> +++ b/drivers/nvmem/nvmem-syscon.c
>> @@ -0,0 +1,105 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2022 Marek Vasut <marex@denx.de>
>> + *
>> + * Based on snvs_lpgpr.c .
>> + */
>> +
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/nvmem-provider.h>
>> +#include <linux/of_device.h>
>> +#include <linux/regmap.h>
>> +
>> +struct nvmem_syscon_priv {
>> +	struct device_d		*dev;
>> +	struct regmap		*regmap;
>> +	struct nvmem_config	cfg;
>> +	unsigned int		off;
>> +};
>> +
>> +static int nvmem_syscon_write(void *context, unsigned int offset, void *val,
>> +			      size_t bytes)
>> +{
>> +	struct nvmem_syscon_priv *priv = context;
>> +
>> +	return regmap_bulk_write(priv->regmap, priv->off + offset,
>> +				 val, bytes / 4);
>> +}
>> +
>> +static int nvmem_syscon_read(void *context, unsigned int offset, void *val,
>> +			     size_t bytes)
>> +{
>> +	struct nvmem_syscon_priv *priv = context;
>> +
>> +	return regmap_bulk_read(priv->regmap, priv->off + offset,
>> +				val, bytes / 4);
>> +}
>> +
>> +static int nvmem_syscon_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *node = dev->of_node;
>> +	struct device_node *syscon_node;
>> +	struct nvmem_syscon_priv *priv;
>> +	struct nvmem_device *nvmem;
>> +	struct nvmem_config *cfg;
>> +	int ret;
>> +
>> +	if (!node)
>> +		return -ENOENT;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	ret = of_property_read_u32_index(node, "reg", 0, &priv->off);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = of_property_read_u32_index(node, "reg", 1, &priv->cfg.size);
>> +	if (ret)
>> +		return ret;
>> +
>> +	syscon_node = of_get_parent(node);
> 
> This does not look correct. You hard-code dependency that it must be a
> child of syscon node. This is weird requirement and not explained in the
> bindings.
> 
> Why this cannot be then generic MMIO node? Why it has to be a child of
> syscon?

Because I already have a syscon node and I want to expose only a subset 
of it to userspace (bootcounter in my case) . See 1/3, I replied there, 
let's continue the discussion there.

I fixed the rest in prep for V3, sorry for the horrid delays in replies.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel