| Message ID | 20220318015451.2869388-4-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Update cros-ec-spi for fingerprint devices | expand |
Hi, On Thu, Mar 17, 2022 at 6:55 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Add gpio control to this driver so that the fingerprint device can be > booted if the BIOS isn't doing it already. This eases bringup of new > hardware as we don't have to wait for the BIOS to be ready, supports > kexec where the GPIOs may not be configured by the previous boot stage, > and is all around good hygiene because we control GPIOs for this device > from the device driver. > > Cc: Guenter Roeck <groeck@chromium.org> > Cc: Douglas Anderson <dianders@chromium.org> > Cc: Craig Hesling <hesling@chromium.org> > Cc: Tom Hughes <tomhughes@chromium.org> > Cc: Alexandru M Stan <amstan@chromium.org> > Cc: Tzung-Bi Shih <tzungbi@kernel.org> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- > drivers/platform/chrome/cros_ec_spi.c | 42 +++++++++++++++++++++++++-- > 1 file changed, 39 insertions(+), 3 deletions(-) > > diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c > index d0f9496076d6..13d413a2fe46 100644 > --- a/drivers/platform/chrome/cros_ec_spi.c > +++ b/drivers/platform/chrome/cros_ec_spi.c > @@ -4,6 +4,7 @@ > // Copyright (C) 2012 Google, Inc > > #include <linux/delay.h> > +#include <linux/gpio/consumer.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/of.h> > @@ -77,6 +78,8 @@ struct cros_ec_spi { > unsigned int start_of_msg_delay; > unsigned int end_of_msg_delay; > struct kthread_worker *high_pri_worker; > + struct gpio_desc *boot0; > + struct gpio_desc *reset; This structure has members described with kernel-doc. You should document your members. > }; > > typedef int (*cros_ec_xfer_fn_t) (struct cros_ec_device *ec_dev, > @@ -690,7 +693,7 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev, > return cros_ec_xfer_high_pri(ec_dev, ec_msg, do_cros_ec_cmd_xfer_spi); > } > > -static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev) > +static int cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev) > { > struct device_node *np = dev->of_node; > u32 val; > @@ -703,6 +706,37 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev) > ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val); > if (!ret) > ec_spi->end_of_msg_delay = val; > + > + if (!of_device_is_compatible(np, "google,cros-ec-fp")) > + return 0; I noticed in your previous patch that you not only added a device-tree match for this device but also a "spi_device_id". ...but won't you fail to do all this important GPIO work in that case? > + ec_spi->boot0 = devm_gpiod_get(dev, "boot0", 0); > + if (IS_ERR(ec_spi->boot0)) > + return PTR_ERR(ec_spi->boot0); Right now these GPIOs don't actually need to be stored in the "ec_spi" structure. They could just be local variables. I guess you're trying to future proof? > + ec_spi->reset = devm_gpiod_get(dev, "reset", 0); > + if (IS_ERR(ec_spi->reset)) > + return PTR_ERR(ec_spi->reset); > + > + /* > + * Take the FPMCU out of reset and wait for it to boot if it's in > + * bootloader mode or held in reset. This isn't the normal flow because > + * typically the BIOS has already powered on the device to avoid the > + * multi-second delay waiting for the FPMCU to boot and be responsive. > + */ > + if (gpiod_get_value(ec_spi->boot0) || gpiod_get_value(ec_spi->reset)) { > + /* Boot0 is sampled on reset deassertion */ > + gpiod_set_value(ec_spi->boot0, 0); > + gpiod_set_value(ec_spi->reset, 1); > + usleep_range(1000, 2000); > + gpiod_set_value(ec_spi->reset, 0); > + > + /* Wait for boot; there isn't a "boot done" signal */ > + dev_info(dev, "Waiting for FPMCU to boot\n"); > + msleep(2000); > + } You added the regulator to the bindings. On herobrine I know that the regulator is a bit of a dummy (at least on herobrine), but I wonder if you should still get/enable it here? In the device tree bindings you listed it as not-optional so, in theory, you could use this to give an error if someone didn't provide the regulator. BTW: it seems like it wouldn't be a _crazy_ amount of extra work to: 1. Add a sysfs hook for turning the regulator on/off 2. Change the Chrome OS userspace to actually use the sysfs hook if it's there. 3. Actually have the kernel in charge of turning the regulator off/on Doing this at the same time as the transition over to the more real "cros-ec-fp" would be nice so we don't have to figure out how to transition later. Said another way: If we don't transition now then I guess later we'd have to find some way to detect that the regulator specified in the kernel was actually a dummy and didn't really control the power?
Quoting Doug Anderson (2022-03-18 13:50:05) > Hi, > > On Thu, Mar 17, 2022 at 6:55 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Add gpio control to this driver so that the fingerprint device can be > > booted if the BIOS isn't doing it already. This eases bringup of new > > hardware as we don't have to wait for the BIOS to be ready, supports > > kexec where the GPIOs may not be configured by the previous boot stage, > > and is all around good hygiene because we control GPIOs for this device > > from the device driver. > > > > Cc: Guenter Roeck <groeck@chromium.org> > > Cc: Douglas Anderson <dianders@chromium.org> > > Cc: Craig Hesling <hesling@chromium.org> > > Cc: Tom Hughes <tomhughes@chromium.org> > > Cc: Alexandru M Stan <amstan@chromium.org> > > Cc: Tzung-Bi Shih <tzungbi@kernel.org> > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > > --- > > drivers/platform/chrome/cros_ec_spi.c | 42 +++++++++++++++++++++++++-- > > 1 file changed, 39 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c > > index d0f9496076d6..13d413a2fe46 100644 > > --- a/drivers/platform/chrome/cros_ec_spi.c > > +++ b/drivers/platform/chrome/cros_ec_spi.c > > @@ -4,6 +4,7 @@ > > // Copyright (C) 2012 Google, Inc > > > > #include <linux/delay.h> > > +#include <linux/gpio/consumer.h> > > #include <linux/kernel.h> > > #include <linux/module.h> > > #include <linux/of.h> > > @@ -77,6 +78,8 @@ struct cros_ec_spi { > > unsigned int start_of_msg_delay; > > unsigned int end_of_msg_delay; > > struct kthread_worker *high_pri_worker; > > + struct gpio_desc *boot0; > > + struct gpio_desc *reset; > > This structure has members described with kernel-doc. You should > document your members. > > > > }; > > > > typedef int (*cros_ec_xfer_fn_t) (struct cros_ec_device *ec_dev, > > @@ -690,7 +693,7 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev, > > return cros_ec_xfer_high_pri(ec_dev, ec_msg, do_cros_ec_cmd_xfer_spi); > > } > > > > -static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev) > > +static int cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev) > > { > > struct device_node *np = dev->of_node; > > u32 val; > > @@ -703,6 +706,37 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev) > > ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val); > > if (!ret) > > ec_spi->end_of_msg_delay = val; > > + > > + if (!of_device_is_compatible(np, "google,cros-ec-fp")) > > + return 0; > > I noticed in your previous patch that you not only added a device-tree > match for this device but also a "spi_device_id". ...but won't you > fail to do all this important GPIO work in that case? I don't know when the spi_device_id path will be used. Never? I can simply drop it from the spi_device_id list for now and we can take up this problem later or never. > > > > + ec_spi->boot0 = devm_gpiod_get(dev, "boot0", 0); > > + if (IS_ERR(ec_spi->boot0)) > > + return PTR_ERR(ec_spi->boot0); > > Right now these GPIOs don't actually need to be stored in the "ec_spi" > structure. They could just be local variables. I guess you're trying > to future proof? Sure I will drop them because they're not useful later and I can save on the kernel-doc. > > > > + ec_spi->reset = devm_gpiod_get(dev, "reset", 0); > > + if (IS_ERR(ec_spi->reset)) > > + return PTR_ERR(ec_spi->reset); > > + > > + /* > > + * Take the FPMCU out of reset and wait for it to boot if it's in > > + * bootloader mode or held in reset. This isn't the normal flow because > > + * typically the BIOS has already powered on the device to avoid the > > + * multi-second delay waiting for the FPMCU to boot and be responsive. > > + */ > > + if (gpiod_get_value(ec_spi->boot0) || gpiod_get_value(ec_spi->reset)) { > > + /* Boot0 is sampled on reset deassertion */ > > + gpiod_set_value(ec_spi->boot0, 0); > > + gpiod_set_value(ec_spi->reset, 1); > > + usleep_range(1000, 2000); > > + gpiod_set_value(ec_spi->reset, 0); > > + > > + /* Wait for boot; there isn't a "boot done" signal */ > > + dev_info(dev, "Waiting for FPMCU to boot\n"); > > + msleep(2000); > > + } > > You added the regulator to the bindings. On herobrine I know that the > regulator is a bit of a dummy (at least on herobrine), but I wonder if > you should still get/enable it here? In the device tree bindings you > listed it as not-optional so, in theory, you could use this to give an > error if someone didn't provide the regulator. Won't the regulator framework introduce a dummy supply if there isn't a supply in DT but this driver calls regulator_get()? Getting and enabling it here will make this even more independent though so it sounds like a good idea. That way we can make it a real regulator in the DTS as long as the firmware isn't controlling it. > > BTW: it seems like it wouldn't be a _crazy_ amount of extra work to: > > 1. Add a sysfs hook for turning the regulator on/off > > 2. Change the Chrome OS userspace to actually use the sysfs hook if it's there. > > 3. Actually have the kernel in charge of turning the regulator off/on > > Doing this at the same time as the transition over to the more real > "cros-ec-fp" would be nice so we don't have to figure out how to > transition later. Said another way: If we don't transition now then I > guess later we'd have to find some way to detect that the regulator > specified in the kernel was actually a dummy and didn't really control > the power? I'd rather not expose regulator control to userspace through some other sysfs attribute. Instead I'd prefer the flashing logic that twiddles gpios and power live all in the kernel and have userspace interact with a character device to program the firmware.
Hi, On Fri, Mar 18, 2022 at 3:01 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > @@ -703,6 +706,37 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev) > > > ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val); > > > if (!ret) > > > ec_spi->end_of_msg_delay = val; > > > + > > > + if (!of_device_is_compatible(np, "google,cros-ec-fp")) > > > + return 0; > > > > I noticed in your previous patch that you not only added a device-tree > > match for this device but also a "spi_device_id". ...but won't you > > fail to do all this important GPIO work in that case? > > I don't know when the spi_device_id path will be used. Never? I can > simply drop it from the spi_device_id list for now and we can take up > this problem later or never. That's fine with me. I was guessing it was relevant for x86 but my experience with the way x86 does things is pretty minimal. > > > + ec_spi->boot0 = devm_gpiod_get(dev, "boot0", 0); > > > + if (IS_ERR(ec_spi->boot0)) > > > + return PTR_ERR(ec_spi->boot0); > > > > Right now these GPIOs don't actually need to be stored in the "ec_spi" > > structure. They could just be local variables. I guess you're trying > > to future proof? > > Sure I will drop them because they're not useful later and I can save on > the kernel-doc. > > > > > > > > + ec_spi->reset = devm_gpiod_get(dev, "reset", 0); > > > + if (IS_ERR(ec_spi->reset)) > > > + return PTR_ERR(ec_spi->reset); > > > + > > > + /* > > > + * Take the FPMCU out of reset and wait for it to boot if it's in > > > + * bootloader mode or held in reset. This isn't the normal flow because > > > + * typically the BIOS has already powered on the device to avoid the > > > + * multi-second delay waiting for the FPMCU to boot and be responsive. > > > + */ > > > + if (gpiod_get_value(ec_spi->boot0) || gpiod_get_value(ec_spi->reset)) { > > > + /* Boot0 is sampled on reset deassertion */ > > > + gpiod_set_value(ec_spi->boot0, 0); > > > + gpiod_set_value(ec_spi->reset, 1); > > > + usleep_range(1000, 2000); > > > + gpiod_set_value(ec_spi->reset, 0); > > > + > > > + /* Wait for boot; there isn't a "boot done" signal */ > > > + dev_info(dev, "Waiting for FPMCU to boot\n"); > > > + msleep(2000); > > > + } > > > > You added the regulator to the bindings. On herobrine I know that the > > regulator is a bit of a dummy (at least on herobrine), but I wonder if > > you should still get/enable it here? In the device tree bindings you > > listed it as not-optional so, in theory, you could use this to give an > > error if someone didn't provide the regulator. > > Won't the regulator framework introduce a dummy supply if there isn't a > supply in DT but this driver calls regulator_get()? Getting and enabling > it here will make this even more independent though so it sounds like a > good idea. That way we can make it a real regulator in the DTS as long > as the firmware isn't controlling it. I was thinking of regulator_get_optional(). You know the call you use when your regulator isn't "optional"? (Sorry, it always cracks me up that "optional" is exactly opposite the meaning for regulator compared to everyone else). > > BTW: it seems like it wouldn't be a _crazy_ amount of extra work to: > > > > 1. Add a sysfs hook for turning the regulator on/off > > > > 2. Change the Chrome OS userspace to actually use the sysfs hook if it's there. > > > > 3. Actually have the kernel in charge of turning the regulator off/on > > > > Doing this at the same time as the transition over to the more real > > "cros-ec-fp" would be nice so we don't have to figure out how to > > transition later. Said another way: If we don't transition now then I > > guess later we'd have to find some way to detect that the regulator > > specified in the kernel was actually a dummy and didn't really control > > the power? > > I'd rather not expose regulator control to userspace through some other > sysfs attribute. Instead I'd prefer the flashing logic that twiddles > gpios and power live all in the kernel and have userspace interact with > a character device to program the firmware. Yeah, that would be even better, you're right. Hmmm, so maybe the answer is to just delay adding the regulator until we're actually ready to specify it correctly and have the flashing happen in the kernel? -Doug
Quoting Doug Anderson (2022-03-18 15:06:59) > Hi, > > On Fri, Mar 18, 2022 at 3:01 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > @@ -703,6 +706,37 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev) > > > > ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val); > > > > if (!ret) > > > > ec_spi->end_of_msg_delay = val; > > > > + > > > > + if (!of_device_is_compatible(np, "google,cros-ec-fp")) > > > > + return 0; > > > > > > I noticed in your previous patch that you not only added a device-tree > > > match for this device but also a "spi_device_id". ...but won't you > > > fail to do all this important GPIO work in that case? > > > > I don't know when the spi_device_id path will be used. Never? I can > > simply drop it from the spi_device_id list for now and we can take up > > this problem later or never. > > That's fine with me. I was guessing it was relevant for x86 but my > experience with the way x86 does things is pretty minimal. > Ah, I think the x86 path also uses the of_device_id, but it is still google,cros-ec-spi so it won't be affected until conforming to the new binding. > > > > > + ec_spi->boot0 = devm_gpiod_get(dev, "boot0", 0); > > > > + if (IS_ERR(ec_spi->boot0)) > > > > + return PTR_ERR(ec_spi->boot0); > > > > > > Right now these GPIOs don't actually need to be stored in the "ec_spi" > > > structure. They could just be local variables. I guess you're trying > > > to future proof? > > > > Sure I will drop them because they're not useful later and I can save on > > the kernel-doc. > > > > > > > > > > > > + ec_spi->reset = devm_gpiod_get(dev, "reset", 0); > > > > + if (IS_ERR(ec_spi->reset)) > > > > + return PTR_ERR(ec_spi->reset); > > > > + > > > > + /* > > > > + * Take the FPMCU out of reset and wait for it to boot if it's in > > > > + * bootloader mode or held in reset. This isn't the normal flow because > > > > + * typically the BIOS has already powered on the device to avoid the > > > > + * multi-second delay waiting for the FPMCU to boot and be responsive. > > > > + */ > > > > + if (gpiod_get_value(ec_spi->boot0) || gpiod_get_value(ec_spi->reset)) { > > > > + /* Boot0 is sampled on reset deassertion */ > > > > + gpiod_set_value(ec_spi->boot0, 0); > > > > + gpiod_set_value(ec_spi->reset, 1); > > > > + usleep_range(1000, 2000); > > > > + gpiod_set_value(ec_spi->reset, 0); > > > > + > > > > + /* Wait for boot; there isn't a "boot done" signal */ > > > > + dev_info(dev, "Waiting for FPMCU to boot\n"); > > > > + msleep(2000); > > > > + } > > > > > > You added the regulator to the bindings. On herobrine I know that the > > > regulator is a bit of a dummy (at least on herobrine), but I wonder if > > > you should still get/enable it here? In the device tree bindings you > > > listed it as not-optional so, in theory, you could use this to give an > > > error if someone didn't provide the regulator. > > > > Won't the regulator framework introduce a dummy supply if there isn't a > > supply in DT but this driver calls regulator_get()? Getting and enabling > > it here will make this even more independent though so it sounds like a > > good idea. That way we can make it a real regulator in the DTS as long > > as the firmware isn't controlling it. > > I was thinking of regulator_get_optional(). You know the call you use > when your regulator isn't "optional"? (Sorry, it always cracks me up > that "optional" is exactly opposite the meaning for regulator compared > to everyone else). Oh my. > > > > > BTW: it seems like it wouldn't be a _crazy_ amount of extra work to: > > > > > > 1. Add a sysfs hook for turning the regulator on/off > > > > > > 2. Change the Chrome OS userspace to actually use the sysfs hook if it's there. > > > > > > 3. Actually have the kernel in charge of turning the regulator off/on > > > > > > Doing this at the same time as the transition over to the more real > > > "cros-ec-fp" would be nice so we don't have to figure out how to > > > transition later. Said another way: If we don't transition now then I > > > guess later we'd have to find some way to detect that the regulator > > > specified in the kernel was actually a dummy and didn't really control > > > the power? > > > > I'd rather not expose regulator control to userspace through some other > > sysfs attribute. Instead I'd prefer the flashing logic that twiddles > > gpios and power live all in the kernel and have userspace interact with > > a character device to program the firmware. > > Yeah, that would be even better, you're right. > > Hmmm, so maybe the answer is to just delay adding the regulator until > we're actually ready to specify it correctly and have the flashing > happen in the kernel? > I can enable it during probe just so that if the BIOS isn't doing it we'll have something that works assuming the DT is actually controlling the regulator. Or do nothing. It doesn't matter right now.
Quoting Stephen Boyd (2022-03-18 16:36:32) > Quoting Doug Anderson (2022-03-18 15:06:59) > > On Fri, Mar 18, 2022 at 3:01 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > I'd rather not expose regulator control to userspace through some other > > > sysfs attribute. Instead I'd prefer the flashing logic that twiddles > > > gpios and power live all in the kernel and have userspace interact with > > > a character device to program the firmware. > > > > Yeah, that would be even better, you're right. > > > > Hmmm, so maybe the answer is to just delay adding the regulator until > > we're actually ready to specify it correctly and have the flashing > > happen in the kernel? > > > > I can enable it during probe just so that if the BIOS isn't doing it > we'll have something that works assuming the DT is actually controlling > the regulator. Or do nothing. It doesn't matter right now. Thinking about it more there's no point in controlling the supply here until we support flashing logic in the kernel. Without flashing support in the kernel and without the BIOS turning on the power we can simply make the regulator always-on and boot-on with real gpio control and then it will be turned on during boot, emulating what the BIOS is doing. The power cycling after flashing the firmware doesn't seem to be necessary from my testing. And even then, the flashing script could unbind that regulator driver if it really needed to control the power.
diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c index d0f9496076d6..13d413a2fe46 100644 --- a/drivers/platform/chrome/cros_ec_spi.c +++ b/drivers/platform/chrome/cros_ec_spi.c @@ -4,6 +4,7 @@ // Copyright (C) 2012 Google, Inc #include <linux/delay.h> +#include <linux/gpio/consumer.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/of.h> @@ -77,6 +78,8 @@ struct cros_ec_spi { unsigned int start_of_msg_delay; unsigned int end_of_msg_delay; struct kthread_worker *high_pri_worker; + struct gpio_desc *boot0; + struct gpio_desc *reset; }; typedef int (*cros_ec_xfer_fn_t) (struct cros_ec_device *ec_dev, @@ -690,7 +693,7 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev, return cros_ec_xfer_high_pri(ec_dev, ec_msg, do_cros_ec_cmd_xfer_spi); } -static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev) +static int cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev) { struct device_node *np = dev->of_node; u32 val; @@ -703,6 +706,37 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev) ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val); if (!ret) ec_spi->end_of_msg_delay = val; + + if (!of_device_is_compatible(np, "google,cros-ec-fp")) + return 0; + + ec_spi->boot0 = devm_gpiod_get(dev, "boot0", 0); + if (IS_ERR(ec_spi->boot0)) + return PTR_ERR(ec_spi->boot0); + + ec_spi->reset = devm_gpiod_get(dev, "reset", 0); + if (IS_ERR(ec_spi->reset)) + return PTR_ERR(ec_spi->reset); + + /* + * Take the FPMCU out of reset and wait for it to boot if it's in + * bootloader mode or held in reset. This isn't the normal flow because + * typically the BIOS has already powered on the device to avoid the + * multi-second delay waiting for the FPMCU to boot and be responsive. + */ + if (gpiod_get_value(ec_spi->boot0) || gpiod_get_value(ec_spi->reset)) { + /* Boot0 is sampled on reset deassertion */ + gpiod_set_value(ec_spi->boot0, 0); + gpiod_set_value(ec_spi->reset, 1); + usleep_range(1000, 2000); + gpiod_set_value(ec_spi->reset, 0); + + /* Wait for boot; there isn't a "boot done" signal */ + dev_info(dev, "Waiting for FPMCU to boot\n"); + msleep(2000); + } + + return 0; } static void cros_ec_spi_high_pri_release(void *worker) @@ -754,8 +788,10 @@ static int cros_ec_spi_probe(struct spi_device *spi) if (!ec_dev) return -ENOMEM; - /* Check for any DT properties */ - cros_ec_spi_dt_probe(ec_spi, dev); + /* Check for any DT properties and boot FPMCU if applicable */ + err = cros_ec_spi_dt_probe(ec_spi, dev); + if (err) + return err; spi_set_drvdata(spi, ec_dev); ec_dev->dev = dev;