Re: [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device

2015-01-21 Thread Javier Martinez Canillas
Hello Lee,

On 01/20/2015 06:11 PM, Javier Martinez Canillas wrote:
 
 But is it really a chardev?  Don't chardevs usually live in
 drivers/char?  It probably uses a chardev node in /dev, but what does
 it really do?  What information can/will userspace obtain from this
 memory block?
 
 
 Right, is a driver that register a chardev but mostly to expose an ioctl
 interface to send commands to the Embedded Controller from user-space.
 
 The Application Processor communicates with Embedded Controller by sending
 commands over an interface. This can be either spi or i2c on ARM (depending
 on the Chromebook model) or LPC on x86 Chromebooks so the platform driver
 instantiated by the cros-ec-dev mfd cell is to allow user-space to send
 commands to the Embedded Controller (using the correct transport method).
 
 So this chardev is used by the ectool binary in ChromeOS to communicate
 with the Embedded Controller.
 

Just FYI, I'll rename it to cros-ec-ctl since as you said is not really
a chardev but that just happens to be the interface chosen to send the ioctl
commands to the driver.

Thanks a lot for your suggestion.

Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device

2015-01-20 Thread Lee Jones
On Fri, 02 Jan 2015, Javier Martinez Canillas wrote:

 The ChromeOS EC character device is an user-space interface to
 allow applications to access the Embedded Controller.
 
 Add a cell for this device so it's spawned from the mfd driver.
 
 Signed-off-by: Javier Martinez Canillas javier.marti...@collabora.co.uk
 ---
 
 Changes since v1: None, new patch.
 
  drivers/mfd/cros_ec.c | 4 
  1 file changed, 4 insertions(+)
 
 diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
 index c872e1b..70f9ed5 100644
 --- a/drivers/mfd/cros_ec.c
 +++ b/drivers/mfd/cros_ec.c
 @@ -118,6 +118,10 @@ static const struct mfd_cell cros_devs[] = {
   .id = 2,
   .of_compatible = google,cros-ec-i2c-tunnel,
   },
 + {
 + .name = cros-ec-dev,

*-dev is seldom suitable for naming things.

Please be more imaginative/descriptive in your naming.

 + .id = 3,
 + },
  };
  
  int cros_ec_register(struct cros_ec_device *ec_dev)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device

2015-01-20 Thread Javier Martinez Canillas
Hello Lee,

On 01/20/2015 05:55 PM, Lee Jones wrote:
 On Tue, 20 Jan 2015, Javier Martinez Canillas wrote:
 
 Hello Lee,
 
 On 01/20/2015 05:29 PM, Lee Jones wrote:
  
  It's not a blocker, but it is a ridiculous name to use inside the
  driver/ directory 'cos almost everything is a dev(ice) here.
  
 
 Right, do you think that cros-ec-chardev will be a more suitable
 name? Sorry, I'm really bad at naming things...
 
 But is it really a chardev?  Don't chardevs usually live in
 drivers/char?  It probably uses a chardev node in /dev, but what does
 it really do?  What information can/will userspace obtain from this
 memory block?
 

Right, is a driver that register a chardev but mostly to expose an ioctl
interface to send commands to the Embedded Controller from user-space.

The Application Processor communicates with Embedded Controller by sending
commands over an interface. This can be either spi or i2c on ARM (depending
on the Chromebook model) or LPC on x86 Chromebooks so the platform driver
instantiated by the cros-ec-dev mfd cell is to allow user-space to send
commands to the Embedded Controller (using the correct transport method).

So this chardev is used by the ectool binary in ChromeOS to communicate
with the Embedded Controller.

The actual driver is located in drivers/platform/chrome/cros_ec_dev.c and
is added by platform/chrome: Add Chrome OS EC userspace device interface
[0] in this series.

In the downstream ChromiumOS kernel tree it is located in drivers/mfd but
I moved to drivers/platform/chrome/ since you asked me to find a better
place for it.

Best regards,
Javier

[0]: https://lkml.org/lkml/2015/1/2/81
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device

2015-01-20 Thread Lee Jones
On Tue, 20 Jan 2015, Javier Martinez Canillas wrote:

 Hello Lee,
 
 On 01/20/2015 05:29 PM, Lee Jones wrote:
  
  It's not a blocker, but it is a ridiculous name to use inside the
  driver/ directory 'cos almost everything is a dev(ice) here.
  
 
 Right, do you think that cros-ec-chardev will be a more suitable
 name? Sorry, I'm really bad at naming things...

But is it really a chardev?  Don't chardevs usually live in
drivers/char?  It probably uses a chardev node in /dev, but what does
it really do?  What information can/will userspace obtain from this
memory block?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device

2015-01-20 Thread Javier Martinez Canillas
Hello Lee,

Thanks a lot for your feedback.

On 01/20/2015 09:20 AM, Lee Jones wrote:
 On Fri, 02 Jan 2015, Javier Martinez Canillas wrote:
 
 The ChromeOS EC character device is an user-space interface to
 allow applications to access the Embedded Controller.
 
 Add a cell for this device so it's spawned from the mfd driver.
 
 Signed-off-by: Javier Martinez Canillas javier.marti...@collabora.co.uk
 ---
 
 Changes since v1: None, new patch.
 
  drivers/mfd/cros_ec.c | 4 
  1 file changed, 4 insertions(+)
 
 diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
 index c872e1b..70f9ed5 100644
 --- a/drivers/mfd/cros_ec.c
 +++ b/drivers/mfd/cros_ec.c
 @@ -118,6 +118,10 @@ static const struct mfd_cell cros_devs[] = {
  .id = 2,
  .of_compatible = google,cros-ec-i2c-tunnel,
  },
 +{
 +.name = cros-ec-dev,
 
 *-dev is seldom suitable for naming things.
 
 Please be more imaginative/descriptive in your naming.


You know this is one of the two hard problems in CS ;-)

But seriously, this is the name used in the downstream ChromiumsOS kernel
and tbh I don't think is that bad since after all is a character device
interface to access the Embedded Controller from user-space. So in that
sense is not worse than the i2c-dev or spidev names but I can try to come
up with something more creative if that will block the patch to be merged.

Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device

2015-01-20 Thread Javier Martinez Canillas
Hello Lee,

On 01/20/2015 05:29 PM, Lee Jones wrote:
 
 It's not a blocker, but it is a ridiculous name to use inside the
 driver/ directory 'cos almost everything is a dev(ice) here.
 

Right, do you think that cros-ec-chardev will be a more suitable
name? Sorry, I'm really bad at naming things...

Best regards,
Javier
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device

2015-01-20 Thread Lee Jones
On Tue, 20 Jan 2015, Javier Martinez Canillas wrote:

 Hello Lee,
 
 Thanks a lot for your feedback.
 
 On 01/20/2015 09:20 AM, Lee Jones wrote:
  On Fri, 02 Jan 2015, Javier Martinez Canillas wrote:
  
  The ChromeOS EC character device is an user-space interface to
  allow applications to access the Embedded Controller.
  
  Add a cell for this device so it's spawned from the mfd driver.
  
  Signed-off-by: Javier Martinez Canillas javier.marti...@collabora.co.uk
  ---
  
  Changes since v1: None, new patch.
  
   drivers/mfd/cros_ec.c | 4 
   1 file changed, 4 insertions(+)
  
  diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
  index c872e1b..70f9ed5 100644
  --- a/drivers/mfd/cros_ec.c
  +++ b/drivers/mfd/cros_ec.c
  @@ -118,6 +118,10 @@ static const struct mfd_cell cros_devs[] = {
 .id = 2,
 .of_compatible = google,cros-ec-i2c-tunnel,
 },
  +  {
  +  .name = cros-ec-dev,
  
  *-dev is seldom suitable for naming things.
  
  Please be more imaginative/descriptive in your naming.
 
 
 You know this is one of the two hard problems in CS ;-)
 
 But seriously, this is the name used in the downstream ChromiumsOS kernel
 and tbh I don't think is that bad since after all is a character device
 interface to access the Embedded Controller from user-space. So in that
 sense is not worse than the i2c-dev or spidev names but I can try to come
 up with something more creative if that will block the patch to be merged.

It's not a blocker, but it is a ridiculous name to use inside the
driver/ directory 'cos almost everything is a dev(ice) here.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html