Re: [PATCH v1] ASoC: tas2783-sdw: Add basic BPT support

[Mark Brown]

> Add for BPT supported.

It might help to expand the acronym here, it's not at all obvious to me
what this is intended to do.

> +enum {
> +	SDW_MSG_FLAG_READ = 0,
> +	SDW_MSG_FLAG_WRITE,
> +};
> +
> +struct sdw_bpt_section {
> +	u32 addr;
> +	u32 len;
> +	u8 *buf;
> +};
> +
> +struct sdw_bpt_msg {
> +	struct sdw_bpt_section *sec;
> +	int sections;
> +	u8 dev_num;
> +	u8 flags;
> +};

These look like generic SoundWire definitons, they should come from
SoundWire headers.

> +static int tas2783_fw_download_bpt(struct tas2783_prv *tas_dev,
> +				   const struct firmware *fmw)
> +{

> +	ret = sdw_bpt_send_sync(bus, slave, &msg);

Can we call this while tas_fw_load() is waiting for the load to
complete?  That gets called from Soundwire callbacks.  There's some
Sashiko reports about the issue (and quite a bit of other stuff).

> +static bool tas2783_bpt_supported(struct tas2783_prv *tas_dev)
> +{
> +	struct sdw_slave *slave = tas_dev->sdw_peripheral;
> +	struct sdw_bus *bus = slave->bus;
> +
> +	return bus && bus->ops && bus->ops->bpt_send_async;

Does that guarantee that the bus actually has BPT, how strict is
Soundwire about not registering the same ops struct for everything and
returning an error if the hardware doesn't have the feature?

> @@ -761,7 +877,7 @@ static void tas2783_fw_ready(const struct firmware *fmw, void *context)
>  	if (!fmw || !fmw->data) {
>  		if (!tas_dev->fw_use_fallback) {
>  			tas_dev->fw_use_fallback = true;
> -			dev_info(tas_dev->dev,
> +			dev_dbg(tas_dev->dev,
>  				"Failed to read preferred fw binary: %s, attempting fallback binary load\n",
>  				tas_dev->rca_binaryname);

Separate patch please.

> @@ -1216,25 +1358,32 @@ static s32 tas_fw_load(struct tas2783_prv *tas_dev, struct sdw_slave *slave)
>  static s32 tas_io_init(struct device *dev, struct sdw_slave *slave)
>  {
>  	struct tas2783_prv *tas_dev = dev_get_drvdata(dev);
> +	uint val;
>  	s32 ret;
>  
>  	if (tas_dev->hw_init)
>  		return 0;
>  
> -	tas_dev->fw_dl_success = false;
> -
> -	ret = regmap_write(tas_dev->regmap, TAS2783_SW_RESET, 0x1);
> +	regmap_read(tas_dev->regmap, TASDEV_REG_SDW(0, 0, 7), &val);
> +	dev_dbg(dev, "Got B0P0R7 = %02X with %d.", val, ret);

We removed the assignment to ret but we're logging it?

-- 
Mark Brown <broonie@xxxxxxxxxx>