Re: [PATCH 1/2] i3c: Add support for HDR modes.

[vitor <vitor.soares@synopsys.com>]

On 22/02/19 15:02, Przemyslaw Gaj wrote:
>>>>  struct i3c_ibi_payload {
>>>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
>>>> index ada956a..fd50473 100644
>>>> --- a/include/linux/i3c/master.h
>>>> +++ b/include/linux/i3c/master.h
>>>> @@ -386,6 +386,10 @@ struct i3c_bus {
>>>>   *		  This method is mandatory.
>>>>   * @priv_xfers: do one or several private I3C SDR transfers
>>>>   *		This method is mandatory.
>>>> + * @send_hdr_cmds: send one or several HDR commands. If there is more than one
>>>> + *		   command, they should ideally be sent in the same HDR
>>>> + *		   transaction.
>>>> + *		   This method is optional.
>>>>   * @attach_i2c_dev: called every time an I2C device is attached to the bus.
>>>>   *		    This is a good place to attach master controller specific
>>>>   *		    data to I2C devices.
>>>> @@ -457,6 +461,9 @@ struct i3c_master_controller_ops {
>>>>  	int (*priv_xfers)(struct i3c_dev_desc *dev,
>>>>  			  struct i3c_priv_xfer *xfers,
>>>>  			  int nxfers);
>>>> +	int (*send_hdr_cmds)(struct i3c_dev_desc *dev,
>>>> +			     const struct i3c_hdr_cmd *cmds,
>>>> +			     int ncmds);
>>>>  	int (*attach_i2c_dev)(struct i2c_dev_desc *dev);
>>>>  	void (*detach_i2c_dev)(struct i2c_dev_desc *dev);
>>>>  	int (*i2c_xfers)(struct i2c_dev_desc *dev,  
>>> With this approach the controller between a start and stop can only transmit in SDR or HDR.
>>>
>>> This is limited for devices that need the following frame:
>>>     <Start><SDR xfer><Repeated Start><HDR command><Stop>
> Actually, there is repeated start between SDR and ENTHDR. There is no repeated
> start between ENTHDR and HDR commands, HDR command has to start immedetly after
> ENTHDR. Of course, hdr restart may occur between HDR commands.

Sorry, I meant all HDR frame.


>> If this is a use case we want to support, then we should probably have
>> something more generic than what we currently have.
>>
>> Something like
>>
>> enum i3c_xfer_type {
>> 	I3C_CCC_XFER,
>> 	I3C_SDR_XFER,
>> 	I3C_HDR_XFER,
>> }
>>
>> struct i3c_xfer {
>> 	enum i3c_xfer_type type;
>> 	union {
>> 		struct i3c_ccc_cmd ccc;
>> 		struct i3c_priv_xfer sdr;
>> 		struct i3c_hdr_cmd hdr;
>> 	};
>> }

@Boris: I remember you don't want to expose CCC commands, I think at least for now we shouldn't expose too.

Do you agree?

>> struct i3c_master_controller_ops {
>> 	...
>> 	int (*i3c_xfers)(struct i3c_dev_desc *dev,
>> 			 struct i3c_xfer *xfers,
>> 			 int nxfers);
>> 	...
>> };
> I agree. This is something we should think of.


Best regards,
Vitor Soares

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c