On 2/22/22 12:19 AM, Song Chen wrote:
Hi Greg,
在 2022/2/22 01:06, Greg KH 写道:
On Fri, Feb 11, 2022 at 08:02:27PM +0800, Song Chen wrote:
Introduce apply in pwm_ops to replace legacy operations, like enable, disable, config and set_polarity.
Signed-off-by: Song Chen chensong_2000@189.cn
. . .
diff --git a/include/linux/greybus/greybus_protocols.h b/include/linux/greybus/greybus_protocols.h index aeb8f9243545..81a6f16de098 100644 --- a/include/linux/greybus/greybus_protocols.h +++ b/include/linux/greybus/greybus_protocols.h @@ -812,8 +812,8 @@ struct gb_pwm_deactivate_request { struct gb_pwm_config_request { __u8 which; - __le32 duty; - __le32 period; + __u64 duty; + __u64 period; } __packed;
Did you just change a greybus protocol message that is sent over the wire? Why? And why drop the endian marking of it?
I discussed with Uwe about losing bit and found there is no perfect way to avoid, even in Uwe's patch[1], as a result, we decided to
The patch you reference [1] does not change the size of the duty cycle and period, it only ensures the value passed is no more than can be represented in an integer.
widen duty and period in gb_pwm_config_request, the other side of the wire is supposed to change accordingly to support 64bit duty and
This is what Greg meant about changing the over-the-wire protocol message format. You can't do that, or rather, if you do that, it is a *lot* more work than just changing it as you have done here.
If it is really required (and that's not clear, at least not at this time), then you need to modify the protocol version, and then make sure the versioning logic works correctly, both on the AP side and on the module side for all existing modules.
I would suggest that it is *not* required though, because the existing module code was built with the assumption that it would be provided a 32-bit unsigned value for its duty cycle and period. So as long as you make sure the AP side doesn't send anything nonsensical, they will continue to work as desired.
The correct fix in this case (assuming you don't want to change the protocol) is to ensure that whatever value is passed in to the pwm_ops->config callback (which is gb_pwm_config()) is adjusted to be within a range usable by the existing protocol. Since it's actually an unsigned value, you could double the range supported without changing the protocol if you wanted to do that.
period too(this will introduce compatibility problem and there is no variable like version to address it), similar with ktime_t in y2038, below is the detail [2]
And similar to addressing the Y2038 issue, it is not an easy thing to do.
endian is a problem, i shouldn't drop it.
This is absolutely correct. Did you attempt to compile this?
-Alex
BR
Song
Are you sure this is ok?
thanks,
greg k-h
-
Alex Elder