[07/12] mfd: syscon: Consider platform data a regmap config name

Message ID 1392138636-29240-8-git-send-email-pawel.moll@arm.com
State New
Headers show

Commit Message

Pawel Moll Feb. 11, 2014, 5:10 p.m.
Use the device platform data as a regmap config
name. This is particularly useful in the regmap
debugfs when there is more than one syscon device
registered, to distinguish the register blocks.

Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
Alternatively I could define a syscon platform data structure,
something like this:

struct syscon_platform_data {
	const char *label;
};

 drivers/mfd/syscon.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Lee Jones Feb. 11, 2014, 5:24 p.m. | #1
> Use the device platform data as a regmap config
> name. This is particularly useful in the regmap
> debugfs when there is more than one syscon device
> registered, to distinguish the register blocks.
> 
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
> Alternatively I could define a syscon platform data structure,
> something like this:
> 
> struct syscon_platform_data {
> 	const char *label;
> };
> 
>  drivers/mfd/syscon.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index 71841f9..ea1770b 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -143,6 +143,7 @@ static int syscon_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	syscon_regmap_config.max_register = res->end - res->start - 3;
> +	syscon_regmap_config.name = dev_get_platdata(&pdev->dev);

Perhaps the answer is waiting for me in the other set, but ...

Isn't this going to be NULL most of the time?

>  	syscon->regmap = devm_regmap_init_mmio(dev, base,
>  					&syscon_regmap_config);
>  	if (IS_ERR(syscon->regmap)) {
Lee Jones Feb. 12, 2014, 8:26 a.m. | #2
> > Use the device platform data as a regmap config
> > name. This is particularly useful in the regmap
> > debugfs when there is more than one syscon device
> > registered, to distinguish the register blocks.
> > 
> > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > Cc: Lee Jones <lee.jones@linaro.org>
> > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> > ---
> ...
> > syscon_regmap_config.max_register = res->end - res->start - 3;
> > +	syscon_regmap_config.name = dev_get_platdata(&pdev->dev);
> 
> Is dev_name(&pdev->dev) can be used for such purpose?

Yes of course.

Either use the automatically generated name or over-ride with
dev->init_name prior to registration or call dev_set_name()
manually. Then retrieve with Alexander's suggestion.

Is there any technical reason why this is not possible with your
implementation?
Pawel Moll Feb. 12, 2014, 11:06 a.m. | #3
On Wed, 2014-02-12 at 08:26 +0000, Lee Jones wrote:
> > > Use the device platform data as a regmap config
> > > name. This is particularly useful in the regmap
> > > debugfs when there is more than one syscon device
> > > registered, to distinguish the register blocks.
> > > 
> > > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > > Cc: Lee Jones <lee.jones@linaro.org>
> > > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> > > ---
> > ...
> > > syscon_regmap_config.max_register = res->end - res->start - 3;
> > > +	syscon_regmap_config.name = dev_get_platdata(&pdev->dev);
> > 
> > Is dev_name(&pdev->dev) can be used for such purpose?
> 
> Yes of course.
> 
> Either use the automatically generated name or over-ride with
> dev->init_name prior to registration or call dev_set_name()
> manually. Then retrieve with Alexander's suggestion.
> 
> Is there any technical reason why this is not possible with your
> implementation?

Hold on, guys. Let me just point out that we're talking "non-DT"
platform devices here (either statically defined struct
platform_device-s or - my case - the MFD cells).

In this case device/driver matching relies completely on device name.
Either the pdev->name must be identical (strcmp) to pdrv->name, or the
pdev->name must be identical (strcmp again) to one of the pdev->id_table
entries. See platform_match() in driver/base/platform.c for more
details.

Therefore the dev_name(&pdev->dev) on a non-DT-originating sysconf
devince will always return "sysconf.*", unless you're ready to maintain
a growing syscon_ids[] list. If so, I will have to add three entries
there ("sys_id", "sys_misc" and "sys_procid"). I hope you are not
seriously considering this idea :-) After all that's what the
platform_data was invented for.

Pawel


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Lee Jones Feb. 12, 2014, 11:18 a.m. | #4
> > > > Use the device platform data as a regmap config
> > > > name. This is particularly useful in the regmap
> > > > debugfs when there is more than one syscon device
> > > > registered, to distinguish the register blocks.
> > > > 
> > > > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > > > Cc: Lee Jones <lee.jones@linaro.org>
> > > > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> > > > ---
> > > ...
> > > > syscon_regmap_config.max_register = res->end - res->start - 3;
> > > > +	syscon_regmap_config.name = dev_get_platdata(&pdev->dev);
> > > 
> > > Is dev_name(&pdev->dev) can be used for such purpose?
> > 
> > Yes of course.
> > 
> > Either use the automatically generated name or over-ride with
> > dev->init_name prior to registration or call dev_set_name()
> > manually. Then retrieve with Alexander's suggestion.
> > 
> > Is there any technical reason why this is not possible with your
> > implementation?
> 
> Hold on, guys. Let me just point out that we're talking "non-DT"
> platform devices here (either statically defined struct
> platform_device-s or - my case - the MFD cells).
> 
> In this case device/driver matching relies completely on device name.
> Either the pdev->name must be identical (strcmp) to pdrv->name, or the
> pdev->name must be identical (strcmp again) to one of the pdev->id_table
> entries. See platform_match() in driver/base/platform.c for more
> details.
> 
> Therefore the dev_name(&pdev->dev) on a non-DT-originating sysconf
> devince will always return "sysconf.*", unless you're ready to maintain
> a growing syscon_ids[] list. If so, I will have to add three entries
> there ("sys_id", "sys_misc" and "sys_procid"). I hope you are not
> seriously considering this idea :-) After all that's what the
> platform_data was invented for.

Ah, I see your predicament. I guess that is a limitation of the was
syscon works. Usually we'd use match tables to differentiate between
various supported devices, but I guess the 'supported devices' list
for syscon is pretty limitless. In which case I support your second
implementation (adding a platform_data container) for the use of
arbitrary/useful-common names.
Pawel Moll Feb. 12, 2014, 11:43 a.m. | #5
On Wed, 2014-02-12 at 11:27 +0000, Alexander Shiyan wrote:
> Yeah, I gave up the idea to use the syscon_ids[] to separate devices for
> non-DT case.
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/mfd/syscon.c?id=5104d2656d4874c51868dc7182016e9501ec99ca
> Instead, I use a hard definition for pdev->id, so that the names of
> syscon-devices are different and can be obtained from the driver
> it uses with syscon_regmap_lookup_by_pdevname().

It is a sort-of-solution (I personally dislike magic numbers, but this
is a separate discussion) for a completely-non-DT case, where you can
guarantee limited number of syscon devices.

In my case I have a DT system, where some of the MFD devices *may*
register syscon cells... No way to enforce ordering.

> I understand this topic correct?

The main idea here is to attach a meaningful label to the syscon
regmaps. My example:

/ # ls -d1 /sys/kernel/debug/regmap/syscon.*
/sys/kernel/debug/regmap/syscon.0.auto
/sys/kernel/debug/regmap/syscon.4.auto
/sys/kernel/debug/regmap/syscon.5.auto

vs

/ # ls -d /sys/kernel/debug/regmap/syscon.*
/sys/kernel/debug/regmap/syscon.0.auto-sys_id
/sys/kernel/debug/regmap/syscon.4.auto-sys_misc
/sys/kernel/debug/regmap/syscon.5.auto-sys_procid

Of course one could also define syscon_regmap_lookup_by_label() (I don't
really need it right now so didn't go that way)

Pawel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 71841f9..ea1770b 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -143,6 +143,7 @@  static int syscon_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	syscon_regmap_config.max_register = res->end - res->start - 3;
+	syscon_regmap_config.name = dev_get_platdata(&pdev->dev);
 	syscon->regmap = devm_regmap_init_mmio(dev, base,
 					&syscon_regmap_config);
 	if (IS_ERR(syscon->regmap)) {