Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver
On 10/20/14 13:09, Andreas Werner wrote: > On Mon, Oct 20, 2014 at 05:11:41PM +0800, Greg KH wrote: >> On Mon, Oct 20, 2014 at 10:33:45AM +0200, Andreas Werner wrote: >>> On Thu, Oct 16, 2014 at 01:44:02PM +0200, Andreas Werner wrote: On Thu, Oct 16, 2014 at 11:59:10AM +0200, Wolfram Sang wrote: > * PGP Signed by an unknown key > > >> I do not want to parse the things in userspace because this EEPROM data >> are related to the hardware and i want to give our customer the easiest >> way >> to access the data without installing any tool. > > I understand that point of view. From an upstream point of view, things > may look different, though. > I also understand your point of view :-). Most customers wants just to have a running system without installing anything. And for me an EEPROM is so simple and should not need a complicated way to access it. >> The current state to read the eeprom data is, that customer needs to >> install a big >> environment where the tool is integrated to have access to those kind of >> simple >> data or they have to write their own code. > > i2cget from i2c-tools? You could do a simple shell script to parse the > data. Or do a board specific hook which reads the data and prints it to > the logfiles... > Yes of course there are a lot of possibilities. This was just an example what we currently use and what was developed years ago. With a driver like this you can also define read only attributes to prevent customer to write or modify the data in the production section. With i2ctools you can just write any data to it you want. >>> Consider how bloated the sysfs-ABI might get if every vendor who uses an >>> eeprom wants to expose the data this way? >>> >> >> Yes and no. The possible sysfs entries gets bloated if every vendor will >> do it >> like this way, but normally there is just one Board EEPROM on the board, >> therefore >> only one driver gets loaded. > > I am not talking about runtime here, I don't care about that. I am > talking about the ABI we create and we have to maintain basically > forever. And with vendor specific configuartion data I have doubts with > that being stable. > Ok, but i do not think that we can make a "general" ABI definition for those kind of devices because every vendor will have its own data in the EEPROM which he want to have. >> I mean its the same for every i2c device like a temperature sensor, I >> can also >> read it from userspace without any special hwmon driver. > > These is a HUGE difference. If I read tempX_input, I don't need to care > if the sensor is I2C or SPI or whatever. The kernel abstracts that away. > The files you create are for your I2C EEPROM only. Data gets > "reformatted" and access gets hidden, but nothing is abstracted away. > It would be different if we had a generic convention for "serial_id" or > stuff like that. But as configuration data is highly specific I don't > see this coming. > For a standard sysfs interface it is a huge difference yes. At the point of few from the EEPROM device it is a device like a temp sensor which could be different from vendor to vendor. Regards Andy >>> >>> Greg what do you think about that driver as a Maintainer of the sysfs? >> >> I maintain the sysfs core that drivers use, I don't dictate the policy >> that those drivers create and send to userspace, that is up to the >> maintainer of the subsystem. There are some basic rules of sysfs (one >> value per file), but other than that, please work with the maintainer to >> come up with an interface that will work for all types of this device, >> not just a one-off interface which does not scale at all, as Wolfram has >> pointed out. >> > > Ok. > >>> To we have other ways to get those kind of drivers in the mainline kernel? >> >> Yes, work on a common interface that your driver can use, and it can be >> accepted. Why do you not want to do that? >> >> thanks, >> >> greg k-h > > I have never talked about that I do not want to do it. I just want to discuss > it with you. > > Right now we are at a point that i know that those kind of drivers can't be > upstream > and i will try to find a way of abstraction to get it upstream. IMO, at24 provides you a good enough abstraction which you parse from user space with a help of a really small utility or a shell script... That is a really small effort. If you want to take it further, may it is worth looking at how the DMI stuff is exported on x86 (except that you talk to eeprom directly)? -- Regards, Igor. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More major
[PATCH v2 3/3] i2c/at91: adopt pinctrl support
Amend the i2c at91 pin controller to optionally take a pin control
handle and set the state of the pins to:
- "default" on boot, resume and before performing an transfer
- "sleep" on suspend()
This should make it possible to optimize energy usage for the pins
both for the suspend/resume cycle
Signed-off-by: Wenyou Yang
Acked-by: Ludovic Desroches
---
drivers/i2c/busses/i2c-at91.c |7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 8f8fd2a..7075834 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -32,6 +32,7 @@
#include
#include
#include
+#include
#define DEFAULT_TWI_CLK_HZ 10 /* max 400 Kbits/s */
#define AT91_I2C_TIMEOUT msecs_to_jiffies(100) /* transfer timeout */
@@ -747,6 +748,8 @@ static int at91_twi_probe(struct platform_device *pdev)
u32 phy_addr;
u32 bus_clk_rate;
+ pinctrl_pm_select_default_state(&pdev->dev);
+
dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
if (!dev)
return -ENOMEM;
@@ -851,6 +854,8 @@ static int at91_twi_runtime_suspend(struct device *dev)
clk_disable_unprepare(twi_dev->clk);
+ pinctrl_pm_select_sleep_state(dev);
+
return 0;
}
@@ -858,6 +863,8 @@ static int at91_twi_runtime_resume(struct device *dev)
{
struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
+ pinctrl_pm_select_default_state(dev);
+
return clk_prepare_enable(twi_dev->clk);
}
--
1.7.9.5
--
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
[PATCH v2 2/3] i2c/at91: add support for system PM
Signed-off-by: Wenyou Yang
Acked-by: Ludovic Desroches
---
drivers/i2c/busses/i2c-at91.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 1b43b08..8f8fd2a 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -861,7 +861,32 @@ static int at91_twi_runtime_resume(struct device *dev)
return clk_prepare_enable(twi_dev->clk);
}
+static int at91_twi_suspend(struct device *dev)
+{
+ if (!pm_runtime_suspended(dev))
+ at91_twi_runtime_suspend(dev);
+
+ return 0;
+}
+
+static int at91_twi_resume(struct device *dev)
+{
+ int ret;
+
+ if (!pm_runtime_suspended(dev)) {
+ ret = at91_twi_runtime_resume(dev);
+ if (ret)
+ return ret;
+ }
+
+ pm_runtime_mark_last_busy(dev);
+ pm_request_autosuspend(dev);
+
+ return 0;
+}
+
static const struct dev_pm_ops at91_twi_pm = {
+ SET_SYSTEM_SLEEP_PM_OPS(at91_twi_suspend, at91_twi_resume)
SET_RUNTIME_PM_OPS(at91_twi_runtime_suspend,
at91_twi_runtime_resume, NULL)
};
--
1.7.9.5
--
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
[PATCH v2 1/3] i2c/at91: add support for runtime PM
Drivers should put the device into low power states proactively whenever the
device is not in use. Thus implement support for runtime PM and use the
autosuspend feature to make sure that we can still perform well in case we see
lots of i2c traffic within short period of time.
Signed-off-by: Wenyou Yang
---
drivers/i2c/busses/i2c-at91.c | 42 +
1 file changed, 34 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 917d545..1b43b08 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -31,10 +31,12 @@
#include
#include
#include
+#include
#define DEFAULT_TWI_CLK_HZ 10 /* max 400 Kbits/s */
#define AT91_I2C_TIMEOUT msecs_to_jiffies(100) /* transfer timeout */
#define AT91_I2C_DMA_THRESHOLD 8 /* enable DMA if
transfer size is bigger than this threshold */
+#define AUTOSUSPEND_TIMEOUT2000
/* AT91 TWI register definitions */
#defineAT91_TWI_CR 0x /* Control Register */
@@ -481,6 +483,10 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct
i2c_msg *msg, int num)
dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
+ ret = pm_runtime_get_sync(dev->dev);
+ if (ret < 0)
+ goto out;
+
/*
* The hardware can handle at most two messages concatenated by a
* repeated start via it's internal address feature.
@@ -488,18 +494,21 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct
i2c_msg *msg, int num)
if (num > 2) {
dev_err(dev->dev,
"cannot handle more than two concatenated messages.\n");
- return 0;
+ ret = 0;
+ goto out;
} else if (num == 2) {
int internal_address = 0;
int i;
if (msg->flags & I2C_M_RD) {
dev_err(dev->dev, "first transfer must be write.\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
if (msg->len > 3) {
dev_err(dev->dev, "first message size must be <= 3.\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
/* 1st msg is put into the internal address, start with 2nd */
@@ -523,7 +532,12 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct
i2c_msg *msg, int num)
ret = at91_do_twi_transfer(dev);
- return (ret < 0) ? ret : num;
+ ret = (ret < 0) ? ret : num;
+out:
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
+ return ret;
}
static u32 at91_twi_func(struct i2c_adapter *adapter)
@@ -795,11 +809,20 @@ static int at91_twi_probe(struct platform_device *pdev)
dev->adapter.timeout = AT91_I2C_TIMEOUT;
dev->adapter.dev.of_node = pdev->dev.of_node;
+ pm_runtime_set_autosuspend_delay(dev->dev, AUTOSUSPEND_TIMEOUT);
+ pm_runtime_use_autosuspend(dev->dev);
+ pm_runtime_set_active(dev->dev);
+ pm_runtime_enable(dev->dev);
+
rc = i2c_add_numbered_adapter(&dev->adapter);
if (rc) {
dev_err(dev->dev, "Adapter %s registration failed\n",
dev->adapter.name);
clk_disable_unprepare(dev->clk);
+
+ pm_runtime_disable(dev->dev);
+ pm_runtime_set_suspended(dev->dev);
+
return rc;
}
@@ -814,6 +837,9 @@ static int at91_twi_remove(struct platform_device *pdev)
i2c_del_adapter(&dev->adapter);
clk_disable_unprepare(dev->clk);
+ pm_runtime_disable(dev->dev);
+ pm_runtime_set_suspended(dev->dev);
+
return 0;
}
@@ -823,7 +849,7 @@ static int at91_twi_runtime_suspend(struct device *dev)
{
struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
- clk_disable(twi_dev->clk);
+ clk_disable_unprepare(twi_dev->clk);
return 0;
}
@@ -832,12 +858,12 @@ static int at91_twi_runtime_resume(struct device *dev)
{
struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
- return clk_enable(twi_dev->clk);
+ return clk_prepare_enable(twi_dev->clk);
}
static const struct dev_pm_ops at91_twi_pm = {
- .runtime_suspend= at91_twi_runtime_suspend,
- .runtime_resume = at91_twi_runtime_resume,
+ SET_RUNTIME_PM_OPS(at91_twi_runtime_suspend,
+ at91_twi_runtime_resume, NULL)
};
#define at91_twi_pm_ops (&at91_twi_pm)
--
1.7.9.5
--
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
[PATCH v2 0/3] i2c/at91: add support PM functions
Hi Wolfram, The patches is to add the PM functions support for the at91 i2c controller. It is based on the i2c/for-next branch of git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git. Best Regards, Wenyou Yang -- Change log: v2.0 According to the advice from Kevin Hilman, 1./ Wrap the runtime suspend/resume functions in CONFIG_PM instead of CONFIG_PM_RUNTIME. 2./ Call the runtime suspend/resume functions directly in the system suspend/resume. Wenyou Yang (3): i2c/at91: add support for runtime PM i2c/at91: add support for system PM i2c/at91: adopt pinctrl support drivers/i2c/busses/i2c-at91.c | 74 - 1 file changed, 66 insertions(+), 8 deletions(-) -- 1.7.9.5 -- 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 2/3] i2c/at91: add support for system PM
Hi Kevin,
> -Original Message-
> From: Kevin Hilman [mailto:khil...@kernel.org]
> Sent: Tuesday, October 21, 2014 2:16 AM
> To: Yang, Wenyou
> Cc: w...@the-dreams.de; Desroches, Ludovic; linux-i2c@vger.kernel.org; linux-
> ker...@vger.kernel.org; Ferre, Nicolas; linux-arm-ker...@lists.infradead.org
> Subject: Re: [PATCH 2/3] i2c/at91: add support for system PM
>
> Wenyou Yang writes:
>
> Add a changelog here describing what you're doing, and why.
>
> > Signed-off-by: Wenyou Yang
> > ---
> > drivers/i2c/busses/i2c-at91.c | 30 ++
> > 1 file changed, 30 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-at91.c
> > b/drivers/i2c/busses/i2c-at91.c index 03b9f48..8f408f8 100644
> > --- a/drivers/i2c/busses/i2c-at91.c
> > +++ b/drivers/i2c/busses/i2c-at91.c
> > @@ -845,6 +845,35 @@ static int at91_twi_remove(struct platform_device
> > *pdev) }
> >
> > #ifdef CONFIG_PM
> > +#ifdef CONFIG_PM_SLEEP
> > +static int at91_twi_suspend(struct device *dev) {
> > + struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
> > +
> > + if (!pm_runtime_suspended(dev))
> > + clk_disable_unprepare(twi_dev->clk);
>
> I would just call at91_twi_runtime_suspend() here.
>
> Then, if you need to add additional steps, you only have to add them in once
> place. This also makes it obvious that ->suspend and
> ->runtime_suspend are doing the exact same thing.
>
> NOTE: you'll need to wrap the runtime_suspend|resume functions in just
> CONFIG_PM instead of CONFIG_PM_RUNTIME for this to work.
Thanks a lot, I will modify it.
>
> Kevin
Best Regards,
Wenyou Yang
--
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 1/3] i2c/at91: add support for runtime PM
Hi Ludovic,
Thanks a lot.
> -Original Message-
> From: Ludovic Desroches [mailto:ludovic.desroc...@atmel.com]
> Sent: Monday, October 20, 2014 8:39 PM
> To: Yang, Wenyou
> Cc: w...@the-dreams.de; Desroches, Ludovic; linux-i2c@vger.kernel.org; linux-
> ker...@vger.kernel.org; Ferre, Nicolas; linux-arm-ker...@lists.infradead.org
> Subject: Re: [PATCH 1/3] i2c/at91: add support for runtime PM
>
> Hi Wenyou,
>
> On Mon, Oct 20, 2014 at 11:42:12AM +0800, Wenyou Yang wrote:
> > Drivers should put the device into low power states proactively
> > whenever the device is not in use. Thus implement support for runtime
> > PM and use the autosuspend feature to make sure that we can still
> > perform well in case we see lots of i2c traffic within short period of time.
> >
> > Signed-off-by: Wenyou Yang
> > ---
> > drivers/i2c/busses/i2c-at91.c | 48 ---
> --
> > 1 file changed, 38 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-at91.c
> > b/drivers/i2c/busses/i2c-at91.c index 917d545..03b9f48 100644
> > --- a/drivers/i2c/busses/i2c-at91.c
> > +++ b/drivers/i2c/busses/i2c-at91.c
> > @@ -31,10 +31,12 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #define DEFAULT_TWI_CLK_HZ 10 /* max 400 Kbits/s
> */
> > #define AT91_I2C_TIMEOUT msecs_to_jiffies(100) /* transfer timeout */
> > #define AT91_I2C_DMA_THRESHOLD 8 /* enable
> DMA if transfer size is bigger than this threshold */
> > +#define AUTOSUSPEND_TIMEOUT2000
> >
> > /* AT91 TWI register definitions */
> > #defineAT91_TWI_CR 0x /* Control Register */
> > @@ -475,12 +477,16 @@ error:
> > static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg
> > *msg, int num) {
> > struct at91_twi_dev *dev = i2c_get_adapdata(adap);
> > - int ret;
> > + int ret = 0;
>
> Not necessary.
>
> > unsigned int_addr_flag = 0;
> > struct i2c_msg *m_start = msg;
> >
> > dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
> >
> > + ret = pm_runtime_get_sync(dev->dev);
> > + if (ret < 0)
> > + goto out;
> > +
> > /*
> > * The hardware can handle at most two messages concatenated by a
> > * repeated start via it's internal address feature.
> > @@ -488,18 +494,21 @@ static int at91_twi_xfer(struct i2c_adapter *adap,
> > struct
> i2c_msg *msg, int num)
> > if (num > 2) {
> > dev_err(dev->dev,
> > "cannot handle more than two concatenated messages.\n");
> > - return 0;
> > + ret = 0;
> > + goto out;
> > } else if (num == 2) {
> > int internal_address = 0;
> > int i;
> >
> > if (msg->flags & I2C_M_RD) {
> > dev_err(dev->dev, "first transfer must be write.\n");
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto out;
> > }
> > if (msg->len > 3) {
> > dev_err(dev->dev, "first message size must be <= 3.\n");
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto out;
> > }
> >
> > /* 1st msg is put into the internal address, start with 2nd */
> > @@
> > -523,7 +532,13 @@ static int at91_twi_xfer(struct i2c_adapter *adap,
> > struct i2c_msg *msg, int num)
> >
> > ret = at91_do_twi_transfer(dev);
> >
> > - return (ret < 0) ? ret : num;
> > + if (ret == 0)
>
> I don't figure out why you've changed this condition.
Right, I will change it.
>
> > + ret = num;
> > +out:
> > + pm_runtime_mark_last_busy(dev->dev);
> > + pm_runtime_put_autosuspend(dev->dev);
> > +
> > + return ret;
> > }
> >
> > static u32 at91_twi_func(struct i2c_adapter *adapter) @@ -795,11
> > +810,20 @@ static int at91_twi_probe(struct platform_device *pdev)
> > dev->adapter.timeout = AT91_I2C_TIMEOUT;
> > dev->adapter.dev.of_node = pdev->dev.of_node;
> >
> > + pm_runtime_set_autosuspend_delay(dev->dev,
> AUTOSUSPEND_TIMEOUT);
> > + pm_runtime_use_autosuspend(dev->dev);
> > + pm_runtime_set_active(dev->dev);
> > + pm_runtime_enable(dev->dev);
> > +
> > rc = i2c_add_numbered_adapter(&dev->adapter);
> > if (rc) {
> > dev_err(dev->dev, "Adapter %s registration failed\n",
> > dev->adapter.name);
> > clk_disable_unprepare(dev->clk);
> > +
> > + pm_runtime_disable(dev->dev);
> > + pm_runtime_set_suspended(dev->dev);
> > +
> > return rc;
> > }
> >
> > @@ -814,16 +838,19 @@ static int at91_twi_remove(struct platform_device
> *pdev)
> > i2c_del_adapter(&dev->adapter);
> > clk_disable_unprepare(dev->clk);
> >
> > + pm_runtime_disable(dev->dev);
> > + pm_runtime_set_suspended(dev->dev);
> > +
> > return 0;
> > }
> >
> > #ifdef CONFIG_PM
> > -
Re: [RESEND/PATCH] i2c: pxa: Use suspend() and resume() instead of the _noirq hooks
Ezequiel Garcia writes:
> The _noirq were previously chosen to make sure all the users of the
> adapter were suspended by the time the adapter itself enters the
> suspended state.
>
> The {suspend,resume}_noirq usage was converted from an earlier
> implementation based on suspend_late and resume_early on this commit:
>
> commit 57f4d4f1b72983f8c76e2f232e064730aeffe599
> Author: Magnus Damm
> Date: Wed Jul 8 13:22:39 2009 +0200
>
> I2C: Rework i2c-pxa suspend_late()/resume_early()
>
> However, all the I2C devices are probed as children of its I2C adapter,
> and hence the device model guarantees they are suspended before its parent,
> and
> resumed after it.
>
> In other words, there's no need to use the _noirq hooks to get a
> suspend/resume
> device/adapter order.
Are you sure *really* about this?
It's usally not the children that are the problem here. It's usually
some other driver trying to use an I2C device e.g. MMC changing voltage
using an I2C-based PMIC during its suspend process.
Kevin
--
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
[RESEND/PATCH] i2c: pxa: Use suspend() and resume() instead of the _noirq hooks
The _noirq were previously chosen to make sure all the users of the
adapter were suspended by the time the adapter itself enters the
suspended state.
The {suspend,resume}_noirq usage was converted from an earlier
implementation based on suspend_late and resume_early on this commit:
commit 57f4d4f1b72983f8c76e2f232e064730aeffe599
Author: Magnus Damm
Date: Wed Jul 8 13:22:39 2009 +0200
I2C: Rework i2c-pxa suspend_late()/resume_early()
However, all the I2C devices are probed as children of its I2C adapter,
and hence the device model guarantees they are suspended before its parent, and
resumed after it.
In other words, there's no need to use the _noirq hooks to get a suspend/resume
device/adapter order.
This commit changes this by using the suspend/resume PM hooks in the I2C
adapter.
Signed-off-by: Ezequiel Garcia
---
Tested on a PXA CM-X300 board with a vendor-provided patch to make
suspend/resume work properly.
Now that the merge window is closed, I'm resending this rebased on
v3.18-rc1.
drivers/i2c/busses/i2c-pxa.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index be671f7..3be19b36 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1292,7 +1292,7 @@ static int i2c_pxa_remove(struct platform_device *dev)
}
#ifdef CONFIG_PM
-static int i2c_pxa_suspend_noirq(struct device *dev)
+static int i2c_pxa_suspend(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct pxa_i2c *i2c = platform_get_drvdata(pdev);
@@ -1302,7 +1302,7 @@ static int i2c_pxa_suspend_noirq(struct device *dev)
return 0;
}
-static int i2c_pxa_resume_noirq(struct device *dev)
+static int i2c_pxa_resume(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct pxa_i2c *i2c = platform_get_drvdata(pdev);
@@ -1314,8 +1314,8 @@ static int i2c_pxa_resume_noirq(struct device *dev)
}
static const struct dev_pm_ops i2c_pxa_dev_pm_ops = {
- .suspend_noirq = i2c_pxa_suspend_noirq,
- .resume_noirq = i2c_pxa_resume_noirq,
+ .suspend = i2c_pxa_suspend,
+ .resume = i2c_pxa_resume,
};
#define I2C_PXA_DEV_PM_OPS (&i2c_pxa_dev_pm_ops)
--
2.1.0
--
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 3/3] i2c/at91: adopt pinctrl support
Wenyou Yang writes: > Amend the i2c at91 pin controller to optionally take a pin control > handle and set the state of the pins to: > > - "default" on boot, resume and before performing an transfer > - "sleep" on suspend() > > This should make it possible to optimize energy usage for the pins > both for the suspend/resume cycle > > Signed-off-by: Wenyou Yang This patch is a good example of why you should just have the ->suspend function call the same function as ->runtime_suspend. If you do that, rather than having to add the pinctrl_pm* calls bo both system PM and runtime PM functions, you could've just added them to the runtime PM functions. Kevin -- 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 2/3] i2c/at91: add support for system PM
Wenyou Yang writes:
Add a changelog here describing what you're doing, and why.
> Signed-off-by: Wenyou Yang
> ---
> drivers/i2c/busses/i2c-at91.c | 30 ++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index 03b9f48..8f408f8 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -845,6 +845,35 @@ static int at91_twi_remove(struct platform_device *pdev)
> }
>
> #ifdef CONFIG_PM
> +#ifdef CONFIG_PM_SLEEP
> +static int at91_twi_suspend(struct device *dev)
> +{
> + struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
> +
> + if (!pm_runtime_suspended(dev))
> + clk_disable_unprepare(twi_dev->clk);
I would just call at91_twi_runtime_suspend() here.
Then, if you need to add additional steps, you only have to add them in
once place. This also makes it obvious that ->suspend and
->runtime_suspend are doing the exact same thing.
NOTE: you'll need to wrap the runtime_suspend|resume functions in just
CONFIG_PM instead of CONFIG_PM_RUNTIME for this to work.
Kevin
--
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 1/3] i2c/at91: add support for runtime PM
Adding Kevin in the CC list since he had some comments about the PM
runtime support for the SPI driver.
On Mon, Oct 20, 2014 at 02:39:14PM +0200, Ludovic Desroches wrote:
> Hi Wenyou,
>
> On Mon, Oct 20, 2014 at 11:42:12AM +0800, Wenyou Yang wrote:
> > Drivers should put the device into low power states proactively whenever the
> > device is not in use. Thus implement support for runtime PM and use the
> > autosuspend feature to make sure that we can still perform well in case we
> > see
> > lots of i2c traffic within short period of time.
> >
> > Signed-off-by: Wenyou Yang
> > ---
> > drivers/i2c/busses/i2c-at91.c | 48
> > -
> > 1 file changed, 38 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> > index 917d545..03b9f48 100644
> > --- a/drivers/i2c/busses/i2c-at91.c
> > +++ b/drivers/i2c/busses/i2c-at91.c
> > @@ -31,10 +31,12 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #define DEFAULT_TWI_CLK_HZ 10 /* max 400 Kbits/s */
> > #define AT91_I2C_TIMEOUT msecs_to_jiffies(100) /* transfer timeout */
> > #define AT91_I2C_DMA_THRESHOLD 8 /* enable DMA
> > if transfer size is bigger than this threshold */
> > +#define AUTOSUSPEND_TIMEOUT2000
> >
> > /* AT91 TWI register definitions */
> > #defineAT91_TWI_CR 0x /* Control Register */
> > @@ -475,12 +477,16 @@ error:
> > static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg,
> > int num)
> > {
> > struct at91_twi_dev *dev = i2c_get_adapdata(adap);
> > - int ret;
> > + int ret = 0;
>
> Not necessary.
>
> > unsigned int_addr_flag = 0;
> > struct i2c_msg *m_start = msg;
> >
> > dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
> >
> > + ret = pm_runtime_get_sync(dev->dev);
> > + if (ret < 0)
> > + goto out;
> > +
> > /*
> > * The hardware can handle at most two messages concatenated by a
> > * repeated start via it's internal address feature.
> > @@ -488,18 +494,21 @@ static int at91_twi_xfer(struct i2c_adapter *adap,
> > struct i2c_msg *msg, int num)
> > if (num > 2) {
> > dev_err(dev->dev,
> > "cannot handle more than two concatenated messages.\n");
> > - return 0;
> > + ret = 0;
> > + goto out;
> > } else if (num == 2) {
> > int internal_address = 0;
> > int i;
> >
> > if (msg->flags & I2C_M_RD) {
> > dev_err(dev->dev, "first transfer must be write.\n");
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto out;
> > }
> > if (msg->len > 3) {
> > dev_err(dev->dev, "first message size must be <= 3.\n");
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto out;
> > }
> >
> > /* 1st msg is put into the internal address, start with 2nd */
> > @@ -523,7 +532,13 @@ static int at91_twi_xfer(struct i2c_adapter *adap,
> > struct i2c_msg *msg, int num)
> >
> > ret = at91_do_twi_transfer(dev);
> >
> > - return (ret < 0) ? ret : num;
> > + if (ret == 0)
>
> I don't figure out why you've changed this condition.
>
> > + ret = num;
> > +out:
> > + pm_runtime_mark_last_busy(dev->dev);
> > + pm_runtime_put_autosuspend(dev->dev);
> > +
> > + return ret;
> > }
> >
> > static u32 at91_twi_func(struct i2c_adapter *adapter)
> > @@ -795,11 +810,20 @@ static int at91_twi_probe(struct platform_device
> > *pdev)
> > dev->adapter.timeout = AT91_I2C_TIMEOUT;
> > dev->adapter.dev.of_node = pdev->dev.of_node;
> >
> > + pm_runtime_set_autosuspend_delay(dev->dev, AUTOSUSPEND_TIMEOUT);
> > + pm_runtime_use_autosuspend(dev->dev);
> > + pm_runtime_set_active(dev->dev);
> > + pm_runtime_enable(dev->dev);
> > +
> > rc = i2c_add_numbered_adapter(&dev->adapter);
> > if (rc) {
> > dev_err(dev->dev, "Adapter %s registration failed\n",
> > dev->adapter.name);
> > clk_disable_unprepare(dev->clk);
> > +
> > + pm_runtime_disable(dev->dev);
> > + pm_runtime_set_suspended(dev->dev);
> > +
> > return rc;
> > }
> >
> > @@ -814,16 +838,19 @@ static int at91_twi_remove(struct platform_device
> > *pdev)
> > i2c_del_adapter(&dev->adapter);
> > clk_disable_unprepare(dev->clk);
> >
> > + pm_runtime_disable(dev->dev);
> > + pm_runtime_set_suspended(dev->dev);
> > +
> > return 0;
> > }
> >
> > #ifdef CONFIG_PM
> > -
> > +#ifdef CONFIG_PM_RUNTIME
> > static int at91_twi_runtime_suspend(struct device *dev)
> > {
> > struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
> >
> > - clk_disable(twi_dev->clk);
> > + clk_disable
Re: [PATCH 2/3] i2c/at91: add support for system PM
Adding Kevin in the CC list since he had some comments about the PM
runtime support for the SPI driver.
On Mon, Oct 20, 2014 at 02:42:42PM +0200, Ludovic Desroches wrote:
> On Mon, Oct 20, 2014 at 11:42:13AM +0800, Wenyou Yang wrote:
> > Signed-off-by: Wenyou Yang
>
> Acked by: Ludovic Desroches
>
> > ---
> > drivers/i2c/busses/i2c-at91.c | 30 ++
> > 1 file changed, 30 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> > index 03b9f48..8f408f8 100644
> > --- a/drivers/i2c/busses/i2c-at91.c
> > +++ b/drivers/i2c/busses/i2c-at91.c
> > @@ -845,6 +845,35 @@ static int at91_twi_remove(struct platform_device
> > *pdev)
> > }
> >
> > #ifdef CONFIG_PM
> > +#ifdef CONFIG_PM_SLEEP
> > +static int at91_twi_suspend(struct device *dev)
> > +{
> > + struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
> > +
> > + if (!pm_runtime_suspended(dev))
> > + clk_disable_unprepare(twi_dev->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int at91_twi_resume(struct device *dev)
> > +{
> > + struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + if (!pm_runtime_suspended(dev)) {
> > + ret = clk_prepare_enable(twi_dev->clk);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + pm_runtime_mark_last_busy(dev);
> > + pm_request_autosuspend(dev);
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > #ifdef CONFIG_PM_RUNTIME
> > static int at91_twi_runtime_suspend(struct device *dev)
> > {
> > @@ -864,6 +893,7 @@ static int at91_twi_runtime_resume(struct device *dev)
> > #endif
> >
> > static const struct dev_pm_ops at91_twi_pm = {
> > + SET_SYSTEM_SLEEP_PM_OPS(at91_twi_suspend, at91_twi_resume)
> > SET_RUNTIME_PM_OPS(at91_twi_runtime_suspend,
> > at91_twi_runtime_resume, NULL)
> > };
> > --
> > 1.7.9.5
> >
--
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 3/3] i2c/at91: adopt pinctrl support
On Mon, Oct 20, 2014 at 11:42:14AM +0800, Wenyou Yang wrote:
> Amend the i2c at91 pin controller to optionally take a pin control
> handle and set the state of the pins to:
>
> - "default" on boot, resume and before performing an transfer
> - "sleep" on suspend()
>
> This should make it possible to optimize energy usage for the pins
> both for the suspend/resume cycle
>
> Signed-off-by: Wenyou Yang
Acked by: Ludovic Desroches
> ---
> drivers/i2c/busses/i2c-at91.c | 12 +++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index 8f408f8..ed2c382 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -32,6 +32,7 @@
> #include
> #include
> #include
> +#include
>
> #define DEFAULT_TWI_CLK_HZ 10 /* max 400 Kbits/s */
> #define AT91_I2C_TIMEOUT msecs_to_jiffies(100) /* transfer timeout */
> @@ -748,6 +749,8 @@ static int at91_twi_probe(struct platform_device *pdev)
> u32 phy_addr;
> u32 bus_clk_rate;
>
> + pinctrl_pm_select_default_state(&pdev->dev);
> +
> dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> if (!dev)
> return -ENOMEM;
> @@ -850,8 +853,10 @@ static int at91_twi_suspend(struct device *dev)
> {
> struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
>
> - if (!pm_runtime_suspended(dev))
> + if (!pm_runtime_suspended(dev)) {
> clk_disable_unprepare(twi_dev->clk);
> + pinctrl_pm_select_sleep_state(dev);
> + }
>
> return 0;
> }
> @@ -862,6 +867,7 @@ static int at91_twi_resume(struct device *dev)
> int ret;
>
> if (!pm_runtime_suspended(dev)) {
> + pinctrl_pm_select_default_state(dev);
> ret = clk_prepare_enable(twi_dev->clk);
> if (ret)
> return ret;
> @@ -881,6 +887,8 @@ static int at91_twi_runtime_suspend(struct device *dev)
>
> clk_disable_unprepare(twi_dev->clk);
>
> + pinctrl_pm_select_sleep_state(dev);
> +
> return 0;
> }
>
> @@ -888,6 +896,8 @@ static int at91_twi_runtime_resume(struct device *dev)
> {
> struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
>
> + pinctrl_pm_select_default_state(dev);
> +
> return clk_prepare_enable(twi_dev->clk);
> }
> #endif
> --
> 1.7.9.5
>
--
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 2/3] i2c/at91: add support for system PM
On Mon, Oct 20, 2014 at 11:42:13AM +0800, Wenyou Yang wrote:
> Signed-off-by: Wenyou Yang
Acked by: Ludovic Desroches
> ---
> drivers/i2c/busses/i2c-at91.c | 30 ++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index 03b9f48..8f408f8 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -845,6 +845,35 @@ static int at91_twi_remove(struct platform_device *pdev)
> }
>
> #ifdef CONFIG_PM
> +#ifdef CONFIG_PM_SLEEP
> +static int at91_twi_suspend(struct device *dev)
> +{
> + struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
> +
> + if (!pm_runtime_suspended(dev))
> + clk_disable_unprepare(twi_dev->clk);
> +
> + return 0;
> +}
> +
> +static int at91_twi_resume(struct device *dev)
> +{
> + struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
> + int ret;
> +
> + if (!pm_runtime_suspended(dev)) {
> + ret = clk_prepare_enable(twi_dev->clk);
> + if (ret)
> + return ret;
> + }
> +
> + pm_runtime_mark_last_busy(dev);
> + pm_request_autosuspend(dev);
> +
> + return 0;
> +}
> +#endif
> +
> #ifdef CONFIG_PM_RUNTIME
> static int at91_twi_runtime_suspend(struct device *dev)
> {
> @@ -864,6 +893,7 @@ static int at91_twi_runtime_resume(struct device *dev)
> #endif
>
> static const struct dev_pm_ops at91_twi_pm = {
> + SET_SYSTEM_SLEEP_PM_OPS(at91_twi_suspend, at91_twi_resume)
> SET_RUNTIME_PM_OPS(at91_twi_runtime_suspend,
> at91_twi_runtime_resume, NULL)
> };
> --
> 1.7.9.5
>
--
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 1/3] i2c/at91: add support for runtime PM
Hi Wenyou,
On Mon, Oct 20, 2014 at 11:42:12AM +0800, Wenyou Yang wrote:
> Drivers should put the device into low power states proactively whenever the
> device is not in use. Thus implement support for runtime PM and use the
> autosuspend feature to make sure that we can still perform well in case we see
> lots of i2c traffic within short period of time.
>
> Signed-off-by: Wenyou Yang
> ---
> drivers/i2c/busses/i2c-at91.c | 48
> -
> 1 file changed, 38 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index 917d545..03b9f48 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -31,10 +31,12 @@
> #include
> #include
> #include
> +#include
>
> #define DEFAULT_TWI_CLK_HZ 10 /* max 400 Kbits/s */
> #define AT91_I2C_TIMEOUT msecs_to_jiffies(100) /* transfer timeout */
> #define AT91_I2C_DMA_THRESHOLD 8 /* enable DMA
> if transfer size is bigger than this threshold */
> +#define AUTOSUSPEND_TIMEOUT 2000
>
> /* AT91 TWI register definitions */
> #define AT91_TWI_CR 0x /* Control Register */
> @@ -475,12 +477,16 @@ error:
> static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int
> num)
> {
> struct at91_twi_dev *dev = i2c_get_adapdata(adap);
> - int ret;
> + int ret = 0;
Not necessary.
> unsigned int_addr_flag = 0;
> struct i2c_msg *m_start = msg;
>
> dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
>
> + ret = pm_runtime_get_sync(dev->dev);
> + if (ret < 0)
> + goto out;
> +
> /*
>* The hardware can handle at most two messages concatenated by a
>* repeated start via it's internal address feature.
> @@ -488,18 +494,21 @@ static int at91_twi_xfer(struct i2c_adapter *adap,
> struct i2c_msg *msg, int num)
> if (num > 2) {
> dev_err(dev->dev,
> "cannot handle more than two concatenated messages.\n");
> - return 0;
> + ret = 0;
> + goto out;
> } else if (num == 2) {
> int internal_address = 0;
> int i;
>
> if (msg->flags & I2C_M_RD) {
> dev_err(dev->dev, "first transfer must be write.\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
> if (msg->len > 3) {
> dev_err(dev->dev, "first message size must be <= 3.\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
>
> /* 1st msg is put into the internal address, start with 2nd */
> @@ -523,7 +532,13 @@ static int at91_twi_xfer(struct i2c_adapter *adap,
> struct i2c_msg *msg, int num)
>
> ret = at91_do_twi_transfer(dev);
>
> - return (ret < 0) ? ret : num;
> + if (ret == 0)
I don't figure out why you've changed this condition.
> + ret = num;
> +out:
> + pm_runtime_mark_last_busy(dev->dev);
> + pm_runtime_put_autosuspend(dev->dev);
> +
> + return ret;
> }
>
> static u32 at91_twi_func(struct i2c_adapter *adapter)
> @@ -795,11 +810,20 @@ static int at91_twi_probe(struct platform_device *pdev)
> dev->adapter.timeout = AT91_I2C_TIMEOUT;
> dev->adapter.dev.of_node = pdev->dev.of_node;
>
> + pm_runtime_set_autosuspend_delay(dev->dev, AUTOSUSPEND_TIMEOUT);
> + pm_runtime_use_autosuspend(dev->dev);
> + pm_runtime_set_active(dev->dev);
> + pm_runtime_enable(dev->dev);
> +
> rc = i2c_add_numbered_adapter(&dev->adapter);
> if (rc) {
> dev_err(dev->dev, "Adapter %s registration failed\n",
> dev->adapter.name);
> clk_disable_unprepare(dev->clk);
> +
> + pm_runtime_disable(dev->dev);
> + pm_runtime_set_suspended(dev->dev);
> +
> return rc;
> }
>
> @@ -814,16 +838,19 @@ static int at91_twi_remove(struct platform_device *pdev)
> i2c_del_adapter(&dev->adapter);
> clk_disable_unprepare(dev->clk);
>
> + pm_runtime_disable(dev->dev);
> + pm_runtime_set_suspended(dev->dev);
> +
> return 0;
> }
>
> #ifdef CONFIG_PM
> -
> +#ifdef CONFIG_PM_RUNTIME
> static int at91_twi_runtime_suspend(struct device *dev)
> {
> struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
>
> - clk_disable(twi_dev->clk);
> + clk_disable_unprepare(twi_dev->clk);
>
> return 0;
> }
> @@ -832,12 +859,13 @@ static int at91_twi_runtime_resume(struct device *dev)
> {
> struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
>
> - return clk_enable(twi_dev->clk);
> + return clk_prepare_enable(twi_dev->clk);
> }
> +#endif
>
> static const struct d
Re: [PATCH v8 3/4] gpiolib: add irq_not_threaded flag to gpio_chip
On Mon, Oct 20, 2014 at 8:08 AM, Alexandre Courbot wrote: > > On Wed, Oct 15, 2014 at 11:48 PM, Octavian Purdila > wrote: > > Some GPIO chips (e.g. the DLN2 USB adapter) have blocking get/set > > operation but do not need a threaded irq handler. > > Sorry if you already explained this (I have been a little bit late > with the GPIO reviews recently), but does this optimization bring a > significant benefit that justifies adding another field in struct > gpio_chip? If so it would be nice to have it in the commit message. If > not, do we need this at all? Hi Alexandre, In the case DLN2 USB GPIO adapter the GPIO interrupt is generated in the completion routine of a receive URB, which means that we are in interrupt context. If a threaded irq is used, we would have to schedule work instead of running to interrupt handler directly which is unnecessary and adds latency. BTW, AFAIC Linus W already merged this patch in his next tree, I am keeping it in this series because it was not pulled in the mfd-next tree. Thanks, Tavi -- 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 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver
On Mon, Oct 20, 2014 at 05:11:41PM +0800, Greg KH wrote: > On Mon, Oct 20, 2014 at 10:33:45AM +0200, Andreas Werner wrote: > > On Thu, Oct 16, 2014 at 01:44:02PM +0200, Andreas Werner wrote: > > > On Thu, Oct 16, 2014 at 11:59:10AM +0200, Wolfram Sang wrote: > > > > * PGP Signed by an unknown key > > > > > > > > > > > > > I do not want to parse the things in userspace because this EEPROM > > > > > data > > > > > are related to the hardware and i want to give our customer the > > > > > easiest way > > > > > to access the data without installing any tool. > > > > > > > > I understand that point of view. From an upstream point of view, things > > > > may look different, though. > > > > > > > > > > I also understand your point of view :-). > > > Most customers wants just to have a running system without installing > > > anything. > > > And for me an EEPROM is so simple and should not need a complicated way > > > to access it. > > > > > > > > The current state to read the eeprom data is, that customer needs to > > > > > install a big > > > > > environment where the tool is integrated to have access to those kind > > > > > of simple > > > > > data or they have to write their own code. > > > > > > > > i2cget from i2c-tools? You could do a simple shell script to parse the > > > > data. Or do a board specific hook which reads the data and prints it to > > > > the logfiles... > > > > > > > > > > Yes of course there are a lot of possibilities. This was just an example > > > what we currently use and what was developed years ago. > > > > > > With a driver like this you can also define read only attributes to > > > prevent customer > > > to write or modify the data in the production section. With i2ctools you > > > can just > > > write any data to it you want. > > > > > > > > > Consider how bloated the sysfs-ABI might get if every vendor who > > > > > > uses an > > > > > > eeprom wants to expose the data this way? > > > > > > > > > > > > > > > > Yes and no. The possible sysfs entries gets bloated if every vendor > > > > > will do it > > > > > like this way, but normally there is just one Board EEPROM on the > > > > > board, therefore > > > > > only one driver gets loaded. > > > > > > > > I am not talking about runtime here, I don't care about that. I am > > > > talking about the ABI we create and we have to maintain basically > > > > forever. And with vendor specific configuartion data I have doubts with > > > > that being stable. > > > > > > > > > > Ok, but i do not think that we can make a "general" ABI definition for > > > those kind > > > of devices because every vendor will have its own data in the EEPROM > > > which he want > > > to have. > > > > > > > > I mean its the same for every i2c device like a temperature sensor, I > > > > > can also > > > > > read it from userspace without any special hwmon driver. > > > > > > > > These is a HUGE difference. If I read tempX_input, I don't need to care > > > > if the sensor is I2C or SPI or whatever. The kernel abstracts that away. > > > > The files you create are for your I2C EEPROM only. Data gets > > > > "reformatted" and access gets hidden, but nothing is abstracted away. > > > > It would be different if we had a generic convention for "serial_id" or > > > > stuff like that. But as configuration data is highly specific I don't > > > > see this coming. > > > > > > > > > > For a standard sysfs interface it is a huge difference yes. At the point > > > of few from the EEPROM device it is a device like a temp sensor which > > > could be different from vendor to vendor. > > > > > > Regards > > > Andy > > > > > > > Greg what do you think about that driver as a Maintainer of the sysfs? > > I maintain the sysfs core that drivers use, I don't dictate the policy > that those drivers create and send to userspace, that is up to the > maintainer of the subsystem. There are some basic rules of sysfs (one > value per file), but other than that, please work with the maintainer to > come up with an interface that will work for all types of this device, > not just a one-off interface which does not scale at all, as Wolfram has > pointed out. > Ok. > > To we have other ways to get those kind of drivers in the mainline kernel? > > Yes, work on a common interface that your driver can use, and it can be > accepted. Why do you not want to do that? > > thanks, > > greg k-h I have never talked about that I do not want to do it. I just want to discuss it with you. Right now we are at a point that i know that those kind of drivers can't be upstream and i will try to find a way of abstraction to get it upstream. Thanks Andy -- 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 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver
On Mon, Oct 20, 2014 at 10:24:22AM +0200, Wolfram Sang wrote: > * PGP Signed by an unknown key > > > Here it gets frustrating. It seems you have no idea what an OS is for, > > not even after I tried to describe it :( > I am pretty sure that i know what an OS is for. > Sorry, that might have been too strong. Still, we can't map any hardware > which is out there 1:1 into userspace, we need abstraction. > > If you want to help with this abstraction, this is more than > appreciated. If you want to keep your driver, this will have to stay > out-of-tree, I'm afraid. > Yes, my goal is to find a good way to get hardware support upstream, and it is also nice to discuss my point of view with your upstream point of few. And yes, i want to help to find a way to develope an abstraction. Regards Andy > Regards, > >Wolfram > > > * Unknown Key > * 0x14A029B6 -- 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 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver
On Mon, Oct 20, 2014 at 10:33:45AM +0200, Andreas Werner wrote: > On Thu, Oct 16, 2014 at 01:44:02PM +0200, Andreas Werner wrote: > > On Thu, Oct 16, 2014 at 11:59:10AM +0200, Wolfram Sang wrote: > > > * PGP Signed by an unknown key > > > > > > > > > > I do not want to parse the things in userspace because this EEPROM data > > > > are related to the hardware and i want to give our customer the easiest > > > > way > > > > to access the data without installing any tool. > > > > > > I understand that point of view. From an upstream point of view, things > > > may look different, though. > > > > > > > I also understand your point of view :-). > > Most customers wants just to have a running system without installing > > anything. > > And for me an EEPROM is so simple and should not need a complicated way > > to access it. > > > > > > The current state to read the eeprom data is, that customer needs to > > > > install a big > > > > environment where the tool is integrated to have access to those kind > > > > of simple > > > > data or they have to write their own code. > > > > > > i2cget from i2c-tools? You could do a simple shell script to parse the > > > data. Or do a board specific hook which reads the data and prints it to > > > the logfiles... > > > > > > > Yes of course there are a lot of possibilities. This was just an example > > what we currently use and what was developed years ago. > > > > With a driver like this you can also define read only attributes to prevent > > customer > > to write or modify the data in the production section. With i2ctools you > > can just > > write any data to it you want. > > > > > > > Consider how bloated the sysfs-ABI might get if every vendor who uses > > > > > an > > > > > eeprom wants to expose the data this way? > > > > > > > > > > > > > Yes and no. The possible sysfs entries gets bloated if every vendor > > > > will do it > > > > like this way, but normally there is just one Board EEPROM on the > > > > board, therefore > > > > only one driver gets loaded. > > > > > > I am not talking about runtime here, I don't care about that. I am > > > talking about the ABI we create and we have to maintain basically > > > forever. And with vendor specific configuartion data I have doubts with > > > that being stable. > > > > > > > Ok, but i do not think that we can make a "general" ABI definition for > > those kind > > of devices because every vendor will have its own data in the EEPROM which > > he want > > to have. > > > > > > I mean its the same for every i2c device like a temperature sensor, I > > > > can also > > > > read it from userspace without any special hwmon driver. > > > > > > These is a HUGE difference. If I read tempX_input, I don't need to care > > > if the sensor is I2C or SPI or whatever. The kernel abstracts that away. > > > The files you create are for your I2C EEPROM only. Data gets > > > "reformatted" and access gets hidden, but nothing is abstracted away. > > > It would be different if we had a generic convention for "serial_id" or > > > stuff like that. But as configuration data is highly specific I don't > > > see this coming. > > > > > > > For a standard sysfs interface it is a huge difference yes. At the point > > of few from the EEPROM device it is a device like a temp sensor which > > could be different from vendor to vendor. > > > > Regards > > Andy > > > > Greg what do you think about that driver as a Maintainer of the sysfs? I maintain the sysfs core that drivers use, I don't dictate the policy that those drivers create and send to userspace, that is up to the maintainer of the subsystem. There are some basic rules of sysfs (one value per file), but other than that, please work with the maintainer to come up with an interface that will work for all types of this device, not just a one-off interface which does not scale at all, as Wolfram has pointed out. > To we have other ways to get those kind of drivers in the mainline kernel? Yes, work on a common interface that your driver can use, and it can be accepted. Why do you not want to do that? thanks, greg k-h -- 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 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver
> Here it gets frustrating. It seems you have no idea what an OS is for, > not even after I tried to describe it :( Sorry, that might have been too strong. Still, we can't map any hardware which is out there 1:1 into userspace, we need abstraction. If you want to help with this abstraction, this is more than appreciated. If you want to keep your driver, this will have to stay out-of-tree, I'm afraid. Regards, Wolfram signature.asc Description: Digital signature
Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver
> Most customers wants just to have a running system without installing > anything. > And for me an EEPROM is so simple and should not need a complicated way > to access it. As I pointed out, there are ways to do it other than a seperate driver. > Yes of course there are a lot of possibilities. This was just an example > what we currently use and what was developed years ago. And please notice that this solution you chose is not acceptible for upstream. We can't have n copies of the at24 driver with just some minor differences. This is a maintenance nightmare. Yes, I know it was easiest to start with and it works, but that does not help here. > With a driver like this you can also define read only attributes to prevent > customer > to write or modify the data in the production section. With i2ctools you can > just > write any data to it you want. i2cget won't modify any data. i2cset does, if anyone wants to do that. BTW it does that also after you removed your driver, so basically no big difference here. > > I am not talking about runtime here, I don't care about that. I am > > talking about the ABI we create and we have to maintain basically > > forever. And with vendor specific configuartion data I have doubts with > > that being stable. > > > > Ok, but i do not think that we can make a "general" ABI definition for those > kind > of devices because every vendor will have its own data in the EEPROM which he > want > to have. Exactly, that was what I was saying. What we probably should have in at24 is regions which should not be exposed to userspace, because they contain config data. That would be nice. I'm not sure if we can add table based decoding to at24, that needs some experiments. But it will really need such experiments and some more effort. > > These is a HUGE difference. If I read tempX_input, I don't need to care > > if the sensor is I2C or SPI or whatever. The kernel abstracts that away. > > The files you create are for your I2C EEPROM only. Data gets > > "reformatted" and access gets hidden, but nothing is abstracted away. > > It would be different if we had a generic convention for "serial_id" or > > stuff like that. But as configuration data is highly specific I don't > > see this coming. > > > > For a standard sysfs interface it is a huge difference yes. At the point > of few from the EEPROM device it is a device like a temp sensor which > could be different from vendor to vendor. Here it gets frustrating. It seems you have no idea what an OS is for, not even after I tried to describe it :( signature.asc Description: Digital signature
Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver
On Thu, Oct 16, 2014 at 01:44:02PM +0200, Andreas Werner wrote: > On Thu, Oct 16, 2014 at 11:59:10AM +0200, Wolfram Sang wrote: > > * PGP Signed by an unknown key > > > > > > > I do not want to parse the things in userspace because this EEPROM data > > > are related to the hardware and i want to give our customer the easiest > > > way > > > to access the data without installing any tool. > > > > I understand that point of view. From an upstream point of view, things > > may look different, though. > > > > I also understand your point of view :-). > Most customers wants just to have a running system without installing > anything. > And for me an EEPROM is so simple and should not need a complicated way > to access it. > > > > The current state to read the eeprom data is, that customer needs to > > > install a big > > > environment where the tool is integrated to have access to those kind of > > > simple > > > data or they have to write their own code. > > > > i2cget from i2c-tools? You could do a simple shell script to parse the > > data. Or do a board specific hook which reads the data and prints it to > > the logfiles... > > > > Yes of course there are a lot of possibilities. This was just an example > what we currently use and what was developed years ago. > > With a driver like this you can also define read only attributes to prevent > customer > to write or modify the data in the production section. With i2ctools you can > just > write any data to it you want. > > > > > Consider how bloated the sysfs-ABI might get if every vendor who uses an > > > > eeprom wants to expose the data this way? > > > > > > > > > > Yes and no. The possible sysfs entries gets bloated if every vendor will > > > do it > > > like this way, but normally there is just one Board EEPROM on the board, > > > therefore > > > only one driver gets loaded. > > > > I am not talking about runtime here, I don't care about that. I am > > talking about the ABI we create and we have to maintain basically > > forever. And with vendor specific configuartion data I have doubts with > > that being stable. > > > > Ok, but i do not think that we can make a "general" ABI definition for those > kind > of devices because every vendor will have its own data in the EEPROM which he > want > to have. > > > > I mean its the same for every i2c device like a temperature sensor, I can > > > also > > > read it from userspace without any special hwmon driver. > > > > These is a HUGE difference. If I read tempX_input, I don't need to care > > if the sensor is I2C or SPI or whatever. The kernel abstracts that away. > > The files you create are for your I2C EEPROM only. Data gets > > "reformatted" and access gets hidden, but nothing is abstracted away. > > It would be different if we had a generic convention for "serial_id" or > > stuff like that. But as configuration data is highly specific I don't > > see this coming. > > > > For a standard sysfs interface it is a huge difference yes. At the point > of few from the EEPROM device it is a device like a temp sensor which > could be different from vendor to vendor. > > Regards > Andy > Greg what do you think about that driver as a Maintainer of the sysfs? To we have other ways to get those kind of drivers in the mainline kernel? Regards Andy > > > > * Unknown Key > > * 0x14A029B6 -- 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
