> > +static const struct reg_sequence rt1320_blind_write[] = {
> > + { 0xc003, 0xe0 },
> ...
> > + { 0xd486, 0xc3 },
> > +};
>
> I would add a comment that the 'blind writes' is an SDCA term to deal with
> platform-specific initialization, but in this case it seems that all the addresses
> targeted are in the vendor-specific space
Sure, will add a comment to describe what blind writes means.
> > +static const struct reg_sequence rt1320_patch_code_write[] = {
> > + { 0x10007000, 0x37 },
> ...
> > + { 0x0000d540, 0x01 },
> > +};
>
> I would add a comment on the targeted register space. It seems to be the
> SDCA function 1, except for the last one?
>
> Also possibly move the tables to a different file to make the driver code easier
> to get to (less scrolling required).
The driver will add a comment for the patch_code_write.
The setting of the last one notifies the patch code and blind writes are done.
That is the handshake mechanism we shall set.
> > +static int rt1320_read_prop(struct sdw_slave *slave) {
> > + struct sdw_slave_prop *prop = &slave->prop;
> > + int nval;
> > + int i, j;
> > + u32 bit;
> > + unsigned long addr;
> > + struct sdw_dpn_prop *dpn;
> > +
> > + sdw_slave_read_prop(slave);
>
> add a comment that this is needed to fetch the lane information from platform
> firmware.
OK, will do.
> > +static const struct sdw_device_id rt1320_id[] = {
> > + SDW_SLAVE_ENTRY_EXT(0x025d, 0x1320, 0x3, 0x0, 0),\
>
> missing class ID 1?
Will add class ID 1.
> > +
> > +/* Function_Status */
> > +#define FUNCTION_NEEDS_INITIALIZATION BIT(5)
> > +#define FUNCTION_HAS_BEEN_RESET BIT(6)
>
> You seem to use only NEEDS_INITIALIZATION, what happens on a RESET?
The RESET should influence the SDCA control.
The blind writes doesn't set the SDCA control. Therefore, the driver doesn't check RESET flag.
> > +#define FUNCTION_BUSY BIT(7)
> not used, can it be asserted?
Will remove it.
> Looks mostly good otherwise, thanks for the patch.
