Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
> unused-but-set-variable by default. How did you enable this warning without > being flooded by the warnings? (I tried W=1) I use W=1. To prevent flooding, I used to replace all -I with -isystem in the Kernel Makefile, but sadly this doesn't work anymore. And I had no time to investigate why. signature.asc Description: Digital signature
Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
On 08/15/2015 01:23 PM, Wolfram Sang wrote:
> On Tue, Jun 16, 2015 at 10:28:12AM -0700, York Sun wrote:
>> Based on i2c-mux-gpio driver, similarly the register based mux
>> switch from one bus to another by setting a single register.
>> The register can be on PCIe bus, local bus, or any memory-mapped
>> address.
>>
>> Signed-off-by: York Sun
>> CC: Wolfram Sang
>> CC: Peter Korsgaard
>
> Mostly good.
>
>> +static int i2c_mux_reg_probe(struct platform_device *pdev)
>> +{
>> +struct regmux *mux;
>> +struct i2c_adapter *parent;
>> +struct resource *res;
>> +int (*deselect)(struct i2c_adapter *, void *, u32);
>> +unsigned int initial_state, class;
>
> gcc says:
>
> drivers/i2c/muxes/i2c-mux-reg.c:182:15: warning: variable 'initial_state' set
> but not used [-Wunused-but-set-variable]
>
> It seens you prepared for setting the initial state but don't do the
> actual set?
Thanks for catching this. I set the initial state variable but used another
variable when it is actually used. Kernel Makefile disables
unused-but-set-variable by default. How did you enable this warning without
being flooded by the warnings? (I tried W=1)
>
>> +static struct platform_driver i2c_mux_reg_driver = {
>> +.probe = i2c_mux_reg_probe,
>> +.remove = i2c_mux_reg_remove,
>> +.driver = {
>> +.owner = THIS_MODULE,
>
> coccicheck says:
>
> drivers/i2c/muxes/i2c-mux-reg.c:288:3-8: No need to set .owner here. The core
> will do it.
Will drop it in next version.
York
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
On Tue, Jun 16, 2015 at 10:28:12AM -0700, York Sun wrote:
> Based on i2c-mux-gpio driver, similarly the register based mux
> switch from one bus to another by setting a single register.
> The register can be on PCIe bus, local bus, or any memory-mapped
> address.
>
> Signed-off-by: York Sun
> CC: Wolfram Sang
> CC: Peter Korsgaard
Mostly good.
> +static int i2c_mux_reg_probe(struct platform_device *pdev)
> +{
> + struct regmux *mux;
> + struct i2c_adapter *parent;
> + struct resource *res;
> + int (*deselect)(struct i2c_adapter *, void *, u32);
> + unsigned int initial_state, class;
gcc says:
drivers/i2c/muxes/i2c-mux-reg.c:182:15: warning: variable 'initial_state' set
but not used [-Wunused-but-set-variable]
It seens you prepared for setting the initial state but don't do the
actual set?
> +static struct platform_driver i2c_mux_reg_driver = {
> + .probe = i2c_mux_reg_probe,
> + .remove = i2c_mux_reg_remove,
> + .driver = {
> + .owner = THIS_MODULE,
coccicheck says:
drivers/i2c/muxes/i2c-mux-reg.c:288:3-8: No need to set .owner here. The core
will do it.
> + .name = "i2c-mux-reg",
> + },
> +};
Thanks,
Wolfram
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
On Thu, 2015-06-18 at 11:42 +0200, Alexander Sverdlin wrote: > Most of the usual drivers in the Kernel have this line. Perhaps people copy and paste without really realizing what this macro is supposed to be for. (I didn't know until a few days ago. I needed you to confirm that I figured it out correctly.) Perhaps reviewers skip over the boilerplate stuff. > Until now no animal was hurt with it. Sure, no overhead and all that. No value too. Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
Hi! On 18/06/15 11:32, ext Paul Bolle wrote: > As I understand it, we've established that: > - this macro setups up udev, and modprobe, and friends, to listen to a > "MODALIAS=platform:i2c-mux-reg" message; and > - that there's currently no code in the kernel that will send out this > message. > > Did I get that right? Because it follows from the above that this line > serves no purpose. Worse, it makes the code actually confusing. Because Are you kidding? > it suggests the availability of functionality that in reality doesn't > exist. Most of the usual drivers in the Kernel have this line. Until now no animal was hurt with it. -- Best regards, Alexander Sverdlin. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
On Thu, 2015-06-18 at 11:04 +0200, Alexander Sverdlin wrote: > Maybe (and hopefully) there will never be a legacy user of this driver. But > this macro > is perfectly fine, adds no overhead (but modinfo) (The test for any line of code is whether it adds value. Adding no overhead is not adding value. That goes for one line macros too.) > and make the module "complete" in a > sense that it supports both types of binding. There is a legacy probe > function in it, > all the support for legacy binding with platform_data in it and this modalias > is simply > the last part of it. As I understand it, we've established that: - this macro setups up udev, and modprobe, and friends, to listen to a "MODALIAS=platform:i2c-mux-reg" message; and - that there's currently no code in the kernel that will send out this message. Did I get that right? Because it follows from the above that this line serves no purpose. Worse, it makes the code actually confusing. Because it suggests the availability of functionality that in reality doesn't exist. Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
Hi!
On 18/06/15 10:08, ext Paul Bolle wrote:
You do not see the platform_device, because there are no users yet, put
> > >> > this MODULE_ALIAS() is perfectly fine, it will allow automatic
> > >> > module loading
> > >> > in non-DT case.
>>> > > Do you mean that it will allow automatic module loading once the patch
>>> > > that adds a struct platform_device with a "i2c-mux-reg" name lands?
>> >
>> > Any platform code which will register the platform_device will trigger
>> > uevent and
>> > udevd will be able to find the module with this macro. This is a legacy
>> > alternative
>> > to device-tree approach.
> That means I've correctly figured out the purpose of this
> MODULE_ALIAS("platform:" stuff. Because it might actually be documented
> somewhere but I managed to not stumble on that documentation.
>
> With that out of the way: am I right in thinking there's currently no
> platform code that triggers that uevent for
> "MODALIAS=platform:i2c-mux-reg"? Because if there's no struct
> platform_device taking care of that I think this MODULE_ALIAS() should
> not be added, not yet.
Maybe (and hopefully) there will never be a legacy user of this driver. But
this macro
is perfectly fine, adds no overhead (but modinfo) and make the module
"complete" in a
sense that it supports both types of binding. There is a legacy probe function
in it,
all the support for legacy binding with platform_data in it and this modalias
is simply
the last part of it.
--
Best regards,
Alexander Sverdlin.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
Hi Alexander,
On Thu, 2015-06-18 at 09:40 +0200, Alexander Sverdlin wrote:
> On 17/06/15 18:03, ext Paul Bolle wrote:
> >> You do not see the platform_device, because there are no users yet, put
> >> > this MODULE_ALIAS() is perfectly fine, it will allow automatic module
> >> > loading
> >> > in non-DT case.
> > Do you mean that it will allow automatic module loading once the patch
> > that adds a struct platform_device with a "i2c-mux-reg" name lands?
>
> Any platform code which will register the platform_device will trigger uevent
> and
> udevd will be able to find the module with this macro. This is a legacy
> alternative
> to device-tree approach.
That means I've correctly figured out the purpose of this
MODULE_ALIAS("platform:" stuff. Because it might actually be documented
somewhere but I managed to not stumble on that documentation.
With that out of the way: am I right in thinking there's currently no
platform code that triggers that uevent for
"MODALIAS=platform:i2c-mux-reg"? Because if there's no struct
platform_device taking care of that I think this MODULE_ALIAS() should
not be added, not yet.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
Hello! On 17/06/15 18:43, ext York Sun wrote: >> > Yeah, this is really bad idea. You maybe want something like >> > __iomem "cookie" here instead of this bare pointer. > Let me try. > Could you think about different access widths, please? Not all buses are 32-bits wide and even on 64-bit CPUs one might have 16-bit bus and 32 bits accesses are not allowed or perform two accesses, etc... So to cover the use-cases which I see one needs to have a possibility to select between __raw_writeb()/__raw_writew()/__raw_writel()/__raw_writeq() (now that I'm thinking about it, I think these native-Endianness functions are preferred and if one has a bus with different Endianness he should think about the conversion in the reg property of subnodes). Very important is readback with corresponding __raw_read*(), but maybe you want to do this optional via additional DT property... -- Best regards, Alexander Sverdlin. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
Hi! On 17/06/15 18:03, ext Paul Bolle wrote: >> You do not see the platform_device, because there are no users yet, put >> > this MODULE_ALIAS() is perfectly fine, it will allow automatic module >> > loading >> > in non-DT case. > Do you mean that it will allow automatic module loading once the patch > that adds a struct platform_device with a "i2c-mux-reg" name lands? Any platform code which will register the platform_device will trigger uevent and udevd will be able to find the module with this macro. This is a legacy alternative to device-tree approach. -- Best regards, Alexander Sverdlin. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
On 06/17/2015 08:03 AM, Alexander Sverdlin wrote:
> Hi!
>
> On 16/06/15 19:28, ext York Sun wrote:
>> Based on i2c-mux-gpio driver, similarly the register based mux
>> switch from one bus to another by setting a single register.
>> The register can be on PCIe bus, local bus, or any memory-mapped
>> address.
>>
>> Signed-off-by: York Sun
>> CC: Wolfram Sang
>> CC: Peter Korsgaard
>> ---
>> .../devicetree/bindings/i2c/i2c-mux-reg.txt| 69 ++
>> drivers/i2c/muxes/Kconfig | 11 +
>> drivers/i2c/muxes/Makefile |1 +
>> drivers/i2c/muxes/i2c-mux-reg.c| 239
>>
>> drivers/i2c/muxes/i2c-mux-reg.h| 38
>> 5 files changed, 358 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
>> create mode 100644 drivers/i2c/muxes/i2c-mux-reg.c
>> create mode 100644 drivers/i2c/muxes/i2c-mux-reg.h
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
>> b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
>> new file mode 100644
>> index 000..ad7cc4f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
>> @@ -0,0 +1,69 @@
>> +Register-based I2C Bus Mux
>> +
>> +This binding describes an I2C bus multiplexer that uses a single regsiter
>
Nice catch.
>
>> +to route the I2C signals.
>> +
>> +Required properties:
>> +- compatible: i2c-mux-reg
>> +- i2c-parent: The phandle of the I2C bus that this multiplexer's master-side
>> + port is connected to.
>> +* Standard I2C mux properties. See mux.txt in this directory.
>> +* I2C child bus nodes. See mux.txt in this directory.
>> +
>> +Optional properties:
>> +- reg: this pair of specifies the register to control the mux.
>> + The depends on its parent node. It can be any memory-mapped
>> + address. If omitted, the resource of this device will be used.
>> +- idle-state: value to set the muxer to when idle. When no value is
>> + given, it defaults to the last value used.
>> +
>> +For each i2c child node, an I2C child bus will be created. They will
>> +be numbered based on their order in the device tree.
>> +
>> +Whenever an access is made to a device on a child bus, the value set
>> +in the revelant node's reg property will be output to the register.
>> +
>> +If an idle state is defined, using the idle-state (optional) property,
>> +whenever an access is not being made to a device on a child bus, the
>> +register will be set according to the idle value.
>> +
>> +If an idle state is not defined, the most recently used value will be
>> +left programmed into the register.
>> +
>> +Example of a mux on PCIe card, the host is a powerpc SoC (big endian):
>> +
>> +i2c-mux {
>> +/* the depends on the address translation
>> + * of the parent device. If omitted, device resource
>> + * will be used instead.
>> + */
>> +reg = <0x6028 0x4>;
>> +compatible = "i2c-mux-reg";
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +i2c-parent = <&i2c1>;
>> +i2c@0 {
>> +reg = <0>;
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +
>> +si5338: clock-generator@70 {
>> +compatible = "silabs,si5338";
>> +reg = <0x70>;
>> +/* other stuff */
>> +};
>> +};
>> +
>> +i2c@1 {
>> +/* data is in little endian on pcie bus */
>> +reg = <0x0100>;
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +
>> +si5338: clock-generator@70 {
>> +compatible = "silabs,si5338";
>> +reg = <0x70>;
>> +/* other stuff */
>> +};
>> +};
>> +};
>> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
>> index f6d313e..77c1257 100644
>> --- a/drivers/i2c/muxes/Kconfig
>> +++ b/drivers/i2c/muxes/Kconfig
>> @@ -29,6 +29,17 @@ config I2C_MUX_GPIO
>>This driver can also be built as a module. If so, the module
>>will be called i2c-mux-gpio.
>>
>> +config I2C_MUX_REG
>> +tristate "Register-based I2C multiplexer"
>> +help
>> + If you say yes to this option, support will be included for a
>> + register based I2C multiplexer. This driver provides access to
>> + I2C busses connected through a MUX, which is controlled
>> + by a sinple register.
>> +
>> + This driver can also be built as a module. If so, the module
>> + will be called i2c-mux-reg.
>> +
>> config I2C_MUX_PCA9541
>> tristate "NXP PCA9541 I2C Master Sel
Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
On Wed, 2015-06-17 at 17:00 +0200, Alexander Sverdlin wrote: > You do not see the platform_device, because there are no users yet, put > this MODULE_ALIAS() is perfectly fine, it will allow automatic module loading > in non-DT case. Do you mean that it will allow automatic module loading once the patch that adds a struct platform_device with a "i2c-mux-reg" name lands? Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
Hi!
On 16/06/15 19:28, ext York Sun wrote:
> Based on i2c-mux-gpio driver, similarly the register based mux
> switch from one bus to another by setting a single register.
> The register can be on PCIe bus, local bus, or any memory-mapped
> address.
>
> Signed-off-by: York Sun
> CC: Wolfram Sang
> CC: Peter Korsgaard
> ---
> .../devicetree/bindings/i2c/i2c-mux-reg.txt| 69 ++
> drivers/i2c/muxes/Kconfig | 11 +
> drivers/i2c/muxes/Makefile |1 +
> drivers/i2c/muxes/i2c-mux-reg.c| 239
>
> drivers/i2c/muxes/i2c-mux-reg.h| 38
> 5 files changed, 358 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
> create mode 100644 drivers/i2c/muxes/i2c-mux-reg.c
> create mode 100644 drivers/i2c/muxes/i2c-mux-reg.h
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
> b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
> new file mode 100644
> index 000..ad7cc4f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
> @@ -0,0 +1,69 @@
> +Register-based I2C Bus Mux
> +
> +This binding describes an I2C bus multiplexer that uses a single regsiter
> +to route the I2C signals.
> +
> +Required properties:
> +- compatible: i2c-mux-reg
> +- i2c-parent: The phandle of the I2C bus that this multiplexer's master-side
> + port is connected to.
> +* Standard I2C mux properties. See mux.txt in this directory.
> +* I2C child bus nodes. See mux.txt in this directory.
> +
> +Optional properties:
> +- reg: this pair of specifies the register to control the mux.
> + The depends on its parent node. It can be any memory-mapped
> + address. If omitted, the resource of this device will be used.
> +- idle-state: value to set the muxer to when idle. When no value is
> + given, it defaults to the last value used.
> +
> +For each i2c child node, an I2C child bus will be created. They will
> +be numbered based on their order in the device tree.
> +
> +Whenever an access is made to a device on a child bus, the value set
> +in the revelant node's reg property will be output to the register.
> +
> +If an idle state is defined, using the idle-state (optional) property,
> +whenever an access is not being made to a device on a child bus, the
> +register will be set according to the idle value.
> +
> +If an idle state is not defined, the most recently used value will be
> +left programmed into the register.
> +
> +Example of a mux on PCIe card, the host is a powerpc SoC (big endian):
> +
> + i2c-mux {
> + /* the depends on the address translation
> + * of the parent device. If omitted, device resource
> + * will be used instead.
> + */
> + reg = <0x6028 0x4>;
> + compatible = "i2c-mux-reg";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + i2c-parent = <&i2c1>;
> + i2c@0 {
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + si5338: clock-generator@70 {
> + compatible = "silabs,si5338";
> + reg = <0x70>;
> + /* other stuff */
> + };
> + };
> +
> + i2c@1 {
> + /* data is in little endian on pcie bus */
> + reg = <0x0100>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + si5338: clock-generator@70 {
> + compatible = "silabs,si5338";
> + reg = <0x70>;
> + /* other stuff */
> + };
> + };
> + };
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index f6d313e..77c1257 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -29,6 +29,17 @@ config I2C_MUX_GPIO
> This driver can also be built as a module. If so, the module
> will be called i2c-mux-gpio.
>
> +config I2C_MUX_REG
> + tristate "Register-based I2C multiplexer"
> + help
> + If you say yes to this option, support will be included for a
> + register based I2C multiplexer. This driver provides access to
> + I2C busses connected through a MUX, which is controlled
> + by a sinple register.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-mux-reg.
> +
> config I2C_MUX_PCA9541
> tristate "NXP PCA9541 I2C Master Selector"
> help
> diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
> index 465778b..bc517bb 100644
> --- a/drivers/i2c/muxes
Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
Hi!
On 17/06/15 10:54, ext Paul Bolle wrote:
>> +MODULE_ALIAS("platform:i2c-mux-reg");
> As far as I understand it, this alias is only useful if there's a
> corresponding struct platform_device, somewhere. Ie, this alias needs a
> platform_device that will fire of a "MODALIAS=platform:i2c-mux-reg"
> uevent when it's created. (I don't know exactly how all that works, so
> I'm handwaving quite a bit here.) That would be platform_device with a
> "i2c-mux-reg" name.
>
> Did I get this right? Because then I think this MODULE_ALIAS() isn't
> needed, as I couldn't find such a corresponding platform_device.
You do not see the platform_device, because there are no users yet, put
this MODULE_ALIAS() is perfectly fine, it will allow automatic module loading
in non-DT case.
--
Best regards,
Alexander Sverdlin.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
A nit and a question follow.
On Tue, 2015-06-16 at 10:28 -0700, York Sun wrote:
> --- /dev/null
> +++ b/drivers/i2c/muxes/i2c-mux-reg.c
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
This states the license is GPL v2.
> +MODULE_LICENSE("GPL");
According to include/linux/module.h this sates the license is GPL v2 or
later. So I think that either the comment at the top of this file or the
ident used in the MODULE_LICENSE() macro needs to change.
> +MODULE_ALIAS("platform:i2c-mux-reg");
As far as I understand it, this alias is only useful if there's a
corresponding struct platform_device, somewhere. Ie, this alias needs a
platform_device that will fire of a "MODALIAS=platform:i2c-mux-reg"
uevent when it's created. (I don't know exactly how all that works, so
I'm handwaving quite a bit here.) That would be platform_device with a
"i2c-mux-reg" name.
Did I get this right? Because then I think this MODULE_ALIAS() isn't
needed, as I couldn't find such a corresponding platform_device.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
