lkml.org 
[lkml]   [2005]   [Apr]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] ds1337 4/4
Jean Delvare wrote:

>>Based on your and Jean's input, following so far sounds reasonable:
>>Create "charge" sysfs entry for ds1339 when it is detected. Do not
>>write any value to Trickle Charge register, until its value is written
>>to this entry.
>
> While I admit I had this in mind in the first place, the more I think of
> it and the less I like it. It's slightly better than changing the
> charging rate right when loading the driver, but that's still dangerous.
> Users could write a value which doesn't match the hardware design, and
> bad things could happen.

I had assumed Ladislav wanted to be able to change this charge rate at
any time, which was the motivation behind adding ds1339 support.

>>How are you using this driver? There is non-static function
>>ds1337_do_command which expects id. How do you know which id belongs
>>to which chip?
>
> I second this question, as it stroke me too. This function doesn't sound
> exactly usable to me. Identifying the device by bus and address would
> make more sense than an arbitrary id you have no way to learn about.

It is used by the Radstone ppc7d platform, arch/ppc/radstone_ppc7d.c
but wasn't added until very recently (2.6.12-rc2 I think).

To be honest, I meant to remove the 'id' thing before submitting the
driver. There's no need to support more than one of these devices.

>>Do you actually have machine with more than one ds1337?
>>Chip has fixed address, so only one can hang on one bus (am I right?)
>
> You are.

Yep. I think the id should be removed asap.

> Back to the issue, some random thoughts summarizing my opinion:
>
> 1* Initializing the battery charge register is a firmware/bios issue, as
> you underlined earlier. It would make sense (and would be easier) to
> just ignore it at the driver level.

Initializing the charge register should be done by the bios if possible.
However, I assume Ladislav still wants to be able to change the register
at runtime so some kernel support is needed?

> 2* If it makes sense to stop the charge, then we should provide a simple
> *switch* to the user, from the default charging register value (as
> previously set by the firmware/bios) to 0 and back. The switch would
> probably be a sysfs file unless a different API already exists.
>
> 3* Having the driver write an arbitrary non-0 value to the register
> should not be done unless the system has been identified. I have no idea
> how your system can be identified (DMI?), but if it can't, then I'd
> better see the register ignored altogether.
>
> 4* Remember that you can always write a simple C tool relying on the
> i2c-dev interface to do the job. The advantage of this approach is that
> you can put big fat warnings and request user confirmation before any
> action.

This makes sense. Ladislav, would this work for you? I guess we'd still
add code to the ds1337 driver to detect ds1339 in order to ensure that
this tool could not modify register 0 of a ds1337 by accident?

/james


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-04-12 21:41    [W:1.133 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site