On Sun, Mar 23, 2014 at 07:32:34PM +0000, Jonathan Cameron wrote:
> On 20/03/14 23:01, Mattia Dongili wrote:
> >From: Javier Achirica <jachirica@xxxxxxxxx>
> >
> >vaios come with two different type of ACPI based ALS controls. The
...
Hi Jonathan,
thanks for the extensive review.
I can answer some of your questions, those related to the taos driver in
particular need to wait for Javier as large part of the work was plain
reverse engineering the windows code that he did.
> I hate to say it but my first reaction to this was, 'What are so
> many functions doing in one file in the first place?'
>
> The embedded world has been busy splitting up and reparcelling large
> files like this so the various devices end up within the correct
> subsystems. I won't even go into all the reasons for this as they
> are listed everywhere else? Is this just a case of modifying old
> code no one wants to touch on a larger scale or is there actually
> resistance to breaking new functionality out of this file and
> into nice clean small chunks?
I don't think there is any resistance per-se and to some extent it has
been done in the past (see meye.ko).
On the other hand there is no documentation on the devices this driver
is about and any abstraction you put in place to generalize access and
to allow splitting functionalities, will break. I have to admit that
things have somehow settled recently (and the Vaio brand has just been
buried, we'll see what happens) so making an attempt may make sense
now.
But then again, I'm not sure that making a mini-subsystem out of an
undocumented device/interface is going to pay out.
> So my first request is can we at least split this new functionality
> out?
we can certainly try. :)
Though some functionalities are so interdependent from each other that
I'm not sure it makes much sense (backlight Fn-keys, panel backlight and
als are the worst).
> As I look through what we have here, it looks like we ought to have
> this functionality alone split into the ALS sensor and a backlight
> driver (which can by all means use the standard interfaces to
> talk to the ALS)... (event support for in kernel users isn't there
> yet, but this might motivate us to hurry up with it).
>
> The bulk of this driver is actually just a set of nasty access routines
> and then functionality that already exists in the existing tsl2563 driver.
> Hence, unless there are very good reasons why not I would suggest the
> following.
>
> 1) Convert the existing tsl2563 driver over to using regmap.
> 2) Add regmap support for the access routines we have here then add the
> little bit of code needed to make the tsl2563 register appopriately. This
> may require a small amount of magic mfd like code in here.
> (plan b on this is to perhaps create an i2c bus driver using the sony
> specific calls).
I need to figure out what regmaps are about (a quick look in
include/linux/regmap.h reveals a lot), this is going to be a piece of
work.
> 3) Figure out what extra bits are that are needed by the ALS driver we have
> here and add them on (kelvin support and perhaps a few other bits).
> 4) Then and only then take on any non ALS elements in here (such as the
> back light).
most of the backlight code here is really a workaround to a sick
behaviour that vaios have once you enable the als device. It's generally
unrelated to the als portion of the driver except that once you
introduce the latter, the former will break. The code is folded together
to avoid being broken at any stage but it can be split to different
changes if it makes more sense.
> The other als driver (one where all the clever stuff is hidden) should ideally
> be a completely separate als driver (be it a very simple one.
>
> Anyhow, your code is reasonably clean but I think some of these questions about
> the fundamental approach need addressing to avoid getting ourselves into a
> nasty mess in the long term. Sorry if this review seems a little negative.
it does indeed but I wasn't expecting this enourmous blob to go through
unnoticed and this is the type of feedback I wanted to get this code
unstuck and finally ready for inclusion.
I'll go through your comments in a few iterations to clean up the
obvious first and evaluate the larger rework (i.e. merging with the
current taos driver and splitting the als portion of sony-laptop) with
more stable code at hand.
Most of the questions you have about the driver itself will need to be
answered by Javier though, I have only a few comments below about
generic code in sony-laptop.c.
> I appreciate that you've only taken the approach taken throughout this file!
>
> Just as an aside, is there any interest in cleaning this whole file up?
> The sonypi device looks like it should registering a whole host of standard
> interfaces...
iirc, sonypi doesn't have enough known functionality to cover the
standard interfaces, it largely predates most of them. It's quite
some time I don't look into it and it's actually a legacy device that
is likely better be deprecated than worked on. If anybody wants to take
it on, I'm not opposed though.
> Jonathan
> >Cc: Jonathan Cameron <jic23@xxxxxxxxxx>
> >Cc: linux-iio@xxxxxxxxxxxxxxx
> >Cc: Jingoo Han <jg1.han@xxxxxxxxxxx>
> >Cc: Marco Chiappero <marco@xxxxxxxxxx>
> >Signed-off-by: Javier Achirica <jachirica@xxxxxxxxx>
> >Signed-off-by: Mattia Dongili <malattia@xxxxxxxx>
> >---
> > Documentation/laptops/sony-laptop.txt | 31 +
> > drivers/platform/x86/Kconfig | 1 +
> > drivers/platform/x86/sony-laptop.c | 1111 ++++++++++++++++++++++++++++++++-
> > 3 files changed, 1128 insertions(+), 15 deletions(-)
> >
> >diff --git a/Documentation/laptops/sony-laptop.txt b/Documentation/laptops/sony-laptop.txt
...
> >diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
...
> >diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
...
[large chop, I'll let Javier comment on those questions]
> >@@ -1065,20 +1206,43 @@ static int sony_nc_get_brightness_ng(struct backlight_device *bd)
> > return (result & 0xff) - sdev->offset;
> > }
> >
> >-static int sony_nc_update_status_ng(struct backlight_device *bd)
> >+static int __sony_nc_set_brightness_ng(struct sony_backlight_props *bl,
> >+ int brightness)
> > {
> >- int value, result;
> >- struct sony_backlight_props *sdev =
> >- (struct sony_backlight_props *)bl_get_data(bd);
> >+ int result = 0;
> >+ int value = brightness + bl->offset;
> >
> >- value = bd->props.brightness + sdev->offset;
> >- if (sony_call_snc_handle(sdev->handle, sdev->cmd_base | (value << 0x10),
> >+ if (sony_call_snc_handle(bl->handle, bl->cmd_base | (value << 0x10),
> > &result))
> > return -EIO;
> >
> > return value;
> > }
> Whilst this stuff is presumably being driver from the ALS it is decidedly
> back light specific. Perhaps I'm idealistic in thinking it should be
> a different driver.
err, this hunk should not have made it here at all. Part of the other
backlight work is in this patch for the reason I explained above but
this one is really extraneous.
> >
> >+
> >+static int sony_nc_update_status_ng(struct backlight_device *bd)
> >+{
> >+ struct sony_backlight_props *sdev =
> >+ (struct sony_backlight_props *)bl_get_data(bd);
> >+
> >+ return __sony_nc_set_brightness_ng(sdev, bd->props.brightness);
> >+}
> >+
> >+static void sony_nc_brightness_changed_ng(struct backlight_device *bd,
> >+ int brightness, int max_brightness)
> >+{
> >+ int new_brightness =
> >+ brightness * bd->props.max_brightness / max_brightness;
> >+ struct sony_backlight_props *sdev =
> >+ (struct sony_backlight_props *)bl_get_data(bd);
> >+
> >+ dprintk("brightness changed cur: %d, max: %d -> new: %d\n", brightness,
> >+ max_brightness, new_brightness);
> >+
> >+ __sony_nc_set_brightness_ng(sdev, new_brightness);
> >+ backlight_force_update(bd, BACKLIGHT_UPDATE_HOTKEY);
> >+}
> >+
> > static const struct backlight_ops sony_backlight_ops = {
> > .options = BL_CORE_SUSPENDRESUME,
> > .update_status = sony_backlight_update_status,
> >@@ -1088,6 +1252,7 @@ static const struct backlight_ops sony_backlight_ng_ops = {
> > .options = BL_CORE_SUSPENDRESUME,
> > .update_status = sony_nc_update_status_ng,
> > .get_brightness = sony_nc_get_brightness_ng,
> >+ .brightness_changed = &sony_nc_brightness_changed_ng,
> > };
> >
> > /*
> >@@ -1201,7 +1366,8 @@ static int sony_nc_hotkeys_decode(u32 event, unsigned int handle)
> > enum event_types {
> > HOTKEY = 1,
> > KILLSWITCH,
> >- GFX_SWITCH
> >+ GFX_SWITCH,
> >+ ALS
> > };
> Hmm. I don't suppose suggesting this should be an mfd with a nice clean
> event hook up interface will go down to well?
ah, a multi function device. Again I'd need to look how it would look
like adding that to sony-laptop but these are essentially for triggering
acpi events.
Would it make it that much more clean than decoding the event from the
GPE notification callback and sending it via ACPI's netlink interface
(which is all we are doing here)?
> > static void sony_nc_notify(struct acpi_device *device, u32 event)
> > {
> >@@ -1276,6 +1442,38 @@ static void sony_nc_notify(struct acpi_device *device, u32 event)
> > ev_type = GFX_SWITCH;
> > real_ev = __sony_nc_gfx_switch_status_get();
> > break;
> >+
> >+ case 0x0143:
> >+ case 0x014b:
> >+ case 0x014c:
> >+ case 0x0163:
> Could you document what these different events are here? Would make
> it easier to review.
yes, I can add documentation but this is not als specific.
In general SNC exposes a number of "handles" (those you see above) in an
indexed table (see find_snc_handle). All event notifications and calls
into the device methods (SN06 and SN07 are the entry points) specify the
offset at which the desired handle is.
Each handle cover one or more functionality based on the arguments provided.
Different handle may cover similar functionalities and are different
from model to model.
The functionalities are easier to see in sony_nc_function_setup where
each handle has it's corresponding setup function called.
The event decoding here (sony_nc_notify) have to look at what handle is
associated to it and decide accordingly (map a hotkey, read a switch
position, read additional "light" event information).
...
> You are pushing events without any direct control interface for them...
> >+ iio_push_event(als_handle->indio_dev,
> >+ IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
> >+ 0,
> >+ IIO_EV_TYPE_THRESH,
> >+ IIO_EV_DIR_EITHER),
> >+ iio_get_time_ns());
...
> Again, you are pushing without there being any sysfs elements to control them..
> With those missing there is no way for userspace to know that the device
> might spit out events, or what they might be...
>
> >+ iio_push_event(als_handle->indio_dev,
> >+ IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
> >+ 0,
> >+ IIO_EV_TYPE_THRESH,
> >+ IIO_EV_DIR_EITHER),
> >+ iio_get_time_ns());
...
> >+static const struct iio_event_spec sony_events[] = {
> >+ {
> >+ .type = IIO_EV_TYPE_THRESH,
> >+ .dir = IIO_EV_DIR_EITHER
> Would expect some control stuff in here as well...
> >+ }
what do you mean? do you have an example I can look at?
Thanks
--
mattia
:wq!
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
